[PATCH v7 00/10] drivers/qcom: add RPMH communication support

[PATCH v7 00/10] drivers/qcom: add RPMH communication support

[Lina Iyer]

Changes in v7:
- Rename 'm' and 'n' and use tcs_id and cmd_id instead
- Bug fix in find_match() and other review comments from Matthias
- Spinlock around get_rpmh_ctrlr()
- DT documentation example fixes
- Rebase on top of 4.16-rc2

Changes in v6:
- Remove tasklet in rpmh-rsc.c
- Remove rpmh_client and use struct device * instead
- Variable changes and bug fixes
- DT binding changes to describe all DRVs in the RSC
- Documentation and comment fixes

Changes in v5:
- Add Reviewed-by tags
- Rebase on top of 4.16

Changes in v4:
- Rename variables as suggested by Stephen and Evan
- Lot of minor syntax and style fixes
- Fix FTRACE compilation error
- Improve doc comments and DT description

Changes in v3:
- Address Steven's comments in FTRACE
- Fix DT documentation as suggested by Rob H
- Fix error handling in IRQ handler as suggested by Evan
- Remove locks in rpmh_flush()
- Improve comments

Changes in v2:
- Added sleep/wake, async and batch requests support
- Addressed Bjorn's comments
- Private FTRACE for drivers/soc/qcom as suggested by Steven
- Sparse checked on these patches
- Use SPDX license commenting sytle

This set of patches add the ability for platform drivers to make use of shared
resources in newer Qualcomm SoCs like SDM845. Resources that are shared between
multiple processors in a SoC are generally controlled by a dedicated remote
processor. The remote processor (Resource Power Manager or RPM in previous QCOM
SoCs) receives requests for resource state from other processors using the
shared resource, aggregates the request and applies the result on the shared
resource. SDM845 advances this concept and uses h/w (hardened I/P) blocks for
aggregating requests and applying the result on the resource. The resources
could be clocks, regulators or bandwidth requests for buses. This new
architecture is called RPM-hardened or RPMH in short.

Since this communication mechanism is completely hardware driven without a
processor intervention on the remote end, existing mechanisms like RPM-SMD are
no longer useful. Also, there is no serialization of data or is data is written
to a shared memory in this new format. The data used is different, unsigned 32
bits are used for representing an address, data and header. Each resource's
property is a unique u32 address and have pre-defined set of property specific
valid values. A request that comprises of <header, addr, data> is sent by
writing to a set of registers from Linux and transmitted to the remote slave
through an internal bus. The remote end aggregates this request along with
requests from other processors for the <addr> and applies the result.

The hardware block that houses this functionality is called Resource State
Coordinator or RSC. Inside the RSC are set of slots for sending RPMH requests
called Trigger Commands Sets (TCS). The set of patches are for writing the
requests into these TCSes and sending them to hardened IP blocks.

The driver design is split into two components. The RSC driver housed in
rpmh-rsc.c and the set of library functions in rpmh.c that frame the request and
transmit it using the controller. This first set of patches allow a simple
synchronous request to be made by the platform drivers. Future patches will add
more functionality that cater to complex drivers and use cases.

Please consider reviewing this patchset.

v1: https://www.spinics.net/lists/devicetree/msg210980.html
v2: https://lkml.org/lkml/2018/2/15/852
v3: https://lkml.org/lkml/2018/3/2/801
v4: https://lkml.org/lkml/2018/3/9/979
v5: https://lkml.org/lkml/2018/4/5/480
v6: https://lkml.org/lkml/2018/4/19/914

Lina Iyer (10):
  drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs
  dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs
  drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE
  drivers: qcom: rpmh: add RPMH helper functions
  drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS
  drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS
  drivers: qcom: rpmh: cache sleep/wake state requests
  drivers: qcom: rpmh: allow requests to be sent asynchronously
  drivers: qcom: rpmh: add support for batch RPMH request
  drivers: qcom: rpmh-rsc: allow active requests from wake TCS

 .../devicetree/bindings/soc/qcom/rpmh-rsc.txt | 132 ++++
 drivers/soc/qcom/Kconfig                      |  10 +
 drivers/soc/qcom/Makefile                     |   4 +
 drivers/soc/qcom/rpmh-internal.h              |  83 +++
 drivers/soc/qcom/rpmh-rsc.c                   | 677 ++++++++++++++++++
 drivers/soc/qcom/rpmh.c                       | 585 +++++++++++++++
 drivers/soc/qcom/trace-rpmh.h                 |  82 +++
 include/dt-bindings/soc/qcom,rpmh-rsc.h       |  14 +
 include/soc/qcom/rpmh.h                       |  51 ++
 include/soc/qcom/tcs.h                        |  56 ++
 10 files changed, 1694 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
 create mode 100644 drivers/soc/qcom/rpmh-internal.h
 create mode 100644 drivers/soc/qcom/rpmh-rsc.c
 create mode 100644 drivers/soc/qcom/rpmh.c
 create mode 100644 drivers/soc/qcom/trace-rpmh.h
 create mode 100644 include/dt-bindings/soc/qcom,rpmh-rsc.h
 create mode 100644 include/soc/qcom/rpmh.h
 create mode 100644 include/soc/qcom/tcs.h

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

[PATCH v7 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs

[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 request 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. Active mode TCSes (AMC) are used to
send requests immediately to the resource, while control TCS are used to
provide specific information to the controller. Sleep and Wake TCS send
sleep and wake requests, after and before the system halt respectively.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---

Changes in v7:
	- rename 'm' and 'n' variable names

Changes in v6:
	- Remove tasklet and response object
	- introduce rpm_write_tcs_cmd() function
	- rename tcs_irq_handler()
	- avoid bool in structures. Fix tcs.h.
Changes in v4:
	- lots of variable name changes as suggested by Stephen B
	- use of const for data pointers
	- fix comments and other code syntax
	- use of bitmap for tcs_in_use instead of atomic
---
 drivers/soc/qcom/Kconfig                |  10 +
 drivers/soc/qcom/Makefile               |   1 +
 drivers/soc/qcom/rpmh-internal.h        |  69 ++++
 drivers/soc/qcom/rpmh-rsc.c             | 481 ++++++++++++++++++++++++
 include/dt-bindings/soc/qcom,rpmh-rsc.h |  14 +
 include/soc/qcom/tcs.h                  |  56 +++
 6 files changed, 631 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 5c4535b545cc..1887e1b1f43b 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -56,6 +56,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 || 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..39d3a059ee50 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
 obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
+obj-$(CONFIG_QCOM_RPMH)	+=	rpmh-rsc.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
new file mode 100644
index 000000000000..cc29176f1303
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -0,0 +1,69 @@
+/* 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 <linux/bitmap.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;
+
+/**
+ * struct tcs_group: group of Trigger Command Sets (TCS) to send state requests
+ * to the controller
+ *
+ * @drv:       the controller
+ * @type:      type of the TCS in this group - active, sleep, wake
+ * @mask:      mask of the TCSes relative to all the TCSes in the RSC
+ * @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
+ * @lock:      lock for synchronizing this TCS writes
+ * @req:       requests that are sent from the TCS
+ */
+struct tcs_group {
+	struct rsc_drv *drv;
+	int type;
+	u32 mask;
+	u32 offset;
+	int num_tcs;
+	int ncpt;
+	spinlock_t lock;
+	const struct tcs_request *req[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * struct rsc_drv: the Direct Resource Voter (DRV) of the
+ * Resource State Coordinator controller (RSC)
+ *
+ * @name:       controller identifier
+ * @tcs_base:   start address of the TCS registers in this controller
+ * @id:         instance id in the controller (Direct Resource Voter)
+ * @num_tcs:    number of TCSes in this DRV
+ * @tcs:        TCS groups
+ * @tcs_in_use: s/w state of the TCS
+ * @lock:       synchronize state of the controller
+ */
+struct rsc_drv {
+	const char *name;
+	void __iomem *tcs_base;
+	int id;
+	int num_tcs;
+	struct tcs_group tcs[TCS_TYPE_NR];
+	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
+	spinlock_t lock;
+};
+
+
+int rpmh_rsc_send_data(struct rsc_drv *drv, const 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..829c4429aa20
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -0,0 +1,481 @@
+// 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/export.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.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 <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 u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
+{
+	return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+			     RSC_DRV_CMD_OFFSET * cmd_id);
+}
+
+static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id,
+			  u32 data)
+{
+	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+		       RSC_DRV_CMD_OFFSET * cmd_id);
+}
+
+static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data)
+{
+	writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+}
+
+static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
+			       u32 data)
+{
+	writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+	for (;;) {
+		if (data == readl(drv->tcs_base + reg +
+				  RSC_DRV_TCS_OFFSET * tcs_id))
+			break;
+		udelay(1);
+	}
+}
+
+static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
+{
+	return !test_bit(tcs_id, drv->tcs_in_use) &&
+	       read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+}
+
+static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+	return &drv->tcs[type];
+}
+
+static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
+					 const 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 const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
+						  int tcs_id)
+{
+	struct tcs_group *tcs;
+	int i;
+
+	for (i = 0; i < drv->num_tcs; i++) {
+		tcs = &drv->tcs[i];
+		if (tcs->mask & BIT(tcs_id))
+			return tcs->req[tcs_id - tcs->offset];
+	}
+
+	return NULL;
+}
+
+/**
+ * tcs_tx_done: TX Done interrupt handler
+ */
+static irqreturn_t tcs_tx_done(int irq, void *p)
+{
+	struct rsc_drv *drv = p;
+	int i, j;
+	unsigned long irq_status;
+	const struct tcs_request *req;
+	struct tcs_cmd *cmd;
+
+	irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
+
+	for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
+		req = get_req_from_tcs(drv, i);
+		if (!req) {
+			WARN_ON(1);
+			goto skip;
+		}
+
+		for (j = 0; j < req->num_cmds; j++) {
+			u32 sts;
+
+			cmd = &req->cmds[j];
+			sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, j);
+			if (!(sts & CMD_STATUS_ISSUED) ||
+			   ((req->wait_for_compl || cmd->wait) &&
+			   !(sts & CMD_STATUS_COMPL))) {
+				pr_err("Incomplete request: %s: addr=%#x data=%#x",
+				       drv->name, cmd->addr, cmd->data);
+			}
+		}
+skip:
+		/* Reclaim the TCS */
+		write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
+		write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i));
+		spin_lock(&drv->lock);
+		clear_bit(i, drv->tcs_in_use);
+		spin_unlock(&drv->lock);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
+			       const struct tcs_request *msg)
+{
+	u32 msgid, cmd_msgid;
+	u32 cmd_enable = 0;
+	u32 cmd_complete;
+	struct tcs_cmd *cmd;
+	int i, j;
+
+	cmd_msgid = CMD_MSGID_LEN;
+	cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
+	cmd_msgid |= CMD_MSGID_WRITE;
+
+	cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
+
+	for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
+		cmd = &msg->cmds[i];
+		cmd_enable |= BIT(j);
+		cmd_complete |= cmd->wait << j;
+		msgid = cmd_msgid;
+		msgid |= cmd->wait ? CMD_MSGID_RESP_REQ : 0;
+		write_tcs_cmd(drv, RSC_DRV_CMD_MSGID, tcs_id, j, msgid);
+		write_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j, cmd->addr);
+		write_tcs_cmd(drv, RSC_DRV_CMD_DATA, tcs_id, j, cmd->data);
+	}
+
+	write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete);
+	cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+	write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
+}
+
+static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
+{
+	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, tcs_id, 0);
+	enable &= ~TCS_AMC_MODE_TRIGGER;
+	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+	enable &= ~TCS_AMC_MODE_ENABLE;
+	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, 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, tcs_id, enable);
+	enable |= TCS_AMC_MODE_TRIGGER;
+	write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+}
+
+static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
+				  const struct tcs_request *msg)
+{
+	unsigned long curr_enabled;
+	u32 addr;
+	int i, j, k;
+	int tcs_id = tcs->offset;
+
+	for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
+		if (tcs_is_free(drv, tcs_id))
+			continue;
+
+		curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+
+		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
+			addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
+			for (k = 0; k < msg->num_cmds; k++) {
+				if (addr == msg->cmds[k].addr)
+					return -EBUSY;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int find_free_tcs(struct tcs_group *tcs)
+{
+	int i;
+
+	for (i = 0; i < tcs->num_tcs; i++) {
+		if (tcs_is_free(tcs->drv, tcs->offset + i))
+			return tcs->offset + i;
+	}
+
+	return -EBUSY;
+}
+
+static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+	struct tcs_group *tcs;
+	int tcs_id;
+	unsigned long flags;
+	int ret;
+
+	tcs = get_tcs_for_msg(drv, msg);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock_irqsave(&tcs->lock, flags);
+	spin_lock(&drv->lock);
+	/*
+	 * The h/w does not like if we send a request to the same address,
+	 * when one is already in-flight or being processed.
+	 */
+	ret = check_for_req_inflight(drv, tcs, msg);
+	if (ret) {
+		spin_unlock(&drv->lock);
+		goto done_write;
+	}
+
+	tcs_id = find_free_tcs(tcs);
+	if (tcs_id < 0) {
+		ret = tcs_id;
+		spin_unlock(&drv->lock);
+		goto done_write;
+	}
+
+	tcs->req[tcs_id - tcs->offset] = msg;
+	set_bit(tcs_id, drv->tcs_in_use);
+	spin_unlock(&drv->lock);
+
+	__tcs_buffer_write(drv, tcs_id, 0, msg);
+	__tcs_trigger(drv, tcs_id);
+
+done_write:
+	spin_unlock_irqrestore(&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, const struct tcs_request *msg)
+{
+	int ret;
+
+	if (!msg || !msg->cmds || !msg->num_cmds ||
+	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	do {
+		ret = tcs_write(drv, msg);
+		if (ret == -EBUSY) {
+			pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=%#x\n",
+					    msg->cmds[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;
+	char drv_id[10] = {0};
+
+	snprintf(drv_id, ARRAY_SIZE(drv_id), "drv-%d", drv->id);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, drv_id);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcs-offset");
+	drv->tcs_base = base + res->start;
+
+	config = readl_relaxed(base + DRV_PRNT_CHLD_CONFIG);
+
+	max_tcs = config;
+	max_tcs &= DRV_NUM_TCS_MASK << (DRV_NUM_TCS_SHIFT * drv->id);
+	max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->id);
+
+	ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
+	ncpt = ncpt >> DRV_NCPT_SHIFT;
+
+	n = of_property_count_u32_elems(dn, "qcom,tcs-config");
+	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->lock);
+
+		if (!tcs->num_tcs || tcs->type == CONTROL_TCS)
+			continue;
+
+		if (st + tcs->num_tcs > max_tcs ||
+		    st + tcs->num_tcs >= BITS_PER_BYTE * sizeof(tcs->mask))
+			return -EINVAL;
+
+		tcs->mask = ((1 << tcs->num_tcs) - 1) << st;
+		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 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->id);
+	if (ret)
+		return ret;
+
+	drv->name = of_get_property(dn, "label", NULL);
+	if (!drv->name)
+		drv->name = dev_name(&pdev->dev);
+
+	ret = rpmh_probe_tcs_config(pdev, drv);
+	if (ret)
+		return ret;
+
+	spin_lock_init(&drv->lock);
+	bitmap_zero(drv->tcs_in_use, MAX_TCS_NR);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(&pdev->dev, irq, tcs_tx_done,
+			       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, drv->tcs[ACTIVE_TCS].mask);
+
+	return devm_of_platform_populate(&pdev->dev);
+}
+
+static const struct of_device_id rpmh_drv_match[] = {
+	{ .compatible = "qcom,rpmh-rsc", },
+	{ }
+};
+
+static struct platform_driver rpmh_driver = {
+	.probe = rpmh_rsc_probe,
+	.driver = {
+		  .name = "rpmh",
+		  .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..262876a59e86
--- /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_SLEEP_STATE:       State of the resource when the processor subsystem
+ *                         is powered down. There is no client using the
+ *                         resource actively.
+ * RPMH_WAKE_ONLY_STATE:   Resume resource state to the value previously
+ *                         requested before the processor was powered down.
+ * 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,
+};
+
+/**
+ * struct 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
+ * @wait: wait for this request to be complete before sending the next
+ */
+struct tcs_cmd {
+	u32 addr;
+	u32 data;
+	u32 wait;
+};
+
+/**
+ * struct tcs_request: A set of tcs_cmds sent together in a TCS
+ *
+ * @state:          state for the request.
+ * @wait_for_compl: wait until we get a response from the h/w accelerator
+ * @num_cmds:       the number of @cmds in this request
+ * @cmds:           an array of tcs_cmds
+ */
+struct tcs_request {
+	enum rpmh_state state;
+	u32 wait_for_compl;
+	u32 num_cmds;
+	struct tcs_cmd *cmds;
+};
+
+#endif /* __SOC_QCOM_TCS_H__ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs

[Lina Iyer]

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

Cc: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v7:
	- Fix example

Changes in v6:
	- Address comments from Stephen Boyd

Changes in v3:
	- Move to soc/qcom
	- Amend text per Stephen's suggestions

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/soc/qcom/rpmh-rsc.txt | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
new file mode 100644
index 000000000000..950d56325284
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
@@ -0,0 +1,132 @@
+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 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 subsystems. Clients
+may request a sleep value for their shared resources in addition to the active
+mode requests.
+
+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(s). The number of DRVs in the dependent on the RSC.
+	            The tcs-offset specifies the start address of the
+	            TCS in the DRVs.
+
+- reg-names:
+	Usage: required
+	Value type: <string>
+	Definition: Maps the register specified in the reg property. Must be
+	            "drv-0", "drv-1", "drv-2" etc and "tcs-offset". The
+
+- 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 that will be used by
+		    this controller.
+
+- 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 nodes 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 -
+DRV0: 0x179C0000
+DRV2: 0x179C0000 + 0x10000 = 0x179D0000
+DRV2: 0x179C0000 + 0x10000 * 2 = 0x179E0000
+TCS-OFFSET: 0xD00
+
+	apps_rsc: rsc@179c0000 {
+		label = "apps_rsc";
+		compatible = "qcom,rpmh-rsc";
+		reg = <0x179c0000 0x10000>, <0x179d0000 0x10000>,
+		      <0x179e0000 0x10000>, <0xd00 0>;
+		reg-names = "drv-0", "drv-1", "drv-2", "tcs-offset";
+		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 -
+DRV0: 0xAF20000
+TCS-OFFSET: 0x1C00
+
+	disp_rsc: rsc@af20000 {
+		label = "disp_rsc";
+		compatible = "qcom,rpmh-rsc";
+		reg = <0xaf20000 0x10000>, <0x1c00 0>;
+		reg-names = "drv-0", "tcs-offset";
+		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

[PATCH v7 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE

[Lina Iyer]

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 v7:
	- varible name changes and white space

Changes in v6:
	- struct tcs_response was removed. Fix in trace as well.
Changes in v4:
	- fix compilation issues, use __assign_str
	- use %#x instead of 0x%08x
Changes in v3:
	- Use __string() instead of char *
	- fix TRACE_INCLUDE_PATH
---
 drivers/soc/qcom/Makefile     |  1 +
 drivers/soc/qcom/rpmh-rsc.c   | 11 ++++-
 drivers/soc/qcom/trace-rpmh.h | 82 +++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/trace-rpmh.h

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 39d3a059ee50..cb6300f6a8e9 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 829c4429aa20..b9ad2e7b1dea 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
 
@@ -136,7 +139,7 @@ static const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
 static irqreturn_t tcs_tx_done(int irq, void *p)
 {
 	struct rsc_drv *drv = p;
-	int i, j;
+	int i, j, err;
 	unsigned long irq_status;
 	const struct tcs_request *req;
 	struct tcs_cmd *cmd;
@@ -150,6 +153,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 			goto skip;
 		}
 
+		err = 0;
 		for (j = 0; j < req->num_cmds; j++) {
 			u32 sts;
 
@@ -160,8 +164,11 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 			   !(sts & CMD_STATUS_COMPL))) {
 				pr_err("Incomplete request: %s: addr=%#x data=%#x",
 				       drv->name, cmd->addr, cmd->data);
+				err = -EIO;
 			}
 		}
+
+		trace_rpmh_tx_done(drv, i, req, err);
 skip:
 		/* Reclaim the TCS */
 		write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
@@ -195,9 +202,11 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
 		cmd_complete |= cmd->wait << j;
 		msgid = cmd_msgid;
 		msgid |= cmd->wait ? CMD_MSGID_RESP_REQ : 0;
+
 		write_tcs_cmd(drv, RSC_DRV_CMD_MSGID, tcs_id, j, msgid);
 		write_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j, cmd->addr);
 		write_tcs_cmd(drv, RSC_DRV_CMD_DATA, tcs_id, j, cmd->data);
+		trace_rpmh_send_msg(drv, tcs_id, j, msgid, cmd);
 	}
 
 	write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete);
diff --git a/drivers/soc/qcom/trace-rpmh.h b/drivers/soc/qcom/trace-rpmh.h
new file mode 100644
index 000000000000..feb0cb455e37
--- /dev/null
+++ b/drivers/soc/qcom/trace-rpmh.h
@@ -0,0 +1,82 @@
+/* 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"
+
+TRACE_EVENT(rpmh_tx_done,
+
+	TP_PROTO(struct rsc_drv *d, int m, const struct tcs_request *r, int e),
+
+	TP_ARGS(d, m, r, e),
+
+	TP_STRUCT__entry(
+			 __string(name, d->name)
+			 __field(int, m)
+			 __field(u32, addr)
+			 __field(u32, data)
+			 __field(int, err)
+	),
+
+	TP_fast_assign(
+		       __assign_str(name, d->name);
+		       __entry->m = m;
+		       __entry->addr = r->cmds[0].addr;
+		       __entry->data = r->cmds[0].data;
+		       __entry->err = e;
+	),
+
+	TP_printk("%s: ack: tcs-m: %d addr: %#x data: %#x errno: %d",
+		  __get_str(name), __entry->m, __entry->addr, __entry->data,
+		  __entry->err)
+);
+
+TRACE_EVENT(rpmh_send_msg,
+
+	TP_PROTO(struct rsc_drv *d, int m, int n, u32 h,
+		 const 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, wait)
+	),
+
+	TP_fast_assign(
+		       __assign_str(name, d->name);
+		       __entry->m = m;
+		       __entry->n = n;
+		       __entry->hdr = h;
+		       __entry->addr = c->addr;
+		       __entry->data = c->data;
+		       __entry->wait = c->wait;
+	),
+
+	TP_printk("%s: send-msg: tcs(m): %d cmd(n): %d msgid: %#x addr: %#x data: %#x complete: %d",
+		  __get_str(name), __entry->m, __entry->n, __entry->hdr,
+		  __entry->addr, __entry->data, __entry->wait)
+);
+
+#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

[PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[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>
---

Changes in v7:
	- Optimization and locking fixes

Changes in v6:
	- replace rpmh_client with device
	- inline wait_for_tx_done()

Changes in v4:
	- use const struct tcs_cmd in API
	- remove wait count from this patch
	- changed -EFAULT to -EINVAL
---
 drivers/soc/qcom/Makefile        |   4 +-
 drivers/soc/qcom/rpmh-internal.h |   6 ++
 drivers/soc/qcom/rpmh-rsc.c      |   8 ++
 drivers/soc/qcom/rpmh.c          | 176 +++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h          |  25 +++++
 5 files changed, 218 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 cb6300f6a8e9..bb395c3202ca 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
 obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.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
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index cc29176f1303..d9a21726e568 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -14,6 +14,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 RPMH_MAX_CTRLR			2
 
 struct rsc_drv;
 
@@ -52,6 +53,7 @@ struct tcs_group {
  * @tcs:        TCS groups
  * @tcs_in_use: s/w state of the TCS
  * @lock:       synchronize state of the controller
+ * @list:       element in list of drv
  */
 struct rsc_drv {
 	const char *name;
@@ -61,9 +63,13 @@ struct rsc_drv {
 	struct tcs_group tcs[TCS_TYPE_NR];
 	DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
 	spinlock_t lock;
+	struct list_head list;
 };
 
+extern struct list_head rsc_drv_list;
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
 
+void rpmh_tx_done(const 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 b9ad2e7b1dea..33270b1d5991 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -61,6 +61,8 @@
 #define CMD_STATUS_ISSUED		BIT(8)
 #define CMD_STATUS_COMPL		BIT(16)
 
+LIST_HEAD(rsc_drv_list);
+
 static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
 {
 	return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
@@ -176,6 +178,8 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
 		spin_lock(&drv->lock);
 		clear_bit(i, drv->tcs_in_use);
 		spin_unlock(&drv->lock);
+		if (req)
+			rpmh_tx_done(req, err);
 	}
 
 	return IRQ_HANDLED;
@@ -467,6 +471,10 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
 	/* Enable the active TCS to send requests immediately */
 	write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, drv->tcs[ACTIVE_TCS].mask);
 
+	INIT_LIST_HEAD(&drv->list);
+	list_add(&drv->list, &rsc_drv_list);
+	dev_set_drvdata(&pdev->dev, drv);
+
 	return devm_of_platform_populate(&pdev->dev);
 }
 
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
new file mode 100644
index 000000000000..74bb82339b01
--- /dev/null
+++ b/drivers/soc/qcom/rpmh.c
@@ -0,0 +1,176 @@
+// 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/jiffies.h>
+#include <linux/kernel.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_TIMEOUT_MS			msecs_to_jiffies(10000)
+
+#define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name)	\
+	struct rpmh_request name = {			\
+		.msg = {				\
+			.state = s,			\
+			.cmds = name.cmd,		\
+			.num_cmds = 0,			\
+			.wait_for_compl = true,		\
+		},					\
+		.cmd = { { 0 } },			\
+		.completion = q,			\
+		.dev = dev,				\
+	}
+
+/**
+ * struct 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
+ * @dev: the device making the request
+ * @err: err return from the controller
+ */
+struct rpmh_request {
+	struct tcs_request msg;
+	struct tcs_cmd cmd[MAX_RPMH_PAYLOAD];
+	struct completion *completion;
+	const struct device *dev;
+	int err;
+};
+
+/**
+ * struct rpmh_ctrlr: our representation of the controller
+ *
+ * @drv: the controller instance
+ */
+struct rpmh_ctrlr {
+	struct rsc_drv *drv;
+};
+
+static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
+static DEFINE_SPINLOCK(rpmh_rsc_lock);
+
+static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
+{
+	int i;
+	struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
+	struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
+	unsigned long flags;
+
+	if (!drv)
+		return ctrlr;
+
+	for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+		if (rpmh_rsc[i].drv == drv) {
+			ctrlr = &rpmh_rsc[i];
+			return ctrlr;
+		}
+	}
+
+	spin_lock_irqsave(&rpmh_rsc_lock, flags);
+	list_for_each_entry(p, &rsc_drv_list, list) {
+		if (drv == p) {
+			for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+				if (!rpmh_rsc[i].drv)
+					break;
+			}
+			if (i == RPMH_MAX_CTRLR) {
+				ctrlr = ERR_PTR(-ENOMEM);
+				break;
+			}
+			rpmh_rsc[i].drv = drv;
+			ctrlr = &rpmh_rsc[i];
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
+
+	return ctrlr;
+}
+
+void rpmh_tx_done(const struct tcs_request *msg, int r)
+{
+	struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
+						    msg);
+	struct completion *compl = rpm_msg->completion;
+
+	rpm_msg->err = r;
+
+	if (r)
+		dev_err(rpm_msg->dev, "RPMH TX fail in msg addr=%#x, err=%d\n",
+			rpm_msg->msg.cmds[0].addr, r);
+
+	/* Signal the blocking thread we are done */
+	if (compl)
+		complete(compl);
+}
+EXPORT_SYMBOL(rpmh_tx_done);
+
+/**
+ * __rpmh_write: send the RPMH request
+ *
+ * @dev: The device making the request
+ * @state: Active/Sleep request type
+ * @rpm_msg: The data that needs to be sent (cmds).
+ */
+static int __rpmh_write(const struct device *dev, enum rpmh_state state,
+			struct rpmh_request *rpm_msg)
+{
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	rpm_msg->msg.state = state;
+
+	if (state != RPMH_ACTIVE_ONLY_STATE)
+		return -EINVAL;
+
+	WARN_ON(irqs_disabled());
+
+	return rpmh_rsc_send_data(ctrlr->drv, &rpm_msg->msg);
+}
+
+/**
+ * rpmh_write: Write a set of RPMH commands and block until response
+ *
+ * @rc: The RPMH handle got from rpmh_get_client
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in @cmd
+ *
+ * May sleep. Do not call from atomic contexts.
+ */
+int rpmh_write(const struct device *dev, enum rpmh_state state,
+	       const struct tcs_cmd *cmd, u32 n)
+{
+	DECLARE_COMPLETION_ONSTACK(compl);
+	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+	int ret;
+
+	if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
+		return -EINVAL;
+
+	memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
+	rpm_msg.msg.num_cmds = n;
+
+	ret = __rpmh_write(dev, state, &rpm_msg);
+	if (ret)
+		return ret;
+
+	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+}
+EXPORT_SYMBOL(rpmh_write);
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
new file mode 100644
index 000000000000..c1d0f902bd71
--- /dev/null
+++ b/include/soc/qcom/rpmh.h
@@ -0,0 +1,25 @@
+/* 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>
+
+
+#if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_write(const struct device *dev, enum rpmh_state state,
+	       const struct tcs_cmd *cmd, u32 n);
+
+#else
+
+static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
+			     const struct tcs_cmd *cmd, u32 n)
+{ return -ENODEV; }
+
+#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

[PATCH v7 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS

[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 the job of this
controller driver to find and arrange the requests into the available
TCSes.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>

---

Changes in v7:
	- Bug fix in find_match()
---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 118 +++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index d9a21726e568..6e19fe458c31 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -14,6 +14,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)
 #define RPMH_MAX_CTRLR			2
 
 struct rsc_drv;
@@ -30,6 +31,8 @@ struct rsc_drv;
  * @ncpt:      number of commands in each TCS
  * @lock:      lock for synchronizing this TCS writes
  * @req:       requests that are sent from the TCS
+ * @cmd_cache: flattened cache of cmds in sleep/wake TCS
+ * @slots:     indicates which of @cmd_addr are occupied
  */
 struct tcs_group {
 	struct rsc_drv *drv;
@@ -40,6 +43,8 @@ struct tcs_group {
 	int ncpt;
 	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
+	u32 *cmd_cache;
+	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
 };
 
 /**
@@ -69,6 +74,9 @@ struct rsc_drv {
 extern struct list_head rsc_drv_list;
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
+int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
+			     const struct tcs_request *msg);
+int rpmh_rsc_invalidate(struct rsc_drv *drv);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);
 
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 33270b1d5991..4e2144a14c31 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -113,6 +113,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);
 	}
@@ -353,6 +359,105 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 }
 EXPORT_SYMBOL(rpmh_rsc_send_data);
 
+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		if (tcs->cmd_cache[i] != cmd[0].addr)
+			continue;
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i + j] != cmd[j].addr) {
+				WARN(1, "Message does not match previous sequence.\n");
+				return -EINVAL;
+			}
+		}
+		return i;
+	}
+
+	return -ENODATA;
+}
+
+static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
+		      int *tcs_id, int *cmd_id)
+{
+	int slot, offset;
+	int i = 0;
+
+	/* Find if we already have the msg in our TCS */
+	slot = find_match(tcs, msg->cmds, msg->num_cmds);
+	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_cmds, 0);
+		if (slot == MAX_TCS_SLOTS)
+			return -ENOMEM;
+		i += tcs->ncpt;
+	} while (slot + msg->num_cmds - 1 >= i);
+
+copy_data:
+	bitmap_set(tcs->slots, slot, msg->num_cmds);
+	/* Copy the addresses of the resources over to the slots */
+	for (i = 0; i < msg->num_cmds; i++)
+		tcs->cmd_cache[slot + i] = msg->cmds[i].addr;
+
+	offset = slot / tcs->ncpt;
+	*tcs_id = offset + tcs->offset;
+	*cmd_id = slot % tcs->ncpt;
+
+	return 0;
+}
+
+static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
+{
+	struct tcs_group *tcs;
+	int tcs_id = 0, cmd_id = 0;
+	unsigned long flags;
+	int ret;
+
+	tcs = get_tcs_for_msg(drv, msg);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock_irqsave(&tcs->lock, flags);
+	/* find the m-th TCS and the n-th position in the TCS to write to */
+	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
+	if (!ret)
+		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
+	spin_unlock_irqrestore(&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, const struct tcs_request *msg)
+{
+	if (!msg || !msg->cmds || !msg->num_cmds ||
+	    msg->num_cmds > 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)
 {
@@ -426,6 +531,19 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
 		tcs->mask = ((1 << tcs->num_tcs) - 1) << st;
 		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_cache = devm_kcalloc(&pdev->dev,
+					      tcs->num_tcs * ncpt, sizeof(u32),
+					      GFP_KERNEL);
+		if (!tcs->cmd_cache)
+			return -ENOMEM;
 	}
 
 	drv->num_tcs = st;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS

[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>
---

Changes in v7:
	- Move bitmap_zero() outside the loop

Changes in v6:
	- remove unnecessary locks around __tcs_invalidate
	- rename function to tcs_invaldiate

Changes in v4:
	- refactored the rphm_rsc_invalidate()
---
 drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 4e2144a14c31..42aedf2d80fe 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 
+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;
+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);
+		return 0;
+	}
+
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			spin_unlock(&tcs->lock);
+			return -EAGAIN;
+		}
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
+	}
+	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
+	spin_unlock(&tcs->lock);
+
+	return 0;
+}
+
+/**
+ * rpmh_rsc_invalidate - Invalidate sleep and wake TCSes
+ *
+ * @drv: the RSC controller
+ */
+int rpmh_rsc_invalidate(struct rsc_drv *drv)
+{
+	int ret;
+
+	ret = tcs_invalidate(drv, SLEEP_TCS);
+	if (!ret)
+		ret = tcs_invalidate(drv, WAKE_TCS);
+
+	return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_invalidate);
+
 static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 					 const struct tcs_request *msg)
 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 07/10] drivers: qcom: rpmh: cache sleep/wake state requests

[Lina Iyer]

Active state requests are sent immediately to the RSC controller, while
sleep and wake state requests are cached in this driver to avoid taxing
the RSC 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>
Reviewed-by: Evan Green <evgreen@chromium.org>
---

Changes in v6:
	- replace rpmh_client with device *

Changes in v4:
	- remove locking for ->dirty in invalidate
	- fix send_single
Changes in v3:
	- Remove locking for flush function
	- Improve comments
---
 drivers/soc/qcom/rpmh.c | 217 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |  11 ++
 2 files changed, 223 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 74bb82339b01..23fcbd9487cd 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -7,10 +7,12 @@
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/list.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>
 
@@ -33,6 +35,21 @@
 		.dev = dev,				\
 	}
 
+/**
+ * struct 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;
+};
+
 /**
  * struct rpmh_request: the message to be sent to rpmh-rsc
  *
@@ -54,9 +71,15 @@ struct rpmh_request {
  * struct 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;
 };
 
 static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
@@ -91,6 +114,8 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
 				break;
 			}
 			rpmh_rsc[i].drv = drv;
+			spin_lock_init(&rpmh_rsc[i].lock);
+			INIT_LIST_HEAD(&rpmh_rsc[i].cache);
 			ctrlr = &rpmh_rsc[i];
 			break;
 		}
@@ -118,29 +143,109 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
 }
 EXPORT_SYMBOL(rpmh_tx_done);
 
+static struct cache_req *__find_req(struct rpmh_ctrlr *ctrlr, u32 addr)
+{
+	struct cache_req *p, *req = NULL;
+
+	list_for_each_entry(p, &ctrlr->cache, list) {
+		if (p->addr == addr) {
+			req = p;
+			break;
+		}
+	}
+
+	return req;
+}
+
+static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
+					   enum rpmh_state state,
+					   struct tcs_cmd *cmd)
+{
+	struct cache_req *req;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	req = __find_req(ctrlr, 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, &ctrlr->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;
+	};
+
+	ctrlr->dirty = true;
+unlock:
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+	return req;
+}
+
 /**
- * __rpmh_write: send the RPMH request
+ * __rpmh_write: Cache and send the RPMH request
  *
  * @dev: The device making the request
  * @state: Active/Sleep request type
  * @rpm_msg: The data that needs to be sent (cmds).
+ *
+ * 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(const struct device *dev, enum rpmh_state state,
 			struct rpmh_request *rpm_msg)
 {
 	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	int ret = -EINVAL;
+	struct cache_req *req;
+	int i;
 
 	if (IS_ERR(ctrlr))
 		return PTR_ERR(ctrlr);
 
 	rpm_msg->msg.state = state;
 
-	if (state != RPMH_ACTIVE_ONLY_STATE)
-		return -EINVAL;
+	/* Cache the request in our store and link the payload */
+	for (i = 0; i < rpm_msg->msg.num_cmds; i++) {
+		req = cache_rpm_request(ctrlr, state, &rpm_msg->msg.cmds[i]);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+	}
+
+	rpm_msg->msg.state = state;
 
-	WARN_ON(irqs_disabled());
+	if (state == RPMH_ACTIVE_ONLY_STATE) {
+		WARN_ON(irqs_disabled());
+		ret = rpmh_rsc_send_data(ctrlr->drv, &rpm_msg->msg);
+	} else {
+		ret = rpmh_rsc_write_ctrl_data(ctrlr->drv, &rpm_msg->msg);
+		/* Clean up our call by spoofing tx_done */
+		rpmh_tx_done(&rpm_msg->msg, ret);
+	}
 
-	return rpmh_rsc_send_data(ctrlr->drv, &rpm_msg->msg);
+	return ret;
 }
 
 /**
@@ -174,3 +279,105 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 	return (ret > 0) ? 0 : -ETIMEDOUT;
 }
 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(const struct device *dev, enum rpmh_state state,
+		       u32 addr, u32 data)
+{
+	DEFINE_RPMH_MSG_ONSTACK(dev, state, NULL, rpm_msg);
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	/* Wake sets are always complete and sleep sets are not */
+	rpm_msg.msg.wait_for_compl = (state == RPMH_WAKE_ONLY_STATE);
+	rpm_msg.cmd[0].addr = addr;
+	rpm_msg.cmd[0].data = data;
+	rpm_msg.msg.num_cmds = 1;
+
+	return rpmh_rsc_write_ctrl_data(ctrlr->drv, &rpm_msg.msg);
+}
+
+/**
+ * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ *
+ * @dev: The device making the request
+ *
+ * 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(const struct device *dev)
+{
+	struct cache_req *p;
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	int ret;
+
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	if (!ctrlr->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, &ctrlr->cache, list) {
+		if (!is_req_valid(p)) {
+			pr_debug("%s: skipping RPMH req: a:%#x s:%#x w:%#x",
+				 __func__, p->addr, p->sleep_val, p->wake_val);
+			continue;
+		}
+		ret = send_single(dev, RPMH_SLEEP_STATE, p->addr, p->sleep_val);
+		if (ret)
+			return ret;
+		ret = send_single(dev, RPMH_WAKE_ONLY_STATE,
+				  p->addr, p->wake_val);
+		if (ret)
+			return ret;
+	}
+
+	ctrlr->dirty = false;
+
+	return 0;
+}
+EXPORT_SYMBOL(rpmh_flush);
+
+/**
+ * rpmh_invalidate: Invalidate all sleep and active sets
+ * sets.
+ *
+ * @dev: The device making the request
+ *
+ * Invalidate the sleep and active values in the TCS blocks.
+ */
+int rpmh_invalidate(const struct device *dev)
+{
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	int ret;
+
+	if (IS_ERR(ctrlr))
+		return PTR_ERR(ctrlr);
+
+	ctrlr->dirty = true;
+
+	do {
+		ret = rpmh_rsc_invalidate(ctrlr->drv);
+	} while (ret == -EAGAIN);
+
+	return ret;
+}
+EXPORT_SYMBOL(rpmh_invalidate);
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index c1d0f902bd71..42e62a0d26d8 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -14,12 +14,23 @@
 int rpmh_write(const struct device *dev, enum rpmh_state state,
 	       const struct tcs_cmd *cmd, u32 n);
 
+int rpmh_flush(const struct device *dev);
+
+int rpmh_invalidate(const struct device *dev);
+
 #else
 
 static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
 			     const struct tcs_cmd *cmd, u32 n)
 { return -ENODEV; }
 
+
+static inline int rpmh_flush(const struct device *dev)
+{ return -ENODEV; }
+
+static inline int rpmh_invalidate(const struct device *dev)
+{ return -ENODEV; }
+
 #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

[PATCH v7 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously

[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>
---

Changes in v6:
	- replace rpmh_client with device *
---
 drivers/soc/qcom/rpmh.c | 52 +++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/rpmh.h |  7 ++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 23fcbd9487cd..1bb62876795c 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -33,6 +33,7 @@
 		.cmd = { { 0 } },			\
 		.completion = q,			\
 		.dev = dev,				\
+		.free = NULL,				\
 	}
 
 /**
@@ -58,6 +59,7 @@ struct cache_req {
  * @completion: triggered when request is done
  * @dev: the device making the request
  * @err: err return from the controller
+ * @free: the request object to be freed at tx_done
  */
 struct rpmh_request {
 	struct tcs_request msg;
@@ -65,6 +67,7 @@ struct rpmh_request {
 	struct completion *completion;
 	const struct device *dev;
 	int err;
+	struct rpmh_request *free;
 };
 
 /**
@@ -137,6 +140,8 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
 		dev_err(rpm_msg->dev, "RPMH TX fail in msg addr=%#x, err=%d\n",
 			rpm_msg->msg.cmds[0].addr, r);
 
+	kfree(rpm_msg->free);
+
 	/* Signal the blocking thread we are done */
 	if (compl)
 		complete(compl);
@@ -248,6 +253,53 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
 	return ret;
 }
 
+static struct rpmh_request *__get_rpmh_msg_async(enum rpmh_state state,
+						 const struct tcs_cmd *cmd,
+						 u32 n)
+{
+	struct rpmh_request *req;
+
+	if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
+		return ERR_PTR(-EINVAL);
+
+	req = kzalloc(sizeof(*req), GFP_ATOMIC);
+	if (!req)
+		return ERR_PTR(-ENOMEM);
+
+	memcpy(req->cmd, cmd, n * sizeof(*cmd));
+
+	req->msg.state = state;
+	req->msg.cmds = req->cmd;
+	req->msg.num_cmds = n;
+	req->free = req;
+
+	return req;
+}
+
+/**
+ * rpmh_write_async: Write a set of RPMH commands
+ *
+ * @dev: The device making the request
+ * @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(const struct device *dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 n)
+{
+	struct rpmh_request *rpm_msg;
+
+	rpm_msg = __get_rpmh_msg_async(state, cmd, n);
+	if (IS_ERR(rpm_msg))
+		return PTR_ERR(rpm_msg);
+
+	return __rpmh_write(dev, 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 42e62a0d26d8..1161a5c77e75 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -14,6 +14,9 @@
 int rpmh_write(const struct device *dev, enum rpmh_state state,
 	       const struct tcs_cmd *cmd, u32 n);
 
+int rpmh_write_async(const struct device *dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 n);
+
 int rpmh_flush(const struct device *dev);
 
 int rpmh_invalidate(const struct device *dev);
@@ -24,6 +27,10 @@ static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
 			     const struct tcs_cmd *cmd, u32 n)
 { return -ENODEV; }
 
+static inline int rpmh_write_async(const struct device *dev,
+				   enum rpmh_state state,
+				   const struct tcs_cmd *cmd, u32 n)
+{ return -ENODEV; }
 
 static inline int rpmh_flush(const struct device *dev)
 { return -ENODEV; }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 09/10] drivers: qcom: rpmh: add support for batch RPMH request

[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>
---

Changes in v7:
	- Check for loop out of bounds

Changes in v6:
	- replace rpmh_client with device *
Changes in v4:
	- reorganize rpmh_write_batch()
	- introduce wait_count here, instead of patch#4
---
 drivers/soc/qcom/rpmh.c | 154 +++++++++++++++++++++++++++++++++++++++-
 include/soc/qcom/rpmh.h |   8 +++
 2 files changed, 160 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 1bb62876795c..a0e277b4b846 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -21,6 +21,8 @@
 #include "rpmh-internal.h"
 
 #define RPMH_TIMEOUT_MS			msecs_to_jiffies(10000)
+#define RPMH_MAX_REQ_IN_BATCH		10
+#define RPMH_MAX_BATCH_CACHE		(2 * RPMH_MAX_REQ_IN_BATCH)
 
 #define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name)	\
 	struct rpmh_request name = {			\
@@ -34,6 +36,7 @@
 		.completion = q,			\
 		.dev = dev,				\
 		.free = NULL,				\
+		.wait_count = NULL,			\
 	}
 
 /**
@@ -60,6 +63,7 @@ struct cache_req {
  * @dev: the device making the request
  * @err: err return from the controller
  * @free: the request object to be freed at tx_done
+ * @wait_count: count of waiters for this completion
  */
 struct rpmh_request {
 	struct tcs_request msg;
@@ -68,6 +72,7 @@ struct rpmh_request {
 	const struct device *dev;
 	int err;
 	struct rpmh_request *free;
+	atomic_t *wait_count;
 };
 
 /**
@@ -77,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;
+	const struct rpmh_request *batch_cache[RPMH_MAX_BATCH_CACHE];
 };
 
 static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
@@ -133,6 +140,7 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
 	struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
 						    msg);
 	struct completion *compl = rpm_msg->completion;
+	atomic_t *wc = rpm_msg->wait_count;
 
 	rpm_msg->err = r;
 
@@ -143,8 +151,13 @@ void rpmh_tx_done(const struct tcs_request *msg, int r)
 	kfree(rpm_msg->free);
 
 	/* Signal the blocking thread we are done */
-	if (compl)
-		complete(compl);
+	if (!compl)
+		return;
+
+	if (wc && !atomic_dec_and_test(wc))
+		return;
+
+	complete(compl);
 }
 EXPORT_SYMBOL(rpmh_tx_done);
 
@@ -332,6 +345,137 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 }
 EXPORT_SYMBOL(rpmh_write);
 
+static int cache_batch(struct rpmh_ctrlr *ctrlr,
+		       struct rpmh_request **rpm_msg, int count)
+{
+	unsigned long flags;
+	int ret = 0;
+	int index = 0;
+	int i;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	while (index < RPMH_MAX_BATCH_CACHE && ctrlr->batch_cache[index])
+		index++;
+	if (index + count >= RPMH_MAX_BATCH_CACHE) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	for (i = 0; i < count; i++)
+		ctrlr->batch_cache[index + i] = rpm_msg[i];
+fail:
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+	return ret;
+}
+
+static int flush_batch(struct rpmh_ctrlr *ctrlr)
+{
+	const 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(&ctrlr->lock, flags);
+	for (i = 0; ctrlr->batch_cache[i]; i++) {
+		rpm_msg = ctrlr->batch_cache[i];
+		ret = rpmh_rsc_write_ctrl_data(ctrlr->drv, &rpm_msg->msg);
+		if (ret)
+			break;
+	}
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+
+	return ret;
+}
+
+static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
+{
+	unsigned long flags;
+	int index = 0;
+
+	spin_lock_irqsave(&ctrlr->lock, flags);
+	while (ctrlr->batch_cache[index]) {
+		kfree(ctrlr->batch_cache[index]->free);
+		ctrlr->batch_cache[index] = NULL;
+		index++;
+	}
+	spin_unlock_irqrestore(&ctrlr->lock, flags);
+}
+
+/**
+ * rpmh_write_batch: Write multiple sets of RPMH commands and wait for the
+ * batch to finish.
+ *
+ * @dev: the device making the request
+ * @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 RSC 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(const struct device *dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 *n)
+{
+	struct rpmh_request *rpm_msg[RPMH_MAX_REQ_IN_BATCH] = { NULL };
+	DECLARE_COMPLETION_ONSTACK(compl);
+	atomic_t wait_count = ATOMIC_INIT(0);
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+	int count = 0;
+	int ret, i;
+
+	if (IS_ERR(ctrlr) || !cmd || !n)
+		return -EINVAL;
+
+	while (n[count++] > 0)
+		;
+	count--;
+	if (!count || count > RPMH_MAX_REQ_IN_BATCH)
+		return -EINVAL;
+
+	for (i = 0; i < count; i++) {
+		rpm_msg[i] = __get_rpmh_msg_async(state, cmd, n[i]);
+		if (IS_ERR_OR_NULL(rpm_msg[i])) {
+			ret = PTR_ERR(rpm_msg[i]);
+			for (; i >= 0; i--)
+				kfree(rpm_msg[i]->free);
+			return ret;
+		}
+		cmd += n[i];
+	}
+
+	if (state != RPMH_ACTIVE_ONLY_STATE)
+		return cache_batch(ctrlr, rpm_msg, count);
+
+	atomic_set(&wait_count, count);
+
+	for (i = 0; i < count; i++) {
+		rpm_msg[i]->completion = &compl;
+		rpm_msg[i]->wait_count = &wait_count;
+		ret = rpmh_rsc_send_data(ctrlr->drv, &rpm_msg[i]->msg);
+		if (ret) {
+			int j;
+
+			pr_err("Error(%d) sending RPMH message addr=%#x\n",
+			       ret, rpm_msg[i]->msg.cmds[0].addr);
+			for (j = i; j < count; j++)
+				rpmh_tx_done(&rpm_msg[j]->msg, ret);
+			break;
+		}
+	}
+
+	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
+	return (ret > 0) ? 0 : -ETIMEDOUT;
+
+}
+EXPORT_SYMBOL(rpmh_write_batch);
+
 static int is_req_valid(struct cache_req *req)
 {
 	return (req->sleep_val != UINT_MAX &&
@@ -383,6 +527,11 @@ int rpmh_flush(const struct device *dev)
 		return 0;
 	}
 
+	/* First flush the cached batch requests */
+	ret = flush_batch(ctrlr);
+	if (ret)
+		return ret;
+
 	/*
 	 * Nobody else should be calling this function other than system PM,
 	 * hence we can run without locks.
@@ -424,6 +573,7 @@ int rpmh_invalidate(const struct device *dev)
 	if (IS_ERR(ctrlr))
 		return PTR_ERR(ctrlr);
 
+	invalidate_batch(ctrlr);
 	ctrlr->dirty = true;
 
 	do {
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index 1161a5c77e75..619e07c75da9 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -17,6 +17,9 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 		     const struct tcs_cmd *cmd, u32 n);
 
+int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
+		     const struct tcs_cmd *cmd, u32 *n);
+
 int rpmh_flush(const struct device *dev);
 
 int rpmh_invalidate(const struct device *dev);
@@ -32,6 +35,11 @@ static inline int rpmh_write_async(const struct device *dev,
 				   const struct tcs_cmd *cmd, u32 n)
 { return -ENODEV; }
 
+static inline int rpmh_write_batch(const struct device *dev,
+				   enum rpmh_state state,
+				   const struct tcs_cmd *cmd, u32 *n)
+{ return -ENODEV; }
+
 static inline int rpmh_flush(const struct device *dev)
 { return -ENODEV; }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS

[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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.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 42aedf2d80fe..26549a31060a 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -153,6 +153,7 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 					 const struct tcs_request *msg)
 {
 	int type;
+	struct tcs_group *tcs;
 
 	switch (msg->state) {
 	case RPMH_ACTIVE_ONLY_STATE:
@@ -168,7 +169,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 const struct tcs_request *get_req_from_tcs(struct rsc_drv *drv,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v7 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE

[Steven Rostedt]

On Wed,  2 May 2018 13:37:42 -0600
Lina Iyer <ilina@codeaurora.org> wrote:

> Log sent RPMH requests and interrupt responses in FTRACE.
> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

It's now up to the subsystem maintainer to apply it.

-- Steve

Re: [PATCH v7 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Lina Iyer (2018-05-02 12:37:41)
> diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> new file mode 100644
> index 000000000000..950d56325284
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> @@ -0,0 +1,132 @@
[...]
> +
> +- qcom,drv-id:
> +       Usage: required
> +       Value type: <u32>
> +       Definition: The id of the DRV in the RSC block that will be used by
> +                   this controller.

used by the OS? instead of by this controller?

> +
> +- 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 nodes 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

0xd00? Instead of what looks like octal.

> +this -
> +DRV0: 0x179C0000
> +DRV2: 0x179C0000 + 0x10000 = 0x179D0000
> +DRV2: 0x179C0000 + 0x10000 * 2 = 0x179E0000
> +TCS-OFFSET: 0xD00
> +
> +       apps_rsc: rsc@179c0000 {
> +               label = "apps_rsc";
> +               compatible = "qcom,rpmh-rsc";
> +               reg = <0x179c0000 0x10000>, <0x179d0000 0x10000>,
> +                     <0x179e0000 0x10000>, <0xd00 0>;
> +               reg-names = "drv-0", "drv-1", "drv-2", "tcs-offset";

tcs-offset should be a property, not a reg property value.

> +               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;

There should be three interrupts? Not 1?

> +               qcom,drv-id = <2>;
> +               qcom,tcs-config = <SLEEP_TCS   3>,
> +                                 <WAKE_TCS    3>,
> +                                 <ACTIVE_TCS  2>,
> +                                 <CONTROL_TCS 1>;
> +       };
> +

Re: [PATCH v7 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Lina Iyer (2018-05-02 12:37:41)
> diff --git a/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> new file mode 100644
> index 000000000000..950d56325284
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt
> @@ -0,0 +1,132 @@
[...]
> +
> +- qcom,drv-id:
> +       Usage: required
> +       Value type: <u32>
> +       Definition: The id of the DRV in the RSC block that will be used by
> +                   this controller.

used by the OS? instead of by this controller?

> +
> +- 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 nodes 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

0xd00? Instead of what looks like octal.

> +this -
> +DRV0: 0x179C0000
> +DRV2: 0x179C0000 + 0x10000 = 0x179D0000
> +DRV2: 0x179C0000 + 0x10000 * 2 = 0x179E0000
> +TCS-OFFSET: 0xD00
> +
> +       apps_rsc: rsc@179c0000 {
> +               label = "apps_rsc";
> +               compatible = "qcom,rpmh-rsc";
> +               reg = <0x179c0000 0x10000>, <0x179d0000 0x10000>,
> +                     <0x179e0000 0x10000>, <0xd00 0>;
> +               reg-names = "drv-0", "drv-1", "drv-2", "tcs-offset";

tcs-offset should be a property, not a reg property value.

> +               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;

There should be three interrupts? Not 1?

> +               qcom,drv-id = <2>;
> +               qcom,tcs-config = <SLEEP_TCS   3>,
> +                                 <WAKE_TCS    3>,
> +                                 <ACTIVE_TCS  2>,
> +                                 <CONTROL_TCS 1>;
> +       };
> +

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> wrote:
> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
> +
> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
> +{
> +       int i;
> +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
> +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
> +       unsigned long flags;
> +
> +       if (!drv)
> +               return ctrlr;
> +
> +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> +               if (rpmh_rsc[i].drv == drv) {
> +                       ctrlr = &rpmh_rsc[i];
> +                       return ctrlr;
> +               }
> +       }
> +
> +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
> +       list_for_each_entry(p, &rsc_drv_list, list) {
> +               if (drv == p) {
> +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> +                               if (!rpmh_rsc[i].drv)
> +                                       break;
> +                       }
> +                       if (i == RPMH_MAX_CTRLR) {
> +                               ctrlr = ERR_PTR(-ENOMEM);
> +                               break;
> +                       }
> +                       rpmh_rsc[i].drv = drv;
> +                       ctrlr = &rpmh_rsc[i];
> +                       break;
> +               }
> +       }
> +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);

I may have missed something, but to me it appears that this whole
"rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
series to remove it at
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch.  From that patch, my
justification was:

> The global rsc_drv_list was (as far as I can tell) racy and not useful
> for anything.
>
> I say it is racy because in general you need some sort of mutual
> exclusion for lists.  If someone is adding to a list while someone
> else is iterating over it then you get badness.
>
> I say it is not useful because the only user of it was
> get_rpmh_ctrlr() and the only thing it did was to verify that the
> "struct rsc_drv *" that it alrady had was in the list.  How could it
> not be?

Note that in v7 of your series you added a spinlock around your access
of "rsc_drv_list", but this doesn't actually remove the race.
Specifically I'm pretty sure that the list primitives don't support
calling list_add() while someone might be iterating over the list and
your spinlock isn't grabbed in rpmh_rsc_probe().

Note that I also say in my patch:

> NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy.  I'm
> not sure why every caller would need its own private global cache of
> stuff.  ...but I left that part alone.

I'll try to dig into this more so I could just be confused, but in
general it seems really odd to have a spinlock and something called a
"cache" at this level.  If we need some sort of mutual exclusion or
caching it seems like it should be stored in memory directly
associated with the RPMh device, not some external global.


-Doug

Re: [PATCH v7 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS

[Matthias Kaehlcke]

Hi Lina,

On Wed, May 02, 2018 at 01:37:44PM -0600, Lina Iyer wrote:
> 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 the job of this
> controller driver to find and arrange the requests into the available
> TCSes.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Reviewed-by: Evan Green <evgreen@chromium.org>
> 
> ---
> 
> Changes in v7:
> 	- Bug fix in find_match()
> ---
>  drivers/soc/qcom/rpmh-internal.h |   8 +++
>  drivers/soc/qcom/rpmh-rsc.c      | 118 +++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index d9a21726e568..6e19fe458c31 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -14,6 +14,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)
>  #define RPMH_MAX_CTRLR			2
>  
>  struct rsc_drv;
> @@ -30,6 +31,8 @@ struct rsc_drv;
>   * @ncpt:      number of commands in each TCS
>   * @lock:      lock for synchronizing this TCS writes
>   * @req:       requests that are sent from the TCS
> + * @cmd_cache: flattened cache of cmds in sleep/wake TCS
> + * @slots:     indicates which of @cmd_addr are occupied
>   */
>  struct tcs_group {
>  	struct rsc_drv *drv;
> @@ -40,6 +43,8 @@ struct tcs_group {
>  	int ncpt;
>  	spinlock_t lock;
>  	const struct tcs_request *req[MAX_TCS_PER_TYPE];
> +	u32 *cmd_cache;
> +	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
>  };
>  
>  /**
> @@ -69,6 +74,9 @@ struct rsc_drv {
>  extern struct list_head rsc_drv_list;
>  
>  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
> +int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
> +			     const struct tcs_request *msg);
> +int rpmh_rsc_invalidate(struct rsc_drv *drv);
>  
>  void rpmh_tx_done(const struct tcs_request *msg, int r);
>  
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 33270b1d5991..4e2144a14c31 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -113,6 +113,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);
>  	}
> @@ -353,6 +359,105 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
>  }
>  EXPORT_SYMBOL(rpmh_rsc_send_data);
>  
> +static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
> +		      int len)
> +{
> +	int i, j;
> +
> +	/* Check for already cached commands */
> +	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
> +		if (tcs->cmd_cache[i] != cmd[0].addr)
> +			continue;
> +		for (j = 0; j < len; j++) {
> +			if (tcs->cmd_cache[i + j] != cmd[j].addr) {

As mentioned in the review of v6, the code could read memory beyond
tcs->cmd_cache if 'i + len > ARRAY_SIZE(tcs->cmd_cache)'

> +				WARN(1, "Message does not match previous sequence.\n");
> +				return -EINVAL;
> +			}
> +		}
> +		return i;
> +	}
> +
> +	return -ENODATA;
> +}
> +
> +static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
> +		      int *tcs_id, int *cmd_id)

The change from m/n => tcs_id/cmd_id in v7 greatly improves
readability, thanks!

Matthias

PS: Please remember to include reviewers of earlier revisions to cc:

Re: [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS

[Matthias Kaehlcke]

Hi Lina,

On Wed, May 02, 2018 at 01:37:45PM -0600, Lina Iyer wrote:
> 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>
> ---
> 
> Changes in v7:
> 	- Move bitmap_zero() outside the loop
> 
> Changes in v6:
> 	- remove unnecessary locks around __tcs_invalidate
> 	- rename function to tcs_invaldiate
> 
> Changes in v4:
> 	- refactored the rphm_rsc_invalidate()
> ---
>  drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 4e2144a14c31..42aedf2d80fe 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
>  	return &drv->tcs[type];
>  }
>  
> +static int tcs_invalidate(struct rsc_drv *drv, int type)
> +{
> +	int m;

nit: use tcs_id?

> +	struct tcs_group *tcs;
> +
> +	tcs = get_tcs_of_type(drv, type);
> +	if (IS_ERR(tcs))
> +		return PTR_ERR(tcs);
> +
> +	spin_lock(&tcs->lock);
> +	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> +		spin_unlock(&tcs->lock);
> +		return 0;
> +	}
> +
> +	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> +		if (!tcs_is_free(drv, m)) {
> +			spin_unlock(&tcs->lock);
> +			return -EAGAIN;
> +		}
> +		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
> +	}
> +	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);

You didn't reply to (or address) my comment on v6:

It could occur that one or more TCSes are disabled, then tcs_is_free()
returns false for the next TCS and the function returns without having
updated tcs->slots for the TCSes that have been disabled.

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Matthias Kaehlcke]

On Thu, May 03, 2018 at 01:26:07PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> wrote:
> > +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
> > +static DEFINE_SPINLOCK(rpmh_rsc_lock);
> > +
> > +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
> > +{
> > +       int i;
> > +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
> > +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
> > +       unsigned long flags;
> > +
> > +       if (!drv)
> > +               return ctrlr;
> > +
> > +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> > +               if (rpmh_rsc[i].drv == drv) {
> > +                       ctrlr = &rpmh_rsc[i];
> > +                       return ctrlr;
> > +               }
> > +       }
> > +
> > +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
> > +       list_for_each_entry(p, &rsc_drv_list, list) {
> > +               if (drv == p) {
> > +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> > +                               if (!rpmh_rsc[i].drv)
> > +                                       break;
> > +                       }
> > +                       if (i == RPMH_MAX_CTRLR) {
> > +                               ctrlr = ERR_PTR(-ENOMEM);
> > +                               break;
> > +                       }
> > +                       rpmh_rsc[i].drv = drv;
> > +                       ctrlr = &rpmh_rsc[i];
> > +                       break;
> > +               }
> > +       }
> > +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
> 
> I may have missed something, but to me it appears that this whole
> "rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
> series to remove it at
> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
> and it simplifies the code a whole bunch.  From that patch, my
> justification was:
> 
> > The global rsc_drv_list was (as far as I can tell) racy and not useful
> > for anything.
> >
> > I say it is racy because in general you need some sort of mutual
> > exclusion for lists.  If someone is adding to a list while someone
> > else is iterating over it then you get badness.
> >
> > I say it is not useful because the only user of it was
> > get_rpmh_ctrlr() and the only thing it did was to verify that the
> > "struct rsc_drv *" that it alrady had was in the list.  How could it
> > not be?

I agree that the list doesn't seem to be very useful.

> Note that in v7 of your series you added a spinlock around your access
> of "rsc_drv_list", but this doesn't actually remove the race.
> Specifically I'm pretty sure that the list primitives don't support
> calling list_add() while someone might be iterating over the list and
> your spinlock isn't grabbed in rpmh_rsc_probe().

Actually adding a lock was my suggestion, but to protect against
another race that is still/again present with your patch:

> if (!rpmh_rsc[i].drv) {
>     rpmh_rsc[i].drv = drv;

This could be executed concurrently with both/all instances seeing
rpmh_rsc[i].drv == NULL and then clobbering each other.

>     spin_lock_init(&rpmh_rsc[i].lock);
>     INIT_LIST_HEAD(&rpmh_rsc[i].cache);
>     return &rpmh_rsc[i];
> }

Re: [PATCH v7 07/10] drivers: qcom: rpmh: cache sleep/wake state requests

[Matthias Kaehlcke]

On Wed, May 02, 2018 at 01:37:46PM -0600, Lina Iyer wrote:
> Active state requests are sent immediately to the RSC controller, while
> sleep and wake state requests are cached in this driver to avoid taxing
> the RSC 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>
> Reviewed-by: Evan Green <evgreen@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[ilina]

On 2018-05-03 14:26, Doug Anderson wrote:
Hi Doug,

> Hi,
> 
> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> 
> wrote:
>> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
>> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
>> +
>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>> +{
>> +       int i;
>> +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
>> +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
>> +       unsigned long flags;
>> +
>> +       if (!drv)
>> +               return ctrlr;
>> +
>> +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>> +               if (rpmh_rsc[i].drv == drv) {
>> +                       ctrlr = &rpmh_rsc[i];
>> +                       return ctrlr;
>> +               }
>> +       }
>> +
>> +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
>> +       list_for_each_entry(p, &rsc_drv_list, list) {
>> +               if (drv == p) {
>> +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>> +                               if (!rpmh_rsc[i].drv)
>> +                                       break;
>> +                       }
>> +                       if (i == RPMH_MAX_CTRLR) {
>> +                               ctrlr = ERR_PTR(-ENOMEM);
>> +                               break;
>> +                       }
>> +                       rpmh_rsc[i].drv = drv;
>> +                       ctrlr = &rpmh_rsc[i];
>> +                       break;
>> +               }
>> +       }
>> +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
> 
> I may have missed something, but to me it appears that this whole
> "rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
> series to remove it at
> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
> and it simplifies the code a whole bunch.  From that patch, my
> justification was:
> 
>> The global rsc_drv_list was (as far as I can tell) racy and not useful
>> for anything.
>> 
>> I say it is racy because in general you need some sort of mutual
>> exclusion for lists.  If someone is adding to a list while someone
>> else is iterating over it then you get badness.
>> 
>> I say it is not useful because the only user of it was
>> get_rpmh_ctrlr() and the only thing it did was to verify that the
>> "struct rsc_drv *" that it alrady had was in the list.  How could it
>> not be?
> 
> Note that in v7 of your series you added a spinlock around your access
> of "rsc_drv_list", but this doesn't actually remove the race.
> Specifically I'm pretty sure that the list primitives don't support
> calling list_add() while someone might be iterating over the list and
> your spinlock isn't grabbed in rpmh_rsc_probe().
> 
> Note that I also say in my patch:
> 
>> NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy.  I'm
>> not sure why every caller would need its own private global cache of
>> stuff.  ...but I left that part alone.
> 
I am not sure I understand this.

> I'll try to dig into this more so I could just be confused, but in
> general it seems really odd to have a spinlock and something called a
> "cache" at this level.  If we need some sort of mutual exclusion or
> caching it seems like it should be stored in memory directly
> associated with the RPMh device, not some external global.
> 
The idea behind the locking is not to avoid the race between rpmh.c and 
rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are 
probed following the probe of the controller. And init is not that we 
are worried about.
The condition here is to prevent the rpmh_rsc[] from being modified 
concurrently by drivers.

Thanks,
Lina

Re: [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS

[ilina]

On 2018-05-03 16:06, Matthias Kaehlcke wrote:
Hi Matthias,

> Hi Lina,
> 
> On Wed, May 02, 2018 at 01:37:45PM -0600, Lina Iyer wrote:
>> 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>
>> ---
>> 
>> Changes in v7:
>> 	- Move bitmap_zero() outside the loop
>> 
>> Changes in v6:
>> 	- remove unnecessary locks around __tcs_invalidate
>> 	- rename function to tcs_invaldiate
>> 
>> Changes in v4:
>> 	- refactored the rphm_rsc_invalidate()
>> ---
>>  drivers/soc/qcom/rpmh-rsc.c | 45 
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>> 
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index 4e2144a14c31..42aedf2d80fe 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct 
>> rsc_drv *drv, int type)
>>  	return &drv->tcs[type];
>>  }
>> 
>> +static int tcs_invalidate(struct rsc_drv *drv, int type)
>> +{
>> +	int m;
> 
> nit: use tcs_id?
> 
It's an iterator. Hence didn't change this.

>> +	struct tcs_group *tcs;
>> +
>> +	tcs = get_tcs_of_type(drv, type);
>> +	if (IS_ERR(tcs))
>> +		return PTR_ERR(tcs);
>> +
>> +	spin_lock(&tcs->lock);
>> +	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>> +		spin_unlock(&tcs->lock);
>> +		return 0;
>> +	}
>> +
>> +	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>> +		if (!tcs_is_free(drv, m)) {
>> +			spin_unlock(&tcs->lock);
>> +			return -EAGAIN;
>> +		}
>> +		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
>> +	}
>> +	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> 
> You didn't reply to (or address) my comment on v6:
> 
> It could occur that one or more TCSes are disabled, then tcs_is_free()
> returns false for the next TCS and the function returns without having
> updated tcs->slots for the TCSes that have been disabled.
How do you mean TCS are disabled? I think I asked that question in my 
mail. Sorry, if I forgot to ask.

TCSes are either available for sending requests or they are busy sending 
requests. They cannot be disabled if they are present.
Individual commands, however are enabled or disabled based on whether 
they have active requests or not.

What we are trying to do here is to cleanup the TCSes of their existing 
requests. Generally, sleep and wake TCSes are not used to send active 
state requests, they are sent through AMC/Active TCS. So they will be 
free. However, in the case of the Display RSC, there is no explicit TCS 
available for sending active state requests. So we overload the wake TCS 
to send the active state requests. Even in that case, TCS would have 
finished and should be free when this function is called. The 
tcs_is_free() check is addition to make sure the requirement is not 
violated.

Hope that clears it up.

Thanks,
Lina

Re: [PATCH v7 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS

[ilina]

On 2018-05-03 15:35, Matthias Kaehlcke wrote:
> Hi Lina,
> 
> On Wed, May 02, 2018 at 01:37:44PM -0600, Lina Iyer wrote:
>> 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 the job of this
>> controller driver to find and arrange the requests into the available
>> TCSes.
>> 
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> Reviewed-by: Evan Green <evgreen@chromium.org>
>> 
>> ---
>> 
>> Changes in v7:
>> 	- Bug fix in find_match()
>> ---
>>  drivers/soc/qcom/rpmh-internal.h |   8 +++
>>  drivers/soc/qcom/rpmh-rsc.c      | 118 
>> +++++++++++++++++++++++++++++++
>>  2 files changed, 126 insertions(+)
>> 
>> diff --git a/drivers/soc/qcom/rpmh-internal.h 
>> b/drivers/soc/qcom/rpmh-internal.h
>> index d9a21726e568..6e19fe458c31 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> @@ -14,6 +14,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)
>>  #define RPMH_MAX_CTRLR			2
>> 
>>  struct rsc_drv;
>> @@ -30,6 +31,8 @@ struct rsc_drv;
>>   * @ncpt:      number of commands in each TCS
>>   * @lock:      lock for synchronizing this TCS writes
>>   * @req:       requests that are sent from the TCS
>> + * @cmd_cache: flattened cache of cmds in sleep/wake TCS
>> + * @slots:     indicates which of @cmd_addr are occupied
>>   */
>>  struct tcs_group {
>>  	struct rsc_drv *drv;
>> @@ -40,6 +43,8 @@ struct tcs_group {
>>  	int ncpt;
>>  	spinlock_t lock;
>>  	const struct tcs_request *req[MAX_TCS_PER_TYPE];
>> +	u32 *cmd_cache;
>> +	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
>>  };
>> 
>>  /**
>> @@ -69,6 +74,9 @@ struct rsc_drv {
>>  extern struct list_head rsc_drv_list;
>> 
>>  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request 
>> *msg);
>> +int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
>> +			     const struct tcs_request *msg);
>> +int rpmh_rsc_invalidate(struct rsc_drv *drv);
>> 
>>  void rpmh_tx_done(const struct tcs_request *msg, int r);
>> 
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index 33270b1d5991..4e2144a14c31 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -113,6 +113,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);
>>  	}
>> @@ -353,6 +359,105 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, 
>> const struct tcs_request *msg)
>>  }
>>  EXPORT_SYMBOL(rpmh_rsc_send_data);
>> 
>> +static int find_match(const struct tcs_group *tcs, const struct 
>> tcs_cmd *cmd,
>> +		      int len)
>> +{
>> +	int i, j;
>> +
>> +	/* Check for already cached commands */
>> +	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
>> +		if (tcs->cmd_cache[i] != cmd[0].addr)
>> +			continue;
>> +		for (j = 0; j < len; j++) {
>> +			if (tcs->cmd_cache[i + j] != cmd[j].addr) {
> 
> As mentioned in the review of v6, the code could read memory beyond
> tcs->cmd_cache if 'i + len > ARRAY_SIZE(tcs->cmd_cache)'
> 
Ok. Will add it.

>> +				WARN(1, "Message does not match previous sequence.\n");
>> +				return -EINVAL;
>> +			}
>> +		}
>> +		return i;
>> +	}
>> +
>> +	return -ENODATA;
>> +}
>> +
>> +static int find_slots(struct tcs_group *tcs, const struct tcs_request 
>> *msg,
>> +		      int *tcs_id, int *cmd_id)
> 
> The change from m/n => tcs_id/cmd_id in v7 greatly improves
> readability, thanks!
> 
> Matthias
> 
> PS: Please remember to include reviewers of earlier revisions to cc:
Sure.

Thanks,
Lina

Re: [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS

[Matthias Kaehlcke]

Hi Lina,

On Tue, May 08, 2018 at 10:14:33AM -0600, ilina@codeaurora.org wrote:
> On 2018-05-03 16:06, Matthias Kaehlcke wrote:
> Hi Matthias,
> 
> > Hi Lina,
> > 
> > On Wed, May 02, 2018 at 01:37:45PM -0600, Lina Iyer wrote:
> > > 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>
> > > ---
> > > 
> > > Changes in v7:
> > > 	- Move bitmap_zero() outside the loop
> > > 
> > > Changes in v6:
> > > 	- remove unnecessary locks around __tcs_invalidate
> > > 	- rename function to tcs_invaldiate
> > > 
> > > Changes in v4:
> > > 	- refactored the rphm_rsc_invalidate()
> > > ---
> > >  drivers/soc/qcom/rpmh-rsc.c | 45
> > > +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > > 
> > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > > index 4e2144a14c31..42aedf2d80fe 100644
> > > --- a/drivers/soc/qcom/rpmh-rsc.c
> > > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > > @@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct
> > > rsc_drv *drv, int type)
> > >  	return &drv->tcs[type];
> > >  }
> > > 
> > > +static int tcs_invalidate(struct rsc_drv *drv, int type)
> > > +{
> > > +	int m;
> > 
> > nit: use tcs_id?
> > 
> It's an iterator. Hence didn't change this.
> 
> > > +	struct tcs_group *tcs;
> > > +
> > > +	tcs = get_tcs_of_type(drv, type);
> > > +	if (IS_ERR(tcs))
> > > +		return PTR_ERR(tcs);
> > > +
> > > +	spin_lock(&tcs->lock);
> > > +	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> > > +		spin_unlock(&tcs->lock);
> > > +		return 0;
> > > +	}
> > > +
> > > +	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> > > +		if (!tcs_is_free(drv, m)) {
> > > +			spin_unlock(&tcs->lock);
> > > +			return -EAGAIN;
> > > +		}
> > > +		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
> > > +	}
> > > +	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> > 
> > You didn't reply to (or address) my comment on v6:
> > 
> > It could occur that one or more TCSes are disabled, then tcs_is_free()
> > returns false for the next TCS and the function returns without having
> > updated tcs->slots for the TCSes that have been disabled.
> How do you mean TCS are disabled? I think I asked that question in my mail.
> Sorry, if I forgot to ask.
> 
> TCSes are either available for sending requests or they are busy sending
> requests. They cannot be disabled if they are present.
> Individual commands, however are enabled or disabled based on whether they
> have active requests or not.

Sorry for conflating terminologies, I don't have a firm grasp on that
yet, what I meant is a TCS being busy sending requests.

> What we are trying to do here is to cleanup the TCSes of their existing
> requests. Generally, sleep and wake TCSes are not used to send active state
> requests, they are sent through AMC/Active TCS. So they will be free.
> However, in the case of the Display RSC, there is no explicit TCS available
> for sending active state requests. So we overload the wake TCS to send the
> active state requests. Even in that case, TCS would have finished and should
> be free when this function is called. The tcs_is_free() check is addition to
> make sure the requirement is not violated.

If you are sure that tcs_is_free(tcs) basically should never return
false when tcs_invalidate() is called I won't insist. If it is a
requirement violation should the code path include a WARN?

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Doug Anderson]

Hi,

On Tue, May 8, 2018 at 9:05 AM,  <ilina@codeaurora.org> wrote:
> On 2018-05-03 14:26, Doug Anderson wrote:
> Hi Doug,
>
>
>> Hi,
>>
>> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> wrote:
>>>
>>> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
>>> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
>>> +
>>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>>> +{
>>> +       int i;
>>> +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
>>> +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
>>> +       unsigned long flags;
>>> +
>>> +       if (!drv)
>>> +               return ctrlr;
>>> +
>>> +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>> +               if (rpmh_rsc[i].drv == drv) {
>>> +                       ctrlr = &rpmh_rsc[i];
>>> +                       return ctrlr;
>>> +               }
>>> +       }
>>> +
>>> +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
>>> +       list_for_each_entry(p, &rsc_drv_list, list) {
>>> +               if (drv == p) {
>>> +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>> +                               if (!rpmh_rsc[i].drv)
>>> +                                       break;
>>> +                       }
>>> +                       if (i == RPMH_MAX_CTRLR) {
>>> +                               ctrlr = ERR_PTR(-ENOMEM);
>>> +                               break;
>>> +                       }
>>> +                       rpmh_rsc[i].drv = drv;
>>> +                       ctrlr = &rpmh_rsc[i];
>>> +                       break;
>>> +               }
>>> +       }
>>> +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
>>
>>
>> I may have missed something, but to me it appears that this whole
>> "rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
>> series to remove it at
>>
>> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
>> and it simplifies the code a whole bunch.  From that patch, my
>> justification was:
>>
>>> The global rsc_drv_list was (as far as I can tell) racy and not useful
>>> for anything.
>>>
>>> I say it is racy because in general you need some sort of mutual
>>> exclusion for lists.  If someone is adding to a list while someone
>>> else is iterating over it then you get badness.
>>>
>>> I say it is not useful because the only user of it was
>>> get_rpmh_ctrlr() and the only thing it did was to verify that the
>>> "struct rsc_drv *" that it alrady had was in the list.  How could it
>>> not be?
>>
>>
>> Note that in v7 of your series you added a spinlock around your access
>> of "rsc_drv_list", but this doesn't actually remove the race.
>> Specifically I'm pretty sure that the list primitives don't support
>> calling list_add() while someone might be iterating over the list and
>> your spinlock isn't grabbed in rpmh_rsc_probe().
>>
>> Note that I also say in my patch:
>>
>>> NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy.  I'm
>>> not sure why every caller would need its own private global cache of
>>> stuff.  ...but I left that part alone.
>>
>>
> I am not sure I understand this.

As I've said I haven't reviewed RPMh in any amount of detail and so
perhaps I don't understand something.

OK, I dug a little more and coded up something for you.  Basically
you're doing a whole bunch of iteration / extra work here to try to
come up with a way to associate an extra bit of data with each "struct
rsc_drv".  Rather than that, just add an extra field into "struct
rsc_drv".  Problem solved.

See http://crosreview.com/1054646 for what I mean.


>> I'll try to dig into this more so I could just be confused, but in
>> general it seems really odd to have a spinlock and something called a
>> "cache" at this level.  If we need some sort of mutual exclusion or
>> caching it seems like it should be stored in memory directly
>> associated with the RPMh device, not some external global.
>>
> The idea behind the locking is not to avoid the race between rpmh.c and
> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
> probed following the probe of the controller. And init is not that we are
> worried about.
> The condition here is to prevent the rpmh_rsc[] from being modified
> concurrently by drivers.

OK, I see the point of the locking now, but not the list still.
Sounds like Matthias agrees with me that the list isn't useful.  Seems
like you should squash my patch at http://crosreview.com/1042883 into
yours.

-Doug

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Lina Iyer]

Hi Doug,

On Thu, May 10 2018 at 16:37 -0600, Doug Anderson wrote:
>Hi,
>
>On Tue, May 8, 2018 at 9:05 AM,  <ilina@codeaurora.org> wrote:
>> On 2018-05-03 14:26, Doug Anderson wrote:
>> Hi Doug,
>>
>>
>>> Hi,
>>>
>>> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> wrote:
>>>>
>>>> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
>>>> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
>>>> +
>>>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>>>> +{
>>>> +       int i;
>>>> +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
>>>> +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
>>>> +       unsigned long flags;
>>>> +
>>>> +       if (!drv)
>>>> +               return ctrlr;
>>>> +
>>>> +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>>> +               if (rpmh_rsc[i].drv == drv) {
>>>> +                       ctrlr = &rpmh_rsc[i];
>>>> +                       return ctrlr;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
>>>> +       list_for_each_entry(p, &rsc_drv_list, list) {
>>>> +               if (drv == p) {
>>>> +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>>> +                               if (!rpmh_rsc[i].drv)
>>>> +                                       break;
>>>> +                       }
>>>> +                       if (i == RPMH_MAX_CTRLR) {
>>>> +                               ctrlr = ERR_PTR(-ENOMEM);
>>>> +                               break;
>>>> +                       }
>>>> +                       rpmh_rsc[i].drv = drv;
>>>> +                       ctrlr = &rpmh_rsc[i];
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
>>>
>>>
>>> I may have missed something, but to me it appears that this whole
>>> "rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
>>> series to remove it at
>>>
>>> <https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
>>> and it simplifies the code a whole bunch.  From that patch, my
>>> justification was:
>>>
>>>> The global rsc_drv_list was (as far as I can tell) racy and not useful
>>>> for anything.
>>>>
>>>> I say it is racy because in general you need some sort of mutual
>>>> exclusion for lists.  If someone is adding to a list while someone
>>>> else is iterating over it then you get badness.
>>>>
>>>> I say it is not useful because the only user of it was
>>>> get_rpmh_ctrlr() and the only thing it did was to verify that the
>>>> "struct rsc_drv *" that it alrady had was in the list.  How could it
>>>> not be?
>>>
>>>
>>> Note that in v7 of your series you added a spinlock around your access
>>> of "rsc_drv_list", but this doesn't actually remove the race.
>>> Specifically I'm pretty sure that the list primitives don't support
>>> calling list_add() while someone might be iterating over the list and
>>> your spinlock isn't grabbed in rpmh_rsc_probe().
>>>
>>> Note that I also say in my patch:
>>>
>>>> NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy.  I'm
>>>> not sure why every caller would need its own private global cache of
>>>> stuff.  ...but I left that part alone.
>>>
>>>
>> I am not sure I understand this.
>
>As I've said I haven't reviewed RPMh in any amount of detail and so
>perhaps I don't understand something.
>
>OK, I dug a little more and coded up something for you.  Basically
>you're doing a whole bunch of iteration / extra work here to try to
>come up with a way to associate an extra bit of data with each "struct
>rsc_drv".  Rather than that, just add an extra field into "struct
>rsc_drv".  Problem solved.
>
>See http://crosreview.com/1054646 for what I mean.
>
I tried to avoid such pointer references and keep it object oriented
with this approach. I agree that we run through a list of 2 (at the max)
RSC to get the drv* from the rpmh_ctrlr. It is not going to be
expensive.

Another things this helps with is that, if the driver is not a child of
the RSC nodes in DT, then the drvdata of the parent would not a RSC node
and accessing that would result in a crash. This offers a cleaner exit
path for the error case.

>
>>> I'll try to dig into this more so I could just be confused, but in
>>> general it seems really odd to have a spinlock and something called a
>>> "cache" at this level.  If we need some sort of mutual exclusion or
>>> caching it seems like it should be stored in memory directly
>>> associated with the RPMh device, not some external global.
>>>
>> The idea behind the locking is not to avoid the race between rpmh.c and
>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
>> probed following the probe of the controller. And init is not that we are
>> worried about.
>> The condition here is to prevent the rpmh_rsc[] from being modified
>> concurrently by drivers.
>
>OK, I see the point of the locking now, but not the list still.
>Sounds like Matthias agrees with me that the list isn't useful.  Seems
>like you should squash my patch at http://crosreview.com/1042883 into
>yours.
>
I saw your approach. I am okay with it for your tree, my approach comes
out of experiences in qcom platforms and how things tend to shape up in
the future. I would want you to consider my reasoning as well, before we
go forward.

Thanks,
Lina

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Doug Anderson]

Hi,

On Fri, May 11, 2018 at 8:06 AM, Lina Iyer <ilina@codeaurora.org> wrote:
>> As I've said I haven't reviewed RPMh in any amount of detail and so
>> perhaps I don't understand something.
>>
>> OK, I dug a little more and coded up something for you.  Basically
>> you're doing a whole bunch of iteration / extra work here to try to
>> come up with a way to associate an extra bit of data with each "struct
>> rsc_drv".  Rather than that, just add an extra field into "struct
>> rsc_drv".  Problem solved.
>>
>> See http://crosreview.com/1054646 for what I mean.
>>
> I tried to avoid such pointer references and keep it object oriented
> with this approach. I agree that we run through a list of 2 (at the max)
> RSC to get the drv* from the rpmh_ctrlr. It is not going to be
> expensive.

Even if you wanted to keep things "object oriented" then IMHO your
code still should change.  Sure, it's not computationally expensive to
iterate through this list, but it adds an extra level of complexity
that doesn't seem justified.

If you _really_ needed an abstraction barrier then at least add a
"void *client_data" to "struct rsc_drv.c".  At least you'd get rid of
the ugly global list and store your pointer directly in the correct
structure rather than creating an external entity.  Now it becomes
100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each
controller.  ...but IMO there's enough intertwining between "rpmh.c"
and "rpmh-rsc.c" that it would just be a waste because now you'd need
to do extra memory allocation and freeing.  ...and if you just
allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal
because this one-time allocation (that affects _all_ RPMh clients)
happens whenever one client happens to do the first access.  This is
one-time init so it makes sense to do it at init time.

I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c"
because both C files call directly into one another and have intimate
knowledge of how the other works.  They aren't really separate things.
Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when
it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls
into "rpmh.c".


OK, so I've been browsing through the source code more so I can be a
little more informed here.  As far as I can tell "rpmh.c"'s goal is:

1. Put a simpler API atop "rpmh-rsc.c".  ...but why not just put that
API directly into "rpmh-rsc.c" anyway?  If there was someone that
needed the more complex API then having a "simpler" wrapper makes
sense, but that's not the case, is it?

2. Add caching atop "rpmh-rsc"


I'll respond to some of your other patches too, but I think that the
amount of code for caching is not very much.  I don't see the benefit
of trying to split the code into two files.  Put them into one and
then delete all the extra code you needed just the try to maintain
some abstraction.


> Another things this helps with is that, if the driver is not a child of
> the RSC nodes in DT, then the drvdata of the parent would not a RSC node
> and accessing that would result in a crash. This offers a cleaner exit
> path for the error case.

Why would the driver not be a child of the RSC nodes?  That's kinda
like saying "if you try to instantiate an i2c device as a platform
device then its probe will crash".  Yeah, it will.  Doctor, it hurts
if I poke myself in my eye with this sharp stick, do you have any
medicine that can help fix that?


>>>> I'll try to dig into this more so I could just be confused, but in
>>>> general it seems really odd to have a spinlock and something called a
>>>> "cache" at this level.  If we need some sort of mutual exclusion or
>>>> caching it seems like it should be stored in memory directly
>>>> associated with the RPMh device, not some external global.
>>>>
>>> The idea behind the locking is not to avoid the race between rpmh.c and
>>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
>>> probed following the probe of the controller. And init is not that we are
>>> worried about.
>>> The condition here is to prevent the rpmh_rsc[] from being modified
>>> concurrently by drivers.
>>
>>
>> OK, I see the point of the locking now, but not the list still.
>> Sounds like Matthias agrees with me that the list isn't useful.  Seems
>> like you should squash my patch at http://crosreview.com/1042883 into
>> yours.
>>
> I saw your approach. I am okay with it for your tree,

I'm not okay with it for the Chrome OS tree.  We need to match
upstream, not fork for style reasons.  I'd rather take a driver that I
think it overly complex but matches upstream than a private driver.


> my approach comes
> out of experiences in qcom platforms and how things tend to shape up in
> the future. I would want you to consider my reasoning as well, before we
> go forward.

I suppose we can get advice from others who have worked in qcom
platforms and see what they think.  My opinions come out of years of
experience working with Linux drivers, so I guess we both have some
experience under our belts that we're trying to leverage.

-Doug

Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions

[Lina Iyer]

On Fri, May 11 2018 at 14:14 -0600, Doug Anderson wrote:
>Hi,
>
>On Fri, May 11, 2018 at 8:06 AM, Lina Iyer <ilina@codeaurora.org> wrote:
>>> As I've said I haven't reviewed RPMh in any amount of detail and so
>>> perhaps I don't understand something.
>>>
>>> OK, I dug a little more and coded up something for you.  Basically
>>> you're doing a whole bunch of iteration / extra work here to try to
>>> come up with a way to associate an extra bit of data with each "struct
>>> rsc_drv".  Rather than that, just add an extra field into "struct
>>> rsc_drv".  Problem solved.
>>>
>>> See http://crosreview.com/1054646 for what I mean.
>>>
>> I tried to avoid such pointer references and keep it object oriented
>> with this approach. I agree that we run through a list of 2 (at the max)
>> RSC to get the drv* from the rpmh_ctrlr. It is not going to be
>> expensive.
>
>Even if you wanted to keep things "object oriented" then IMHO your
>code still should change.  Sure, it's not computationally expensive to
>iterate through this list, but it adds an extra level of complexity
>that doesn't seem justified.
>
>If you _really_ needed an abstraction barrier then at least add a
>"void *client_data" to "struct rsc_drv.c".  At least you'd get rid of
>the ugly global list and store your pointer directly in the correct
>structure rather than creating an external entity.  Now it becomes
>100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each
>controller.  ...but IMO there's enough intertwining between "rpmh.c"
>and "rpmh-rsc.c" that it would just be a waste because now you'd need
>to do extra memory allocation and freeing.  ...and if you just
>allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal
>because this one-time allocation (that affects _all_ RPMh clients)
>happens whenever one client happens to do the first access.  This is
>one-time init so it makes sense to do it at init time.
>
>I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c"
>because both C files call directly into one another and have intimate
>knowledge of how the other works.  They aren't really separate things.
>Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when
>it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls
>into "rpmh.c".
>
>
>OK, so I've been browsing through the source code more so I can be a
>little more informed here.  As far as I can tell "rpmh.c"'s goal is:
>
>1. Put a simpler API atop "rpmh-rsc.c".  ...but why not just put that
>API directly into "rpmh-rsc.c" anyway?  If there was someone that
>needed the more complex API then having a "simpler" wrapper makes
>sense, but that's not the case, is it?
>
>2. Add caching atop "rpmh-rsc"
>
>
>I'll respond to some of your other patches too, but I think that the
>amount of code for caching is not very much.  I don't see the benefit
>of trying to split the code into two files.  Put them into one and
>then delete all the extra code you needed just the try to maintain
>some abstraction.
>
>
>> Another things this helps with is that, if the driver is not a child of
>> the RSC nodes in DT, then the drvdata of the parent would not a RSC node
>> and accessing that would result in a crash. This offers a cleaner exit
>> path for the error case.
>
>Why would the driver not be a child of the RSC nodes?  That's kinda
>like saying "if you try to instantiate an i2c device as a platform
>device then its probe will crash".  Yeah, it will.  Doctor, it hurts
>if I poke myself in my eye with this sharp stick, do you have any
>medicine that can help fix that?
>
>
>>>>> I'll try to dig into this more so I could just be confused, but in
>>>>> general it seems really odd to have a spinlock and something called a
>>>>> "cache" at this level.  If we need some sort of mutual exclusion or
>>>>> caching it seems like it should be stored in memory directly
>>>>> associated with the RPMh device, not some external global.
>>>>>
>>>> The idea behind the locking is not to avoid the race between rpmh.c and
>>>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
>>>> probed following the probe of the controller. And init is not that we are
>>>> worried about.
>>>> The condition here is to prevent the rpmh_rsc[] from being modified
>>>> concurrently by drivers.
>>>
>>>
>>> OK, I see the point of the locking now, but not the list still.
>>> Sounds like Matthias agrees with me that the list isn't useful.  Seems
>>> like you should squash my patch at http://crosreview.com/1042883 into
>>> yours.
>>>
>> I saw your approach. I am okay with it for your tree,
>
>I'm not okay with it for the Chrome OS tree.  We need to match
>upstream, not fork for style reasons.  I'd rather take a driver that I
>think it overly complex but matches upstream than a private driver.
>
>
>> my approach comes
>> out of experiences in qcom platforms and how things tend to shape up in
>> the future. I would want you to consider my reasoning as well, before we
>> go forward.
>
>I suppose we can get advice from others who have worked in qcom
>platforms and see what they think.  My opinions come out of years of
>experience working with Linux drivers, so I guess we both have some
>experience under our belts that we're trying to leverage.
>
Doug, I am sorry it came out the wrong way. I meant to say, knowing
qualcomm platforms, as it has been in the past, we need this
flexibility. Things change with hardware variants just a wee bit that it
doesn't warrant a new interface, just a new driver or part of it to be
re-written. Keeping code separate out like this helps us maintain the
driver better.

Thanks for your reviews. I will try to address your comments before my
vacation, but I doubt I will get to all of it.

Thanks,
Lina