[PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4

[Quentin Schulz]

Hi Johan,

On 1/17/23 15:44, Johan Jonker wrote:
> 
> 
> On 1/17/23 10:46, Quentin Schulz wrote:
>> Hi Johan,
>>
>> On 1/16/23 20:45, Johan Jonker wrote:
>>> Sync rk3066/rk3188 DT files from Linux.
>>> This is the state as of linux-next v6.2-rc4.
>>> New nfc node for MK808 rk3066a.
>>> CRU nodes now have a clock property.
>>> To prefend dtoc errors a fixed clock must also be
>>> included for tpl/spl in the rk3xxx-u-boot.dtsi file.
>>>
>>> Signed-off-by: Johan Jonker <jbx6244 at gmail.com>
>>> ---
> 
> [..]
> 
>>> @@ -223,7 +224,7 @@
>>>            #size-cells = <1>;
>>>            ranges;
>>>
>>> -        gpio0: gpio0 at 2000a000 {
>>> +        gpio0: gpio at 2000a000 {
>>>                compatible = "rockchip,rk3188-gpio-bank0";
>>>                reg = <0x2000a000 0x100>;
>>>                interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
>>> @@ -236,7 +237,7 @@
>>>                #interrupt-cells = <2>;
>>>            };
>>>
>>> -        gpio1: gpio1 at 2003c000 {
> 
>>> +        gpio1: gpio at 2003c000 {
> 
> Hi,
> 
> LOL: I made that binding change on request from Linux DT maintainers.
> Node names should generic.
> 

Yup I know. I tried to use a 5.19 DTS on a 5.10 kernel while debugging 
some stuff and was quite surprised to not even be able to reach userspace.

> ===
> 
> My full u-boot is able to boot a Linux kernel for rk3066a.
> Only when I give the command below it crashes:
> 
> gpio status -a
> 
> Could you confirm what other parts are effected?
> 

Well... Anything using GPIOs I believe? So Devices with a phandle to a 
gpio from an SoC GPIO controller, or some platform code getting a GPIO 
udevice to interact with some GPIO. Pinctrl shouldn't be affected 
AFAICT, so that might be the reason why most things seems to have worked 
fine for you?

> If it's boots then it's good enough for me and move forward, so please merge.(Kever)
> 

Since there's only one board using it, I guess that's your/Kever';s 
call, but that's not the case for PX30/RK3399 for example... where we'll 
need a similar change (though yes, my PX30 board also boots fine without 
and currently has this new DTSI, so broken GPIO driver).

> Driver fixes for u-boot depending on Linux DT changes is already very time consuming enough!
> 
> ===
> 
> Using DT path or node name is wrong.
> 
> Comment by robh+dt:
> /sys/bus/platform/devices/ paths are not an ABI. I'll consider
> nodenames an ABI if a change is noticed, but not for sysfs path.
> 
> https://urldefense.com/v3/__https://lore.kernel.org/linux-rockchip/CAL_JsqJGx_MwX9ynMiCUm_xP3t6xWbrtiUi-F*2ceXqZ2Su5tA@mail.gmail.com/__;Kw!!OOPJP91ZZw!iA_hrQyQy4KmdSwe46spmhBqsGQRRG4X_39kp9-JtCChxwMUdG22ICVmKLZHaIogqzTRzCZuHJ_rZ1hw5urOjubS9xJrRg$
> 

Yeah well... We weren't using the sysfs path but still have an issue :)

> Make use of the properties and a compatible in a node instead.
> 

That we can agree on.

> ===
> A little test.
> From: rockchip_gpio_probe():
> 
> int main() {
> 	//char dev_name[] = "gpio1 at 2003c000";
> 	char dev_name[] = "gpio at 2003c000";
> 	char priv_name[2];
> 	int priv_bank;
> 	char *end;
> 
> 	end = strrchr(dev_name, '@');
> 	priv_bank = trailing_strtoln(dev_name, end);
> 
> 	priv_name[0] = 'A' + priv_bank;
> 	priv_name[1] = 0;
> 
> 	printf("priv_bank: %d\n", priv_bank);
> 	printf("priv_name: %s\n", priv_name);
> 	return 0;
> }
> 
> // priv_bank: 1
> // priv_name: B
> 
> A change of node name gives:
> 
> // priv_bank: -1
> // priv_name: @
> 
> ===
> 
> Linux driver gpio-rockchip.c has an alias or else an increment id.
> 
> id = of_alias_get_id(np, "gpio");
> 	if (id < 0)
> 		id = gpio++;
> 
> Problem:
> Probe order is not guarantied.
> Aliases should go to board files and not in dtsi files anymore.
> 

And also, nothing forbids any board user to have gpio2= <&gpio0> in 
their aliases. We shouldn't rely on aliases for anything else than how 
it;s exposed to userspace I believe. Here it's a mistake anyways.

> ===
> 
> The current compatible string is generic, but these strings should normally be SoC orientated.
> 
> Proposal:
> 
> replace:
> compatible = "rockchip,gpio-bank";
> 
> by:
> compatible = "rockchip,rk3188-gpio-bank";
> 
> With that known we can lookup the reg address in a lookup table and it's name for u-boot.
> 
> struct lookup_table rk_gpio_rk3188_data[] = {
> 	{0x2000a000, "A"},
> 	{0x2003c000, "B"},
> 	{0x2003e000, "C"},
> 	{0x20080000, "D"},
> };
> 
> 
> 	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },
> 

Yes, that's an option too.

> ===
> 
> If needed I can help with changes to rockchip,gpio-bank.yaml
> 
> Let me know if it's useful or that you have an other solution as long as we get forward here.
> 

I believe Kever will want this to be upstreamed first in the kernel 
(though I think he doesn't mind if we sync stuff with linux-next, so 
that should make it faster to be merged in U-Boot).

I suggested:

rockchip,gpio-controller = <0>;
in gpio0,
rockchip,gpio-controller = <1>;
in gpio1, etc....

in the DT and reading this information from the driver directly. In 
which case, there's very little to do in the driver except getting this 
number and adding it to 'A'. We could also have a char there and not do 
this gymnastic with adding a number to the 'A' character.

Cheers,
Quentin