* [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-06 19:45 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs Lina Iyer
` (8 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
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 <ilina@codeaurora.org>
---
drivers/soc/qcom/Kconfig | 10 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/rpmh-internal.h | 87 +++++
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, 761 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
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb83341d..779f0d14748d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -55,6 +55,16 @@ config QCOM_RMTFS_MEM
Say y here if you intend to boot the modem remoteproc.
+config QCOM_RPMH
+ bool "Qualcomm RPM-Hardened (RPMH) Communication"
+ depends on ARCH_QCOM && ARM64 && OF
+ help
+ Support for communication with the hardened-RPM blocks in
+ Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
+ internal bus to transmit state requests for shared resources. A set
+ of hardware components aggregate requests for these resources and
+ help apply the aggregated state on the resource.
+
config QCOM_SMEM
tristate "Qualcomm Shared Memory Manager (SMEM)"
depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf2814e6d..e7d04f0e3616 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
new file mode 100644
index 000000000000..12faec77c4f3
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -0,0 +1,87 @@
+/* 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>
+
+#define TCS_TYPE_NR 4
+#define MAX_CMDS_PER_TCS 16
+#define MAX_TCS_PER_TYPE 3
+#define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+
+struct rsc_drv;
+
+/**
+ * tcs_response: Response object for a request
+ *
+ * @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 {
+ struct rsc_drv *drv;
+ struct tcs_request *msg;
+ u32 m;
+ int err;
+ struct list_head list;
+};
+
+/**
+ * tcs_group: group of Trigger Command Sets for a request state
+ *
+ * @drv: the controller
+ * @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;
+ u32 tcs_mask;
+ u32 tcs_offset;
+ int num_tcs;
+ int ncpt;
+ spinlock_t tcs_lock;
+ struct tcs_response *responses[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * rsc_drv: the Resource State Coordinator controller
+ *
+ * @name: controller identifier
+ * @tcs_base: start address of the TCS registers in this controller
+ * @drv_id: instance id in the controller (Direct Resource Voter)
+ * @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__ */
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
new file mode 100644
index 000000000000..6cb91a0114ee
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -0,0 +1,593 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
+
+#include <linux/atomic.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include <asm-generic/io.h>
+#include <soc/qcom/tcs.h>
+#include <dt-bindings/soc/qcom,rpmh-rsc.h>
+
+#include "rpmh-internal.h"
+
+#define RSC_DRV_TCS_OFFSET 672
+#define RSC_DRV_CMD_OFFSET 20
+
+/* DRV Configuration Information Register */
+#define DRV_PRNT_CHLD_CONFIG 0x0C
+#define DRV_NUM_TCS_MASK 0x3F
+#define DRV_NUM_TCS_SHIFT 6
+#define DRV_NCPT_MASK 0x1F
+#define DRV_NCPT_SHIFT 27
+
+/* Register offsets */
+#define RSC_DRV_IRQ_ENABLE 0x00
+#define RSC_DRV_IRQ_STATUS 0x04
+#define RSC_DRV_IRQ_CLEAR 0x08
+#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10
+#define RSC_DRV_CONTROL 0x14
+#define RSC_DRV_STATUS 0x18
+#define RSC_DRV_CMD_ENABLE 0x1C
+#define RSC_DRV_CMD_MSGID 0x30
+#define RSC_DRV_CMD_ADDR 0x34
+#define RSC_DRV_CMD_DATA 0x38
+#define RSC_DRV_CMD_STATUS 0x3C
+#define RSC_DRV_CMD_RESP_DATA 0x40
+
+#define TCS_AMC_MODE_ENABLE BIT(16)
+#define TCS_AMC_MODE_TRIGGER BIT(24)
+
+/* TCS CMD register bit mask */
+#define CMD_MSGID_LEN 8
+#define CMD_MSGID_RESP_REQ BIT(8)
+#define CMD_MSGID_WRITE BIT(16)
+#define CMD_STATUS_ISSUED BIT(8)
+#define CMD_STATUS_COMPL BIT(16)
+
+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);
+}
+
+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);
+ }
+}
+
+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);
+ }
+
+ 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;
+
+ 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;
+
+ 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) {
+ 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)))) {
+ err = -EIO;
+ 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;
+}
+
+/**
+ * tcs_notify_tx_done: TX Done for requests that got a response
+ *
+ * @data: the tasklet argument
+ *
+ * Tasklet function to notify MBOX that we are done with the request.
+ * Handles all pending reponses whenever run.
+ */
+static void tcs_notify_tx_done(unsigned long data)
+{
+ struct rsc_drv *drv = (struct rsc_drv *)data;
+ struct tcs_response *resp;
+ unsigned long flags;
+
+ for (;;) {
+ spin_lock_irqsave(&drv->drv_lock, flags);
+ if (list_empty(&drv->response_pending)) {
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ break;
+ }
+ resp = list_first_entry(&drv->response_pending,
+ struct tcs_response, list);
+ list_del(&resp->list);
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ free_response(resp);
+ }
+}
+
+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, j;
+
+ 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, j = n; i < msg->num_payload; i++, j++) {
+ cmd = &msg->payload[i];
+ cmd_enable |= BIT(j);
+ cmd_complete |= cmd->complete << j;
+ msgid = cmd_msgid;
+ msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
+ write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, j, msgid);
+ write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j, cmd->addr);
+ write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, j, cmd->data);
+ }
+
+ 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);
+}
+
+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);
+ enable &= ~TCS_AMC_MODE_ENABLE;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+
+ /* Enable the AMC mode on the TCS and then trigger the TCS */
+ enable = TCS_AMC_MODE_ENABLE;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+ enable |= TCS_AMC_MODE_TRIGGER;
+ write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
+}
+
+static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
+ struct tcs_request *msg)
+{
+ unsigned long curr_enabled;
+ u32 addr;
+ int i, j, k;
+ int m = tcs->tcs_offset;
+
+ for (i = 0; i < tcs->num_tcs; i++, m++) {
+ if (tcs_is_free(drv, m))
+ continue;
+
+ curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
+
+ for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
+ addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j);
+ for (k = 0; k < msg->num_payload; k++) {
+ if (addr == msg->payload[k].addr)
+ return -EBUSY;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int find_free_tcs(struct tcs_group *tcs)
+{
+ int m;
+
+ for (m = 0; m < tcs->num_tcs; m++) {
+ if (tcs_is_free(tcs->drv, tcs->tcs_offset + m))
+ break;
+ }
+
+ return (m != tcs->num_tcs) ? m : -EBUSY;
+}
+
+static int tcs_mbox_write(struct rsc_drv *drv, struct tcs_request *msg)
+{
+ struct tcs_group *tcs;
+ int m;
+ struct tcs_response *resp = NULL;
+ unsigned long flags;
+ int ret = 0;
+
+ tcs = get_tcs_for_msg(drv, msg);
+ if (IS_ERR(tcs))
+ return PTR_ERR(tcs);
+
+ spin_lock_irqsave(&tcs->tcs_lock, flags);
+ m = find_free_tcs(tcs);
+ if (m < 0) {
+ ret = m;
+ goto done_write;
+ }
+
+ /*
+ * The h/w does not like if we send a request to the same address,
+ * when one is already in-flight or bring processed.
+ */
+ ret = check_for_req_inflight(drv, tcs, msg);
+ if (ret)
+ goto done_write;
+
+ resp = setup_response(drv, msg, m);
+ if (IS_ERR_OR_NULL(resp)) {
+ ret = PTR_ERR(resp);
+ goto done_write;
+ }
+ resp->m = m;
+
+ atomic_set(&drv->tcs_in_use[m], 1);
+ __tcs_buffer_write(drv, m, 0, msg);
+ __tcs_trigger(drv, m);
+
+done_write:
+ spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+ return ret;
+}
+
+/**
+ * rpmh_rsc_send_data: Validate the incoming message and write to the
+ * appropriate TCS block.
+ *
+ * @drv: the controller
+ * @msg: the data to be sent
+ *
+ * Return: 0 on success, -EINVAL on error.
+ * Note: This call blocks until a valid data is written to the TCS.
+ */
+int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg)
+{
+ int ret = 0;
+
+ if (!msg || !msg->payload || !msg->num_payload ||
+ msg->num_payload > MAX_RPMH_PAYLOAD)
+ return -EINVAL;
+
+ do {
+ ret = tcs_mbox_write(drv, msg);
+ if (ret == -EBUSY) {
+ pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=0x%x\n",
+ msg->payload[0].addr);
+ udelay(10);
+ }
+ } while (ret == -EBUSY);
+
+ return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_send_data);
+
+static int rpmh_probe_tcs_config(struct platform_device *pdev,
+ struct rsc_drv *drv)
+{
+ struct tcs_type_config {
+ u32 type;
+ u32 n;
+ } tcs_cfg[TCS_TYPE_NR] = { { 0 } };
+ struct device_node *dn = pdev->dev.of_node;
+ u32 config, max_tcs, ncpt;
+ int i, ret, n, st = 0;
+ struct tcs_group *tcs;
+ struct resource *res;
+ void __iomem *base;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "drv");
+ if (!res)
+ return -EINVAL;
+
+ base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcs");
+ if (!res)
+ return -EINVAL;
+
+ drv->tcs_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drv->tcs_base))
+ return PTR_ERR(drv->tcs_base);
+
+ config = readl_relaxed(base + DRV_PRNT_CHLD_CONFIG);
+
+ max_tcs = config;
+ max_tcs &= (DRV_NUM_TCS_MASK << (DRV_NUM_TCS_SHIFT * drv->drv_id));
+ max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
+
+ ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
+ ncpt = ncpt >> DRV_NCPT_SHIFT;
+
+ n = of_property_count_elems_of_size(dn, "qcom,tcs-config",
+ sizeof(u32));
+ if (n != 2 * TCS_TYPE_NR)
+ return -EINVAL;
+
+ for (i = 0; i < TCS_TYPE_NR; i++) {
+ ret = of_property_read_u32_index(dn, "qcom,tcs-config",
+ i * 2, &tcs_cfg[i].type);
+ if (ret)
+ return ret;
+ if (tcs_cfg[i].type >= TCS_TYPE_NR)
+ return -EINVAL;
+
+ ret = of_property_read_u32_index(dn, "qcom,tcs-config",
+ i * 2 + 1, &tcs_cfg[i].n);
+ if (ret)
+ return ret;
+ if (tcs_cfg[i].n > MAX_TCS_PER_TYPE)
+ return -EINVAL;
+ }
+
+ for (i = 0; i < TCS_TYPE_NR; i++) {
+ tcs = &drv->tcs[tcs_cfg[i].type];
+ if (tcs->drv)
+ return -EINVAL;
+ tcs->drv = drv;
+ tcs->type = tcs_cfg[i].type;
+ tcs->num_tcs = tcs_cfg[i].n;
+ tcs->ncpt = ncpt;
+ spin_lock_init(&tcs->tcs_lock);
+
+ if (!tcs->num_tcs || tcs->type == CONTROL_TCS)
+ continue;
+
+ if (st + tcs->num_tcs > max_tcs ||
+ st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
+ return -EINVAL;
+
+ tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
+ tcs->tcs_offset = st;
+ st += tcs->num_tcs;
+ }
+
+ drv->num_tcs = st;
+
+ return 0;
+}
+
+static int rpmh_rsc_probe(struct platform_device *pdev)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ struct rsc_drv *drv;
+ int i, ret, irq;
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
+ if (ret)
+ return ret;
+
+ drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
+ if (!drv->name)
+ drv->name = dev_name(&pdev->dev);
+
+ ret = rpmh_probe_tcs_config(pdev, drv);
+ if (ret)
+ return ret;
+
+ INIT_LIST_HEAD(&drv->response_pending);
+ spin_lock_init(&drv->drv_lock);
+ tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
+
+ for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
+ atomic_set(&drv->tcs_in_use[i], 0);
+
+ irq = of_irq_get(dn, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
+ IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND,
+ drv->name, drv);
+ if (ret)
+ return ret;
+
+ /* Enable the active TCS to send requests immediately */
+ write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0,
+ drv->tcs[ACTIVE_TCS].tcs_mask);
+
+ return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id rpmh_drv_match[] = {
+ { .compatible = "qcom,rpmh-rsc", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, rpm_drv_match);
+
+static struct platform_driver rpmh_driver = {
+ .probe = rpmh_rsc_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = rpmh_drv_match,
+ },
+};
+
+static int __init rpmh_driver_init(void)
+{
+ return platform_driver_register(&rpmh_driver);
+}
+arch_initcall(rpmh_driver_init);
diff --git a/include/dt-bindings/soc/qcom,rpmh-rsc.h b/include/dt-bindings/soc/qcom,rpmh-rsc.h
new file mode 100644
index 000000000000..868f998ea998
--- /dev/null
+++ b/include/dt-bindings/soc/qcom,rpmh-rsc.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __DT_QCOM_RPMH_RSC_H__
+#define __DT_QCOM_RPMH_RSC_H__
+
+#define SLEEP_TCS 0
+#define WAKE_TCS 1
+#define ACTIVE_TCS 2
+#define CONTROL_TCS 3
+
+#endif /* __DT_QCOM_RPMH_RSC_H__ */
diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
new file mode 100644
index 000000000000..9465f0560f7a
--- /dev/null
+++ b/include/soc/qcom/tcs.h
@@ -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
+ */
+struct tcs_request {
+ enum rpmh_state state;
+ bool is_complete;
+ u32 num_payload;
+ struct tcs_cmd *payload;
+};
+
+#endif /* __SOC_QCOM_TCS_H__ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
2018-03-02 16:43 ` [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs Lina Iyer
@ 2018-03-06 19:45 ` Stephen Boyd
2018-03-09 21:33 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-06 19:45 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:08)
> 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
s/voltages etc/voltages, etc/
> 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
s/reques /request/ ?
> 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 <ilina@codeaurora.org>
> ---
> drivers/soc/qcom/Kconfig | 10 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/rpmh-internal.h | 87 +++++
> 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, 761 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
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index e050eb83341d..779f0d14748d 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -55,6 +55,16 @@ config QCOM_RMTFS_MEM
>
> Say y here if you intend to boot the modem remoteproc.
>
> +config QCOM_RPMH
> + bool "Qualcomm RPM-Hardened (RPMH) Communication"
No module?
> + depends on ARCH_QCOM && ARM64 && OF
Add support for compile test?
> + help
> + Support for communication with the hardened-RPM blocks in
> + Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
> + internal bus to transmit state requests for shared resources. A set
> + of hardware components aggregate requests for these resources and
> + help apply the aggregated state on the resource.
> +
> config QCOM_SMEM
> tristate "Qualcomm Shared Memory Manager (SMEM)"
> depends on ARCH_QCOM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index dcebf2814e6d..e7d04f0e3616 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> +obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o
Keep alphabetically sorted?
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> new file mode 100644
> index 000000000000..12faec77c4f3
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -0,0 +1,87 @@
> +/* 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>
> +
> +#define TCS_TYPE_NR 4
> +#define MAX_CMDS_PER_TCS 16
> +#define MAX_TCS_PER_TYPE 3
> +#define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
> +
> +struct rsc_drv;
> +
> +/**
> + * tcs_response: Response object for a request
struct tcs_response -
> + *
> + * @drv: the controller
> + * @msg: the request for this response
> + * @m: the tcs identifier
> + * @err: error reported in the response
> + * @list: link list object.
What is it linked to? Also drop full-stop please.
> + */
> +struct tcs_response {
> + struct rsc_drv *drv;
> + struct tcs_request *msg;
> + u32 m;
> + int err;
> + struct list_head list;
> +};
> +
> +/**
> + * tcs_group: group of Trigger Command Sets for a request state
> + *
> + * @drv: the controller
> + * @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;
> + u32 tcs_mask;
> + u32 tcs_offset;
Maybe just mask? and offset? tcs is already in the struct name.
> + int num_tcs;
> + int ncpt;
num_cmd_per_tcs? It doesn't actually look to be used though.
> + spinlock_t tcs_lock;
> + struct tcs_response *responses[MAX_TCS_PER_TYPE];
const perhaps? Doubtful that responses would be changed after they're
filled in.
> +};
> +
> +/**
> + * rsc_drv: the Resource State Coordinator controller
> + *
> + * @name: controller identifier
> + * @tcs_base: start address of the TCS registers in this controller
> + * @drv_id: instance id in the controller (Direct Resource Voter)
just 'id'? 'drv' is already in the name of the struct.
> + * @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];
Why not a bitmap with atomic set on the bit?
> + spinlock_t drv_lock;
> +};
> +
> +
> +int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg);
const msg?
> +
> +#endif /* __RPM_INTERNAL_H__ */
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> new file mode 100644
> index 000000000000..6cb91a0114ee
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -0,0 +1,593 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
> +
> +#include <linux/atomic.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
If the driver doesn't become tristate, this should become export.h
instead of module.h
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include <asm-generic/io.h>
No. Use <linux/io.h> instead.
> +#include <soc/qcom/tcs.h>
> +#include <dt-bindings/soc/qcom,rpmh-rsc.h>
> +
> +#include "rpmh-internal.h"
> +
> +#define RSC_DRV_TCS_OFFSET 672
> +#define RSC_DRV_CMD_OFFSET 20
> +
> +/* DRV Configuration Information Register */
> +#define DRV_PRNT_CHLD_CONFIG 0x0C
> +#define DRV_NUM_TCS_MASK 0x3F
> +#define DRV_NUM_TCS_SHIFT 6
> +#define DRV_NCPT_MASK 0x1F
> +#define DRV_NCPT_SHIFT 27
> +
> +/* Register offsets */
> +#define RSC_DRV_IRQ_ENABLE 0x00
> +#define RSC_DRV_IRQ_STATUS 0x04
> +#define RSC_DRV_IRQ_CLEAR 0x08
> +#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10
> +#define RSC_DRV_CONTROL 0x14
> +#define RSC_DRV_STATUS 0x18
> +#define RSC_DRV_CMD_ENABLE 0x1C
> +#define RSC_DRV_CMD_MSGID 0x30
> +#define RSC_DRV_CMD_ADDR 0x34
> +#define RSC_DRV_CMD_DATA 0x38
> +#define RSC_DRV_CMD_STATUS 0x3C
> +#define RSC_DRV_CMD_RESP_DATA 0x40
> +
> +#define TCS_AMC_MODE_ENABLE BIT(16)
> +#define TCS_AMC_MODE_TRIGGER BIT(24)
> +
> +/* TCS CMD register bit mask */
> +#define CMD_MSGID_LEN 8
> +#define CMD_MSGID_RESP_REQ BIT(8)
> +#define CMD_MSGID_WRITE BIT(16)
> +#define CMD_STATUS_ISSUED BIT(8)
> +#define CMD_STATUS_COMPL BIT(16)
> +
> +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;
return tcs?
> + }
> +
> + if (i == drv->num_tcs) {
> + WARN(1, "Incorrect TCS index %d", m);
> + tcs = NULL;
> + }
And then unconditional WARN and return 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);
Why not kzalloc?
> + 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.
> + */
Add a spinlock assert on tcs_lock for this?
> + 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);
> +}
> +
> +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);
Hopefully this never gets stuck. Add a timeout?
> + }
> +}
> +
> +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);
> + }
> +
> + 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;
Impossible, resp was just dereferenced two lines above.
> +
> + 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;
> +
> + 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);
resp->err = 0;
> + if (!resp) {
> + WARN_ON(1);
if (WARN_ON(!resp)) {
> + 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)))) {
> + err = -EIO;
resp->err = -EIO;
> + break;
> + }
> + }
> +
> + resp->err = err;
Drop.
> +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;
> +}
> +
> +/**
> + * tcs_notify_tx_done: TX Done for requests that got a response
> + *
> + * @data: the tasklet argument
> + *
> + * Tasklet function to notify MBOX that we are done with the request.
> + * Handles all pending reponses whenever run.
> + */
> +static void tcs_notify_tx_done(unsigned long data)
> +{
> + struct rsc_drv *drv = (struct rsc_drv *)data;
> + struct tcs_response *resp;
> + unsigned long flags;
> +
> + for (;;) {
> + spin_lock_irqsave(&drv->drv_lock, flags);
> + if (list_empty(&drv->response_pending)) {
> + spin_unlock_irqrestore(&drv->drv_lock, flags);
> + break;
> + }
> + resp = list_first_entry(&drv->response_pending,
> + struct tcs_response, list);
Maybe this could just be:
spin_lock_irqsave(&drv->drv_lock, flags);
resp = list_first_entry_or_null(&drv->response_pending,
struct tcs_response, list);
if (!resp) {
spin_unlock_irqrestore(&drv->drv_lock, flags);
break;
}
> + list_del(&resp->list);
> + spin_unlock_irqrestore(&drv->drv_lock, flags);
> + free_response(resp);
But all this function does is free the structure? Will it do more later?
> + }
> +}
> +
> +static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
> + struct tcs_request *msg)
> +{
> + u32 msgid, cmd_msgid = 0;
Drop assignment to 0.
> + u32 cmd_enable = 0;
> + u32 cmd_complete;
> + struct tcs_cmd *cmd;
> + int i, j;
> +
> + cmd_msgid = CMD_MSGID_LEN;
> + cmd_msgid |= (msg->is_complete) ? CMD_MSGID_RESP_REQ : 0;
Drop useless parenthesis.
> + cmd_msgid |= CMD_MSGID_WRITE;
> +
> + cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
> +
> + for (i = 0, j = n; i < msg->num_payload; i++, j++) {
> + cmd = &msg->payload[i];
> + cmd_enable |= BIT(j);
> + cmd_complete |= cmd->complete << j;
> + msgid = cmd_msgid;
> + msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
Drop useless parenthesis.
> + write_tcs_reg(drv, RSC_DRV_CMD_MSGID, m, j, msgid);
> + write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j, cmd->addr);
> + write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, j, cmd->data);
> + }
> +
> + 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);
> +}
> +
> +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);
> + enable &= ~TCS_AMC_MODE_ENABLE;
> + write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
> +
> + /* Enable the AMC mode on the TCS and then trigger the TCS */
> + enable = TCS_AMC_MODE_ENABLE;
> + write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
> + enable |= TCS_AMC_MODE_TRIGGER;
> + write_tcs_reg_sync(drv, RSC_DRV_CONTROL, m, 0, enable);
> +}
> +
> +static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> + struct tcs_request *msg)
> +{
> + unsigned long curr_enabled;
> + u32 addr;
> + int i, j, k;
> + int m = tcs->tcs_offset;
> +
> + for (i = 0; i < tcs->num_tcs; i++, m++) {
> + if (tcs_is_free(drv, m))
> + continue;
> +
> + curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0);
> +
> + for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> + addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j);
> + for (k = 0; k < msg->num_payload; k++) {
> + if (addr == msg->payload[k].addr)
> + return -EBUSY;
> + }
> + }
> + }
There isn't any way to do this in software only? Hopefully this isn't
costly to read the TCS to see if something matches.
> +
> + return 0;
> +}
> +
> +static int find_free_tcs(struct tcs_group *tcs)
> +{
> + int m;
> +
> + for (m = 0; m < tcs->num_tcs; m++) {
> + if (tcs_is_free(tcs->drv, tcs->tcs_offset + m))
> + break;
return m?
> + }
> +
> + return (m != tcs->num_tcs) ? m : -EBUSY;
And then return -EBUSY otherwise?
> +}
> +
> +static int tcs_mbox_write(struct rsc_drv *drv, struct tcs_request *msg)
> +{
> + struct tcs_group *tcs;
> + int m;
> + struct tcs_response *resp = NULL;
> + unsigned long flags;
> + int ret = 0;
Drop assignment.
> +
> + tcs = get_tcs_for_msg(drv, msg);
> + if (IS_ERR(tcs))
> + return PTR_ERR(tcs);
> +
> + spin_lock_irqsave(&tcs->tcs_lock, flags);
> + m = find_free_tcs(tcs);
> + if (m < 0) {
> + ret = m;
> + goto done_write;
> + }
> +
> + /*
> + * The h/w does not like if we send a request to the same address,
> + * when one is already in-flight or bring processed.
s/bring/being/
> + */
> + ret = check_for_req_inflight(drv, tcs, msg);
> + if (ret)
> + goto done_write;
> +
> + resp = setup_response(drv, msg, m);
> + if (IS_ERR_OR_NULL(resp)) {
Looks to only return an error pointer on failure?
> + ret = PTR_ERR(resp);
> + goto done_write;
> + }
> + resp->m = m;
> +
> + atomic_set(&drv->tcs_in_use[m], 1);
> + __tcs_buffer_write(drv, m, 0, msg);
> + __tcs_trigger(drv, m);
> +
> +done_write:
> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
> + return ret;
> +}
> +
> +/**
> + * rpmh_rsc_send_data: Validate the incoming message and write to the
> + * appropriate TCS block.
> + *
> + * @drv: the controller
> + * @msg: the data to be sent
> + *
> + * Return: 0 on success, -EINVAL on error.
> + * Note: This call blocks until a valid data is written to the TCS.
> + */
> +int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg)
> +{
> + int ret = 0;
Drop initial assignment.
> +
> + if (!msg || !msg->payload || !msg->num_payload ||
> + msg->num_payload > MAX_RPMH_PAYLOAD)
> + return -EINVAL;
> +
> + do {
> + ret = tcs_mbox_write(drv, msg);
> + if (ret == -EBUSY) {
> + pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=0x%x\n",
"addr=%#x\n"
> + msg->payload[0].addr);
> + udelay(10);
> + }
> + } while (ret == -EBUSY);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(rpmh_rsc_send_data);
> +
> +static int rpmh_probe_tcs_config(struct platform_device *pdev,
> + struct rsc_drv *drv)
> +{
> + struct tcs_type_config {
> + u32 type;
> + u32 n;
> + } tcs_cfg[TCS_TYPE_NR] = { { 0 } };
> + struct device_node *dn = pdev->dev.of_node;
> + u32 config, max_tcs, ncpt;
> + int i, ret, n, st = 0;
> + struct tcs_group *tcs;
> + struct resource *res;
> + void __iomem *base;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "drv");
> + if (!res)
> + return -EINVAL;
> +
Drop this return -EINVAL stuff and just call devm_ioremap_resource()
with the potentially NULL res in hand.
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcs");
> + if (!res)
> + return -EINVAL;
> +
> + drv->tcs_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(drv->tcs_base))
> + return PTR_ERR(drv->tcs_base);
> +
> + config = readl_relaxed(base + DRV_PRNT_CHLD_CONFIG);
> +
> + max_tcs = config;
> + max_tcs &= (DRV_NUM_TCS_MASK << (DRV_NUM_TCS_SHIFT * drv->drv_id));
Drop one useless parenthesis.
> + max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
> +
> + ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
> + ncpt = ncpt >> DRV_NCPT_SHIFT;
> +
> + n = of_property_count_elems_of_size(dn, "qcom,tcs-config",
> + sizeof(u32));
of_property_count_u32_elems()?
> + if (n != 2 * TCS_TYPE_NR)
> + return -EINVAL;
> +
> + for (i = 0; i < TCS_TYPE_NR; i++) {
> + ret = of_property_read_u32_index(dn, "qcom,tcs-config",
> + i * 2, &tcs_cfg[i].type);
> + if (ret)
> + return ret;
> + if (tcs_cfg[i].type >= TCS_TYPE_NR)
> + return -EINVAL;
> +
> + ret = of_property_read_u32_index(dn, "qcom,tcs-config",
> + i * 2 + 1, &tcs_cfg[i].n);
> + if (ret)
> + return ret;
> + if (tcs_cfg[i].n > MAX_TCS_PER_TYPE)
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < TCS_TYPE_NR; i++) {
> + tcs = &drv->tcs[tcs_cfg[i].type];
> + if (tcs->drv)
> + return -EINVAL;
> + tcs->drv = drv;
> + tcs->type = tcs_cfg[i].type;
> + tcs->num_tcs = tcs_cfg[i].n;
> + tcs->ncpt = ncpt;
> + spin_lock_init(&tcs->tcs_lock);
> +
> + if (!tcs->num_tcs || tcs->type == CONTROL_TCS)
> + continue;
> +
> + if (st + tcs->num_tcs > max_tcs ||
> + st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
What is 8? BITS_PER_BYTE?
> + return -EINVAL;
> +
> + tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
> + tcs->tcs_offset = st;
> + st += tcs->num_tcs;
> + }
> +
> + drv->num_tcs = st;
> +
> + return 0;
> +}
> +
> +static int rpmh_rsc_probe(struct platform_device *pdev)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct rsc_drv *drv;
> + int i, ret, irq;
> +
> + drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
> + if (!drv)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
> + if (ret)
> + return ret;
> +
> + drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
> + if (!drv->name)
> + drv->name = dev_name(&pdev->dev);
> +
> + ret = rpmh_probe_tcs_config(pdev, drv);
> + if (ret)
> + return ret;
> +
> + INIT_LIST_HEAD(&drv->response_pending);
> + spin_lock_init(&drv->drv_lock);
> + tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
> +
> + for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
> + atomic_set(&drv->tcs_in_use[i], 0);
> +
> + irq = of_irq_get(dn, 0);
> + if (irq < 0)
> + return irq;
This is a platform device, please use platform device APIs to get
resources like irqs.
> +
> + ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
> + IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND,
> + drv->name, drv);
> + if (ret)
> + return ret;
> +
> + /* Enable the active TCS to send requests immediately */
> + write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0,
> + drv->tcs[ACTIVE_TCS].tcs_mask);
> +
> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
devm_of_platform_populate()?
> +}
> +
> +static const struct of_device_id rpmh_drv_match[] = {
> + { .compatible = "qcom,rpmh-rsc", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rpm_drv_match);
> +
> +static struct platform_driver rpmh_driver = {
> + .name = KBUILD_MODNAME,
Can you just put the name you want?
> + .of_match_table = rpmh_drv_match,
> + },
> +};
> +
> +static int __init rpmh_driver_init(void)
> +{
> + return platform_driver_register(&rpmh_driver);
> +}
> +arch_initcall(rpmh_driver_init);
> diff --git a/include/dt-bindings/soc/qcom,rpmh-rsc.h b/include/dt-bindings/soc/qcom,rpmh-rsc.h
> new file mode 100644
> index 000000000000..868f998ea998
> --- /dev/null
> +++ b/include/dt-bindings/soc/qcom,rpmh-rsc.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __DT_QCOM_RPMH_RSC_H__
> +#define __DT_QCOM_RPMH_RSC_H__
> +
> +#define SLEEP_TCS 0
> +#define WAKE_TCS 1
> +#define ACTIVE_TCS 2
> +#define CONTROL_TCS 3
> +
> +#endif /* __DT_QCOM_RPMH_RSC_H__ */
> diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
> new file mode 100644
> index 000000000000..9465f0560f7a
> --- /dev/null
> +++ b/include/soc/qcom/tcs.h
> @@ -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;
Maybe 'wait' instead of 'complete'?
> +};
> +
> +/**
> + * tcs_request: A set of tcs_cmds sent togther in a TCS
s/togther/together/
> + *
> + * @state: state for the request.
state for the request to apply to?
> + * @is_complete: expect a response from the h/w accelerator
is_complete sounds like we're going to read this variable? But this
looks to be more like 'wait' to indicate that we want to wait for
response if this bool is true? Rename to 'wait'?
> + * @num_payload: the number of tcs_cmds in thsi payload
s/thsi/this/
> + * @payload: an array of tcs_cmds
> + */
> +struct tcs_request {
> + enum rpmh_state state;
> + bool is_complete;
> + u32 num_payload;
num_cmds?
> + struct tcs_cmd *payload;
cmds?
> +};
> +
> +#endif /* __SOC_QCOM_TCS_H__ */
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
2018-03-06 19:45 ` Stephen Boyd
@ 2018-03-09 21:33 ` Lina Iyer
2018-03-09 21:37 ` Stephen Boyd
0 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-09 21:33 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
Hi Stephen,
I will address all the comments in the next spin of the patch. Here are
some responses to the questions.
On Tue, Mar 06 2018 at 12:45 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:08)
[...]
> +#include <linux/module.h>
>
>If the driver doesn't become tristate, this should become export.h
>instead of module.h
>
MODULE_DEVICE_TABLE seems to need this.
[...]
>> +
>> +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);
>
>Hopefully this never gets stuck. Add a timeout?
>
No, it wont. The read is to make sure that the write went through before
we exit this call.
[...]
>> + list_del(&resp->list);
>> + spin_unlock_irqrestore(&drv->drv_lock, flags);
>> + free_response(resp);
>
>But all this function does is free the structure? Will it do more later?
>
Hopefully, I would like to use a pre-allocateed pool instead of alloc
and free.
> + for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
>> + addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j);
>> + for (k = 0; k < msg->num_payload; k++) {
>> + if (addr == msg->payload[k].addr)
>> + return -EBUSY;
>> + }
>> + }
>> + }
>
>There isn't any way to do this in software only? Hopefully this isn't
>costly to read the TCS to see if something matches.
>
It is, but not too expensive. The alternatives involves more locking..
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
2018-03-09 21:33 ` Lina Iyer
@ 2018-03-09 21:37 ` Stephen Boyd
0 siblings, 0 replies; 36+ messages in thread
From: Stephen Boyd @ 2018-03-09 21:37 UTC (permalink / raw)
To: Lina Iyer
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Fri, Mar 9, 2018 at 1:33 PM, Lina Iyer <ilina@codeaurora.org> wrote:
> Hi Stephen,
>
> I will address all the comments in the next spin of the patch. Here are
> some responses to the questions.
>
> On Tue, Mar 06 2018 at 12:45 -0700, Stephen Boyd wrote:
>>
>> Quoting Lina Iyer (2018-03-02 08:43:08)
>
> [...]
>>
>> +#include <linux/module.h>
>>
>> If the driver doesn't become tristate, this should become export.h
>> instead of module.h
>>
> MODULE_DEVICE_TABLE seems to need this.
>
If you don't have tristate then MODULE_DEVICE_TABLE isn't needed.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
2018-03-02 16:43 ` [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-06 22:30 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Lina Iyer
` (7 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer, devicetree
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: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v2:
- Amend text to describe the registers in reg property
- Add reg-names for the registers
- Update examples to use GIC_SPI in interrupts instead of 0
- Rephrase incorrect description
Changes in v3:
- Fix unwanted capitalization
- Remove clients from the examples, this doc does not describe
them
- Rephrase introductory paragraph
- Remove hardware specifics from DT bindings
---
.../devicetree/bindings/arm/msm/rpmh-rsc.txt | 131 +++++++++++++++++++++
1 file changed, 131 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
diff --git a/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
new file mode 100644
index 000000000000..afd3817cc615
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
@@ -0,0 +1,131 @@
+RPMH RSC:
+------------
+
+Resource Power Manager Hardened (RPMH) is the mechanism for communicating with
+the hardened resource accelerators on Qualcomm SoCs. Requests to the resources
+can be written to the Trigger Command Set (TCS) 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.
+
+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.
+
+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
+ ACTIVE_TCS
+ CONTROL_TCS
+ - Cell #2 (Number of TCS): <u32>
+
+- label:
+ Usage: optional
+ Value type: <string>
+ Definition: Name for the RSC. The name would be used in trace logs.
+
+Drivers that want to use the RSC to communicate with RPMH must specify their
+bindings as child of the RSC controllers they wish to communicate with.
+
+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>;
+ reg-names = "drv", "tcs";
+ interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+ qcom,drv-id = <2>;
+ qcom,tcs-config = <SLEEP_TCS 3>,
+ <WAKE_TCS 3>,
+ <ACTIVE_TCS 2>,
+ <CONTROL_TCS 1>;
+ };
+
+Example 2:
+
+For a TCS whose RSC base address is 0xAF20000 and is at DRV id of 0, the
+register offsets for DRV0 start at 01C00, the register calculations are like
+this -
+First tuple: 0xAF20000
+Second tuple: 0xAF20000 + 0x1C00 = 0xAF21C00
+
+ disp_rsc: rsc@af20000 {
+ label = "disp_rsc";
+ compatible = "qcom,rpmh-rsc";
+ reg = <0xaf20000 0x10000>, <0xaf21c00 0x3000>;
+ reg-names = "drv", "tcs";
+ interrupts = <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>;
+ qcom,drv-id = <0>;
+ qcom,tcs-config = <SLEEP_TCS 1>,
+ <WAKE_TCS 1>,
+ <ACTIVE_TCS 0>,
+ <CONTROL_TCS 0>;
+ };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs
2018-03-02 16:43 ` [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs Lina Iyer
@ 2018-03-06 22:30 ` Stephen Boyd
2018-03-07 20:54 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-06 22:30 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer, devicetree
Quoting Lina Iyer (2018-03-02 08:43:09)
> Add device binding documentation for Qualcomm Technology Inc's RPMH RSC
> driver. The hardware block is used for communicating resource state
s/driver/hardware/
> requests for shared resources.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>
> Changes in v2:
> - Amend text to describe the registers in reg property
> - Add reg-names for the registers
> - Update examples to use GIC_SPI in interrupts instead of 0
> - Rephrase incorrect description
>
> Changes in v3:
> - Fix unwanted capitalization
> - Remove clients from the examples, this doc does not describe
> them
> - Rephrase introductory paragraph
> - Remove hardware specifics from DT bindings
> ---
> .../devicetree/bindings/arm/msm/rpmh-rsc.txt | 131 +++++++++++++++++++++
> 1 file changed, 131 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
> new file mode 100644
> index 000000000000..afd3817cc615
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
Shouldn't this go into bindings/soc/qcom/ ?
> @@ -0,0 +1,131 @@
> +RPMH RSC:
> +------------
> +
> +Resource Power Manager Hardened (RPMH) is the mechanism for communicating with
> +the hardened resource accelerators on Qualcomm SoCs. Requests to the resources
> +can be written to the Trigger Command Set (TCS) 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.
> +
> +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
s/subsystem/subsystems/
s/Drivers/Clients/ ?
> +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.
Not sure what this last sentence has to do with the DT binding. We
generally try to avoid saying 'Linux' or 'driver' in DT binding
documents, because they usually document hardware.
> +
> +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
optional?
> + 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
> + ACTIVE_TCS
> + CONTROL_TCS
> + - Cell #2 (Number of TCS): <u32>
> +
> +- label:
> + Usage: optional
> + Value type: <string>
> + Definition: Name for the RSC. The name would be used in trace logs.
> +
> +Drivers that want to use the RSC to communicate with RPMH must specify their
> +bindings as child of the RSC controllers they wish to communicate with.
Is that going to work for drivers that want to talk to two or more RSCs?
I suppose that they'll have to hook up into some sort of framework like
clk or regulator and then drivers that want to use two RSCs for those
things would be linked to those sub-device drivers with the normal clk
or regulator bindings?
> +
> +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>;
> + reg-names = "drv", "tcs";
> + interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> + qcom,drv-id = <2>;
> + qcom,tcs-config = <SLEEP_TCS 3>,
> + <WAKE_TCS 3>,
> + <ACTIVE_TCS 2>,
> + <CONTROL_TCS 1>;
Could qcom,tcs-config become something more like below?
#qcom,sleep-tcs = <3>;
#qcom,wake-tcs = <3>;
#qcom,active-tcs = <2>;
#qcom,control-tcs = <1>;
I don't really understand the binding design to have many cells with the
*_TCS defines indicating what comes next.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs
2018-03-06 22:30 ` Stephen Boyd
@ 2018-03-07 20:54 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-07 20:54 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel, devicetree
On Tue, Mar 06 2018 at 15:30 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:09)
>> Add device binding documentation for Qualcomm Technology Inc's RPMH RSC
>> driver. The hardware block is used for communicating resource state
>
>s/driver/hardware/
>
Ok.
>> requests for shared resources.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>
>> Changes in v2:
>> - Amend text to describe the registers in reg property
>> - Add reg-names for the registers
>> - Update examples to use GIC_SPI in interrupts instead of 0
>> - Rephrase incorrect description
>>
>> Changes in v3:
>> - Fix unwanted capitalization
>> - Remove clients from the examples, this doc does not describe
>> them
>> - Rephrase introductory paragraph
>> - Remove hardware specifics from DT bindings
>> ---
>> .../devicetree/bindings/arm/msm/rpmh-rsc.txt | 131 +++++++++++++++++++++
>> 1 file changed, 131 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
>> new file mode 100644
>> index 000000000000..afd3817cc615
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt
>
>Shouldn't this go into bindings/soc/qcom/ ?
>
Didn;t realize the location. Thanks for pointing out.
> +
>> +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
>
>s/subsystem/subsystems/
>s/Drivers/Clients/ ?
>
Ok
>> +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.
>
>Not sure what this last sentence has to do with the DT binding. We
>generally try to avoid saying 'Linux' or 'driver' in DT binding
>documents, because they usually document hardware.
>
This para can go away.
>> +
>> +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
>
>optional?
>
No, it is required. Driver has been using the by_name for clarity.
>> + 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
>> + ACTIVE_TCS
>> + CONTROL_TCS
>> + - Cell #2 (Number of TCS): <u32>
>> +
>> +- label:
>> + Usage: optional
>> + Value type: <string>
>> + Definition: Name for the RSC. The name would be used in trace logs.
>> +
>> +Drivers that want to use the RSC to communicate with RPMH must specify their
>> +bindings as child of the RSC controllers they wish to communicate with.
>
>Is that going to work for drivers that want to talk to two or more RSCs?
>I suppose that they'll have to hook up into some sort of framework like
>clk or regulator and then drivers that want to use two RSCs for those
>things would be linked to those sub-device drivers with the normal clk
>or regulator bindings?
>
Correct. This follows the same model as RPM SMD driver.
>> +
>> +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>;
>> + reg-names = "drv", "tcs";
>> + interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
>> + qcom,drv-id = <2>;
>> + qcom,tcs-config = <SLEEP_TCS 3>,
>> + <WAKE_TCS 3>,
>> + <ACTIVE_TCS 2>,
>> + <CONTROL_TCS 1>;
>
>Could qcom,tcs-config become something more like below?
>
> #qcom,sleep-tcs = <3>;
> #qcom,wake-tcs = <3>;
> #qcom,active-tcs = <2>;
> #qcom,control-tcs = <1>;
>
>I don't really understand the binding design to have many cells with the
>*_TCS defines indicating what comes next.
>
This format helps preserve the order in which the TCS are designed in
the firmware. Thats additional information that is described by the
cells.
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
2018-03-02 16:43 ` [PATCH v3 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs Lina Iyer
2018-03-02 16:43 ` [PATCH v3 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-06 5:38 ` kbuild test robot
2018-03-02 16:43 ` [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions Lina Iyer
` (6 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer, Steven Rostedt
Log sent RPMH requests and interrupt responses in FTRACE.
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
- Use __string() instead of char *
- fix TRACE_INCLUDE_PATH
---
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
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index e7d04f0e3616..bb013d56b5a1 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+CFLAGS_rpmh-rsc.o := -I$(src)
obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o
obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 6cb91a0114ee..4322e23a2ba6 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -24,6 +24,9 @@
#include "rpmh-internal.h"
+#define CREATE_TRACE_POINTS
+#include "trace-rpmh.h"
+
#define RSC_DRV_TCS_OFFSET 672
#define RSC_DRV_CMD_OFFSET 20
@@ -239,6 +242,7 @@ static irqreturn_t tcs_irq_handler(int irq, void *p)
/* 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));
+ trace_rpmh_notify_irq(drv, resp);
atomic_set(&drv->tcs_in_use[m], 0);
send_tcs_response(resp);
}
@@ -270,6 +274,7 @@ static void tcs_notify_tx_done(unsigned long data)
struct tcs_response, list);
list_del(&resp->list);
spin_unlock_irqrestore(&drv->drv_lock, flags);
+ trace_rpmh_notify_tx_done(drv, resp);
free_response(resp);
}
}
@@ -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, j, msgid);
write_tcs_reg(drv, RSC_DRV_CMD_ADDR, m, j, cmd->addr);
write_tcs_reg(drv, RSC_DRV_CMD_DATA, m, j, cmd->data);
+ trace_rpmh_send_msg(drv, m, j, msgid, cmd);
}
write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
diff --git a/drivers/soc/qcom/trace-rpmh.h b/drivers/soc/qcom/trace-rpmh.h
new file mode 100644
index 000000000000..b23ad8457224
--- /dev/null
+++ b/drivers/soc/qcom/trace-rpmh.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#if !defined(_TRACE_RPMH_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_RPMH_H
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM rpmh
+
+#include <linux/tracepoint.h>
+#include "rpmh-internal.h"
+
+DECLARE_EVENT_CLASS(rpmh_notify,
+
+ TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
+
+ TP_ARGS(d, r),
+
+ TP_STRUCT__entry(
+ __string(name, d->name)
+ __field(int, m)
+ __field(u32, addr)
+ __field(int, errno)
+ ),
+
+ TP_fast_assign(
+ __assign_string(name, d->name);
+ __entry->m = r->m;
+ __entry->addr = r->msg->payload[0].addr;
+ __entry->errno = r->err;
+ ),
+
+ TP_printk("%s: ack: tcs-m:%d addr: 0x%08x errno: %d",
+ __get_str(name), __entry->m, __entry->addr, __entry->errno)
+);
+
+DEFINE_EVENT(rpmh_notify, rpmh_notify_irq,
+ TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
+ TP_ARGS(d, r)
+);
+
+DEFINE_EVENT(rpmh_notify, rpmh_notify_tx_done,
+ TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
+ TP_ARGS(d, r)
+);
+
+
+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(
+ __string(name, d->name)
+ __field(int, m)
+ __field(int, n)
+ __field(u32, hdr)
+ __field(u32, addr)
+ __field(u32, data)
+ __field(bool, complete)
+ ),
+
+ TP_fast_assign(
+ __assign_string(name, d->name);
+ __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",
+ __get_str(name), __entry->m, __entry->n, __entry->hdr,
+ __entry->addr, __entry->data, __entry->complete)
+);
+
+#endif /* _TRACE_RPMH_H */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace-rpmh
+
+#include <trace/define_trace.h>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-02 16:43 ` [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Lina Iyer
@ 2018-03-06 5:38 ` kbuild test robot
2018-03-06 21:47 ` Steven Rostedt
0 siblings, 1 reply; 36+ messages in thread
From: kbuild test robot @ 2018-03-06 5:38 UTC (permalink / raw)
To: Lina Iyer
Cc: kbuild-all, andy.gross, david.brown, linux-arm-msm, linux-soc,
rnayak, bjorn.andersson, linux-kernel, Lina Iyer, Steven Rostedt
[-- Attachment #1: Type: text/plain, Size: 6797 bytes --]
Hi Lina,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.16-rc4 next-20180306]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Lina-Iyer/drivers-qcom-add-RPMH-communication-support/20180305-225623
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All error/warnings (new ones prefixed by >>):
In file included from include/trace/define_trace.h:96:0,
from drivers/soc/qcom/trace-rpmh.h:89,
from drivers/soc/qcom/rpmh-rsc.c:28:
drivers/soc/qcom/./trace-rpmh.h: In function 'trace_event_raw_event_rpmh_notify':
>> drivers/soc/qcom/./trace-rpmh.h:29:3: error: implicit declaration of function '__assign_string'; did you mean '__assign_str'? [-Werror=implicit-function-declaration]
__assign_string(name, d->name);
^
include/trace/trace_events.h:719:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:28:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:29:19: error: 'name' undeclared (first use in this function); did you mean 'node'?
__assign_string(name, d->name);
^
include/trace/trace_events.h:719:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:28:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
drivers/soc/qcom/./trace-rpmh.h:29:19: note: each undeclared identifier is reported only once for each function it appears in
__assign_string(name, d->name);
^
include/trace/trace_events.h:719:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:28:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
drivers/soc/qcom/./trace-rpmh.h: In function 'trace_event_raw_event_rpmh_send_msg':
drivers/soc/qcom/./trace-rpmh.h:67:19: error: 'name' undeclared (first use in this function); did you mean 'node'?
__assign_string(name, d->name);
^
include/trace/trace_events.h:719:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
include/trace/trace_events.h:78:9: note: in expansion of macro 'PARAMS'
PARAMS(assign), \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:50:1: note: in expansion of macro 'TRACE_EVENT'
TRACE_EVENT(rpmh_send_msg,
^~~~~~~~~~~
drivers/soc/qcom/./trace-rpmh.h:66:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
In file included from include/trace/define_trace.h:97:0,
from drivers/soc/qcom/trace-rpmh.h:89,
from drivers/soc/qcom/rpmh-rsc.c:28:
drivers/soc/qcom/./trace-rpmh.h: In function 'perf_trace_rpmh_notify':
>> drivers/soc/qcom/./trace-rpmh.h:29:19: error: 'name' undeclared (first use in this function); did you mean 'node'?
__assign_string(name, d->name);
^
include/trace/perf.h:66:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:28:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
drivers/soc/qcom/./trace-rpmh.h: In function 'perf_trace_rpmh_send_msg':
drivers/soc/qcom/./trace-rpmh.h:67:19: error: 'name' undeclared (first use in this function); did you mean 'node'?
__assign_string(name, d->name);
^
include/trace/perf.h:66:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
{ assign; } \
^~~~~~
include/trace/trace_events.h:78:9: note: in expansion of macro 'PARAMS'
PARAMS(assign), \
^~~~~~
>> drivers/soc/qcom/./trace-rpmh.h:50:1: note: in expansion of macro 'TRACE_EVENT'
TRACE_EVENT(rpmh_send_msg,
^~~~~~~~~~~
drivers/soc/qcom/./trace-rpmh.h:66:2: note: in expansion of macro 'TP_fast_assign'
TP_fast_assign(
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +29 drivers/soc/qcom/./trace-rpmh.h
16
17 TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
18
19 TP_ARGS(d, r),
20
21 TP_STRUCT__entry(
22 __string(name, d->name)
23 __field(int, m)
24 __field(u32, addr)
25 __field(int, errno)
26 ),
27
> 28 TP_fast_assign(
> 29 __assign_string(name, d->name);
30 __entry->m = r->m;
31 __entry->addr = r->msg->payload[0].addr;
32 __entry->errno = r->err;
33 ),
34
35 TP_printk("%s: ack: tcs-m:%d addr: 0x%08x errno: %d",
36 __get_str(name), __entry->m, __entry->addr, __entry->errno)
37 );
38
39 DEFINE_EVENT(rpmh_notify, rpmh_notify_irq,
40 TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
41 TP_ARGS(d, r)
42 );
43
44 DEFINE_EVENT(rpmh_notify, rpmh_notify_tx_done,
45 TP_PROTO(struct rsc_drv *d, struct tcs_response *r),
46 TP_ARGS(d, r)
47 );
48
49
> 50 TRACE_EVENT(rpmh_send_msg,
51
52 TP_PROTO(struct rsc_drv *d, int m, int n, u32 h, struct tcs_cmd *c),
53
54 TP_ARGS(d, m, n, h, c),
55
56 TP_STRUCT__entry(
57 __string(name, d->name)
58 __field(int, m)
59 __field(int, n)
60 __field(u32, hdr)
61 __field(u32, addr)
62 __field(u32, data)
63 __field(bool, complete)
64 ),
65
66 TP_fast_assign(
> 67 __assign_string(name, d->name);
68 __entry->m = m;
69 __entry->n = n;
70 __entry->hdr = h;
71 __entry->addr = c->addr;
72 __entry->data = c->data;
73 __entry->complete = c->complete;
74 ),
75
76 TP_printk("%s: send-msg: tcs(m): %d cmd(n): %d msgid: 0x%08x addr: 0x%08x data: 0x%08x complete: %d",
77 __get_str(name), __entry->m, __entry->n, __entry->hdr,
78 __entry->addr, __entry->data, __entry->complete)
79 );
80
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59118 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-06 5:38 ` kbuild test robot
@ 2018-03-06 21:47 ` Steven Rostedt
2018-03-06 21:56 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Steven Rostedt @ 2018-03-06 21:47 UTC (permalink / raw)
To: kbuild test robot
Cc: Lina Iyer, kbuild-all, andy.gross, david.brown, linux-arm-msm,
linux-soc, rnayak, bjorn.andersson, linux-kernel
On Tue, 6 Mar 2018 13:38:06 +0800
kbuild test robot <lkp@intel.com> wrote:
> >> drivers/soc/qcom/./trace-rpmh.h:29:3: error: implicit declaration of function '__assign_string'; did you mean '__assign_str'? [-Werror=implicit-function-declaration]
Yes, you meant __assign_str(). I may have said __assign_string() in my
comments, but I was doing it from memory, not actually compiling code.
Please make sure you test the code and make sure it builds before
posting. And displays the tracepoint as you expect it to.
Thanks,
-- Steve
> __assign_string(name, d->name);
> ^
> include/trace/trace_events.h:719:4: note: in definition of macro 'DECLARE_EVENT_CLASS'
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-06 21:47 ` Steven Rostedt
@ 2018-03-06 21:56 ` Lina Iyer
2018-03-06 22:05 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-06 21:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: kbuild test robot, kbuild-all, andy.gross, david.brown,
linux-arm-msm, linux-soc, rnayak, bjorn.andersson, linux-kernel
On Tue, Mar 06 2018 at 14:47 -0700, Steven Rostedt wrote:
>On Tue, 6 Mar 2018 13:38:06 +0800
>kbuild test robot <lkp@intel.com> wrote:
>
>> >> drivers/soc/qcom/./trace-rpmh.h:29:3: error: implicit declaration of function '__assign_string'; did you mean '__assign_str'? [-Werror=implicit-function-declaration]
>
>Yes, you meant __assign_str(). I may have said __assign_string() in my
>comments, but I was doing it from memory, not actually compiling code.
>
>Please make sure you test the code and make sure it builds before
>posting. And displays the tracepoint as you expect it to.
>
I compiled and checked for sparse. Didn't explictly test for this. Not
sure why it did not fail for me.
-- Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-06 21:56 ` Lina Iyer
@ 2018-03-06 22:05 ` Lina Iyer
2018-03-06 22:34 ` Steven Rostedt
0 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-06 22:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: kbuild test robot, kbuild-all, andy.gross, david.brown,
linux-arm-msm, linux-soc, rnayak, bjorn.andersson, linux-kernel
On Tue, Mar 06 2018 at 14:56 -0700, Lina Iyer wrote:
>On Tue, Mar 06 2018 at 14:47 -0700, Steven Rostedt wrote:
>>On Tue, 6 Mar 2018 13:38:06 +0800
>>kbuild test robot <lkp@intel.com> wrote:
>>
>>>>> drivers/soc/qcom/./trace-rpmh.h:29:3: error: implicit declaration of function '__assign_string'; did you mean '__assign_str'? [-Werror=implicit-function-declaration]
>>
>>Yes, you meant __assign_str(). I may have said __assign_string() in my
>>comments, but I was doing it from memory, not actually compiling code.
>>
>>Please make sure you test the code and make sure it builds before
>>posting. And displays the tracepoint as you expect it to.
>>
>I compiled and checked for sparse. Didn't explictly test for this. Not
>sure why it did not fail for me.
>
Duh. FTRACE seems to have been disabled. Will fix and update.
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
2018-03-06 22:05 ` Lina Iyer
@ 2018-03-06 22:34 ` Steven Rostedt
0 siblings, 0 replies; 36+ messages in thread
From: Steven Rostedt @ 2018-03-06 22:34 UTC (permalink / raw)
To: Lina Iyer
Cc: kbuild test robot, kbuild-all, andy.gross, david.brown,
linux-arm-msm, linux-soc, rnayak, bjorn.andersson, linux-kernel
On Tue, 6 Mar 2018 15:05:42 -0700
Lina Iyer <ilina@codeaurora.org> wrote:
> Duh. FTRACE seems to have been disabled. Will fix and update.
Don't feel bad. You're not alone, I've done that too ;-)
-- Steve
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (2 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 18:57 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS Lina Iyer
` (5 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Sending RPMH requests and waiting for response from the controller
through a callback is common functionality across all platform drivers.
To simplify drivers, add a library functions to create RPMH client and
send resource state requests.
rpmh_write() is a synchronous blocking call that can be used to send
active state requests.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
drivers/soc/qcom/Makefile | 4 +-
drivers/soc/qcom/rpmh-internal.h | 2 +
drivers/soc/qcom/rpmh-rsc.c | 7 ++
drivers/soc/qcom/rpmh.c | 257 +++++++++++++++++++++++++++++++++++++++
include/soc/qcom/rpmh.h | 34 ++++++
5 files changed, 303 insertions(+), 1 deletion(-)
create mode 100644 drivers/soc/qcom/rpmh.c
create mode 100644 include/soc/qcom/rpmh.h
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index bb013d56b5a1..5c9c6c442ea9 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -13,4 +13,6 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
-obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o
+obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o
+qcom_rpmh-y += rpmh-rsc.o
+qcom_rpmh-y += rpmh.o
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 12faec77c4f3..1442a64ac4c5 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -84,4 +84,6 @@ struct rsc_drv {
int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg);
+void rpmh_tx_done(struct tcs_request *msg, int r);
+
#endif /* __RPM_INTERNAL_H__ */
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 4322e23a2ba6..b5b39b86f904 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -263,6 +263,8 @@ static void tcs_notify_tx_done(unsigned long data)
struct rsc_drv *drv = (struct rsc_drv *)data;
struct tcs_response *resp;
unsigned long flags;
+ struct tcs_request *msg;
+ int err;
for (;;) {
spin_lock_irqsave(&drv->drv_lock, flags);
@@ -275,7 +277,10 @@ static void tcs_notify_tx_done(unsigned long data)
list_del(&resp->list);
spin_unlock_irqrestore(&drv->drv_lock, flags);
trace_rpmh_notify_tx_done(drv, resp);
+ msg = resp->msg;
+ err = resp->err;
free_response(resp);
+ rpmh_tx_done(msg, err);
}
}
@@ -575,6 +580,8 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0,
drv->tcs[ACTIVE_TCS].tcs_mask);
+ dev_set_drvdata(&pdev->dev, drv);
+
return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
}
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
new file mode 100644
index 000000000000..d95ea3fa8b67
--- /dev/null
+++ b/drivers/soc/qcom/rpmh.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/atomic.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.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/types.h>
+#include <linux/wait.h>
+
+#include <soc/qcom/rpmh.h>
+
+#include "rpmh-internal.h"
+
+#define RPMH_MAX_MBOXES 2
+#define RPMH_TIMEOUT (10 * HZ)
+
+#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \
+ struct rpmh_request name = { \
+ .msg = { \
+ .state = s, \
+ .payload = name.cmd, \
+ .num_payload = 0, \
+ .is_complete = true, \
+ }, \
+ .cmd = { { 0 } }, \
+ .completion = q, \
+ .wait_count = c, \
+ .rc = rc, \
+ }
+
+/**
+ * rpmh_request: the message to be sent to rpmh-rsc
+ *
+ * @msg: the request
+ * @cmd: the payload that will be part of the @msg
+ * @completion: triggered when request is done
+ * @wait_count: count of waiters for this completion
+ * @err: err return from the controller
+ */
+struct rpmh_request {
+ struct tcs_request msg;
+ struct tcs_cmd cmd[MAX_RPMH_PAYLOAD];
+ struct completion *completion;
+ atomic_t *wait_count;
+ struct rpmh_client *rc;
+ int err;
+};
+
+/**
+ * rpmh_ctrlr: our representation of the controller
+ *
+ * @drv: the controller instance
+ */
+struct rpmh_ctrlr {
+ struct rsc_drv *drv;
+};
+
+/**
+ * rpmh_client: the client object
+ *
+ * @dev: the platform device that is the owner
+ * @ctrlr: the controller associated with this client.
+ */
+struct rpmh_client {
+ struct device *dev;
+ struct rpmh_ctrlr *ctrlr;
+};
+
+static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES];
+static DEFINE_MUTEX(rpmh_ctrlr_mutex);
+
+void rpmh_tx_done(struct tcs_request *msg, int r)
+{
+ struct rpmh_request *rpm_msg = container_of(msg,
+ struct rpmh_request, msg);
+ atomic_t *wc = rpm_msg->wait_count;
+ struct completion *compl = rpm_msg->completion;
+
+ rpm_msg->err = r;
+
+ if (r)
+ dev_err(rpm_msg->rc->dev,
+ "RPMH TX fail in msg addr 0x%x, err=%d\n",
+ rpm_msg->msg.payload[0].addr, r);
+
+ /* Signal the blocking thread we are done */
+ if (wc && atomic_dec_and_test(wc) && compl)
+ complete(compl);
+}
+EXPORT_SYMBOL(rpmh_tx_done);
+
+/**
+ * wait_for_tx_done: Wait until the response is received.
+ *
+ * @rc: The RPMH client
+ * @compl: The completion object
+ * @addr: An addr that we sent in that request
+ * @data: The data for the address in that request
+ *
+ */
+static int wait_for_tx_done(struct rpmh_client *rc,
+ struct completion *compl, u32 addr, u32 data)
+{
+ int ret;
+
+ ret = wait_for_completion_timeout(compl, RPMH_TIMEOUT);
+ if (ret)
+ dev_dbg(rc->dev,
+ "RPMH response received addr=0x%x data=0x%x\n",
+ addr, data);
+ else
+ dev_err(rc->dev,
+ "RPMH response timeout addr=0x%x data=0x%x\n",
+ addr, data);
+
+ return (ret > 0) ? 0 : -ETIMEDOUT;
+}
+
+/**
+ * __rpmh_write: send the RPMH request
+ *
+ * @rc: The RPMH client
+ * @state: Active/Sleep request type
+ * @rpm_msg: The data that needs to be sent (payload).
+ */
+static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+ struct rpmh_request *rpm_msg)
+{
+ int ret = -EFAULT;
+
+ rpm_msg->msg.state = state;
+
+ if (state == RPMH_ACTIVE_ONLY_STATE) {
+ WARN_ON(irqs_disabled());
+ ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg);
+ if (!ret)
+ dev_dbg(rc->dev,
+ "RPMH request sent addr=0x%x, data=0x%x\n",
+ rpm_msg->msg.payload[0].addr,
+ rpm_msg->msg.payload[0].data);
+ else
+ dev_warn(rc->dev,
+ "Error in RPMH request addr=0x%x, data=0x%x\n",
+ rpm_msg->msg.payload[0].addr,
+ rpm_msg->msg.payload[0].data);
+ }
+
+ return ret;
+}
+
+/**
+ * rpmh_write: Write a set of RPMH commands and block until response
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in payload
+ *
+ * May sleep. Do not call from atomic contexts.
+ */
+int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int n)
+{
+ DECLARE_COMPLETION_ONSTACK(compl);
+ atomic_t wait_count = ATOMIC_INIT(1);
+ DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg);
+ int ret;
+
+ if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
+ return -EINVAL;
+
+ might_sleep();
+
+ memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
+ rpm_msg.msg.num_payload = n;
+
+ ret = __rpmh_write(rc, state, &rpm_msg);
+ if (ret)
+ return ret;
+
+ return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data);
+}
+EXPORT_SYMBOL(rpmh_write);
+
+static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
+{
+ int i;
+ struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent);
+ struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT);
+
+ if (!drv)
+ return ctrlr;
+
+ mutex_lock(&rpmh_ctrlr_mutex);
+ for (i = 0; i < RPMH_MAX_MBOXES; i++) {
+ if (rpmh_rsc[i].drv == drv) {
+ ctrlr = &rpmh_rsc[i];
+ goto unlock;
+ }
+ }
+
+ for (i = 0; i < RPMH_MAX_MBOXES; i++) {
+ if (rpmh_rsc[i].drv == NULL) {
+ ctrlr = &rpmh_rsc[i];
+ ctrlr->drv = drv;
+ break;
+ }
+ }
+ WARN_ON(i == RPMH_MAX_MBOXES);
+unlock:
+ mutex_unlock(&rpmh_ctrlr_mutex);
+ return ctrlr;
+}
+
+/**
+ * rpmh_get_client: Get the RPMh handle
+ *
+ * @pdev: the platform device which needs to communicate with RPM
+ * accelerators
+ * May sleep.
+ */
+struct rpmh_client *rpmh_get_client(struct platform_device *pdev)
+{
+ struct rpmh_client *rc;
+
+ rc = kzalloc(sizeof(*rc), GFP_KERNEL);
+ if (!rc)
+ return ERR_PTR(-ENOMEM);
+
+ rc->dev = &pdev->dev;
+ rc->ctrlr = get_rpmh_ctrlr(pdev);
+ if (IS_ERR(rc->ctrlr)) {
+ kfree(rc);
+ return ERR_PTR(-EFAULT);
+ }
+
+ return rc;
+}
+EXPORT_SYMBOL(rpmh_get_client);
+
+/**
+ * rpmh_release: Release the RPMH client
+ *
+ * @rc: The RPMh handle to be freed.
+ */
+void rpmh_release(struct rpmh_client *rc)
+{
+ kfree(rc);
+}
+EXPORT_SYMBOL(rpmh_release);
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
new file mode 100644
index 000000000000..53cc145990c2
--- /dev/null
+++ b/include/soc/qcom/rpmh.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SOC_QCOM_RPMH_H__
+#define __SOC_QCOM_RPMH_H__
+
+#include <soc/qcom/tcs.h>
+#include <linux/platform_device.h>
+
+struct rpmh_client;
+
+#if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int n);
+
+struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
+
+void rpmh_release(struct rpmh_client *rc);
+
+#else
+
+static inline int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int n)
+{ return -ENODEV; }
+
+static inline struct rpmh_client *rpmh_get_client(struct platform_device *pdev)
+{ return ERR_PTR(-ENODEV); }
+
+static inline void rpmh_release(struct rpmh_client *rc) { }
+#endif /* CONFIG_QCOM_RPMH */
+
+#endif /* __SOC_QCOM_RPMH_H__ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions
2018-03-02 16:43 ` [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions Lina Iyer
@ 2018-03-08 18:57 ` Stephen Boyd
2018-03-08 21:37 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 18:57 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:11)
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> new file mode 100644
> index 000000000000..d95ea3fa8b67
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.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/types.h>
> +#include <linux/wait.h>
> +
> +#include <soc/qcom/rpmh.h>
> +
> +#include "rpmh-internal.h"
> +
> +#define RPMH_MAX_MBOXES 2
> +#define RPMH_TIMEOUT (10 * HZ)
Can this be in ms instead of HZ units?
> +
> +/**
> + * rpmh_ctrlr: our representation of the controller
> + *
> + * @drv: the controller instance
> + */
> +struct rpmh_ctrlr {
> + struct rsc_drv *drv;
> +};
> +
> +/**
> + * rpmh_client: the client object
same kernel doc issues here.
> + *
> + * @dev: the platform device that is the owner
> + * @ctrlr: the controller associated with this client.
> + */
> +struct rpmh_client {
> + struct device *dev;
> + struct rpmh_ctrlr *ctrlr;
> +};
> +
> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES];
> +static DEFINE_MUTEX(rpmh_ctrlr_mutex);
> +
> +void rpmh_tx_done(struct tcs_request *msg, int r)
> +{
> + struct rpmh_request *rpm_msg = container_of(msg,
> + struct rpmh_request, msg);
> + atomic_t *wc = rpm_msg->wait_count;
> + struct completion *compl = rpm_msg->completion;
> +
> + rpm_msg->err = r;
> +
> + if (r)
> + dev_err(rpm_msg->rc->dev,
> + "RPMH TX fail in msg addr 0x%x, err=%d\n",
> + rpm_msg->msg.payload[0].addr, r);
> +
> + /* Signal the blocking thread we are done */
> + if (wc && atomic_dec_and_test(wc) && compl)
I don't understand this whole thing. The atomic variable is always set
to 1 in this patch, and then we will do a dec and test and then complete
when that happens. There is the case where it isn't assigned, but then
this logic doesn't happen at all. There must be some future code that
uses this? Can you add the atomic counting stuff in that patch when we
need to count more than one?
And then if that future happens, maybe consider changing from a count to
a chained DMA list style type of thing, where each message has a single
element that's written, but each message can have a 'wait' bit or not
that would cause this code to call complete if it's there. Then drivers
can wait on any certain part of the message completion (or multiple of
them) without us having to do a count.
> + complete(compl);
> +}
> +EXPORT_SYMBOL(rpmh_tx_done);
> +
> +/**
> + * wait_for_tx_done: Wait until the response is received.
> + *
> + * @rc: The RPMH client
> + * @compl: The completion object
> + * @addr: An addr that we sent in that request
> + * @data: The data for the address in that request
> + *
> + */
> +static int wait_for_tx_done(struct rpmh_client *rc,
> + struct completion *compl, u32 addr, u32 data)
> +{
> + int ret;
> +
> + ret = wait_for_completion_timeout(compl, RPMH_TIMEOUT);
> + if (ret)
> + dev_dbg(rc->dev,
> + "RPMH response received addr=0x%x data=0x%x\n",
> + addr, data);
> + else
> + dev_err(rc->dev,
> + "RPMH response timeout addr=0x%x data=0x%x\n",
> + addr, data);
> +
> + return (ret > 0) ? 0 : -ETIMEDOUT;
> +}
> +
> +/**
> + * __rpmh_write: send the RPMH request
> + *
> + * @rc: The RPMH client
> + * @state: Active/Sleep request type
> + * @rpm_msg: The data that needs to be sent (payload).
> + */
> +static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> + struct rpmh_request *rpm_msg)
> +{
> + int ret = -EFAULT;
Not sure -EFAULT is the right error value here. -EINVAL?
> +
> + rpm_msg->msg.state = state;
> +
> + if (state == RPMH_ACTIVE_ONLY_STATE) {
> + WARN_ON(irqs_disabled());
> + ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg);
> + if (!ret)
> + dev_dbg(rc->dev,
> + "RPMH request sent addr=0x%x, data=0x%x\n",
> + rpm_msg->msg.payload[0].addr,
> + rpm_msg->msg.payload[0].data);
> + else
> + dev_warn(rc->dev,
> + "Error in RPMH request addr=0x%x, data=0x%x\n",
> + rpm_msg->msg.payload[0].addr,
> + rpm_msg->msg.payload[0].data);
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * rpmh_write: Write a set of RPMH commands and block until response
> + *
> + * @rc: The RPMh handle got from rpmh_get_dev_channel
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in payload
> + *
> + * May sleep. Do not call from atomic contexts.
> + */
> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> + struct tcs_cmd *cmd, int n)
> +{
> + DECLARE_COMPLETION_ONSTACK(compl);
> + atomic_t wait_count = ATOMIC_INIT(1);
> + DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg);
> + int ret;
> +
> + if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
How is rc IS_ERR_OR_NULL at this point?
Should n be unsigned then?
> + return -EINVAL;
> +
> + might_sleep();
The wait_for_tx_done() would handle this might_sleep?
> +
> + memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
> + rpm_msg.msg.num_payload = n;
> +
> + ret = __rpmh_write(rc, state, &rpm_msg);
> + if (ret)
> + return ret;
> +
> + return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data);
> +}
> +EXPORT_SYMBOL(rpmh_write);
> +
> +static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
> +{
> + int i;
> + struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent);
> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT);
Not sure -EFAULT is the right error value here.
> +
> + if (!drv)
> + return ctrlr;
> +
> + mutex_lock(&rpmh_ctrlr_mutex);
> + for (i = 0; i < RPMH_MAX_MBOXES; i++) {
> + if (rpmh_rsc[i].drv == drv) {
> + ctrlr = &rpmh_rsc[i];
> + goto unlock;
> + }
> + }
> +
> + for (i = 0; i < RPMH_MAX_MBOXES; i++) {
> + if (rpmh_rsc[i].drv == NULL) {
> + ctrlr = &rpmh_rsc[i];
> + ctrlr->drv = drv;
> + break;
> + }
> + }
> + WARN_ON(i == RPMH_MAX_MBOXES);
> +unlock:
> + mutex_unlock(&rpmh_ctrlr_mutex);
> + return ctrlr;
> +}
> +
> +/**
> + * rpmh_get_client: Get the RPMh handle
> + *
> + * @pdev: the platform device which needs to communicate with RPM
> + * accelerators
> + * May sleep.
> + */
> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev)
Given that the child devices are fairly well aware that they're rpmh
device drivers, why do we need this set of APIs in a different file and
also why do we need to have a client cookie design? It would make sense
to have the cookie if the device hierarchy wasn't direct, but that
doesn't seem to be the case here. Also it would make things easier to
follow if the code was folded into the same C file. It looks like we may
have two files exporting symbols to each other but not anywhere else.
Take a look at clk-rpm.c in clk/qcom and you'll see that we don't do any
sort of client cookie. Instead, the parent device drv data has the
pointer we want to get, and then the rpm APIs take that pointer.
> +{
> + struct rpmh_client *rc;
> +
> + rc = kzalloc(sizeof(*rc), GFP_KERNEL);
> + if (!rc)
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions
2018-03-08 18:57 ` Stephen Boyd
@ 2018-03-08 21:37 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-08 21:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Thu, Mar 08 2018 at 11:57 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:11)
>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>> new file mode 100644
>> index 000000000000..d95ea3fa8b67
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpmh.c
>> @@ -0,0 +1,257 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/atomic.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.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/types.h>
>> +#include <linux/wait.h>
>> +
>> +#include <soc/qcom/rpmh.h>
>> +
>> +#include "rpmh-internal.h"
>> +
>> +#define RPMH_MAX_MBOXES 2
>> +#define RPMH_TIMEOUT (10 * HZ)
>
>Can this be in ms instead of HZ units?
>
Hmm.. I changed it upon recommendation from Bjorn.
>> +
>> +/**
>> + * rpmh_ctrlr: our representation of the controller
>> + *
>> + * @drv: the controller instance
>> + */
>> +struct rpmh_ctrlr {
>> + struct rsc_drv *drv;
>> +};
>> +
>> +/**
>> + * rpmh_client: the client object
>
>same kernel doc issues here.
>
Have addressed it in the rev I am working on.
>> + *
>> + * @dev: the platform device that is the owner
>> + * @ctrlr: the controller associated with this client.
>> + */
>> +struct rpmh_client {
>> + struct device *dev;
>> + struct rpmh_ctrlr *ctrlr;
>> +};
>> +
>> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES];
>> +static DEFINE_MUTEX(rpmh_ctrlr_mutex);
>> +
>> +void rpmh_tx_done(struct tcs_request *msg, int r)
>> +{
>> + struct rpmh_request *rpm_msg = container_of(msg,
>> + struct rpmh_request, msg);
>> + atomic_t *wc = rpm_msg->wait_count;
>> + struct completion *compl = rpm_msg->completion;
>> +
>> + rpm_msg->err = r;
>> +
>> + if (r)
>> + dev_err(rpm_msg->rc->dev,
>> + "RPMH TX fail in msg addr 0x%x, err=%d\n",
>> + rpm_msg->msg.payload[0].addr, r);
>> +
>> + /* Signal the blocking thread we are done */
>> + if (wc && atomic_dec_and_test(wc) && compl)
>
>I don't understand this whole thing. The atomic variable is always set
>to 1 in this patch, and then we will do a dec and test and then complete
>when that happens. There is the case where it isn't assigned, but then
>this logic doesn't happen at all. There must be some future code that
>uses this? Can you add the atomic counting stuff in that patch when we
>need to count more than one?
>
Yes. This is needed for batch requests. I felt it would be much of a
change to get this removed and added back in. Let me see what I can do.
>And then if that future happens, maybe consider changing from a count to
>a chained DMA list style type of thing, where each message has a single
>element that's written, but each message can have a 'wait' bit or not
>that would cause this code to call complete if it's there. Then drivers
>can wait on any certain part of the message completion (or multiple of
>them) without us having to do a count.
>
Not sure what is the benefit of that. This accomplishes just the same.
>> + complete(compl);
>> +}
>> +EXPORT_SYMBOL(rpmh_tx_done);
>> +
>> +/**
>> + * wait_for_tx_done: Wait until the response is received.
>> + *
>> + * @rc: The RPMH client
>> + * @compl: The completion object
>> + * @addr: An addr that we sent in that request
>> + * @data: The data for the address in that request
>> + *
>> + */
>> +static int wait_for_tx_done(struct rpmh_client *rc,
>> + struct completion *compl, u32 addr, u32 data)
>> +{
>> + int ret;
>> +
>> + ret = wait_for_completion_timeout(compl, RPMH_TIMEOUT);
>> + if (ret)
>> + dev_dbg(rc->dev,
>> + "RPMH response received addr=0x%x data=0x%x\n",
>> + addr, data);
>> + else
>> + dev_err(rc->dev,
>> + "RPMH response timeout addr=0x%x data=0x%x\n",
>> + addr, data);
>> +
>> + return (ret > 0) ? 0 : -ETIMEDOUT;
>> +}
>> +
>> +/**
>> + * __rpmh_write: send the RPMH request
>> + *
>> + * @rc: The RPMH client
>> + * @state: Active/Sleep request type
>> + * @rpm_msg: The data that needs to be sent (payload).
>> + */
>> +static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> + struct rpmh_request *rpm_msg)
>> +{
>> + int ret = -EFAULT;
>
>Not sure -EFAULT is the right error value here. -EINVAL?
>
Sure.
>> +
>> + rpm_msg->msg.state = state;
>> +
>> + if (state == RPMH_ACTIVE_ONLY_STATE) {
>> + WARN_ON(irqs_disabled());
>> + ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg);
>> + if (!ret)
>> + dev_dbg(rc->dev,
>> + "RPMH request sent addr=0x%x, data=0x%x\n",
>> + rpm_msg->msg.payload[0].addr,
>> + rpm_msg->msg.payload[0].data);
>> + else
>> + dev_warn(rc->dev,
>> + "Error in RPMH request addr=0x%x, data=0x%x\n",
>> + rpm_msg->msg.payload[0].addr,
>> + rpm_msg->msg.payload[0].data);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * rpmh_write: Write a set of RPMH commands and block until response
>> + *
>> + * @rc: The RPMh handle got from rpmh_get_dev_channel
>> + * @state: Active/sleep set
>> + * @cmd: The payload data
>> + * @n: The number of elements in payload
>> + *
>> + * May sleep. Do not call from atomic contexts.
>> + */
>> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> + struct tcs_cmd *cmd, int n)
>> +{
>> + DECLARE_COMPLETION_ONSTACK(compl);
>> + atomic_t wait_count = ATOMIC_INIT(1);
>> + DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg);
>> + int ret;
>> +
>> + if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
>
>How is rc IS_ERR_OR_NULL at this point?
>
If the rpmh_get_client() had failed and the driver failed to check that.
>Should n be unsigned then?
>
OK
>> + return -EINVAL;
>> +
>> + might_sleep();
>
>The wait_for_tx_done() would handle this might_sleep?
>
Not that I am failing here.. but it just felt right to report this
earlier than later.
>> +
>> + memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
>> + rpm_msg.msg.num_payload = n;
>> +
>> + ret = __rpmh_write(rc, state, &rpm_msg);
>> + if (ret)
>> + return ret;
>> +
>> + return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data);
>> +}
>> +EXPORT_SYMBOL(rpmh_write);
>> +
>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
>> +{
>> + int i;
>> + struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent);
>> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT);
>
>Not sure -EFAULT is the right error value here.
>
Will fix.
>> +
>> + if (!drv)
>> + return ctrlr;
>> +
>> + mutex_lock(&rpmh_ctrlr_mutex);
>> + for (i = 0; i < RPMH_MAX_MBOXES; i++) {
>> + if (rpmh_rsc[i].drv == drv) {
>> + ctrlr = &rpmh_rsc[i];
>> + goto unlock;
>> + }
>> + }
>> +
>> + for (i = 0; i < RPMH_MAX_MBOXES; i++) {
>> + if (rpmh_rsc[i].drv == NULL) {
>> + ctrlr = &rpmh_rsc[i];
>> + ctrlr->drv = drv;
>> + break;
>> + }
>> + }
>> + WARN_ON(i == RPMH_MAX_MBOXES);
>> +unlock:
>> + mutex_unlock(&rpmh_ctrlr_mutex);
>> + return ctrlr;
>> +}
>> +
>> +/**
>> + * rpmh_get_client: Get the RPMh handle
>> + *
>> + * @pdev: the platform device which needs to communicate with RPM
>> + * accelerators
>> + * May sleep.
>> + */
>> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev)
>
>Given that the child devices are fairly well aware that they're rpmh
>device drivers, why do we need this set of APIs in a different file and
>also why do we need to have a client cookie design? It would make sense
>to have the cookie if the device hierarchy wasn't direct, but that
>doesn't seem to be the case here. Also it would make things easier to
>follow if the code was folded into the same C file. It looks like we may
>have two files exporting symbols to each other but not anywhere else.
>
There are so many concepts in these files that clobbering them would
just make them too ugly and hard to maintain. rpmh-rsc.c is a file that
deals with the hardware and this is the helper set of functions. This
does buffer management and caching that need to be burdened on a file
that is hardware centric.
>Take a look at clk-rpm.c in clk/qcom and you'll see that we don't do any
>sort of client cookie. Instead, the parent device drv data has the
>pointer we want to get, and then the rpm APIs take that pointer.
>
You still have to get a cookie of sorts. It just uses the parent deivce
data as a cookie. The way I see, it's no better or worse than this.
-- Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (3 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 04/10] drivers: qcom: rpmh: add RPMH helper functions Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 19:41 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS Lina Iyer
` (4 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, 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 <ilina@codeaurora.org>
---
drivers/soc/qcom/rpmh-internal.h | 7 +++
drivers/soc/qcom/rpmh-rsc.c | 128 +++++++++++++++++++++++++++++++++++++++
2 files changed, 135 insertions(+)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 1442a64ac4c5..65dfe1716265 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -13,6 +13,7 @@
#define MAX_CMDS_PER_TCS 16
#define MAX_TCS_PER_TYPE 3
#define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+#define MAX_TCS_SLOTS (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
struct rsc_drv;
@@ -44,6 +45,8 @@ struct tcs_response {
* @ncpt: number of commands in each TCS
* @tcs_lock: lock for synchronizing this TCS writes
* @responses: response objects for requests sent from each TCS
+ * @cmd_addr: flattened cache of cmds in sleep/wake TCS
+ * @slots: indicates which of @cmd_addr are occupied
*/
struct tcs_group {
struct rsc_drv *drv;
@@ -54,6 +57,9 @@ struct tcs_group {
int ncpt;
spinlock_t tcs_lock;
struct tcs_response *responses[MAX_TCS_PER_TYPE];
+ u32 *cmd_addr;
+ DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
+
};
/**
@@ -83,6 +89,7 @@ struct rsc_drv {
int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg);
+int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, struct tcs_request *msg);
void rpmh_tx_done(struct tcs_request *msg, int r);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index b5b39b86f904..34e780d9670f 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -6,6 +6,7 @@
#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
#include <linux/atomic.h>
+#include <linux/bitmap.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
@@ -178,6 +179,12 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
case RPMH_ACTIVE_ONLY_STATE:
type = ACTIVE_TCS;
break;
+ case RPMH_WAKE_ONLY_STATE:
+ type = WAKE_TCS;
+ break;
+ case RPMH_SLEEP_STATE:
+ type = SLEEP_TCS;
+ break;
default:
return ERR_PTR(-EINVAL);
}
@@ -450,6 +457,114 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg)
}
EXPORT_SYMBOL(rpmh_rsc_send_data);
+static int find_match(struct tcs_group *tcs, struct tcs_cmd *cmd, int len)
+{
+ bool found = false;
+ int i = 0, j;
+
+ /* Check for already cached commands */
+ while ((i = find_next_bit(tcs->slots, MAX_TCS_SLOTS, i)) <
+ MAX_TCS_SLOTS) {
+ if (tcs->cmd_addr[i] != cmd[0].addr) {
+ i++;
+ continue;
+ }
+ /* sanity check to ensure the seq is same */
+ for (j = 1; j < len; j++) {
+ WARN((tcs->cmd_addr[i + j] != cmd[j].addr),
+ "Message does not match previous sequence.\n");
+ return -EINVAL;
+ }
+ found = true;
+ break;
+ }
+
+ return found ? i : -1;
+}
+
+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;
+
+copy_data:
+ bitmap_set(tcs->slots, slot, msg->num_payload);
+ /* Copy the addresses of the resources over to the slots */
+ if (tcs->cmd_addr) {
+ for (i = 0; i < msg->num_payload; i++)
+ tcs->cmd_addr[slot + i] = msg->payload[i].addr;
+ }
+
+ offset = slot / tcs->ncpt;
+ *m = offset + tcs->tcs_offset;
+ *n = slot % tcs->ncpt;
+
+ return 0;
+}
+
+static int tcs_ctrl_write(struct rsc_drv *drv, struct tcs_request *msg)
+{
+ struct tcs_group *tcs;
+ int m = 0, n = 0;
+ unsigned long flags;
+ int ret = 0;
+
+ tcs = get_tcs_for_msg(drv, msg);
+ if (IS_ERR(tcs))
+ return PTR_ERR(tcs);
+
+ spin_lock_irqsave(&tcs->tcs_lock, flags);
+ /* find the m-th TCS and the n-th position in the TCS to write to */
+ ret = find_slots(tcs, msg, &m, &n);
+ if (!ret)
+ __tcs_buffer_write(drv, m, n, msg);
+ spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+
+ return ret;
+}
+
+/**
+ * rpmh_rsc_write_ctrl_data: Write request to the controller
+ *
+ * @drv: the controller
+ * @msg: the data to be written to the controller
+ *
+ * There is no response returned for writing the request to the controller.
+ */
+int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, struct tcs_request *msg)
+{
+ if (!msg || !msg->payload || !msg->num_payload ||
+ msg->num_payload > MAX_RPMH_PAYLOAD) {
+ pr_err("Payload error\n");
+ return -EINVAL;
+ }
+
+ /* Data sent to this API will not be sent immediately */
+ if (msg->state == RPMH_ACTIVE_ONLY_STATE)
+ return -EINVAL;
+
+ return tcs_ctrl_write(drv, msg);
+}
+EXPORT_SYMBOL(rpmh_rsc_write_ctrl_data);
+
static int rpmh_probe_tcs_config(struct platform_device *pdev,
struct rsc_drv *drv)
{
@@ -530,6 +645,19 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
tcs->tcs_offset = st;
st += tcs->num_tcs;
+
+ /*
+ * Allocate memory to cache sleep and wake requests to
+ * avoid reading TCS register memory.
+ */
+ if (tcs->type == ACTIVE_TCS)
+ continue;
+
+ tcs->cmd_addr = devm_kzalloc(&pdev->dev,
+ sizeof(u32) * tcs->num_tcs * ncpt,
+ GFP_KERNEL);
+ if (!tcs->cmd_addr)
+ return -ENOMEM;
}
drv->num_tcs = st;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS
2018-03-02 16:43 ` [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS Lina Iyer
@ 2018-03-08 19:41 ` Stephen Boyd
2018-03-08 23:58 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 19:41 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:12)
> 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
it is the job?
> driver to find arrange the requests into the available TCSes.
find and arrange?
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> drivers/soc/qcom/rpmh-internal.h | 7 +++
> drivers/soc/qcom/rpmh-rsc.c | 128 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 135 insertions(+)
>
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index 1442a64ac4c5..65dfe1716265 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -13,6 +13,7 @@
> #define MAX_CMDS_PER_TCS 16
> #define MAX_TCS_PER_TYPE 3
> #define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
> +#define MAX_TCS_SLOTS (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
>
> struct rsc_drv;
>
> @@ -44,6 +45,8 @@ struct tcs_response {
> * @ncpt: number of commands in each TCS
> * @tcs_lock: lock for synchronizing this TCS writes
> * @responses: response objects for requests sent from each TCS
> + * @cmd_addr: flattened cache of cmds in sleep/wake TCS
Maybe 'cmds' or 'cmd_cache'?
> + * @slots: indicates which of @cmd_addr are occupied
> */
> struct tcs_group {
> struct rsc_drv *drv;
> @@ -54,6 +57,9 @@ struct tcs_group {
> int ncpt;
> spinlock_t tcs_lock;
> struct tcs_response *responses[MAX_TCS_PER_TYPE];
> + u32 *cmd_addr;
> + DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
> +
Drop the newline please.
> };
>
> /**
> @@ -450,6 +457,114 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg)
> }
> EXPORT_SYMBOL(rpmh_rsc_send_data);
>
> +static int find_match(struct tcs_group *tcs, struct tcs_cmd *cmd, int len)
const tcs and cmd?
> +{
> + bool found = false;
Drop.
> + int i = 0, j;
> +
> + /* Check for already cached commands */
> + while ((i = find_next_bit(tcs->slots, MAX_TCS_SLOTS, i)) <
for_each_set_bit(... ?
> + MAX_TCS_SLOTS) {
> + if (tcs->cmd_addr[i] != cmd[0].addr) {
> + i++;
> + continue;
> + }
> + /* sanity check to ensure the seq is same */
> + for (j = 1; j < len; j++) {
> + WARN((tcs->cmd_addr[i + j] != cmd[j].addr),
> + "Message does not match previous sequence.\n");
> + return -EINVAL;
> + }
Can you fold the if and for loop together?
for (j = 0; j < len; j++) {
if (tcs->cmd_addr[i + j] != cmd[j].addr) {
if (j == 0)
break; /* Try another slot */
WARN("Message doesn't match previous sequence\n");
return -EINVAL;
} else if (j == len - 1) {
return i;
}
}
}
return -ENODATA;
> + found = true;
> + break;
> + }
> +
> + return found ? i : -1;
Is there space between slots? Just trying to understand how we
differentiate two adjacent cmd buffers with the bitmap scheme if this
loop is looking for free bits to find slots. Or maybe we need two
bitmaps where one is the allocated region and the other is something
indicating the start bit of a message
> +}
> +
> +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;
Shouldn't this goto skip setting the bits in tcs->slots?
> +
> + /* 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;
Would be nice to remove this duplicate condition somehow. Maybe a goto?
> +
> +copy_data:
> + bitmap_set(tcs->slots, slot, msg->num_payload);
> + /* Copy the addresses of the resources over to the slots */
> + if (tcs->cmd_addr) {
find_match() above didn't check for tcs->cmd_addr. Does this ever happen
to fail?
> + for (i = 0; i < msg->num_payload; i++)
> + tcs->cmd_addr[slot + i] = msg->payload[i].addr;
> + }
> +
> + offset = slot / tcs->ncpt;
> + *m = offset + tcs->tcs_offset;
> + *n = slot % tcs->ncpt;
> +
> + return 0;
> +}
> +
> +static int tcs_ctrl_write(struct rsc_drv *drv, struct tcs_request *msg)
> +{
> + struct tcs_group *tcs;
> + int m = 0, n = 0;
> + unsigned long flags;
> + int ret = 0;
Drop initial assignment please.
> +
> + tcs = get_tcs_for_msg(drv, msg);
> + if (IS_ERR(tcs))
> + return PTR_ERR(tcs);
> +
> + spin_lock_irqsave(&tcs->tcs_lock, flags);
> + /* find the m-th TCS and the n-th position in the TCS to write to */
> + ret = find_slots(tcs, msg, &m, &n);
> + if (!ret)
> + __tcs_buffer_write(drv, m, n, msg);
> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
> +
> + return ret;
> +}
> +
[...]
> @@ -530,6 +645,19 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
> tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
> tcs->tcs_offset = st;
> st += tcs->num_tcs;
> +
> + /*
> + * Allocate memory to cache sleep and wake requests to
> + * avoid reading TCS register memory.
> + */
> + if (tcs->type == ACTIVE_TCS)
> + continue;
> +
> + tcs->cmd_addr = devm_kzalloc(&pdev->dev,
devm_kcalloc(&pdev->dev, tcs->num_tcs * ncpt, sizeof(u32) ?
> + sizeof(u32) * tcs->num_tcs * ncpt,
> + GFP_KERNEL);
> + if (!tcs->cmd_addr)
> + return -ENOMEM;
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS
2018-03-08 19:41 ` Stephen Boyd
@ 2018-03-08 23:58 ` Lina Iyer
2018-03-09 15:45 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-08 23:58 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Thu, Mar 08 2018 at 12:41 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:12)
>> 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
>
>it is the job?
>
>> driver to find arrange the requests into the available TCSes.
>
>find and arrange?
>
Ok to both.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> drivers/soc/qcom/rpmh-internal.h | 7 +++
>> drivers/soc/qcom/rpmh-rsc.c | 128 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 135 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>> index 1442a64ac4c5..65dfe1716265 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> @@ -13,6 +13,7 @@
>> #define MAX_CMDS_PER_TCS 16
>> #define MAX_TCS_PER_TYPE 3
>> #define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
>> +#define MAX_TCS_SLOTS (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
>>
>> struct rsc_drv;
>>
>> @@ -44,6 +45,8 @@ struct tcs_response {
>> * @ncpt: number of commands in each TCS
>> * @tcs_lock: lock for synchronizing this TCS writes
>> * @responses: response objects for requests sent from each TCS
>> + * @cmd_addr: flattened cache of cmds in sleep/wake TCS
>
>Maybe 'cmds' or 'cmd_cache'?
>
Ok
>> + * @slots: indicates which of @cmd_addr are occupied
>> */
>> struct tcs_group {
>> struct rsc_drv *drv;
>> @@ -54,6 +57,9 @@ struct tcs_group {
>> int ncpt;
>> spinlock_t tcs_lock;
>> struct tcs_response *responses[MAX_TCS_PER_TYPE];
>> + u32 *cmd_addr;
>> + DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
>> +
>
>Drop the newline please.
>
Done.
>> };
>>
>> /**
>> @@ -450,6 +457,114 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg)
>> }
>> EXPORT_SYMBOL(rpmh_rsc_send_data);
>>
>> +static int find_match(struct tcs_group *tcs, struct tcs_cmd *cmd, int len)
>
>const tcs and cmd?
>
Ok
>> +{
>> + bool found = false;
>
>Drop.
>
Ok
>> + int i = 0, j;
>> +
>> + /* Check for already cached commands */
>> + while ((i = find_next_bit(tcs->slots, MAX_TCS_SLOTS, i)) <
>
>for_each_set_bit(... ?
>
Ok
>> + MAX_TCS_SLOTS) {
>> + if (tcs->cmd_addr[i] != cmd[0].addr) {
>> + i++;
>> + continue;
>> + }
>> + /* sanity check to ensure the seq is same */
>> + for (j = 1; j < len; j++) {
>> + WARN((tcs->cmd_addr[i + j] != cmd[j].addr),
>> + "Message does not match previous sequence.\n");
>> + return -EINVAL;
>> + }
>
>Can you fold the if and for loop together?
>
> for (j = 0; j < len; j++) {
> if (tcs->cmd_addr[i + j] != cmd[j].addr) {
> if (j == 0)
> break; /* Try another slot */
> WARN("Message doesn't match previous sequence\n");
> return -EINVAL;
> } else if (j == len - 1) {
> return i;
> }
> }
> }
>
> return -ENODATA;
>
OK
>> + found = true;
>> + break;
>> + }
>> +
>> + return found ? i : -1;
>
>Is there space between slots? Just trying to understand how we
>differentiate two adjacent cmd buffers with the bitmap scheme if this
>loop is looking for free bits to find slots. Or maybe we need two
>bitmaps where one is the allocated region and the other is something
>indicating the start bit of a message
>
there could be holes and we may have a set of commands that we would
like to fit together but may not sufficiently fit into any hole. So we
pick the first contiguous available slots and go with it.
We don't need multiple bitmaps.
>> +}
>> +
>> +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;
>
>Shouldn't this goto skip setting the bits in tcs->slots?
>
No, we overwrite what we found with this new 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;
>
>Would be nice to remove this duplicate condition somehow. Maybe a goto?
>
I would return instead of the break earlier instead of this here.
>> +
>> +copy_data:
>> + bitmap_set(tcs->slots, slot, msg->num_payload);
>> + /* Copy the addresses of the resources over to the slots */
>> + if (tcs->cmd_addr) {
>
>find_match() above didn't check for tcs->cmd_addr. Does this ever happen
>to fail?
>
Not allocated for active TCSes. I should be checking for it there as
well. Not sure how I didnt see a failure.
>> + for (i = 0; i < msg->num_payload; i++)
>> + tcs->cmd_addr[slot + i] = msg->payload[i].addr;
>> + }
>> +
>> + offset = slot / tcs->ncpt;
>> + *m = offset + tcs->tcs_offset;
>> + *n = slot % tcs->ncpt;
>> +
>> + return 0;
>> +}
>> +
>> +static int tcs_ctrl_write(struct rsc_drv *drv, struct tcs_request *msg)
>> +{
>> + struct tcs_group *tcs;
>> + int m = 0, n = 0;
>> + unsigned long flags;
>> + int ret = 0;
>
>Drop initial assignment please.
>
Ok
>> +
>> + tcs = get_tcs_for_msg(drv, msg);
>> + if (IS_ERR(tcs))
>> + return PTR_ERR(tcs);
>> +
>> + spin_lock_irqsave(&tcs->tcs_lock, flags);
>> + /* find the m-th TCS and the n-th position in the TCS to write to */
>> + ret = find_slots(tcs, msg, &m, &n);
>> + if (!ret)
>> + __tcs_buffer_write(drv, m, n, msg);
>> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
>> +
>> + return ret;
>> +}
>> +
>[...]
>> @@ -530,6 +645,19 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
>> tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
>> tcs->tcs_offset = st;
>> st += tcs->num_tcs;
>> +
>> + /*
>> + * Allocate memory to cache sleep and wake requests to
>> + * avoid reading TCS register memory.
>> + */
>> + if (tcs->type == ACTIVE_TCS)
>> + continue;
>> +
>> + tcs->cmd_addr = devm_kzalloc(&pdev->dev,
>
>devm_kcalloc(&pdev->dev, tcs->num_tcs * ncpt, sizeof(u32) ?
>
Ok
Thanks,
Lina
>> + sizeof(u32) * tcs->num_tcs * ncpt,
>> + GFP_KERNEL);
>> + if (!tcs->cmd_addr)
>> + return -ENOMEM;
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS
2018-03-08 23:58 ` Lina Iyer
@ 2018-03-09 15:45 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-09 15:45 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Thu, Mar 08 2018 at 16:58 -0700, Lina Iyer wrote:
>On Thu, Mar 08 2018 at 12:41 -0700, Stephen Boyd wrote:
>>Quoting Lina Iyer (2018-03-02 08:43:12)
>>>+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;
>>
>>Shouldn't this goto skip setting the bits in tcs->slots?
>>
>No, we overwrite what we found with this new 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;
>>
>>Would be nice to remove this duplicate condition somehow. Maybe a goto?
>>
>I would return instead of the break earlier instead of this here.
>>>+
>>>+copy_data:
>>>+ bitmap_set(tcs->slots, slot, msg->num_payload);
>>>+ /* Copy the addresses of the resources over to the slots */
>>>+ if (tcs->cmd_addr) {
>>
>>find_match() above didn't check for tcs->cmd_addr. Does this ever happen
>>to fail?
>>
>Not allocated for active TCSes. I should be checking for it there as
>well. Not sure how I didnt see a failure.
>
Ah, this function is never called for active tcs which would have the
tcs->cmd_addr to be NULL. I dont need this check.
-- Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (4 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 20:40 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests Lina Iyer
` (3 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Allow sleep and wake commands to be cleared from the respective TCSes,
so that they can be re-populated.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
drivers/soc/qcom/rpmh-internal.h | 1 +
drivers/soc/qcom/rpmh-rsc.c | 46 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 65dfe1716265..d2f73788a24d 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -90,6 +90,7 @@ struct rsc_drv {
int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_request *msg);
int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, struct tcs_request *msg);
+int rpmh_rsc_invalidate(struct rsc_drv *drv);
void rpmh_tx_done(struct tcs_request *msg, int r);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 34e780d9670f..e9f5a1f387fd 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -170,6 +170,52 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
return tcs;
}
+/**
+ * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
+ *
+ * @drv: the mailbox controller
+ */
+int rpmh_rsc_invalidate(struct rsc_drv *drv)
+{
+ struct tcs_group *tcs;
+ int m, type, ret = 0;
+ int inv_types[] = { WAKE_TCS, SLEEP_TCS };
+ unsigned long drv_flags, flags;
+
+ /* Lock the DRV and clear sleep and wake TCSes */
+ spin_lock_irqsave(&drv->drv_lock, drv_flags);
+ for (type = 0; type < ARRAY_SIZE(inv_types); type++) {
+ tcs = get_tcs_of_type(drv, inv_types[type]);
+ if (IS_ERR(tcs))
+ continue;
+
+ spin_lock_irqsave(&tcs->tcs_lock, flags);
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+ spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+ continue;
+ }
+
+ /* Clear the enable register for each TCS of the type */
+ for (m = tcs->tcs_offset;
+ m < tcs->tcs_offset + tcs->num_tcs; m++) {
+ if (!tcs_is_free(drv, m)) {
+ spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+ ret = -EAGAIN;
+ goto drv_unlock;
+ }
+ write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
+ /* Mark the TCS slots as free */
+ bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
+ }
+ spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+ }
+drv_unlock:
+ spin_unlock_irqrestore(&drv->drv_lock, drv_flags);
+
+ return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_invalidate);
+
static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
struct tcs_request *msg)
{
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS
2018-03-02 16:43 ` [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS Lina Iyer
@ 2018-03-08 20:40 ` Stephen Boyd
2018-03-09 16:41 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 20:40 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:13)
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 34e780d9670f..e9f5a1f387fd 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -170,6 +170,52 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
> return tcs;
> }
>
> +/**
> + * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
> + *
> + * @drv: the mailbox controller
> + */
> +int rpmh_rsc_invalidate(struct rsc_drv *drv)
> +{
> + struct tcs_group *tcs;
> + int m, type, ret = 0;
> + int inv_types[] = { WAKE_TCS, SLEEP_TCS };
> + unsigned long drv_flags, flags;
> +
> + /* Lock the DRV and clear sleep and wake TCSes */
> + spin_lock_irqsave(&drv->drv_lock, drv_flags);
> + for (type = 0; type < ARRAY_SIZE(inv_types); type++) {
> + tcs = get_tcs_of_type(drv, inv_types[type]);
> + if (IS_ERR(tcs))
> + continue;
> +
> + spin_lock_irqsave(&tcs->tcs_lock, flags);
Should just be a spin_lock() because we already irq saved a few lines above.
> + if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
> + continue;
> + }
> +
> + /* Clear the enable register for each TCS of the type */
> + for (m = tcs->tcs_offset;
> + m < tcs->tcs_offset + tcs->num_tcs; m++) {
> + if (!tcs_is_free(drv, m)) {
> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
> + ret = -EAGAIN;
> + goto drv_unlock;
> + }
> + write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
> + /* Mark the TCS slots as free */
> + bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> + }
> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
Maybe make another function called rpmh_tcs_invalidate(drv, enum TCS_FOO)
and put this logic inside it? And then change from a for loop to two
direct calls:
lock()
ret = rpmh_tcs_invalidate(drv, WAKE_TCS);
if (!ret)
ret = rpmh_tcs_invalidate(drv, SLEEP_TCS);
unlock()
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS
2018-03-08 20:40 ` Stephen Boyd
@ 2018-03-09 16:41 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-09 16:41 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Thu, Mar 08 2018 at 13:40 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:13)
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index 34e780d9670f..e9f5a1f387fd 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -170,6 +170,52 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
>> return tcs;
>> }
>>
>> +/**
>> + * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
>> + *
>> + * @drv: the mailbox controller
>> + */
>> +int rpmh_rsc_invalidate(struct rsc_drv *drv)
>> +{
>> + struct tcs_group *tcs;
>> + int m, type, ret = 0;
>> + int inv_types[] = { WAKE_TCS, SLEEP_TCS };
>> + unsigned long drv_flags, flags;
>> +
>> + /* Lock the DRV and clear sleep and wake TCSes */
>> + spin_lock_irqsave(&drv->drv_lock, drv_flags);
>> + for (type = 0; type < ARRAY_SIZE(inv_types); type++) {
>> + tcs = get_tcs_of_type(drv, inv_types[type]);
>> + if (IS_ERR(tcs))
>> + continue;
>> +
>> + spin_lock_irqsave(&tcs->tcs_lock, flags);
>
>Should just be a spin_lock() because we already irq saved a few lines above.
>
Agreed.
>> + if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
>> + continue;
>> + }
>> +
>> + /* Clear the enable register for each TCS of the type */
>> + for (m = tcs->tcs_offset;
>> + m < tcs->tcs_offset + tcs->num_tcs; m++) {
>> + if (!tcs_is_free(drv, m)) {
>> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
>> + ret = -EAGAIN;
>> + goto drv_unlock;
>> + }
>> + write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
>> + /* Mark the TCS slots as free */
>> + bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
>> + }
>> + spin_unlock_irqrestore(&tcs->tcs_lock, flags);
>
>Maybe make another function called rpmh_tcs_invalidate(drv, enum TCS_FOO)
>and put this logic inside it? And then change from a for loop to two
>direct calls:
>
> lock()
> ret = rpmh_tcs_invalidate(drv, WAKE_TCS);
> if (!ret)
> ret = rpmh_tcs_invalidate(drv, SLEEP_TCS);
> unlock()
>
Agreed. That will be better. Will fix.
-- Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (5 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-05 20:44 ` Evan Green
2018-03-02 16:43 ` [PATCH v3 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously Lina Iyer
` (2 subsequent siblings)
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
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 <ilina@codeaurora.org>
---
Changes in v3:
- Remove locking for flush function
- Improve comments
---
drivers/soc/qcom/rpmh.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++-
include/soc/qcom/rpmh.h | 10 +++
2 files changed, 217 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index d95ea3fa8b67..8a04009075b8 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -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)
+ goto existing;
+
+ req = kzalloc(sizeof(*req), GFP_ATOMIC);
+ if (!req) {
+ req = ERR_PTR(-ENOMEM);
+ goto unlock;
+ }
+
+ req->addr = cmd->addr;
+ req->sleep_val = req->wake_val = UINT_MAX;
+ 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,108 @@ 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
+ *
+ * Return: -EBUSY if the controller is busy, probably waiting on a response
+ * to a RPMH request sent earlier.
+ *
+ * This function is generally called from the sleep code from the last CPU
+ * that is powering down the entire system. Since no other RPMH API would be
+ * executing at this time, it is safe to run lockless.
+ */
+int rpmh_flush(struct rpmh_client *rc)
+{
+ struct cache_req *p;
+ struct rpmh_ctrlr *rpm = rc->ctrlr;
+ int ret;
+
+ if (IS_ERR_OR_NULL(rc))
+ return -EINVAL;
+
+ if (!rpm->dirty) {
+ pr_debug("Skipping flush, TCS has latest data.\n");
+ return 0;
+ }
+
+ /*
+ * 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;
+ ret = send_single(rc, RPMH_WAKE_ONLY_STATE, p->addr,
+ p->wake_val);
+ if (ret)
+ return ret;
+ }
+
+ rpm->dirty = false;
+
+ 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;
+
+ spin_lock_irqsave(&rpm->lock, flags);
+ rpm->dirty = true;
+ spin_unlock_irqrestore(&rpm->lock, flags);
+
+ 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 +414,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;
}
}
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index 53cc145990c2..a3f1246ce49a 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests
2018-03-02 16:43 ` [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests Lina Iyer
@ 2018-03-05 20:44 ` Evan Green
2018-03-06 22:12 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Evan Green @ 2018-03-05 20:44 UTC (permalink / raw)
To: Lina Iyer
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc,
Rajendra Nayak, Bjorn Andersson, linux-kernel
Hi Lina,
On Fri, Mar 2, 2018 at 8:43 AM, Lina Iyer <ilina@codeaurora.org> wrote:
> 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 <ilina@codeaurora.org>
> ---
>
> Changes in v3:
> - Remove locking for flush function
> - Improve comments
> ---
> drivers/soc/qcom/rpmh.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++-
> include/soc/qcom/rpmh.h | 10 +++
> 2 files changed, 217 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index d95ea3fa8b67..8a04009075b8 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
[...]
> +
> +/**
> + * 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;
> +
> + spin_lock_irqsave(&rpm->lock, flags);
> + rpm->dirty = true;
> + spin_unlock_irqrestore(&rpm->lock, flags);
Thanks for removing the locking from the flush path. I was hoping to
see the locking removed around this statement as well. The way I
understand it, all of the racy bits are attempting to set dirty to
true, so you don't need a lock to protect multiple threads from
setting the same value. The only time dirty is read or cleared is in
the single-threaded PM path, so there are no potentially dangerous
interactions.
If no one has any other comments on the series, then I don't need to
hold everything up based on this one tweak alone. But if you end up
spinning it again for other reasons, consider making this change as
well.
Thanks,
-Evan
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests
2018-03-05 20:44 ` Evan Green
@ 2018-03-06 22:12 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-06 22:12 UTC (permalink / raw)
To: Evan Green
Cc: Andy Gross, David Brown, linux-arm-msm, linux-soc,
Rajendra Nayak, Bjorn Andersson, linux-kernel
On Mon, Mar 05 2018 at 13:45 -0700, Evan Green wrote:
>Hi Lina,
>
>On Fri, Mar 2, 2018 at 8:43 AM, Lina Iyer <ilina@codeaurora.org> wrote:
>> 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 <ilina@codeaurora.org>
>> ---
>>
>> Changes in v3:
>> - Remove locking for flush function
>> - Improve comments
>> ---
>> drivers/soc/qcom/rpmh.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++-
>> include/soc/qcom/rpmh.h | 10 +++
>> 2 files changed, 217 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>> index d95ea3fa8b67..8a04009075b8 100644
>> --- a/drivers/soc/qcom/rpmh.c
>> +++ b/drivers/soc/qcom/rpmh.c
>[...]
>> +
>> +/**
>> + * 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;
>> +
>> + spin_lock_irqsave(&rpm->lock, flags);
>> + rpm->dirty = true;
>> + spin_unlock_irqrestore(&rpm->lock, flags);
>
>Thanks for removing the locking from the flush path. I was hoping to
>see the locking removed around this statement as well. The way I
>understand it, all of the racy bits are attempting to set dirty to
>true, so you don't need a lock to protect multiple threads from
>setting the same value. The only time dirty is read or cleared is in
>the single-threaded PM path, so there are no potentially dangerous
>interactions.
>
Fair point. Will take care of it.
>If no one has any other comments on the series, then I don't need to
>hold everything up based on this one tweak alone. But if you end up
>spinning it again for other reasons, consider making this change as
>well.
>
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (6 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 07/10] drivers: qcom: rpmh: cache sleep/wake state requests Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 21:06 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request Lina Iyer
2018-03-02 16:43 ` [PATCH v3 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS Lina Iyer
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Platform drivers that want to send a request but do not want to block
until the RPMH request completes have now a new API -
rpmh_write_async().
The API allocates memory and send the requests and returns the control
back to the platform driver. The tx_done callback from the controller is
handled in the context of the controller's thread and frees the
allocated memory. This API allows RPMH requests from atomic contexts as
well.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
drivers/soc/qcom/rpmh.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
include/soc/qcom/rpmh.h | 8 ++++++++
2 files changed, 60 insertions(+)
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 8a04009075b8..a02d9f685b2b 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -35,6 +35,7 @@
.completion = q, \
.wait_count = c, \
.rc = rc, \
+ .free = NULL, \
}
@@ -61,6 +62,7 @@ struct cache_req {
* @completion: triggered when request is done
* @wait_count: count of waiters for this completion
* @err: err return from the controller
+ * @free: the request object to be freed at tx_done
*/
struct rpmh_request {
struct tcs_request msg;
@@ -69,6 +71,7 @@ struct rpmh_request {
atomic_t *wait_count;
struct rpmh_client *rc;
int err;
+ struct rpmh_request *free;
};
/**
@@ -114,6 +117,8 @@ void rpmh_tx_done(struct tcs_request *msg, int r)
"RPMH TX fail in msg addr 0x%x, err=%d\n",
rpm_msg->msg.payload[0].addr, r);
+ kfree(rpm_msg->free);
+
/* Signal the blocking thread we are done */
if (wc && atomic_dec_and_test(wc) && compl)
complete(compl);
@@ -257,6 +262,53 @@ static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
return ret;
}
+static struct rpmh_request *__get_rpmh_msg_async(struct rpmh_client *rc,
+ enum rpmh_state state,
+ struct tcs_cmd *cmd, int n)
+{
+ struct rpmh_request *req;
+
+ if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
+ return ERR_PTR(-EINVAL);
+
+ req = kcalloc(1, sizeof(*req), GFP_ATOMIC);
+ if (!req)
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(req->cmd, cmd, n * sizeof(*cmd));
+
+ req->msg.state = state;
+ req->msg.payload = req->cmd;
+ req->msg.num_payload = n;
+ req->free = req;
+
+ return req;
+}
+
+/**
+ * rpmh_write_async: Write a set of RPMH commands
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in payload
+ *
+ * Write a set of RPMH commands, the order of commands is maintained
+ * and will be sent as a single shot.
+ */
+int rpmh_write_async(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int n)
+{
+ struct rpmh_request *rpm_msg;
+
+ rpm_msg = __get_rpmh_msg_async(rc, state, cmd, n);
+ if (IS_ERR(rpm_msg))
+ return PTR_ERR(rpm_msg);
+
+ return __rpmh_write(rc, state, rpm_msg);
+}
+EXPORT_SYMBOL(rpmh_write_async);
+
/**
* rpmh_write: Write a set of RPMH commands and block until response
*
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index a3f1246ce49a..172a649f1a1c 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -15,6 +15,9 @@ struct rpmh_client;
int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
struct tcs_cmd *cmd, int n);
+int rpmh_write_async(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int n);
+
struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
int rpmh_flush(struct rpmh_client *rc);
@@ -32,6 +35,11 @@ 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_write_async(struct rpmh_client *rc,
+ enum rpmh_state state,
+ struct tcs_cmd *cmd, int n)
+{ return -ENODEV; }
+
static inline int rpmh_flush(struct rpmh_client *rc)
{ return -ENODEV; }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously
2018-03-02 16:43 ` [PATCH v3 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously Lina Iyer
@ 2018-03-08 21:06 ` Stephen Boyd
0 siblings, 0 replies; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 21:06 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:15)
> @@ -69,6 +71,7 @@ struct rpmh_request {
> atomic_t *wait_count;
> struct rpmh_client *rc;
> int err;
> + struct rpmh_request *free;
> };
>
> /**
> @@ -114,6 +117,8 @@ void rpmh_tx_done(struct tcs_request *msg, int r)
> "RPMH TX fail in msg addr 0x%x, err=%d\n",
> rpm_msg->msg.payload[0].addr, r);
>
> + kfree(rpm_msg->free);
Is this potentially freeing something which is then used after this call
later in this function? It looks like the compiler could be reloading
from freed memory for the wc and compl variables after this kfree is
called. At the least, please add some sort of comment or if we don't
ever need to free a _different_ rpm_msg than the existing one, make it a
flag so it becomes very obvious that we're freeing the same memory that
we loaded from in this function.
> +
> /* Signal the blocking thread we are done */
> if (wc && atomic_dec_and_test(wc) && compl)
> complete(compl);
> @@ -257,6 +262,53 @@ static int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> return ret;
> }
>
> +static struct rpmh_request *__get_rpmh_msg_async(struct rpmh_client *rc,
> + enum rpmh_state state,
> + struct tcs_cmd *cmd, int n)
> +{
> + struct rpmh_request *req;
> +
> + if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
unsigned n?
> + return ERR_PTR(-EINVAL);
> +
> + req = kcalloc(1, sizeof(*req), GFP_ATOMIC);
kzalloc()?
> + if (!req)
> + return ERR_PTR(-ENOMEM);
> +
> + memcpy(req->cmd, cmd, n * sizeof(*cmd));
> +
> + req->msg.state = state;
> + req->msg.payload = req->cmd;
> + req->msg.num_payload = n;
> + req->free = req;
> +
> + return req;
> +}
> +
> +/**
> + * rpmh_write_async: Write a set of RPMH commands
... and don't wait for a result?
> + *
> + * @rc: The RPMh handle got from rpmh_get_dev_channel
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in payload
> + *
> + * Write a set of RPMH commands, the order of commands is maintained
> + * and will be sent as a single shot.
> + */
> +int rpmh_write_async(struct rpmh_client *rc, enum rpmh_state state,
> + struct tcs_cmd *cmd, int n)
> +{
> + struct rpmh_request *rpm_msg;
> +
> + rpm_msg = __get_rpmh_msg_async(rc, state, cmd, n);
> + if (IS_ERR(rpm_msg))
> + return PTR_ERR(rpm_msg);
> +
> + return __rpmh_write(rc, state, rpm_msg);
> +}
> +EXPORT_SYMBOL(rpmh_write_async);
> +
> /**
> * rpmh_write: Write a set of RPMH commands and block until response
> *
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (7 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 21:59 ` Stephen Boyd
2018-03-02 16:43 ` [PATCH v3 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS Lina Iyer
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, 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 <ilina@codeaurora.org>
---
drivers/soc/qcom/rpmh.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++
include/soc/qcom/rpmh.h | 8 +++
2 files changed, 158 insertions(+)
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index a02d9f685b2b..19e84b031c0d 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -22,6 +22,7 @@
#define RPMH_MAX_MBOXES 2
#define RPMH_TIMEOUT (10 * HZ)
+#define RPMH_MAX_REQ_IN_BATCH 10
#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \
struct rpmh_request name = { \
@@ -81,12 +82,14 @@ struct rpmh_request {
* @cache: the list of cached requests
* @lock: synchronize access to the controller data
* @dirty: was the cache updated since flush
+ * @batch_cache: Cache sleep and wake requests sent as batch
*/
struct rpmh_ctrlr {
struct rsc_drv *drv;
struct list_head cache;
spinlock_t lock;
bool dirty;
+ struct rpmh_request *batch_cache[2 * RPMH_MAX_REQ_IN_BATCH];
};
/**
@@ -343,6 +346,146 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
}
EXPORT_SYMBOL(rpmh_write);
+static int cache_batch(struct rpmh_client *rc,
+ struct rpmh_request **rpm_msg, int count)
+{
+ struct rpmh_ctrlr *rpm = rc->ctrlr;
+ unsigned long flags;
+ int ret = 0;
+ int index = 0;
+ int i;
+
+ spin_lock_irqsave(&rpm->lock, flags);
+ while (rpm->batch_cache[index])
+ index++;
+ if (index + count >= 2 * RPMH_MAX_REQ_IN_BATCH) {
+ ret = -ENOMEM;
+ goto fail;
+ }
+
+ for (i = 0; i < count; i++)
+ rpm->batch_cache[index + i] = rpm_msg[i];
+fail:
+ spin_unlock_irqrestore(&rpm->lock, flags);
+
+ return ret;
+}
+
+static int flush_batch(struct rpmh_client *rc)
+{
+ struct rpmh_ctrlr *rpm = rc->ctrlr;
+ struct rpmh_request *rpm_msg;
+ unsigned long flags;
+ int ret = 0;
+ int i;
+
+ /* Send Sleep/Wake requests to the controller, expect no response */
+ spin_lock_irqsave(&rpm->lock, flags);
+ for (i = 0; rpm->batch_cache[i]; i++) {
+ rpm_msg = rpm->batch_cache[i];
+ ret = rpmh_rsc_write_ctrl_data(rc->ctrlr->drv, &rpm_msg->msg);
+ if (ret)
+ break;
+ }
+ spin_unlock_irqrestore(&rpm->lock, flags);
+
+ return ret;
+}
+
+static void invalidate_batch(struct rpmh_client *rc)
+{
+ struct rpmh_ctrlr *rpm = rc->ctrlr;
+ unsigned long flags;
+ int index = 0;
+ int i;
+
+ spin_lock_irqsave(&rpm->lock, flags);
+ while (rpm->batch_cache[index])
+ index++;
+ for (i = 0; i < index; i++) {
+ kfree(rpm->batch_cache[i]->free);
+ rpm->batch_cache[i] = NULL;
+ }
+ spin_unlock_irqrestore(&rpm->lock, flags);
+}
+
+/**
+ * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
+ * batch to finish.
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The array of count of elements in each batch, 0 terminated.
+ *
+ * Write a request to the mailbox controller without caching. If the request
+ * state is ACTIVE, then the requests are treated as completion request
+ * and sent to the controller immediately. The function waits until all the
+ * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
+ * request is sent as fire-n-forget and no ack is expected.
+ *
+ * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
+ */
+int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int *n)
+{
+ struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL };
+ DECLARE_COMPLETION_ONSTACK(compl);
+ atomic_t wait_count = ATOMIC_INIT(0); /* overwritten */
+ int count = 0;
+ int ret, i, j;
+
+ if (IS_ERR_OR_NULL(rc) || !cmd || !n)
+ return -EINVAL;
+
+ while (n[count++] > 0)
+ ;
+ count--;
+ if (!count || count > RPMH_MAX_REQ_IN_BATCH)
+ return -EINVAL;
+
+ /* Create async request batches */
+ for (i = 0; i < count; i++) {
+ rpm_msg[i] = __get_rpmh_msg_async(rc, state, cmd, n[i]);
+ if (IS_ERR_OR_NULL(rpm_msg[i])) {
+ for (j = 0 ; j < i; j++)
+ kfree(rpm_msg[j]->free);
+ return PTR_ERR(rpm_msg[i]);
+ }
+ cmd += n[i];
+ }
+
+ /* Send if Active and wait for the whole set to complete */
+ if (state == RPMH_ACTIVE_ONLY_STATE) {
+ might_sleep();
+ atomic_set(&wait_count, count);
+ for (i = 0; i < count; i++) {
+ rpm_msg[i]->completion = &compl;
+ rpm_msg[i]->wait_count = &wait_count;
+ /* Bypass caching and write to mailbox directly */
+ ret = rpmh_rsc_send_data(rc->ctrlr->drv,
+ &rpm_msg[i]->msg);
+ if (ret < 0) {
+ pr_err(
+ "Error(%d) sending RPMH message addr=0x%x\n",
+ ret, rpm_msg[i]->msg.payload[0].addr);
+ break;
+ }
+ }
+ /* For those unsent requests, spoof tx_done */
+ for (j = i; j < count; j++)
+ rpmh_tx_done(&rpm_msg[j]->msg, ret);
+ return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data);
+ }
+
+ /*
+ * Cache sleep/wake data in store.
+ * But flush batch first before flushing all other data.
+ */
+ return cache_batch(rc, rpm_msg, count);
+}
+EXPORT_SYMBOL(rpmh_write_batch);
+
static int is_req_valid(struct cache_req *req)
{
return (req->sleep_val != UINT_MAX &&
@@ -391,6 +534,11 @@ int rpmh_flush(struct rpmh_client *rc)
return 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.
@@ -433,6 +581,8 @@ int rpmh_invalidate(struct rpmh_client *rc)
if (IS_ERR_OR_NULL(rc))
return -EINVAL;
+ invalidate_batch(rc);
+
spin_lock_irqsave(&rpm->lock, flags);
rpm->dirty = true;
spin_unlock_irqrestore(&rpm->lock, flags);
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index 172a649f1a1c..6dc1042534d6 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -18,6 +18,9 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
int rpmh_write_async(struct rpmh_client *rc, enum rpmh_state state,
struct tcs_cmd *cmd, int n);
+int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
+ struct tcs_cmd *cmd, int *n);
+
struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
int rpmh_flush(struct rpmh_client *rc);
@@ -40,6 +43,11 @@ static inline int rpmh_write_async(struct rpmh_client *rc,
struct tcs_cmd *cmd, int n)
{ return -ENODEV; }
+static inline int rpmh_write_batch(struct rpmh_client *rc,
+ enum rpmh_state state,
+ struct tcs_cmd *cmd, int *n)
+{ return -ENODEV; }
+
static inline int rpmh_flush(struct rpmh_client *rc)
{ return -ENODEV; }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request
2018-03-02 16:43 ` [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request Lina Iyer
@ 2018-03-08 21:59 ` Stephen Boyd
2018-03-08 22:55 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 21:59 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:16)
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index a02d9f685b2b..19e84b031c0d 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -22,6 +22,7 @@
>
> #define RPMH_MAX_MBOXES 2
> #define RPMH_TIMEOUT (10 * HZ)
> +#define RPMH_MAX_REQ_IN_BATCH 10
Is 10 some software limit? Or hardware always has 10 available?
>
> #define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \
> struct rpmh_request name = { \
> @@ -81,12 +82,14 @@ struct rpmh_request {
> * @cache: the list of cached requests
> * @lock: synchronize access to the controller data
> * @dirty: was the cache updated since flush
> + * @batch_cache: Cache sleep and wake requests sent as batch
> */
> struct rpmh_ctrlr {
> struct rsc_drv *drv;
> struct list_head cache;
> spinlock_t lock;
> bool dirty;
> + struct rpmh_request *batch_cache[2 * RPMH_MAX_REQ_IN_BATCH];
Can it be const?
> };
>
> /**
> @@ -343,6 +346,146 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> }
> EXPORT_SYMBOL(rpmh_write);
>
> +static int cache_batch(struct rpmh_client *rc,
> + struct rpmh_request **rpm_msg, int count)
> +{
> + struct rpmh_ctrlr *rpm = rc->ctrlr;
> + unsigned long flags;
> + int ret = 0;
> + int index = 0;
> + int i;
> +
> + spin_lock_irqsave(&rpm->lock, flags);
> + while (rpm->batch_cache[index])
If batch_cache is full.
And if adjacent memory has bits set....
This loop can go forever?
Please add bounds.
> + index++;
> + if (index + count >= 2 * RPMH_MAX_REQ_IN_BATCH) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + for (i = 0; i < count; i++)
> + rpm->batch_cache[index + i] = rpm_msg[i];
> +fail:
> + spin_unlock_irqrestore(&rpm->lock, flags);
> +
> + return ret;
> +}
> +
[...]
> +static void invalidate_batch(struct rpmh_client *rc)
> +{
> + struct rpmh_ctrlr *rpm = rc->ctrlr;
> + unsigned long flags;
> + int index = 0;
> + int i;
> +
> + spin_lock_irqsave(&rpm->lock, flags);
> + while (rpm->batch_cache[index])
> + index++;
> + for (i = 0; i < index; i++) {
> + kfree(rpm->batch_cache[i]->free);
> + rpm->batch_cache[i] = NULL;
> + }
> + spin_unlock_irqrestore(&rpm->lock, flags);
> +}
> +
> +/**
> + * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
> + * batch to finish.
> + *
> + * @rc: The RPMh handle got from rpmh_get_dev_channel
Is the API called rpmh_get_dev_channel()?
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The array of count of elements in each batch, 0 terminated.
> + *
> + * Write a request to the mailbox controller without caching. If the request
> + * state is ACTIVE, then the requests are treated as completion request
> + * and sent to the controller immediately. The function waits until all the
> + * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
> + * request is sent as fire-n-forget and no ack is expected.
> + *
> + * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
> + */
> +int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
> + struct tcs_cmd *cmd, int *n)
I'm lost why n is a pointer, and cmd is not a double pointer if n stays
as a pointer. Are there clients calling this API with a contiguous chunk
of commands but then they want to break that chunk up into many
requests? Maybe I've lost track of commands and requests and how they
differ.
> +{
> + struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL };
> + DECLARE_COMPLETION_ONSTACK(compl);
> + atomic_t wait_count = ATOMIC_INIT(0); /* overwritten */
> + int count = 0;
> + int ret, i, j;
> +
> + if (IS_ERR_OR_NULL(rc) || !cmd || !n)
> + return -EINVAL;
> +
> + while (n[count++] > 0)
> + ;
> + count--;
> + if (!count || count > RPMH_MAX_REQ_IN_BATCH)
> + return -EINVAL;
> +
> + /* Create async request batches */
> + for (i = 0; i < count; i++) {
> + rpm_msg[i] = __get_rpmh_msg_async(rc, state, cmd, n[i]);
> + if (IS_ERR_OR_NULL(rpm_msg[i])) {
> + for (j = 0 ; j < i; j++)
Weird space before that ;
Also, why not use 'i' again and decrement? ret could be assigned
PTR_ERR() value and make the next potential problem go away.
> + kfree(rpm_msg[j]->free);
I hope rpm_msg[j]->free doesn't point to rpm_msg[i] here.
> + return PTR_ERR(rpm_msg[i]);
> + }
> + cmd += n[i];
> + }
> +
> + /* Send if Active and wait for the whole set to complete */
> + if (state == RPMH_ACTIVE_ONLY_STATE) {
> + might_sleep();
> + atomic_set(&wait_count, count);
Aha, here's the wait counter.
> + for (i = 0; i < count; i++) {
> + rpm_msg[i]->completion = &compl;
> + rpm_msg[i]->wait_count = &wait_count;
But then we just assign the same count and completion to each rpm_msg?
Why? Can't we just put the completion on the final one and have the
completion called there?
> + /* Bypass caching and write to mailbox directly */
> + ret = rpmh_rsc_send_data(rc->ctrlr->drv,
> + &rpm_msg[i]->msg);
> + if (ret < 0) {
> + pr_err(
> + "Error(%d) sending RPMH message addr=0x%x\n",
> + ret, rpm_msg[i]->msg.payload[0].addr);
> + break;
> + }
> + }
> + /* For those unsent requests, spoof tx_done */
Why? Comments shouldn't say what the code is doing, but explain why
things don't make sense.
> + for (j = i; j < count; j++)
> + rpmh_tx_done(&rpm_msg[j]->msg, ret);
> + return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data);
> + }
> +
> + /*
> + * Cache sleep/wake data in store.
> + * But flush batch first before flushing all other data.
> + */
> + return cache_batch(rc, rpm_msg, count);
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request
2018-03-08 21:59 ` Stephen Boyd
@ 2018-03-08 22:55 ` Lina Iyer
2018-03-16 17:00 ` Stephen Boyd
0 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-08 22:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Thu, Mar 08 2018 at 14:59 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-02 08:43:16)
>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>> index a02d9f685b2b..19e84b031c0d 100644
>> --- a/drivers/soc/qcom/rpmh.c
>> +++ b/drivers/soc/qcom/rpmh.c
>> @@ -22,6 +22,7 @@
>>
>> #define RPMH_MAX_MBOXES 2
>> #define RPMH_TIMEOUT (10 * HZ)
>> +#define RPMH_MAX_REQ_IN_BATCH 10
>
>Is 10 some software limit? Or hardware always has 10 available?
>
Arbitary s/w limit.
>>
>> #define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \
>> struct rpmh_request name = { \
>> @@ -81,12 +82,14 @@ struct rpmh_request {
>> * @cache: the list of cached requests
>> * @lock: synchronize access to the controller data
>> * @dirty: was the cache updated since flush
>> + * @batch_cache: Cache sleep and wake requests sent as batch
>> */
>> struct rpmh_ctrlr {
>> struct rsc_drv *drv;
>> struct list_head cache;
>> spinlock_t lock;
>> bool dirty;
>> + struct rpmh_request *batch_cache[2 * RPMH_MAX_REQ_IN_BATCH];
>
>Can it be const?
>
Yes, fixed.
>> };
>>
>> /**
>> @@ -343,6 +346,146 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> }
>> EXPORT_SYMBOL(rpmh_write);
>>
>> +static int cache_batch(struct rpmh_client *rc,
>> + struct rpmh_request **rpm_msg, int count)
>> +{
>> + struct rpmh_ctrlr *rpm = rc->ctrlr;
>> + unsigned long flags;
>> + int ret = 0;
>> + int index = 0;
>> + int i;
>> +
>> + spin_lock_irqsave(&rpm->lock, flags);
>> + while (rpm->batch_cache[index])
>
>If batch_cache is full.
>And if adjacent memory has bits set....
>
>This loop can go forever?
>
>Please add bounds.
>
How so? The if() below will ensure that it will not exceed bounds.
>> + index++;
>> + if (index + count >= 2 * RPMH_MAX_REQ_IN_BATCH) {
>> + ret = -ENOMEM;
>> + goto fail;
>> + }
>> +
>> + for (i = 0; i < count; i++)
>> + rpm->batch_cache[index + i] = rpm_msg[i];
>> +fail:
>> + spin_unlock_irqrestore(&rpm->lock, flags);
>> +
>> + return ret;
>> +}
>> +
>[...]
>> +static void invalidate_batch(struct rpmh_client *rc)
>> +{
>> + struct rpmh_ctrlr *rpm = rc->ctrlr;
>> + unsigned long flags;
>> + int index = 0;
>> + int i;
>> +
>> + spin_lock_irqsave(&rpm->lock, flags);
>> + while (rpm->batch_cache[index])
>> + index++;
>> + for (i = 0; i < index; i++) {
>> + kfree(rpm->batch_cache[i]->free);
>> + rpm->batch_cache[i] = NULL;
>> + }
>> + spin_unlock_irqrestore(&rpm->lock, flags);
>> +}
>> +
>> +/**
>> + * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
>> + * batch to finish.
>> + *
>> + * @rc: The RPMh handle got from rpmh_get_dev_channel
>
>Is the API called rpmh_get_dev_channel()?
>
Old code. Will fix in this and other places.
>> + * @state: Active/sleep set
>> + * @cmd: The payload data
>> + * @n: The array of count of elements in each batch, 0 terminated.
>> + *
>> + * Write a request to the mailbox controller without caching. If the request
>> + * state is ACTIVE, then the requests are treated as completion request
>> + * and sent to the controller immediately. The function waits until all the
>> + * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
>> + * request is sent as fire-n-forget and no ack is expected.
>> + *
>> + * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
>> + */
>> +int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
>> + struct tcs_cmd *cmd, int *n)
>
>I'm lost why n is a pointer, and cmd is not a double pointer if n stays
>as a pointer. Are there clients calling this API with a contiguous chunk
>of commands but then they want to break that chunk up into many
>requests?
>
That is correct. Clients want to provide a big buffer that this API will
break it up into requests specified in *n.
>Maybe I've lost track of commands and requests and how they
>differ.
>
>> +{
>> + struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL };
>> + DECLARE_COMPLETION_ONSTACK(compl);
>> + atomic_t wait_count = ATOMIC_INIT(0); /* overwritten */
>> + int count = 0;
>> + int ret, i, j;
>> +
>> + if (IS_ERR_OR_NULL(rc) || !cmd || !n)
>> + return -EINVAL;
>> +
>> + while (n[count++] > 0)
>> + ;
>> + count--;
>> + if (!count || count > RPMH_MAX_REQ_IN_BATCH)
>> + return -EINVAL;
>> +
>> + /* Create async request batches */
>> + for (i = 0; i < count; i++) {
>> + rpm_msg[i] = __get_rpmh_msg_async(rc, state, cmd, n[i]);
>> + if (IS_ERR_OR_NULL(rpm_msg[i])) {
>> + for (j = 0 ; j < i; j++)
>
>Weird space before that ;
>
Ok.
>Also, why not use 'i' again and decrement? ret could be assigned
>PTR_ERR() value and make the next potential problem go away.
>
Ok
>> + kfree(rpm_msg[j]->free);
>
>I hope rpm_msg[j]->free doesn't point to rpm_msg[i] here.
>
It doesn't.
>> + return PTR_ERR(rpm_msg[i]);
>> + }
>> + cmd += n[i];
>> + }
>> +
>> + /* Send if Active and wait for the whole set to complete */
>> + if (state == RPMH_ACTIVE_ONLY_STATE) {
>> + might_sleep();
>> + atomic_set(&wait_count, count);
>
>Aha, here's the wait counter.
>
:)
I am removing it from the earlier patch and introducing the wait_count
here. Not bad as I though.
>> + for (i = 0; i < count; i++) {
>> + rpm_msg[i]->completion = &compl;
>> + rpm_msg[i]->wait_count = &wait_count;
>
>But then we just assign the same count and completion to each rpm_msg?
>Why? Can't we just put the completion on the final one and have the
>completion called there?
>
The order of the responses is not gauranteed to be sequential and in the
order it was sent. So we have to do this.
>> + /* Bypass caching and write to mailbox directly */
>> + ret = rpmh_rsc_send_data(rc->ctrlr->drv,
>> + &rpm_msg[i]->msg);
>> + if (ret < 0) {
>> + pr_err(
>> + "Error(%d) sending RPMH message addr=0x%x\n",
>> + ret, rpm_msg[i]->msg.payload[0].addr);
>> + break;
>> + }
>> + }
>> + /* For those unsent requests, spoof tx_done */
>
>Why? Comments shouldn't say what the code is doing, but explain why
>things don't make sense.
>
Will remove..
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request
2018-03-08 22:55 ` Lina Iyer
@ 2018-03-16 17:00 ` Stephen Boyd
2018-03-26 15:30 ` Lina Iyer
0 siblings, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2018-03-16 17:00 UTC (permalink / raw)
To: Lina Iyer
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
Quoting Lina Iyer (2018-03-08 14:55:40)
> On Thu, Mar 08 2018 at 14:59 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-03-02 08:43:16)
> >> @@ -343,6 +346,146 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> >> }
> >> EXPORT_SYMBOL(rpmh_write);
> >>
> >> +static int cache_batch(struct rpmh_client *rc,
> >> + struct rpmh_request **rpm_msg, int count)
> >> +{
> >> + struct rpmh_ctrlr *rpm = rc->ctrlr;
> >> + unsigned long flags;
> >> + int ret = 0;
> >> + int index = 0;
> >> + int i;
> >> +
> >> + spin_lock_irqsave(&rpm->lock, flags);
> >> + while (rpm->batch_cache[index])
> >
> >If batch_cache is full.
> >And if adjacent memory has bits set....
> >
> >This loop can go forever?
> >
> >Please add bounds.
> >
> How so? The if() below will ensure that it will not exceed bounds.
Right, the if below will make sure we don't run off the end, but unless
rpm->batch_cache has a sentinel at the end we can't guarantee we won't
run off the end of the array and into some other portion of memory that
also has a bit set in a word. And then we may read into some unallocated
space. Or maybe I missed something.
>
> >> + index++;
> >> + if (index + count >= 2 * RPMH_MAX_REQ_IN_BATCH) {
> >> + ret = -ENOMEM;
> >> + goto fail;
> >> + }
> >> +
> >> + for (i = 0; i < count; i++)
> >> + rpm->batch_cache[index + i] = rpm_msg[i];
> >> +fail:
> >> + spin_unlock_irqrestore(&rpm->lock, flags);
> >> +
> >> + return ret;
> >> +}
> >> +
>
> >> + * @state: Active/sleep set
> >> + * @cmd: The payload data
> >> + * @n: The array of count of elements in each batch, 0 terminated.
> >> + *
> >> + * Write a request to the mailbox controller without caching. If the request
> >> + * state is ACTIVE, then the requests are treated as completion request
> >> + * and sent to the controller immediately. The function waits until all the
> >> + * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
> >> + * request is sent as fire-n-forget and no ack is expected.
> >> + *
> >> + * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
> >> + */
> >> +int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
> >> + struct tcs_cmd *cmd, int *n)
> >
> >I'm lost why n is a pointer, and cmd is not a double pointer if n stays
> >as a pointer. Are there clients calling this API with a contiguous chunk
> >of commands but then they want to break that chunk up into many
> >requests?
> >
> That is correct. Clients want to provide a big buffer that this API will
> break it up into requests specified in *n.
Is that for bus scaling?
> >> + return PTR_ERR(rpm_msg[i]);
> >> + }
> >> + cmd += n[i];
> >> + }
> >> +
> >> + /* Send if Active and wait for the whole set to complete */
> >> + if (state == RPMH_ACTIVE_ONLY_STATE) {
> >> + might_sleep();
> >> + atomic_set(&wait_count, count);
> >
> >Aha, here's the wait counter.
> >
> :)
> I am removing it from the earlier patch and introducing the wait_count
> here. Not bad as I though.
Thanks!
>
> >> + for (i = 0; i < count; i++) {
> >> + rpm_msg[i]->completion = &compl;
> >> + rpm_msg[i]->wait_count = &wait_count;
> >
> >But then we just assign the same count and completion to each rpm_msg?
> >Why? Can't we just put the completion on the final one and have the
> >completion called there?
> >
> The order of the responses is not gauranteed to be sequential and in the
> order it was sent. So we have to do this.
OK! That is sad.
>
> >> + /* Bypass caching and write to mailbox directly */
> >> + ret = rpmh_rsc_send_data(rc->ctrlr->drv,
> >> + &rpm_msg[i]->msg);
> >> + if (ret < 0) {
> >> + pr_err(
> >> + "Error(%d) sending RPMH message addr=0x%x\n",
> >> + ret, rpm_msg[i]->msg.payload[0].addr);
> >> + break;
> >> + }
> >> + }
> >> + /* For those unsent requests, spoof tx_done */
> >
> >Why? Comments shouldn't say what the code is doing, but explain why
> >things don't make sense.
> >
> Will remove..
>
Oh, I was hoping for more details, not less.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request
2018-03-16 17:00 ` Stephen Boyd
@ 2018-03-26 15:30 ` Lina Iyer
0 siblings, 0 replies; 36+ messages in thread
From: Lina Iyer @ 2018-03-26 15:30 UTC (permalink / raw)
To: Stephen Boyd
Cc: andy.gross, david.brown, linux-arm-msm, linux-soc, rnayak,
bjorn.andersson, linux-kernel
On Fri, Mar 16 2018 at 11:00 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-03-08 14:55:40)
>> On Thu, Mar 08 2018 at 14:59 -0700, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2018-03-02 08:43:16)
>> >> @@ -343,6 +346,146 @@ int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> >> }
>> >> EXPORT_SYMBOL(rpmh_write);
>> >>
>> >> +static int cache_batch(struct rpmh_client *rc,
>> >> + struct rpmh_request **rpm_msg, int count)
>> >> +{
>> >> + struct rpmh_ctrlr *rpm = rc->ctrlr;
>> >> + unsigned long flags;
>> >> + int ret = 0;
>> >> + int index = 0;
>> >> + int i;
>> >> +
>> >> + spin_lock_irqsave(&rpm->lock, flags);
>> >> + while (rpm->batch_cache[index])
>> >
>> >If batch_cache is full.
>> >And if adjacent memory has bits set....
>> >
>> >This loop can go forever?
>> >
>> >Please add bounds.
>> >
>> How so? The if() below will ensure that it will not exceed bounds.
>
>Right, the if below will make sure we don't run off the end, but unless
>rpm->batch_cache has a sentinel at the end we can't guarantee we won't
>run off the end of the array and into some other portion of memory that
>also has a bit set in a word. And then we may read into some unallocated
>space. Or maybe I missed something.
>
The rpmh_write_batch checks to make sure the number of requests do not
exceed the max and would not exceed the value. This is ensured by the
write_batch request.
A write_batch follows an invalidate and therefore would ensure that that
the batch_cache does not overflow, but I can add a simple check, though
its unnecessary with the general use of this API.
>>
>> >> + index++;
>> >> + if (index + count >= 2 * RPMH_MAX_REQ_IN_BATCH) {
>> >> + ret = -ENOMEM;
>> >> + goto fail;
>> >> + }
>> >> +
>> >> + for (i = 0; i < count; i++)
>> >> + rpm->batch_cache[index + i] = rpm_msg[i];
>> >> +fail:
>> >> + spin_unlock_irqrestore(&rpm->lock, flags);
>> >> +
>> >> + return ret;
>> >> +}
>> >> +
>>
>> >> + * @state: Active/sleep set
>> >> + * @cmd: The payload data
>> >> + * @n: The array of count of elements in each batch, 0 terminated.
>> >> + *
>> >> + * Write a request to the mailbox controller without caching. If the request
>> >> + * state is ACTIVE, then the requests are treated as completion request
>> >> + * and sent to the controller immediately. The function waits until all the
>> >> + * commands are complete. If the request was to SLEEP or WAKE_ONLY, then the
>> >> + * request is sent as fire-n-forget and no ack is expected.
>> >> + *
>> >> + * May sleep. Do not call from atomic contexts for ACTIVE_ONLY requests.
>> >> + */
>> >> +int rpmh_write_batch(struct rpmh_client *rc, enum rpmh_state state,
>> >> + struct tcs_cmd *cmd, int *n)
>> >
>> >I'm lost why n is a pointer, and cmd is not a double pointer if n stays
>> >as a pointer. Are there clients calling this API with a contiguous chunk
>> >of commands but then they want to break that chunk up into many
>> >requests?
>> >
>> That is correct. Clients want to provide a big buffer that this API will
>> break it up into requests specified in *n.
>
>Is that for bus scaling?
>
Yes.
>> >> + /* For those unsent requests, spoof tx_done */
>> >
>> >Why? Comments shouldn't say what the code is doing, but explain why
>> >things don't make sense.
>> >
>> Will remove..
>>
>
>Oh, I was hoping for more details, not less.
Hmm.. I thought it was fairly obvious that we spoof tx_done so we could
complete when the wait_count reaches 0. I will add that to the comments.
Thanks,
Lina
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS
2018-03-02 16:43 [PATCH v3 00/10] drivers/qcom: add RPMH communication support Lina Iyer
` (8 preceding siblings ...)
2018-03-02 16:43 ` [PATCH v3 09/10] drivers: qcom: rpmh: add support for batch RPMH request Lina Iyer
@ 2018-03-02 16:43 ` Lina Iyer
2018-03-08 20:47 ` Stephen Boyd
9 siblings, 1 reply; 36+ messages in thread
From: Lina Iyer @ 2018-03-02 16:43 UTC (permalink / raw)
To: andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Some RSCs may only have sleep and wake TCS, i.e, there is no dedicated
TCS for active mode request, but drivers may still want to make active
requests from these RSCs. In such cases re-purpose the wake TCS to send
active state requests.
The requirement for this is that the driver is aware that the wake TCS
is being repurposed to send active request, hence the sleep and wake
TCSes be invalidated before the active request is sent.
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
drivers/soc/qcom/rpmh-rsc.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e9f5a1f387fd..d9cfa7aaf49c 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -220,6 +220,7 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
struct tcs_request *msg)
{
int type;
+ struct tcs_group *tcs;
switch (msg->state) {
case RPMH_ACTIVE_ONLY_STATE:
@@ -235,7 +236,22 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
return ERR_PTR(-EINVAL);
}
- return get_tcs_of_type(drv, type);
+ /*
+ * If we are making an active request on a RSC that does not have a
+ * dedicated TCS for active state use, then re-purpose a wake TCS to
+ * send active votes.
+ * NOTE: The driver must be aware that this RSC does not have a
+ * dedicated AMC, and therefore would invalidate the sleep and wake
+ * TCSes before making an active state request.
+ */
+ tcs = get_tcs_of_type(drv, type);
+ if (msg->state == RPMH_ACTIVE_ONLY_STATE && IS_ERR(tcs)) {
+ tcs = get_tcs_of_type(drv, WAKE_TCS);
+ if (!IS_ERR(tcs))
+ rpmh_rsc_invalidate(drv);
+ }
+
+ return tcs;
}
static void send_tcs_response(struct tcs_response *resp)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS
2018-03-02 16:43 ` [PATCH v3 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS Lina Iyer
@ 2018-03-08 20:47 ` Stephen Boyd
0 siblings, 0 replies; 36+ messages in thread
From: Stephen Boyd @ 2018-03-08 20:47 UTC (permalink / raw)
To: Lina Iyer, andy.gross, david.brown, linux-arm-msm, linux-soc
Cc: rnayak, bjorn.andersson, linux-kernel, Lina Iyer
Quoting Lina Iyer (2018-03-02 08:43:17)
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e9f5a1f387fd..d9cfa7aaf49c 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -220,6 +220,7 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
> struct tcs_request *msg)
> {
> int type;
> + struct tcs_group *tcs;
const?
>
> switch (msg->state) {
> case RPMH_ACTIVE_ONLY_STATE:
> @@ -235,7 +236,22 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
> return ERR_PTR(-EINVAL);
> }
>
> - return get_tcs_of_type(drv, type);
> + /*
> + * If we are making an active request on a RSC that does not have a
> + * dedicated TCS for active state use, then re-purpose a wake TCS to
> + * send active votes.
> + * NOTE: The driver must be aware that this RSC does not have a
> + * dedicated AMC, and therefore would invalidate the sleep and wake
> + * TCSes before making an active state request.
> + */
> + tcs = get_tcs_of_type(drv, type);
> + if (msg->state == RPMH_ACTIVE_ONLY_STATE && IS_ERR(tcs)) {
> + tcs = get_tcs_of_type(drv, WAKE_TCS);
> + if (!IS_ERR(tcs))
> + rpmh_rsc_invalidate(drv);
> + }
> +
> + return tcs;
^ permalink raw reply [flat|nested] 36+ messages in thread