[PATCH 1/3] arm64: dts: qcom: msm8916: Add unit name for /soc node

[PATCH 1/3] arm64: dts: qcom: msm8916: Add unit name for /soc node

[Stephan Gerhold]

This fixes the following warning when building with W=1:
Warning (unit_address_vs_reg): /soc: node has a reg or ranges property,
but no unit name

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 008b98fe8c6b..5551dba2d5fd 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -414,7 +414,7 @@ wcnss_smsm: wcnss@6 {
 		};
 	};
 
-	soc: soc {
+	soc: soc@0 {
 		#address-cells = <1>;
 		#size-cells = <1>;
 		ranges = <0 0 0 0xffffffff>;
-- 
2.33.0

[PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Stephan Gerhold]

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 {
 			reg = <0x0 0x86300000 0x0 0x100000>;
 			no-map;
 		};
-- 
2.33.0

[PATCH 3/3] arm64: dts: qcom: msm8916: Add "qcom,msm8916-sdhci" compatible

[Stephan Gerhold]

According to Documentation/devicetree/bindings/mmc/sdhci-msm.txt
a SoC specific compatible should be used in addition to the IP version
compatible, but for some reason it was never added for MSM8916.

Add the "qcom,msm8916-sdhci" compatible additionally to make the
device tree match the documented bindings.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 arch/arm64/boot/dts/qcom/msm8916.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 95dea20cde75..5879be0805b6 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1420,7 +1420,7 @@ lpass_codec: audio-codec@771c000 {
 		};
 
 		sdhc_1: sdhci@7824000 {
-			compatible = "qcom,sdhci-msm-v4";
+			compatible = "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4";
 			reg = <0x07824900 0x11c>, <0x07824000 0x800>;
 			reg-names = "hc_mem", "core_mem";
 
@@ -1438,7 +1438,7 @@ sdhc_1: sdhci@7824000 {
 		};
 
 		sdhc_2: sdhci@7864000 {
-			compatible = "qcom,sdhci-msm-v4";
+			compatible = "qcom,msm8916-sdhci", "qcom,sdhci-msm-v4";
 			reg = <0x07864900 0x11c>, <0x07864000 0x800>;
 			reg-names = "hc_mem", "core_mem";
 
-- 
2.33.0

Re: [PATCH 1/3] arm64: dts: qcom: msm8916: Add unit name for /soc node

[Stephen Boyd]

Quoting Stephan Gerhold (2021-09-21 08:21:18)
> This fixes the following warning when building with W=1:
> Warning (unit_address_vs_reg): /soc: node has a reg or ranges property,
> but no unit name
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Stephen Boyd]

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

Re: [PATCH 3/3] arm64: dts: qcom: msm8916: Add "qcom,msm8916-sdhci" compatible

[Stephen Boyd]

Quoting Stephan Gerhold (2021-09-21 08:21:20)
> According to Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> a SoC specific compatible should be used in addition to the IP version
> compatible, but for some reason it was never added for MSM8916.
>
> Add the "qcom,msm8916-sdhci" compatible additionally to make the
> device tree match the documented bindings.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Bjorn Andersson]

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

Re: [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Stephan Gerhold]

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

Re: [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Stephen Boyd]

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.

Re: (subset) [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Bjorn Andersson]

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,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>