[PATCH RFC] WIP: mfd: add support for Qualcomm RPM

[PATCH RFC] WIP: mfd: add support for Qualcomm RPM

[Josh Cartwright]

The Resource Power Manager (RPM) is responsible managing SoC-wide
resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
This driver provides an implementation of the message-RAM-based
communication protocol.

Note, this is a rewrite of the driver as it exists in the downstream
tree[1], making a few simplifying assumptions to clean it up, and adding
device tree support.

[1]: https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/mach-msm/rpm.c?h=msm-3.4

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
This patch is intended to act as a starting point for discussions on how we
should proceed going forward supporting RPM.  In particular, figuring out how
to model RPM and it's controlled resources in device tree.

I've chosen a path where a subnode logically separates the RPM resources; it's
intended each set of resources will be controlled by a single driver.  For
example, an RPM-controlled regulator might consume two RPM_TYPE_REQ resources
described in 'reg'.

Effectively, this pushes the "generic resource ID" -> "SoC-specific resource
ID" mapping out of the large data tables that exist in msm-3.4 into the device
tree.  An alternative approach would be to still maintain the SoC-specific
tables, and have each node matched to it's resources using a unique compatible
string.

Any comments appreciated!

Thanks,
  Josh
 Documentation/devicetree/bindings/mfd/qcom,rpm.txt |  68 +++++
 drivers/mfd/Kconfig                                |   9 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/qcom-rpm.c                             | 314 +++++++++++++++++++++
 include/linux/mfd/qcom_rpm.h                       |  64 +++++
 5 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/qcom,rpm.txt
 create mode 100644 drivers/mfd/qcom-rpm.c
 create mode 100644 include/linux/mfd/qcom_rpm.h

diff --git a/Documentation/devicetree/bindings/mfd/qcom,rpm.txt b/Documentation/devicetree/bindings/mfd/qcom,rpm.txt
new file mode 100644
index 0000000..617018f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/qcom,rpm.txt
@@ -0,0 +1,68 @@
+Qualcomm Resource Power Manager (RPM)
+
+This driver is used to interface with Resource Power Manager (RPM).  The RPM is
+responsible managing SoC-wide resources (clocks, regulators, etc) on MSM and
+other Qualcomm chipsets.
+
+Required properties:
+
+- compatible: must be one of:
+	"qcom,rpm-apq8064"
+	"qcom,rpm-ipq8064"
+
+- reg: must contain two register specifiers, in the following order:
+	specifier 0: RPM Message RAM
+	specifier 1: IPC register
+
+- reg-names: must contain the following, in order:
+	"msg_ram"
+	"ipc"
+
+- interrupts: must contain the following three interrupt specifiers, in order:
+	specifier 0: RPM Acknowledgement Interrupt
+	specifier 1: Error Interrupt
+	specifier 2: Wakeup interrupt
+
+- interrupt-names: must contain the following, in order:
+	"ack"
+	"err"
+	"wakeup"
+
+- ipc-bit: bit written to the IPC register to notify RPM of a pending request
+
+- #address-cells: must be 3
+	cell 0: offset in ACK and REQ register spaces corresponding to the register
+	cell 1: type field, one of RPM_TYPE_REQ (0) or RPM_TYPE_STATUS (1)
+	cell 2: indicates the selector bit to set when writing this register,
+		this cell is ignored (and should be set to zero) when type is
+		RPM_TYPE_STATUS
+
+Example:
+
+	#include <dt-bindings/mfd/qcom_rpm.h>
+
+	rpm@108000 {
+		compatible = "qcom,rpm-ipq8064";
+		reg = <0x00108000 0x1000>,
+		      <0x02011008 0x4>;
+		reg-names = "msg_ram",
+			    "ipc";
+		interrupts = <GIC_SPI 19 0>,
+			     <GIC_SPI 21 0>,
+			     <GIC_SPI 22 0>;
+		interrupt-names = "ack",
+				  "err",
+				  "wakeup";
+		ipc-bit = <2>;
+
+		#address-cells = <3>;
+		#size-cells = <0>;
+
+		subnode {
+			compatible = "...";
+			reg = <464 RPM_TYPE_REQ 30>,
+			      <468 RPM_TYPE_REQ 30>,
+			      <118 RPM_TYPE_STATUS 0>;
+		};
+	};
+
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 49bb445..b387ba9 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -497,6 +497,15 @@ config MFD_PM8XXX_IRQ
 	  This is required to use certain other PM 8xxx features, such as GPIO
 	  and MPP.
 
+config MFD_QCOM_RPM
+	tristate "Qualcomm Resource Power Manager (RPM) driver"
+	depends on (ARCH_QCOM || COMPILE_TEST)
+	help
+	  The Resource Power Manager (RPM) is responsible managing SoC-wide
+	  resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
+	  This driver provides an implementation of the message-RAM-based
+	  communication protocol.
+
 config MFD_RDC321X
 	tristate "RDC R-321x southbridge"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5aea5ef..a51fe46 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
 obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
 obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
 obj-$(CONFIG_MFD_PM8XXX_IRQ) 	+= pm8xxx-irq.o
+obj-$(CONFIG_MFD_QCOM_RPM)	+= qcom-rpm.o
 obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
diff --git a/drivers/mfd/qcom-rpm.c b/drivers/mfd/qcom-rpm.c
new file mode 100644
index 0000000..ff33bc6
--- /dev/null
+++ b/drivers/mfd/qcom-rpm.c
@@ -0,0 +1,314 @@
+/* Copyright (c) 2010-2012,2014 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.
+ *
+ */
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/qcom_rpm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/mfd/qcom-rpm.h>
+
+#define RPM_SUPPORTED_VERS_MAJOR	3
+
+#define RPM_STATUS_VERSION_MAJOR	0
+
+#define RPM_CONTROL_VERSION_MAJOR	0x00
+#define RPM_CONTROL_VERSION_MINOR	0x04
+#define RPM_CONTROL_VERSION_BUILD	0x08
+#define RPM_CONTROL_REQ_CTX		0x0C
+#define RPM_CONTROL_REQ_SEL		0x2C
+#define RPM_CONTROL_ACK_CTX		0x3C
+#define RPM_CONTROL_ACK_SEL		0x5C
+
+struct qcom_rpm {
+	struct device *dev;
+	void __iomem *status;
+	void __iomem *ctrl;
+	void __iomem *req;
+	void __iomem *ack;
+	void __iomem *ipc_reg;
+	u32 ipc_val;
+	u32 *ctx_ack;
+	u32 (*sel_masks_ack)[5];
+	struct qcom_rpm_req *pending_req;
+	size_t num_req;
+	struct completion done;
+	struct mutex lock;
+};
+
+static void qcom_rpm_kick(struct qcom_rpm *rpm)
+{
+	writel(rpm->ipc_val, rpm->ipc_reg);
+}
+
+int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
+		       const struct qcom_rpm_req *req, u32 *data, size_t len)
+{
+	u32 __iomem *req_sel_reg = rpm->ctrl + RPM_CONTROL_REQ_SEL;
+	u32 sel_masks[5] = { }, sel_masks_ack[5];
+	u32 ctx_ack;
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		sel_masks[req->sel_reg] |= req->sel_mask;
+
+	mutex_lock(&rpm->lock);
+
+	rpm->ctx_ack = &ctx_ack;
+	rpm->sel_masks_ack = &sel_masks_ack;
+
+	for (i = 0; i < len; i++)
+		writel_relaxed(data[i], rpm->req + req[i].offset);
+
+	for (i = 0; i < ARRAY_SIZE(sel_masks); i++)
+		writel_relaxed(sel_masks[i], &req_sel_reg[i]);
+
+	writel_relaxed(ctx, rpm->ctrl + RPM_CONTROL_REQ_CTX);
+
+	qcom_rpm_kick(rpm);
+
+	wait_for_completion(&rpm->done);
+	reinit_completion(&rpm->done);
+
+	for (i = 0; i < rpm->num_req; i++)
+		data[i] = readl_relaxed(rpm->ack + rpm->pending_req[i].offset);
+
+	mutex_unlock(&rpm->lock);
+
+	if (ctx_ack & QCOM_RPM_CTX_REJECTED)
+		return -ENOSPC;
+
+	ctx_ack &= ~QCOM_RPM_CTX_REJECTED;
+	if (WARN_ON(ctx_ack != ctx)) {
+		dev_err(rpm->dev, "received bad context ack.\n");
+		return -EFAULT;
+	}
+
+	if (WARN_ON(memcmp(sel_masks, sel_masks_ack, sizeof(sel_masks)))) {
+		dev_err(rpm->dev,
+			"requested writes failed to be acknowledged.\n");
+		return -EFAULT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_rpm_write_ctx);
+
+static irqreturn_t qcom_rpm_ack_irq(int irq, void *devid)
+{
+	struct qcom_rpm *rpm = devid;
+	u32 __iomem *ack_sel_reg = rpm->ctrl + RPM_CONTROL_ACK_SEL;
+	unsigned int i;
+
+	*rpm->ctx_ack = readl_relaxed(rpm->ctrl + RPM_CONTROL_ACK_CTX);
+
+	for (i = 0; i < ARRAY_SIZE(*rpm->sel_masks_ack); i++) {
+		*rpm->sel_masks_ack[i] = readl_relaxed(&ack_sel_reg[i]);
+		writel_relaxed(0, &ack_sel_reg[i]);
+	}
+
+	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_ACK_CTX);
+
+	/* Ignore notifications for now */
+	if (*rpm->ctx_ack & QCOM_RPM_CTX_NOTIFICATION)
+		return IRQ_HANDLED;
+
+	complete(&rpm->done);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t qcom_rpm_err_irq(int irq, void *devid)
+{
+	struct qcom_rpm *rpm = devid;
+
+	WARN(1, "RPM triggered fatal error. RPM communication unreliable.");
+	writel_relaxed(1, rpm->ipc_reg);
+
+	return IRQ_HANDLED;
+}
+
+static const __be32 *qcom_decode_reg_type(struct platform_device *pdev,
+					  unsigned int which, unsigned int type)
+{
+	const struct device_node *np = pdev->dev.of_node;
+	const __be32 *cell;
+	int sz;
+
+	cell = of_get_property(np, "reg", &sz);
+	if (!cell)
+		return ERR_PTR(-EINVAL);
+
+	sz /= 3 * sizeof(u32);
+
+	for (; sz--; cell += 3) {
+
+		if (be32_to_cpup(&cell[1]) != type)
+			continue;
+
+		if (!which--)
+			return cell;
+
+	}
+
+	return ERR_PTR(-ENOENT);
+}
+
+int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
+		     struct qcom_rpm_req *req)
+{
+	const __be32 *cell;
+	u32 sel_bit;
+
+	cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_REQ);
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	req->offset = be32_to_cpup(&cell[0]);
+
+	sel_bit = be32_to_cpup(&cell[2]);
+
+	req->sel_reg  = sel_bit / 32;
+	req->sel_mask = BIT(sel_bit % 32);
+	return 0;
+}
+EXPORT_SYMBOL(qcom_rpm_get_req);
+
+const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
+					unsigned int which)
+{
+	struct qcom_rpm *rpm = qcom_rpm_get(pdev);
+	const __be32 *cell;
+
+	cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_STATUS);
+	if (IS_ERR(cell))
+		return (const void __iomem *) cell;
+
+	return rpm->status + be32_to_cpup(&cell[0]);
+}
+EXPORT_SYMBOL(qcom_rpm_get_status);
+
+static int qcom_rpm_check_version(struct qcom_rpm *rpm)
+{
+	u32 vers_major;
+
+	vers_major = readl_relaxed(rpm->status + RPM_STATUS_VERSION_MAJOR);
+
+	if (vers_major != RPM_SUPPORTED_VERS_MAJOR) {
+		dev_err(rpm->dev, "RPM driver does not support firmware with major version %d\n",
+			vers_major);
+		return -EINVAL;
+	}
+
+	writel_relaxed(vers_major, rpm->ctrl + RPM_CONTROL_VERSION_MAJOR);
+	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_VERSION_MINOR);
+	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_VERSION_BUILD);
+	return 0;
+}
+
+static int qcom_rpm_probe(struct platform_device *pdev)
+{
+	struct qcom_rpm *rpm;
+	struct resource *res;
+	int err, irq;
+	u32 bit;
+
+	rpm = devm_kzalloc(&pdev->dev, sizeof(*rpm), GFP_KERNEL);
+	if (!rpm)
+		return -ENOMEM;
+
+	rpm->dev = &pdev->dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "msg_ram");
+	rpm->status = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rpm->status))
+		return PTR_ERR(rpm->status);
+
+	rpm->ctrl = rpm->status + 0x400;
+	rpm->req  = rpm->status + 0x600;
+	rpm->ack  = rpm->status + 0xA00;
+
+	err = qcom_rpm_check_version(rpm);
+	if (err)
+		return err;
+
+	init_completion(&rpm->done);
+	mutex_init(&rpm->lock);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipc");
+	rpm->ipc_reg = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rpm->ipc_reg))
+		return PTR_ERR(rpm->ipc_reg);
+
+	err = of_property_read_u32(pdev->dev.of_node, "ipc-bit", &bit);
+	if (err) {
+		dev_err(&pdev->dev, "ipc-bit property unspecified.\n");
+		return -EINVAL;
+	}
+
+	if (bit > 31) {
+		dev_err(&pdev->dev, "invalid ipc-bit specified.\n");
+		return -EINVAL;
+	}
+
+	rpm->ipc_val = BIT(bit);
+
+	irq = platform_get_irq_byname(pdev, "ack");
+	if (irq < 0) {
+		dev_err(&pdev->dev, "invalid ack interrupt specified.\n");
+		return irq;
+	}
+
+	err = devm_request_irq(&pdev->dev, irq, qcom_rpm_ack_irq,
+			       IRQF_TRIGGER_RISING, "rpm_ack", rpm);
+	if (err) {
+		dev_err(&pdev->dev, "unable to request ack interrupt\n");
+		return err;
+	}
+
+	irq = platform_get_irq_byname(pdev, "err");
+	if (irq < 0) {
+		dev_err(&pdev->dev, "invalid err interrupt specified.\n");
+		return irq;
+	}
+
+	err = devm_request_irq(&pdev->dev, irq, qcom_rpm_err_irq,
+			       IRQF_TRIGGER_RISING, "rpm_err", rpm);
+	if (err) {
+		dev_err(&pdev->dev, "unable to request err interrupt\n");
+		return err;
+	}
+
+	platform_set_drvdata(pdev, rpm);
+
+	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id qcom_rpm_of_match[] = {
+	{ .compatible = "qcom,rpm-apq8064", },
+	{ .compatible = "qcom,rpm-ipq8064", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
+
+static struct platform_driver msm_rpm_platform_driver = {
+	.probe			= qcom_rpm_probe,
+	.driver = {
+		.name		= "qcom_rpm",
+		.of_match_table	= qcom_rpm_of_match,
+	},
+};
+module_platform_driver(msm_rpm_platform_driver);
diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
new file mode 100644
index 0000000..1f585bf
--- /dev/null
+++ b/include/linux/mfd/qcom_rpm.h
@@ -0,0 +1,64 @@
+/* Copyright (c) 2014 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.
+ *
+ */
+#ifndef QCOM_RPM_H
+#define QCOM_RPM_H
+
+#include <linux/platform_device.h>
+
+struct qcom_rpm;
+
+static inline struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev)
+{
+	struct platform_device *parent;
+
+	parent = to_platform_device(pdev->dev.parent);
+
+	return platform_get_drvdata(parent);
+}
+
+struct qcom_rpm_req {
+	unsigned int offset;
+	unsigned int sel_reg;
+	u32 sel_mask;
+};
+
+enum qcom_rpm_context_mask {
+	QCOM_RPM_CTX_SET_ACTIVE		= BIT(0),
+	QCOM_RPM_CTX_SET_SLEEP		= BIT(1),
+	QCOM_RPM_CTX_NOTIFICATION	= BIT(30),
+	QCOM_RPM_CTX_REJECTED		= BIT(31),
+};
+
+int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
+		       const struct qcom_rpm_req *req, u32 *data, size_t len);
+
+static inline int qcom_rpm_req_write(struct qcom_rpm *rpm,
+				     const struct qcom_rpm_req *req, u32 data)
+{
+	return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_ACTIVE, req, &data, 1);
+}
+
+static inline int qcom_rpm_req_write_sleep(struct qcom_rpm *rpm,
+					   const struct qcom_rpm_req *req,
+					   u32 data)
+{
+	return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_SLEEP, req, &data, 1);
+}
+
+int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
+		     struct qcom_rpm_req *req);
+
+const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
+					unsigned int which);
+
+#endif /* QCOM_RPM_H */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH RFC] WIP: mfd: add support for Qualcomm RPM

[Kumar Gala]

On Apr 10, 2014, at 5:17 PM, Josh Cartwright <joshc@codeaurora.org> wrote:

> The Resource Power Manager (RPM) is responsible managing SoC-wide
> resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
> This driver provides an implementation of the message-RAM-based
> communication protocol.
> 
> Note, this is a rewrite of the driver as it exists in the downstream
> tree[1], making a few simplifying assumptions to clean it up, and adding
> device tree support.
> 
> [1]: https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/mach-msm/rpm.c?h=msm-3.4
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
> This patch is intended to act as a starting point for discussions on how we
> should proceed going forward supporting RPM.  In particular, figuring out how
> to model RPM and it's controlled resources in device tree.
> 
> I've chosen a path where a subnode logically separates the RPM resources; it's
> intended each set of resources will be controlled by a single driver.  For
> example, an RPM-controlled regulator might consume two RPM_TYPE_REQ resources
> described in 'reg'.
> 
> Effectively, this pushes the "generic resource ID" -> "SoC-specific resource
> ID" mapping out of the large data tables that exist in msm-3.4 into the device
> tree.  An alternative approach would be to still maintain the SoC-specific
> tables, and have each node matched to it's resources using a unique compatible
> string.
> 
> Any comments appreciated!
> 
> Thanks,
>  Josh
> Documentation/devicetree/bindings/mfd/qcom,rpm.txt |  68 +++++
> drivers/mfd/Kconfig                                |   9 +
> drivers/mfd/Makefile                               |   1 +
> drivers/mfd/qcom-rpm.c                             | 314 +++++++++++++++++++++
> include/linux/mfd/qcom_rpm.h                       |  64 +++++
> 5 files changed, 456 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/qcom,rpm.txt
> create mode 100644 drivers/mfd/qcom-rpm.c
> create mode 100644 include/linux/mfd/qcom_rpm.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,rpm.txt b/Documentation/devicetree/bindings/mfd/qcom,rpm.txt
> new file mode 100644
> index 0000000..617018f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/qcom,rpm.txt
> @@ -0,0 +1,68 @@
> +Qualcomm Resource Power Manager (RPM)
> +
> +This driver is used to interface with Resource Power Manager (RPM).  The RPM is
> +responsible managing SoC-wide resources (clocks, regulators, etc) on MSM and
> +other Qualcomm chipsets.
> +
> +Required properties:
> +
> +- compatible: must be one of:
> +	"qcom,rpm-apq8064"
> +	"qcom,rpm-ipq8064"
> +
> +- reg: must contain two register specifiers, in the following order:
> +	specifier 0: RPM Message RAM
> +	specifier 1: IPC register
> +
> +- reg-names: must contain the following, in order:
> +	"msg_ram"
> +	"ipc"
> +
> +- interrupts: must contain the following three interrupt specifiers, in order:
> +	specifier 0: RPM Acknowledgement Interrupt
> +	specifier 1: Error Interrupt
> +	specifier 2: Wakeup interrupt
> +
> +- interrupt-names: must contain the following, in order:
> +	"ack"
> +	"err"
> +	"wakeup"
> +
> +- ipc-bit: bit written to the IPC register to notify RPM of a pending request

I assume this varies between apq8064 and ipq8064, I think it might be better just encoded in the .data field of the of_device_id table.

Probably need some description about the child/subnode you’ve got.

> +
> +- #address-cells: must be 3
> +	cell 0: offset in ACK and REQ register spaces corresponding to the register
> +	cell 1: type field, one of RPM_TYPE_REQ (0) or RPM_TYPE_STATUS (1)
> +	cell 2: indicates the selector bit to set when writing this register,
> +		this cell is ignored (and should be set to zero) when type is
> +		RPM_TYPE_STATUS
> +
> +Example:
> +
> +	#include <dt-bindings/mfd/qcom_rpm.h>
> +
> +	rpm@108000 {
> +		compatible = "qcom,rpm-ipq8064";
> +		reg = <0x00108000 0x1000>,
> +		      <0x02011008 0x4>;
> +		reg-names = "msg_ram",
> +			    "ipc";
> +		interrupts = <GIC_SPI 19 0>,
> +			     <GIC_SPI 21 0>,
> +			     <GIC_SPI 22 0>;
> +		interrupt-names = "ack",
> +				  "err",
> +				  "wakeup";
> +		ipc-bit = <2>;
> +
> +		#address-cells = <3>;
> +		#size-cells = <0>;
> +
> +		subnode {
> +			compatible = "...";
> +			reg = <464 RPM_TYPE_REQ 30>,
> +			      <468 RPM_TYPE_REQ 30>,
> +			      <118 RPM_TYPE_STATUS 0>;
> +		};
> +	};
> +
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 49bb445..b387ba9 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -497,6 +497,15 @@ config MFD_PM8XXX_IRQ
> 	  This is required to use certain other PM 8xxx features, such as GPIO
> 	  and MPP.
> 
> +config MFD_QCOM_RPM
> +	tristate "Qualcomm Resource Power Manager (RPM) driver"
> +	depends on (ARCH_QCOM || COMPILE_TEST)
> +	help
> +	  The Resource Power Manager (RPM) is responsible managing SoC-wide
> +	  resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
> +	  This driver provides an implementation of the message-RAM-based
> +	  communication protocol.
> +
> config MFD_RDC321X
> 	tristate "RDC R-321x southbridge"
> 	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5aea5ef..a51fe46 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o ssbi.o
> obj-$(CONFIG_MFD_PM8XXX_IRQ) 	+= pm8xxx-irq.o
> +obj-$(CONFIG_MFD_QCOM_RPM)	+= qcom-rpm.o
> obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
> obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
> obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
> diff --git a/drivers/mfd/qcom-rpm.c b/drivers/mfd/qcom-rpm.c
> new file mode 100644
> index 0000000..ff33bc6
> --- /dev/null
> +++ b/drivers/mfd/qcom-rpm.c
> @@ -0,0 +1,314 @@
> +/* Copyright (c) 2010-2012,2014 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.
> + *
> + */
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/qcom_rpm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/mfd/qcom-rpm.h>
> +
> +#define RPM_SUPPORTED_VERS_MAJOR	3
> +
> +#define RPM_STATUS_VERSION_MAJOR	0
> +
> +#define RPM_CONTROL_VERSION_MAJOR	0x00
> +#define RPM_CONTROL_VERSION_MINOR	0x04
> +#define RPM_CONTROL_VERSION_BUILD	0x08
> +#define RPM_CONTROL_REQ_CTX		0x0C
> +#define RPM_CONTROL_REQ_SEL		0x2C
> +#define RPM_CONTROL_ACK_CTX		0x3C
> +#define RPM_CONTROL_ACK_SEL		0x5C
> +
> +struct qcom_rpm {
> +	struct device *dev;
> +	void __iomem *status;
> +	void __iomem *ctrl;
> +	void __iomem *req;
> +	void __iomem *ack;
> +	void __iomem *ipc_reg;
> +	u32 ipc_val;
> +	u32 *ctx_ack;
> +	u32 (*sel_masks_ack)[5];
> +	struct qcom_rpm_req *pending_req;
> +	size_t num_req;
> +	struct completion done;
> +	struct mutex lock;
> +};
> +
> +static void qcom_rpm_kick(struct qcom_rpm *rpm)
> +{
> +	writel(rpm->ipc_val, rpm->ipc_reg);
> +}
> +
> +int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
> +		       const struct qcom_rpm_req *req, u32 *data, size_t len)
> +{
> +	u32 __iomem *req_sel_reg = rpm->ctrl + RPM_CONTROL_REQ_SEL;
> +	u32 sel_masks[5] = { }, sel_masks_ack[5];
> +	u32 ctx_ack;
> +	size_t i;
> +
> +	for (i = 0; i < len; i++)
> +		sel_masks[req->sel_reg] |= req->sel_mask;
> +
> +	mutex_lock(&rpm->lock);
> +
> +	rpm->ctx_ack = &ctx_ack;
> +	rpm->sel_masks_ack = &sel_masks_ack;
> +
> +	for (i = 0; i < len; i++)
> +		writel_relaxed(data[i], rpm->req + req[i].offset);
> +
> +	for (i = 0; i < ARRAY_SIZE(sel_masks); i++)
> +		writel_relaxed(sel_masks[i], &req_sel_reg[i]);
> +
> +	writel_relaxed(ctx, rpm->ctrl + RPM_CONTROL_REQ_CTX);
> +
> +	qcom_rpm_kick(rpm);
> +
> +	wait_for_completion(&rpm->done);
> +	reinit_completion(&rpm->done);
> +
> +	for (i = 0; i < rpm->num_req; i++)
> +		data[i] = readl_relaxed(rpm->ack + rpm->pending_req[i].offset);
> +
> +	mutex_unlock(&rpm->lock);
> +
> +	if (ctx_ack & QCOM_RPM_CTX_REJECTED)
> +		return -ENOSPC;
> +
> +	ctx_ack &= ~QCOM_RPM_CTX_REJECTED;
> +	if (WARN_ON(ctx_ack != ctx)) {
> +		dev_err(rpm->dev, "received bad context ack.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (WARN_ON(memcmp(sel_masks, sel_masks_ack, sizeof(sel_masks)))) {
> +		dev_err(rpm->dev,
> +			"requested writes failed to be acknowledged.\n");
> +		return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_rpm_write_ctx);
> +
> +static irqreturn_t qcom_rpm_ack_irq(int irq, void *devid)
> +{
> +	struct qcom_rpm *rpm = devid;
> +	u32 __iomem *ack_sel_reg = rpm->ctrl + RPM_CONTROL_ACK_SEL;
> +	unsigned int i;
> +
> +	*rpm->ctx_ack = readl_relaxed(rpm->ctrl + RPM_CONTROL_ACK_CTX);
> +
> +	for (i = 0; i < ARRAY_SIZE(*rpm->sel_masks_ack); i++) {
> +		*rpm->sel_masks_ack[i] = readl_relaxed(&ack_sel_reg[i]);
> +		writel_relaxed(0, &ack_sel_reg[i]);
> +	}
> +
> +	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_ACK_CTX);
> +
> +	/* Ignore notifications for now */
> +	if (*rpm->ctx_ack & QCOM_RPM_CTX_NOTIFICATION)
> +		return IRQ_HANDLED;
> +
> +	complete(&rpm->done);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_rpm_err_irq(int irq, void *devid)
> +{
> +	struct qcom_rpm *rpm = devid;
> +
> +	WARN(1, "RPM triggered fatal error. RPM communication unreliable.");
> +	writel_relaxed(1, rpm->ipc_reg);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const __be32 *qcom_decode_reg_type(struct platform_device *pdev,
> +					  unsigned int which, unsigned int type)
> +{
> +	const struct device_node *np = pdev->dev.of_node;
> +	const __be32 *cell;
> +	int sz;
> +
> +	cell = of_get_property(np, "reg", &sz);
> +	if (!cell)
> +		return ERR_PTR(-EINVAL);
> +
> +	sz /= 3 * sizeof(u32);
> +
> +	for (; sz--; cell += 3) {
> +
> +		if (be32_to_cpup(&cell[1]) != type)
> +			continue;
> +
> +		if (!which--)
> +			return cell;
> +
> +	}
> +
> +	return ERR_PTR(-ENOENT);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +		     struct qcom_rpm_req *req)
> +{
> +	const __be32 *cell;
> +	u32 sel_bit;
> +
> +	cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_REQ);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	req->offset = be32_to_cpup(&cell[0]);
> +
> +	sel_bit = be32_to_cpup(&cell[2]);
> +
> +	req->sel_reg  = sel_bit / 32;
> +	req->sel_mask = BIT(sel_bit % 32);
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_req);
> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +					unsigned int which)
> +{
> +	struct qcom_rpm *rpm = qcom_rpm_get(pdev);
> +	const __be32 *cell;
> +
> +	cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_STATUS);
> +	if (IS_ERR(cell))
> +		return (const void __iomem *) cell;
> +
> +	return rpm->status + be32_to_cpup(&cell[0]);
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_status);
> +
> +static int qcom_rpm_check_version(struct qcom_rpm *rpm)
> +{
> +	u32 vers_major;
> +
> +	vers_major = readl_relaxed(rpm->status + RPM_STATUS_VERSION_MAJOR);
> +
> +	if (vers_major != RPM_SUPPORTED_VERS_MAJOR) {
> +		dev_err(rpm->dev, "RPM driver does not support firmware with major version %d\n",
> +			vers_major);
> +		return -EINVAL;
> +	}
> +
> +	writel_relaxed(vers_major, rpm->ctrl + RPM_CONTROL_VERSION_MAJOR);
> +	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_VERSION_MINOR);
> +	writel_relaxed(0, rpm->ctrl + RPM_CONTROL_VERSION_BUILD);
> +	return 0;
> +}
> +
> +static int qcom_rpm_probe(struct platform_device *pdev)
> +{
> +	struct qcom_rpm *rpm;
> +	struct resource *res;
> +	int err, irq;
> +	u32 bit;
> +
> +	rpm = devm_kzalloc(&pdev->dev, sizeof(*rpm), GFP_KERNEL);
> +	if (!rpm)
> +		return -ENOMEM;
> +
> +	rpm->dev = &pdev->dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "msg_ram");
> +	rpm->status = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rpm->status))
> +		return PTR_ERR(rpm->status);
> +
> +	rpm->ctrl = rpm->status + 0x400;
> +	rpm->req  = rpm->status + 0x600;
> +	rpm->ack  = rpm->status + 0xA00;
> +
> +	err = qcom_rpm_check_version(rpm);
> +	if (err)
> +		return err;
> +
> +	init_completion(&rpm->done);
> +	mutex_init(&rpm->lock);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipc");
> +	rpm->ipc_reg = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rpm->ipc_reg))
> +		return PTR_ERR(rpm->ipc_reg);
> +
> +	err = of_property_read_u32(pdev->dev.of_node, "ipc-bit", &bit);
> +	if (err) {
> +		dev_err(&pdev->dev, "ipc-bit property unspecified.\n");
> +		return -EINVAL;
> +	}
> +
> +	if (bit > 31) {
> +		dev_err(&pdev->dev, "invalid ipc-bit specified.\n");
> +		return -EINVAL;
> +	}
> +
> +	rpm->ipc_val = BIT(bit);
> +
> +	irq = platform_get_irq_byname(pdev, "ack");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "invalid ack interrupt specified.\n");
> +		return irq;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, irq, qcom_rpm_ack_irq,
> +			       IRQF_TRIGGER_RISING, "rpm_ack", rpm);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to request ack interrupt\n");
> +		return err;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "err");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "invalid err interrupt specified.\n");
> +		return irq;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, irq, qcom_rpm_err_irq,
> +			       IRQF_TRIGGER_RISING, "rpm_err", rpm);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to request err interrupt\n");
> +		return err;
> +	}
> +
> +	platform_set_drvdata(pdev, rpm);
> +
> +	return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +}
> +
> +static const struct of_device_id qcom_rpm_of_match[] = {
> +	{ .compatible = "qcom,rpm-apq8064", },
> +	{ .compatible = "qcom,rpm-ipq8064", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_rpm_of_match);
> +
> +static struct platform_driver msm_rpm_platform_driver = {
> +	.probe			= qcom_rpm_probe,
> +	.driver = {
> +		.name		= "qcom_rpm",
> +		.of_match_table	= qcom_rpm_of_match,
> +	},
> +};
> +module_platform_driver(msm_rpm_platform_driver);
> diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h
> new file mode 100644
> index 0000000..1f585bf
> --- /dev/null
> +++ b/include/linux/mfd/qcom_rpm.h
> @@ -0,0 +1,64 @@
> +/* Copyright (c) 2014 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.
> + *
> + */
> +#ifndef QCOM_RPM_H
> +#define QCOM_RPM_H
> +
> +#include <linux/platform_device.h>
> +
> +struct qcom_rpm;
> +
> +static inline struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev)
> +{
> +	struct platform_device *parent;
> +
> +	parent = to_platform_device(pdev->dev.parent);
> +
> +	return platform_get_drvdata(parent);
> +}
> +
> +struct qcom_rpm_req {
> +	unsigned int offset;
> +	unsigned int sel_reg;
> +	u32 sel_mask;
> +};
> +
> +enum qcom_rpm_context_mask {
> +	QCOM_RPM_CTX_SET_ACTIVE		= BIT(0),
> +	QCOM_RPM_CTX_SET_SLEEP		= BIT(1),
> +	QCOM_RPM_CTX_NOTIFICATION	= BIT(30),
> +	QCOM_RPM_CTX_REJECTED		= BIT(31),
> +};
> +
> +int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
> +		       const struct qcom_rpm_req *req, u32 *data, size_t len);
> +
> +static inline int qcom_rpm_req_write(struct qcom_rpm *rpm,
> +				     const struct qcom_rpm_req *req, u32 data)
> +{
> +	return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_ACTIVE, req, &data, 1);
> +}
> +
> +static inline int qcom_rpm_req_write_sleep(struct qcom_rpm *rpm,
> +					   const struct qcom_rpm_req *req,
> +					   u32 data)
> +{
> +	return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_SLEEP, req, &data, 1);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +		     struct qcom_rpm_req *req);
> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +					unsigned int which);
> +
> +#endif /* QCOM_RPM_H */
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH RFC] WIP: mfd: add support for Qualcomm RPM

[Bjorn Andersson]

On Thu 10 Apr 15:17 PDT 2014, Josh Cartwright wrote:

Hi Josh,

Thanks for posting this RFC.

> The Resource Power Manager (RPM) is responsible managing SoC-wide
> resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
> This driver provides an implementation of the message-RAM-based
> communication protocol.
> 
> Note, this is a rewrite of the driver as it exists in the downstream
> tree[1], making a few simplifying assumptions to clean it up, and adding
> device tree support.
> 
> [1]: https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/mach-msm/rpm.c?h=msm-3.4
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
> This patch is intended to act as a starting point for discussions on how we
> should proceed going forward supporting RPM.  In particular, figuring out how
> to model RPM and it's controlled resources in device tree.

As you know I wrote up a simple driver for this to get regulators a few months
back, so I'm happy to see this discussion.

> 
> I've chosen a path where a subnode logically separates the RPM resources; it's
> intended each set of resources will be controlled by a single driver.  For
> example, an RPM-controlled regulator might consume two RPM_TYPE_REQ resources
> described in 'reg'.

I agree, having each subnode be a 1:1 for a resource exposed by the rpm driver
seems to be the most logical idea. I did an experiment where I made my rpm
driver a mfd with regulator cells, but fell back to just having generic
platform_drivers like you do.


>From the RPMs perspective each logical resource is represented by the following
properties:
1) A consecutive range of addresses used for updating a value (what you call
   REQ)
2) An offset in a bitmap used to indicate what resources are set by the urrent
   write
3) A consecutive range of addresses used for reading the status of a resource
   (what you call STATUS)

For the individual clock client we have:
  4 bytes of requested clock rate

For the individual regulator client we have:
  4 or 8 bytes, depending on regulator type, of a packed struct containing the
  various properties of the regulator.

For msm_bus the data is represented by 29 words, I'm not sure if there's some
additional logic so that not all of these needs to be updated at once, there is
still just one selector bit.

At least the clock and regulators are only ever interested in operating on
entire logical resources. It's arguably never correct to write only 4 bytes for
a ldo update.


The API must therefor describe the entirety of a logical resource, either via
passing a list of req's or by abstracting the rpm details out and simply
exposing logical resources.

> 
> Effectively, this pushes the "generic resource ID" -> "SoC-specific resource
> ID" mapping out of the large data tables that exist in msm-3.4 into the device
> tree.  An alternative approach would be to still maintain the SoC-specific
> tables, and have each node matched to it's resources using a unique compatible
> string.

In my solution I did choose to expose logical resources, that simply maps to a
lookup table where I store offset for writes and reds, the selector bit and the
resource's size.

The rpm doesn't care about the internal representation of the clients data and
the clients doesn't care about the internal representation of the rpm's data.

[snip]
> +Qualcomm Resource Power Manager (RPM)
> +
> +This driver is used to interface with Resource Power Manager (RPM).  The RPM is
> +responsible managing SoC-wide resources (clocks, regulators, etc) on MSM and
> +other Qualcomm chipsets.
> +
> +Required properties:
> +
> +- compatible: must be one of:
> +       "qcom,rpm-apq8064"
> +       "qcom,rpm-ipq8064"
> +
> +- reg: must contain two register specifiers, in the following order:
> +       specifier 0: RPM Message RAM
> +       specifier 1: IPC register
> +
> +- reg-names: must contain the following, in order:
> +       "msg_ram"
> +       "ipc"
> +
> +- interrupts: must contain the following three interrupt specifiers, in order:
> +       specifier 0: RPM Acknowledgement Interrupt
> +       specifier 1: Error Interrupt
> +       specifier 2: Wakeup interrupt
> +
> +- interrupt-names: must contain the following, in order:
> +       "ack"
> +       "err"
> +       "wakeup"
> +
> +- ipc-bit: bit written to the IPC register to notify RPM of a pending request
> +
> +- #address-cells: must be 3
> +       cell 0: offset in ACK and REQ register spaces corresponding to the register
> +       cell 1: type field, one of RPM_TYPE_REQ (0) or RPM_TYPE_STATUS (1)
> +       cell 2: indicates the selector bit to set when writing this register,
> +               this cell is ignored (and should be set to zero) when type is
> +               RPM_TYPE_STATUS

As you've showed below, this requires a common parser-function to be feasable.
The natural place is to put this in the rpm implementation, but I found it
strange to have a function in the rpm driver, taking a pointer to a child node
and operating on that.

> +
> +Example:
> +
> +       #include <dt-bindings/mfd/qcom_rpm.h>
> +
> +       rpm@108000 {
> +               compatible = "qcom,rpm-ipq8064";
> +               reg = <0x00108000 0x1000>,
> +                     <0x02011008 0x4>;
> +               reg-names = "msg_ram",
> +                           "ipc";
> +               interrupts = <GIC_SPI 19 0>,
> +                            <GIC_SPI 21 0>,
> +                            <GIC_SPI 22 0>;
> +               interrupt-names = "ack",
> +                                 "err",
> +                                 "wakeup";
> +               ipc-bit = <2>;
> +
> +               #address-cells = <3>;
> +               #size-cells = <0>;

This looks good.

> +
> +               subnode {
> +                       compatible = "...";
> +                       reg = <464 RPM_TYPE_REQ 30>,
> +                             <468 RPM_TYPE_REQ 30>,
> +                             <118 RPM_TYPE_STATUS 0>;
> +               };
> +       };

It could be argued that these 3 values (base + size) is representing how things
are connected and therefor should be represented in DT. But I believe this is a
leaking abstraction and that this should be considered internal properties of
the mfd.

With a mapping table in the rpm the clients would be exposed to a simple u32
defining the resource to be accessed, this removes the need for any special
parsing function:

               pm8921_ldo16 {
                       compatible = "qcom,rpm-pm8921-ldo"
                       reg = <PM8921_LDO16>;

		       regulator-xxx = <yyy>;
               };


[snip]
> +
> +static const __be32 *qcom_decode_reg_type(struct platform_device *pdev,
> +                                         unsigned int which, unsigned int type)
> +{
> +       const struct device_node *np = pdev->dev.of_node;
> +       const __be32 *cell;
> +       int sz;
> +
> +       cell = of_get_property(np, "reg", &sz);
> +       if (!cell)
> +               return ERR_PTR(-EINVAL);
> +
> +       sz /= 3 * sizeof(u32);
> +
> +       for (; sz--; cell += 3) {
> +
> +               if (be32_to_cpup(&cell[1]) != type)
> +                       continue;
> +
> +               if (!which--)
> +                       return cell;
> +
> +       }
> +
> +       return ERR_PTR(-ENOENT);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +                    struct qcom_rpm_req *req)
> +{
> +       const __be32 *cell;
> +       u32 sel_bit;
> +
> +       cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_REQ);
> +       if (IS_ERR(cell))
> +               return PTR_ERR(cell);
> +
> +       req->offset = be32_to_cpup(&cell[0]);
> +
> +       sel_bit = be32_to_cpup(&cell[2]);
> +
> +       req->sel_reg  = sel_bit / 32;
> +       req->sel_mask = BIT(sel_bit % 32);
> +       return 0;
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_req);

To me it's not clean to have a function in the middle of the rpm driver that's
supposed to operate on the clients pdev.

> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +                                       unsigned int which)
> +{
> +       struct qcom_rpm *rpm = qcom_rpm_get(pdev);
> +       const __be32 *cell;
> +
> +       cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_STATUS);
> +       if (IS_ERR(cell))
> +               return (const void __iomem *) cell;
> +
> +       return rpm->status + be32_to_cpup(&cell[0]);
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_status);

I think you want to expose the current status, not just return a pointer that
the clients can deference at unknown times in the future.

I also find it inconsistent that the write function takes a reference to the
rpm device while the read function takes a reference to the client.

[snip]
> +#ifndef QCOM_RPM_H
> +#define QCOM_RPM_H
> +
> +#include <linux/platform_device.h>
> +
> +struct qcom_rpm;
> +
> +static inline struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev)
> +{
> +       struct platform_device *parent;
> +
> +       parent = to_platform_device(pdev->dev.parent);
> +
> +       return platform_get_drvdata(parent);
> +}

Most drivers would hide this in the api, but I think this looks good. The
clients would use this to acquire a qcom_rpm pointer in their probe function,
that they later can used when calling the api.

> +
> +struct qcom_rpm_req {
> +       unsigned int offset;
> +       unsigned int sel_reg;
> +       u32 sel_mask;
> +};

In my view, this is internal matters of the rpm.

> +
> +enum qcom_rpm_context_mask {
> +       QCOM_RPM_CTX_SET_ACTIVE         = BIT(0),
> +       QCOM_RPM_CTX_SET_SLEEP          = BIT(1),
> +       QCOM_RPM_CTX_NOTIFICATION       = BIT(30),
> +       QCOM_RPM_CTX_REJECTED           = BIT(31),

These defines does not belong to the public interface.

> +};
> +
> +int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
> +                      const struct qcom_rpm_req *req, u32 *data, size_t len);
> +
> +static inline int qcom_rpm_req_write(struct qcom_rpm *rpm,
> +                                    const struct qcom_rpm_req *req, u32 data)
> +{
> +       return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_ACTIVE, req, &data, 1);

Most resources does not have size 1; e.g. all ldos are size 2 (words) and
should be updated atomically.

For it to make sense to have the child list all req's this api must take a list
of reqs or the api should be that the client pass some base and then a size.
But as you don't have a mapping table or anything to compare this with it's not
possible to detect invalid writes.

> +}
> +
> +static inline int qcom_rpm_req_write_sleep(struct qcom_rpm *rpm,
> +                                          const struct qcom_rpm_req *req,
> +                                          u32 data)
> +{
> +       return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_SLEEP, req, &data, 1);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +                    struct qcom_rpm_req *req);
> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +                                       unsigned int which);

I would like you to simplify this api to be:

/* pdev is the childs pdev */
struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev);

int qcom_rpm_read(struct qcom_rpm *rpm, resource, void *data, size_t len);
int qcom_rpm_write(struct qcom_rpm *rpm, resource, void *data, size_t len);
int qcom_rpm_write_sleep(struct qcom_rpm *rpm, resource, void *data, size_t len);

The common write function is not supposed to be used by the clients, so this
would be moved into rpm.c


This api is creating an abstraction between what is rpm and what is client
data. Furthermore it holds for the SMD based rpm as well, that has the same
representation for at least clocks. So we have a potential for some re-use.

> +
> +#endif /* QCOM_RPM_H */
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

I'll post my code to the mailing list as well, for reference.

Regards,
Bjorn