drivers/qcom: add RPMH communication support

Add controller driver for QCOM SoCs that have hardware based shared resource management. The hardware IP known as RSC (Resource State Coordinator) houses multiple Direct Resource Voter (DRV) for different execution levels. A DRV is a unique voter on the state of a shared resource. A Trigger Control Set (TCS) is a bunch of slots that can house multiple resource state requests, that when triggered will issue those requests through an internal bus to the Resource Power Manager Hardened (RPMH) blocks. These hardware blocks are capable of adjusting clocks, voltages etc. The resource state request from a DRV are aggregated along with state requests from other processors in the SoC and the aggregate value is applied on the resource.

Some important aspects of the RPMH communication -
- Requests are <addr, value> with some header information
- Multiple requests (upto 16) may be sent through a TCS, at a time
- Requests in a TCS are sent in sequence
- Requests may be fire-n-forget or completion (response expected)
- Multiple TCS from the same DRV may be triggered simultaneously
- Cannot send a request if another reques for the same addr is in progress from the same DRV
- When all the requests from a TCS are complete, an IRQ is raised
- The IRQ handler needs to clear the TCS before it is available for reuse
- TCS configuration is specific to a DRV
- Platform drivers may use DRV from different RSCs to make requests

Resource state requests made when CPUs are active are called 'active' state requests. Requests made when all the CPUs are powered down (idle state) are called 'sleep' state requests. They are matched by a corresponding 'wake' state requests which puts the resources back in to previously requested active state before resuming any CPU. TCSes are dedicated for each type of requests. Control TCS are used to provide specific information to the controller.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/Kconfig                |  10 +
 drivers/soc/qcom/Makefile               |   1 +
 drivers/soc/qcom/rpmh-internal.h        |  86 +++++
 drivers/soc/qcom/rpmh-rsc.c             | 593 ++++++++++++++++++++++++++++++++
 include/dt-bindings/soc/qcom,rpmh-rsc.h |  14 +
 include/soc/qcom/tcs.h                  |  56 +++
 6 files changed, 760 insertions(+)
 create mode 100644 drivers/soc/qcom/rpmh-internal.h
 create mode 100644 drivers/soc/qcom/rpmh-rsc.c
 create mode 100644 include/dt-bindings/soc/qcom,rpmh-rsc.h
 create mode 100644 include/soc/qcom/tcs.h

drivers/soc/qcom/rpmh-internal.h new file +10 -0

new file mode 100644
index 000000000000..dccf9de1a75f
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+
+#ifndef __RPM_INTERNAL_H__
+#define __RPM_INTERNAL_H__
+
+#include <soc/qcom/tcs.h>

drivers/soc/qcom/rpmh-internal.h new file +8 -0

+ *
+ * @drv: the controller
+ * @msg: the request for this response
+ * @m: the tcs identifier
+ * @err: error reported in the response
+ * @list: link list object.
+ */
+struct tcs_response {

drivers/soc/qcom/rpmh-internal.h new file +11 -0

+ * @type: type of the TCS in this group - active, sleep, wake
+ * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
+ * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
+ * @num_tcs: number of TCSes in this type
+ * @ncpt: number of commands in each TCS
+ * @tcs_lock: lock for synchronizing this TCS writes
+ * @responses: response objects for requests sent from each TCS
+ */
+struct tcs_group {
+       struct rsc_drv *drv;
+       int type;

drivers/soc/qcom/rpmh-internal.h new file +27 -0

+ *
+ * @name: controller identifier
+ * @tcs_base: start address of the TCS registers in this controller
+ * @drv_id: instance id in the controller (DRV)
+ * @num_tcs: number of TCSes in this DRV
+ * @tasklet: handle responses, off-load work from IRQ handler
+ * @response_pending: list of responses that needs to be sent to caller
+ * @tcs: TCS groups
+ * @tcs_in_use: s/w state of the TCS
+ * @drv_lock: synchronize state of the controller
+ */
+struct rsc_drv {
+       const char *name;
+       void __iomem *tcs_base;
+       int drv_id;
+       int num_tcs;
+       struct tasklet_struct tasklet;
+       struct list_head response_pending;
+       struct tcs_group tcs[TCS_TYPE_NR];
+       atomic_t tcs_in_use[MAX_TCS_NR];
+       spinlock_t drv_lock;
+};
+
+
+int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg);
+
+#endif /* __RPM_INTERNAL_H__ */

drivers/soc/qcom/rpmh-rsc.c new file +1 -0

new file mode 100644
index 000000000000..ceb8aea237d6
@@ -0,0 +1,593 @@
+// SPDX-License-Identifier: GPL-2.0

drivers/soc/qcom/rpmh-rsc.c new file +70 -0

+
+static struct tcs_group *get_tcs_from_index(struct rsc_drv *drv, int m)
+{
+       struct tcs_group *tcs = NULL;
+       int i;
+
+       for (i = 0; i < drv->num_tcs; i++) {
+               tcs = &drv->tcs[i];
+               if (tcs->tcs_mask & BIT(m))
+                       break;
+       }
+
+       if (i == drv->num_tcs) {
+               WARN(1, "Incorrect TCS index %d", m);
+               tcs = NULL;
+       }
+
+       return tcs;
+}
+
+static struct tcs_response *setup_response(struct rsc_drv *drv,
+                                         struct tcs_request *msg, int m)
+{
+       struct tcs_response *resp;
+       struct tcs_group *tcs;
+
+       resp = kcalloc(1, sizeof(*resp), GFP_ATOMIC);
+       if (!resp)
+               return ERR_PTR(-ENOMEM);
+
+       resp->drv = drv;
+       resp->msg = msg;
+       resp->err = 0;
+
+       tcs = get_tcs_from_index(drv, m);
+       if (!tcs)
+               return ERR_PTR(-EINVAL);
+
+       /*
+        * We should have been called from a TCS-type locked context, and
+        * we overwrite the previously saved response.
+        */
+       tcs->responses[m - tcs->tcs_offset] = resp;
+
+       return resp;
+}
+
+static void free_response(struct tcs_response *resp)
+{
+       kfree(resp);
+}
+
+static struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
+{
+       struct tcs_group *tcs = get_tcs_from_index(drv, m);
+
+       return tcs->responses[m - tcs->tcs_offset];
+}
+
+static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int m, int n)
+{
+       return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * m +
+                           RSC_DRV_CMD_OFFSET * n);
+}
+
+static void write_tcs_reg(struct rsc_drv *drv, int reg, int m, int n,
+                               u32 data)
+{
+       writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * m +
+                     RSC_DRV_CMD_OFFSET * n);

drivers/soc/qcom/rpmh-rsc.c new file +42 -0

+       }
+}
+
+static bool tcs_is_free(struct rsc_drv *drv, int m)
+{
+       return !atomic_read(&drv->tcs_in_use[m]) &&
+              read_tcs_reg(drv, RSC_DRV_STATUS, m, 0);
+}
+
+static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+       int i;
+       struct tcs_group *tcs;
+
+       for (i = 0; i < TCS_TYPE_NR; i++) {
+               if (type == drv->tcs[i].type)
+                       break;
+       }
+
+       if (i == TCS_TYPE_NR)
+               return ERR_PTR(-EINVAL);
+
+       tcs = &drv->tcs[i];
+       if (!tcs->num_tcs)
+               return ERR_PTR(-EINVAL);
+
+       return tcs;
+}
+
+static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
+                                       struct tcs_request *msg)
+{
+       int type;
+
+       switch (msg->state) {
+       case RPMH_ACTIVE_ONLY_STATE:
+               type = ACTIVE_TCS;
+               break;
+       default:
+               return ERR_PTR(-EINVAL);
+       }
+

drivers/soc/qcom/rpmh-rsc.c new file +20 -0

+
+       spin_lock_irqsave(&drv->drv_lock, flags);
+       INIT_LIST_HEAD(&resp->list);
+       list_add_tail(&resp->list, &drv->response_pending);
+       spin_unlock_irqrestore(&drv->drv_lock, flags);
+
+       tasklet_schedule(&drv->tasklet);
+}
+
+/**
+ * tcs_irq_handler: TX Done interrupt handler
+ */
+static irqreturn_t tcs_irq_handler(int irq, void *p)
+{
+       struct rsc_drv *drv = p;
+       int m, i;
+       u32 irq_status, sts;
+       struct tcs_response *resp;
+       struct tcs_cmd *cmd;
+       int err;

drivers/soc/qcom/rpmh-rsc.c new file +16 -0

+                               break;
+                       }
+               }
+
+               resp->err = err;
+skip_resp:
+               /* Reclaim the TCS */
+               write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
+               write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
+               atomic_set(&drv->tcs_in_use[m], 0);
+               send_tcs_response(resp);
+       }
+
+       return IRQ_HANDLED;
+}
+

include/soc/qcom/tcs.h new file +41 -0

new file mode 100644
index 000000000000..9465f0560f7a
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SOC_QCOM_TCS_H__
+#define __SOC_QCOM_TCS_H__
+
+#define MAX_RPMH_PAYLOAD       16
+
+/**
+ * rpmh_state: state for the request
+ *
+ * RPMH_WAKE_ONLY_STATE:   Resume resource state to the value previously
+ *                         requested before the processor was powered down.
+ * RPMH_SLEEP_STATE:       State of the resource when the processor subsystem
+ *                         is powered down. There is no client using the
+ *                         resource actively.
+ * RPMH_ACTIVE_ONLY_STATE: Active or AMC mode requests. Resource state
+ *                         is aggregated immediately.
+ */
+enum rpmh_state {
+       RPMH_SLEEP_STATE,
+       RPMH_WAKE_ONLY_STATE,
+       RPMH_ACTIVE_ONLY_STATE,
+};
+
+/**
+ * tcs_cmd: an individual request to RPMH.
+ *
+ * @addr:     the address of the resource slv_id:18:16 | offset:0:15
+ * @data:     the resource state request
+ * @complete: wait for this request to be complete before sending the next
+ */
+struct tcs_cmd {
+       u32 addr;
+       u32 data;
+       bool complete;
+};
+
+/**

Hi Lina,

drivers/soc/qcom/rpmh-internal.h line 20

Quoted 3 lines · Lina Iyer

drivers/soc/qcom/rpmh-internal.h new file +3 -0

+
+/**
+ * tcs_response: Response object for a request

Can you embed the acronym definition, ie: tcs_response: Responses for a Trigger Command Set.

Quoted 12 lines

+ *
+ * @drv: the controller
+ * @msg: the request for this response
+ * @m: the tcs identifier
+ * @err: error reported in the response
+ * @list: link list object.
+ */

drivers/soc/qcom/rpmh-internal.h new file +5 -0

+struct tcs_response {
+       struct rsc_drv *drv;
+       struct tcs_request *msg;
+       u32 m;
+       int err;

drivers/soc/qcom/rpmh-internal.h line 38

Quoted 6 lines

drivers/soc/qcom/rpmh-internal.h new file +6 -0

+       struct list_head list;
+};
+
+/**
+ * tcs_group: group of TCSes for a request state
+ *

Document @drv.

Quoted 15 lines

+ * @type: type of the TCS in this group - active, sleep, wake
+ * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
+ * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
+ * @num_tcs: number of TCSes in this type
+ * @ncpt: number of commands in each TCS
+ * @tcs_lock: lock for synchronizing this TCS writes
+ * @responses: response objects for requests sent from each TCS
+ */

drivers/soc/qcom/rpmh-internal.h new file +7 -0

+struct tcs_group {
+       struct rsc_drv *drv;
+       int type;
+       u32 tcs_mask;
+       u32 tcs_offset;
+       int num_tcs;
+       int ncpt;

drivers/soc/qcom/rpmh-internal.h line 59

Quoted 6 lines

drivers/soc/qcom/rpmh-internal.h new file +6 -0

+       spinlock_t tcs_lock;
+       struct tcs_response *responses[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * rsc_drv: the RSC controller

Would be more helpfully described as Resource State Coordinator controller.

drivers/soc/qcom/rpmh-rsc.c line 139

Quoted 8 lines

drivers/soc/qcom/rpmh-rsc.c new file +8 -0

+static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int m, int n,
+                             u32 data)
+{
+       write_tcs_reg(drv, reg, m, n, data);
+       for (;;) {
+               if (data == read_tcs_reg(drv, reg, m, n))
+                       break;
+               udelay(1);

Should this time out and return a failure?

Quoted 42 lines

drivers/soc/qcom/rpmh-rsc.c new file +42 -0

+       }
+}
+
+static bool tcs_is_free(struct rsc_drv *drv, int m)
+{
+       return !atomic_read(&drv->tcs_in_use[m]) &&
+              read_tcs_reg(drv, RSC_DRV_STATUS, m, 0);
+}
+
+static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+       int i;
+       struct tcs_group *tcs;
+
+       for (i = 0; i < TCS_TYPE_NR; i++) {
+               if (type == drv->tcs[i].type)
+                       break;
+       }
+
+       if (i == TCS_TYPE_NR)
+               return ERR_PTR(-EINVAL);
+
+       tcs = &drv->tcs[i];
+       if (!tcs->num_tcs)
+               return ERR_PTR(-EINVAL);
+
+       return tcs;
+}
+
+static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
+                                       struct tcs_request *msg)
+{
+       int type;
+
+       switch (msg->state) {
+       case RPMH_ACTIVE_ONLY_STATE:
+               type = ACTIVE_TCS;
+               break;
+       default:
+               return ERR_PTR(-EINVAL);
+       }
+

drivers/soc/qcom/rpmh-rsc.c line 191

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c new file +10 -0

+       return get_tcs_of_type(drv, type);
+}
+
+static void send_tcs_response(struct tcs_response *resp)
+{
+       struct rsc_drv *drv = resp->drv;
+       unsigned long flags;
+
+       if (!resp)
+               return;

Does this ever happen? Ah, I see that it might in the irq handler. But get_response already assumes that there is a response, and reaches through it. So I don't think you need this here nor the check+label in the irq handler.

Is requesting an index out of range purely a developer error, or could it happen in some sort of runtime scarcity situation? If it's a developer error, I'd get rid of all the null checking. If it's something that might really happen under the right circumstances, then my comment above doesn't stand and you'd want to fix the null dereference in get_response instead.

Quoted 20 lines

drivers/soc/qcom/rpmh-rsc.c new file +20 -0

+
+       spin_lock_irqsave(&drv->drv_lock, flags);
+       INIT_LIST_HEAD(&resp->list);
+       list_add_tail(&resp->list, &drv->response_pending);
+       spin_unlock_irqrestore(&drv->drv_lock, flags);
+
+       tasklet_schedule(&drv->tasklet);
+}
+
+/**
+ * tcs_irq_handler: TX Done interrupt handler
+ */
+static irqreturn_t tcs_irq_handler(int irq, void *p)
+{
+       struct rsc_drv *drv = p;
+       int m, i;
+       u32 irq_status, sts;
+       struct tcs_response *resp;
+       struct tcs_cmd *cmd;
+       int err;

drivers/soc/qcom/rpmh-rsc.c line 221

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c new file +10 -0

+
+       irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
+
+       for (m = 0; m < drv->num_tcs; m++) {
+               if (!(irq_status & (u32)BIT(m)))
+                       continue;
+
+               err = 0;
+               resp = get_response(drv, m);
+               if (!resp) {

I mention this above, but I don't think get_response can gracefully return null, so is this needed?

drivers/soc/qcom/rpmh-rsc.c line 232

Quoted 11 lines

drivers/soc/qcom/rpmh-rsc.c new file +11 -0

+                       WARN_ON(1);
+                       goto skip_resp;
+               }
+
+               for (i = 0; i < resp->msg->num_payload; i++) {
+                       cmd = &resp->msg->payload[i];
+                       sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, m, i);
+                       if ((!(sts & CMD_STATUS_ISSUED)) ||
+                               ((resp->msg->is_complete || cmd->complete) &&
+                               (!(sts & CMD_STATUS_COMPL)))) {
+                               resp->err = -EIO;

You're trying to set this to -EIO, but then you clobber it to err (which is zero) below

Quoted 16 lines

drivers/soc/qcom/rpmh-rsc.c new file +16 -0

+                               break;
+                       }
+               }
+
+               resp->err = err;
+skip_resp:
+               /* Reclaim the TCS */
+               write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
+               write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
+               atomic_set(&drv->tcs_in_use[m], 0);
+               send_tcs_response(resp);
+       }
+
+       return IRQ_HANDLED;
+}
+

[...]

drivers/soc/qcom/rpmh-rsc.c line 319

Quoted 13 lines

drivers/soc/qcom/rpmh-rsc.c new file +13 -0

+
+static void __tcs_trigger(struct rsc_drv *drv, int m)
+{
+       u32 enable;
+
+       /*
+        * HW req: Clear the DRV_CONTROL and enable TCS again
+        * While clearing ensure that the AMC mode trigger is cleared
+        * and then the mode enable is cleared.
+        */
+       enable = read_tcs_reg(drv, RSC_DRV_CONTROL, m, 0);
+       enable &= ~TCS_AMC_MODE_TRIGGER;
+       write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);

If write_tcs_reg_sync can fail as I suggest, you'd have to fail out of here too.

[...]

Quoted 54 lines

include/soc/qcom/tcs.h new file +48 -0

new file mode 100644
index 000000000000..9465f0560f7a
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SOC_QCOM_TCS_H__
+#define __SOC_QCOM_TCS_H__
+
+#define MAX_RPMH_PAYLOAD       16
+
+/**
+ * rpmh_state: state for the request
+ *
+ * RPMH_WAKE_ONLY_STATE:   Resume resource state to the value previously
+ *                         requested before the processor was powered down.
+ * RPMH_SLEEP_STATE:       State of the resource when the processor subsystem
+ *                         is powered down. There is no client using the
+ *                         resource actively.
+ * RPMH_ACTIVE_ONLY_STATE: Active or AMC mode requests. Resource state
+ *                         is aggregated immediately.
+ */
+enum rpmh_state {
+       RPMH_SLEEP_STATE,
+       RPMH_WAKE_ONLY_STATE,
+       RPMH_ACTIVE_ONLY_STATE,
+};
+
+/**
+ * tcs_cmd: an individual request to RPMH.
+ *
+ * @addr:     the address of the resource slv_id:18:16 | offset:0:15
+ * @data:     the resource state request
+ * @complete: wait for this request to be complete before sending the next
+ */
+struct tcs_cmd {
+       u32 addr;
+       u32 data;
+       bool complete;
+};
+
+/**
+ * tcs_request: A set of tcs_cmds sent togther in a TCS
+ *
+ * @state:       state for the request.
+ * @is_complete: expect a response from the h/w accelerator
+ * @num_payload: the number of tcs_cmds in thsi payload
+ * @payload:     an array of tcs_cmds
+ */

include/soc/qcom/tcs.h line 51

Quoted 3 lines

include/soc/qcom/tcs.h new file +3 -0

+struct tcs_request {
+       enum rpmh_state state;
+       bool is_complete;

is_complete is kind of a misleading name. I would expect this to indicate "the request is complete", not "expecting a completion interrupt". Maybe everybody knows this and I'm just the confused newbie.

Add device binding documentation for Qualcomm Technology Inc's RPMH RSC driver. The hardware block is used for communicating resource state requests for shared resources.

Cc [email protected]

Signed-off-by Lina Iyer <[email protected]>

---

 .../devicetree/bindings/arm/msm/rpmh-rsc.txt       | 146 +++++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt new file +85 -0

new file mode 100644
index 000000000000..6cb36ae98bf9
@@ -0,0 +1,146 @@
+RPMH RSC:
+------------
+
+RPMH is the mechanism for communicating with the hardened resource
+accelerators. Requests to the resources can be written to the TCS mailbox
+registers and using a (addr, val) pair and triggered. Messages in the TCS are
+then sent in sequence over an internal bus.
+
+The hardware block (Direct Resource Voter or DRV) is a part of the h/w entity
+(Resource State Coordinator a.k.a RSC) that can handle a multiple sleep and
+active/wake resource requests. Multiple such DRVs can exist in a SoC and can
+be written to from Linux. The structure of each DRV follows the same template
+with a few variations that are captured by the properties here.
+
+Each DRV could have 'm' TCS instances. Each TCS could have 'n' slots. Each
+slot has a header (u32), address (u32), data (u32), status (u32) and a
+read-response (u32). A TCS when triggered will send all the enabled commands
+of the 'n' commands in that slot in sequence.
+
+A TCS may be triggered from Linux or triggered by the F/W after all the CPUs
+have powered off to facilitate idle power saving. TCS could be classified as -
+
+	SLEEP,  /* Triggered by F/W */
+	WAKE,   /* Triggered by F/W */
+	ACTIVE, /* Triggered by Linux */
+	CONTROL /* Triggered by F/W */
+
+The order in which they are described in the DT, should match the hardware
+configuration.
+
+Requests can be made for the state of a resource, when the subsystem is active
+or idle. When all subsystems like Modem, GPU, CPU are idle, the resource state
+will be an aggregate of the sleep votes from each of those subsystem. Drivers
+may request a sleep value for their shared resources in addition to the active
+mode requests.
+
+Control requests are instance specific requests that may or may not reach an
+accelerator. Only one platform device in Linux can request a control channel
+on a DRV.
+
+CONTROLLER:
+----------
+
+PROPERTIES:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "qcom,rpmh-rsc".
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: The first register specifies the base address of the DRV.
+	            The second register specifies the start address of the
+	            TCS.
+
+- reg-names:
+	Usage: required
+	Value type: <string>
+	Definition: Maps the register specified in the reg property. Must be
+	            "drv" and "tcs".
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-interrupt>
+	Definition: The interrupt that trips when a message complete/response
+	           is received for this DRV from the accelerators.
+
+- qcom,drv-id:
+	Usage: required
+	Value type: <u32>
+	Definition: the id of the DRV in the RSC block.
+
+- qcom,tcs-config:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: the tuple defining the configuration of TCS.
+	            Must have 2 cells which describe each TCS type.
+	            <type number_of_tcs>.
+	            The order of the TCS must match the hardware
+	            configuration.
+	- Cell #1 (TCS Type): TCS types to be specified -
+		SLEEP_TCS
+		WAKE_TCS

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt new file +14 -0

+specify their bindings as child of the corresponding RSC controllers.
+
+EXAMPLE 1:
+
+For a TCS whose RSC base address is is 0x179C0000 and is at a DRV id of 2, the
+register offsets for DRV2 start at 0D00, the register calculations are like
+this -
+First tuple: 0x179C0000 + 0x10000 * 2 = 0x179E0000
+Second tuple: 0x179E0000 + 0xD00 = 0x179E0D00
+
+	apps_rsc: rsc@179e000 {
+		label = "apps_rsc";
+		compatible = "qcom,rpmh-rsc";
+		reg = <0x179e0000 0x10000>, <0x179e0d00 0x3000>;

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt new file +3 -0

+		};
+	};
+

drivers/soc/qcom/rpmh-rsc.c +0 -0

 	}
 
 	write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);

drivers/soc/qcom/trace-rpmh.h new file +18 -0

+TRACE_EVENT(rpmh_send_msg,
+
+	TP_PROTO(struct rsc_drv *d, int m, int n, u32 h, struct tcs_cmd *c),
+
+	TP_ARGS(d, m, n, h, c),
+
+	TP_STRUCT__entry(
+		__field(const char*, d->name)
+		__field(int, m)
+		__field(int, n)
+		__field(u32, hdr)
+		__field(u32, addr)
+		__field(u32, data)
+		__field(bool, complete)
+	),
+
+	TP_fast_assign(
+		__entry->name = s;

drivers/soc/qcom/trace-rpmh.h new file +3 -0

+			__entry->addr, __entry->data, __entry->complete)
+);
+

drivers/soc/qcom/rpmh-rsc.c static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n, · line 306

Quoted 5 lines · On Thu, 15 Feb 2018 10:35:00 -0700 Lina Iyer <[email protected]> wrote:

drivers/soc/qcom/rpmh-rsc.c +1 -0

@@ -298,6 +303,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
 		write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, n + i, msgid);
 		write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr);
 		write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, n + i, cmd->data);
+		trace_rpmh_send_msg(drv, m, n + i, msgid, cmd);

No biggy, but I'm curious to why you didn't do something this:

(quoted diff) +16 -0

+static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
+                             struct tcs_request *msg)
+{
+       u32 msgid, cmd_msgid = 0;
+       u32 cmd_enable = 0;
+       u32 cmd_complete;
+       struct tcs_cmd *cmd;
+       int i;
+
+       cmd_msgid = CMD_MSGID_LEN;
+       cmd_msgid |= (msg->is_complete) ? CMD_MSGID_RESP_REQ : 0;
+       cmd_msgid |= CMD_MSGID_WRITE;
+
+       cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
+
+       for (i = 0; i < msg->num_payload; i++) {

		int bit = n + i;

(quoted diff) +8 -0

+               cmd = &msg->payload[i];
+               cmd_enable |= BIT(bit);
+               cmd_complete |= cmd->complete << (n + i);
+               msgid = cmd_msgid;
+               msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
+               write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, bit, msgid);
+               write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, bit, cmd->addr);
+               write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, bit, cmd->data);

		trace_rpmh_send_msg(drv, m, bit, msgid, cmd);

The compiler should optimize that, so this isn't really a big deal, but I was just curious.

(quoted diff) +6 -0

+       }
+
+       write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
+       cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
+       write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
+}

Quoted 4 lines

drivers/soc/qcom/rpmh-rsc.c +0 -0

 	}
 
 	write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);

[..]

Quoted 18 lines

drivers/soc/qcom/trace-rpmh.h new file +18 -0

+TRACE_EVENT(rpmh_send_msg,
+
+	TP_PROTO(struct rsc_drv *d, int m, int n, u32 h, struct tcs_cmd *c),
+
+	TP_ARGS(d, m, n, h, c),
+
+	TP_STRUCT__entry(
+		__field(const char*, d->name)
+		__field(int, m)
+		__field(int, n)
+		__field(u32, hdr)
+		__field(u32, addr)
+		__field(u32, data)
+		__field(bool, complete)
+	),
+
+	TP_fast_assign(
+		__entry->name = s;

drivers/soc/qcom/trace-rpmh.h line 77

Quoted 10 lines

drivers/soc/qcom/trace-rpmh.h new file +10 -0

+		__entry->m = m;
+		__entry->n = n;
+		__entry->hdr = h;
+		__entry->addr = c->addr;
+		__entry->data = c->data;
+		__entry->complete = c->complete;
+	),
+
+	TP_printk("%s: send-msg: tcs(m): %d cmd(n): %d msgid: 0x%08x addr: 0x%08x data: 0x%08x complete: %d",
+			__entry->name, __entry->m, __entry->n, __entry->hdr,

I'm sorry I didn't catch this in my other reviews, but please don't use direct strings in TP_printk(). In trace-cmd and perf, it has no access to that information when reading this trace event. Not to mention, if drv is freed between the time it is recorded, and the time it is read in the trace buffer, you are now referencing random memory.

The way to do this in a trace event is to use the string functionality:

	TP_STRUCT__entry(
		__string(name, d->name)
		[..]
	TP_fast_assign(
		__assign_string(name, d->name)
		[..]
	TP_printk("%s: ...",
		__get_str(name), ...

Then the name is recorded in the ring buffer at the time of execution of the trace event, and trace-cmd and perf can read it, and there's no worries about it being freed between recording and reading the tracing buffer.

Log sent RPMH requests and interrupt responses in FTRACE.

Cc Steven Rostedt <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---

drivers/soc/qcom/Makefile     |  1 +
drivers/soc/qcom/rpmh-rsc.c   |  6 +++
drivers/soc/qcom/trace-rpmh.h | 89 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 96 insertions(+)

create mode 100644 drivers/soc/qcom/trace-rpmh.h

drivers/soc/qcom/Makefile +0 -0

index 7426979ea9fb..094b639ece2f 100644

drivers/soc/qcom/Makefile line 2

Quoted 5 lines

drivers/soc/qcom/Makefile +1 -0

@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+CFLAGS_trace-rpmh.o := -I$(src)  

I did this here..

...

drivers/soc/qcom/Makefile

Quoted 3 lines

drivers/soc/qcom/Makefile +3 -0

+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../drivers/soc/qcom
+  

Yet, I had to define the whole path here for it to compile. Per comments I should just need to do this -

#define TRACE_INCLUDE_PATH .

But it doesn't work. What am I missing?

drivers/soc/qcom/Makefile

Quoted 5 lines

drivers/soc/qcom/Makefile +4 -0

+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace-rpmh
+
+#include <trace/define_trace.h>

Thanks,
Lina

Quoted 6 lines · On Thu, 15 Feb 2018 20:41:18 +0000 Lina Iyer <[email protected]> wrote:

drivers/soc/qcom/Makefile line 2

Quoted 5 lines

drivers/soc/qcom/Makefile +1 -0

@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+CFLAGS_trace-rpmh.o := -I$(src)

I did this here..

Needs to be:

CFLAGS_rpmh-rsc.o

You need to use the C file that has the CREATE_TRACE_POINTS define.

-- Steve

Quoted 21 lines

...

drivers/soc/qcom/Makefile

Quoted 3 lines

drivers/soc/qcom/Makefile +3 -0

+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../drivers/soc/qcom
+

Yet, I had to define the whole path here for it to compile. Per comments I should just need to do this -

#define TRACE_INCLUDE_PATH .

But it doesn't work. What am I missing?

drivers/soc/qcom/Makefile

Quoted 5 lines

drivers/soc/qcom/Makefile +4 -1

+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace-rpmh
+
+#include <trace/define_trace.h>
--

Thanks,
Lina

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is job of this controller driver to find arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   7 +++
 drivers/soc/qcom/rpmh-rsc.c      | 126 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +17 -0

+static int find_slots(struct tcs_group *tcs, struct tcs_request *msg,
+                    int *m, int *n)
+{
+       int slot, offset;
+       int i = 0;
+
+       /* Find if we already have the msg in our TCS */
+       slot = find_match(tcs, msg->payload, msg->num_payload);
+       if (slot >= 0)
+               goto copy_data;
+
+       /* Do over, until we can fit the full payload in a TCS */
+       do {
+               slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
+                                                i, msg->num_payload, 0);
+               if (slot == MAX_TCS_SLOTS)
+                       break;

Hello Lina,

Quoted 17 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is job of this controller driver to find arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |   7 +++
 drivers/soc/qcom/rpmh-rsc.c      | 126 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

[...]

Quoted 23 lines

drivers/soc/qcom/rpmh-rsc.c +23 -0

+static int find_slots(struct tcs_group *tcs, struct tcs_request *msg,
+                    int *m, int *n)
+{
+       int slot, offset;
+       int i = 0;
+
+       /* Find if we already have the msg in our TCS */
+       slot = find_match(tcs, msg->payload, msg->num_payload);
+       if (slot >= 0)
+               goto copy_data;
+
+       /* Do over, until we can fit the full payload in a TCS */
+       do {
+               slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
+                                                i, msg->num_payload, 0);
+               if (slot == MAX_TCS_SLOTS)
+                       break;
+               i += tcs->ncpt;
+       } while (slot + msg->num_payload - 1 >= i);
+
+       if (slot == MAX_TCS_SLOTS)
+               return -ENOMEM;
+

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg) · line 511

Quoted 4 lines

drivers/soc/qcom/rpmh-rsc.c +4 -0

+copy_data:
+       bitmap_set(tcs->slots, slot, msg->num_payload);
+       /* Copy the addresses of the resources over to the slots */
+       for (i = 0; tcs->cmd_addr && i < msg->num_payload; i++)

I don't think tcs->cmd_addr can be null, can it? Above, find_match() is already reaching through cmd_addr with enthusiasm. If kept, it could at least be moved outside of the loop.

Active state requests are sent immediately to the mailbox controller, while sleep and wake state requests are cached in this driver to avoid taxing the mailbox controller repeatedly. The cached values will be sent to the controller when the rpmh_flush() is called.

Generally, flushing is a system PM activity and may be called from the system PM drivers when the system is entering suspend or deeper sleep modes during cpuidle.

Also allow invalidating the cached requests, so they may be re-populated again.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |  10 +++
 2 files changed, 222 insertions(+), 1 deletion(-)

drivers/soc/qcom/rpmh.c +49 -0

index d95ea3fa8b67..671bc03ad77a 100644
@@ -6,11 +6,13 @@
 #include <linux/atomic.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/wait.h>
 

@@ -35,6 +37,22 @@
                .rc = rc,                               \
        }
 
+
+/**
+ * cache_req : the request object for caching
+ *
+ * @addr: the address of the resource
+ * @sleep_val: the sleep vote
+ * @wake_val: the wake vote
+ * @list: linked list obj
+ */
+struct cache_req {
+       u32 addr;
+       u32 sleep_val;
+       u32 wake_val;
+       struct list_head list;
+};
+
 /**
  * rpmh_request: the message to be sent to rpmh-rsc
  *

@@ -57,9 +75,15 @@ struct rpmh_request {
  * rpmh_ctrlr: our representation of the controller
  *
  * @drv: the controller instance
+ * @cache: the list of cached requests
+ * @lock: synchronize access to the controller data
+ * @dirty: was the cache updated since flush
  */
 struct rpmh_ctrlr {
        struct rsc_drv *drv;
+       struct list_head cache;
+       spinlock_t lock;
+       bool dirty;
 };
 
 /**

@@ -123,17 +147,91 @@ static int wait_for_tx_done(struct rpmh_client *rc,
        return (ret > 0) ? 0 : -ETIMEDOUT;
 }
 
+static struct cache_req *__find_req(struct rpmh_client *rc, u32 addr)
+{
+       struct cache_req *p, *req = NULL;
+
+       list_for_each_entry(p, &rc->ctrlr->cache, list) {
+               if (p->addr == addr) {
+                       req = p;
+                       break;
+               }
+       }
+
+       return req;
+}
+
+static struct cache_req *cache_rpm_request(struct rpmh_client *rc,
+                                         enum rpmh_state state,
+                                         struct tcs_cmd *cmd)
+{
+       struct cache_req *req;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       unsigned long flags;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       req = __find_req(rc, cmd->addr);
+       if (req)

drivers/soc/qcom/rpmh.c +108 -1

+       INIT_LIST_HEAD(&req->list);
+       list_add_tail(&req->list, &rpm->cache);
+
+existing:
+       switch (state) {
+       case RPMH_ACTIVE_ONLY_STATE:
+               if (req->sleep_val != UINT_MAX)
+                       req->wake_val = cmd->data;
+               break;
+       case RPMH_WAKE_ONLY_STATE:
+               req->wake_val = cmd->data;
+               break;
+       case RPMH_SLEEP_STATE:
+               req->sleep_val = cmd->data;
+               break;
+       default:
+               break;
+       };
+
+       rpm->dirty = true;
+unlock:
+       spin_unlock_irqrestore(&rpm->lock, flags);
+
+       return req;
+}
+
 /**
- * __rpmh_write: send the RPMH request
+ * __rpmh_write: Cache and send the RPMH request
  *
  * @rc: The RPMH client
  * @state: Active/Sleep request type
  * @rpm_msg: The data that needs to be sent (payload).
+ *
+ * Cache the RPMH request and send if the state is ACTIVE_ONLY.
+ * SLEEP/WAKE_ONLY requests are not sent to the controller at
+ * this time. Use rpmh_flush() to send them to the controller.
  */
 static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
                       struct rpmh_request *rpm_msg)
 {
        int ret = -EFAULT;
+       struct cache_req *req;
+       int i;
+
+       /* Cache the request in our store and link the payload */
+       for (i = 0; i < rpm_msg->msg.num_payload; i++) {
+               req = cache_rpm_request(rc, state, &rpm_msg->msg.payload[i]);
+               if (IS_ERR(req))
+                       return PTR_ERR(req);
+       }
 
        rpm_msg->msg.state = state;
 

@@ -150,6 +248,10 @@ static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
                                "Error in RPMH request addr=0x%x, data=0x%x\n",
                                rpm_msg->msg.payload[0].addr,
                                rpm_msg->msg.payload[0].data);
+       } else {
+               ret = rpmh_rsc_write_ctrl_data(rc->ctrlr->drv, &rpm_msg->msg);
+               /* Clean up our call by spoofing tx_done */
+               rpmh_tx_done(&rpm_msg->msg, ret);
        }
 
        return ret;

@@ -189,6 +291,113 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
 }
 EXPORT_SYMBOL(rpmh_write);
 
+static int is_req_valid(struct cache_req *req)
+{
+       return (req->sleep_val != UINT_MAX &&
+              req->wake_val != UINT_MAX &&
+              req->sleep_val != req->wake_val);
+}
+
+static int send_single(struct rpmh_client *rc, enum rpmh_state state,
+                     u32 addr, u32 data)
+{
+       DEFINE_RPMH_MSG_ONSTACK(rc, state, NULL, NULL, rpm_msg);
+
+       /* Wake sets are always complete and sleep sets are not */
+       rpm_msg.msg.is_complete = (state == RPMH_WAKE_ONLY_STATE);
+       rpm_msg.cmd[0].addr = addr;
+       rpm_msg.cmd[0].data = data;
+       rpm_msg.msg.num_payload = 1;
+       rpm_msg.msg.is_complete = false;
+
+       return rpmh_rsc_write_ctrl_data(rc->ctrlr->drv, &rpm_msg.msg);
+}
+
+/**
+ * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * This function is generally called from the sleep code from the last CPU
+ * that is powering down the entire system.
+ *
+ * Returns -EBUSY if the controller is busy, probably waiting on a response
+ * to a RPMH request sent earlier.
+ */
+int rpmh_flush(struct rpmh_client *rc)
+{
+       struct cache_req *p;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       int ret;
+       unsigned long flags;
+
+       if (IS_ERR_OR_NULL(rc))
+               return -EINVAL;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       if (!rpm->dirty) {
+               pr_debug("Skipping flush, TCS has latest data.\n");
+               spin_unlock_irqrestore(&rpm->lock, flags);
+               return 0;
+       }
+       spin_unlock_irqrestore(&rpm->lock, flags);
+
+       /*
+        * Nobody else should be calling this function other than system PM,,
+        * hence we can run without locks.
+        */
+       list_for_each_entry(p, &rc->ctrlr->cache, list) {
+               if (!is_req_valid(p)) {
+                       pr_debug("%s: skipping RPMH req: a:0x%x s:0x%x w:0x%x",
+                               __func__, p->addr, p->sleep_val, p->wake_val);
+                       continue;
+               }
+               ret = send_single(rc, RPMH_SLEEP_STATE, p->addr, p->sleep_val);
+               if (ret)
+                       return ret;

drivers/soc/qcom/rpmh.c +14 -0

+       return 0;
+}
+EXPORT_SYMBOL(rpmh_flush);
+
+/**
+ * rpmh_invalidate: Invalidate all sleep and active sets
+ * sets.
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * Invalidate the sleep and active values in the TCS blocks.
+ */
+int rpmh_invalidate(struct rpmh_client *rc)
+{

drivers/soc/qcom/rpmh.c +11 -0

+
+       do {
+               ret = rpmh_rsc_invalidate(rc->ctrlr->drv);
+       } while (ret == -EAGAIN);
+
+       return ret;
+}
+EXPORT_SYMBOL(rpmh_invalidate);
+
 static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
 {
        int i;

@@ -210,6 +419,8 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
                if (rpmh_rsc[i].drv == NULL) {
                        ctrlr = &rpmh_rsc[i];
                        ctrlr->drv = drv;
+                       spin_lock_init(&ctrlr->lock);
+                       INIT_LIST_HEAD(&ctrlr->cache);
                        break;
                }
        }

include/soc/qcom/rpmh.h +10 -1

index 53cc145990c2..a3f1246ce49a 100644
@@ -17,6 +17,10 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
 
 struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
 
+int rpmh_flush(struct rpmh_client *rc);
+
+int rpmh_invalidate(struct rpmh_client *rc);
+
 void rpmh_release(struct rpmh_client *rc);
 
 #else

@@ -28,6 +32,12 @@ static inline int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
 static inline struct rpmh_client *rpmh_get_client(struct platform_device *pdev)
 { return ERR_PTR(-ENODEV); }
 
+static inline int rpmh_flush(struct rpmh_client *rc)
+{ return -ENODEV; }
+
+static inline int rpmh_invalidate(struct rpmh_client *rc)
+{ return -ENODEV; }
+
 static inline void rpmh_release(struct rpmh_client *rc) { }
 #endif /* CONFIG_QCOM_RPMH */
 
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer <[email protected]> wrote:

Quoted 18 lines

drivers/soc/qcom/rpmh.c +18 -0

+static struct cache_req *cache_rpm_request(struct rpmh_client *rc,
+                                         enum rpmh_state state,
+                                         struct tcs_cmd *cmd)
+{
+       struct cache_req *req;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       unsigned long flags;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       req = __find_req(rc, cmd->addr);
+       if (req)
+               goto existing;
+
+       req = kzalloc(sizeof(*req), GFP_ATOMIC);
+       if (!req) {
+               req = ERR_PTR(-ENOMEM);
+               goto unlock;
+       }

drivers/soc/qcom/rpmh.c static int wait_for_tx_done(struct rpmh_client *rc, · line 184

Quoted 3 lines

drivers/soc/qcom/rpmh.c +3 -0

+
+       req->addr = cmd->addr;
+       req->sleep_val = req->wake_val = UINT_MAX;

So UINT_MAX is really never a valid value to write? Maybe it would be good to at least print some sort of complaint if somebody sends down a request with this value. Otherwise the request is silently ignored and would be quite challenging to track down.

Quoted 42 lines

drivers/soc/qcom/rpmh.c +42 -0

+/**
+ * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * This function is generally called from the sleep code from the last CPU
+ * that is powering down the entire system.
+ *
+ * Returns -EBUSY if the controller is busy, probably waiting on a response
+ * to a RPMH request sent earlier.
+ */
+int rpmh_flush(struct rpmh_client *rc)
+{
+       struct cache_req *p;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       int ret;
+       unsigned long flags;
+
+       if (IS_ERR_OR_NULL(rc))
+               return -EINVAL;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       if (!rpm->dirty) {
+               pr_debug("Skipping flush, TCS has latest data.\n");
+               spin_unlock_irqrestore(&rpm->lock, flags);
+               return 0;
+       }
+       spin_unlock_irqrestore(&rpm->lock, flags);
+
+       /*
+        * Nobody else should be calling this function other than system PM,,
+        * hence we can run without locks.
+        */
+       list_for_each_entry(p, &rc->ctrlr->cache, list) {
+               if (!is_req_valid(p)) {
+                       pr_debug("%s: skipping RPMH req: a:0x%x s:0x%x w:0x%x",
+                               __func__, p->addr, p->sleep_val, p->wake_val);
+                       continue;
+               }
+               ret = send_single(rc, RPMH_SLEEP_STATE, p->addr, p->sleep_val);
+               if (ret)
+                       return ret;

drivers/soc/qcom/rpmh.c int rpmh_write(struct rpmh_client *rc, enum rpmh_state state, · line 367

Quoted 10 lines

drivers/soc/qcom/rpmh.c +10 -0

+               ret = send_single(rc, RPMH_WAKE_ONLY_STATE, p->addr,
+                                               p->wake_val);
+               if (ret)
+                       return ret;
+       }
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       rpm->dirty = false;
+       spin_unlock_irqrestore(&rpm->lock, flags);
+

I've got some questions on the locking in this function.

I understand that the lock protects the list, and I'm surmising that you don't want to hold the lock across send_single (even though there's another lock in there that's held for most of that time, so I think you could). I'm still a newbie to Linux in general, so I'll pose this as a question: is it generally okay in Linux to traverse across a list that may have items concurrently added to it? You're never removing items from this list, so I think there are no actual bugs, but it does seem like it relies on the implementation details of the list. And if you ever did remove items from the list, this would bite you.

Also, why do you need to acquire the lock just to set dirty to false? Right now it looks like there's a race where someone could add an element to this list just after you've terminated this loop (but before you have the lock), but then the dirty = false here clobbers their dirty = true, and the item is never sent during future flushes.

I think it would be safer and faster to set dirty = false before iterating through the list (either within the lock or outside of it given that this is the only place that reads or clears dirty). That way if new elements sneak in you know that they will either be flushed already or dirty will be true for them on a subsequent flush.

Quoted 19 lines

drivers/soc/qcom/rpmh.c +19 -0

+       return 0;
+}
+EXPORT_SYMBOL(rpmh_flush);
+
+/**
+ * rpmh_invalidate: Invalidate all sleep and active sets
+ * sets.
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * Invalidate the sleep and active values in the TCS blocks.
+ */
+int rpmh_invalidate(struct rpmh_client *rc)
+{
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       int ret;
+       unsigned long flags;
+
+       if (IS_ERR_OR_NULL(rc))

drivers/soc/qcom/rpmh.c int rpmh_write(struct rpmh_client *rc, enum rpmh_state state, · line 391

Quoted 5 lines

drivers/soc/qcom/rpmh.c +5 -0

+               return -EINVAL;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       rpm->dirty = true;
+       spin_unlock_irqrestore(&rpm->lock, flags);

I don't think the lock acquire/release provides anything here, can't you just set dirty = true?

So rpmh_invalidate clears any pending requests in the hardware, but all the cached address/data pairs are all still in the cache, right? As soon as someone else adds a new request and sets dirty to true, all of these old ones get resent as well at flush, right? Is that the desired behavior? Does anyone ever need to remove an address/data pair from the RPMh's to-do list?

Quoted 9 lines

drivers/soc/qcom/rpmh.c +9 -0

+
+       do {
+               ret = rpmh_rsc_invalidate(rc->ctrlr->drv);
+       } while (ret == -EAGAIN);
+
+       return ret;
+}
+EXPORT_SYMBOL(rpmh_invalidate);
+

Quoted 4 lines · Evan Green

Hi Lina,

On Thu, Feb 15, 2018 at 9:35 AM, Lina Iyer <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 19 lines

drivers/soc/qcom/rpmh.c +19 -0

+static struct cache_req *cache_rpm_request(struct rpmh_client *rc,
+                                         enum rpmh_state state,
+                                         struct tcs_cmd *cmd)
+{
+       struct cache_req *req;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       unsigned long flags;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       req = __find_req(rc, cmd->addr);
+       if (req)
+               goto existing;
+
+       req = kzalloc(sizeof(*req), GFP_ATOMIC);
+       if (!req) {
+               req = ERR_PTR(-ENOMEM);
+               goto unlock;
+       }
+

Quoted 9 lines

drivers/soc/qcom/rpmh.c static int wait_for_tx_done(struct rpmh_client *rc, · line 184

Quoted 2 lines

drivers/soc/qcom/rpmh.c +2 -0

+       req->addr = cmd->addr;
+       req->sleep_val = req->wake_val = UINT_MAX;

So UINT_MAX is really never a valid value to write? Maybe it would be good to at least print some sort of complaint if somebody sends down a request with this value. Otherwise the request is silently ignored and would be quite challenging to track down.

Yes, UINT_MAX is a invalid value. I have not encountered anything in the spec that points to hold UINT_MAX as a valid value.
It would also be quite inefficient to validate each input. Drivers generally know what they are doing. These are data sent to the hardware and there is a general understanding that incorrect data may fail silently.

Quoted 62 lines

Quoted 48 lines

+/**
+ * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * This function is generally called from the sleep code from the last CPU
+ * that is powering down the entire system.
+ *
+ * Returns -EBUSY if the controller is busy, probably waiting on a response
+ * to a RPMH request sent earlier.
+ */

drivers/soc/qcom/rpmh.c +20 -0

+int rpmh_flush(struct rpmh_client *rc)
+{
+       struct cache_req *p;
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       int ret;
+       unsigned long flags;
+
+       if (IS_ERR_OR_NULL(rc))
+               return -EINVAL;
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       if (!rpm->dirty) {
+               pr_debug("Skipping flush, TCS has latest data.\n");
+               spin_unlock_irqrestore(&rpm->lock, flags);
+               return 0;
+       }
+       spin_unlock_irqrestore(&rpm->lock, flags);
+
+       /*
+        * Nobody else should be calling this function other than system

PM,,

drivers/soc/qcom/rpmh.c +5 -0

+        * hence we can run without locks.
+        */
+       list_for_each_entry(p, &rc->ctrlr->cache, list) {
+               if (!is_req_valid(p)) {
+                       pr_debug("%s: skipping RPMH req: a:0x%x s:0x%x

w:0x%x",
+ __func__, p->addr, p->sleep_val,
p->wake_val);

drivers/soc/qcom/rpmh.c +3 -0

+                       continue;
+               }
+               ret = send_single(rc, RPMH_SLEEP_STATE, p->addr,

p->sleep_val);

drivers/soc/qcom/rpmh.c +2 -0

+               if (ret)
+                       return ret;

drivers/soc/qcom/rpmh.c int rpmh_write(struct rpmh_client *rc, enum rpmh_state state, · line 367

Quoted 10 lines

drivers/soc/qcom/rpmh.c +10 -0

+               ret = send_single(rc, RPMH_WAKE_ONLY_STATE, p->addr,
+                                               p->wake_val);
+               if (ret)
+                       return ret;
+       }
+
+       spin_lock_irqsave(&rpm->lock, flags);
+       rpm->dirty = false;
+       spin_unlock_irqrestore(&rpm->lock, flags);
+

I've got some questions on the locking in this function.

Quoted 11 lines

I understand that the lock protects the list, and I'm surmising that you don't want to hold the lock across send_single (even though there's another lock in there that's held for most of that time, so I think you could). I'm still a newbie to Linux in general, so I'll pose this as a question: is it generally okay in Linux to traverse across a list that may have items concurrently added to it? You're never removing items from this list, so I think there are no actual bugs, but it does seem like it relies on the implementation details of the list. And if you ever did remove items from the list, this would bite you.

It is generally not advisable to traverse a list if the list is being modified externally. The reason why it is okay here, is the context of this function call. The only caller of this function will be a system PM driver. As noted in the comments, this is called when the last CPU is powering down and in such a context, there are no other active users of this library.

Quoted 6 lines

Also, why do you need to acquire the lock just to set dirty to false? Right now it looks like there's a race where someone could add an element to this list just after you've terminated this loop (but before you have the lock), but then the dirty = false here clobbers their dirty = true, and the item is never sent during future flushes.

Quoted 6 lines

I think it would be safer and faster to set dirty = false before iterating through the list (either within the lock or outside of it given that this is the only place that reads or clears dirty). That way if new elements sneak in you know that they will either be flushed already or dirty will be true for them on a subsequent flush.

->dirty will be accessed as a u32 and so does not require a lock per-se. The lock here guarantees a certain system state when the flag is changed. While in this function it is not needed since, there will be no active threads changing the system state. I could possibly remove the lock in this function.

Quoted 21 lines

Quoted 21 lines

drivers/soc/qcom/rpmh.c +20 -0

+       return 0;
+}
+EXPORT_SYMBOL(rpmh_flush);
+
+/**
+ * rpmh_invalidate: Invalidate all sleep and active sets
+ * sets.
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ *
+ * Invalidate the sleep and active values in the TCS blocks.
+ */
+int rpmh_invalidate(struct rpmh_client *rc)
+{
+       struct rpmh_ctrlr *rpm = rc->ctrlr;
+       int ret;
+       unsigned long flags;
+
+       if (IS_ERR_OR_NULL(rc))
+               return -EINVAL;

Quoted 9 lines

drivers/soc/qcom/rpmh.c static int wait_for_tx_done(struct rpmh_client *rc, · line 206

Quoted 4 lines

drivers/soc/qcom/rpmh.c +4 -0

+
+       spin_lock_irqsave(&rpm->lock, flags);
+       rpm->dirty = true;
+       spin_unlock_irqrestore(&rpm->lock, flags);

I don't think the lock acquire/release provides anything here, can't you just set dirty = true?

It helps synchronize to the state of the controller.

Quoted 8 lines

So rpmh_invalidate clears any pending requests in the hardware, but all the cached address/data pairs are all still in the cache, right? As soon as someone else adds a new request and sets dirty to true, all of these old ones get resent as well at flush, right? Is that the desired behavior? Does anyone ever need to remove an address/data pair from the RPMh's to-do list?

Drivers overwrite their requests and I have not had a requirement to remove their previous votes. Removing from the controller's registers is good enough to not send the data. If a driver calls invalidate, it is generally followed by a new sleep/wake request.

Thanks,
Lina

Quoted 9 lines

Quoted 9 lines

drivers/soc/qcom/rpmh.c +9 -0

+
+       do {
+               ret = rpmh_rsc_invalidate(rc->ctrlr->drv);
+       } while (ret == -EAGAIN);
+
+       return ret;
+}
+EXPORT_SYMBOL(rpmh_invalidate);
+

Platform drivers need make a lot of resource state requests at the same time, say, at the start or end of an usecase. It can be quite inefficient to send each request separately. Instead they can give the RPMH library a batch of requests to be sent and wait on the whole transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple RPMH command sets. Each RPMH command set is set asynchronously and the API blocks until all the command sets are complete and receive their tx_done callbacks.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 158 insertions(+)

drivers/soc/qcom/rpmh.c +0 -0

index dff4c46be3af..6f60bb9a4dfa 100644

drivers/soc/qcom/rpmh.c +5 -0

@@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
        }
        spin_unlock_irqrestore(&rpm->lock, flags);
 
+       /* First flush the cached batch requests */
+       ret = flush_batch(rc);
+       if (ret)
+               return ret;
+
        /*
         * Nobody else should be calling this function other than system PM,,
         * hence we can run without locks.

Hello Lina,

Quoted 21 lines · Lina Iyer

Platform drivers need make a lot of resource state requests at the same time, say, at the start or end of an usecase. It can be quite inefficient to send each request separately. Instead they can give the RPMH library a batch of requests to be sent and wait on the whole transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple RPMH command sets. Each RPMH command set is set asynchronously and the API blocks until all the command sets are complete and receive their tx_done callbacks.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 158 insertions(+)

drivers/soc/qcom/rpmh.c +0 -0

index dff4c46be3af..6f60bb9a4dfa 100644

[...]

Quoted 4 lines

drivers/soc/qcom/rpmh.c +0 -0

@@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
        }
        spin_unlock_irqrestore(&rpm->lock, flags);
 

drivers/soc/qcom/rpmh.c int rpmh_flush(struct rpmh_client *rc) · line 547

Quoted 8 lines

drivers/soc/qcom/rpmh.c +5 -0

+       /* First flush the cached batch requests */
+       ret = flush_batch(rc);
+       if (ret)
+               return ret;
+
        /*
         * Nobody else should be calling this function other than system PM,,
         * hence we can run without locks.

Quoted 4 lines

drivers/soc/qcom/rpmh.c +0 -0

@@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
        if (IS_ERR_OR_NULL(rc))
                return -EINVAL;
 

drivers/soc/qcom/rpmh.c int rpmh_invalidate(struct rpmh_client *rc) · line 590

Quoted 2 lines

drivers/soc/qcom/rpmh.c +2 -0

+       invalidate_batch(rc);
+

Similarly to my comments in patch 7, aren't there races here with adding new elements? After flush_batch, but before invalidate_batch, somebody could call cache_batch, which would add new things on the end of the array. These new items would be clobbered by invalidate_batch, without having been flushed first.

Quoted 34 lines · Evan Green

Hello Lina,

Quoted 23 lines · Lina Iyer

Platform drivers need make a lot of resource state requests at the same time, say, at the start or end of an usecase. It can be quite inefficient to send each request separately. Instead they can give the RPMH library a batch of requests to be sent and wait on the whole transaction to be complete.

rpmh_write_batch() is a blocking call that can be used to send multiple RPMH command sets. Each RPMH command set is set asynchronously and the API blocks until all the command sets are complete and receive their tx_done callbacks.

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 150
++++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 158 insertions(+)

drivers/soc/qcom/rpmh.c +0 -0

index dff4c46be3af..6f60bb9a4dfa 100644

[...]

Quoted 5 lines

drivers/soc/qcom/rpmh.c +0 -0

@@ -394,6 +537,11 @@ int rpmh_flush(struct rpmh_client *rc)
        }
        spin_unlock_irqrestore(&rpm->lock, flags);
 

Quoted 9 lines

Quoted 9 lines

drivers/soc/qcom/rpmh.c +5 -0

+       /* First flush the cached batch requests */
+       ret = flush_batch(rc);
+       if (ret)
+               return ret;
+
        /*
         * Nobody else should be calling this function other than system
 PM,,

* hence we can run without locks.

This comment and the comment in the header of this function.

Quoted 4 lines

Quoted 4 lines

drivers/soc/qcom/rpmh.c +0 -0

@@ -438,6 +586,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
        if (IS_ERR_OR_NULL(rc))
                return -EINVAL;
 

Quoted 10 lines

drivers/soc/qcom/rpmh.c int rpmh_invalidate(struct rpmh_client *rc) · line 590

Quoted 2 lines

drivers/soc/qcom/rpmh.c +2 -0

+       invalidate_batch(rc);
+

Similarly to my comments in patch 7, aren't there races here with adding new elements? After flush_batch, but before invalidate_batch, somebody could call cache_batch, which would add new things on the end of the array. These new items would be clobbered by invalidate_batch, without having been flushed first.

Flush is a system PM function and is not called by drivers and is not expected to run conncurrently with other threads using the RPMH library.

Thanks,
Lina