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: <2dc46681-894d-4521-bfa7-3e9209691e0a@gmail.com>
Date:   Thu, 27 Oct 2022 19:53:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH v1 3/4] ARM: dts: rockchip: add rk3128.dtsi
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        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>
 <f45a4dcb-12e6-9a72-dbb3-7ec198ff2b1d@linaro.org>
From:   Johan Jonker <jbx6244@gmail.com>
In-Reply-To: <f45a4dcb-12e6-9a72-dbb3-7ec198ff2b1d@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Krzysztof, Kever, Heiko and others,

On 10/27/22 16:58, Krzysztof Kozlowski wrote:
> On 26/10/2022 20:53, Johan Jonker wrote:
>> Add basic rk3128 support.
>>
> 
> Thank you for your patch. There is something to discuss/improve.

Thank you for your review.

Some more questions/comments below.

> 
>> +#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;

Is gpio OK here?

>> +		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.

I just follow current Rockchip DT common practice.

Do we need to change all Rockchip boards?
Would like to hear from Heiko what's the plan here?
Syncing to U-boot is already a mess...

So far only instructions/changes and discussion about mmc nodes.

Can Rockchip specific rules be publicized in a central place? 

===
mmc aliases on reg order, availability and without number gap.
===

Heiko's sort rules:

compatible
reg
interrupts
[alphabetical]
status [if needed]

===
My incomplete list:

For nodes:
If exists on top: model, compatible and chosen.
Sort things without reg alphabetical first,
then sort the rest by reg address.

Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.
Keep an empty line between properties and nodes.

Exceptions:
Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
and dma-names.
Sort simple-audio-card,name above other simple-audio-card properties.
Sort regulator-name above other regulator properties.
Sort regulator-min-microvolt above regulator-max-microvolt.

> 
>> +	};
>> +
>> +	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?

rk3128 doesn't have a tsadc.
As long as this thermal stuff is not implemented with drivers and regulators I would prefer to keep it basic in the DT for now.
Just keep things simple for now till someone with hardware can fix that.

https://github.com/rockchip-linux/kernel/blob/develop-4.4/arch/arm/boot/dts/rk312x.dtsi#L315

	tsadc: tsadc {
		compatible = "rockchip,rk3126-tsadc-virtual";
		nvmem-cells = <&cpu_leakage>;
		nvmem-cell-names = "cpu_leakage";
		#thermal-sensor-cells = <1>;
		status = "disabled";
	};
> 
>> +			#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";

Original 2 interrupts:

>> +		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)>;

Could someone help here with the correct interrupts properties?

This was later changed to 4 interrupts:

		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)>,
			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
See:
arm: dts: enable arm arch virtual timer for rk312x
https://github.com/rockchip-linux/kernel/commit/405105146fd8a25ef3c9c46d18895ad243a6db2d

===

How to calculate the number?
Please advise? Thanks!

From rk3128 TRM show only 3 interrupts: ????
PPI
27 VIRTUAL TIMER High level
29 SECURE PHYSICAL TIMER High level
30 NON-SECURE PHY TIMER High level

>> +		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>;

How can this number be 9 if the lowest PPI IRQ ID is:

27 VIRTUAL TIMER High level

Can someone help here? Thanks!

> 

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

OK
Will check which defines ends up with 0xf04. 
interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; ??

> 
>> +		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

You are absolutely correct. Except for Rockchip usb2phy nodes ....
#phy-cells is located in a sub node, so we keep as it is... ;)

dt-bindings: phy: rename phy nodename in phy-rockchip-inno-usb2.yaml 
https://lore.kernel.org/all/20210601164800.7670-2-jbx6244@gmail.com/

> 
>> +			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
> 

Kind regards,

Johan Jonker