[PATCH v2 0/2] Resolve MPM register space situation

[PATCH v2 0/2] Resolve MPM register space situation

[Konrad Dybcio]

v1 -> v2:
- deprecate 'reg', make qcom,rpm-msg-ram required [1/2]
- Use devm_ioremap() [2/2]

Link to v1: https://lore.kernel.org/r/20230328-topic-msgram_mpm-v1-0-1b788a5f5a33@linaro.org

Depends on resolution of https://github.com/devicetree-org/dt-schema/issues/104

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: Pass MSG RAM slice through phandle
      irqchip: irq-qcom-mpm: Support passing a slice of SRAM as reg space

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

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

[PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice 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' deprecated.

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

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
index 509d20c091af..61fc5b1b74dc 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.yaml
@@ -29,6 +29,12 @@ properties:
     maxItems: 1
     description:
       Specifies the base address and size of vMPM registers in RPM MSG RAM.
+    deprecated: true
+
+  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
@@ -64,23 +70,22 @@ properties:
 
 required:
   - compatible
-  - reg
   - interrupts
   - mboxes
   - interrupt-controller
   - '#interrupt-cells'
   - qcom,mpm-pin-count
   - qcom,mpm-pin-map
+  - qcom,rpm-msg-ram
 
 additionalProperties: false
 
 examples:
   - |
     #include <dt-bindings/interrupt-controller/arm-gic.h>
-    mpm: interrupt-controller@45f01b8 {
+    mpm: interrupt-controller {
         compatible = "qcom,mpm";
         interrupts = <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>;
-        reg = <0x45f01b8 0x1000>;
         mboxes = <&apcs_glb 1>;
         interrupt-controller;
         #interrupt-cells = <2>;
@@ -93,4 +98,5 @@ examples:
                            <86 183>,
                            <90 260>,
                            <91 260>;
+        qcom,rpm-msg-ram = <&apss_mpm>;
     };

-- 
2.40.0

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Rob Herring]

On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230328-topic-msgram_mpm-v2-1-e24a48e57f0d@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Rob Herring]

On Wed, Apr 05, 2023 at 07:22:40AM -0500, Rob Herring wrote:
> 
> On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
> > 
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > ---
> >  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict

Looks like this is colliding with the example template which has to 
craft an interrupt provider for 'interrupts' properties. Either adding a 
parent node or using interrupts-extended instead should work-around it.

Rob

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 5.04.2023 15:47, Rob Herring wrote:
> On Wed, Apr 05, 2023 at 07:22:40AM -0500, Rob Herring wrote:
>>
>> On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>
>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
> 
> Looks like this is colliding with the example template which has to 
> craft an interrupt provider for 'interrupts' properties. Either adding a 
> parent node or using interrupts-extended instead should work-around it.
Check the devicetree-org issue linked in the cover letter, please!

I suppose wrapping it in a parent node could work as a temporary
measure, but since it belongs outside /soc, I'd have to make up
a bogus simple-bus, I think.

Konrad
> 
> Rob

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Krzysztof Kozlowski]

On 05/04/2023 15:49, Konrad Dybcio wrote:
> 
> 
> On 5.04.2023 15:47, Rob Herring wrote:
>> On Wed, Apr 05, 2023 at 07:22:40AM -0500, Rob Herring wrote:
>>>
>>> On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>
>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>
>>> yamllint warnings/errors:
>>>
>>> dtschema/dtc warnings/errors:
>>> Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
>>
>> Looks like this is colliding with the example template which has to 
>> craft an interrupt provider for 'interrupts' properties. Either adding a 
>> parent node or using interrupts-extended instead should work-around it.
> Check the devicetree-org issue linked in the cover letter, please!
> 
> I suppose wrapping it in a parent node could work as a temporary
> measure, but since it belongs outside /soc, I'd have to make up
> a bogus simple-bus, I think.

I don't think your issue in dtschema is accurate. As Rob suggested, you
need wrapping node.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 6.04.2023 19:45, Krzysztof Kozlowski wrote:
> On 05/04/2023 15:49, Konrad Dybcio wrote:
>>
>>
>> On 5.04.2023 15:47, Rob Herring wrote:
>>> On Wed, Apr 05, 2023 at 07:22:40AM -0500, Rob Herring wrote:
>>>>
>>>> On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
>>>>>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>
>>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>>
>>>> yamllint warnings/errors:
>>>>
>>>> dtschema/dtc warnings/errors:
>>>> Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
>>>
>>> Looks like this is colliding with the example template which has to 
>>> craft an interrupt provider for 'interrupts' properties. Either adding a 
>>> parent node or using interrupts-extended instead should work-around it.
>> Check the devicetree-org issue linked in the cover letter, please!
>>
>> I suppose wrapping it in a parent node could work as a temporary
>> measure, but since it belongs outside /soc, I'd have to make up
>> a bogus simple-bus, I think.
> 
> I don't think your issue in dtschema is accurate. As Rob suggested, you
> need wrapping node.
I don't really know what kind.. I can add something like:

rpm {
	compatible = "qcom,rpm", "simple-mfd";

	mpm: interrupt-controller {
	...
};

And then only introduce a very simple YAML for "qcom,rpm"
describing what it is and documenting the compatible.

Or I can push it under rpm-requests{}.

Konrad
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Krzysztof Kozlowski]

On 06/04/2023 21:55, Konrad Dybcio wrote:
> 
> 
> On 6.04.2023 19:45, Krzysztof Kozlowski wrote:
>> On 05/04/2023 15:49, Konrad Dybcio wrote:
>>>
>>>
>>> On 5.04.2023 15:47, Rob Herring wrote:
>>>> On Wed, Apr 05, 2023 at 07:22:40AM -0500, Rob Herring wrote:
>>>>>
>>>>> On Wed, 05 Apr 2023 12:48:34 +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' deprecated.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/interrupt-controller/qcom,mpm.yaml   | 12 +++++++++---
>>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>
>>>>> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>>>>> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>>>>>
>>>>> yamllint warnings/errors:
>>>>>
>>>>> dtschema/dtc warnings/errors:
>>>>> Documentation/devicetree/bindings/interrupt-controller/qcom,mpm.example.dts:22.35-38.11: Warning (node_name_vs_property_name): /example-0/interrupt-controller: node name and property name conflict
>>>>
>>>> Looks like this is colliding with the example template which has to 
>>>> craft an interrupt provider for 'interrupts' properties. Either adding a 
>>>> parent node or using interrupts-extended instead should work-around it.
>>> Check the devicetree-org issue linked in the cover letter, please!
>>>
>>> I suppose wrapping it in a parent node could work as a temporary
>>> measure, but since it belongs outside /soc, I'd have to make up
>>> a bogus simple-bus, I think.
>>
>> I don't think your issue in dtschema is accurate. As Rob suggested, you
>> need wrapping node.
> I don't really know what kind.. I can add something like:
> 
> rpm {
> 	compatible = "qcom,rpm", "simple-mfd";
> 
> 	mpm: interrupt-controller {
> 	...
> };
> 
> And then only introduce a very simple YAML for "qcom,rpm"
> describing what it is and documenting the compatible.
> 
> Or I can push it under rpm-requests{}.

It does not matter really what kind of wrapper. Can be:

sram {
    interrupt-controller {

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Stephan Gerhold]

On Thu, Apr 06, 2023 at 09:55:40PM +0200, Konrad Dybcio wrote:
> [...]
> I don't really know what kind.. I can add something like:
> 
> rpm {
> 	compatible = "qcom,rpm", "simple-mfd";
> 
> 	mpm: interrupt-controller {
> 	...
> };
> 

IMO we should indeed add something like this, because the current
representation of the RPM below the top level /smd node is misleading.
"SMD" is not a device, bus, component or anything like that. It is just
the communication protocol. There should not be a top-level DT node for
this.

Instead there should be a dedicated device tree node for the RPM like in
your example above, which will allow adding properties and subnodes to
it as needed.

For unrelated reasons I actually have some patches for this, that switch
the /smd top-level node to a "remoteproc-like" node dedicated to the
RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
add additional (optional) properties like "resets" and "iommus" for the
RPM, but it would allow adding arbitrary subnodes as well:

https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e

I could finish those up and post them if that would help...

Thanks,
Stephan

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index dcbc5972248b22..1c24b01bd268c8 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -310,10 +310,10 @@
 		};
 	};
 
-	smd {
-		compatible = "qcom,smd";
+	rpm: remoteproc-rpm {
+		compatible = "qcom,msm8916-rpm-proc", "qcom,rpm-proc";
 
-		rpm {
+		smd-edge {
 			interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
 			qcom,ipc = <&apcs 8 0>;
 			qcom,smd-edge = <15>;

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 7.04.2023 13:36, Stephan Gerhold wrote:
> On Thu, Apr 06, 2023 at 09:55:40PM +0200, Konrad Dybcio wrote:
>> [...]
>> I don't really know what kind.. I can add something like:
>>
>> rpm {
>> 	compatible = "qcom,rpm", "simple-mfd";
>>
>> 	mpm: interrupt-controller {
>> 	...
>> };
>>
> 
> IMO we should indeed add something like this, because the current
> representation of the RPM below the top level /smd node is misleading.
> "SMD" is not a device, bus, component or anything like that. It is just
> the communication protocol. There should not be a top-level DT node for
> this.
> 
> Instead there should be a dedicated device tree node for the RPM like in
> your example above, which will allow adding properties and subnodes to
> it as needed.
> 
> For unrelated reasons I actually have some patches for this, that switch
> the /smd top-level node to a "remoteproc-like" node dedicated to the
> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
> add additional (optional) properties like "resets" and "iommus" for the
> RPM, but it would allow adding arbitrary subnodes as well:
> 
> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
> 
> I could finish those up and post them if that would help...
Krzysztof, what do you think?

On a note, the bindings check is gone with dtschema-2023.4

Konrad
> 
> Thanks,
> Stephan
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index dcbc5972248b22..1c24b01bd268c8 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -310,10 +310,10 @@
>  		};
>  	};
>  
> -	smd {
> -		compatible = "qcom,smd";
> +	rpm: remoteproc-rpm {
> +		compatible = "qcom,msm8916-rpm-proc", "qcom,rpm-proc";
>  
> -		rpm {
> +		smd-edge {
>  			interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
>  			qcom,ipc = <&apcs 8 0>;
>  			qcom,smd-edge = <15>;
> 
> 

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Krzysztof Kozlowski]

On 12/04/2023 13:47, Konrad Dybcio wrote:
>> For unrelated reasons I actually have some patches for this, that switch
>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>> add additional (optional) properties like "resets" and "iommus" for the
>> RPM, but it would allow adding arbitrary subnodes as well:
>>
>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>
>> I could finish those up and post them if that would help...
> Krzysztof, what do you think?

I don't know what is there in MSM8916 and how it should be represented.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>> For unrelated reasons I actually have some patches for this, that switch
>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>> add additional (optional) properties like "resets" and "iommus" for the
>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>
>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>
>>> I could finish those up and post them if that would help...
>> Krzysztof, what do you think?
> 
> I don't know what is there in MSM8916 and how it should be represented.
Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
which communicates over the SMD protocol (or G-LINK on >=8996).

The Qualcomm firmware loads the RPM fw blob and sets it up early in
the boot process, but msm8916-mainline folks managed to get TF-A
going and due to it being less.. invasive.. than the Qualcomm TZ,
RPM needs a bit more handling to be accessible.

The M3 core is wired up through the CNoC bus and we communicate
with it through the MSG RAM and the "APCS mailbox".

Konrad
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Krzysztof Kozlowski]

On 12/04/2023 14:09, Konrad Dybcio wrote:
> 
> 
> On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
>> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>>> For unrelated reasons I actually have some patches for this, that switch
>>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>>> add additional (optional) properties like "resets" and "iommus" for the
>>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>>
>>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>>
>>>> I could finish those up and post them if that would help...
>>> Krzysztof, what do you think?
>>
>> I don't know what is there in MSM8916 and how it should be represented.
> Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
> which communicates over the SMD protocol (or G-LINK on >=8996).
> 
> The Qualcomm firmware loads the RPM fw blob and sets it up early in
> the boot process, but msm8916-mainline folks managed to get TF-A
> going and due to it being less.. invasive.. than the Qualcomm TZ,
> RPM needs a bit more handling to be accessible.
> 
> The M3 core is wired up through the CNoC bus and we communicate
> with it through the MSG RAM and the "APCS mailbox".

Thanks, that's actually good description. Yet I still do not know what
is exactly the problem and the question. Linking some out of tree
commits does not give me the answer, at least I cannot get that answer
form the link.

For example what I don't understand is: why additional resources (like
resets) can be provided only in new binding, but not in the old.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 12.04.2023 18:53, Krzysztof Kozlowski wrote:
> On 12/04/2023 14:09, Konrad Dybcio wrote:
>>
>>
>> On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>>>> For unrelated reasons I actually have some patches for this, that switch
>>>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>>>> add additional (optional) properties like "resets" and "iommus" for the
>>>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>>>
>>>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>>>
>>>>> I could finish those up and post them if that would help...
>>>> Krzysztof, what do you think?
>>>
>>> I don't know what is there in MSM8916 and how it should be represented.
>> Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
>> which communicates over the SMD protocol (or G-LINK on >=8996).
>>
>> The Qualcomm firmware loads the RPM fw blob and sets it up early in
>> the boot process, but msm8916-mainline folks managed to get TF-A
>> going and due to it being less.. invasive.. than the Qualcomm TZ,
>> RPM needs a bit more handling to be accessible.
>>
>> The M3 core is wired up through the CNoC bus and we communicate
>> with it through the MSG RAM and the "APCS mailbox".
> 
> Thanks, that's actually good description. Yet I still do not know what
> is exactly the problem and the question. Linking some out of tree
> commits does not give me the answer, at least I cannot get that answer
> form the link.
> 
> For example what I don't understand is: why additional resources (like
> resets) can be provided only in new binding, but not in the old.
The old binding dictates that the rpm node (which in turn
holds all "devices" that only interface with RPM, like RPMCC) is
a child of smd{}, which does not make sense logically, as SMD is
a protocol (e.g. we don't place devices connected over i2c under
/i2c{}). The rpm node lacks a compatible, as it's representing
an "smd channel", so there's no driver so there's no way to assert
resets etc.

On newer SoCs that still implement SMD RPM (like 8996), we do
actually have a driver and a parent node which it binds to
(rpm-glink).

AFAIU:
In both cases, the "final" drivers (rpmcc, rpmpd..) are bound
after hitting a SMD/GLINK callback that tells Linux we're ready
to rock. That's an issue for Stephan, as these callbacks won't
ever happen if the RPM core is not initialized (and TF-A doesn't
do that).

Konrad
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Krzysztof Kozlowski]

On 12/04/2023 19:06, Konrad Dybcio wrote:
> 
> 
> On 12.04.2023 18:53, Krzysztof Kozlowski wrote:
>> On 12/04/2023 14:09, Konrad Dybcio wrote:
>>>
>>>
>>> On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
>>>> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>>>>> For unrelated reasons I actually have some patches for this, that switch
>>>>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>>>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>>>>> add additional (optional) properties like "resets" and "iommus" for the
>>>>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>>>>
>>>>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>>>>
>>>>>> I could finish those up and post them if that would help...
>>>>> Krzysztof, what do you think?
>>>>
>>>> I don't know what is there in MSM8916 and how it should be represented.
>>> Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
>>> which communicates over the SMD protocol (or G-LINK on >=8996).
>>>
>>> The Qualcomm firmware loads the RPM fw blob and sets it up early in
>>> the boot process, but msm8916-mainline folks managed to get TF-A
>>> going and due to it being less.. invasive.. than the Qualcomm TZ,
>>> RPM needs a bit more handling to be accessible.
>>>
>>> The M3 core is wired up through the CNoC bus and we communicate
>>> with it through the MSG RAM and the "APCS mailbox".
>>
>> Thanks, that's actually good description. Yet I still do not know what
>> is exactly the problem and the question. Linking some out of tree
>> commits does not give me the answer, at least I cannot get that answer
>> form the link.
>>
>> For example what I don't understand is: why additional resources (like
>> resets) can be provided only in new binding, but not in the old.
> The old binding dictates that the rpm node (which in turn
> holds all "devices" that only interface with RPM, like RPMCC) is
> a child of smd{}, which does not make sense logically, as SMD is
> a protocol (e.g. we don't place devices connected over i2c under
> /i2c{}).

We do. All devices connected over I2C are under i2c node which is the
controller. The example is different than what you have here...

>  The rpm node lacks a compatible, as it's representing
> an "smd channel", so there's no driver so there's no way to assert
> resets etc.

You have rpm-requests which has compatible. These are not its resources?

> 
> On newer SoCs that still implement SMD RPM (like 8996), we do
> actually have a driver and a parent node which it binds to
> (rpm-glink).

You want to add RPM resets to rpm-glink node? This also does not look right.

> 
> AFAIU:
> In both cases, the "final" drivers (rpmcc, rpmpd..) are bound
> after hitting a SMD/GLINK callback that tells Linux we're ready
> to rock. That's an issue for Stephan, as these callbacks won't
> ever happen if the RPM core is not initialized (and TF-A doesn't
> do that).

To me half or almost all of Qualcomm remote-proc-related bindings, like
SMD, GLINK and associated processors, are difficult to read, half-baked
and developed to match the current Linux/SW need. When the Linux drivers
changed, new bindings were added... If you want to fix it, sure go
ahead, but design everything to match something rational, not again to
match one specific SW/FW implementation.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: mpm: Pass MSG RAM slice through phandle

[Konrad Dybcio]

On 13.04.2023 10:50, Krzysztof Kozlowski wrote:
> On 12/04/2023 19:06, Konrad Dybcio wrote:
>>
>>
>> On 12.04.2023 18:53, Krzysztof Kozlowski wrote:
>>> On 12/04/2023 14:09, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 12.04.2023 13:55, Krzysztof Kozlowski wrote:
>>>>> On 12/04/2023 13:47, Konrad Dybcio wrote:
>>>>>>> For unrelated reasons I actually have some patches for this, that switch
>>>>>>> the /smd top-level node to a "remoteproc-like" node dedicated to the
>>>>>>> RPM, similar to how WCNSS/ADSP/Modem/etc are represented. I need this to
>>>>>>> add additional (optional) properties like "resets" and "iommus" for the
>>>>>>> RPM, but it would allow adding arbitrary subnodes as well:
>>>>>>>
>>>>>>> https://github.com/msm8916-mainline/linux/commit/35231ac28703805daa8220f1233847c7df34589e
>>>>>>>
>>>>>>> I could finish those up and post them if that would help...
>>>>>> Krzysztof, what do you think?
>>>>>
>>>>> I don't know what is there in MSM8916 and how it should be represented.
>>>> Similarly to other Qualcomm SoCs, MSM8916 has a RPM (Cortex-M3) core,
>>>> which communicates over the SMD protocol (or G-LINK on >=8996).
>>>>
>>>> The Qualcomm firmware loads the RPM fw blob and sets it up early in
>>>> the boot process, but msm8916-mainline folks managed to get TF-A
>>>> going and due to it being less.. invasive.. than the Qualcomm TZ,
>>>> RPM needs a bit more handling to be accessible.
>>>>
>>>> The M3 core is wired up through the CNoC bus and we communicate
>>>> with it through the MSG RAM and the "APCS mailbox".
>>>
>>> Thanks, that's actually good description. Yet I still do not know what
>>> is exactly the problem and the question. Linking some out of tree
>>> commits does not give me the answer, at least I cannot get that answer
>>> form the link.
>>>
>>> For example what I don't understand is: why additional resources (like
>>> resets) can be provided only in new binding, but not in the old.
>> The old binding dictates that the rpm node (which in turn
>> holds all "devices" that only interface with RPM, like RPMCC) is
>> a child of smd{}, which does not make sense logically, as SMD is
>> a protocol (e.g. we don't place devices connected over i2c under
>> /i2c{}).
> 
> We do. All devices connected over I2C are under i2c node which is the
> controller. The example is different than what you have here...
> 
>>  The rpm node lacks a compatible, as it's representing
>> an "smd channel", so there's no driver so there's no way to assert
>> resets etc.
> 
> You have rpm-requests which has compatible. These are not its resources?
I believe we misrepresented this 10y ago and now we're stuck with that
legacy..

Currently we have:

[1]
smd {
	rpm {
		rpm-requests {
			compatible = "qcom,rpm-msm8916"

or

[2]
rpm-glink {
	rpm-requests {
		compatible = "qcom,rpm-sm6375"


In the case of [1], 'smd' is a communication protocol and the 'rpm'
node describes the RPM's "smd edge" (think a communication channel
assigned to the RPM processor)


In the case of [2], rpm-glink is also just a description of the G-LINK
communication protocol/"bus" (putting bus in quotes, as GLINK is really a
very very fancy set of mailboxes)

So we've really been describing the protocols and not the hardware buses..

What Stephan and I were trying to say, is that there's no great node that
actually represents the Cortex-M3 RPM core itself.

The rpm-requests node is the closest, but it won't fit his purpose, as it 
depends on the communication with the CM3 already being active - it will
only get registered through qcom_glink_rx_open / qcom_channel_state_worker
for GLINK/SMD respectively. These channels will only be open if the core
is up, but for that to happen its reset line must be deasserted.

Stephen proposed restructuring that to be centered around the CM3 core and
not the communication protocol.

I know you're not very fond of downstream tree commits, but looking at his
branch, I think that's it:

https://github.com/msm8916-mainline/linux/commit/e4e90fd3f711295461ee17891567e75e2342e5c8

I'd be in favour of such restructurization - makes things much more clear
and sane.

Stephen, if you're willing to do it, I can test your patches on both GLINK
and SMD platforms.

> 
>>
>> On newer SoCs that still implement SMD RPM (like 8996), we do
>> actually have a driver and a parent node which it binds to
>> (rpm-glink).
> 
> You want to add RPM resets to rpm-glink node? This also does not look right.
No, I was just pointing out that rpm-requests' direct parent node has a
driver bound to it in case of GLINK but not in the case of SMD


> 
>>
>> AFAIU:
>> In both cases, the "final" drivers (rpmcc, rpmpd..) are bound
>> after hitting a SMD/GLINK callback that tells Linux we're ready
>> to rock. That's an issue for Stephan, as these callbacks won't
>> ever happen if the RPM core is not initialized (and TF-A doesn't
>> do that).
> 
> To me half or almost all of Qualcomm remote-proc-related bindings, like
> SMD, GLINK and associated processors, are difficult to read, half-baked
> and developed to match the current Linux/SW need.
Agreed :/

When the Linux drivers
> changed, new bindings were added... If you want to fix it, sure go
> ahead, but design everything to match something rational, not again to
> match one specific SW/FW implementation.
I don't think it's worth the hassle.. we may add it to the "we'll fix it
when we eventually find some gamebreaking issue that requires us to break
the 10yo backwards compatibility for some deep core driver, if that happens"
list..

Konrad
> 
> Best regards,
> Krzysztof
> 

[PATCH v2 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 | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
index d30614661eea..ee5f39a4a42a 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 = devm_ioremap(dev, 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);

-- 
2.40.0

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

[Shawn Guo]

On Wed, Apr 05, 2023 at 12:48:35PM +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>

Acked-by: Shawn Guo <shawn.guo@linaro.org>