diff mbox series

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

Message ID 08de3f4b-e33f-95c8-3297-814ea107272a@gmail.com
State New
Headers
Series [v1,1/4] dt-bindings: gpio: rockchip,gpio-bank: add compatible string per SoC |

Commit Message

Johan Jonker Jan. 18, 2023, 12:13 p.m. UTC 
  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.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 .../bindings/gpio/rockchip,gpio-bank.yaml         | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

--
2.20.1

Comments

Rob Herring Jan. 18, 2023, 3:32 p.m. UTC | #1 
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.

GPIO shouldn't really have an alias either IMO.

Rob
Johan Jonker Jan. 18, 2023, 5:12 p.m. UTC | #2 
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@theobroma-systems.com/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@theobroma-systems.com/

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

[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
Krzysztof Kozlowski Jan. 18, 2023, 6:32 p.m. UTC | #3 
On 18/01/2023 18:12, Johan Jonker wrote:
> 
> 
> 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.

Still do not see how compatible is related to this.

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

Again, how compatible is related to this?

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

Which you did not do... Your patch is doing something entirely else.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml b/Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml
index affd823c8..72fdfcc65 100644
--- a/Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml
@@ -13,7 +13,22 @@  properties:
   compatible:
     enum:
       - rockchip,gpio-bank
+      - rockchip,px30-gpio-bank
+      - rockchip,rk3036-gpio-bank
+      - rockchip,rk3066a-gpio-bank
+      - rockchip,rk3128-gpio-bank
+      - rockchip,rk3188-gpio-bank
       - rockchip,rk3188-gpio-bank0
+      - rockchip,rk3228-gpio-bank
+      - rockchip,rk3288-gpio-bank
+      - rockchip,rk3328-gpio-bank
+      - rockchip,rk3308-gpio-bank
+      - rockchip,rk3368-gpio-bank
+      - rockchip,rk3399-gpio-bank
+      - rockchip,rk3568-gpio-bank
+      - rockchip,rk3588-gpio-bank
+      - rockchip,rv1108-gpio-bank
+      - rockchip,rv1126-gpio-bank

   reg:
     maxItems: 1