Re: [PATCH 10/15] arm64: dts: qcom: Add MSM8953 device tree

Luca Weiss
2022-02-19T11:51:00Z

Hi Bjorn,

On Dienstag, 15. Februar 2022 17:40:20 CET Bjorn Andersson wrote:
> On Wed 12 Jan 13:40 CST 2022, Luca Weiss wrote:
> > From: Vladimir Lypak <vladimir.lypak@gmail.com>
> >
> > Add a base DT for MSM8953 SoC.
> >
> > Co-developed-by: Luca Weiss <luca@z3ntu.xyz>
> > Signed-off-by: Vladimir Lypak <vladimir.lypak@gmail.com>
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> >
> > arch/arm64/boot/dts/qcom/msm8953.dtsi | 1337 +++++++++++++++++++++++++
> > 1 file changed, 1337 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/qcom/msm8953.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8953.dtsi
> > b/arch/arm64/boot/dts/qcom/msm8953.dtsi new file mode 100644
> > index 000000000000..59918b527750
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/msm8953.dtsi
> > @@ -0,0 +1,1337 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/* Copyright (c) 2022, The Linux Foundation. All rights reserved. */
> > +
> > +#include <dt-bindings/clock/qcom,gcc-msm8953.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/power/qcom-rpmpd.h>
> > +#include <dt-bindings/thermal/thermal.h>
> > +
> > +/ {
> > + interrupt-parent = <&intc>;
> > +
> > + #address-cells = <2>;
> > + #size-cells = <2>;
>
> Why do you have address/size-cells of 2, and then limit them to 1 in
> /soc?

Quite a few arm64 qcom dtsi files use #address-cells & #size-cells = <1> which
also makes reg properties quite more readable - and we don't need the 64 bits
on 8953.

>
> > +
> > + aliases {
> > + i2c1 = &i2c_1;
> > + i2c2 = &i2c_2;
> > + i2c3 = &i2c_3;
> > + i2c4 = &i2c_4;
> > + i2c5 = &i2c_5;
> > + i2c6 = &i2c_6;
> > + i2c7 = &i2c_7;
> > + i2c8 = &i2c_8;
>
> It was expressed a while back that you should specify alias only for the
> things that you have enabled in your .dts.

Sure, I'll remove it.

>
> > + };
>
> [..]
>
> > + tcsr_mutex: hwlock@1905000 {
> > + compatible = "qcom,tcsr-mutex";
> > + reg = <0x1905000 0x20000>;
> > + #hwlock-cells = <1>;
> > + };
> > +
> > + tcsr: syscon@1937000 {
> > + compatible = "qcom,tcsr-msm8953", "syscon";
> > + reg = <0x1937000 0x30000>;
> > + };
> > +
> > + tcsr_phy_clk_scheme_sel: syscon@193f044 {
>
> I don't fancy exposing a single word from the middle of &tcsr using a
> syscon. The tcsr node should express the TCSR region and if we need to
> reference bits of information within that we should do that in some
> structured way.
>
> Wouldn't nvmem be a good candidate for this?

The qusb2 bindings expect the reference like this as far as I can tell,

qphy->tcsr = syscon_regmap_lookup_by_phandle(dev->of_node,
"qcom,tcsr-syscon");

So I don't think we can change this without changing the driver as well?

Regards
Luca

>
> > + compatible = "syscon";
> > + reg = <0x193f044 0x4>;
> > + };
> > +
>
> Regards,
> Bjorn