| Message ID | 20230328-topic-msgram_mpm-v2-0-e24a48e57f0d@linaro.org |
|---|---|
| Headers | show |
| Series | Resolve MPM register space situation | expand |
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 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>
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 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
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,