mbox series

[0/2] Resolve MPM register space situation

Message ID 20230328-topic-msgram_mpm-v1-0-1b788a5f5a33@linaro.org
Headers show
Series Resolve MPM register space situation | expand

Message

Konrad Dybcio March 28, 2023, 10:02 a.m. UTC
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,

Comments

Shawn Guo March 29, 2023, 3:34 a.m. UTC | #1
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>
>
Shawn Guo March 29, 2023, 3:49 a.m. UTC | #2
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
>
Shawn Guo March 29, 2023, 3:58 a.m. UTC | #3
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

> 			};
> 		};
> 	};
> };
Konrad Dybcio March 29, 2023, 11:06 a.m. UTC | #4
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
>>
Konrad Dybcio March 29, 2023, 11:18 a.m. UTC | #5
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
> 
>> 			};
>> 		};
>> 	};
>> };
Shawn Guo March 29, 2023, 1:28 p.m. UTC | #6
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
Konrad Dybcio March 29, 2023, 1:30 p.m. UTC | #7
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
Shawn Guo March 30, 2023, 1:34 a.m. UTC | #8
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
Shawn Guo March 30, 2023, 1:50 a.m. UTC | #9
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
Konrad Dybcio March 30, 2023, 11:17 a.m. UTC | #10
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
Konrad Dybcio April 1, 2023, 12:06 p.m. UTC | #11
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