Message-Id: <B220CF49-1717-491C-AFD9-D8C4FD2DEF88@gmail.com>
Date: Wed, 12 Feb 2025 11:07:35 +0100
From: András Szemző <szemzo.andras@...il.com>
To: Andre Przywara <andre.przywara@....com>
Cc: Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Linus Walleij <linus.walleij@...aro.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Maxime Ripard <mripard@...nel.org>,
 Vinod Koul <vkoul@...nel.org>,
 Kishon Vijay Abraham I <kishon@...nel.org>,
 Ulf Hansson <ulf.hansson@...aro.org>,
 Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>,
 Albert Ou <aou@...s.berkeley.edu>,
 Uwe Kleine-König <u.kleine-koenig@...libre.com>,
 Florian Fainelli <florian.fainelli@...adcom.com>,
 linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org,
 linux-gpio@...r.kernel.org,
 linux-pm@...r.kernel.org,
 linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 09/10] ARM: dts: sun8i: add DTSI file for V853

Hi,

Thanks for your review!

> On 6 Feb 2025, at 17:19, Andre Przywara <andre.przywara@....com> wrote:
> 
> On Wed,  5 Feb 2025 13:52:24 +0100
> Andras Szemzo <szemzo.andras@...il.com> wrote:
> 
> Hi,
> 
>> V853/V851 is a new SoC by Allwinner. Add a basic dtsi file for it.
>> 
>> Signed-off-by: Andras Szemzo <szemzo.andras@...il.com>
>> ---
>> arch/arm/boot/dts/allwinner/sun8i-v853.dtsi | 656 ++++++++++++++++++++
>> 1 file changed, 656 insertions(+)
>> create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v853.dtsi
>> 
>> diff --git a/arch/arm/boot/dts/allwinner/sun8i-v853.dtsi b/arch/arm/boot/dts/allwinner/sun8i-v853.dtsi
>> new file mode 100644
>> index 000000000000..8b82b8783127
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/allwinner/sun8i-v853.dtsi
>> @@ -0,0 +1,656 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
>> +// Copyright (C) 2024 Andras Szemzo <szemzo.andras@...il.com>
>> +
>> +#include <dt-bindings/clock/sun6i-rtc.h>
>> +#include <dt-bindings/clock/allwinner,sun8i-v853-r-ccu.h>
>> +#include <dt-bindings/reset/allwinner,sun8i-v853-r-ccu.h>
>> +#include <dt-bindings/clock/allwinner,sun8i-v853-ccu.h>
>> +#include <dt-bindings/reset/allwinner,sun8i-v853-ccu.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/power/allwinner,sun8i-v853-ppu.h>
>> +
>> +/ {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + dcxo: dcxo-clk {
> 
> What's people's opinion about the node name? Traditionally we call this
> "osc24M", in all the other SoCs except D1 and A100. So is this the new
> black?
> 

I renamed it because node names can’t contain capital letters, and D1/A100 already use
dcxo.

>> + compatible = "fixed-clock";
>> + clock-output-names = "osc24M";
>> + #clock-cells = <0>;
>> + };
>> +
>> + cpus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + cpu0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a7","arm,armv7";
>> + reg = <0>;
>> + clocks = <&ccu CLK_CPU>;
>> + clock-names = "cpu";
>> + clock-frequency = <912000000>;
>> + clock-latency = <200000>;
>> + cooling-min-level = <5>;
>> + cooling-max-level = <0>;
>> + #cooling-cells = <2>; /* min followed by max */
>> + dynamic-power-coefficient = <142>;
>> + };
>> + };
>> +
>> + pmu {
>> + compatible = "arm,cortex-a7-pmu";
>> + interrupts = <GIC_SPI 172 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 173 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-parent = <&gic>;
> 
> Please move interrupt-parent to the root node, as the first property. Then
> you can lose it here, and in the timer and soc node.
> 

ok.

>> + };
>> +
>> + timer {
>> + compatible = "arm,armv7-timer";
>> + 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)>;
>> + clock-frequency = <24000000>;
> 
> Please remove this, and let the firmware set CNTFRQ. U-Boot does this
> automatically (in nonsec_virt.S), and it should be just a few lines to add
> to awboot's arch/arm32/arch_timer.c.
> 

I actually already did that.

> Or, even better: provide proper U-Boot support ;-)
> 
>> + interrupt-parent = <&gic>;
>> + arm,cpu-registers-not-fw-configured;
> 
> Same here, please fix your firmware to configure the non-secure world
> correctly, and remove this property.
> I am happy to help you with that.
> 

ok and thanks.

>> + arm,no-tick-in-suspend;
>> + };
>> +
>> + soc {
>> + compatible = "simple-bus";
>> + ranges;
>> + interrupt-parent = <&gic>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + dma-noncoherent;
>> +
>> + pio: pinctrl@...0000 {
>> + compatible = "allwinner,sun8i-v853-pinctrl";
>> + reg = <0x2000000 0x800>;
>> + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_APB0>, <&dcxo>, <&rtc CLK_OSC32K>;
>> + clock-names = "apb", "hosc", "losc";
>> + gpio-controller;
>> + interrupt-controller;
>> + #gpio-cells = <3>;
>> + #interrupt-cells = <3>;
>> +
>> + /omit-if-no-ref/
> 
> You don't need omit-if-no-ref for *all* nodes, just the more obscure ones,
> which are only used by one or two boards.
> Here I'd say, just have it for: uart2, dsi, spi3, i2c3, and remove that
> tag for the others.
> 

ok

>> + uart0_ph9_pins: uart0-ph9-pins {
>> + pins = "PH9", "PH10";
>> + function = "uart0";
>> + allwinner,pinmux = <5>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + uart2_pe12_pins: uart2-pe12-pins {
>> + pins = "PE12", "PE13";
>> + function = "uart2";
>> + allwinner,pinmux = <6>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + dsi_2lane_pins: dsi-2lane-pins {
>> + pins = "PD1", "PD2", "PD3", "PD4", "PD5", "PD6";
>> + function = "dsi";
>> + allwinner,pinmux = <5>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + spi0_pins: spi0-pins {
>> + pins = "PC0", "PC1", "PC2", "PC3", "PC4", "PC5";
>> + function = "spi0";
>> + allwinner,pinmux = <4>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + spi3_pins: spi3-pins {
>> + pins = "PH11", "PH12", "PH13", "PH14";
>> + function = "spi3";
>> + allwinner,pinmux = <6>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + i2c2_ph11_pins: i2c2-ph11-pins {
>> + pins = "PH11", "PH12";
>> + function = "i2c2";
>> + allwinner,pinmux = <5>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + i2c3_ph13_pins: i2c3-ph13-pins {
>> + pins = "PH13", "PH14";
>> + function = "i2c3";
>> + allwinner,pinmux = <5>;
>> + };
>> +
>> + /omit-if-no-ref/
>> + mmc0_pins: mmc0-pins {
>> + pins = "PF0", "PF1", "PF2", "PF3", "PF4", "PF5";
>> + function = "sdc0";
>> + allwinner,pinmux = <2>;
>> + drive-strength = <30>;
>> + bias-pull-up;
>> + };
>> +
>> + /omit-if-no-ref/
>> + mmc1_pins: mmc1-pins {
>> + pins = "PE0", "PE1", "PE2", "PE3", "PE4", "PE5";
>> + function = "sdc1";
>> + allwinner,pinmux = <6>;
>> + drive-strength = <30>;
>> + bias-pull-up;
>> + };
>> +
>> + /omit-if-no-ref/
>> + rmii_pe_pins: rmii-pe-pins {
>> + pins = "PE0", "PE1", "PE2", "PE3", "PE4",
>> +       "PE5", "PE6", "PE7", "PE8", "PE9";
>> + function = "emac";
>> + allwinner,pinmux = <3>;
>> + };
>> + };
>> +
>> + ccu: clock-controller@...1000 {
>> + compatible = "allwinner,sun8i-v853-ccu";
>> + reg = <0x2001000 0x1000>;
>> + clocks = <&dcxo>,
>> + <&rtc CLK_IOSC>,
>> + <&rtc CLK_OSC32K>;
>> + clock-names = "hosc", "iosc", "losc";
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + gpadc: adc@...9000 {
>> + compatible = "allwinner,sun20i-d1-gpadc";
> 
> Please use a fallback notation:
> compatible = "allwinner,sun8i-v853-gpadc",
>     "allwinner,sun20i-d1-gpadc";
> 
> This allows to later add quirks should we need some for this SoC. 
> And this requires a binding patch.
> 

ok for all the compatibles below.

>> + reg = <0x2009000 0x0400>;
>> + clocks = <&ccu CLK_BUS_GPADC>;
>> + resets = <&ccu RST_BUS_GPADC>;
>> + interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
>> + status = "disabled";
>> + #io-channel-cells = <1>;
>> + };
>> +
>> + i2s0: i2s@...2000 {
>> + compatible = "allwinner,sun50i-r329-i2s";
> 
> Same here, fallback compatible.
> 
>> + reg = <0x2032000 0x1000>;
>> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2S0>,
>> + <&ccu CLK_I2S0>;
>> + clock-names = "apb", "mod";
>> + resets = <&ccu RST_BUS_I2S0>;
>> + dmas = <&dma 3>, <&dma 3>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #sound-dai-cells = <0>;
>> + };
>> +
>> + i2s1: i2s@...3000 {
>> + compatible = "allwinner,sun50i-r329-i2s";
>> + reg = <0x2033000 0x1000>;
>> + interrupts = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2S1>,
>> + <&ccu CLK_I2S1>;
>> + clock-names = "apb", "mod";
>> + resets = <&ccu RST_BUS_I2S1>;
>> + dmas = <&dma 4>, <&dma 4>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #sound-dai-cells = <0>;
>> + };
>> +
>> + timer: timer@...0000 {
>> + compatible = "allwinner,sun20i-d1-timer",
> 
> This should be allwinner,sun8i-v853-timer.
> 
>> +     "allwinner,sun8i-a23-timer";
>> + reg = <0x2050000 0xa0>;
>> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&dcxo>;
>> + };
>> +
>> + wdt: watchdog@...00a0 {
>> + compatible = "allwinner,sun20i-d1-wdt-reset",
>> +     "allwinner,sun20i-d1-wdt";
> 
> Same here, use a more specific compatible, then the sun20i as a fallback.
> 
>> + reg = <0x20500a0 0x20>;
>> + interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&dcxo>, <&rtc CLK_OSC32K>;
>> + clock-names = "hosc", "losc";
>> + status = "reserved";
>> + };
>> +
>> + uart0: serial@...0000 {
>> + compatible = "snps,dw-apb-uart";
> 
> The UART is an exception, it's just the generic Synopsis name, so this is
> fine.
> 
>> + reg = <0x2500000 0x400>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_UART0>;
>> + resets = <&ccu RST_BUS_UART0>;
>> + dmas = <&dma 14>, <&dma 14>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + };
>> +
>> + uart1: serial@...0400 {
>> + compatible = "snps,dw-apb-uart";
>> + reg = <0x2500400 0x400>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_UART1>;
>> + resets = <&ccu RST_BUS_UART1>;
>> + dmas = <&dma 15>, <&dma 15>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + };
>> +
>> + uart2: serial@...0800 {
>> + compatible = "snps,dw-apb-uart";
>> + reg = <0x2500800 0x400>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_UART2>;
>> + resets = <&ccu RST_BUS_UART2>;
>> + dmas = <&dma 16>, <&dma 16>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + };
>> +
>> + uart3: serial@...0c00 {
>> + compatible = "snps,dw-apb-uart";
>> + reg = <0x2500c00 0x400>;
>> + reg-io-width = <4>;
>> + reg-shift = <2>;
>> + interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_UART3>;
>> + resets = <&ccu RST_BUS_UART3>;
>> + dmas = <&dma 17>, <&dma 17>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + };
>> +
>> + i2c0: i2c@...2000 {
>> + compatible = "allwinner,sun8i-v536-i2c",
>> +     "allwinner,sun6i-a31-i2c";
> 
> This needs three compatible strings:
> compatible = "allwinner,sun8i-v853-i2c",
>     "allwinner,sun8i-v536-i2c",
>     "allwinner,sun6i-a31-i2c";
> 
>> + reg = <0x2502000 0x400>;
>> + interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2C0>;
>> + resets = <&ccu RST_BUS_I2C0>;
>> + dmas = <&dma 43>, <&dma 43>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + i2c1: i2c@...2400 {
>> + compatible = "allwinner,sun8i-v536-i2c",
>> +     "allwinner,sun6i-a31-i2c";
>> + reg = <0x2502400 0x400>;
>> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2C1>;
>> + resets = <&ccu RST_BUS_I2C1>;
>> + dmas = <&dma 44>, <&dma 44>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + i2c2: i2c@...2800 {
>> + compatible = "allwinner,sun8i-v536-i2c",
>> +     "allwinner,sun6i-a31-i2c";
>> + reg = <0x2502800 0x400>;
>> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2C2>;
>> + resets = <&ccu RST_BUS_I2C2>;
>> + dmas = <&dma 45>, <&dma 45>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + i2c3: i2c@...2c00 {
>> + compatible = "allwinner,sun8i-v536-i2c",
>> +     "allwinner,sun6i-a31-i2c";
>> + reg = <0x2502c00 0x400>;
>> + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2C3>;
>> + resets = <&ccu RST_BUS_I2C3>;
>> + dmas = <&dma 46>, <&dma 46>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + i2c4: i2c@...3000 {
>> + compatible = "allwinner,sun8i-v536-i2c",
>> +     "allwinner,sun6i-a31-i2c";
>> + reg = <0x2503000 0x400>;
>> + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_I2C4>;
>> + resets = <&ccu RST_BUS_I2C4>;
>> + dmas = <&dma 47>, <&dma 47>;
>> + dma-names = "rx", "tx";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + syscon: syscon@...0000 {
>> + compatible = "allwinner,sun8i-h3-system-control";
> 
> I think you got the idea by now, fallbacks ... and for the others below as
> well.
> 
>> + reg = <0x3000000 0x1000>;
>> + ranges;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + sram_c: sram@...00 {
>> + compatible = "mmio-sram";
>> + reg = <0x00020000 0x21000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0x00020000 0x21000>;
>> +
>> + ve_sram: sram-section@0 {
>> +    compatible = "allwinner,sun8i-v3s-sram-c", 
>> + "allwinner,sun4i-a10-sram-c1";
>> +    reg = <0x000000 0x21000>;
>> + };
>> + };
>> + };
>> +
>> + dma: dma-controller@...2000 {
>> + compatible = "allwinner,sun20i-d1-dma";
>> + reg = <0x3002000 0x1000>;
>> + interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_DMA>, <&ccu CLK_MBUS_DMA>;
>> + clock-names = "bus", "mbus";
>> + resets = <&ccu RST_BUS_DMA>;
>> + dma-channels = <16>;
>> + dma-requests = <48>;
>> + #dma-cells = <1>;
>> + };
>> +
>> + hwspinlock: hwlock@...5000 {
>> +    compatible = "allwinner,sun6i-a31-hwspinlock";
>> + reg = <0x3005000 0x1000>;
>> + clocks = <&ccu CLK_BUS_SPINLOCK>;
>> + clock-names = "ahb";
>> + resets = <&ccu RST_BUS_SPINLOCK>;
>> + reset-names = "ahb";
>> + interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + sid: efuse@...6000 {
>> + compatible = "allwinner,sun20i-d1-sid";
>> + reg = <0x3006000 0x1000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + ths_calib: ths-calib@14 {
>> + reg = <0x14 0x8>;
>> + };
>> + };
>> +
>> + hstimer@...8000 {
>> + compatible = "allwinner,sun7i-a20-hstimer";
>> + reg = <0x03008000 0x1000>;
>> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
>> +     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_HSTIMER>;
>> + };
>> +
>> + gic: interrupt-controller@...1000 {
>> + compatible = "arm,gic-400";
> 
> (no fallbacks needed here, it's an actual GIC-400)
> 
>> + reg = <0x03021000 0x1000>, /* GIC Dist */
>> +      <0x03022000 0x2000>, /* GIC CPU */
>> +      <0x03024000 0x2000>, /* GIC VCPU Control */
>> +      <0x03026000 0x2000>; /* GIC VCPU */
>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
>> + interrupt-controller;
>> + #interrupt-cells = <3>;
>> + };
>> +
>> + mbus: dram-controller@...2000 {
>> + compatible = "allwinner,sun20i-d1-mbus";
>> + reg = <0x3102000 0x1000>,
>> +      <0x3103000 0x1000>;
>> + reg-names = "mbus", "dram";
>> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_MBUS>,
>> + <&ccu CLK_DRAM>,
>> + <&ccu CLK_BUS_DRAM>;
>> + clock-names = "mbus", "dram", "bus";
>> + dma-ranges = <0 0x40000000 0x80000000>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + #interconnect-cells = <1>;
>> + };
>> +
>> + mmc0: mmc@...0000 {
>> + compatible = "allwinner,sun20i-d1-mmc";
>> + reg = <0x4020000 0x1000>;
>> + interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
>> + clock-names = "ahb", "mmc";
>> + resets = <&ccu RST_BUS_MMC0>;
>> + reset-names = "ahb";
>> + no-sdio;
>> + no-mmc;
> 
> I don't think we use those two properties in mainline?
> 

yes, it’s remained from the BSP, I’ll remove.

>> + cap-sd-highspeed;
>> + max-frequency = <150000000>;
>> + mmc-ddr-1_8v;
> 
> This looks wrong here, definitely you should not have voltage dependent
> properties in the SoC .dtsi.
> 
> And you can add the pinctrl properties already here, since there is only
> one set of pins for MMC0.
> 

ok

>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + mmc1: mmc@...1000 {
>> + compatible = "allwinner,sun20i-d1-mmc";
>> + reg = <0x4021000 0x1000>;
>> + interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
>> + clock-names = "ahb", "mmc";
>> + resets = <&ccu RST_BUS_MMC1>;
>> + reset-names = "ahb";
>> + cap-sd-highspeed;
>> + cap-mmc-highspeed;
>> + max-frequency = <150000000>;
>> + mmc-ddr-1_8v;
> 
> same here, please remove.
> 
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + mmc2: mmc@...2000 {
>> + compatible = "allwinner,sun20i-d1-mmc";
>> + reg = <0x4022000 0x1000>;
>> + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_MMC2>, <&ccu CLK_MMC2>;
>> + clock-names = "ahb", "mmc";
>> + resets = <&ccu RST_BUS_MMC2>;
>> + reset-names = "ahb";
>> + no-sdio;
>> + no-sd;
>> + cap-mmc-highspeed;
>> + max-frequency = <150000000>;
>> + mmc-ddr-1_8v;
>> + mmc-ddr-3_3v;
> 
> Please leave that to the board DTs to set, they know which voltage PortC is
> running on.
> 
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + spi0: spi@...5000 {
>> + compatible = "allwinner,sun50i-r329-spi";
>> + reg = <0x4025000 0x1000>;
>> + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>;
>> + clock-names = "ahb", "mod";
>> + resets = <&ccu RST_BUS_SPI0>;
>> + dmas = <&dma 22>, <&dma 22>;
>> + dma-names = "rx", "tx";
>> + num-cs = <1>;
> 
> Is that so? I see SPI0_CS0, SPI0_CS1, SPI1_CS0, SPI1_CS1 pins.
> 

Sure, I implemented it from v851s datasheet, where there is no xCS1.

>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + spi1: spi@...6000 {
>> + compatible = "allwinner,sun50i-r329-spi-dbi",
>> +     "allwinner,sun50i-r329-spi";
>> + reg = <0x4026000 0x1000>;
>> + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>;
>> + clock-names = "ahb", "mod";
>> + resets = <&ccu RST_BUS_SPI1>;
>> + dmas = <&dma 23>, <&dma 23>;
>> + dma-names = "rx", "tx";
>> + num-cs = <1>;
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + spi2: spi@...7000 {
>> + compatible = "allwinner,sun50i-r329-spi";
>> + reg = <0x4027000 0x1000>;
>> + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_SPI2>, <&ccu CLK_SPI2>;
>> + clock-names = "ahb", "mod";
>> + resets = <&ccu RST_BUS_SPI2>;
>> + dmas = <&dma 24>, <&dma 24>;
>> + dma-names = "rx", "tx";
>> + num-cs = <1>;
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + spi3: spi@...8000 {
>> + compatible = "allwinner,sun50i-r329-spi";
>> + reg = <0x4028000 0x1000>;
>> + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_SPI3>, <&ccu CLK_SPI3>;
>> + clock-names = "ahb", "mod";
>> + resets = <&ccu RST_BUS_SPI3>;
>> + dmas = <&dma 25>, <&dma 25>;
>> + dma-names = "rx", "tx";
>> + num-cs = <1>;
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + usb_otg: usb@...0000 {
>> + compatible = "allwinner,sun8i-a33-musb";
>> + reg = <0x4100000 0x400>;
>> + interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "mc";
>> + clocks = <&ccu CLK_BUS_OTG>;
>> + resets = <&ccu RST_BUS_OTG>;
>> + extcon = <&usbphy 0>;
>> + phys = <&usbphy 0>;
>> + phy-names = "usb";
>> + status = "disabled";
>> + };
>> +
>> + usbphy: phy@...0400 {
>> + compatible = "allwinner,sun8i-v853-usb-phy";
>> + reg = <0x4100400 0x100>,
>> +      <0x4101800 0x100>;
>> + reg-names = "phy_ctrl",
>> +    "pmu0";
>> + clocks = <&ccu CLK_USB_OHCI>;
>> + clock-names = "usb0_phy";
>> + resets = <&ccu RST_USB_PHY>;
>> + reset-names = "usb0_reset";
>> + status = "disabled";
>> + #phy-cells = <1>;
>> + };
>> +
>> + ehci0: usb@...1000 {
>> + compatible = "generic-ehci";
>> + reg = <0x4101000 0x100>;
>> + interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_OHCI>,
>> + <&ccu CLK_BUS_EHCI>,
>> + <&ccu CLK_USB_OHCI>;
>> + resets = <&ccu RST_BUS_OHCI>,
>> + <&ccu RST_BUS_EHCI>;
>> + phys = <&usbphy 0>;
>> + phy-names = "usb";
>> + status = "disabled";
>> + };
>> +
>> + ohci0: usb@...1400 {
>> + compatible = "generic-ohci";
>> + reg = <0x4101400 0x100>;
>> + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&ccu CLK_BUS_OHCI>,
>> + <&ccu CLK_USB_OHCI>;
>> + resets = <&ccu RST_BUS_OHCI>;
>> + phys = <&usbphy 0>;
>> + phy-names = "usb";
>> + status = "disabled";
>> + };
>> +
>> + ppu: power-controller@...1000 {
>> + compatible = "allwinner,sun8i-v853-ppu";
>> + reg = <0x7001000 0x400>;
>> + clocks = <&r_ccu CLK_BUS_R_PPU>;
>> + resets = <&r_ccu RST_BUS_R_PPU>;
>> + #power-domain-cells = <1>;
>> + };
>> +
>> + r_ccu: clock-controller@...0000 {
>> + compatible = "allwinner,sun8i-v853-r-ccu";
>> + reg = <0x7010000 0x230>;
>> + clocks = <&dcxo>,
>> + <&rtc CLK_IOSC>,
>> + <&rtc CLK_OSC32K>,
>> + <&ccu CLK_PLL_PERIPH_200M>,
>> + <&ccu CLK_PLL_AUDIO_4X>;
>> + clock-names = "hosc",
>> +      "iosc",
>> +      "losc",
>> +      "pll-periph",
>> +      "pll-audio";
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + nmi_intc: intc-nmi@...0320 {
>> + compatible = "allwinner,sun9i-a80-nmi";
>> + interrupt-parent = <&gic>;
> 
> not needed
> 

ok

>> + #interrupt-cells = <2>;
>> + #address-cells = <0>;
>> + interrupt-controller;
>> + reg = <0x07010320 0xc>;
>> + interrupts = <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>;
> 
> reg and interrupts typically come right after compatible
> 

ok

> That's it for now, I will check all the numbers against the manual later,
> and verify the clock names and such.
> 
> Cheers,
> Andre
> 
>> + };
>> +
>> + rtc: rtc@...0000 {
>> + compatible = "allwinner,sun50i-r329-rtc";
>> + reg = <0x7090000 0x400>;
>> + interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&r_ccu CLK_BUS_R_RTC>;
>> + clock-names = "bus";
>> + resets = <&r_ccu RST_BUS_R_RTC>;
>> + wakeup-source;
>> + #clock-cells = <1>;
>> + };
>> + };
>> +};
> 

Powered by blists - more mailing lists