Re: [PATCH v2 5/5] arm64: dts: Add Unisoc's SC9863A SoC support

From: Rob Herring
Date: Fri Nov 15 2019 - 09:43:39 EST

Next message: Mark Rutland: "Re: [PATCH v5 12/14] arm64: vdso: disable Shadow Call Stack"
Previous message: Aisha Gaddafi: "Dear Friend,"
In reply to: Chunyan Zhang: "Re: [PATCH v2 5/5] arm64: dts: Add Unisoc's SC9863A SoC support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Nov 15, 2019 at 2:59 AM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
>
> On Fri, 15 Nov 2019 at 05:05, Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > On Mon, Nov 11, 2019 at 05:02:30PM +0800, Chunyan Zhang wrote:
> > >
> > > Add basic DT to support Unisoc's SC9863A, with this patch,
> > > the board sp9863a-1h10 can run into console.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > > ---
> > > arch/arm64/boot/dts/sprd/Makefile | 3 +-
> > > arch/arm64/boot/dts/sprd/sc9863a.dtsi | 536 ++++++++++++++++++++++
> > > arch/arm64/boot/dts/sprd/sharkl3.dtsi | 188 ++++++++
> > > arch/arm64/boot/dts/sprd/sp9863a-1h10.dts | 40 ++
> > > 4 files changed, 766 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/arm64/boot/dts/sprd/sc9863a.dtsi
> > > create mode 100644 arch/arm64/boot/dts/sprd/sharkl3.dtsi
> > > create mode 100644 arch/arm64/boot/dts/sprd/sp9863a-1h10.dts
> > >
> > > diff --git a/arch/arm64/boot/dts/sprd/Makefile b/arch/arm64/boot/dts/sprd/Makefile
> > > index 2bdc23804f40..f4f1f5148cc2 100644
> > > --- a/arch/arm64/boot/dts/sprd/Makefile
> > > +++ b/arch/arm64/boot/dts/sprd/Makefile
> > > @@ -1,3 +1,4 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > > dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> > > - sp9860g-1h10.dtb
> > > + sp9860g-1h10.dtb \
> > > + sp9863a-1h10.dtb
> > > diff --git a/arch/arm64/boot/dts/sprd/sc9863a.dtsi b/arch/arm64/boot/dts/sprd/sc9863a.dtsi
> > > new file mode 100644
> > > index 000000000000..578d71a932d9
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/sprd/sc9863a.dtsi
> > > @@ -0,0 +1,536 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Unisoc SC9863A SoC DTS file
> > > + *
> > > + * Copyright (C) 2019, Unisoc Inc.
> > > + */
> > > +
> > > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +#include "sharkl3.dtsi"
> > > +
> > > +/ {
> > > + cpus {
> > > + #address-cells = <2>;
> > > + #size-cells = <0>;
> > > +
> > > + cpu-map {
> > > + cluster0 {
> > > + core0 {
> > > + cpu = <&CPU0>;
> > > + };
> > > + core1 {
> > > + cpu = <&CPU1>;
> > > + };
> > > + core2 {
> > > + cpu = <&CPU2>;
> > > + };
> > > + core3 {
> > > + cpu = <&CPU3>;
> > > + };
> > > + };
> > > +
> > > + cluster1 {
> > > + core0 {
> > > + cpu = <&CPU4>;
> > > + };
> > > + core1 {
> > > + cpu = <&CPU5>;
> > > + };
> > > + core2 {
> > > + cpu = <&CPU6>;
> > > + };
> > > + core3 {
> > > + cpu = <&CPU7>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + CPU0: cpu@0 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x0>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU1: cpu@100 {
> >
> > Your numbering seems odd. This follows the MPIDR reg? Normally a cluster
> > would share the same number in one of the bytes.
>
> We're using A55, and the spec says that bit[15:8] identifies
> individual cores within the local DynamIQâ cluster

Okay.

> Also, we only support one cluster.

cpu-map shows 2 clusters.

>
> >
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x100>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU2: cpu@200 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x200>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU3: cpu@300 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x300>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU4: cpu@400 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x400>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU5: cpu@500 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x500>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU6: cpu@600 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x600>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > +
> > > + CPU7: cpu@700 {
> > > + device_type = "cpu";
> > > + compatible = "arm,cortex-a55";
> > > + reg = <0x0 0x700>;
> > > + enable-method = "psci";
> > > + cpu-idle-states = <&CORE_PD>;
> > > + };
> > > + };
> > > +
> > > + idle-states {
> > > + entry-method = "arm,psci";
> > > + CORE_PD: core_pd {
> > > + compatible = "arm,idle-state";
> > > + entry-latency-us = <4000>;
> > > + exit-latency-us = <4000>;
> > > + min-residency-us = <10000>;
> > > + local-timer-stop;
> > > + arm,psci-suspend-param = <0x00010000>;
> > > + };
> > > + };
> > > +
> > > + gic: interrupt-controller@14000000 {
> >
> > Should go under a bus node.
>
> I didn't get your point, can you give me more details about this?

Memory mapped peripherals should go under a 'simple-bus' node rather
than be at the top level.

> > > + compatible = "arm,gic-v3";
> > > + #interrupt-cells = <3>;
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > + ranges;
> > > + redistributor-stride = <0x0 0x20000>; /* 128KB stride */
> > > + #redistributor-regions = <1>;
> > > + interrupt-controller;
> > > + reg = <0x0 0x14000000 0 0x20000>, /* GICD */
> > > + <0x0 0x14040000 0 0x100000>; /* GICR */
> > > + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> > > +
> > > + v2m_0: v2m@0 {
> > > + compatible = "arm,gic-v2m-frame";
> >
> > This is a GICv2 thing...
>
> Will remove it.
>
> >
> > > + msi-controller;
> >
> > Goes in the parent. Please run your dts file with
> > 'make W=12 dtbs_check' and fix the warnings.
>
> Ok (sorry for missing to do this check)
>
> >
> > > + reg = <0 0 0 0x1000>;
> > > + };
> > > + };
> > > +
> > > + psci {
> > > + compatible = "arm,psci-0.2";
> > > + method = "smc";
> > > + };
> > > +
> > > + timer {
> > > + compatible = "arm,armv8-timer";
> > > + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>, /* Physical Secure PPI */
> > > + <GIC_PPI 14 IRQ_TYPE_LEVEL_HIGH>, /* Physical Non-Secure PPI */
> > > + <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>, /* Virtual PPI */
> > > + <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>; /* Hipervisor PPI */
> > > + };
> > > +
> > > + pmu {
> > > + compatible = "arm,armv8-pmuv3";
> > > + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
> > > + <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;
> > > + };
> > > +
> > > + soc {
> > > + funnel@10001000 {
> > > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
> > > + reg = <0 0x10001000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + funnel_soc_out_port: endpoint {
> > > + remote-endpoint = <&etb_in>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + funnel_soc_in_port: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_ca55_out_port>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etb@10003000 {
> > > + compatible = "arm,coresight-tmc", "arm,primecell";
> > > + reg = <0 0x10003000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + in-ports {
> > > + port {
> > > + etb_in: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_soc_out_port>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + funnel@12001000 {
> > > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
> > > + reg = <0 0x12001000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + funnel_little_out_port: endpoint {
> > > + remote-endpoint =
> > > + <&etf_little_in>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + funnel_little_in_port0: endpoint {
> > > + remote-endpoint = <&etm0_out>;
> > > + };
> > > + };
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + funnel_little_in_port1: endpoint {
> > > + remote-endpoint = <&etm1_out>;
> > > + };
> > > + };
> > > +
> > > + port@2 {
> > > + reg = <2>;
> > > + funnel_little_in_port2: endpoint {
> > > + remote-endpoint = <&etm2_out>;
> > > + };
> > > + };
> > > +
> > > + port@3 {
> > > + reg = <3>;
> > > + funnel_little_in_port3: endpoint {
> > > + remote-endpoint = <&etm3_out>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etf@12002000 {
> > > + compatible = "arm,coresight-tmc", "arm,primecell";
> > > + reg = <0 0x12002000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etf_little_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_ca55_in_port0>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-port {
> > > + port {
> > > + etf_little_in: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_little_out_port>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etf@12003000 {
> > > + compatible = "arm,coresight-tmc", "arm,primecell";
> > > + reg = <0 0x12003000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etf_big_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_ca55_in_port1>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-ports {
> > > + port {
> > > + etf_big_in: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_big_out_port>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + funnel@12004000 {
> > > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
> > > + reg = <0 0x12004000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + funnel_ca55_out_port: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_soc_in_port>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + funnel_ca55_in_port0: endpoint {
> > > + remote-endpoint =
> > > + <&etf_little_out>;
> > > + };
> > > + };
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + funnel_ca55_in_port1: endpoint {
> > > + remote-endpoint =
> > > + <&etf_big_out>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + funnel@12005000 {
> > > + compatible = "arm,coresight-dynamic-funnel", "arm,primecell";
> > > + reg = <0 0x12005000 0 0x1000>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + funnel_big_out_port: endpoint {
> > > + remote-endpoint =
> > > + <&etf_big_in>;
> > > + };
> > > + };
> > > + };
> > > +
> > > + in-ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + funnel_big_in_port0: endpoint {
> > > + remote-endpoint = <&etm4_out>;
> > > + };
> > > + };
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + funnel_big_in_port1: endpoint {
> > > + remote-endpoint = <&etm5_out>;
> > > + };
> > > + };
> > > +
> > > + port@2 {
> > > + reg = <2>;
> > > + funnel_big_in_port2: endpoint {
> > > + remote-endpoint = <&etm6_out>;
> > > + };
> > > + };
> > > +
> > > + port@3 {
> > > + reg = <3>;
> > > + funnel_big_in_port3: endpoint {
> > > + remote-endpoint = <&etm7_out>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13040000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13040000 0 0x1000>;
> > > + cpu = <&CPU0>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm0_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_little_in_port0>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13140000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13140000 0 0x1000>;
> > > + cpu = <&CPU1>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm1_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_little_in_port1>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13240000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13240000 0 0x1000>;
> > > + cpu = <&CPU2>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm2_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_little_in_port2>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13340000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13340000 0 0x1000>;
> > > + cpu = <&CPU3>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm3_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_little_in_port3>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13440000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13440000 0 0x1000>;
> > > + cpu = <&CPU4>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm4_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_big_in_port0>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13540000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13540000 0 0x1000>;
> > > + cpu = <&CPU5>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm5_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_big_in_port1>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13640000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13640000 0 0x1000>;
> > > + cpu = <&CPU6>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm6_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_big_in_port2>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > +
> > > + etm@13740000 {
> > > + compatible = "arm,coresight-etm4x", "arm,primecell";
> > > + reg = <0 0x13740000 0 0x1000>;
> > > + cpu = <&CPU7>;
> > > + clocks = <&ext_26m>;
> > > + clock-names = "apb_pclk";
> > > +
> > > + out-ports {
> > > + port {
> > > + etm7_out: endpoint {
> > > + remote-endpoint =
> > > + <&funnel_big_in_port3>;
> > > + };
> > > + };
> > > + };
> > > + };
> > > + };
> > > +};
> > > diff --git a/arch/arm64/boot/dts/sprd/sharkl3.dtsi b/arch/arm64/boot/dts/sprd/sharkl3.dtsi
> > > new file mode 100644
> > > index 000000000000..3ef233f70dc4
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/sprd/sharkl3.dtsi
> > > @@ -0,0 +1,188 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Unisoc Sharkl3 platform DTS file
> > > + *
> > > + * Copyright (C) 2019, Unisoc Inc.
> > > + */
> > > +
> > > +/ {
> > > + interrupt-parent = <&gic>;
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > +
> > > + soc: soc {
> > > + compatible = "simple-bus";
> > > + #address-cells = <2>;
> > > + #size-cells = <2>;
> > > + ranges;
> > > +
> > > + ap_ahb_regs: syscon@20e00000 {
> > > + compatible = "syscon";
> > > + reg = <0 0x20e00000 0 0x10000>;
> > > + };
> > > +
> > > + pub_apb_regs: syscon@300e0000 {
> > > + compatible = "syscon";
> >
> > "syscon" should also have a specific compatible. What are all these
> > blocks? Looks like placeholders. If so, just drop them.
>
> The purppse is to make these addresses mapped for many peripharls
> whose some controller registers are in the same address base with one
> of syscons listed here.
> Under those peripharl device nodes there's a property refer to syscon, like
> https://elixir.bootlin.com/linux/v5.4-rc7/source/arch/arm64/boot/dts/sprd/sc9860.dtsi#L227

Okay, but you should have a specific compatible for each block in
addition to 'syscon'.

Also, do you really have 64KB of registers in each block? Define
what's actually there at least down to a page size to avoid
unnecessary mappings.

> In this way, devices can use the virtual address base which were
> mapped by syscon driver.
>
> You can also refer to the commit massage in the patch-set:
> https://lkml.org/lkml/2019/11/14/368

I agree with what Arnd said there. I don't really want to see syscon
extended. It's use is really for cases where we don't have another
binding defined. For example, one could define a clock controller
block a syscon and then do all clock control with drivers directly
accessing the clock registers, but we don't do that because we have a
clock binding. If your syscon accesses are much more than needing to
access a register field or 2 for something that doesn't fit into any
binding, then we should consider whether a binding is needed. As you
don't have the client side of any of this defined, I can't really
tell.

Rob

Next message: Mark Rutland: "Re: [PATCH v5 12/14] arm64: vdso: disable Shadow Call Stack"
Previous message: Aisha Gaddafi: "Dear Friend,"
In reply to: Chunyan Zhang: "Re: [PATCH v2 5/5] arm64: dts: Add Unisoc's SC9863A SoC support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]