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

  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(-)
  

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.
  

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
  

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
  

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
  

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
>
  

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
  

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>;
  

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>;
> 
>
  

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
  

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
>
  

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
  

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
>
  

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
  

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
>