| Message ID | 20230328-topic-msgram_mpm-v2-1-e24a48e57f0d@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Resolve MPM register space situation | expand |
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 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 >
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>; };
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(-)