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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: "Heiko Stübner" <heiko@sntech.de>,
	"Johan Jonker" <jbx6244@gmail.com>,
	kever.yang@rock-chips.com
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
Subject: Re: [PATCH v1 3/4] ARM: dts: rockchip: add rk3128.dtsi
Date: Thu, 27 Oct 2022 17:15:21 -0400	[thread overview]
Message-ID: <f2652e0e-fb08-efb4-e25a-36a335f0c457@linaro.org> (raw)
In-Reply-To: <22076018.EfDdHjke4D@diego>

On 27/10/2022 16:02, Heiko Stübner wrote:
> Am Donnerstag, 27. Oktober 2022, 21:43:43 CEST schrieb 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.
> 
> Subsystems use the aliases for numbering their instances.
> So the i2c0 alias causes the i2c bus getting the number 0 in the operating
> system as well - making it i2c0 there too.

No one argues with these...

> 
> 
>>>>> +		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...
> 
> I remember only parts of the discussion for the previous socs. Back then
> Arnd was advocating mainly for moving the mmc aliases to boards.
> 
> As the aliases in general also determine the naming of the bus instance,
> I'm very much in favor of having the hardware-i2c5 being named i2c5
> in all cases ;-) . Having these hardware busses getting random numbers
> really calls for chaos.
> 
> So I'd really like us to continue the way we arrived at with the previous
> socs now :-)

No, not only mmc. UART, I2C, SPI - all of these should go to the board.

https://lore.kernel.org/linux-rockchip/CAK8P3a25iYksubCnQb1-e5yj=crEsK37RB9Hn4ZGZMwcVVrG7g@mail.gmail.com/

No one here discusses whether ordering should be random or not.

We discuss that this is a property of the board, not of a SoC.

> 

Best regards,
Krzysztof