| 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
| ||
|
Message-ID: <19e3d0a1-02e3-44bf-ae42-cd090bfde471@kernel.org>
Date: Mon, 16 Sep 2024 09:04:58 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: arturs.artamonovs@...log.com, Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Greg Malysa <greg.malysa@...esys.com>,
Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Utsav Agarwal <Utsav.Agarwal@...log.com>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, Thomas Gleixner <tglx@...utronix.de>,
Andi Shyti <andi.shyti@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Olof Johansson <olof@...om.net>, soc@...nel.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-serial@...r.kernel.org, adsp-linux@...log.com,
Nathan Barrett-Morrison <nathan.morrison@...esys.com>
Subject: Re: [PATCH 19/21] arm64: dts: adi: sc598: add device tree
On 12/09/2024 20:25, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@...log.com>
>
> Add ADI SC598-EZKIT device tree.
> Support UART console as output.
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@...log.com>
> Signed-off-by: Utsav Agarwal <Utsav.Agarwal@...log.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> Co-developed-by: Greg Malysa <greg.malysa@...esys.com>
> Signed-off-by: Greg Malysa <greg.malysa@...esys.com>
> ---
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/adi/Makefile | 2 +
> arch/arm64/boot/dts/adi/sc598-som-ezkit.dts | 14 ++
> arch/arm64/boot/dts/adi/sc598-som.dtsi | 58 +++++
> arch/arm64/boot/dts/adi/sc59x-64.dtsi | 367 ++++++++++++++++++++++++++++
> 5 files changed, 442 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 21cd3a87f385309c3a655a67a3bee5f0abed7545..9b3996a8e01d8e7d264c44c075d7a50ee350ba44 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> subdir-y += actions
> +subdir-y += adi
> subdir-y += airoha
> subdir-y += allwinner
> subdir-y += altera
> diff --git a/arch/arm64/boot/dts/adi/Makefile b/arch/arm64/boot/dts/adi/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..1bf54bc97095e1ea3577953d379746fbc0ea02a9
> --- /dev/null
> +++ b/arch/arm64/boot/dts/adi/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_SC59X_64) += sc598-som-ezkit.dtb
> diff --git a/arch/arm64/boot/dts/adi/sc598-som-ezkit.dts b/arch/arm64/boot/dts/adi/sc598-som-ezkit.dts
> new file mode 100644
> index 0000000000000000000000000000000000000000..a8db6d5ea764f917faa6839d3d4f0b5217b927b8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/adi/sc598-som-ezkit.dts
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2021-2024 - Analog Devices Inc.
> + * Author: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> + */
> +
> +/dts-v1/;
> +
> +#include "sc598-som.dtsi"
> +
> +/ {
> + model = "ADI 64-bit SC598 SOM EZ Kit";
> + compatible = "adi,sc598-som-ezkit", "adi,sc59x-64";
Where is adi,sc598-som-ezlite?
> +};
> diff --git a/arch/arm64/boot/dts/adi/sc598-som.dtsi b/arch/arm64/boot/dts/adi/sc598-som.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..3b90f367db1a24de1e1dddc4db3c219736c5b90f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/adi/sc598-som.dtsi
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2021-2024 - Analog Devices Inc.
> + * Author: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/adi-adsp.h>
> +#include "sc59x-64.dtsi"
> +
> +/ {
> + chosen {
> + stdout-path = &uart1;
> + bootargs = "earlycon=adi_uart,0x31003000 console=ttySC0,115200 mem=224M";
Drop entire bootargs. Look how other SoCs do it, if you need port speed.
> + };
> +
> + memory@...00000 {
> + device_type = "memory";
> + reg = <0x90000000 0x0e000000>;
> + };
> +
> + memory@...40000 {
> + device_type = "memory";
> + reg = <0x20040000 0x40000>;
> + };
> +
> + scb: scb-bus {
What is this?
> + sec: sec@...89000 {
And this?
> + adi,sharc-cores = <2>;
> + };
> + };
Drop entire node.
> +
Fix redundant blank lines.
> +};
> +
> +&uart0 {
> + pinctrl-0 = <&uart0_default>;
> + pinctrl-names = "default";
> + status = "okay";
> +};
> +
> +&i2c0 {
> + status = "okay";
> +};
> +
> +&i2c1 {
> + status = "disabled";
> +};
> +
> +&pinctrl0 {
> + uart0_default: uart0-default-pins {
> + pins {
> + pinmux = <ADI_ADSP_PINMUX('A', 6, ADI_ADSP_PINFUNC_ALT1)>,
> + <ADI_ADSP_PINMUX('A', 7, ADI_ADSP_PINFUNC_ALT1)>;
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/adi/sc59x-64.dtsi b/arch/arm64/boot/dts/adi/sc59x-64.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..4a9aa08b4acb0936c97e683562e05da063a4e193
> --- /dev/null
> +++ b/arch/arm64/boot/dts/adi/sc59x-64.dtsi
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2021-2024 - Analog Devices Inc.
> + * Author: Nathan Barrett-Morrison <nathan.morrison@...esys.com>
> + */
> +
> +#include <dt-bindings/clock/adi-sc5xx-clock.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + model = "ADI 64-bit SC59X";
> + compatible = "adi,sc59x-64";
> +
> + interrupt-parent = <&gic>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + chosen { };
Drop
> +
> + aliases {
> + serial0 = &uart0;
> + serial2 = &uart2;
> + serial3 = &uart3;
> + };
Drop or move to board DTS. Not a property of the SoC.
> +
> + cpus {
> + #address-cells = <0x2>;
> + #size-cells = <0x0>;
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a55";
> + reg = <0x0 0x0>;
> + enable-method = "spin-table";
> + cpu-release-addr = <0x0 0xdeadbeef>;
> + clocks = <&clk ADSP_SC598_CLK_ARM>, <&clk ADSP_SC598_CLK_DDR>;
> + };
> + };
> +
> + pmu {
Order nodes alphabetically. See DTS coding style.
> + compatible = "arm,armv8-pmuv3";
> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + };
> +
> + gic: interrupt-controller@...00000 {
This cannot be outside of SoC. See writing-bindings and DTS coding style.
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <3>;
> + interrupt-controller;
> + reg = <0x31200000 0x40000>, /* GIC Dist */
> + <0x31240000 0x40000>; /* GICR */
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, /* Physical Secure */
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, /* Physical Non-Secure */
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, /* Virtual */
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>; /* Hypervisor */
> + };
> +
> + clocks {
> + sys_clkin0: oscillator@1 {
There is no way you tested it. It's obvious W=1 warning.
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <25000000>;
> + clock-output-names = "sys_clkin0";
> + };
> +
> + sys_clkin1: oscillator@2 {
How are these properties of the SoC? Where are they located physically?
See DTS coding style.
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <25000000>;
> + clock-output-names = "sys_clkin1";
> + };
> + };
> +
> + clk: clocks@...8d000 {
> + compatible = "adi,sc598-clocks";
> + reg = <0x3108d000 0x1000>,
> + <0x3108e000 0x1000>,
> + <0x3108f000 0x1000>,
> + <0x310a9000 0x1000>;
> + #clock-cells = <1>;
> + clocks = <&sys_clkin0>, <&sys_clkin1>;
> + clock-names = "sys_clkin0", "sys_clkin1";
> + status = "okay";
Drop... everywhere.
> + };
> +
> + scb: scb-bus {
What is scb-bus?
See DTS coding style or any other SoC. This is supposed to be just sco@
with proper unit address.
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + rcu: rcu@...8c000 {
> + compatible = "adi,reset-controller";
> + reg = <0x3108c000 0x1000>;
> + status = "okay";
Oh...
> + };
> +
> + sec: sec@...89000 {
Random order of nodes? See DTS coding style.
> + compatible = "adi,system-event-controller";
> + reg = <0x31089000 0x1000>;
> + adi,rcu = <&rcu>;
> + status = "okay";
> + };
> +
> + uart0: uart@...03000 {
Never tested.
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + compatible = "adi,uart";
> + reg = <0x31003000 0x40>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + interrupt-parent = <&gic>;
> + interrupt-names = "tx", "rx", "status";
> + interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> + adi,use-edbo;
> + status = "disabled";
> + };
> +
> + uart1: uart@...03400 {
> + compatible = "adi,uart";
> + reg = <0x31003400 0x40>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + interrupt-parent = <&gic>;
> + interrupt-names = "tx", "rx", "status";
> + interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
> + adi,use-edbo;
> + status = "disabled";
> + };
> +
> + uart2: uart@...03800 {
> + compatible = "adi,uart";
> + reg = <0x31003800 0x40>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + interrupt-parent = <&gic>;
> + interrupt-names = "tx", "rx", "status";
> + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
> + adi,use-edbo;
> + status = "disabled";
> + };
> +
> + uart3: uart@...03c00 {
> + compatible = "adi,uart";
> + reg = <0x31003C00 0x40>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + interrupt-parent = <&gic>;
> + interrupt-names = "tx", "rx", "status";
> + interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
> + adi,use-edbo;
> + status = "disabled";
> + };
> +
> + i2c0: twi@...01400 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Completely wrong order of properties. Please follow DTS coding style.
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,twi";
You cannot have generic compatibles in the SoC.
> + reg = <0x31001400 0xFF>;
I already commented on lower case hex.
> + interrupts = <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>;
> + clock-khz = <100>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + status = "disabled";
> + };
> +
> + i2c1: twi@...01500 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,twi";
> + reg = <0x31001500 0xFF>;
> + interrupts = <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;
> + clock-khz = <100>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + status = "disabled";
> + };
> +
> + i2c3: twi@...01000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,twi";
> + reg = <0x31001000 0xFF>;
> + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> + clock-khz = <100>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + status = "disabled";
> + };
> +
> + i2c4: twi@...01100 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,twi";
> + reg = <0x31001100 0xFF>;
> + interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>;
> + clock-khz = <100>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + status = "disabled";
> + };
> +
> + i2c5: twi@...01200 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,twi";
> + reg = <0x31001200 0xFF>;
> + interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
> + clock-khz = <100>;
> + clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> + clock-names = "sclk0";
> + status = "disabled";
> + };
> +
> + pinctrl0: pinctrl@...04600 {
> + compatible = "adi,adsp-pinctrl";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x31004600 0x400>;
> + adi,port-sizes = <16 16 16 16 16 16 16 16 7>;
> + };
> +
> + pint0: pint@...05000 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005000 0xFF>;
> + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint1: pint@...05100 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005100 0xFF>;
> + interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint2: pint@...05200 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "adi,adsp-pint";
> + reg = <0x31005200 0xFF>;
> + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint3: pint@...05300 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005300 0xFF>;
> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint4: pint@...05400 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005400 0xFF>;
> + interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint5: pint@...05500 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005500 0xFF>;
> + interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint6: pint@...05600 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005600 0xFF>;
> + interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + pint7: pint@...05700 {
> + compatible = "adi,adsp-pint";
> + reg = <0x31005700 0xFF>;
> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + gpa: gport@...04000 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004000 0x7F>;
> + gpio-ranges = <&pinctrl0 0 0 16>;
> + adi,pint = <&pint0 1>;
> + status = "okay";
> + };
> +
> + gpb: gport@...04080 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004080 0x7F>;
> + gpio-ranges = <&pinctrl0 0 16 16>;
> + adi,pint = <&pint0 0>;
> + status = "okay";
> + };
> +
> + gpc: gport@...04100 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004100 0x7F>;
> + gpio-ranges = <&pinctrl0 0 32 16>;
> + adi,pint = <&pint2 1>;
> + status = "okay";
> + };
> +
> + gpd: gport@...04180 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004180 0x7F>;
> + gpio-ranges = <&pinctrl0 0 48 16>;
> + adi,pint = <&pint2 0>;
> + };
> +
> + gpe: gport@...04200 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004200 0x7F>;
> + gpio-ranges = <&pinctrl0 0 64 16>;
> + adi,pint = <&pint4 1>;
> + };
> +
> + gpf: gport@...04280 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004280 0x7F>;
> + gpio-ranges = <&pinctrl0 0 80 16>;
> + adi,pint = <&pint4 0>;
> + };
> +
> + gpg: gport@...04300 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004300 0x7F>;
> + gpio-ranges = <&pinctrl0 0 96 16>;
> + adi,pint = <&pint6 1>;
> + };
> +
> + gph: gport@...04380 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004380 0x7F>;
> + gpio-ranges = <&pinctrl0 0 112 16>;
> + adi,pint = <&pint6 0>;
> + };
> +
> + gpi: gport@...04400 {
> + compatible = "adi,adsp-port-gpio";
> + gpio-controller;
> + #gpio-cells = <2>;
> + reg = <0x31004400 0x7F>;
> + gpio-ranges = <&pinctrl0 0 128 7>;
> + adi,pint = <&pint7 1>;
> + };
> +
All your patches have such sloppy blank lines here and there.
> + };
> +};
>
Best regards,
Krzysztof
Powered by blists - more mailing lists