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

[Johan Jonker <jbx6244@gmail.com>]

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]>;