| Message ID | 20210921152120.6710-2-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] arm64: dts: qcom: msm8916: Add unit name for /soc node | expand |
Quoting Stephan Gerhold (2021-09-21 08:21:19) > Using underscores in device tree nodes is not very common. > Additionally, the _region suffix in "smem_region@..." is not really > useful since it's obvious that it describes a reserved memory region. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index 5551dba2d5fd..95dea20cde75 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -41,7 +41,7 @@ tz-apps@86000000 { > no-map; > }; > > - smem_mem: smem_region@86300000 { > + smem_mem: smem@86300000 { Shouldn't that be smem_mem: memory@86300000? Node names should be generic. > reg = <0x0 0x86300000 0x0 0x100000>; > no-map; > };
On Tue 21 Sep 13:20 CDT 2021, Stephen Boyd wrote: > Quoting Stephan Gerhold (2021-09-21 08:21:19) > > Using underscores in device tree nodes is not very common. > > Additionally, the _region suffix in "smem_region@..." is not really > > useful since it's obvious that it describes a reserved memory region. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > index 5551dba2d5fd..95dea20cde75 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > @@ -41,7 +41,7 @@ tz-apps@86000000 { > > no-map; > > }; > > > > - smem_mem: smem_region@86300000 { > > + smem_mem: smem@86300000 { > > Shouldn't that be smem_mem: memory@86300000? Node names should be > generic. > I agree, that seems better. In the meantime, I've picked patch 1 and 3. Regards, Bjorn > > reg = <0x0 0x86300000 0x0 0x100000>; > > no-map; > > };
On Tue, Sep 21, 2021 at 11:20:18AM -0700, Stephen Boyd wrote: > Quoting Stephan Gerhold (2021-09-21 08:21:19) > > Using underscores in device tree nodes is not very common. > > Additionally, the _region suffix in "smem_region@..." is not really > > useful since it's obvious that it describes a reserved memory region. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > index 5551dba2d5fd..95dea20cde75 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > @@ -41,7 +41,7 @@ tz-apps@86000000 { > > no-map; > > }; > > > > - smem_mem: smem_region@86300000 { > > + smem_mem: smem@86300000 { > > Shouldn't that be smem_mem: memory@86300000? Node names should be > generic. > The way I read it, the DT schema [1] and device tree specification [2] interprets the generic name recommendation a bit different here: > Following the generic-names recommended practice, node names should > reflect the purpose of the node (ie. "framebuffer" or "dma-pool"). "framebuffer" or "dma-pool" would also be "memory", yet they suggest a name reflecting the purpose instead. The purpose of the node is "smem", it's not just arbitrary "memory". However, my main problem with using memory@ here is that it actually causes new dtbs_check errors: apq8016-sbc.dt.yaml: memory@86000000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86400000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86500000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86680000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86700000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@867e0000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@86800000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@89300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@89900000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) apq8016-sbc.dt.yaml: memory@8ea00000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) Looks like it thinks this is a definition of physical memory now. I would rather not add more errors. :) Stephan [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml [2]: https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter3-devicenodes.rst#reserved-memory-child-nodes
Quoting Stephan Gerhold (2021-09-21 12:45:43) > On Tue, Sep 21, 2021 at 11:20:18AM -0700, Stephen Boyd wrote: > > Quoting Stephan Gerhold (2021-09-21 08:21:19) > > > Using underscores in device tree nodes is not very common. > > > Additionally, the _region suffix in "smem_region@..." is not really > > > useful since it's obvious that it describes a reserved memory region. > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > > --- > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > index 5551dba2d5fd..95dea20cde75 100644 > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > @@ -41,7 +41,7 @@ tz-apps@86000000 { > > > no-map; > > > }; > > > > > > - smem_mem: smem_region@86300000 { > > > + smem_mem: smem@86300000 { > > > > Shouldn't that be smem_mem: memory@86300000? Node names should be > > generic. > > > > The way I read it, the DT schema [1] and device tree specification [2] > interprets the generic name recommendation a bit different here: > > > Following the generic-names recommended practice, node names should > > reflect the purpose of the node (ie. "framebuffer" or "dma-pool"). > > "framebuffer" or "dma-pool" would also be "memory", yet they suggest > a name reflecting the purpose instead. The purpose of the node is > "smem", it's not just arbitrary "memory". I don't think most people know what 'smem' means. Maybe if the node name was 'shared-memory' it would be OK. > > However, my main problem with using memory@ here is that it actually > causes new dtbs_check errors: > > apq8016-sbc.dt.yaml: memory@86000000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86400000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86500000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86680000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86700000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@867e0000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@86800000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@89300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@89900000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > apq8016-sbc.dt.yaml: memory@8ea00000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml) > > Looks like it thinks this is a definition of physical memory now. > I would rather not add more errors. :) Got it. Doesn't seem right that the schema is checking for memory node names anywhere except for in the root of the tree. Rob? I also see that the reserved memory binding could do with some YAML conversion.
On Tue, 21 Sep 2021 17:21:19 +0200, Stephan Gerhold wrote: > Using underscores in device tree nodes is not very common. > Additionally, the _region suffix in "smem_region@..." is not really > useful since it's obvious that it describes a reserved memory region. > > Applied, thanks! [2/3] arm64: dts: qcom: msm8916: Drop underscore in node name commit: 9095d054851f73d0f3527f9d822f92673007df1e Best regards,
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index 5551dba2d5fd..95dea20cde75 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -41,7 +41,7 @@ tz-apps@86000000 { no-map; }; - smem_mem: smem_region@86300000 { + smem_mem: smem@86300000 { reg = <0x0 0x86300000 0x0 0x100000>; no-map; };
Using underscores in device tree nodes is not very common. Additionally, the _region suffix in "smem_region@..." is not really useful since it's obvious that it describes a reserved memory region. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)