drivers/qcom: add RPMH communication support

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

new file mode 100644
index 000000000000..3e68cef5513e
@@ -0,0 +1,674 @@
+/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ *

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

+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include <asm-generic/io.h>
+#include <soc/qcom/tcs.h>
+#include <dt-bindings/soc/qcom,rpmh-rsc.h>
+
+#include "rpmh-internal.h"
+
+#define MAX_CMDS_PER_TCS		16
+#define MAX_TCS_PER_TYPE		3
+
+#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)
+
+#define TCS_TYPE_NR			4
+#define MAX_TCS_NR			(MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+
+struct rsc_drv;
+
+/**
+ * tcs_response: Response object for a request
+ *
+ * @drv: the controller
+ * @msg: the request for this response
+ * @m: the tcs identifier
+ * @err: error reported in the response
+ * @list: link list object.
+ */
+struct tcs_response {
+	struct rsc_drv *drv;
+	struct tcs_mbox_msg *msg;
+	u32 m;
+	int err;
+	struct list_head list;
+};
+
+/**
+ * tcs_mbox: group of TCSes for a request state

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

+	struct rsc_drv *drv;
+	int type;
+	u32 tcs_mask;
+	u32 tcs_offset;
+	int num_tcs;
+	int ncpt;
+	spinlock_t tcs_lock;
+	struct tcs_response *responses[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * rsc_drv: the RSC controller
+ *
+ * @pdev: platform device of this controller
+ * @name: controller identifier
+ * @base: start address of the controller
+ * @reg_base: start address of the registers in this controller

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

+			break;
+	}
+
+	if (i == drv->num_tcs) {
+		WARN(1, "Incorrect TCS index %d", m);
+		tcs = NULL;
+	}
+
+	return tcs;
+}

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

+	tcs->responses[m - tcs->tcs_offset] = resp;
+
+	return resp;
+}
+
+static void free_response(struct tcs_response *resp)
+{
+	kfree(resp);
+}
+

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

+}
+
+static inline void write_tcs_reg_sync(void __iomem *base, int reg, int m, int n,
+				u32 data)
+{
+	do {
+		write_tcs_reg(base, reg, m, n, data);
+		if (data == read_tcs_reg(base, reg, m, n))
+			break;

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

+}
+
+static inline struct tcs_mbox *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+	int i;
+	struct tcs_mbox *tcs;
+
+	for (i = 0; i < TCS_TYPE_NR; i++)
+		if (type == drv->tcs[i].type)
+			break;
+
+	if (i == TCS_TYPE_NR)
+		return ERR_PTR(-EINVAL);
+
+	tcs = &drv->tcs[i];
+	if (!tcs->num_tcs)
+		return ERR_PTR(-EINVAL);
+
+	return tcs;
+}
+
+static inline struct tcs_mbox *get_tcs_for_msg(struct rsc_drv *drv,
+						struct tcs_mbox_msg *msg)
+{
+	int type = -1;
+

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

+
+	return get_tcs_of_type(drv, type);
+}
+
+static inline void send_tcs_response(struct tcs_response *resp)
+{

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

+
+/**
+ * tcs_irq_handler: TX Done interrupt handler
+ */
+static irqreturn_t tcs_irq_handler(int irq, void *p)
+{
+	struct rsc_drv *drv = p;
+	void __iomem *base = drv->reg_base;
+	int m, i;
+	u32 irq_status, sts;
+	struct tcs_response *resp;
+	struct tcs_cmd *cmd;
+	int err;

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

+				}
+			}
+		}
+
+		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
+		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
+		atomic_set(&drv->tcs_in_use[m], 0);
+
+		if (resp) {
+			resp->err = err;
+			send_tcs_response(resp);
+		}
+	}
+
+	return IRQ_HANDLED;
+}

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

+{
+	struct rsc_drv *drv = (struct rsc_drv *)data;
+	struct tcs_response *resp;
+	unsigned long flags;
+	int err;
+
+	do {
+		spin_lock_irqsave(&drv->drv_lock, flags);
+		if (list_empty(&drv->response_pending)) {

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

+}
+
+static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
+				struct tcs_mbox_msg *msg)
+{
+	void __iomem *base = drv->reg_base;
+	u32 msgid, cmd_msgid = 0;
+	u32 cmd_enable = 0;
+	u32 cmd_complete;
+	struct tcs_cmd *cmd;
+	int i;
+
+	cmd_msgid = CMD_MSGID_LEN;
+	cmd_msgid |= (msg->is_complete) ? CMD_MSGID_RESP_REQ : 0;
+	cmd_msgid |= CMD_MSGID_WRITE;
+
+	cmd_complete = read_tcs_reg(base, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
+
+	for (i = 0; i < msg->num_payload; i++) {
+		cmd = &msg->payload[i];
+		cmd_enable |= BIT(n + i);
+		cmd_complete |= cmd->complete << (n + i);
+		msgid = cmd_msgid;
+		msgid |= (cmd->complete) ? CMD_MSGID_RESP_REQ : 0;
+		write_tcs_reg(base, RSC_DRV_CMD_MSGID, m, n + i, msgid);
+		write_tcs_reg(base, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr);
+		write_tcs_reg(base, RSC_DRV_CMD_DATA, m, n + i, cmd->data);
+	}
+
+	write_tcs_reg(base, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
+	cmd_enable |= read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
+	write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
+}
+
+static void __tcs_trigger(struct rsc_drv *drv, int m)
+{
+	void __iomem *base = drv->reg_base;
+	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(base, RSC_DRV_CONTROL, m, 0);
+	enable &= ~TCS_AMC_MODE_TRIGGER;
+	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
+	enable &= ~TCS_AMC_MODE_ENABLE;
+	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
+
+	/* Enable the AMC mode on the TCS and then trigger the TCS */
+	enable = TCS_AMC_MODE_ENABLE;
+	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
+	enable |= TCS_AMC_MODE_TRIGGER;
+	write_tcs_reg_sync(base, RSC_DRV_CONTROL, m, 0, enable);
+}
+
+static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_mbox *tcs,
+						struct tcs_mbox_msg *msg)
+{
+	u32 curr_enabled, addr;
+	int i, j, k;
+	void __iomem *base = drv->reg_base;
+	int m = tcs->tcs_offset;

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

+
+			addr = read_tcs_reg(base, RSC_DRV_CMD_ADDR, m, j);
+			for (k = 0; k < msg->num_payload; k++) {
+				if (addr == msg->payload[k].addr)
+					return -EBUSY;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static int find_free_tcs(struct tcs_mbox *tcs)
+{
+	int m;
+
+	for (m = 0; m < tcs->num_tcs; m++)
+		if (tcs_is_free(tcs->drv, tcs->tcs_offset + m))
+			break;
+
+	return (m != tcs->num_tcs) ? m : -EBUSY;
+}
+
+static int tcs_mbox_write(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
+{
+	struct tcs_mbox *tcs;
+	int m;
+	struct tcs_response *resp = NULL;
+	unsigned long flags;
+	int ret = 0;
+
+	tcs = get_tcs_for_msg(drv, msg);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock_irqsave(&tcs->tcs_lock, flags);
+	m = find_free_tcs(tcs);
+	if (m < 0) {
+		ret = m;
+		goto done_write;
+	}
+
+	/*
+	 * The h/w does not like if we send a request to the same address,
+	 * when one is already in-flight or bring processed.
+	 */
+	ret = check_for_req_inflight(drv, tcs, msg);
+	if (ret)
+		goto done_write;
+
+	resp = setup_response(drv, msg, m);
+	if (IS_ERR_OR_NULL(resp)) {
+		ret = PTR_ERR(resp);
+		goto done_write;
+	}
+	resp->m = m;
+
+	atomic_set(&drv->tcs_in_use[m], 1);
+	__tcs_buffer_write(drv, m, 0, msg);
+	__tcs_trigger(drv, m);
+
+done_write:
+	spin_unlock_irqrestore(&tcs->tcs_lock, flags);
+	return ret;
+}
+

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

+ */
+int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
+{
+	int ret = 0;
+

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

+			udelay(10);
+		}
+	} while (ret == -EBUSY);
+
+	return ret;

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

+	int st = 0;
+	int i, ret, nelem;
+	u32 config, max_tcs, ncpt;
+	int tcs_type_count[TCS_TYPE_NR] = { 0 };
+	struct resource *res;
+
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
+	if (ret)
+		return ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+	drv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drv->base))
+		return PTR_ERR(drv->base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -EINVAL;
+	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drv->reg_base))
+		return PTR_ERR(drv->reg_base);
+
+	config = read_drv_config(drv->base);
+	max_tcs = config & (DRV_NUM_TCS_MASK <<
+				(DRV_NUM_TCS_SHIFT * drv->drv_id));
+	max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
+	ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
+	ncpt = ncpt >> DRV_NCPT_SHIFT;
+
+	nelem = of_property_count_elems_of_size(dn, "qcom,tcs-config",
+						sizeof(u32));
+	if (!nelem || (nelem % 2) || (nelem > 2 * TCS_TYPE_NR))
+		return -EINVAL;
+
+	ret = of_property_read_u32_array(dn, "qcom,tcs-config", val, nelem);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < (nelem / 2); i++) {

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

+
+	for (i = 0; i < (nelem / 2); i++) {
+		tcs = &drv->tcs[val[2 * i]];
+		tcs->drv = drv;
+		tcs->type = val[2 * i];
+		tcs->num_tcs = val[2 * i + 1];
+		tcs->ncpt = ncpt;
+		spin_lock_init(&tcs->tcs_lock);
+
+		if (tcs->num_tcs <= 0 || tcs->type == CONTROL_TCS)
+			continue;
+
+		if (tcs->num_tcs > MAX_TCS_PER_TYPE ||
+			st + tcs->num_tcs > max_tcs ||
+			st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
+			return -EINVAL;
+
+		tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
+		tcs->tcs_offset = st;
+		st += tcs->num_tcs;
+	}
+
+	drv->num_tcs = st;
+	drv->pdev = pdev;
+	INIT_LIST_HEAD(&drv->response_pending);
+	spin_lock_init(&drv->drv_lock);
+	tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
+
+	drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
+	if (!drv->name)
+		drv->name = dev_name(&pdev->dev);
+
+	irq = of_irq_get(dn, 0);
+	if (irq < 0)
+		return irq;
+

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

+
+	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id rpmh_drv_match[] = {

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

+	},
+};
+
+static int __init rpmh_driver_init(void)
+{
+	return platform_driver_register(&rpmh_driver);
+}
+arch_initcall(rpmh_driver_init);

include/dt-bindings/soc/qcom,rpmh-rsc.h new file +16 -0

new file mode 100644
index 000000000000..24a56a5cf096
@@ -0,0 +1,16 @@
+/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define SLEEP_TCS	0
+#define WAKE_TCS	1
+#define ACTIVE_TCS	2
+#define CONTROL_TCS	3

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

new file mode 100644
index 000000000000..07cf6d8d43e3
@@ -0,0 +1,38 @@
+/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+

Quoted 8 lines · Lina Iyer

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

new file mode 100644
index 000000000000..3e68cef5513e
@@ -0,0 +1,674 @@
+/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ *

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

Quoted 10 lines

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

+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */

Please use SPDX headers for new files.

// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. */

Quoted 5 lines

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

+
+#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
+
+#include <linux/atomic.h>
+#include <linux/delay.h>

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

Quoted 6 lines

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

+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>

Unused?

Quoted 6 lines

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

+/**
+ * tcs_mbox: group of TCSes for a request state
+ *
+ * @type: type of the TCS in this group - active, sleep, wake
+ * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
+ * @tcs_offset: start of the TCS group relative to the TCSes in the RSC

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

Quoted 6 lines

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

+ * @num_tcs: number of TCSes in this type
+ * @ncpt: number of commands in each TCS
+ * @tcs_lock: lock for synchronizing this TCS writes
+ * @responses: response objects for requests sent from each TCS
+ */
+struct tcs_mbox {

struct tcs_group ?

Quoted 22 lines

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

+	struct rsc_drv *drv;
+	int type;
+	u32 tcs_mask;
+	u32 tcs_offset;
+	int num_tcs;
+	int ncpt;
+	spinlock_t tcs_lock;
+	struct tcs_response *responses[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * rsc_drv: the RSC controller
+ *
+ * @pdev: platform device of this controller
+ * @name: controller identifier
+ * @base: start address of the controller
+ * @reg_base: start address of the registers in this controller
+ * @drv_id: instance id in the controller (DRV)
+ * @num_tcs: number of TCSes in this DRV
+ * @tasklet: handle responses, off-load work from IRQ handler
+ * @response_pending: list of responses that needs to be sent to caller
+ * @tcs: TCS groups

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

Quoted 5 lines

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

+ * @tcs_in_use: s/w state of the TCS
+ * @drv_lock: synchronize state of the controller
+ */
+struct rsc_drv {
+	struct platform_device *pdev;

This is not used outside rpmh_rsc_probe() and I presume you would like to be able to use it for dev_prints(), if so store the device * instead.

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

Quoted 4 lines

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

+	const char *name;
+	void __iomem *base;
+	void __iomem *reg_base;
+	int drv_id;

@base and @drv_id are local to rpmh_rsc_probe(), so you can make it them local variables.

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

Quoted 5 lines

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

+	int num_tcs;
+	struct tasklet_struct tasklet;
+	struct list_head response_pending;
+	struct tcs_mbox tcs[TCS_TYPE_NR];
+	atomic_t tcs_in_use[MAX_TCS_NR];

Does this really need to be an atomic_t? Seems like it's protected by the drv_lock.

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

Quoted 4 lines

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

+	spinlock_t drv_lock;
+};
+
+static inline struct tcs_mbox *get_tcs_from_index(struct rsc_drv *drv, int m)

get_tcs_group_by_index(), or maybe even get_tcs_group_by_tcs()? To clarify that this resolves a group and not a single tcs.

Also, please drop the "inline".

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

Quoted 7 lines

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

+{
+	struct tcs_mbox *tcs = NULL;
+	int i;
+
+	for (i = 0; i < drv->num_tcs; i++) {
+		tcs = &drv->tcs[i];
+		if (tcs->tcs_mask & (u32)BIT(m))

I don't think you need this cast.

Quoted 16 lines

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

+			break;
+	}
+
+	if (i == drv->num_tcs) {
+		WARN(1, "Incorrect TCS index %d", m);
+		tcs = NULL;
+	}
+
+	return tcs;
+}
+
+static struct tcs_response *setup_response(struct rsc_drv *drv,
+			struct tcs_mbox_msg *msg, int m)
+{
+	struct tcs_response *resp;
+	struct tcs_mbox *tcs;

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

Quoted 4 lines

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

+
+	resp = kcalloc(1, sizeof(*resp), GFP_ATOMIC);
+	if (!resp)
+		return ERR_PTR(-ENOMEM);

Rather than allocating a response for each write request and freeing it in the tasklet I think you should just allocate MAX_TCS_PER_TYPE responses when you're probing the driver.

Quoted 9 lines

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

+
+	resp->drv = drv;
+	resp->msg = msg;
+	resp->err = 0;
+
+	tcs = get_tcs_from_index(drv, m);
+	if (!tcs)
+		return ERR_PTR(-EINVAL);
+

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

Quoted 4 lines

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

+	/*
+	 * We should have been called from a TCS-type locked context, and
+	 * we overwrite the previously saved response.
+	 */

I assume that this overwrite wouldn't happen because we're only writing to the tcs if it's not in use? Wouldn't overwriting a valid response lead to us leaking the previously allocated resp?

Quoted 16 lines

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

+	tcs->responses[m - tcs->tcs_offset] = resp;
+
+	return resp;
+}
+
+static void free_response(struct tcs_response *resp)
+{
+	kfree(resp);
+}
+
+static inline struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
+{
+	struct tcs_mbox *tcs = get_tcs_from_index(drv, m);
+
+	return tcs->responses[m - tcs->tcs_offset];
+}

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

Quoted 4 lines

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

+
+static inline u32 read_drv_config(void __iomem *base)
+{
+	return le32_to_cpu(readl_relaxed(base + DRV_PRNT_CHLD_CONFIG));

readl_relaxed() on arm64 is defined as le32_to_cpu(__raw_readl(c)), so I would suggest that you drop the le32_to_cpu() and just inline your readl() directly in rpmh_rsc_probe().

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

Quoted 4 lines

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

+}
+
+static inline u32 read_tcs_reg(void __iomem *base, int reg, int m, int n)
+{

I would suggest that you pass the rsc_drv to these functions, so that it's obvious which "base" we're operating on here.

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

Quoted 2 lines

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

+	return le32_to_cpu(readl_relaxed(base + reg +
+			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n));

No le32_to_cpu()

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

Quoted 7 lines

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

+}
+
+static inline void write_tcs_reg(void __iomem *base, int reg, int m, int n,
+				u32 data)
+{
+	writel_relaxed(cpu_to_le32(data), base + reg +
+			RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n);

As with readl, writel does cpu_to_le32()

Quoted 14 lines

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

+}
+
+static inline void write_tcs_reg_sync(void __iomem *base, int reg, int m, int n,
+				u32 data)
+{
+	do {
+		write_tcs_reg(base, reg, m, n, data);
+		if (data == read_tcs_reg(base, reg, m, n))
+			break;
+		udelay(1);
+	} while (1);
+}
+
+static inline bool tcs_is_free(struct rsc_drv *drv, int m)

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

Quoted 5 lines

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

+{
+	void __iomem *base = drv->reg_base;
+
+	return read_tcs_reg(base, RSC_DRV_STATUS, m, 0) &&
+			!atomic_read(&drv->tcs_in_use[m]);

If you flip these conditions around you won't do the io access when tcs_in_use[m].

What are the cases when tcs_in_use will tell you that the tcs is free, but the hardware will say it's not?

Quoted 30 lines

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

+}
+
+static inline struct tcs_mbox *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+	int i;
+	struct tcs_mbox *tcs;
+
+	for (i = 0; i < TCS_TYPE_NR; i++)
+		if (type == drv->tcs[i].type)
+			break;
+
+	if (i == TCS_TYPE_NR)
+		return ERR_PTR(-EINVAL);
+
+	tcs = &drv->tcs[i];
+	if (!tcs->num_tcs)
+		return ERR_PTR(-EINVAL);
+
+	return tcs;
+}
+
+static inline struct tcs_mbox *get_tcs_for_msg(struct rsc_drv *drv,
+						struct tcs_mbox_msg *msg)
+{
+	int type = -1;
+
+	switch (msg->state) {
+	case RPMH_ACTIVE_ONLY_STATE:
+		type = ACTIVE_TCS;
+		break;

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

Quoted 6 lines

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

+	default:
+		break;
+	}
+
+	if (type < 0)
+		return ERR_PTR(-EINVAL);

afaict you can put this return in the default case.

Quoted 11 lines

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

+
+	return get_tcs_of_type(drv, type);
+}
+
+static inline void send_tcs_response(struct tcs_response *resp)
+{
+	struct rsc_drv *drv = resp->drv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drv->drv_lock, flags);
+	INIT_LIST_HEAD(&resp->list);

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

Quoted 5 lines

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

+	list_add_tail(&resp->list, &drv->response_pending);
+	spin_unlock_irqrestore(&drv->drv_lock, flags);
+
+	tasklet_schedule(&drv->tasklet);
+}

Is there any particular reason why this uses a tasklet, instead of just handling the tx_done event from the interrupt handler directly?

Quoted 18 lines

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

+
+/**
+ * tcs_irq_handler: TX Done interrupt handler
+ */
+static irqreturn_t tcs_irq_handler(int irq, void *p)
+{
+	struct rsc_drv *drv = p;
+	void __iomem *base = drv->reg_base;
+	int m, i;
+	u32 irq_status, sts;
+	struct tcs_response *resp;
+	struct tcs_cmd *cmd;
+	int err;
+
+	irq_status = read_tcs_reg(base, RSC_DRV_IRQ_STATUS, 0, 0);
+
+	for (m = 0; m < drv->num_tcs; m++) {
+		if (!(irq_status & (u32)BIT(m)))

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

Quoted 5 lines

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

+			continue;
+
+		err = 0;
+		resp = get_response(drv, m);
+		if (!resp) {

This conditional section dereferences resp, so I suspect that the conditional is backwards.

Quoted 4 lines

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

+			for (i = 0; i < resp->msg->num_payload; i++) {
+				cmd = &resp->msg->payload[i];
+				sts = read_tcs_reg(base, RSC_DRV_CMD_STATUS,
+							m, i);

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

Quoted 6 lines

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

+				if ((!(sts & CMD_STATUS_ISSUED)) ||
+					((resp->msg->is_complete ||
+					  cmd->complete) &&
+					(!(sts & CMD_STATUS_COMPL)))) {
+					resp->err = -EIO;
+					break;

As this completes the response, aren't we leaking the tcs_in_use?

Quoted 21 lines

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

+				}
+			}
+		}
+
+		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
+		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
+		atomic_set(&drv->tcs_in_use[m], 0);
+
+		if (resp) {
+			resp->err = err;
+			send_tcs_response(resp);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * tcs_notify_tx_done: TX Done for requests that got a response
+ *
+ * @data: the tasklet argument

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

Quoted 5 lines

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

+ *
+ * Tasklet function to notify MBOX that we are done with the request.
+ * Handles all pending reponses whenever run.
+ */
+static void tcs_notify_tx_done(unsigned long data)

I think you should drop the tasklet and just invoke the handling of responses directly from the irq context.

Quoted 13 lines

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

+{
+	struct rsc_drv *drv = (struct rsc_drv *)data;
+	struct tcs_response *resp;
+	unsigned long flags;
+	int err;
+
+	do {
+		spin_lock_irqsave(&drv->drv_lock, flags);
+		if (list_empty(&drv->response_pending)) {
+			spin_unlock_irqrestore(&drv->drv_lock, flags);
+			break;
+		}
+		resp = list_first_entry(&drv->response_pending,

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

Quoted 6 lines

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

+					struct tcs_response, list);
+		list_del(&resp->list);
+		spin_unlock_irqrestore(&drv->drv_lock, flags);
+		err = resp->err;
+		free_response(resp);
+	} while (1);

Use for (;;) {} or while (1) {}, rather than do {} while(1)

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

Quoted 5 lines

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

+		curr_enabled = read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
+
+		for (j = 0; j < MAX_CMDS_PER_TCS; j++) {
+			if (!(curr_enabled & (u32)BIT(j)))
+				continue;

Consider using for_each_set_bit() here.

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

Quoted 10 lines

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

+/**
+ * rpmh_rsc_send_data: Validate the incoming message and write to the
+ * appropriate TCS block.
+ *
+ * @drv: the controller
+ * @msg: the data to be sent
+ *
+ * Returns a negative error for invalid message structure and invalid
+ * message combination, -EBUSY if there is an other active request for
+ * the channel in process, otherwise bubbles up internal error.

Kernel-doc style is "Return: a negative..."

The function never returns -EBUSY, as you're looping while this is being returned.

Also, it would be nice if the doc mentions that 0 is returned on success, so perhaps just "Return: 0 on success, negative errno on error"?

Quoted 9 lines

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

+ */
+int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
+{
+	int ret = 0;
+
+	if (!msg || !msg->payload || !msg->num_payload ||
+			msg->num_payload > MAX_RPMH_PAYLOAD)
+		return -EINVAL;
+

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

Quoted 6 lines

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

+	do {
+		ret = tcs_mbox_write(drv, msg);
+		if (ret == -EBUSY) {
+			pr_info_ratelimited(
+			"TCS Busy, retrying RPMH message send: addr=0x%x\n",
+			msg->payload[0].addr);

Indent wrapped lines to the start parenthesis.

Quoted 11 lines

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

+			udelay(10);
+		}
+	} while (ret == -EBUSY);
+
+	return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_send_data);
+
+static int rpmh_rsc_probe(struct platform_device *pdev)
+{
+	struct device_node *dn = pdev->dev.of_node;

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

Quoted 4 lines

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

+	struct rsc_drv *drv;
+	struct tcs_mbox *tcs;
+	int irq;
+	u32 val[8] = { 0 };

8 here should be TCS_TYPE_NR * 2, but I like Tomas(?) suggestion in the PDC driver of representing arrays of tuples like this as a structured type.

Quoted 45 lines

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

+	int st = 0;
+	int i, ret, nelem;
+	u32 config, max_tcs, ncpt;
+	int tcs_type_count[TCS_TYPE_NR] = { 0 };
+	struct resource *res;
+
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
+	if (ret)
+		return ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+	drv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drv->base))
+		return PTR_ERR(drv->base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -EINVAL;
+	drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drv->reg_base))
+		return PTR_ERR(drv->reg_base);
+
+	config = read_drv_config(drv->base);
+	max_tcs = config & (DRV_NUM_TCS_MASK <<
+				(DRV_NUM_TCS_SHIFT * drv->drv_id));
+	max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
+	ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
+	ncpt = ncpt >> DRV_NCPT_SHIFT;
+
+	nelem = of_property_count_elems_of_size(dn, "qcom,tcs-config",
+						sizeof(u32));
+	if (!nelem || (nelem % 2) || (nelem > 2 * TCS_TYPE_NR))
+		return -EINVAL;
+
+	ret = of_property_read_u32_array(dn, "qcom,tcs-config", val, nelem);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < (nelem / 2); i++) {

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

Quoted 10 lines

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

+		if (val[2 * i] >= TCS_TYPE_NR)
+			return -EINVAL;
+		tcs_type_count[val[2 * i]]++;
+		if (tcs_type_count[val[2 * i]] > 1)
+			return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(tcs_type_count); i++)
+		if (!tcs_type_count[i])
+			return -EINVAL;

I think it's better if replace these two loops by adding a check in below loop to ensure that tcs->drv is NULL and that tcs->type is valid and then follow up with a loop checking that each drv->tcs was initialized.

That way you don't need to do the counting of the various types.

Quoted 41 lines

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

+
+	for (i = 0; i < (nelem / 2); i++) {
+		tcs = &drv->tcs[val[2 * i]];
+		tcs->drv = drv;
+		tcs->type = val[2 * i];
+		tcs->num_tcs = val[2 * i + 1];
+		tcs->ncpt = ncpt;
+		spin_lock_init(&tcs->tcs_lock);
+
+		if (tcs->num_tcs <= 0 || tcs->type == CONTROL_TCS)
+			continue;
+
+		if (tcs->num_tcs > MAX_TCS_PER_TYPE ||
+			st + tcs->num_tcs > max_tcs ||
+			st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
+			return -EINVAL;
+
+		tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
+		tcs->tcs_offset = st;
+		st += tcs->num_tcs;
+	}
+
+	drv->num_tcs = st;
+	drv->pdev = pdev;
+	INIT_LIST_HEAD(&drv->response_pending);
+	spin_lock_init(&drv->drv_lock);
+	tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
+
+	drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
+	if (!drv->name)
+		drv->name = dev_name(&pdev->dev);
+
+	irq = of_irq_get(dn, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
+			IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND, drv->name, drv);
+	if (ret)
+		return ret;
+

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

Quoted 5 lines

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

+	write_tcs_reg(drv->reg_base, RSC_DRV_IRQ_ENABLE, 0, 0,
+					drv->tcs[ACTIVE_TCS].tcs_mask);
+
+	for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
+		atomic_set(&drv->tcs_in_use[i], 0);

Perhaps do this before requesting the irq? If nothing else that saves future readers from wondering if it makes any difference...

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

Quoted 2 lines

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

+
+	dev_set_drvdata(&pdev->dev, drv);

This isn't used until the next patch, so move it there to clarify its purpose.

Quoted 5 lines

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

+
+	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id rpmh_drv_match[] = {

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

Quoted 10 lines

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

+	{ .compatible = "qcom,rpmh-rsc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpm_drv_match);
+
+static struct platform_driver rpmh_driver = {
+	.probe = rpmh_rsc_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = rpmh_drv_match,

As we don't want to handle unbind, add:

.suppress_bind_attrs = true,

Quoted 8 lines

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

+	},
+};
+

include/dt-bindings/soc/qcom,rpmh-rsc.h new file +0 -0

new file mode 100644
index 000000000000..24a56a5cf096

include/dt-bindings/soc/qcom,rpmh-rsc.h

Quoted 5 lines

include/dt-bindings/soc/qcom,rpmh-rsc.h new file +5 -0

+struct tcs_cmd {
+	u32 addr;		/* slv_id:18:16 | offset:0:15 */
+	u32 data;		/* data for resource (or read response) */
+	bool complete;		/* wait for completion before sending next */
+};

I think you should use kernel-doc to describe the structs.

include/dt-bindings/soc/qcom,rpmh-rsc.h

Quoted 8 lines

include/dt-bindings/soc/qcom,rpmh-rsc.h new file +8 -0

+
+enum rpmh_state {
+	RPMH_ACTIVE_ONLY_STATE,	/* Active only (= AMC) */
+	RPMH_WAKE_ONLY_STATE,	/* Wake only */
+	RPMH_SLEEP_STATE,	/* Sleep */
+};
+
+struct tcs_mbox_msg {

struct tcs_request ?

include/dt-bindings/soc/qcom,rpmh-rsc.h

Quoted 7 lines

include/dt-bindings/soc/qcom,rpmh-rsc.h new file +7 -0

+	enum rpmh_state state;	/* request state */
+	bool is_complete;	/* wait for resp from accelerator */
+	u32 num_payload;	/* Limited to MAX_RPMH_PAYLOAD in one msg */
+	struct tcs_cmd *payload;/* array of tcs_cmds */
+};
+
+#endif /* __SOC_QCOM_TCS_H__ */

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

Cc [email protected]

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

---
 .../devicetree/bindings/arm/msm/rpmh-rsc.txt       | 134 +++++++++++++++++++++

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

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

new file mode 100644
index 000000000000..b0cd55caf916
@@ -0,0 +1,134 @@
+RPMH RSC:
+------------
+
+RPMH is the mechanism for communicating with the hardened resource
+accelerators. Requests to the resources can be written to the TCS mailbox
+registers and using a (addr, val) pair and triggered. Messages in the TCS are
+then sent in sequence over an internal bus.
+
+The hardware block (Direct Resource Voter or DRV) is a part of the h/w entity
+(Resource State Coordinator a.k.a RSC) that can handle a multiple sleep and
+active/wake resource requests. Multiple such DRVs can exist in a SoC and can
+be written to from Linux. The structure of each DRV follows the same template
+with a few variations that are captured by the properties here.
+
+Each DRV could have 'm' TCS instances. Each TCS could have 'n' slots. Each
+slot has a header (u32), address (u32), data (u32), status (u32) and a
+read-response (u32). A TCS when triggered will send all the enabled commands
+of the 'n' commands in that slot in sequence.
+
+A TCS may be triggered from Linux or triggered by the F/W after all the CPUs
+have powered off to faciliate 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 aggregeate of the sleep votes from each of those subsystem. Drivers
+may request a sleep value for their shared resources in addition to the active
+mode requests.
+
+Control requests are instance specific requests that may or may not reach an
+accelerator. Only one platform device in Linux can request a control channel
+on a DRV.
+
+CONTROLLER:
+----------
+
+PROPERTIES:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "qcom,rpmh-rsc".
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: the first element specifies the base address of the DRV,
+	            the second element specifies the size of the region.
+
+- interrupts:
+	Usage: required

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

+
+EXAMPLE 1:
+
+For a TCS whose RSC base address is is 0x179C0000 and is at a DRV of 2, the
+register offsets for DRV2 start at 0D00, the register calculations are like
+this -
+First tuple: 0x179C0000 + 0x10000 * 2 = 0x179E0000
+Second tuple: 0x179E0000 + 0xD00  = 0x179E0D00
+
+	apps_rsc: rsc@179e000 {
+		compatible = "qcom,rpmh-rsc";
+		label = "apps_rsc";
+		reg = <0x179e0000 0x10000>, <0x179e0d00 0x3000>;
+		interrupts = <0 5 0>;
+		qcom,drv-id = <2>;
+		qcom,tcs-config = <SLEEP_TCS   3>,
+				  <WAKE_TCS    3>,
+				  <ACTIVE_TCS  2>,
+				  <CONTROL_TCS 1>;
+
+		foo-clk {
+			compatible = "foo-clk";
+		};
+	};
+
+EXAMPLE 2:
+
+For a TCS whose RSC base address is 0xAF20000 and is at DRV of 0, the register
+offsets for DRV0 start at 01C00, the register calculations are like this -
+First tuple: 0xAF20000
+Second tuple: 0xAF20000 + 0x1C00
+
+	disp_rsc: rsc@af20000 {
+		label = "disp_rsc";
+		compatible = "qcom,rpmh-rsc";
+		reg = <0xaf20000 0x10000>, <0xaf21c00 0x3000>;
+		interrupts = <0 129 0>;
+		qcom,drv-id = <0>;
+		qcom,tcs-config = <SLEEP_TCS 1>,
+				<WAKE_TCS    1>,
+				<ACTIVE_TCS  0>,
+				<CONTROL_TCS 1>;
+
+		foo-clk {
+			compatible = "foo-clk";
+		};
+	};

Signature · 7 lines

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

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

Quoted 4 lines · Lina Iyer

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

Quoted 4 lines

Cc [email protected]

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

---
 .../devicetree/bindings/arm/msm/rpmh-rsc.txt       | 134 +++++++++++++++++++++

bindings/mailbox instead?

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt line 68

Quoted 6 lines

+- qcom,drv-id:

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

+	Usage: required
+	Value type: <u32>
+	Definition: the id of the DRV in the RSC block.
+
+- qcom, tcs-config:

         ^

space

Quoted 4 lines

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

+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: the tuple definining the configuration of TCS.
+	            Must have 2 cells which describe each TCS type.

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt line 78

Quoted 6 lines

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

+	            <type number_of_tcs>
+	- Cell #1 (TCS Type): TCS types can be specified -
+		SLEEP_TCS
+		WAKE_TCS
+		ACTIVE_TCS
+		CONTROL_TCS

These defines belong in this patch.

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt line 79

Quoted 1 line

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

+	- Cell #2 (Number of TCS): <u32>

Is there some sort of range of numbers?

Documentation/devicetree/bindings/arm/msm/rpmh-rsc.txt line 86

Quoted 7 lines

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

+
+- label:
+	Usage: optional
+	Value type: <string>
+	Definition: Name for the RSC. The name would be used in trace logs.
+
+Clients the want to use the RSC to communicate with RPMH would specify their

Clients the want?

Quoted 1 line

+bindings as child of the corresponding RSC controllers.

Need to be specific here about what the child nodes are.

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

+	- Cell #1 (TCS Type): TCS types can be specified -
+		SLEEP_TCS
+		WAKE_TCS
+		ACTIVE_TCS
+		CONTROL_TCS
+	- Cell #2 (Number of TCS): <u32>
+

Log sent RPMH requests and interrupt responses in FTRACE.

Cc Steven Rostedt <[email protected]>

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

---
 drivers/soc/qcom/rpmh-rsc.c | 13 ++++++-
 include/trace/events/rpmh.h | 89 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 include/trace/events/rpmh.h

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

index 3e68cef5513e..424dc939b2e6 100644
@@ -33,6 +33,9 @@
 
 #include "rpmh-internal.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/rpmh.h>
+
 #define MAX_CMDS_PER_TCS		16
 #define MAX_TCS_PER_TYPE		3
 

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

 		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
 		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
 		atomic_set(&drv->tcs_in_use[m], 0);

@@ -351,7 +356,8 @@ static void tcs_notify_tx_done(unsigned long data)
 	struct rsc_drv *drv = (struct rsc_drv *)data;
 	struct tcs_response *resp;
 	unsigned long flags;
-	int err;
+	int err, m;
+	struct tcs_mbox_msg *msg;
 
 	do {
 		spin_lock_irqsave(&drv->drv_lock, flags);

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

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

include/trace/events/rpmh.h new file +15 -0

new file mode 100644
index 000000000000..2cc44fc5ff95
@@ -0,0 +1,89 @@
+/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM rpmh
+

include/trace/events/rpmh.h new file +41 -0

+	),
+
+	TP_printk("%s: ack: tcs-m:%d addr: 0x%08x errno: %d",
+		__entry->name, __entry->m, __entry->addr, __entry->errno)
+);
+
+DEFINE_EVENT(rpmh_ack_recvd, rpmh_notify_irq,
+	TP_PROTO(const char *s, int m, u32 addr, int err),
+	TP_ARGS(s, m, addr, err)
+);
+
+DEFINE_EVENT(rpmh_ack_recvd, rpmh_notify,
+	TP_PROTO(const char *s, int m, u32 addr, int err),
+	TP_ARGS(s, m, addr, err)
+);
+
+TRACE_EVENT(rpmh_send_msg,
+
+	TP_PROTO(const char *s, int m, int n, u32 h, u32 a, u32 v, bool c),
+
+	TP_ARGS(s, m, n, h, a, v, c),
+
+	TP_STRUCT__entry(
+		__field(const char*, name)
+		__field(int, m)
+		__field(int, n)
+		__field(u32, hdr)
+		__field(u32, addr)
+		__field(u32, data)
+		__field(bool, complete)
+	),
+
+	TP_fast_assign(
+		__entry->name = s;
+		__entry->m = m;
+		__entry->n = n;
+		__entry->hdr = h;
+		__entry->addr = a;
+		__entry->data = v;
+		__entry->complete = c;
+	),

I wasn't Cc'd on the rest of the patches and this wasn't Cc'd to LKML, and I'm not on any of the mailing lists that were Cc'd, I gotta just look at what I have here in this patch.

On Thu, 18 Jan 2018 17:01:56 -0700
Lina Iyer <[email protected]> wrote:

drivers/soc/qcom/rpmh-rsc.c static irqreturn_t tcs_irq_handler(int irq, void *p) · line 332

Quoted 6 lines

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

@@ -325,6 +328,8 @@ static irqreturn_t tcs_irq_handler(int irq, void *p)
 			}
 		}
 
+		trace_rpmh_notify_irq(drv->name, m, resp->msg->payload[0].addr,
+						resp->err);

Below, m came from resp->m, is it the same here?

Quoted 14 lines

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

 		write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
 		write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
 		atomic_set(&drv->tcs_in_use[m], 0);

@@ -351,7 +356,8 @@ static void tcs_notify_tx_done(unsigned long data)
 	struct rsc_drv *drv = (struct rsc_drv *)data;
 	struct tcs_response *resp;
 	unsigned long flags;
-	int err;
+	int err, m;
+	struct tcs_mbox_msg *msg;
 
 	do {
 		spin_lock_irqsave(&drv->drv_lock, flags);

drivers/soc/qcom/rpmh-rsc.c static void tcs_notify_tx_done(unsigned long data) · line 376

Quoted 7 lines

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

@@ -364,7 +370,10 @@ static void tcs_notify_tx_done(unsigned long data)
 		list_del(&resp->list);
 		spin_unlock_irqrestore(&drv->drv_lock, flags);
 		err = resp->err;
+		m = resp->m;
+		msg = resp->msg;
 		free_response(resp);
+		trace_rpmh_notify(drv->name, m, msg->payload[0].addr, err);

The reason I ask, is that this is causing more code to be executed even when tracing is off. What about passing in resp and assigning it within the tracepoint.

	trace_rpmh_notify(drv->name, resp);

That would keep extra code from being used within the normal code path and only executed in the tracepoint. Get rid of the m = resp->m, msg = resp->msg.

Even if m is something different above, you can still just pass in:

	trace_rpmh_notify(drv->name, m, resp);

Quoted 2 lines

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

 	} while (1);
 }

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

Quoted 7 lines

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

@@ -393,6 +402,8 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n,
 		write_tcs_reg(base, RSC_DRV_CMD_MSGID, m, n + i, msgid);
 		write_tcs_reg(base, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr);
 		write_tcs_reg(base, RSC_DRV_CMD_DATA, m, n + i, cmd->data);
+		trace_rpmh_send_msg(drv->name, m, n + i, msgid, cmd->addr,
+						cmd->data, cmd->complete);

Here just pass in cmd. The less the parameters of a tracepoints, the more likely gcc wont leak code that would be executed when tracing is off.

include/trace/events/rpmh.h line 25

Quoted 7 lines

include/trace/events/rpmh.h new file +7 -0

+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(rpmh_ack_recvd,
+
+	TP_PROTO(const char *s, int m, u32 addr, int errno),
+
+	TP_ARGS(s, m, addr, errno),

Then we could just do:

	TP_PROTO(const char *s, struct tcs_response *resp),

	TP_ARGS(s, resp),

include/trace/events/rpmh.h line 38

Quoted 13 lines

include/trace/events/rpmh.h new file +13 -0

+
+	TP_STRUCT__entry(
+		__field(const char *, name)
+		__field(int, m)
+		__field(u32, addr)
+		__field(int, errno)
+	),
+
+	TP_fast_assign(
+		__entry->name = s;
+		__entry->m = m;
+		__entry->addr = addr;
+		__entry->errno = errno;

	__entry->m = resp->m;
	__entry->addr = resp->msg->payload[0].addr;
	__entry->errno = resp->err;

-- Steve

Hi Steven,

Quoted 5 lines · Steven Rostedt

I wasn't Cc'd on the rest of the patches and this wasn't Cc'd to LKML, and I'm not on any of the mailing lists that were Cc'd, I gotta just look at what I have here in this patch.

I am so sorry. Will add LKML in the future.

Quoted 22 lines

Quoted 10 lines

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

 	struct rsc_drv *drv = (struct rsc_drv *)data;
 	struct tcs_response *resp;
 	unsigned long flags;
-	int err;
+	int err, m;
+	struct tcs_mbox_msg *msg;
 
 	do {
 		spin_lock_irqsave(&drv->drv_lock, flags);

drivers/soc/qcom/rpmh-rsc.c static void tcs_notify_tx_done(unsigned long data) · line 376

Quoted 7 lines

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

@@ -364,7 +370,10 @@ static void tcs_notify_tx_done(unsigned long data)
 		list_del(&resp->list);
 		spin_unlock_irqrestore(&drv->drv_lock, flags);
 		err = resp->err;
+		m = resp->m;
+		msg = resp->msg;
 		free_response(resp);
+		trace_rpmh_notify(drv->name, m, msg->payload[0].addr, err);  

The reason I ask, is that this is causing more code to be executed even when tracing is off. What about passing in resp and assigning it within the tracepoint.

Quoted 2 lines

	trace_rpmh_notify(drv->name, resp);

The free_response(resp) will free the resp object and accessing resp->m is invalid after that.

Quoted 2 lines · On Fri, 19 Jan 2018 07:35:01 +0000 Lina Iyer…

Hi Steven,

Quoted 6 lines · Steven Rostedt

Quoted 3 lines

	trace_rpmh_notify(drv->name, resp);

The free_response(resp) will free the resp object and accessing resp->m is invalid after that.

What about placing the trace before the free?

drivers/soc/qcom/rpmh-internal.h +0 -0

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

+#define RPMH_TIMEOUT msecs_to_jiffies(10000)

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

+
+#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name)	\
+	struct rpmh_msg name = {			\
+		.msg = {				\
+			.state = s,			\
+			.payload = name.cmd,		\
+			.num_payload = 0,		\
+			.is_complete = true,		\
+			.invalidate = false,		\
+		},					\
+		.cmd = { { 0 } },			\
+		.completion = q,			\
+		.wait_count = c,			\
+		.rc = rc,				\
+	}
+

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

+};
+
+/**
+ * rpmh_client: the client object
+ *

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

+
+void rpmh_tx_done(struct tcs_mbox_msg *msg, int r)
+{
+	struct rpmh_msg *rpm_msg = container_of(msg, struct rpmh_msg, msg);
+	atomic_t *wc = rpm_msg->wait_count;
+	struct completion *compl = rpm_msg->completion;
+
+	rpm_msg->err = r;

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

+}
+EXPORT_SYMBOL(rpmh_tx_done);
+
+/**
+ * wait_for_tx_done: Wait until the response is received.
+ *
+ * @rc: The RPMH client
+ * @compl: The completion object
+ * @addr: An addr that we sent in that request
+ * @data: The data for the address in that request
+ *
+ */
+static inline int wait_for_tx_done(struct rpmh_client *rc,
+		struct completion *compl, u32 addr, u32 data)
+{
+	int ret;

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

+
+	return  (ret > 0) ? 0 : -ETIMEDOUT;
+}
+
+/**
+ * __rpmh_write: send the RPMH request
+ *
+ * @rc: The RPMH client
+ * @state: Active/Sleep request type
+ * @rpm_msg: The data that needs to be sent (payload).
+ */
+int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+			struct rpmh_msg *rpm_msg)
+{
+	int ret = -EFAULT;
+
+	rpm_msg->msg.state = state;
+
+	if (state == RPMH_ACTIVE_ONLY_STATE) {
+		WARN_ON(irqs_disabled());
+		ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg);

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

+	}
+
+	return ret;
+}
+
+/**
+ * rpmh_write: Write a set of RPMH commands and block until response
+ *
+ * @rc: The RPMh handle got from rpmh_get_dev_channel
+ * @state: Active/sleep set
+ * @cmd: The payload data
+ * @n: The number of elements in payload
+ *
+ * May sleep. Do not call from atomic contexts.
+ */
+int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+			struct tcs_cmd *cmd, int n)
+{
+	DECLARE_COMPLETION_ONSTACK(compl);
+	atomic_t wait_count = ATOMIC_INIT(1);
+	DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg);
+	int ret;
+
+	if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD)
+		return -EINVAL;
+

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

+}
+EXPORT_SYMBOL(rpmh_write);
+
+static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev)
+{
+	int i;
+	struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent);
+	struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT);
+
+	if (!drv)
+		return ctrlr;
+
+	mutex_lock(&rpmh_ctrlr_mutex);
+	for (i = 0; i < RPMH_MAX_MBOXES; i++)
+		if (rpmh_rsc[i].drv == drv) {
+			ctrlr = &rpmh_rsc[i];

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

+{
+	struct rpmh_client *rc;
+
+	rc = kzalloc(sizeof(*rc), GFP_KERNEL);
+	if (!rc)
+		return ERR_PTR(-ENOMEM);
+
+	rc->dev = &pdev->dev;
+	rc->ctrlr = get_rpmh_ctrlr(pdev);
+	if (IS_ERR(rc->ctrlr)) {
+		kfree(rc);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return rc;
+}

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

+}
+EXPORT_SYMBOL(rpmh_release);

include/soc/qcom/rpmh.h +0 -0

drivers/soc/qcom/Makefile obj-$(CONFIG_QCOM_SMP2P) += smp2p.o · line 14

Quoted 10 lines · Lina Iyer

drivers/soc/qcom/Makefile +1 -1

index b7951ce87663..0dba46387f1c 100644
@@ -11,4 +11,4 @@ obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
-obj-$(CONFIG_QCOM_RPMH)	+=	rpmh-rsc.o
+obj-$(CONFIG_QCOM_RPMH)	+=	rpmh-rsc.o rpmh.o

I think it would be better if you built these two objects into the same module;

obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o
qcom_rpmh-y += rpmh-rsc.o
qcom_rpmh-y += rpmh.o

Quoted 1 line

drivers/soc/qcom/rpmh-internal.h +0 -0

[..]

Quoted 1 line

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

[..]

Quoted 1 line

+#define RPMH_TIMEOUT msecs_to_jiffies(10000)

10 * HZ

Quoted 20 lines

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

+
+#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name)	\
+	struct rpmh_msg name = {			\
+		.msg = {				\
+			.state = s,			\
+			.payload = name.cmd,		\
+			.num_payload = 0,		\
+			.is_complete = true,		\
+			.invalidate = false,		\
+		},					\
+		.cmd = { { 0 } },			\
+		.completion = q,			\
+		.wait_count = c,			\
+		.rc = rc,				\
+	}
+
+/**
+ * rpmh_msg: the message to be sent to rpmh-rsc
+ *
+ * @msg: the request

Quoted 6 lines

+ * @cmd: the payload that will be part of the @msg
+ * @completion: triggered when request is done
+ * @wait_count: count of waiters for this completion
+ * @err: err return from the controller
+ */
+struct rpmh_msg {

struct rpmh_request ?

drivers/soc/qcom/rpmh.c line 60

Quoted 4 lines

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

+	struct tcs_mbox_msg msg;
+	struct tcs_cmd cmd[MAX_RPMH_PAYLOAD];
+	struct completion *completion;
+	atomic_t *wait_count;

When will @wait_count > 1? As far as I can see the only purpose would be to be able to control whether you should complete @completion 0 or N times; but 0 times is covered already by not specifying a @completion.

Quoted 7 lines

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

+	struct rpmh_client *rc;
+	int err;
+};
+
+/**
+ * rpmh_ctrlr: our representation of the controller
+ *

drivers/soc/qcom/rpmh.c line 71

Quoted 4 lines

+ * @drv: the controller instance
+ */

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

+struct rpmh_ctrlr {
+	struct rsc_drv *drv;

Is this going to grow in the future? Otherwise just drop it and reference the rsc_drv directly. (Even if it's growing it might be cleaner to introduce it at that point)

Quoted 11 lines

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

+};
+
+/**
+ * rpmh_client: the client object
+ *
+ * @dev: the platform device that is the owner
+ * @ctrlr: the controller associated with this client.
+ */
+struct rpmh_client {
+	struct device *dev;
+	struct rpmh_ctrlr *ctrlr;

drivers/soc/qcom/rpmh.c line 86

Quoted 4 lines

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

+};
+
+static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES];
+static DEFINE_MUTEX(rpmh_ctrlr_mutex);

The client needs a reference to the rsc_drv, using the rpmh_ctrlr abstraction and these global variables only seems to add unnecessary complexity.

Quoted 13 lines

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

+
+void rpmh_tx_done(struct tcs_mbox_msg *msg, int r)
+{
+	struct rpmh_msg *rpm_msg = container_of(msg, struct rpmh_msg, msg);
+	atomic_t *wc = rpm_msg->wait_count;
+	struct completion *compl = rpm_msg->completion;
+
+	rpm_msg->err = r;
+
+	if (r)
+		dev_err(rpm_msg->rc->dev,
+			"RPMH TX fail in msg addr 0x%x, err=%d\n",
+			rpm_msg->msg.payload[0].addr, r);