Re: [PATCH v1 1/4] dt-bindings: gpio: rockchip,gpio-bank: add compatible string per SoC

[Johan Jonker <jbx6244@xxxxxxxxx>]

On 1/18/23 16:32, Rob Herring wrote:
> On Wed, Jan 18, 2023 at 01:13:23PM +0100, Johan Jonker wrote:
>> Currently all Rockchip gpio nodes have the same compatible.
>> Replace all the compatibles in gpio nodes to be able to
>> give them a consistent ID independent from probe order or alias.
> 
> I fail to see how the compatible change affects probe order or aliases. 
> It is also an ABI break if there is not the existing compatible as a 
> fallback. State the problem you are trying to solve with this change, 
> not just what your solution is.

Hi Rob,

Since the yaml conversion of rockchip,gpio-bank.yaml we have generic "gpio" names instead of "gpio-1".
For both Linux and U-boot there's a need for a consisted ID order between the nodes.

The kernel has no logic to decide between the first compatible and the fallback.
A fallback doesn't have ability to add/select "data" with probe, but have to use
of_device_is_compatible(np, "rockchip,rk3188-gpio-bank") for  "15"  SoCs instead.

I can produce a serie with fall back.
Let us know how to move forward here.

Kind regards,

Johan Jonker


===
Linux driver behavior:

	id = of_alias_get_id(np, "gpio");
	if (id < 0)
		id = gpio++;

Problems:
Alias not always available in existing DT files(not part of the binding)
Probe order is not guarantied and possible number gap for rk3066a between gpio4 and gpio6.
(id counter gives not consistent result)

===
U-boot rk_gpio.c current behavior:

	end = strrchr(dev->name, '@');
	priv->bank = trailing_strtoln(dev->name, end);
	priv->name[0] = 'A' + priv->bank;
	uc_priv->bank_name = priv->name;

Problems:
Crash when node name has no "gpio-1" format

U-boot rk-gpio proposed: 

	priv->name[0] = 'A' + dev_seq(dev);
	uc_priv->bank_name = priv->name;

Problems:
Reduced FDT's and rk3066a gives number gaps.

===

My proposal:

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 },

===


See links for previous discussions:

[PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4 Johan Jonker
https://lore.kernel.org/linux-rockchip/6ec4427e-f56f-7545-2296-bc75d74f4364@xxxxxxxxxxxxxxxxxxxxx/T/#t

[PATCH 1/3] arm64: dts: rk3399: sync rk3399.dtsi from 6.1-rc1 Peter Robinson
https://lore.kernel.org/u-boot/0ab9a600-b517-0ce5-d189-99fc8eddfa60@xxxxxxxxxxxxxxxxxxxxx/

[PATCH/RFC] rockchip: derive GPIO bank from alias if available John Keeping
https://lore.kernel.org/u-boot/CAPnjgZ3j7YVJt+B-uEKS_rsSRBSR-kmyLujV3RozL7kbFNrqFw@xxxxxxxxxxxxxx/

[PATCH] rockchip: derive GPIO bank from alias if available John Keeping
https://lore.kernel.org/u-boot/Y8gaqasu9ho0vl8X@donbot/T/#m00e3823cf74d35a7c8536a1193ed1a77c7698135

Broken U-boot gpio driver:
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/gpio/rk_gpio.c#L154-L157

> 
> GPIO shouldn't really have an alias either IMO.
> 
> Rob