Re: [PATCH v1 3/4] ARM: dts: rockchip: add rk3128.dtsi

[Krzysztof Kozlowski]

On 27/10/2022 13:53, Johan Jonker wrote:
> 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?

Could be, but let me rephrase it - why do you need aliases in DTSI? What
do these aliases represent?

The SoC pieces (nodes in DTSI) do not rely on aliases.

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

Heiko might have his own preference which then over-rules my
recommendation here. But in general this applies to all boards, so other
boards could be fixed as well. Different point is whether it is actually
worth fixing them...

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

I don't know what does it mean. Do you discuss with my comment? Wasn't
my comment exactly like this?

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

Is there a question to me?

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

And this is related to operating-points-v2?

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

I have no clue what do you refer now.

I did not comment here, so I guess nothing more to me?

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

How phy-cells are related?

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

You mean parent device bindings expect usb2phy? If so, then it's fine.

Best regards,
Krzysztof