| Message ID | 20240726110355.2181563-9-kevin_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce ASPEED AST27XX BMC SoC | expand |
On 26/07/2024 13:03, Kevin Chen wrote: > --- > arch/arm64/boot/dts/Makefile | 1 + > arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi | 217 ++++++++++++++++++++++ > 2 files changed, 218 insertions(+) > create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile > index 21cd3a87f385..c909c19dc5dd 100644 > --- a/arch/arm64/boot/dts/Makefile > +++ b/arch/arm64/boot/dts/Makefile > @@ -34,3 +34,4 @@ subdir-y += tesla > subdir-y += ti > subdir-y += toshiba > subdir-y += xilinx > +subdir-y += aspeed > diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > new file mode 100644 > index 000000000000..858ab95251e4 > --- /dev/null > +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > @@ -0,0 +1,217 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +#include <dt-bindings/clock/aspeed,ast2700-clk.h> > +#include <dt-bindings/reset/aspeed,ast2700-reset.h> > +#include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> > + > +/ { > + model = "Aspeed BMC"; Model of what? No, drop. > + compatible = "aspeed,ast2700"; Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-parent = <&gic>; > + > + aliases { > + serial12 = &uart12; Nope. Such aliases are board specific. > + }; > + > + cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cpu@0 { > + compatible = "arm,cortex-a35"; > + enable-method = "psci"; > + device_type = "cpu"; > + reg = <0>; > + d-cache-size = <0x8000>; > + d-cache-line-size = <64>; > + d-cache-sets = <128>; > + i-cache-size = <0x8000>; > + i-cache-line-size = <64>; > + i-cache-sets = <256>; > + next-level-cache = <&l2>; > + }; > + > + cpu@1 { > + compatible = "arm,cortex-a35"; > + enable-method = "psci"; > + device_type = "cpu"; > + reg = <1>; > + d-cache-size = <0x8000>; > + d-cache-line-size = <64>; > + d-cache-sets = <128>; > + i-cache-size = <0x8000>; > + i-cache-line-size = <64>; > + i-cache-sets = <256>; > + next-level-cache = <&l2>; > + }; > + > + cpu@2 { > + compatible = "arm,cortex-a35"; > + enable-method = "psci"; > + device_type = "cpu"; > + reg = <2>; > + d-cache-size = <0x8000>; > + d-cache-line-size = <64>; > + d-cache-sets = <128>; > + i-cache-size = <0x8000>; > + i-cache-line-size = <64>; > + i-cache-sets = <256>; > + next-level-cache = <&l2>; > + }; > + > + cpu@3 { > + compatible = "arm,cortex-a35"; > + enable-method = "psci"; > + device_type = "cpu"; > + reg = <3>; > + d-cache-size = <0x8000>; > + d-cache-line-size = <64>; > + d-cache-sets = <128>; > + i-cache-size = <0x8000>; > + i-cache-line-size = <64>; > + i-cache-sets = <256>; > + next-level-cache = <&l2>; > + }; > + > + l2: l2-cache0 { > + compatible = "cache"; > + cache-size = <0x80000>; > + cache-line-size = <64>; > + cache-sets = <1024>; > + cache-level = <2>; > + }; > + }; > + > + pmu { > + compatible = "arm,cortex-a35-pmu"; > + interrupt-parent = <&gic>; > + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; > + }; > + > + psci { Order the nodes according to DTS coding style. Fix all your patches: 1. To pass flawlessly checkpatch (you did not run it) 2. To pass dt_binding_check and dtbs_check (you did not run these) 3. To adhere to kernel coding style 4. To adhere to DTS coding style > + compatible = "arm,psci-1.0"; > + method = "smc"; > + }; > + > + gic: interrupt-controller@12200000 { Nope, this cannot be outside of SoC. > + compatible = "arm,gic-v3"; > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; > + #interrupt-cells = <3>; > + interrupt-controller; > + interrupt-parent = <&gic>; > + #redistributor-regions = <1>; > + reg = <0 0x12200000 0 0x10000>, //GICD > + <0 0x12280000 0 0x80000>, //GICR > + <0 0x40440000 0 0x1000>; //GICC Read DTS coding style and order this correctly. > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupt-parent = <&gic>; > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>; > + arm,cpu-registers-not-fw-configured; > + always-on; > + }; > + > + soc0: soc@10000000 { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + soc0_sram: sram@10000000 { > + compatible = "mmio-sram"; > + reg = <0x0 0x10000000 0x0 0x20000>; /* 128KiB SRAM on soc0 */ > + ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>; > + #address-cells = <2>; > + #size-cells = <2>; > + no-memory-wc; > + > + exported@0 { > + reg = <0 0x0 0 0x20000>; > + export; > + }; > + }; > + > + syscon0: syscon@12c02000 { > + compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd"; > + reg = <0x0 0x12c02000 0x0 0x1000>; > + ranges = <0x0 0x0 0 0x12c02000 0 0x1000>; > + #address-cells = <2>; > + #size-cells = <2>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + > + silicon-id@0 { > + compatible = "aspeed,ast2700-silicon-id", "aspeed,silicon-id"; > + reg = <0 0x0 0 0x4>; > + }; > + > + scu_ic0: interrupt-controller@1D0 { DTS coding style... > + #interrupt-cells = <1>; > + compatible = "aspeed,ast2700-scu-ic0"; > + reg = <0 0x1d0 0 0xc>; > + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + }; > + > + scu_ic1: interrupt-controller@1E0 { > + #interrupt-cells = <1>; > + compatible = "aspeed,ast2700-scu-ic1"; > + reg = <0 0x1e0 0 0xc>; > + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + }; > + > + soc0_rst: reset-controller@200 { > + reg = <0 0x200 0 0x40>; > + }; > + > + soc0_clk: clock-controller@240 { > + reg = <0 0x240 0 0x1c0>; > + }; > + }; > + > + }; > + > + soc1: soc@14000000 { Wait, what, to socs? > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + syscon1: syscon@14c02000 { > + compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd"; > + reg = <0x0 0x14c02000 0x0 0x1000>; > + ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>; > + #address-cells = <2>; > + #size-cells = <2>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + > + soc1_rst: reset-controller@200 { > + #reset-cells = <1>; > + }; > + > + soc1_clk: clock-controller@240 { > + reg = <0 0x240 0 0x1c0>; > + }; > + }; > + > + uart12: serial@14c33b00 { > + compatible = "ns16550a"; > + reg = <0x0 0x14c33b00 0x0 0x100>; > + reg-shift = <2>; > + reg-io-width = <4>; > + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; > + no-loopback-test; > + pinctrl-names = "default"; What is this? > + }; > + }; > +}; > + Best regards, Krzysztof
Hi Krzk, >> --- >> arch/arm64/boot/dts/Makefile | 1 + >> arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi | 217 ++++++++++++++++++++++ >> 2 files changed, 218 insertions(+) >> create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >> >> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile >> index 21cd3a87f385..c909c19dc5dd 100644 >> --- a/arch/arm64/boot/dts/Makefile >> +++ b/arch/arm64/boot/dts/Makefile >> @@ -34,3 +34,4 @@ subdir-y += tesla >> subdir-y += ti >> subdir-y += toshiba >> subdir-y += xilinx >> +subdir-y += aspeed >> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >> new file mode 100644 >> index 000000000000..858ab95251e4 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >> @@ -0,0 +1,217 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +#include <dt-bindings/clock/aspeed,ast2700-clk.h> >> +#include <dt-bindings/reset/aspeed,ast2700-reset.h> >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> >> + >> +/ { >> + model = "Aspeed BMC"; > >Model of what? No, drop. Can I change to "model = "AST2700 EVB"" > >> + compatible = "aspeed,ast2700"; > >Please run scripts/checkpatch.pl and fix reported warnings. Then please >run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. >Some warnings can be ignored, especially from --strict run, but the code >here looks like it needs a fix. Feel free to get in touch if the warning >is not clear. > > >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-parent = <&gic>; >> + >> + aliases { >> + serial12 = &uart12; > >Nope. Such aliases are board specific. Agree, I will move to ast2700-evb.dts. > >> + }; >> + >> + cpus { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + cpu@0 { >> + compatible = "arm,cortex-a35"; >> + enable-method = "psci"; >> + device_type = "cpu"; >> + reg = <0>; >> + d-cache-size = <0x8000>; >> + d-cache-line-size = <64>; >> + d-cache-sets = <128>; >> + i-cache-size = <0x8000>; >> + i-cache-line-size = <64>; >> + i-cache-sets = <256>; >> + next-level-cache = <&l2>; >> + }; >> + >> + cpu@1 { >> + compatible = "arm,cortex-a35"; >> + enable-method = "psci"; >> + device_type = "cpu"; >> + reg = <1>; >> + d-cache-size = <0x8000>; >> + d-cache-line-size = <64>; >> + d-cache-sets = <128>; >> + i-cache-size = <0x8000>; >> + i-cache-line-size = <64>; >> + i-cache-sets = <256>; >> + next-level-cache = <&l2>; >> + }; >> + >> + cpu@2 { >> + compatible = "arm,cortex-a35"; >> + enable-method = "psci"; >> + device_type = "cpu"; >> + reg = <2>; >> + d-cache-size = <0x8000>; >> + d-cache-line-size = <64>; >> + d-cache-sets = <128>; >> + i-cache-size = <0x8000>; >> + i-cache-line-size = <64>; >> + i-cache-sets = <256>; >> + next-level-cache = <&l2>; >> + }; >> + >> + cpu@3 { >> + compatible = "arm,cortex-a35"; >> + enable-method = "psci"; >> + device_type = "cpu"; >> + reg = <3>; >> + d-cache-size = <0x8000>; >> + d-cache-line-size = <64>; >> + d-cache-sets = <128>; >> + i-cache-size = <0x8000>; >> + i-cache-line-size = <64>; >> + i-cache-sets = <256>; >> + next-level-cache = <&l2>; >> + }; >> + >> + l2: l2-cache0 { >> + compatible = "cache"; >> + cache-size = <0x80000>; >> + cache-line-size = <64>; >> + cache-sets = <1024>; >> + cache-level = <2>; >> + }; >> + }; >> + >> + pmu { >> + compatible = "arm,cortex-a35-pmu"; >> + interrupt-parent = <&gic>; >> + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; >> + }; >> + >> + psci { > >Order the nodes according to DTS coding style. > >Fix all your patches: >1. To pass flawlessly checkpatch (you did not run it) >2. To pass dt_binding_check and dtbs_check (you did not run these) >3. To adhere to kernel coding style >4. To adhere to DTS coding style Agree. > >> + compatible = "arm,psci-1.0"; >> + method = "smc"; >> + }; >> + >> + gic: interrupt-controller@12200000 { > >Nope, this cannot be outside of SoC. Agree. > >> + compatible = "arm,gic-v3"; >> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; >> + #interrupt-cells = <3>; >> + interrupt-controller; >> + interrupt-parent = <&gic>; >> + #redistributor-regions = <1>; >> + reg = <0 0x12200000 0 0x10000>, //GICD >> + <0 0x12280000 0 0x80000>, //GICR >> + <0 0x40440000 0 0x1000>; //GICC > >Read DTS coding style and order this correctly. Agree. > >> + }; >> + >> + timer { >> + compatible = "arm,armv8-timer"; >> + interrupt-parent = <&gic>; >> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>; >> + arm,cpu-registers-not-fw-configured; >> + always-on; >> + }; >> + >> + soc0: soc@10000000 { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + soc0_sram: sram@10000000 { >> + compatible = "mmio-sram"; >> + reg = <0x0 0x10000000 0x0 0x20000>; /* 128KiB SRAM on soc0 */ >> + ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + no-memory-wc; >> + >> + exported@0 { >> + reg = <0 0x0 0 0x20000>; >> + export; >> + }; >> + }; >> + >> + syscon0: syscon@12c02000 { >> + compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd"; >> + reg = <0x0 0x12c02000 0x0 0x1000>; >> + ranges = <0x0 0x0 0 0x12c02000 0 0x1000>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + >> + silicon-id@0 { >> + compatible = "aspeed,ast2700-silicon-id", "aspeed,silicon-id"; >> + reg = <0 0x0 0 0x4>; >> + }; >> + >> + scu_ic0: interrupt-controller@1D0 { > >DTS coding style... Agree. I will fix to "scu_ic0: interrupt-controller@1d0 {" > >> + #interrupt-cells = <1>; >> + compatible = "aspeed,ast2700-scu-ic0"; >> + reg = <0 0x1d0 0 0xc>; >> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + }; >> + >> + scu_ic1: interrupt-controller@1E0 { >> + #interrupt-cells = <1>; >> + compatible = "aspeed,ast2700-scu-ic1"; >> + reg = <0 0x1e0 0 0xc>; >> + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + }; >> + >> + soc0_rst: reset-controller@200 { >> + reg = <0 0x200 0 0x40>; >> + }; >> + >> + soc0_clk: clock-controller@240 { >> + reg = <0 0x240 0 0x1c0>; >> + }; >> + }; >> + >> + }; >> + >> + soc1: soc@14000000 { > >Wait, what, to socs? Yes, two socs in different die. > >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + syscon1: syscon@14c02000 { >> + compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd"; >> + reg = <0x0 0x14c02000 0x0 0x1000>; >> + ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + >> + soc1_rst: reset-controller@200 { >> + #reset-cells = <1>; >> + }; >> + >> + soc1_clk: clock-controller@240 { >> + reg = <0 0x240 0 0x1c0>; >> + }; >> + }; >> + >> + uart12: serial@14c33b00 { >> + compatible = "ns16550a"; >> + reg = <0x0 0x14c33b00 0x0 0x100>; >> + reg-shift = <2>; >> + reg-io-width = <4>; >> + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; >> + no-loopback-test; >> + pinctrl-names = "default"; > >What is this? BMC uart using in uart12 in soc1. > >> + }; >> + }; >> +}; >> + -- Best Regards, Kevin. Chen
Hi Krzk, > > --- > > arch/arm64/boot/dts/Makefile | 1 + > > arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi | 217 > > ++++++++++++++++++++++ > > 2 files changed, 218 insertions(+) > > create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > > > > diff --git a/arch/arm64/boot/dts/Makefile > > b/arch/arm64/boot/dts/Makefile index 21cd3a87f385..c909c19dc5dd > 100644 > > --- a/arch/arm64/boot/dts/Makefile > > +++ b/arch/arm64/boot/dts/Makefile > > @@ -34,3 +34,4 @@ subdir-y += tesla > > subdir-y += ti > > subdir-y += toshiba > > subdir-y += xilinx > > +subdir-y += aspeed > > diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > > b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > > new file mode 100644 > > index 000000000000..858ab95251e4 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > > @@ -0,0 +1,217 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later #include > > +<dt-bindings/clock/aspeed,ast2700-clk.h> > > +#include <dt-bindings/reset/aspeed,ast2700-reset.h> > > +#include <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> > > + > > +/ { > > + model = "Aspeed BMC"; > > Model of what? No, drop. Can I change to "model = "AST2700 EVB"" > > > + compatible = "aspeed,ast2700"; > > Please run scripts/checkpatch.pl and fix reported warnings. Then please run > `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code here > looks like it needs a fix. Feel free to get in touch if the warning is not clear. > > > > + #address-cells = <2>; > > + #size-cells = <2>; > > + interrupt-parent = <&gic>; > > + > > + aliases { > > + serial12 = &uart12; > > Nope. Such aliases are board specific. Agree, I will move to ast2700-evb.dts. > > > + }; > > + > > + cpus { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + cpu@0 { > > + compatible = "arm,cortex-a35"; > > + enable-method = "psci"; > > + device_type = "cpu"; > > + reg = <0>; > > + d-cache-size = <0x8000>; > > + d-cache-line-size = <64>; > > + d-cache-sets = <128>; > > + i-cache-size = <0x8000>; > > + i-cache-line-size = <64>; > > + i-cache-sets = <256>; > > + next-level-cache = <&l2>; > > + }; > > + > > + cpu@1 { > > + compatible = "arm,cortex-a35"; > > + enable-method = "psci"; > > + device_type = "cpu"; > > + reg = <1>; > > + d-cache-size = <0x8000>; > > + d-cache-line-size = <64>; > > + d-cache-sets = <128>; > > + i-cache-size = <0x8000>; > > + i-cache-line-size = <64>; > > + i-cache-sets = <256>; > > + next-level-cache = <&l2>; > > + }; > > + > > + cpu@2 { > > + compatible = "arm,cortex-a35"; > > + enable-method = "psci"; > > + device_type = "cpu"; > > + reg = <2>; > > + d-cache-size = <0x8000>; > > + d-cache-line-size = <64>; > > + d-cache-sets = <128>; > > + i-cache-size = <0x8000>; > > + i-cache-line-size = <64>; > > + i-cache-sets = <256>; > > + next-level-cache = <&l2>; > > + }; > > + > > + cpu@3 { > > + compatible = "arm,cortex-a35"; > > + enable-method = "psci"; > > + device_type = "cpu"; > > + reg = <3>; > > + d-cache-size = <0x8000>; > > + d-cache-line-size = <64>; > > + d-cache-sets = <128>; > > + i-cache-size = <0x8000>; > > + i-cache-line-size = <64>; > > + i-cache-sets = <256>; > > + next-level-cache = <&l2>; > > + }; > > + > > + l2: l2-cache0 { > > + compatible = "cache"; > > + cache-size = <0x80000>; > > + cache-line-size = <64>; > > + cache-sets = <1024>; > > + cache-level = <2>; > > + }; > > + }; > > + > > + pmu { > > + compatible = "arm,cortex-a35-pmu"; > > + interrupt-parent = <&gic>; > > + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_HIGH)>; > > + }; > > + > > + psci { > > Order the nodes according to DTS coding style. > > Fix all your patches: > 1. To pass flawlessly checkpatch (you did not run it) 2. To pass > dt_binding_check and dtbs_check (you did not run these) 3. To adhere to > kernel coding style 4. To adhere to DTS coding style Agree. > > > + compatible = "arm,psci-1.0"; > > + method = "smc"; > > + }; > > + > > + gic: interrupt-controller@12200000 { > > Nope, this cannot be outside of SoC. Agree. > > > + compatible = "arm,gic-v3"; > > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_HIGH)>; > > + #interrupt-cells = <3>; > > + interrupt-controller; > > + interrupt-parent = <&gic>; > > + #redistributor-regions = <1>; > > + reg = <0 0x12200000 0 0x10000>, //GICD > > + <0 0x12280000 0 0x80000>, //GICR > > + <0 0x40440000 0 0x1000>; //GICC > > Read DTS coding style and order this correctly. Agree. > > > + }; > > + > > + timer { > > + compatible = "arm,armv8-timer"; > > + interrupt-parent = <&gic>; > > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | > IRQ_TYPE_LEVEL_LOW)>; > > + arm,cpu-registers-not-fw-configured; > > + always-on; > > + }; > > + > > + soc0: soc@10000000 { > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + > > + soc0_sram: sram@10000000 { > > + compatible = "mmio-sram"; > > + reg = <0x0 0x10000000 0x0 0x20000>; /* 128KiB SRAM on soc0 > */ > > + ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + no-memory-wc; > > + > > + exported@0 { > > + reg = <0 0x0 0 0x20000>; > > + export; > > + }; > > + }; > > + > > + syscon0: syscon@12c02000 { > > + compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd"; > > + reg = <0x0 0x12c02000 0x0 0x1000>; > > + ranges = <0x0 0x0 0 0x12c02000 0 0x1000>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + #clock-cells = <1>; > > + #reset-cells = <1>; > > + > > + silicon-id@0 { > > + compatible = "aspeed,ast2700-silicon-id", > "aspeed,silicon-id"; > > + reg = <0 0x0 0 0x4>; > > + }; > > + > > + scu_ic0: interrupt-controller@1D0 { > > DTS coding style... Agree. I will fix to "scu_ic0: interrupt-controller@1d0 {" > > > + #interrupt-cells = <1>; > > + compatible = "aspeed,ast2700-scu-ic0"; > > + reg = <0 0x1d0 0 0xc>; > > + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-controller; > > + }; > > + > > + scu_ic1: interrupt-controller@1E0 { > > + #interrupt-cells = <1>; > > + compatible = "aspeed,ast2700-scu-ic1"; > > + reg = <0 0x1e0 0 0xc>; > > + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-controller; > > + }; > > + > > + soc0_rst: reset-controller@200 { > > + reg = <0 0x200 0 0x40>; > > + }; > > + > > + soc0_clk: clock-controller@240 { > > + reg = <0 0x240 0 0x1c0>; > > + }; > > + }; > > + > > + }; > > + > > + soc1: soc@14000000 { > > Wait, what, to socs? Yes. In AST2700, there are two socs with different base address for use. > > > + compatible = "simple-bus"; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + > > + syscon1: syscon@14c02000 { > > + compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd"; > > + reg = <0x0 0x14c02000 0x0 0x1000>; > > + ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + #clock-cells = <1>; > > + #reset-cells = <1>; > > + > > + soc1_rst: reset-controller@200 { > > + #reset-cells = <1>; > > + }; > > + > > + soc1_clk: clock-controller@240 { > > + reg = <0 0x240 0 0x1c0>; > > + }; > > + }; > > + > > + uart12: serial@14c33b00 { > > + compatible = "ns16550a"; > > + reg = <0x0 0x14c33b00 0x0 0x100>; > > + reg-shift = <2>; > > + reg-io-width = <4>; > > + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; > > + no-loopback-test; > > + pinctrl-names = "default"; > > What is this? BMC UART is used in uart12 in soc1. > > > + }; > > + }; > > +}; > > + > > Best regards, > Krzysztof -- Best Regards, Kevin.Chen ************* Email Confidentiality Notice ******************** 免責聲明: 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作! DISCLAIMER: This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
On 15/08/2024 07:50, Kevin Chen wrote: >>> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >>> new file mode 100644 >>> index 000000000000..858ab95251e4 >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >>> @@ -0,0 +1,217 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later >>> +#include <dt-bindings/clock/aspeed,ast2700-clk.h> >>> +#include <dt-bindings/reset/aspeed,ast2700-reset.h> >>> +#include <dt-bindings/interrupt-controller/arm-gic.h> >>> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> >>> + >>> +/ { >>> + model = "Aspeed BMC"; >> >> Model of what? No, drop. > Can I change to "model = "AST2700 EVB"" Model of what? No, it does not make sense here. .. >>> + >>> + uart12: serial@14c33b00 { >>> + compatible = "ns16550a"; >>> + reg = <0x0 0x14c33b00 0x0 0x100>; >>> + reg-shift = <2>; >>> + reg-io-width = <4>; >>> + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; >>> + no-loopback-test; >>> + pinctrl-names = "default"; >> >> What is this? > BMC uart using in uart12 in soc1. No, that line. pinctrl-names do not make sense here without values. > >> >>> + }; >>> + }; >>> +}; >>> + > > -- > Best Regards, > Kevin. Chen > ________________________________ > 寄件者: Krzysztof Kozlowski <krzk@kernel.org> > 寄件日期: 2024年7月26日 下午 07:19 > 收件者: Kevin Chen <kevin_chen@aspeedtech.com>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; joel@jms.id.au <joel@jms.id.au>; andrew@codeconstruct.com.au <andrew@codeconstruct.com.au>; lee@kernel.org <lee@kernel.org>; catalin.marinas@arm.com <catalin.marinas@arm.com>; will@kernel.org <will@kernel.org>; arnd@arndb.de <arnd@arndb.de>; olof@lixom.net <olof@lixom.net>; soc@kernel.org <soc@kernel.org>; mturquette@baylibre.com <mturquette@baylibre.com>; sboyd@kernel.org <sboyd@kernel.org>; p.zabel@pengutronix.de <p.zabel@pengutronix.de>; quic_bjorande@quicinc.com <quic_bjorande@quicinc.com>; geert+renesas@glider.be <geert+renesas@glider.be>; dmitry.baryshkov@linaro.org <dmitry.baryshkov@linaro.org>; shawnguo@kernel.org <shawnguo@kernel.org>; neil.armstrong@linaro.org <neil.armstrong@linaro.org>; m.szyprowski@samsung.com <m.szyprowski@samsung.com>; nfraprado@collabora.com <nfraprado@collabora.com>; u-kumar1@ti.com <u-kumar1@ti.com>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-clk@vger.kernel.org <linux-clk@vger.kernel.org> > 主旨: Re: [PATCH v1 08/10] arm64: dts: aspeed: Add initial AST27XX device tree > Why do you quote my email twice? ... >> + uart12: serial@14c33b00 { >> + compatible = "ns16550a"; >> + reg = <0x0 0x14c33b00 0x0 0x100>; >> + reg-shift = <2>; >> + reg-io-width = <4>; >> + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; >> + no-loopback-test; >> + pinctrl-names = "default"; > > What is this? > >> + }; >> + }; >> +}; >> + > > Best regards, > Krzysztof > > ************* Email Confidentiality Notice ******************** > 免責聲明: > 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作! > > DISCLAIMER: > This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you. Maybe I am the intended recipient of your message, maybe not. I don't want to have any legal questions regarding upstream, public collaboration, thus probably I should just remove your messages. Please talk with your IT that such disclaimers in open-source are not desired (and maybe even harmful). If you do not understand why, please also see: https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835 If you need to go around company SMTP server, then consider using b4 web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html Please be informed that by responding to this email you agree that all communications from you and/or your company is made public. In other words, all messages originating from you and/or your company will be made public. Best regards, Krzysztof
> >>> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > >>> b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > >>> new file mode 100644 > >>> index 000000000000..858ab95251e4 > >>> --- /dev/null > >>> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi > >>> @@ -0,0 +1,217 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-or-later #include > >>> +<dt-bindings/clock/aspeed,ast2700-clk.h> > >>> +#include <dt-bindings/reset/aspeed,ast2700-reset.h> > >>> +#include <dt-bindings/interrupt-controller/arm-gic.h> > >>> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> > >>> + > >>> +/ { > >>> + model = "Aspeed BMC"; > >> > >> Model of what? No, drop. > > Can I change to "model = "AST2700 EVB"" > > Model of what? No, it does not make sense here. Should I change to model = "Aspeed BMC SoC"; > > > .. > > > >>> + > >>> + uart12: serial@14c33b00 { > >>> + compatible = "ns16550a"; > >>> + reg = <0x0 0x14c33b00 0x0 0x100>; > >>> + reg-shift = <2>; > >>> + reg-io-width = <4>; > >>> + clocks = <&syscon1 > SCU1_CLK_GATE_UART12CLK>; > >>> + no-loopback-test; > >>> + pinctrl-names = "default"; > >> > >> What is this? > > BMC uart using in uart12 in soc1. > > No, that line. pinctrl-names do not make sense here without values. Agree, I will remove the line of pinctrl-names in next patch. Pinctrl property needed to be defined in the future patches. > > > > >> > >>> + }; > >>> + }; > >>> +}; > >>> + > > > > -- > > Best Regards, > > Kevin. Chen > > > > Why do you quote my email twice? > > > ... > > >> + uart12: serial@14c33b00 { > >> + compatible = "ns16550a"; > >> + reg = <0x0 0x14c33b00 0x0 0x100>; > >> + reg-shift = <2>; > >> + reg-io-width = <4>; > >> + clocks = <&syscon1 > SCU1_CLK_GATE_UART12CLK>; > >> + no-loopback-test; > >> + pinctrl-names = "default"; > > > > What is this? > > > >> + }; > >> + }; > >> +}; > >> + > > > > Best regards, > > Krzysztof > > > other confidential information. If you have received it in error, please notify the > sender by reply e-mail and immediately delete the e-mail and any > attachments without copying or disclosing the contents. Thank you. > > Maybe I am the intended recipient of your message, maybe not. I don't want to > have any legal questions regarding upstream, public collaboration, thus > probably I should just remove your messages. > > Please talk with your IT that such disclaimers in open-source are not desired > (and maybe even harmful). > If you do not understand why, please also see: > https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835 > > If you need to go around company SMTP server, then consider using b4 > web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html > > Please be informed that by responding to this email you agree that all > communications from you and/or your company is made public. In other words, > all messages originating from you and/or your company will be made public. > > Best regards, > Krzysztof
On 07/10/2024 11:26, Kevin Chen wrote: >>>>> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >>>>> b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >>>>> new file mode 100644 >>>>> index 000000000000..858ab95251e4 >>>>> --- /dev/null >>>>> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi >>>>> @@ -0,0 +1,217 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later #include >>>>> +<dt-bindings/clock/aspeed,ast2700-clk.h> >>>>> +#include <dt-bindings/reset/aspeed,ast2700-reset.h> >>>>> +#include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> >>>>> + >>>>> +/ { >>>>> + model = "Aspeed BMC"; >>>> >>>> Model of what? No, drop. >>> Can I change to "model = "AST2700 EVB"" >> >> Model of what? No, it does not make sense here. > Should I change to model = "Aspeed BMC SoC"; SoCs do not have models. Please take a look how *every* other SoC is doing. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile index 21cd3a87f385..c909c19dc5dd 100644 --- a/arch/arm64/boot/dts/Makefile +++ b/arch/arm64/boot/dts/Makefile @@ -34,3 +34,4 @@ subdir-y += tesla subdir-y += ti subdir-y += toshiba subdir-y += xilinx +subdir-y += aspeed diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi new file mode 100644 index 000000000000..858ab95251e4 --- /dev/null +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +#include <dt-bindings/clock/aspeed,ast2700-clk.h> +#include <dt-bindings/reset/aspeed,ast2700-reset.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h> + +/ { + model = "Aspeed BMC"; + compatible = "aspeed,ast2700"; + #address-cells = <2>; + #size-cells = <2>; + interrupt-parent = <&gic>; + + aliases { + serial12 = &uart12; + }; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu@0 { + compatible = "arm,cortex-a35"; + enable-method = "psci"; + device_type = "cpu"; + reg = <0>; + d-cache-size = <0x8000>; + d-cache-line-size = <64>; + d-cache-sets = <128>; + i-cache-size = <0x8000>; + i-cache-line-size = <64>; + i-cache-sets = <256>; + next-level-cache = <&l2>; + }; + + cpu@1 { + compatible = "arm,cortex-a35"; + enable-method = "psci"; + device_type = "cpu"; + reg = <1>; + d-cache-size = <0x8000>; + d-cache-line-size = <64>; + d-cache-sets = <128>; + i-cache-size = <0x8000>; + i-cache-line-size = <64>; + i-cache-sets = <256>; + next-level-cache = <&l2>; + }; + + cpu@2 { + compatible = "arm,cortex-a35"; + enable-method = "psci"; + device_type = "cpu"; + reg = <2>; + d-cache-size = <0x8000>; + d-cache-line-size = <64>; + d-cache-sets = <128>; + i-cache-size = <0x8000>; + i-cache-line-size = <64>; + i-cache-sets = <256>; + next-level-cache = <&l2>; + }; + + cpu@3 { + compatible = "arm,cortex-a35"; + enable-method = "psci"; + device_type = "cpu"; + reg = <3>; + d-cache-size = <0x8000>; + d-cache-line-size = <64>; + d-cache-sets = <128>; + i-cache-size = <0x8000>; + i-cache-line-size = <64>; + i-cache-sets = <256>; + next-level-cache = <&l2>; + }; + + l2: l2-cache0 { + compatible = "cache"; + cache-size = <0x80000>; + cache-line-size = <64>; + cache-sets = <1024>; + cache-level = <2>; + }; + }; + + pmu { + compatible = "arm,cortex-a35-pmu"; + interrupt-parent = <&gic>; + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; + }; + + psci { + compatible = "arm,psci-1.0"; + method = "smc"; + }; + + gic: interrupt-controller@12200000 { + compatible = "arm,gic-v3"; + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>; + #interrupt-cells = <3>; + interrupt-controller; + interrupt-parent = <&gic>; + #redistributor-regions = <1>; + reg = <0 0x12200000 0 0x10000>, //GICD + <0 0x12280000 0 0x80000>, //GICR + <0 0x40440000 0 0x1000>; //GICC + }; + + timer { + compatible = "arm,armv8-timer"; + interrupt-parent = <&gic>; + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>, + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>; + arm,cpu-registers-not-fw-configured; + always-on; + }; + + soc0: soc@10000000 { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + soc0_sram: sram@10000000 { + compatible = "mmio-sram"; + reg = <0x0 0x10000000 0x0 0x20000>; /* 128KiB SRAM on soc0 */ + ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>; + #address-cells = <2>; + #size-cells = <2>; + no-memory-wc; + + exported@0 { + reg = <0 0x0 0 0x20000>; + export; + }; + }; + + syscon0: syscon@12c02000 { + compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd"; + reg = <0x0 0x12c02000 0x0 0x1000>; + ranges = <0x0 0x0 0 0x12c02000 0 0x1000>; + #address-cells = <2>; + #size-cells = <2>; + #clock-cells = <1>; + #reset-cells = <1>; + + silicon-id@0 { + compatible = "aspeed,ast2700-silicon-id", "aspeed,silicon-id"; + reg = <0 0x0 0 0x4>; + }; + + scu_ic0: interrupt-controller@1D0 { + #interrupt-cells = <1>; + compatible = "aspeed,ast2700-scu-ic0"; + reg = <0 0x1d0 0 0xc>; + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + }; + + scu_ic1: interrupt-controller@1E0 { + #interrupt-cells = <1>; + compatible = "aspeed,ast2700-scu-ic1"; + reg = <0 0x1e0 0 0xc>; + interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + }; + + soc0_rst: reset-controller@200 { + reg = <0 0x200 0 0x40>; + }; + + soc0_clk: clock-controller@240 { + reg = <0 0x240 0 0x1c0>; + }; + }; + + }; + + soc1: soc@14000000 { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + syscon1: syscon@14c02000 { + compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd"; + reg = <0x0 0x14c02000 0x0 0x1000>; + ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>; + #address-cells = <2>; + #size-cells = <2>; + #clock-cells = <1>; + #reset-cells = <1>; + + soc1_rst: reset-controller@200 { + #reset-cells = <1>; + }; + + soc1_clk: clock-controller@240 { + reg = <0 0x240 0 0x1c0>; + }; + }; + + uart12: serial@14c33b00 { + compatible = "ns16550a"; + reg = <0x0 0x14c33b00 0x0 0x100>; + reg-shift = <2>; + reg-io-width = <4>; + clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>; + no-loopback-test; + pinctrl-names = "default"; + }; + }; +}; +