| Message ID | 20210928044546.4111223-1-bjorn.andersson@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/3] dt-bindings: soc: smem: Make indirection optional | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > In the olden days the Qualcomm shared memory (SMEM) region consisted of > multiple chunks of memory, so SMEM was described as a standalone node > with references to its various memory regions. > > But practically all modern Qualcomm platforms has a single reserved memory > region used for SMEM. So rather than having to use two nodes to describe > the one SMEM region, update the binding to allow the reserved-memory > region alone to describe SMEM. > > The olden format is preserved as valid, as this is widely used already. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > index f7e17713b3d8..4149cf2b66be 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > [...] > @@ -43,6 +55,20 @@ examples: > #size-cells = <1>; > ranges; > > + smem@fa00000 { I think this is a good opportunity to make a decision which node name should be used here. :) You use smem@ here but mentioned before that you think using the generic memory@ would be better [1]. And you use memory@ in PATCH 3/3: - smem_mem: memory@86000000 { + memory@86000000 { + compatible = "qcom,smem"; reg = <0x0 0x86000000 0 0x200000>; no-map; + hwlocks = <&tcsr_mutex 3>; }; However, if you would use memory@ as example in this DT schema, Rob's bot would complain with the same error that I mentioned earlier [2]: soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property From schema: dtschema/schemas/memory.yaml We should either fix the error when using memory@ or start using some different node name (Stephen Boyd suggested shared-memory@ for example). Otherwise we'll just keep introducing more and more dtbs_check errors for the Qualcomm device trees. Thanks, Stephan [1]: https://lore.kernel.org/linux-arm-msm/YUo0suaIugOco1Vu@builder.lan/ [2]: https://lore.kernel.org/linux-arm-msm/YUo2ZzQktf2iSec%2F@gerhold.net/
On Mon, 27 Sep 2021 21:45:44 -0700, Bjorn Andersson wrote: > In the olden days the Qualcomm shared memory (SMEM) region consisted of > multiple chunks of memory, so SMEM was described as a standalone node > with references to its various memory regions. > > But practically all modern Qualcomm platforms has a single reserved memory > region used for SMEM. So rather than having to use two nodes to describe > the one SMEM region, update the binding to allow the reserved-memory > region alone to describe SMEM. > > The olden format is preserved as valid, as this is widely used already. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > 1 file changed, 30 insertions(+), 4 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/soc/qcom/qcom,smem.example.dt.yaml:0:0: /example-1/soc/sram@fc428000: failed to match any schema with compatible: ['qcom,rpm-msg-ram'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1533702 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > multiple chunks of memory, so SMEM was described as a standalone node > > with references to its various memory regions. > > > > But practically all modern Qualcomm platforms has a single reserved memory > > region used for SMEM. So rather than having to use two nodes to describe > > the one SMEM region, update the binding to allow the reserved-memory > > region alone to describe SMEM. > > > > The olden format is preserved as valid, as this is widely used already. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > index f7e17713b3d8..4149cf2b66be 100644 > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > [...] > > @@ -43,6 +55,20 @@ examples: > > #size-cells = <1>; > > ranges; > > > > + smem@fa00000 { > > I think this is a good opportunity to make a decision which node name > should be used here. :) reserved-memory node names are kind of a mess, so I haven't tried for any standard... It needs to be solved globally. > > You use smem@ here but mentioned before that you think using the generic > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > - smem_mem: memory@86000000 { > + memory@86000000 { > + compatible = "qcom,smem"; > reg = <0x0 0x86000000 0 0x200000>; > no-map; > + hwlocks = <&tcsr_mutex 3>; > }; > > However, if you would use memory@ as example in this DT schema, > Rob's bot would complain with the same error that I mentioned earlier [2]: > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > From schema: dtschema/schemas/memory.yaml > > We should either fix the error when using memory@ or start using some > different node name (Stephen Boyd suggested shared-memory@ for example). > Otherwise we'll just keep introducing more and more dtbs_check errors > for the Qualcomm device trees. A different node name. A node name should only have 1 meaning and 'memory' is already defined. The main issue here is what to name nodes with only a size and no address. Rob
On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > multiple chunks of memory, so SMEM was described as a standalone node > > > with references to its various memory regions. > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > region used for SMEM. So rather than having to use two nodes to describe > > > the one SMEM region, update the binding to allow the reserved-memory > > > region alone to describe SMEM. > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > --- > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > index f7e17713b3d8..4149cf2b66be 100644 > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > [...] > > > @@ -43,6 +55,20 @@ examples: > > > #size-cells = <1>; > > > ranges; > > > > > > + smem@fa00000 { > > > > I think this is a good opportunity to make a decision which node name > > should be used here. :) > > reserved-memory node names are kind of a mess, so I haven't tried for > any standard... It needs to be solved globally. > I'd be happy to paint the shed any color you decide :) That said, the binding itself doesn't mandate any node name, so it's just the example here that would be "wrong" - and just as wrong as it currently is. > > > > You use smem@ here but mentioned before that you think using the generic > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > - smem_mem: memory@86000000 { > > + memory@86000000 { > > + compatible = "qcom,smem"; > > reg = <0x0 0x86000000 0 0x200000>; > > no-map; > > + hwlocks = <&tcsr_mutex 3>; > > }; > > > > However, if you would use memory@ as example in this DT schema, > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > From schema: dtschema/schemas/memory.yaml > > > > We should either fix the error when using memory@ or start using some > > different node name (Stephen Boyd suggested shared-memory@ for example). > > Otherwise we'll just keep introducing more and more dtbs_check errors > > for the Qualcomm device trees. > > A different node name. A node name should only have 1 meaning and > 'memory' is already defined. > > The main issue here is what to name nodes with only a size and no address. > This particular node has both address and size (as does all of the other reserved-memory regions we use upstream today)... Regards, Bjorn
On Tue, Sep 28, 2021 at 12:49 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > > > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > > multiple chunks of memory, so SMEM was described as a standalone node > > > > with references to its various memory regions. > > > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > > region used for SMEM. So rather than having to use two nodes to describe > > > > the one SMEM region, update the binding to allow the reserved-memory > > > > region alone to describe SMEM. > > > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > --- > > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > index f7e17713b3d8..4149cf2b66be 100644 > > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > [...] > > > > @@ -43,6 +55,20 @@ examples: > > > > #size-cells = <1>; > > > > ranges; > > > > > > > > + smem@fa00000 { > > > > > > I think this is a good opportunity to make a decision which node name > > > should be used here. :) > > > > reserved-memory node names are kind of a mess, so I haven't tried for > > any standard... It needs to be solved globally. > > > > I'd be happy to paint the shed any color you decide :) I didn't ask for it to be painted. Unless it is for everyone, I don't care unless there's some clear pattern used already. > That said, the binding itself doesn't mandate any node name, so it's > just the example here that would be "wrong" - and just as wrong as it > currently is. The example is right. The dts is wrong. Perhaps we need a schema for 'any node name that doesn't match already defined ones'. > > > You use smem@ here but mentioned before that you think using the generic > > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > > > - smem_mem: memory@86000000 { > > > + memory@86000000 { > > > + compatible = "qcom,smem"; > > > reg = <0x0 0x86000000 0 0x200000>; > > > no-map; > > > + hwlocks = <&tcsr_mutex 3>; > > > }; > > > > > > However, if you would use memory@ as example in this DT schema, > > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > > From schema: dtschema/schemas/memory.yaml > > > > > > We should either fix the error when using memory@ or start using some > > > different node name (Stephen Boyd suggested shared-memory@ for example). > > > Otherwise we'll just keep introducing more and more dtbs_check errors > > > for the Qualcomm device trees. > > > > A different node name. A node name should only have 1 meaning and > > 'memory' is already defined. > > > > The main issue here is what to name nodes with only a size and no address. > > > > This particular node has both address and size (as does all of the other > reserved-memory regions we use upstream today)... I'm not talking about *just* QCom. If we define something here, it's got to cover everyone. In summary, you can't use 'memory' or anything other established, standard node name. Rob
On Tue 28 Sep 14:51 CDT 2021, Rob Herring wrote: > On Tue, Sep 28, 2021 at 12:49 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Tue 28 Sep 12:34 CDT 2021, Rob Herring wrote: > > > > > On Tue, Sep 28, 2021 at 5:22 AM Stephan Gerhold <stephan@gerhold.net> wrote: > > > > > > > > On Mon, Sep 27, 2021 at 09:45:44PM -0700, Bjorn Andersson wrote: > > > > > In the olden days the Qualcomm shared memory (SMEM) region consisted of > > > > > multiple chunks of memory, so SMEM was described as a standalone node > > > > > with references to its various memory regions. > > > > > > > > > > But practically all modern Qualcomm platforms has a single reserved memory > > > > > region used for SMEM. So rather than having to use two nodes to describe > > > > > the one SMEM region, update the binding to allow the reserved-memory > > > > > region alone to describe SMEM. > > > > > > > > > > The olden format is preserved as valid, as this is widely used already. > > > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > > > --- > > > > > .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- > > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > index f7e17713b3d8..4149cf2b66be 100644 > > > > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml > > > > > [...] > > > > > @@ -43,6 +55,20 @@ examples: > > > > > #size-cells = <1>; > > > > > ranges; > > > > > > > > > > + smem@fa00000 { > > > > > > > > I think this is a good opportunity to make a decision which node name > > > > should be used here. :) > > > > > > reserved-memory node names are kind of a mess, so I haven't tried for > > > any standard... It needs to be solved globally. > > > > > > > I'd be happy to paint the shed any color you decide :) > > I didn't ask for it to be painted. Unless it is for everyone, I don't > care unless there's some clear pattern used already. > As Stephan indicated, I feel that I'll set precedence when I change "memory" -> "smem" in the last patch. > > That said, the binding itself doesn't mandate any node name, so it's > > just the example here that would be "wrong" - and just as wrong as it > > currently is. > > The example is right. The dts is wrong. > But I can't both not paint the node and resolve the fact that the dts is wrong. So which one should I go with? Should we leave the node name as is until we've decided what to do with the reserved-memory children? Or should I start accepting patches that changes "memory" to a list of non-generic names? > Perhaps we need a schema for 'any node name that doesn't match already > defined ones'. > > > > > You use smem@ here but mentioned before that you think using the generic > > > > memory@ would be better [1]. And you use memory@ in PATCH 3/3: > > > > > > > > - smem_mem: memory@86000000 { > > > > + memory@86000000 { > > > > + compatible = "qcom,smem"; > > > > reg = <0x0 0x86000000 0 0x200000>; > > > > no-map; > > > > + hwlocks = <&tcsr_mutex 3>; > > > > }; > > > > > > > > However, if you would use memory@ as example in this DT schema, > > > > Rob's bot would complain with the same error that I mentioned earlier [2]: > > > > > > > > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property > > > > From schema: dtschema/schemas/memory.yaml > > > > > > > > We should either fix the error when using memory@ or start using some > > > > different node name (Stephen Boyd suggested shared-memory@ for example). > > > > Otherwise we'll just keep introducing more and more dtbs_check errors > > > > for the Qualcomm device trees. > > > > > > A different node name. A node name should only have 1 meaning and > > > 'memory' is already defined. > > > > > > The main issue here is what to name nodes with only a size and no address. > > > > > > > This particular node has both address and size (as does all of the other > > reserved-memory regions we use upstream today)... > > I'm not talking about *just* QCom. If we define something here, it's > got to cover everyone. > > In summary, you can't use 'memory' or anything other established, > standard node name. > I know that "memory" is wrong, but I'm not sure about what you're asking me to do. Regards, Bjorn
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml index f7e17713b3d8..4149cf2b66be 100644 --- a/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,smem.yaml @@ -10,14 +10,18 @@ maintainers: - Andy Gross <agross@kernel.org> - Bjorn Andersson <bjorn.andersson@linaro.org> -description: | - This binding describes the Qualcomm Shared Memory Manager, used to share data - between various subsystems and OSes in Qualcomm platforms. +description: + This binding describes the Qualcomm Shared Memory Manager, a region of + reserved-memory used to share data between various subsystems and OSes in + Qualcomm platforms. properties: compatible: const: qcom,smem + reg: + maxItems: 1 + memory-region: maxItems: 1 description: handle to memory reservation for main SMEM memory region. @@ -29,11 +33,19 @@ properties: $ref: /schemas/types.yaml#/definitions/phandle description: handle to RPM message memory resource + no-map: true + required: - compatible - - memory-region - hwlocks +oneOf: + - required: + - reg + - no-map + - required: + - memory-region + additionalProperties: false examples: @@ -43,6 +55,20 @@ examples: #size-cells = <1>; ranges; + smem@fa00000 { + compatible = "qcom,smem"; + reg = <0xfa00000 0x200000>; + no-map; + + hwlocks = <&tcsr_mutex 3>; + }; + }; + - | + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + smem_region: smem@fa00000 { reg = <0xfa00000 0x200000>; no-map;
In the olden days the Qualcomm shared memory (SMEM) region consisted of multiple chunks of memory, so SMEM was described as a standalone node with references to its various memory regions. But practically all modern Qualcomm platforms has a single reserved memory region used for SMEM. So rather than having to use two nodes to describe the one SMEM region, update the binding to allow the reserved-memory region alone to describe SMEM. The olden format is preserved as valid, as this is widely used already. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- .../bindings/soc/qcom/qcom,smem.yaml | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-)