[PATCH] rockchip: derive GPIO bank from alias if available

[PATCH] rockchip: derive GPIO bank from alias if available

[John Keeping]

Upstream device trees now use standard node names like "gpio@ff..." but
the rk_gpio driver expects a name like "gpio0@ff..." (note the index
before the @).

This is not a change that can be made in a -u-boot.dtsi file, so
updating to the latest upstream device trees requires updating the
driver.

Other GPIO drivers already use the sequence number to derive the bank.
This can be set explicitly using an alias in the device tree (which can
be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
order in the device tree anyway so when no aliases are supplied the
indexes should still be correct.

Note that the bank name is only used by the `gpio` command or
potentially by board code via dm_gpio_lookup_name() and related
functions; it is not used by driver code which will lookup GPIOs from
the device tree by phandle.  No Rockchip platforms make any use of
dm_gpio_lookup_name() or `gpio` in the boot scripts.

Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Cc: Johan Jonker <jbx6244@gmail.com>
Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/gpio/rk_gpio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
index 68f30157a9..8568f10cd0 100644
--- a/drivers/gpio/rk_gpio.c
+++ b/drivers/gpio/rk_gpio.c
@@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
 {
 	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
 	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
-	char *end;
 	int ret;
 
 	priv->regs = dev_read_addr_ptr(dev);
@@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
 		return ret;
 
 	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
-	end = strrchr(dev->name, '@');
-	priv->bank = trailing_strtoln(dev->name, end);
-	priv->name[0] = 'A' + priv->bank;
+
+	priv->name[0] = 'A' + dev_seq(dev);
 	uc_priv->bank_name = priv->name;
 
 	return 0;
-- 
2.39.0

Re: [PATCH] rockchip: derive GPIO bank from alias if available

[Johan Jonker]

On 1/17/23 19:15, John Keeping wrote:
> Upstream device trees now use standard node names like "gpio@ff..." but
> the rk_gpio driver expects a name like "gpio0@ff..." (note the index
> before the @).
> 
> This is not a change that can be made in a -u-boot.dtsi file, so
> updating to the latest upstream device trees requires updating the
> driver.
> 
> Other GPIO drivers already use the sequence number to derive the bank.
> This can be set explicitly using an alias in the device tree (which can
> be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
> order in the device tree anyway so when no aliases are supplied the
> indexes should still be correct.
> 
> Note that the bank name is only used by the `gpio` command or
> potentially by board code via dm_gpio_lookup_name() and related
> functions; it is not used by driver code which will lookup GPIOs from
> the device tree by phandle.  No Rockchip platforms make any use of
> dm_gpio_lookup_name() or `gpio` in the boot scripts.
> 
> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Cc: Johan Jonker <jbx6244@gmail.com>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/gpio/rk_gpio.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> index 68f30157a9..8568f10cd0 100644
> --- a/drivers/gpio/rk_gpio.c
> +++ b/drivers/gpio/rk_gpio.c
> @@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
>  {
>  	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>  	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
> -	char *end;
>  	int ret;
>  
>  	priv->regs = dev_read_addr_ptr(dev);
> @@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
>  		return ret;
>  
>  	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
> -	end = strrchr(dev->name, '@');
> -	priv->bank = trailing_strtoln(dev->name, end);
> -	priv->name[0] = 'A' + priv->bank;

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

Hi,

Some comments. Have a look if it's useful.
===
1:
Tested with rk3066a mk808 in only full u-boot.

The gpio banks have a number gap.

	aliases {
		gpio0 = &gpio0;
		gpio1 = &gpio1;
		gpio2 = &gpio2;
		gpio3 = &gpio3;
		gpio4 = &gpio4;

		gpio6 = &gpio6;
	};

With aliases gpio6 is called G.
Without gpio6 is called F.
There's no name consistency for command scripts.
===
2:
For example: rk3288-evb-u-boot.dtsi

What happens in tpl or spl with only gpio3 and gpio8 in dt-plat.c.

Or with SPL FDT blob:

[v1,01/17] rockchip: spl: fix reloc gd and FDT blob pointer
http://patchwork.ozlabs.org/project/uboot/patch/20220508150825.21711-2-jbx6244@gmail.com/

Not in use in mainline. Just saying there's more possible in SPL/TPL.
===
The use of aliases is more of something of an additional thing that we not always can count on that is there.

Johan

 

>  	uc_priv->bank_name = priv->name;
>  
>  	return 0;

Re: [PATCH] rockchip: derive GPIO bank from alias if available

[John Keeping]

On Tue, Jan 17, 2023 at 08:52:22PM +0100, Johan Jonker wrote:
> 
> 
> On 1/17/23 19:15, John Keeping wrote:
> > Upstream device trees now use standard node names like "gpio@ff..." but
> > the rk_gpio driver expects a name like "gpio0@ff..." (note the index
> > before the @).
> > 
> > This is not a change that can be made in a -u-boot.dtsi file, so
> > updating to the latest upstream device trees requires updating the
> > driver.
> > 
> > Other GPIO drivers already use the sequence number to derive the bank.
> > This can be set explicitly using an alias in the device tree (which can
> > be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
> > order in the device tree anyway so when no aliases are supplied the
> > indexes should still be correct.
> > 
> > Note that the bank name is only used by the `gpio` command or
> > potentially by board code via dm_gpio_lookup_name() and related
> > functions; it is not used by driver code which will lookup GPIOs from
> > the device tree by phandle.  No Rockchip platforms make any use of
> > dm_gpio_lookup_name() or `gpio` in the boot scripts.
> > 
> > Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > Cc: Johan Jonker <jbx6244@gmail.com>
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >  drivers/gpio/rk_gpio.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> > index 68f30157a9..8568f10cd0 100644
> > --- a/drivers/gpio/rk_gpio.c
> > +++ b/drivers/gpio/rk_gpio.c
> > @@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
> >  {
> >  	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> >  	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
> > -	char *end;
> >  	int ret;
> >  
> >  	priv->regs = dev_read_addr_ptr(dev);
> > @@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
> >  		return ret;
> >  
> >  	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
> > -	end = strrchr(dev->name, '@');
> > -	priv->bank = trailing_strtoln(dev->name, end);
> > -	priv->name[0] = 'A' + priv->bank;
> 
> > +
> > +	priv->name[0] = 'A' + dev_seq(dev);
> 
> Some comments. Have a look if it's useful.
> ===
> 1:
> Tested with rk3066a mk808 in only full u-boot.
> 
> The gpio banks have a number gap.
> 
> 	aliases {
> 		gpio0 = &gpio0;
> 		gpio1 = &gpio1;
> 		gpio2 = &gpio2;
> 		gpio3 = &gpio3;
> 		gpio4 = &gpio4;
> 
> 		gpio6 = &gpio6;
> 	};
> 
> With aliases gpio6 is called G.
> Without gpio6 is called F.
> There's no name consistency for command scripts.

True, but those scripts are going to be board specific and already
depend on board data.  The name will be consistent per board.

> ===
> 2:
> For example: rk3288-evb-u-boot.dtsi
> 
> What happens in tpl or spl with only gpio3 and gpio8 in dt-plat.c.

Is the bank name used in tpl/spl at all?  There's no script support
there and it could only be used in board code.

> Or with SPL FDT blob:
> 
> [v1,01/17] rockchip: spl: fix reloc gd and FDT blob pointer
> http://patchwork.ozlabs.org/project/uboot/patch/20220508150825.21711-2-jbx6244@gmail.com/
> 
> Not in use in mainline. Just saying there's more possible in SPL/TPL.
> ===
> The use of aliases is more of something of an additional thing that we not always can count on that is there.

Again this is board specific - if a board wants this scheme of GPIO
naming it can enable aliases and rely on that.

I have been thinking of another option which is a bigger change to the
bank name - it doesn't have to be a letter, could we use the register
address as the bank name?  So the GPIO would be something like
ff750000.1 (although I don't know how the naming scheme works, so having
a separator may not work).

What do you think of that idea?


Regards,
John

Re: [PATCH] rockchip: derive GPIO bank from alias if available

[Johan Jonker]

On 1/18/23 17:13, John Keeping wrote:
> On Tue, Jan 17, 2023 at 08:52:22PM +0100, Johan Jonker wrote:
>>
>>
>> On 1/17/23 19:15, John Keeping wrote:
>>> Upstream device trees now use standard node names like "gpio@ff..." but
>>> the rk_gpio driver expects a name like "gpio0@ff..." (note the index
>>> before the @).
>>>
>>> This is not a change that can be made in a -u-boot.dtsi file, so
>>> updating to the latest upstream device trees requires updating the
>>> driver.
>>>
>>> Other GPIO drivers already use the sequence number to derive the bank.
>>> This can be set explicitly using an alias in the device tree (which can
>>> be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
>>> order in the device tree anyway so when no aliases are supplied the
>>> indexes should still be correct.
>>>
>>> Note that the bank name is only used by the `gpio` command or
>>> potentially by board code via dm_gpio_lookup_name() and related
>>> functions; it is not used by driver code which will lookup GPIOs from
>>> the device tree by phandle.  No Rockchip platforms make any use of
>>> dm_gpio_lookup_name() or `gpio` in the boot scripts.
>>>
>>> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>> Cc: Johan Jonker <jbx6244@gmail.com>
>>> Signed-off-by: John Keeping <john@metanate.com>
>>> ---
>>>  drivers/gpio/rk_gpio.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>> index 68f30157a9..8568f10cd0 100644
>>> --- a/drivers/gpio/rk_gpio.c
>>> +++ b/drivers/gpio/rk_gpio.c
>>> @@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>  {
>>>  	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>>  	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
>>> -	char *end;
>>>  	int ret;
>>>  
>>>  	priv->regs = dev_read_addr_ptr(dev);
>>> @@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>  		return ret;
>>>  
>>>  	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>> -	end = strrchr(dev->name, '@');
>>> -	priv->bank = trailing_strtoln(dev->name, end);
>>> -	priv->name[0] = 'A' + priv->bank;
>>
>>> +
>>> +	priv->name[0] = 'A' + dev_seq(dev);
>>
>> Some comments. Have a look if it's useful.
>> ===
>> 1:
>> Tested with rk3066a mk808 in only full u-boot.
>>
>> The gpio banks have a number gap.
>>
>> 	aliases {
>> 		gpio0 = &gpio0;
>> 		gpio1 = &gpio1;
>> 		gpio2 = &gpio2;
>> 		gpio3 = &gpio3;
>> 		gpio4 = &gpio4;
>>
>> 		gpio6 = &gpio6;
>> 	};
>>
>> With aliases gpio6 is called G.
>> Without gpio6 is called F.
>> There's no name consistency for command scripts.
> 
> True, but those scripts are going to be board specific and already
> depend on board data.  The name will be consistent per board.
> 
>> ===
>> 2:
>> For example: rk3288-evb-u-boot.dtsi
>>
>> What happens in tpl or spl with only gpio3 and gpio8 in dt-plat.c.
> 
> Is the bank name used in tpl/spl at all?  There's no script support
> there and it could only be used in board code.
> 
>> Or with SPL FDT blob:
>>
>> [v1,01/17] rockchip: spl: fix reloc gd and FDT blob pointer
>> http://patchwork.ozlabs.org/project/uboot/patch/20220508150825.21711-2-jbx6244@gmail.com/
>>
>> Not in use in mainline. Just saying there's more possible in SPL/TPL.
>> ===
>> The use of aliases is more of something of an additional thing that we not always can count on that is there.
> 
> Again this is board specific - if a board wants this scheme of GPIO
> naming it can enable aliases and rely on that.
> 
> I have been thinking of another option which is a bigger change to the
> bank name - it doesn't have to be a letter, could we use the register
> address as the bank name?  So the GPIO would be something like
> ff750000.1 (although I don't know how the naming scheme works, so having
> a separator may not work).
> 
> What do you think of that idea?
> 
> 
> Regards,
> John

Hi,

I might changed my mind.
Nothing fixed yet.
It all depends on the DT maintainers.
See gpio.txt
Maybe we should use the gpio-ranges property parsing to derive it's ID.
Adding it to all gpio nodes like rk3588s.dtsi
Is that useful for other Rockchip SoCs?

&gpio1 {
			gpio-ranges = <&pinctrl 0 32 32>;
};

Bank size is 32.
ID = offset / 32

===

See Linux gpio-rcar.c example:

	struct of_phandle_args args;

	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
	
	//*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;

	//if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
	//	dev_warn(p->dev, "Invalid number of gpio lines %u, using %u\n",
	//		 *npins, RCAR_MAX_GPIO_PER_BANK);
	//	*npins = RCAR_MAX_GPIO_PER_BANK;
	//}

===

Give me a bit time for patches.

Is that property suitable?
Let me know what you think of that.

Johan

===

The gpio-ranges property described below represents this with
a discrete set of ranges mapping pins from the pin controller local number space
to pins in the GPIO controller local number space.

The format is: <[pin controller phandle], [GPIO controller offset], [pin controller offset], [number of pins]>;

Re: [PATCH] rockchip: derive GPIO bank from alias if available

[Kever Yang]

Hi John,

     I have pick below patch for gpio bank support, which is the same 
logic in kernel and other SoCs.

https://patchwork.ozlabs.org/project/uboot/patch/20230213222742.135093-2-macroalpha82@gmail.com/


Thanks,

- Kever

On 2023/1/18 02:15, John Keeping wrote:
> Upstream device trees now use standard node names like "gpio@ff..." but
> the rk_gpio driver expects a name like "gpio0@ff..." (note the index
> before the @).
>
> This is not a change that can be made in a -u-boot.dtsi file, so
> updating to the latest upstream device trees requires updating the
> driver.
>
> Other GPIO drivers already use the sequence number to derive the bank.
> This can be set explicitly using an alias in the device tree (which can
> be added in a -u-boot.dtsi file) but Rockchip GPIO banks are defined in
> order in the device tree anyway so when no aliases are supplied the
> indexes should still be correct.
>
> Note that the bank name is only used by the `gpio` command or
> potentially by board code via dm_gpio_lookup_name() and related
> functions; it is not used by driver code which will lookup GPIOs from
> the device tree by phandle.  No Rockchip platforms make any use of
> dm_gpio_lookup_name() or `gpio` in the boot scripts.
>
> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Cc: Johan Jonker <jbx6244@gmail.com>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>   drivers/gpio/rk_gpio.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> index 68f30157a9..8568f10cd0 100644
> --- a/drivers/gpio/rk_gpio.c
> +++ b/drivers/gpio/rk_gpio.c
> @@ -142,7 +142,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
>   {
>   	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>   	struct rockchip_gpio_priv *priv = dev_get_priv(dev);
> -	char *end;
>   	int ret;
>   
>   	priv->regs = dev_read_addr_ptr(dev);
> @@ -151,9 +150,8 @@ static int rockchip_gpio_probe(struct udevice *dev)
>   		return ret;
>   
>   	uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
> -	end = strrchr(dev->name, '@');
> -	priv->bank = trailing_strtoln(dev->name, end);
> -	priv->name[0] = 'A' + priv->bank;
> +
> +	priv->name[0] = 'A' + dev_seq(dev);
>   	uc_priv->bank_name = priv->name;
>   
>   	return 0;