[PATCH 0/2] Resolve MPM register space situation

[PATCH 0/2] Resolve MPM register space situation

[Konrad Dybcio]

The MPM (and some other things, irrelevant to this patchset) resides
(as far as the ARM cores are concerned, anyway) in a MMIO-mapped region
that's a portion of the RPM (low-power management core)'s RAM, known
as the RPM Message RAM. Representing this relation in the Device Tree
creates some challenges, as one would either have to treat a memory
region as a bus, map nodes in a way such that their reg-s would be
overlapping, or supply the nodes with a slice of that region.

This series implements the third option, by adding a qcom,rpm-msg-ram
property, which has been used for some drivers poking into this region
before. Bindings ABI compatibility is preserved through keeping the
"normal" (a.k.a read the reg property and map that region) way of
passing the register space.

Example representation with this patchset:

/ {
	[...]

	mpm: interrupt-controller {
		compatible = "qcom,mpm";
		qcom,rpm-msg-ram = <&apss_mpm>;
		[...]
	};

	[...]

	soc: soc@0 {
		[...]

		rpm_msg_ram: sram@45f0000 {
			compatible = "qcom,rpm-msg-ram", "mmio-sram";
			reg = <0 0x045f0000 0 0x7000>;
			#address-cells = <1>;
			#size-cells = <1>;
			ranges = <0 0x0 0x045f0000 0x7000>;

			apss_mpm: sram@1b8 {
				reg = <0x1b8 0x48>;
			};
		};
	};
};

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
      dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle
      irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

 .../bindings/interrupt-controller/qcom,mpm.yaml    |  6 ++++-
 drivers/irqchip/irq-qcom-mpm.c                     | 30 ++++++++++++++++++----
 2 files changed, 30 insertions(+), 6 deletions(-)
---
base-commit: a6faf7ea9fcb7267d06116d4188947f26e00e57e
change-id: 20230328-topic-msgram_mpm-c688be3bc294

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/2] dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle

[Konrad Dybcio]

Due to the wild nature of the Qualcomm RPM Message RAM, we can't really
use 'reg' to point to the MPM's slice of Message RAM without cutting into
an already-defined RPM MSG RAM node used for GLINK and SMEM.

Document passing the register space as a slice of SRAM through the
qcom,rpm-msg-ram property. This also makes 'reg' no longer required.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml          | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
index 509d20c091af..77fe5e0b378f 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
@@ -30,6 +30,11 @@ properties:
     description:
       Specifies the base address and size of vMPM registers in RPM MSG RAM.
 
+  qcom,rpm-msg-ram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the APSS MPM slice of the RPM Message RAM
+
   interrupts:
     maxItems: 1
     description:
@@ -64,7 +69,6 @@ properties:
 
 required:
   - compatible
-  - reg
   - interrupts
   - mboxes
   - interrupt-controller

-- 
2.40.0

[PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Konrad Dybcio]

The MPM hardware is accessible to us from the ARM CPUs through a shared
memory region (RPM MSG RAM) that's also concurrently accessed by other
kinds of cores on the system (like modem, ADSP etc.). Modeling this
relation in a (somewhat) sane manner in the device tree basically
requires us to either present the MPM as a child of said memory region
(which makes little sense, as a mapped memory carveout is not a bus),
define nodes which bleed their register spaces into one another, or
passing their slice of the MSG RAM through some kind of a property.

Go with the third option and add a way to map a region passed through
the "qcom,rpm-msg-ram" property as our register space.

The current way of using 'reg' is preserved for ABI reasons.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/irqchip/irq-qcom-mpm.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
index d30614661eea..6fe59f4deef4 100644
--- a/drivers/irqchip/irq-qcom-mpm.c
+++ b/drivers/irqchip/irq-qcom-mpm.c
@@ -14,6 +14,7 @@
 #include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
@@ -322,8 +323,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
 	struct device *dev = &pdev->dev;
 	struct irq_domain *parent_domain;
 	struct generic_pm_domain *genpd;
+	struct device_node *msgram_np;
 	struct qcom_mpm_priv *priv;
 	unsigned int pin_cnt;
+	struct resource res;
 	int i, irq;
 	int ret;
 
@@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
 
 	raw_spin_lock_init(&priv->lock);
 
-	priv->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
+	/* If we have a handle to an RPM message ram partition, use it. */
+	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
+	if (msgram_np) {
+		ret = of_address_to_resource(msgram_np, 0, &res);
+		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
+		priv->base = ioremap(res.start, resource_size(&res));
+		of_node_put(msgram_np);
+		if (IS_ERR(priv->base))
+			return PTR_ERR(priv->base);
+	} else {
+		/* Otherwise, fall back to simple MMIO. */
+		priv->base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(priv->base))
+			return PTR_ERR(priv->base);
+	}
 
 	for (i = 0; i < priv->reg_stride; i++) {
 		qcom_mpm_write(priv, MPM_REG_ENABLE, i, 0);
@@ -387,8 +402,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
 	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	if (irq < 0) {
+		ret = irq;
+		goto unmap_base;
+	}
 
 	genpd = &priv->genpd;
 	genpd->flags = GENPD_FLAG_IRQ_SAFE;
@@ -451,6 +468,9 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
 	mbox_free_channel(priv->mbox_chan);
 remove_genpd:
 	pm_genpd_remove(genpd);
+unmap_base:
+	if (res.start)
+		iounmap(priv->base);
 	return ret;
 }
 

-- 
2.40.0

Re: [PATCH 0/2] Resolve MPM register space situation

[Shawn Guo]

On Tue, Mar 28, 2023 at 12:02:51PM +0200, Konrad Dybcio wrote:
> The MPM (and some other things, irrelevant to this patchset) resides
> (as far as the ARM cores are concerned, anyway) in a MMIO-mapped region
> that's a portion of the RPM (low-power management core)'s RAM, known
> as the RPM Message RAM. Representing this relation in the Device Tree
> creates some challenges, as one would either have to treat a memory
> region as a bus, map nodes in a way such that their reg-s would be
> overlapping, or supply the nodes with a slice of that region.
> 
> This series implements the third option, by adding a qcom,rpm-msg-ram
> property, which has been used for some drivers poking into this region
> before. Bindings ABI compatibility is preserved through keeping the
> "normal" (a.k.a read the reg property and map that region) way of
> passing the register space.

I have to admit that I wasn't aware of it, this message RAM is also
accessed by cores like modem, ADSP etc.  I agree in principle this is
a good change!

Shawn

> 
> Example representation with this patchset:
> 
> / {
> 	[...]
> 
> 	mpm: interrupt-controller {
> 		compatible = "qcom,mpm";
> 		qcom,rpm-msg-ram = <&apss_mpm>;
> 		[...]
> 	};
> 
> 	[...]
> 
> 	soc: soc@0 {
> 		[...]
> 
> 		rpm_msg_ram: sram@45f0000 {
> 			compatible = "qcom,rpm-msg-ram", "mmio-sram";
> 			reg = <0 0x045f0000 0 0x7000>;
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			ranges = <0 0x0 0x045f0000 0x7000>;
> 
> 			apss_mpm: sram@1b8 {
> 				reg = <0x1b8 0x48>;
> 			};
> 		};
> 	};
> };
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> Konrad Dybcio (2):
>       dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle
>       irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space
> 
>  .../bindings/interrupt-controller/qcom,mpm.yaml    |  6 ++++-
>  drivers/irqchip/irq-qcom-mpm.c                     | 30 ++++++++++++++++++----
>  2 files changed, 30 insertions(+), 6 deletions(-)
> ---
> base-commit: a6faf7ea9fcb7267d06116d4188947f26e00e57e
> change-id: 20230328-topic-msgram_mpm-c688be3bc294
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@linaro.org>
> 

Re: [PATCH 1/2] dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle

[Shawn Guo]

On Tue, Mar 28, 2023 at 12:02:52PM +0200, Konrad Dybcio wrote:
> Due to the wild nature of the Qualcomm RPM Message RAM, we can't really
> use 'reg' to point to the MPM's slice of Message RAM without cutting into
> an already-defined RPM MSG RAM node used for GLINK and SMEM.
> 
> Document passing the register space as a slice of SRAM through the
> qcom,rpm-msg-ram property. This also makes 'reg' no longer required.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml          | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> index 509d20c091af..77fe5e0b378f 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> @@ -30,6 +30,11 @@ properties:
>      description:
>        Specifies the base address and size of vMPM registers in RPM MSG RAM.
>  
> +  qcom,rpm-msg-ram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the APSS MPM slice of the RPM Message RAM
> +
>    interrupts:
>      maxItems: 1
>      description:
> @@ -64,7 +69,6 @@ properties:
>  
>  required:
>    - compatible
> -  - reg

It's not my call, but I wonder if we need to maintain the 'reg' ABI
at all, as there is no DTS landed so far.  In either case, I suggest
we update the example to adopt the new way.

Shawn

>    - interrupts
>    - mboxes
>    - interrupt-controller
> 
> -- 
> 2.40.0
> 

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Shawn Guo]

On Tue, Mar 28, 2023 at 12:02:53PM +0200, Konrad Dybcio wrote:
> The MPM hardware is accessible to us from the ARM CPUs through a shared
> memory region (RPM MSG RAM) that's also concurrently accessed by other
> kinds of cores on the system (like modem, ADSP etc.). Modeling this
> relation in a (somewhat) sane manner in the device tree basically
> requires us to either present the MPM as a child of said memory region
> (which makes little sense, as a mapped memory carveout is not a bus),
> define nodes which bleed their register spaces into one another, or
> passing their slice of the MSG RAM through some kind of a property.
> 
> Go with the third option and add a way to map a region passed through
> the "qcom,rpm-msg-ram" property as our register space.
> 
> The current way of using 'reg' is preserved for ABI reasons.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/irqchip/irq-qcom-mpm.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> index d30614661eea..6fe59f4deef4 100644
> --- a/drivers/irqchip/irq-qcom-mpm.c
> +++ b/drivers/irqchip/irq-qcom-mpm.c
> @@ -14,6 +14,7 @@
>  #include <linux/mailbox_client.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> @@ -322,8 +323,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>  	struct device *dev = &pdev->dev;
>  	struct irq_domain *parent_domain;
>  	struct generic_pm_domain *genpd;
> +	struct device_node *msgram_np;
>  	struct qcom_mpm_priv *priv;
>  	unsigned int pin_cnt;
> +	struct resource res;
>  	int i, irq;
>  	int ret;
>  
> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>  
>  	raw_spin_lock_init(&priv->lock);
>  
> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(priv->base))
> -		return PTR_ERR(priv->base);
> +	/* If we have a handle to an RPM message ram partition, use it. */
> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
> +	if (msgram_np) {
> +		ret = of_address_to_resource(msgram_np, 0, &res);
> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
> +		priv->base = ioremap(res.start, resource_size(&res));

Are you suggesting that other cores/drivers will also need to access
the mpm slice below?

	apss_mpm: sram@1b8 {
		reg = <0x1b8 0x48>;
	};

Shawn

> +		of_node_put(msgram_np);
> +		if (IS_ERR(priv->base))
> +			return PTR_ERR(priv->base);
> +	} else {
> +		/* Otherwise, fall back to simple MMIO. */
> +		priv->base = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(priv->base))
> +			return PTR_ERR(priv->base);
> +	}
>  
>  	for (i = 0; i < priv->reg_stride; i++) {
>  		qcom_mpm_write(priv, MPM_REG_ENABLE, i, 0);
> @@ -387,8 +402,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0)
> -		return irq;
> +	if (irq < 0) {
> +		ret = irq;
> +		goto unmap_base;
> +	}
>  
>  	genpd = &priv->genpd;
>  	genpd->flags = GENPD_FLAG_IRQ_SAFE;
> @@ -451,6 +468,9 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>  	mbox_free_channel(priv->mbox_chan);
>  remove_genpd:
>  	pm_genpd_remove(genpd);
> +unmap_base:
> +	if (res.start)
> +		iounmap(priv->base);
>  	return ret;
>  }
>  
> 
> -- 
> 2.40.0
> 

Re: [PATCH 0/2] Resolve MPM register space situation

[Shawn Guo]

On Tue, Mar 28, 2023 at 12:02:51PM +0200, Konrad Dybcio wrote:
> The MPM (and some other things, irrelevant to this patchset) resides
> (as far as the ARM cores are concerned, anyway) in a MMIO-mapped region
> that's a portion of the RPM (low-power management core)'s RAM, known
> as the RPM Message RAM. Representing this relation in the Device Tree
> creates some challenges, as one would either have to treat a memory
> region as a bus, map nodes in a way such that their reg-s would be
> overlapping, or supply the nodes with a slice of that region.
> 
> This series implements the third option, by adding a qcom,rpm-msg-ram
> property, which has been used for some drivers poking into this region
> before. Bindings ABI compatibility is preserved through keeping the
> "normal" (a.k.a read the reg property and map that region) way of
> passing the register space.
> 
> Example representation with this patchset:
> 
> / {
> 	[...]
> 
> 	mpm: interrupt-controller {
> 		compatible = "qcom,mpm";
> 		qcom,rpm-msg-ram = <&apss_mpm>;
> 		[...]
> 	};
> 
> 	[...]
> 
> 	soc: soc@0 {
> 		[...]
> 
> 		rpm_msg_ram: sram@45f0000 {
> 			compatible = "qcom,rpm-msg-ram", "mmio-sram";
> 			reg = <0 0x045f0000 0 0x7000>;
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			ranges = <0 0x0 0x045f0000 0x7000>;
> 
> 			apss_mpm: sram@1b8 {
> 				reg = <0x1b8 0x48>;

Per "vMPM register map" in the driver, the slice size should be 0x44
instead of 0x48.  Is there one register missing from the driver
comment?

PS. It seems the "n" formula in the driver comment should be corrected
as below.

  n = DIV_ROUND_UP(pin_cnt, 32) - 1

Shawn

> 			};
> 		};
> 	};
> };

Re: [PATCH 1/2] dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle

[Krzysztof Kozlowski]

On 28/03/2023 12:02, Konrad Dybcio wrote:
> Due to the wild nature of the Qualcomm RPM Message RAM, we can't really
> use 'reg' to point to the MPM's slice of Message RAM without cutting into
> an already-defined RPM MSG RAM node used for GLINK and SMEM.
> 
> Document passing the register space as a slice of SRAM through the
> qcom,rpm-msg-ram property. This also makes 'reg' no longer required.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml          | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> index 509d20c091af..77fe5e0b378f 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
> @@ -30,6 +30,11 @@ properties:
>      description:
>        Specifies the base address and size of vMPM registers in RPM MSG RAM.
>  
> +  qcom,rpm-msg-ram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the APSS MPM slice of the RPM Message RAM
> +
>    interrupts:
>      maxItems: 1
>      description:
> @@ -64,7 +69,6 @@ properties:
>  
>  required:
>    - compatible
> -  - reg

Either:
1. make reg deprecated and require qcom,rpm-msg-ram
or
2. you need oneOf:required for reg and qcom,rpm-msg-ram

>    - interrupts
>    - mboxes
>    - interrupt-controller
> 

Best regards,
Krzysztof

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Konrad Dybcio]

On 29.03.2023 05:49, Shawn Guo wrote:
> On Tue, Mar 28, 2023 at 12:02:53PM +0200, Konrad Dybcio wrote:
>> The MPM hardware is accessible to us from the ARM CPUs through a shared
>> memory region (RPM MSG RAM) that's also concurrently accessed by other
>> kinds of cores on the system (like modem, ADSP etc.). Modeling this
>> relation in a (somewhat) sane manner in the device tree basically
>> requires us to either present the MPM as a child of said memory region
>> (which makes little sense, as a mapped memory carveout is not a bus),
>> define nodes which bleed their register spaces into one another, or
>> passing their slice of the MSG RAM through some kind of a property.
>>
>> Go with the third option and add a way to map a region passed through
>> the "qcom,rpm-msg-ram" property as our register space.
>>
>> The current way of using 'reg' is preserved for ABI reasons.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/irqchip/irq-qcom-mpm.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
>> index d30614661eea..6fe59f4deef4 100644
>> --- a/drivers/irqchip/irq-qcom-mpm.c
>> +++ b/drivers/irqchip/irq-qcom-mpm.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/mailbox_client.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_address.h>
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_domain.h>
>> @@ -322,8 +323,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>  	struct device *dev = &pdev->dev;
>>  	struct irq_domain *parent_domain;
>>  	struct generic_pm_domain *genpd;
>> +	struct device_node *msgram_np;
>>  	struct qcom_mpm_priv *priv;
>>  	unsigned int pin_cnt;
>> +	struct resource res;
>>  	int i, irq;
>>  	int ret;
>>  
>> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>  
>>  	raw_spin_lock_init(&priv->lock);
>>  
>> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
>> -	if (IS_ERR(priv->base))
>> -		return PTR_ERR(priv->base);
>> +	/* If we have a handle to an RPM message ram partition, use it. */
>> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
>> +	if (msgram_np) {
>> +		ret = of_address_to_resource(msgram_np, 0, &res);
>> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
>> +		priv->base = ioremap(res.start, resource_size(&res));
> 
> Are you suggesting that other cores/drivers will also need to access
> the mpm slice below?
> 
> 	apss_mpm: sram@1b8 {
> 		reg = <0x1b8 0x48>;
> 	};
Yes, the RPM M3 core. Other slices may be accessed
by any core at any time.

Konrad
> 
> Shawn
> 
>> +		of_node_put(msgram_np);
>> +		if (IS_ERR(priv->base))
>> +			return PTR_ERR(priv->base);
>> +	} else {
>> +		/* Otherwise, fall back to simple MMIO. */
>> +		priv->base = devm_platform_ioremap_resource(pdev, 0);
>> +		if (IS_ERR(priv->base))
>> +			return PTR_ERR(priv->base);
>> +	}
>>  
>>  	for (i = 0; i < priv->reg_stride; i++) {
>>  		qcom_mpm_write(priv, MPM_REG_ENABLE, i, 0);
>> @@ -387,8 +402,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>  	}
>>  
>>  	irq = platform_get_irq(pdev, 0);
>> -	if (irq < 0)
>> -		return irq;
>> +	if (irq < 0) {
>> +		ret = irq;
>> +		goto unmap_base;
>> +	}
>>  
>>  	genpd = &priv->genpd;
>>  	genpd->flags = GENPD_FLAG_IRQ_SAFE;
>> @@ -451,6 +468,9 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>  	mbox_free_channel(priv->mbox_chan);
>>  remove_genpd:
>>  	pm_genpd_remove(genpd);
>> +unmap_base:
>> +	if (res.start)
>> +		iounmap(priv->base);
>>  	return ret;
>>  }
>>  
>>
>> -- 
>> 2.40.0
>>

Re: [PATCH 0/2] Resolve MPM register space situation

[Konrad Dybcio]

On 29.03.2023 05:58, Shawn Guo wrote:
> On Tue, Mar 28, 2023 at 12:02:51PM +0200, Konrad Dybcio wrote:
>> The MPM (and some other things, irrelevant to this patchset) resides
>> (as far as the ARM cores are concerned, anyway) in a MMIO-mapped region
>> that's a portion of the RPM (low-power management core)'s RAM, known
>> as the RPM Message RAM. Representing this relation in the Device Tree
>> creates some challenges, as one would either have to treat a memory
>> region as a bus, map nodes in a way such that their reg-s would be
>> overlapping, or supply the nodes with a slice of that region.
>>
>> This series implements the third option, by adding a qcom,rpm-msg-ram
>> property, which has been used for some drivers poking into this region
>> before. Bindings ABI compatibility is preserved through keeping the
>> "normal" (a.k.a read the reg property and map that region) way of
>> passing the register space.
>>
>> Example representation with this patchset:
>>
>> / {
>> 	[...]
>>
>> 	mpm: interrupt-controller {
>> 		compatible = "qcom,mpm";
>> 		qcom,rpm-msg-ram = <&apss_mpm>;
>> 		[...]
>> 	};
>>
>> 	[...]
>>
>> 	soc: soc@0 {
>> 		[...]
>>
>> 		rpm_msg_ram: sram@45f0000 {
>> 			compatible = "qcom,rpm-msg-ram", "mmio-sram";
>> 			reg = <0 0x045f0000 0 0x7000>;
>> 			#address-cells = <1>;
>> 			#size-cells = <1>;
>> 			ranges = <0 0x0 0x045f0000 0x7000>;
>>
>> 			apss_mpm: sram@1b8 {
>> 				reg = <0x1b8 0x48>;
> 
> Per "vMPM register map" in the driver, the slice size should be 0x44
> instead of 0x48.  Is there one register missing from the driver
> comment?
Yeah we should be using 0x44..

> 
> PS. It seems the "n" formula in the driver comment should be corrected
> as below.
> 
>   n = DIV_ROUND_UP(pin_cnt, 32) - 1
Or since we're counting from zero, the ENABLEn should become
ENABLE(n-1) etc.

Konrad

> 
> Shawn
> 
>> 			};
>> 		};
>> 	};
>> };

Re: [PATCH 1/2] dt-bindings: interrupt-controller: mpm: Allow passing reg through phandle

[Konrad Dybcio]

On 29.03.2023 10:27, Krzysztof Kozlowski wrote:
> On 28/03/2023 12:02, Konrad Dybcio wrote:
>> Due to the wild nature of the Qualcomm RPM Message RAM, we can't really
>> use 'reg' to point to the MPM's slice of Message RAM without cutting into
>> an already-defined RPM MSG RAM node used for GLINK and SMEM.
>>
>> Document passing the register space as a slice of SRAM through the
>> qcom,rpm-msg-ram property. This also makes 'reg' no longer required.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml          | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
>> index 509d20c091af..77fe5e0b378f 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
>> @@ -30,6 +30,11 @@ properties:
>>      description:
>>        Specifies the base address and size of vMPM registers in RPM MSG RAM.
>>  
>> +  qcom,rpm-msg-ram:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      Phandle to the APSS MPM slice of the RPM Message RAM
>> +
>>    interrupts:
>>      maxItems: 1
>>      description:
>> @@ -64,7 +69,6 @@ properties:
>>  
>>  required:
>>    - compatible
>> -  - reg
> 
> Either:
> 1. make reg deprecated and require qcom,rpm-msg-ram
> or
> 2. you need oneOf:required for reg and qcom,rpm-msg-ram
Right, let's go with 1.

Konrad
> 
>>    - interrupts
>>    - mboxes
>>    - interrupt-controller
>>
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Shawn Guo]

On Wed, Mar 29, 2023 at 01:06:11PM +0200, Konrad Dybcio wrote:
> 
> 
> On 29.03.2023 05:49, Shawn Guo wrote:
> > On Tue, Mar 28, 2023 at 12:02:53PM +0200, Konrad Dybcio wrote:
> >> The MPM hardware is accessible to us from the ARM CPUs through a shared
> >> memory region (RPM MSG RAM) that's also concurrently accessed by other
> >> kinds of cores on the system (like modem, ADSP etc.). Modeling this
> >> relation in a (somewhat) sane manner in the device tree basically
> >> requires us to either present the MPM as a child of said memory region
> >> (which makes little sense, as a mapped memory carveout is not a bus),
> >> define nodes which bleed their register spaces into one another, or
> >> passing their slice of the MSG RAM through some kind of a property.
> >>
> >> Go with the third option and add a way to map a region passed through
> >> the "qcom,rpm-msg-ram" property as our register space.
> >>
> >> The current way of using 'reg' is preserved for ABI reasons.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>  drivers/irqchip/irq-qcom-mpm.c | 30 +++++++++++++++++++++++++-----
> >>  1 file changed, 25 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> >> index d30614661eea..6fe59f4deef4 100644
> >> --- a/drivers/irqchip/irq-qcom-mpm.c
> >> +++ b/drivers/irqchip/irq-qcom-mpm.c
> >> @@ -14,6 +14,7 @@
> >>  #include <linux/mailbox_client.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of.h>
> >> +#include <linux/of_address.h>
> >>  #include <linux/of_device.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/pm_domain.h>
> >> @@ -322,8 +323,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
> >>  	struct device *dev = &pdev->dev;
> >>  	struct irq_domain *parent_domain;
> >>  	struct generic_pm_domain *genpd;
> >> +	struct device_node *msgram_np;
> >>  	struct qcom_mpm_priv *priv;
> >>  	unsigned int pin_cnt;
> >> +	struct resource res;
> >>  	int i, irq;
> >>  	int ret;
> >>  
> >> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
> >>  
> >>  	raw_spin_lock_init(&priv->lock);
> >>  
> >> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
> >> -	if (IS_ERR(priv->base))
> >> -		return PTR_ERR(priv->base);
> >> +	/* If we have a handle to an RPM message ram partition, use it. */
> >> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
> >> +	if (msgram_np) {
> >> +		ret = of_address_to_resource(msgram_np, 0, &res);
> >> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
> >> +		priv->base = ioremap(res.start, resource_size(&res));
> > 
> > Are you suggesting that other cores/drivers will also need to access
> > the mpm slice below?
> > 
> > 	apss_mpm: sram@1b8 {
> > 		reg = <0x1b8 0x48>;
> > 	};
> Yes, the RPM M3 core. Other slices may be accessed
> by any core at any time.

Hmm, let me reword my question.  Other than irq-qcom-mpm, is there any
other Linux drivers that also need to request this slice region?
Otherwise, I do not understand why devm_ioremap_resource() cannot be
used.

Shawn

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Konrad Dybcio]

On 29.03.2023 15:28, Shawn Guo wrote:
> On Wed, Mar 29, 2023 at 01:06:11PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 29.03.2023 05:49, Shawn Guo wrote:
>>> On Tue, Mar 28, 2023 at 12:02:53PM +0200, Konrad Dybcio wrote:
>>>> The MPM hardware is accessible to us from the ARM CPUs through a shared
>>>> memory region (RPM MSG RAM) that's also concurrently accessed by other
>>>> kinds of cores on the system (like modem, ADSP etc.). Modeling this
>>>> relation in a (somewhat) sane manner in the device tree basically
>>>> requires us to either present the MPM as a child of said memory region
>>>> (which makes little sense, as a mapped memory carveout is not a bus),
>>>> define nodes which bleed their register spaces into one another, or
>>>> passing their slice of the MSG RAM through some kind of a property.
>>>>
>>>> Go with the third option and add a way to map a region passed through
>>>> the "qcom,rpm-msg-ram" property as our register space.
>>>>
>>>> The current way of using 'reg' is preserved for ABI reasons.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  drivers/irqchip/irq-qcom-mpm.c | 30 +++++++++++++++++++++++++-----
>>>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
>>>> index d30614661eea..6fe59f4deef4 100644
>>>> --- a/drivers/irqchip/irq-qcom-mpm.c
>>>> +++ b/drivers/irqchip/irq-qcom-mpm.c
>>>> @@ -14,6 +14,7 @@
>>>>  #include <linux/mailbox_client.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>>  #include <linux/of_device.h>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/pm_domain.h>
>>>> @@ -322,8 +323,10 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>>>  	struct device *dev = &pdev->dev;
>>>>  	struct irq_domain *parent_domain;
>>>>  	struct generic_pm_domain *genpd;
>>>> +	struct device_node *msgram_np;
>>>>  	struct qcom_mpm_priv *priv;
>>>>  	unsigned int pin_cnt;
>>>> +	struct resource res;
>>>>  	int i, irq;
>>>>  	int ret;
>>>>  
>>>> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>>>  
>>>>  	raw_spin_lock_init(&priv->lock);
>>>>  
>>>> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
>>>> -	if (IS_ERR(priv->base))
>>>> -		return PTR_ERR(priv->base);
>>>> +	/* If we have a handle to an RPM message ram partition, use it. */
>>>> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
>>>> +	if (msgram_np) {
>>>> +		ret = of_address_to_resource(msgram_np, 0, &res);
>>>> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
>>>> +		priv->base = ioremap(res.start, resource_size(&res));
>>>
>>> Are you suggesting that other cores/drivers will also need to access
>>> the mpm slice below?
>>>
>>> 	apss_mpm: sram@1b8 {
>>> 		reg = <0x1b8 0x48>;
>>> 	};
>> Yes, the RPM M3 core. Other slices may be accessed
>> by any core at any time.
> 
> Hmm, let me reword my question.  Other than irq-qcom-mpm, is there any
> other Linux drivers that also need to request this slice region?
No.

> Otherwise, I do not understand why devm_ioremap_resource() cannot be
> used.
drivers/rpmsg/qcom_glink_rpm.c calls devm_ioremap on the entire
RPM MSG RAM.

Konrad

> 
> Shawn

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Shawn Guo]

On Wed, Mar 29, 2023 at 03:30:12PM +0200, Konrad Dybcio wrote:
> >>>> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
> >>>>  
> >>>>  	raw_spin_lock_init(&priv->lock);
> >>>>  
> >>>> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
> >>>> -	if (IS_ERR(priv->base))
> >>>> -		return PTR_ERR(priv->base);
> >>>> +	/* If we have a handle to an RPM message ram partition, use it. */
> >>>> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
> >>>> +	if (msgram_np) {
> >>>> +		ret = of_address_to_resource(msgram_np, 0, &res);
> >>>> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
> >>>> +		priv->base = ioremap(res.start, resource_size(&res));
> >>>
> >>> Are you suggesting that other cores/drivers will also need to access
> >>> the mpm slice below?
> >>>
> >>> 	apss_mpm: sram@1b8 {
> >>> 		reg = <0x1b8 0x48>;
> >>> 	};
> >> Yes, the RPM M3 core. Other slices may be accessed
> >> by any core at any time.
> > 
> > Hmm, let me reword my question.  Other than irq-qcom-mpm, is there any
> > other Linux drivers that also need to request this slice region?
> No.
> 
> > Otherwise, I do not understand why devm_ioremap_resource() cannot be
> > used.
> drivers/rpmsg/qcom_glink_rpm.c calls devm_ioremap on the entire
> RPM MSG RAM.

Can we use devm_ioremap() too instead of ioremap() here?

Shawn

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Shawn Guo]

On Wed, Mar 29, 2023 at 03:30:12PM +0200, Konrad Dybcio wrote:
> > Otherwise, I do not understand why devm_ioremap_resource() cannot be
> > used.
> drivers/rpmsg/qcom_glink_rpm.c calls devm_ioremap on the entire
> RPM MSG RAM.

qcom_glink_rpm driver remaps the entire RPM MSG RAM, but it doesn't seem
to request any region.  So MPM can still call devm_ioremap_resource() on
its slice, no?

Shawn

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Konrad Dybcio]

On 30.03.2023 03:50, Shawn Guo wrote:
> On Wed, Mar 29, 2023 at 03:30:12PM +0200, Konrad Dybcio wrote:
>>> Otherwise, I do not understand why devm_ioremap_resource() cannot be
>>> used.
>> drivers/rpmsg/qcom_glink_rpm.c calls devm_ioremap on the entire
>> RPM MSG RAM.
> 
> qcom_glink_rpm driver remaps the entire RPM MSG RAM, but it doesn't seem
> to request any region.  So MPM can still call devm_ioremap_resource() on
> its slice, no?
FWIW, I did get a 'can't request resource error'.

Konrad
> 
> Shawn

Re: [PATCH 2/2] irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

[Konrad Dybcio]

On 30.03.2023 03:34, Shawn Guo wrote:
> On Wed, Mar 29, 2023 at 03:30:12PM +0200, Konrad Dybcio wrote:
>>>>>> @@ -374,9 +377,21 @@ static int qcom_mpm_init(struct device_node *np, struct device_node *parent)
>>>>>>  
>>>>>>  	raw_spin_lock_init(&priv->lock);
>>>>>>  
>>>>>> -	priv->base = devm_platform_ioremap_resource(pdev, 0);
>>>>>> -	if (IS_ERR(priv->base))
>>>>>> -		return PTR_ERR(priv->base);
>>>>>> +	/* If we have a handle to an RPM message ram partition, use it. */
>>>>>> +	msgram_np = of_parse_phandle(np, "qcom,rpm-msg-ram", 0);
>>>>>> +	if (msgram_np) {
>>>>>> +		ret = of_address_to_resource(msgram_np, 0, &res);
>>>>>> +		/* Don't use devm_ioremap_resource, as we're accessing a shared region. */
>>>>>> +		priv->base = ioremap(res.start, resource_size(&res));
>>>>>
>>>>> Are you suggesting that other cores/drivers will also need to access
>>>>> the mpm slice below?
>>>>>
>>>>> 	apss_mpm: sram@1b8 {
>>>>> 		reg = <0x1b8 0x48>;
>>>>> 	};
>>>> Yes, the RPM M3 core. Other slices may be accessed
>>>> by any core at any time.
>>>
>>> Hmm, let me reword my question.  Other than irq-qcom-mpm, is there any
>>> other Linux drivers that also need to request this slice region?
>> No.
>>
>>> Otherwise, I do not understand why devm_ioremap_resource() cannot be
>>> used.
>> drivers/rpmsg/qcom_glink_rpm.c calls devm_ioremap on the entire
>> RPM MSG RAM.
> 
> Can we use devm_ioremap() too instead of ioremap() here?
Hm. Yes, we totally can!

Konrad
> 
> Shawn