[PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Jonas Karlman]

On 2023-03-19 13:55, Johan Jonker wrote:
> 
> 
> On 3/19/23 13:20, Jonas Karlman wrote:
>> Hi Johan,
>> On 2023-03-19 12:34, Johan Jonker wrote:
>>>
>>>
>>> On 3/18/23 21:20, Simon Glass wrote:
>>>> Hi Johan,
>>>>
>>>> On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244 at gmail.com> wrote:
>>>>>
>>>>> The current divider to calculate the bank ID can change.
>>>>> Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
>>>>
>>>
>>>> What is the motivation for this patch?
>>>
>>> The gpio-ranges property format:
>>>
>>> gpio-ranges = <[pin controller phandle], [GPIO controller offset],
>>>                 [pin controller offset], [number of pins]>;
>>>
>>> 1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.
>>> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins.
>>
>> Is there a reason why gpio-ranges is used to determine bank id?
>>
>> In the series at [1], add support for rk35xx gpio banks, sent yesterday
>> I changed this to rely on the dev alias id, same as in the linux driver.
>>
>> Any reason why using the device alias id won't work for the same purpose
>> in u-boot?
> 
> Aliases are board/user orientated based on availability.
> See discussion elsewhere(by Arnd Bergmann and others, see lore link unknown for now)
> All aliases should be moved out of the dtsi files, so there's no guaranty that they are there anymore.
> Aliases should follow the Rockchip TRM naming to prevent confusion.(by Heiko)
> Special rules apply to mmc aliases: based on availability, reg order and without number gap. 

Thanks for this information.

> 
> U-Boot/bootloader specific:
> Aliases get lost with reduced DT's and are problematic with overlays,etc(ie. it needs special handling on top of that).
> Aliases are not in the dtb platdata structure made by dtoc for TPL/SPL.(current drivers don't work yet, but in the future they may)

Aliases seem to be included in the reduced DT compiled for SPL for my
Quartz64-A board, see below. For dtb platdata I can see this as a problem,
yet the rk gpio driver does not support dtb platdata and it has spl
helper functions a board can use if SPL_OF_REAL is not an option.
Do you have plans to add platdata support to the rk gpio driver?

  model = "Pine64 RK3566 Quartz64-A Board";

  aliases {
    gpio0 = "/pinctrl/gpio at fdd60000";
    gpio3 = "/pinctrl/gpio at fe760000";
    serial2 = "/serial at fe660000";
    spi0 = "/spi at fe300000";
    mmc0 = "/mmc at fe310000";
    mmc1 = "/mmc at fe2b0000";
  };

I am fairly new to DT overlays, how is alias problematic with overlays?

> 
> Best is to have a property inside the node for parsing and reduced nodes.
> gpio-ranges is the only option currently available.
> (not saying that is perfect, but at least don't invent new properties that needs to be continuously supported as legacy)
> 

My understanding is that this is also just abusing the gpio-ranges prop.
Still not understanding the advantage compare to using the aliases.

gpio-ranges advantages:
- prop in node that gets included in dtb platdata

aliases advantages:
- already (ab)used by linux driver
- fewer lines of code in driver and u-boot.dtsi

Both options abuse the device tree, at least with aliases we abuse it
in the same way as the linux driver. Or are there patches to change
this behavior also for the linux driver?

Regards,
Jonas

> 
>>
>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230318235651.826148-3-jonas@kwiboo.se/
>>
>> Regards,
>> Jonas
>>
>>>
>>> ===
>>>
>>> Theoretical example:
>>>
>>> gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1
>>>
>>> vs.
>>>
>>> gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
>>>               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1
>>>
>>> Both descriptions are valid.
>>> The number of pins in the second example is reduced to 16 per item.
>>> Using that as divider will give a wrong bank number.
>>> Use a constant instead.
>>> For the Rockchip situation simple parsing the first item is enough the know it's bank number for now.
>>> To do it correct one could parse a list of gpio-range items, but that makes it more complicated.
>>>
>>> From gpio.txt:
>>> Each offset runs from 0 to N. It is perfectly fine to pile any number of
>>> ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
>>> in practice these ranges are often lumped in discrete sets.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Johan Jonker <jbx6244 at gmail.com>
>>>>> ---
>>>>>  drivers/gpio/rk_gpio.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>
>>>>>
>>>>> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
>>>>> index f7ad4d68..0a2acf18 100644
>>>>> --- a/drivers/gpio/rk_gpio.c
>>>>> +++ b/drivers/gpio/rk_gpio.c
>>>>> @@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
>>>>>                                              0, &args);
>>>>>         if (!ret || ret != -ENOENT) {
>>>>>                 uc_priv->gpio_count = args.args[2];
>>>>> -               priv->bank = args.args[1] / args.args[2];
>>>>> +               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
>>>>>         } else {
>>>>>                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
>>>>>                 end = strrchr(dev->name, '@');
>>>>> --
>>>>> 2.20.1
>>>>>
>>>>
>>>> Regards,
>>>> SImon
>>