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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgvXdFZfWEzHF2Oy@yoga>
Date:   Tue, 15 Feb 2022 10:40:20 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Luca Weiss <luca@...tu.xyz>
Cc:     linux-arm-msm@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
        Vladimir Lypak <vladimir.lypak@...il.com>,
        Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/15] arm64: dts: qcom: Add MSM8953 device tree

On Wed 12 Jan 13:40 CST 2022, Luca Weiss wrote:

> From: Vladimir Lypak <vladimir.lypak@...il.com>
> 
> Add a base DT for MSM8953 SoC.
> 
> Co-developed-by: Luca Weiss <luca@...tu.xyz>
> Signed-off-by: Vladimir Lypak <vladimir.lypak@...il.com>
> Signed-off-by: Luca Weiss <luca@...tu.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?

> +
> +	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.

> +	};
[..]
> +		tcsr_mutex: hwlock@...5000 {
> +			compatible = "qcom,tcsr-mutex";
> +			reg = <0x1905000 0x20000>;
> +			#hwlock-cells = <1>;
> +		};
> +
> +		tcsr: syscon@...7000 {
> +			compatible = "qcom,tcsr-msm8953", "syscon";
> +			reg = <0x1937000 0x30000>;
> +		};
> +
> +		tcsr_phy_clk_scheme_sel: syscon@...f044 {

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?

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

Regards,
Bjorn