| 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: <6b6a7094-9f0e-dbc3-424a-54fd426e8243@linaro.org>
Date: Mon, 23 Jan 2023 09:20:12 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Li Chen <lchen@...arella.com>, Li Chen <me@...ux.beauty>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc: open list <linux-kernel@...r.kernel.org>,
"moderated list:ARM/Ambarella SoC support"
<linux-arm-kernel@...ts.infradead.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>
Subject: Re: [PATCH 15/15] arm64: dts: ambarella: introduce Ambarella s6lm SoC
On 23/01/2023 08:32, Li Chen wrote:
> This patch adds s6lm's devicetree.
>
> Signed-off-by: Li Chen <lchen@...arella.com>
> Change-Id: Idc7a04cc64a53b3296bc9fcf9dd3cd648ebefeae
> ---
> MAINTAINERS | 2 +
> .../boot/dts/ambarella/ambarella-s6lm.dtsi | 332 ++++++++++++++++++
> .../boot/dts/ambarella/s6lm_pineapple.dts | 29 ++
> 3 files changed, 363 insertions(+)
> create mode 100644 arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> create mode 100644 arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1360fe2f4236..d9d1033b6452 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1894,6 +1894,8 @@ ARM/Ambarella SoC support
> M: Li Chen <me@...ux.beauty>
> L: linux-arm-kernel@...ts.infradead.org (moderated for non-subscribers)
> S: Supported
> +F: arch/arm64/boot/dts/ambarella/ambarella-s6lm.dts
> +F: arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> F: Documentation/devicetree/bindings/arm/ambarella.yaml
> F: Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> F: Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> diff --git a/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi b/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> new file mode 100644
> index 000000000000..c3fa77b33e04
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Ambarella,Inc. - http://www.ambarella.com/
> + * Author: Cao Rongrong <rrcao@...arella.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
Drop boilerplate.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> + compatible = "ambarella,s6lm";
> + interrupt-parent = <&gic>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + aliases {
> + serial0 = &uart0;
> + nand = &nand0;
> + };
> +
> + chosen {
> + stdout-path = &uart0;
> + };
> +
> + /*
> + * the memory node will be overwritten in Amboot,
> + * here is just the default value.
> + */
> + memory {
> + device_type = "memory";
> + reg = <0x00200000 0x07e00000>; /* 126M */
> + };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + };
> +
> + psci {
> + compatible = "arm,psci-0.2";
> + method = "smc";
> + };
> +
> + pmu {
> + compatible = "arm,cortex-a53-pmu";
> + interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> + <0 5 0x4>,
??? Just compare these two lines above...
> + <0 6 0x4>,
> + <0 7 0x4>;
> + };
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + cpu@0 {
> + compatible = "arm,cortex-a53", "arm,armv8";
> + device_type = "cpu";
> + reg = <0x0>;
> + enable-method = "psci";
> + };
> +
> + cpu@1 {
> + compatible = "arm,cortex-a53", "arm,armv8";
> + device_type = "cpu";
> + reg = <0x1>;
> + enable-method = "psci";
> + };
> +
> + cpu@2 {
> + compatible = "arm,cortex-a53", "arm,armv8";
> + device_type = "cpu";
> + reg = <0x2>;
> + enable-method = "psci";
> + };
> +
> + cpu@3 {
> + compatible = "arm,cortex-a53", "arm,armv8";
> + device_type = "cpu";
> + reg = <0x3>;
> + enable-method = "psci";
> + };
> + };
> +
> + gic: interrupt-controller@...00000 {
> + compatible = "arm,gic-400";
reg should be second property.
> + #interrupt-cells = <3>;
> + #address-cells = <0>;
> + interrupt-controller;
> + reg = <0xf3001000 0x1000>, /* GIC Dist */
> + <0xf3002000 0x2000>, /* GIC CPU */
> + /* following are not used if no virtulization */
> + <0xf3004000 0x2000>, /* GIC VCPU Control */
> + <0xf3006000 0x2000>; /* GIC VCPU */
> + interrupts = <GIC_PPI 9 0xf04>; /* GIC Maintenence IRQ */
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13 0xf08>, /* Secure Phys IRQ */
> + <1 14 0xf08>, /* Non-secure Phys IRQ */
And you do not see here anything odd?
> + <1 11 0xf08>, /* Virt IRQ */
> + <1 10 0xf08>; /* Hyp IRQ */
> + };
> +
> + n_apb@...00000 { /* Non-Secure APB, but configurable */
No underscores in node names. Generic node names, so just "bus"
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xe4000000 0x01000000>;
> + ranges;
> +
> + uart0: uart@...00000 {
serial
> + compatible = "ambarella,uart";
> + reg = <0xe4000000 0x1000>;
> + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart0_pins>;
> + clocks = <&gclk_uart0>;
> + amb,ignore-fe;
> + status = "ok";
> + };
> + };
> +
> + s_apb@...00000 { /* Secure APB, but configurable */
bus
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xec000000 0x01000000>;
reg is second property
> + ranges;
> +
> + pinctrl: pinctrl@...c003000 {
> + compatible = "ambarella,pinctrl";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0xec003000 0x1000>,
> + <0xec004000 0x1000>,
> + <0xec005000 0x1000>,
> + <0xec000000 0x1000>;
reg is second
> + reg-names = "gpio0", "gpio1", "gpio2", "iomux";
> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> + amb,pull-regmap = <&scratchpad_syscon 0x14 0x30>;
> + amb,ds-regmap = <&rct_syscon>;
> +
> + gpio: gpio@0 {
> + reg = <0>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-ranges = <&pinctrl 0 0 86>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + uart0_pins: uart0@0 {
> + reg = <0>;
Why do you have unit addresses here? What is this?
> + amb,pinmux-ids = <0x100a 0x100b>;
This will need more explanation... your bindings are not ready for
review, so we will get to this later when you rewrite your bindings and
test them before sending.
> + };
> +
> + snand0_pins_a: snand0@0 {
> + reg = <0>;
> + amb,pinmux-ids = <0x2024 0x2025 0x202a 0x202b
> + 0x202c 0x202d>;
> + };
> +
> + snand0_pins_b: snand0@1 {
> + reg = <1>;
> + amb,pinmux-ids = <0x4036 0x4037 0x4038 0x4039
> + 0x403a 0x403b>;
> + };
> + };
> + };
> +
> + n_ahb@...00000 { /* Non-Secure AHB, but configurable */
bus
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xe0000000 0x01000000>;
reg is a second property.
> + ranges;
> +
> + cpuid_syscon: cpuid@...00000 {
> + compatible = "ambarella,cpuid", "syscon";
> + reg = <0xe0000000 0x1000>;
> + };
> +
> + nand0: nand@...02000 {
> + compatible = "ambarella,nand";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xe0002000 0x1000>;
ditto
> + interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>; /* fio_irq */
> + pinctrl-names = "default";
> + pinctrl-0 = <&snand0_pins_a>;
> + clocks = <&pll_out_enet>;
> + nand-on-flash-bbt;
> + /* amb,soft-ecc = <6>; */
Drop dead code
> + };
> +
> + scratchpad_syscon: scratchpad_syscon@...22000 {
> + compatible = "ambarella,scratchpad", "syscon";
> + reg = <0xe0022000 0x100>;
> + };
> + };
> +
> + rct@...80000 {
bus
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xed080000 0x1000>;
> + ranges;
> +
> + rct_syscon: rct_syscon@...80000 {
syscon
(no underscores in node names, generic node names)
> + compatible = "ambarella,rct", "syscon";
> + reg = <0xed080000 0x1000>;
> + };
> + };
> +
> + clocks {
> + compatible = "ambarella,clkpll-v0";
Nope, nope. Register proper clock controller.
> + #address-cells = <1>;
> + #size-cells = <1>;
Why? You do not have any children with unit addresses.
> +
> + /*
> + * This is a dummy clock, to be used as placeholder on other
> + * mux clocks when corresponding bits in register don't
> + * represent real parent clock.
> + */
> + gclk_dummy: gclk_dummy {
NAK. Register proper clock controller.
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <0>;
> + };
> +
> + /*
> + * Fixed 24MHz oscillator inputs to SoC
> + */
> + osc: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <24000000>;
This is actually the only clock which makes sense, but it is not a
property of SoC. Put it with the board DTS. At least frequency goes
there to note it.
> + clock-output-names = "osc";
> + };
> +
> + gclk_core: gclk-core {
NAK here and for all other weird nodes below
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "gclk_core";
> + amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;
> + };
> +
> + gclk_ddr: gclk-ddr {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "gclk_ddr";
> + amb,clk-regmap = <&rct_syscon 0x0dc 0x0e0 0x110 0x114 0x000 0x000>;
> + amb,fix-divider = <2>;
> + };
> +
> + gclk_cortex: gclk-cortex {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "gclk_cortex";
> + amb,clk-regmap = <&rct_syscon 0x264 0x268 0x26c 0x270 0x000 0x000>;
> + };
> +
> + gclk_axi: gclk-axi {
> + #clock-cells = <0>;
> + compatible = "fixed-factor-clock";
> + clocks = <&gclk_cortex>;
> + clock-output-names = "gclk_axi";
> + clock-mult = <1>;
> + clock-div = <3>;
> + };
> +
> + gclk_so: gclk-so {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "gclk_so";
> + assigned-clocks = <&gclk_so>;
> + assigned-clock-rates = <24000000>;
> + amb,clk-regmap = <&rct_syscon 0x024 0x028 0x11c 0x120 0x04c 0x030>;
> + amb,frac-mode;
> + };
> +
> + pll_out_vo2: pll-out-vo2 {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "pll_out_vo2";
> + assigned-clocks = <&pll_out_vo2>;
> + assigned-clock-rates = <24000000>;
> + amb,clk-regmap = <&rct_syscon 0x0c0 0x0c4 0x13c 0x140 0x0c8 0x000>;
> + amb,frac-mode;
> + };
> +
> + pll_out_sd: pll-out-sd {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "pll_out_sd";
> + amb,clk-regmap = <&rct_syscon 0x4ac 0x4b0 0x4b4 0x4b8 0x000 0x000>;
> + };
> + pll_out_enet: pll-out-enet {
> + #clock-cells = <0>;
> + compatible = "ambarella,pll-clock";
> + clocks = <&osc>;
> + clock-output-names = "pll_out_enet";
> + amb,clk-regmap = <&rct_syscon 0x520 0x524 0x528 0x52c 0x000 0x000>;
> + };
> + gclk_uart0: gclk-uart0 {
> + #clock-cells = <0>;
> + compatible = "ambarella,composite-clock";
> + clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> + clock-output-names = "gclk_uart0";
> + assigned-clocks = <&gclk_uart0>;
> + assigned-clock-parents = <&osc>;
> + assigned-clock-rates = <24000000>;
> + amb,mux-regmap = <&rct_syscon 0x1c8>;
> + amb,div-regmap = <&rct_syscon 0x038>;
> + amb,div-width = <24>;
> + amb,div-shift = <0>;
> + };
> + };
> +};
> diff --git a/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts b/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
> new file mode 100644
> index 000000000000..2d5d6d18e738
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2023 Ambarella,Inc. - http://www.ambarella.com/
> + * Author: Cao Rongrong <rrcao@...arella.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/dts-v1/;
> +
> +#include "ambarella-s6lm.dtsi"
> +
> +/ {
> + model = "Ambarella S6LM Pineapple Board";
> + compatible = "ambarella,s6lm";
Missing board compatible.
> +
> + chosen {
> + bootargs = "console=ttyS0 ubi.mtd=lnx root=ubi0:rootfs rw
> + rootfstype=ubifs init=/linuxrc";
Drop bootargs. These should not be needed in DTS. Otherwise - why do you
need them? Why bootloader cannot provide these?
> + };
> +
> + n_apb@...00000 {
> + i2c1: i2c@...09000 {
No, use override by phandle/label.
> + status = "ok";
> + };
> + };
> +};
Best regards,
Krzysztof
Powered by blists - more mailing lists