All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND PATCH v1 0/2] Add changes to support SPMI resource protection
@ 2022-10-25  5:06 Fenglin Wu
  2022-10-25  5:06 ` [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr Fenglin Wu
  2022-10-25  5:06 ` [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional Fenglin Wu
  0 siblings, 2 replies; 5+ messages in thread
From: Fenglin Wu @ 2022-10-25  5:06 UTC (permalink / raw)
  To: linux-arm-msm, linux-kernel, sboyd
  Cc: quic_collinsd, quic_subbaram, quic_fenglinw

If a secure VM uses the SPMI PMIC arbiter driver to access PMIC modules
with secure access, such as in a trust UI feature when the secure VM is
accessing PMIC modules that supply to display power rails, the display
driver in primary VM (no-secure) needs to translate the SPMI address of
the PMIC modules and get the corresponding physical SoC register range
within the SPMI PMIC arbiter that is used to initiate SPMI write transactions,
and lend the memory range to the secure VM via a hypervisor call to prevent
any SPMI access to these modules from the non-secure VM. Hence, an API for
such SPMI address translation is added and exported.

Further, the secure-VM that loads the SPMI PMIC arbiter driver can't specify
the PMIC arbiter HLOS EE summary IRQ becuase it can't have the permission,
also the secure VM has no needs to use the PMIC modules interrupt, hence add
a change to make the interrupt support optional for the secure-VM to specify
the PMIC arbiter device node without interrupt support. The driver change has
a binding document change which has already been applied:
https://lore.kernel.org/all/YmxnIQ9niVbyASfN@robh.at.kernel.org/

David Collins (2):
  spmi: pmic-arb: add support to map SPMI addresses to physical addr
  spmi: pmic-arb: make interrupt support optional

 drivers/spmi/spmi-pmic-arb.c           | 149 ++++++++++++++++++++++---
 include/linux/soc/qcom/spmi-pmic-arb.h |  23 ++++
 2 files changed, 155 insertions(+), 17 deletions(-)
 create mode 100644 include/linux/soc/qcom/spmi-pmic-arb.h

-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr
  2022-10-25  5:06 [RESEND PATCH v1 0/2] Add changes to support SPMI resource protection Fenglin Wu
@ 2022-10-25  5:06 ` Fenglin Wu
  2022-10-25  6:48   ` Dmitry Baryshkov
  2022-10-25  5:06 ` [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional Fenglin Wu
  1 sibling, 1 reply; 5+ messages in thread
From: Fenglin Wu @ 2022-10-25  5:06 UTC (permalink / raw)
  To: linux-arm-msm, linux-kernel, sboyd, Andy Gross, Bjorn Andersson,
	Konrad Dybcio
  Cc: quic_collinsd, quic_subbaram, quic_fenglinw, David Collins

From: David Collins <collinsd@codeaurora.org>

Add an exported function which can map an SPMI address to the
physical address range of the registers used to write to that
SPMI address.

The feature can be used by consumer drivers that need to lock
down access to certain PMIC peripherals temporarily for certain
secure use cases.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c           | 104 +++++++++++++++++++++++++
 include/linux/soc/qcom/spmi-pmic-arb.h |  23 ++++++
 2 files changed, 127 insertions(+)
 create mode 100644 include/linux/soc/qcom/spmi-pmic-arb.h

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 2cf3203b2397..1e7f5a9ff4bc 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spmi.h>
+#include <linux/soc/qcom/spmi-pmic-arb.h>
 
 /* PMIC Arbiter configuration registers */
 #define PMIC_ARB_VERSION		0x0000
@@ -124,6 +125,8 @@ struct apid_data {
  *
  * @rd_base:		on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:		on v1 "core", on v2 "chnls"    register base off DT.
+ * @wr_base_phys:	base physical address of the register range used for
+ * 			SPMI write commands.
  * @intr:		address of the SPMI interrupt control registers.
  * @cnfg:		address of the PMIC Arbiter configuration registers.
  * @lock:		lock to synchronize accesses.
@@ -141,6 +144,7 @@ struct apid_data {
 struct spmi_pmic_arb {
 	void __iomem		*rd_base;
 	void __iomem		*wr_base;
+	phys_addr_t		wr_base_phys;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
 	void __iomem		*core;
@@ -180,6 +184,9 @@ struct spmi_pmic_arb {
  * @irq_clear:		on v1 address of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
  *			on v2 address of SPMI_PIC_IRQ_CLEARn.
  * @apid_map_offset:	offset of PMIC_ARB_REG_CHNLn
+ * @wr_addr_map:	maps from an SPMI address to the physical address
+ * 			range of the registers used to perform an SPMI write
+ * 			command to the SPMI address.
  */
 struct pmic_arb_ver_ops {
 	const char *ver_str;
@@ -196,6 +203,8 @@ struct pmic_arb_ver_ops {
 	void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
 	void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
 	u32 (*apid_map_offset)(u16 n);
+	int (*wr_addr_map)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+			   struct resource *res_out);
 };
 
 static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,
@@ -972,6 +981,21 @@ static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 	return 0x800 + 0x80 * pmic_arb->channel;
 }
 
+static int pmic_arb_wr_addr_map_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
+				   u16 addr, struct resource *res_out)
+{
+	int rc;
+
+	rc = pmic_arb_offset_v1(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
+	if (rc < 0)
+		return rc;
+
+	res_out->start = pmic_arb->wr_base_phys + rc;
+	res_out->end = res_out->start + 0x80 - 1;
+
+	return 0;
+}
+
 static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 {
 	struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
@@ -1111,6 +1135,21 @@ static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 	return 0x1000 * pmic_arb->ee + 0x8000 * apid;
 }
 
+static int pmic_arb_wr_addr_map_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
+				   u16 addr, struct resource *res_out)
+{
+	int rc;
+
+	rc = pmic_arb_offset_v2(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
+	if (rc < 0)
+		return rc;
+
+	res_out->start = pmic_arb->wr_base_phys + rc;
+	res_out->end = res_out->start + 0x1000 - 1;
+
+	return 0;
+}
+
 /*
  * v5 offset per ee and per apid for observer channels and per apid for
  * read/write channels.
@@ -1145,6 +1184,21 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 	return offset;
 }
 
+static int pmic_arb_wr_addr_map_v5(struct spmi_pmic_arb *pmic_arb, u8 sid,
+				   u16 addr, struct resource *res_out)
+{
+	int rc;
+
+	rc = pmic_arb_offset_v5(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
+	if (rc < 0)
+		return rc;
+
+	res_out->start = pmic_arb->wr_base_phys + rc;
+	res_out->end = res_out->start + 0x10000 - 1;
+
+	return 0;
+}
+
 static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
 {
 	return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
@@ -1254,6 +1308,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
 	.irq_status		= pmic_arb_irq_status_v1,
 	.irq_clear		= pmic_arb_irq_clear_v1,
 	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
+	.wr_addr_map		= pmic_arb_wr_addr_map_v1,
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v2 = {
@@ -1267,6 +1322,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
 	.irq_status		= pmic_arb_irq_status_v2,
 	.irq_clear		= pmic_arb_irq_clear_v2,
 	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
+	.wr_addr_map		= pmic_arb_wr_addr_map_v2,
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v3 = {
@@ -1280,6 +1336,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
 	.irq_status		= pmic_arb_irq_status_v2,
 	.irq_clear		= pmic_arb_irq_clear_v2,
 	.apid_map_offset	= pmic_arb_apid_map_offset_v2,
+	.wr_addr_map		= pmic_arb_wr_addr_map_v2,
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v5 = {
@@ -1293,6 +1350,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
 	.irq_status		= pmic_arb_irq_status_v5,
 	.irq_clear		= pmic_arb_irq_clear_v5,
 	.apid_map_offset	= pmic_arb_apid_map_offset_v5,
+	.wr_addr_map		= pmic_arb_wr_addr_map_v5,
 };
 
 static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
@@ -1302,6 +1360,50 @@ static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
 	.translate = qpnpint_irq_domain_translate,
 };
 
+static int _spmi_pmic_arb_map_address(struct spmi_pmic_arb *pmic_arb,
+				u32 spmi_address, struct resource *res_out)
+{
+	u32 sid, addr;
+
+	sid = (spmi_address >> 16) & 0xF;
+	addr = spmi_address & 0xFFFF;
+
+	return pmic_arb->ver_ops->wr_addr_map(pmic_arb, sid, addr, res_out);
+}
+
+/**
+ * spmi_pmic_arb_map_address() - returns physical addresses of registers used to
+ *		write to the PMIC peripheral at spmi_address
+ * @ctrl:		SPMI controller device pointer
+ * @spmi_address:	20-bit SPMI address of the form: 0xSPPPP
+ *			where S = global PMIC SID and
+ *			PPPP = SPMI address within the slave
+ * @res_out:		Resource struct (allocated by the caller) in which
+ *			physical addresses for the range are passed via start
+ *			and end elements
+ *
+ * Returns: 0 on success or an errno on failure.
+ */
+int spmi_pmic_arb_map_address(struct spmi_controller *ctrl, u32 spmi_address,
+			      struct resource *res_out)
+{
+	struct spmi_pmic_arb *pmic_arb;
+
+	if (!ctrl || !res_out) {
+		pr_err("%s: Invalid pointer\n", __func__);
+		return -EINVAL;
+	}
+
+	pmic_arb = spmi_controller_get_drvdata(ctrl);
+	if (!pmic_arb) {
+		pr_err("Missing PMIC arbiter device\n");
+		return -ENODEV;
+	}
+
+	return _spmi_pmic_arb_map_address(pmic_arb, spmi_address, res_out);
+}
+EXPORT_SYMBOL(spmi_pmic_arb_map_address);
+
 static int spmi_pmic_arb_probe(struct platform_device *pdev)
 {
 	struct spmi_pmic_arb *pmic_arb;
@@ -1341,6 +1443,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	if (hw_ver < PMIC_ARB_VERSION_V2_MIN) {
 		pmic_arb->ver_ops = &pmic_arb_v1;
 		pmic_arb->wr_base = core;
+		pmic_arb->wr_base_phys = res->start;
 		pmic_arb->rd_base = core;
 	} else {
 		pmic_arb->core = core;
@@ -1367,6 +1470,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 			err = PTR_ERR(pmic_arb->wr_base);
 			goto err_put_ctrl;
 		}
+		pmic_arb->wr_base_phys = res->start;
 	}
 
 	dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
diff --git a/include/linux/soc/qcom/spmi-pmic-arb.h b/include/linux/soc/qcom/spmi-pmic-arb.h
new file mode 100644
index 000000000000..adb999d6e246
--- /dev/null
+++ b/include/linux/soc/qcom/spmi-pmic-arb.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
+
+#ifndef _SPMI_PMIC_ARB_H
+#define _SPMI_PMIC_ARB_H
+
+#include <linux/spmi.h>
+#include <linux/ioport.h>
+#include <linux/types.h>
+
+
+#if IS_ENABLED(CONFIG_SPMI_MSM_PMIC_ARB)
+int spmi_pmic_arb_map_address(struct spmi_controller *ctrl, u32 spmi_address,
+			      struct resource *res_out);
+#else
+static inline int spmi_pmic_arb_map_address(struct spmi_controller *ctrl,
+				u32 spmi_address, struct resource *res_out)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional
  2022-10-25  5:06 [RESEND PATCH v1 0/2] Add changes to support SPMI resource protection Fenglin Wu
  2022-10-25  5:06 ` [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr Fenglin Wu
@ 2022-10-25  5:06 ` Fenglin Wu
  2022-10-25  6:25   ` Dmitry Baryshkov
  1 sibling, 1 reply; 5+ messages in thread
From: Fenglin Wu @ 2022-10-25  5:06 UTC (permalink / raw)
  To: linux-arm-msm, linux-kernel, sboyd
  Cc: quic_collinsd, quic_subbaram, quic_fenglinw, David Collins

From: David Collins <collinsd@codeaurora.org>

Make the support of PMIC peripheral interrupts optional for
spmi-pmic-arb devices.  This is useful in situations where
SPMI address mapping is required without the need for IRQ
support.

Signed-off-by: David Collins <collinsd@codeaurora.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
 drivers/spmi/spmi-pmic-arb.c | 45 ++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 1e7f5a9ff4bc..c14cffd8a313 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -1490,10 +1490,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
-	pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
-	if (pmic_arb->irq < 0) {
-		err = pmic_arb->irq;
-		goto err_put_ctrl;
+	if (of_find_property(pdev->dev.of_node, "interrupt-controller", NULL)) {
+		pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
+		if (pmic_arb->irq < 0) {
+			err = pmic_arb->irq;
+			goto err_put_ctrl;
+		}
 	}
 
 	err = of_property_read_u32(pdev->dev.of_node, "qcom,channel", &channel);
@@ -1553,17 +1555,22 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		}
 	}
 
-	dev_dbg(&pdev->dev, "adding irq domain\n");
-	pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
-					 &pmic_arb_irq_domain_ops, pmic_arb);
-	if (!pmic_arb->domain) {
-		dev_err(&pdev->dev, "unable to create irq_domain\n");
-		err = -ENOMEM;
-		goto err_put_ctrl;
+	if (pmic_arb->irq > 0) {
+		dev_dbg(&pdev->dev, "adding irq domain\n");
+		pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
+					    &pmic_arb_irq_domain_ops, pmic_arb);
+		if (!pmic_arb->domain) {
+			dev_err(&pdev->dev, "unable to create irq_domain\n");
+			err = -ENOMEM;
+			goto err_put_ctrl;
+		}
+
+		irq_set_chained_handler_and_data(pmic_arb->irq,
+						pmic_arb_chained_irq, pmic_arb);
+	} else {
+		dev_dbg(&pdev->dev, "not supporting PMIC interrupts\n");
 	}
 
-	irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
-					pmic_arb);
 	err = spmi_controller_add(ctrl);
 	if (err)
 		goto err_domain_remove;
@@ -1571,8 +1578,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	return 0;
 
 err_domain_remove:
-	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
-	irq_domain_remove(pmic_arb->domain);
+	if (pmic_arb->irq > 0) {
+		irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+		irq_domain_remove(pmic_arb->domain);
+	}
 err_put_ctrl:
 	spmi_controller_put(ctrl);
 	return err;
@@ -1583,8 +1592,10 @@ static int spmi_pmic_arb_remove(struct platform_device *pdev)
 	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
 	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	spmi_controller_remove(ctrl);
-	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
-	irq_domain_remove(pmic_arb->domain);
+	if (pmic_arb->irq > 0) {
+		irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+		irq_domain_remove(pmic_arb->domain);
+	}
 	spmi_controller_put(ctrl);
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional
  2022-10-25  5:06 ` [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional Fenglin Wu
@ 2022-10-25  6:25   ` Dmitry Baryshkov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2022-10-25  6:25 UTC (permalink / raw)
  To: Fenglin Wu
  Cc: linux-arm-msm, linux-kernel, sboyd, quic_collinsd, quic_subbaram,
	David Collins

On Tue, 25 Oct 2022 at 08:24, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
> From: David Collins <collinsd@codeaurora.org>
>
> Make the support of PMIC peripheral interrupts optional for
> spmi-pmic-arb devices.  This is useful in situations where
> SPMI address mapping is required without the need for IRQ
> support.

When or why would this be needed? Please provide a description of the usecase.

>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 45 ++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 1e7f5a9ff4bc..c14cffd8a313 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -1490,10 +1490,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>                 goto err_put_ctrl;
>         }
>
> -       pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> -       if (pmic_arb->irq < 0) {
> -               err = pmic_arb->irq;
> -               goto err_put_ctrl;
> +       if (of_find_property(pdev->dev.of_node, "interrupt-controller", NULL)) {

Ugh. Please use platform_get_irq_byname_optional() instead. It returs
-ENXIO if the irq is not present.

> +               pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
> +               if (pmic_arb->irq < 0) {
> +                       err = pmic_arb->irq;
> +                       goto err_put_ctrl;
> +               }
>         }
>
>         err = of_property_read_u32(pdev->dev.of_node, "qcom,channel", &channel);
> @@ -1553,17 +1555,22 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       dev_dbg(&pdev->dev, "adding irq domain\n");
> -       pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
> -                                        &pmic_arb_irq_domain_ops, pmic_arb);
> -       if (!pmic_arb->domain) {
> -               dev_err(&pdev->dev, "unable to create irq_domain\n");
> -               err = -ENOMEM;
> -               goto err_put_ctrl;
> +       if (pmic_arb->irq > 0) {
> +               dev_dbg(&pdev->dev, "adding irq domain\n");
> +               pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
> +                                           &pmic_arb_irq_domain_ops, pmic_arb);
> +               if (!pmic_arb->domain) {
> +                       dev_err(&pdev->dev, "unable to create irq_domain\n");
> +                       err = -ENOMEM;
> +                       goto err_put_ctrl;
> +               }
> +
> +               irq_set_chained_handler_and_data(pmic_arb->irq,
> +                                               pmic_arb_chained_irq, pmic_arb);
> +       } else {
> +               dev_dbg(&pdev->dev, "not supporting PMIC interrupts\n");
>         }
>
> -       irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
> -                                       pmic_arb);
>         err = spmi_controller_add(ctrl);
>         if (err)
>                 goto err_domain_remove;
> @@ -1571,8 +1578,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>         return 0;
>
>  err_domain_remove:
> -       irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
> -       irq_domain_remove(pmic_arb->domain);
> +       if (pmic_arb->irq > 0) {
> +               irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
> +               irq_domain_remove(pmic_arb->domain);
> +       }
>  err_put_ctrl:
>         spmi_controller_put(ctrl);
>         return err;
> @@ -1583,8 +1592,10 @@ static int spmi_pmic_arb_remove(struct platform_device *pdev)
>         struct spmi_controller *ctrl = platform_get_drvdata(pdev);
>         struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
>         spmi_controller_remove(ctrl);
> -       irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
> -       irq_domain_remove(pmic_arb->domain);
> +       if (pmic_arb->irq > 0) {
> +               irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
> +               irq_domain_remove(pmic_arb->domain);
> +       }
>         spmi_controller_put(ctrl);
>         return 0;
>  }
> --
> 2.25.1
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr
  2022-10-25  5:06 ` [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr Fenglin Wu
@ 2022-10-25  6:48   ` Dmitry Baryshkov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Baryshkov @ 2022-10-25  6:48 UTC (permalink / raw)
  To: Fenglin Wu
  Cc: linux-arm-msm, linux-kernel, sboyd, Andy Gross, Bjorn Andersson,
	Konrad Dybcio, quic_collinsd, quic_subbaram, David Collins

On Tue, 25 Oct 2022 at 08:08, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
> From: David Collins <collinsd@codeaurora.org>
>
> Add an exported function which can map an SPMI address to the
> physical address range of the registers used to write to that
> SPMI address.
>
> The feature can be used by consumer drivers that need to lock
> down access to certain PMIC peripherals temporarily for certain
> secure use cases.

No-no-no-no. First, typically the API is only accepted together with
the actual API user. It is bad to have an API call that has no users.

Next, the API you are trying to add doesn't match what you have
described so far. Instead of locking the device from the ARB driver,
you are providing the child devices with the wormhole, meaning that
the child device will have know how to program the ARB. This is not
correct. The ARB programming should happen from the ARB driver, not
from the PMIC/etc drivers.

Last, but not least. The API you have tried to add is added as the one
specific to the spmi_pmic_arb. This is usually a very bad practice.
Please rework this as an API that doesn't depend on the backing SPMI
arbiter.

>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
>  drivers/spmi/spmi-pmic-arb.c           | 104 +++++++++++++++++++++++++
>  include/linux/soc/qcom/spmi-pmic-arb.h |  23 ++++++
>  2 files changed, 127 insertions(+)
>  create mode 100644 include/linux/soc/qcom/spmi-pmic-arb.h
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 2cf3203b2397..1e7f5a9ff4bc 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/spmi.h>
> +#include <linux/soc/qcom/spmi-pmic-arb.h>
>
>  /* PMIC Arbiter configuration registers */
>  #define PMIC_ARB_VERSION               0x0000
> @@ -124,6 +125,8 @@ struct apid_data {
>   *
>   * @rd_base:           on v1 "core", on v2 "observer" register base off DT.
>   * @wr_base:           on v1 "core", on v2 "chnls"    register base off DT.
> + * @wr_base_phys:      base physical address of the register range used for
> + *                     SPMI write commands.
>   * @intr:              address of the SPMI interrupt control registers.
>   * @cnfg:              address of the PMIC Arbiter configuration registers.
>   * @lock:              lock to synchronize accesses.
> @@ -141,6 +144,7 @@ struct apid_data {
>  struct spmi_pmic_arb {
>         void __iomem            *rd_base;
>         void __iomem            *wr_base;
> +       phys_addr_t             wr_base_phys;
>         void __iomem            *intr;
>         void __iomem            *cnfg;
>         void __iomem            *core;
> @@ -180,6 +184,9 @@ struct spmi_pmic_arb {
>   * @irq_clear:         on v1 address of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
>   *                     on v2 address of SPMI_PIC_IRQ_CLEARn.
>   * @apid_map_offset:   offset of PMIC_ARB_REG_CHNLn
> + * @wr_addr_map:       maps from an SPMI address to the physical address
> + *                     range of the registers used to perform an SPMI write
> + *                     command to the SPMI address.
>   */
>  struct pmic_arb_ver_ops {
>         const char *ver_str;
> @@ -196,6 +203,8 @@ struct pmic_arb_ver_ops {
>         void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n);
>         void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n);
>         u32 (*apid_map_offset)(u16 n);
> +       int (*wr_addr_map)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> +                          struct resource *res_out);
>  };
>
>  static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,
> @@ -972,6 +981,21 @@ static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>         return 0x800 + 0x80 * pmic_arb->channel;
>  }
>
> +static int pmic_arb_wr_addr_map_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
> +                                  u16 addr, struct resource *res_out)
> +{
> +       int rc;
> +
> +       rc = pmic_arb_offset_v1(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
> +       if (rc < 0)
> +               return rc;
> +
> +       res_out->start = pmic_arb->wr_base_phys + rc;
> +       res_out->end = res_out->start + 0x80 - 1;
> +
> +       return 0;
> +}
> +
>  static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
>  {
>         struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
> @@ -1111,6 +1135,21 @@ static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>         return 0x1000 * pmic_arb->ee + 0x8000 * apid;
>  }
>
> +static int pmic_arb_wr_addr_map_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
> +                                  u16 addr, struct resource *res_out)
> +{
> +       int rc;
> +
> +       rc = pmic_arb_offset_v2(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
> +       if (rc < 0)
> +               return rc;
> +
> +       res_out->start = pmic_arb->wr_base_phys + rc;
> +       res_out->end = res_out->start + 0x1000 - 1;
> +
> +       return 0;
> +}
> +
>  /*
>   * v5 offset per ee and per apid for observer channels and per apid for
>   * read/write channels.
> @@ -1145,6 +1184,21 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>         return offset;
>  }
>
> +static int pmic_arb_wr_addr_map_v5(struct spmi_pmic_arb *pmic_arb, u8 sid,
> +                                  u16 addr, struct resource *res_out)
> +{
> +       int rc;
> +
> +       rc = pmic_arb_offset_v5(pmic_arb, sid, addr, PMIC_ARB_CHANNEL_RW);
> +       if (rc < 0)
> +               return rc;
> +
> +       res_out->start = pmic_arb->wr_base_phys + rc;
> +       res_out->end = res_out->start + 0x10000 - 1;
> +
> +       return 0;
> +}
> +
>  static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
>  {
>         return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
> @@ -1254,6 +1308,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
>         .irq_status             = pmic_arb_irq_status_v1,
>         .irq_clear              = pmic_arb_irq_clear_v1,
>         .apid_map_offset        = pmic_arb_apid_map_offset_v2,
> +       .wr_addr_map            = pmic_arb_wr_addr_map_v1,
>  };
>
>  static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> @@ -1267,6 +1322,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
>         .irq_status             = pmic_arb_irq_status_v2,
>         .irq_clear              = pmic_arb_irq_clear_v2,
>         .apid_map_offset        = pmic_arb_apid_map_offset_v2,
> +       .wr_addr_map            = pmic_arb_wr_addr_map_v2,
>  };
>
>  static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> @@ -1280,6 +1336,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
>         .irq_status             = pmic_arb_irq_status_v2,
>         .irq_clear              = pmic_arb_irq_clear_v2,
>         .apid_map_offset        = pmic_arb_apid_map_offset_v2,
> +       .wr_addr_map            = pmic_arb_wr_addr_map_v2,
>  };
>
>  static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> @@ -1293,6 +1350,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
>         .irq_status             = pmic_arb_irq_status_v5,
>         .irq_clear              = pmic_arb_irq_clear_v5,
>         .apid_map_offset        = pmic_arb_apid_map_offset_v5,
> +       .wr_addr_map            = pmic_arb_wr_addr_map_v5,
>  };
>
>  static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
> @@ -1302,6 +1360,50 @@ static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
>         .translate = qpnpint_irq_domain_translate,
>  };
>
> +static int _spmi_pmic_arb_map_address(struct spmi_pmic_arb *pmic_arb,
> +                               u32 spmi_address, struct resource *res_out)
> +{
> +       u32 sid, addr;
> +
> +       sid = (spmi_address >> 16) & 0xF;
> +       addr = spmi_address & 0xFFFF;
> +
> +       return pmic_arb->ver_ops->wr_addr_map(pmic_arb, sid, addr, res_out);
> +}
> +
> +/**
> + * spmi_pmic_arb_map_address() - returns physical addresses of registers used to
> + *             write to the PMIC peripheral at spmi_address
> + * @ctrl:              SPMI controller device pointer
> + * @spmi_address:      20-bit SPMI address of the form: 0xSPPPP
> + *                     where S = global PMIC SID and
> + *                     PPPP = SPMI address within the slave
> + * @res_out:           Resource struct (allocated by the caller) in which
> + *                     physical addresses for the range are passed via start
> + *                     and end elements
> + *
> + * Returns: 0 on success or an errno on failure.
> + */
> +int spmi_pmic_arb_map_address(struct spmi_controller *ctrl, u32 spmi_address,
> +                             struct resource *res_out)
> +{
> +       struct spmi_pmic_arb *pmic_arb;
> +
> +       if (!ctrl || !res_out) {
> +               pr_err("%s: Invalid pointer\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       pmic_arb = spmi_controller_get_drvdata(ctrl);
> +       if (!pmic_arb) {
> +               pr_err("Missing PMIC arbiter device\n");
> +               return -ENODEV;
> +       }
> +
> +       return _spmi_pmic_arb_map_address(pmic_arb, spmi_address, res_out);
> +}
> +EXPORT_SYMBOL(spmi_pmic_arb_map_address);
> +
>  static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  {
>         struct spmi_pmic_arb *pmic_arb;
> @@ -1341,6 +1443,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>         if (hw_ver < PMIC_ARB_VERSION_V2_MIN) {
>                 pmic_arb->ver_ops = &pmic_arb_v1;
>                 pmic_arb->wr_base = core;
> +               pmic_arb->wr_base_phys = res->start;
>                 pmic_arb->rd_base = core;
>         } else {
>                 pmic_arb->core = core;
> @@ -1367,6 +1470,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>                         err = PTR_ERR(pmic_arb->wr_base);
>                         goto err_put_ctrl;
>                 }
> +               pmic_arb->wr_base_phys = res->start;
>         }
>
>         dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
> diff --git a/include/linux/soc/qcom/spmi-pmic-arb.h b/include/linux/soc/qcom/spmi-pmic-arb.h
> new file mode 100644
> index 000000000000..adb999d6e246
> --- /dev/null
> +++ b/include/linux/soc/qcom/spmi-pmic-arb.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
> +
> +#ifndef _SPMI_PMIC_ARB_H
> +#define _SPMI_PMIC_ARB_H
> +
> +#include <linux/spmi.h>
> +#include <linux/ioport.h>
> +#include <linux/types.h>
> +
> +
> +#if IS_ENABLED(CONFIG_SPMI_MSM_PMIC_ARB)
> +int spmi_pmic_arb_map_address(struct spmi_controller *ctrl, u32 spmi_address,
> +                             struct resource *res_out);
> +#else
> +static inline int spmi_pmic_arb_map_address(struct spmi_controller *ctrl,
> +                               u32 spmi_address, struct resource *res_out)
> +{
> +       return -ENODEV;
> +}
> +#endif
> +
> +#endif
> --
> 2.25.1
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-10-25  6:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-25  5:06 [RESEND PATCH v1 0/2] Add changes to support SPMI resource protection Fenglin Wu
2022-10-25  5:06 ` [RESEND PATCH v1 1/2] spmi: pmic-arb: add support to map SPMI addresses to physical addr Fenglin Wu
2022-10-25  6:48   ` Dmitry Baryshkov
2022-10-25  5:06 ` [RESEND PATCH v1 2/2] spmi: pmic-arb: make interrupt support optional Fenglin Wu
2022-10-25  6:25   ` Dmitry Baryshkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.