Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f45a4dcb-12e6-9a72-dbb3-7ec198ff2b1d@linaro.org>
Date:   Thu, 27 Oct 2022 10:58:38 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH v1 3/4] ARM: dts: rockchip: add rk3128.dtsi
Content-Language: en-US
To:     Johan Jonker <jbx6244@gmail.com>, kever.yang@rock-chips.com,
        heiko@sntech.de
Cc:     sjg@chromium.org, philipp.tomsich@vrull.eu,
        zhangqing@rock-chips.com, hjc@rock-chips.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, daniel.lezcano@linaro.org,
        tglx@linutronix.de, arnd@arndb.de, olof@lixom.net, soc@kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org
References: <da1252eb-85e9-bdb8-0542-207173523523@gmail.com>
 <674b875a-0dfa-eff2-5018-eafed851707f@gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <674b875a-0dfa-eff2-5018-eafed851707f@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26/10/2022 20:53, Johan Jonker wrote:
> Add basic rk3128 support.
> 

Thank you for your patch. There is something to discuss/improve.

> +#include <dt-bindings/clock/rk3128-cru.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +
> +/ {
> +	compatible = "rockchip,rk3128";
> +	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	aliases {
> +		gpio0 = &gpio0;
> +		gpio1 = &gpio1;
> +		gpio2 = &gpio2;
> +		gpio3 = &gpio3;
> +		i2c0 = &i2c0;
> +		i2c1 = &i2c1;
> +		i2c2 = &i2c2;
> +		i2c3 = &i2c3;
> +		spi0 = &spi0;
> +		serial0 = &uart0;
> +		serial1 = &uart1;
> +		serial2 = &uart2;

Bus aliases are board specific and represent what is actually available
on headers/pins etc. These do not belong to SoC DTSI.

> +	};
> +
> +	arm-pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@f00 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0xf00>;
> +			clock-latency = <40000>;
> +			clocks = <&cru ARMCLK>;
> +			operating-points = <
> +				/* KHz    uV */
> +				 816000 1000000
> +			>;

Why not operating-points-v2?

> +			#cooling-cells = <2>; /* min followed by max */
> +		};
> +
> +		cpu1: cpu@f01 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0xf01>;
> +		};
> +
> +		cpu2: cpu@f02 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0xf02>;
> +		};
> +
> +		cpu3: cpu@f03 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0xf03>;
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv7-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +		arm,cpu-registers-not-fw-configured;
> +		clock-frequency = <24000000>;
> +	};
> +
> +	xin24m: oscillator {
> +		compatible = "fixed-clock";
> +		clock-frequency = <24000000>;
> +		clock-output-names = "xin24m";
> +		#clock-cells = <0>;
> +	};
> +
> +	pmu: syscon@100a0000 {
> +		compatible = "rockchip,rk3128-pmu", "syscon", "simple-mfd";
> +		reg = <0x100a0000 0x1000>;
> +	};
> +
> +	gic: interrupt-controller@10139000 {
> +		compatible = "arm,cortex-a7-gic";
> +		reg = <0x10139000 0x1000>,
> +		      <0x1013a000 0x1000>,
> +		      <0x1013c000 0x2000>,
> +		      <0x1013e000 0x2000>;
> +		interrupts = <GIC_PPI 9 0xf04>;

f04 looks like a mask, so use standard defines for it.

> +		interrupt-controller;
> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +	};
> +
> +	usb_otg: usb@10180000 {
> +		compatible = "rockchip,rk3128-usb", "rockchip,rk3066-usb", "snps,dwc2";
> +		reg = <0x10180000 0x40000>;
> +		interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru HCLK_OTG>;
> +		clock-names = "otg";
> +		dr_mode = "otg";
> +		phys = <&usb2phy_otg>;
> +		phy-names = "usb2-phy";
> +		status = "disabled";
> +	};
> +
> +	usb_host_ehci: usb@101c0000 {
> +		compatible = "generic-ehci";
> +		reg = <0x101c0000 0x20000>;
> +		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +		phys = <&usb2phy_host>;
> +		phy-names = "usb";
> +		status = "disabled";
> +	};
> +
> +	usb_host_ohci: usb@101e0000 {
> +		compatible = "generic-ohci";
> +		reg = <0x101e0000 0x20000>;
> +		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +		phys = <&usb2phy_host>;
> +		phy-names = "usb";
> +		status = "disabled";
> +	};
> +
> +	sdmmc: mmc@10214000 {
> +		compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
> +		reg = <0x10214000 0x4000>;
> +		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru HCLK_SDMMC>, <&cru SCLK_SDMMC>,
> +			 <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;
> +		clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +		dmas = <&pdma 10>;
> +		dma-names = "rx-tx";
> +		fifo-depth = <256>;
> +		max-frequency = <150000000>;
> +		resets = <&cru SRST_SDMMC>;
> +		reset-names = "reset";
> +		status = "disabled";
> +	};
> +
> +	sdio: mmc@10218000 {
> +		compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
> +		reg = <0x10218000 0x4000>;
> +		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru HCLK_SDIO>, <&cru SCLK_SDIO>,
> +			 <&cru SCLK_SDIO_DRV>, <&cru SCLK_SDIO_SAMPLE>;
> +		clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +		dmas = <&pdma 11>;
> +		dma-names = "rx-tx";
> +		fifo-depth = <256>;
> +		max-frequency = <150000000>;
> +		resets = <&cru SRST_SDIO>;
> +		reset-names = "reset";
> +		status = "disabled";
> +	};
> +
> +	emmc: mmc@1021c000 {
> +		compatible = "rockchip,rk3128-dw-mshc", "rockchip,rk3288-dw-mshc";
> +		reg = <0x1021c000 0x4000>;
> +		interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru HCLK_EMMC>, <&cru SCLK_EMMC>,
> +			 <&cru SCLK_EMMC_DRV>, <&cru SCLK_EMMC_SAMPLE>;
> +		clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
> +		dmas = <&pdma 12>;
> +		dma-names = "rx-tx";
> +		fifo-depth = <256>;
> +		max-frequency = <150000000>;
> +		resets = <&cru SRST_EMMC>;
> +		reset-names = "reset";
> +		status = "disabled";
> +	};
> +
> +	nfc: nand-controller@10500000 {
> +		compatible = "rockchip,rk3128-nfc", "rockchip,rk2928-nfc";
> +		reg = <0x10500000 0x4000>;
> +		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru HCLK_NANDC>, <&cru SCLK_NANDC>;
> +		clock-names = "ahb", "nfc";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&flash_ale &flash_bus8 &flash_cle &flash_cs0
> +			     &flash_dqs &flash_rdn &flash_rdy &flash_wrn>;
> +		status = "disabled";
> +	};
> +
> +	cru: clock-controller@20000000 {
> +		compatible = "rockchip,rk3128-cru";
> +		reg = <0x20000000 0x1000>;
> +		clocks = <&xin24m>;
> +		clock-names = "xin24m";
> +		rockchip,grf = <&grf>;
> +		#clock-cells = <1>;
> +		#reset-cells = <1>;
> +		assigned-clocks = <&cru PLL_GPLL>;
> +		assigned-clock-rates = <594000000>;
> +	};
> +
> +	grf: syscon@20008000 {
> +		compatible = "rockchip,rk3128-grf", "syscon", "simple-mfd";
> +		reg = <0x20008000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		usb2phy: usb2phy@17c {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "rockchip,rk3128-usb2phy";
> +			reg = <0x017c 0x0c>;
> +			clocks = <&cru SCLK_OTGPHY0>;
> +			clock-names = "phyclk";
> +			clock-output-names = "usb480m_phy";
> +			#clock-cells = <0>;
> +			status = "disabled";
> +


Best regards,
Krzysztof