[PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider
Jonas Karlman
jonas at kwiboo.se
Sun Mar 19 15:56:50 CET 2023
On 2023-03-19 14:51, Jonas Karlman wrote:
> 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?
I have now found your latest linux series at [2].
After reading a little bit more I can better understand the using of
gpio-ranges and that there is an end goal of moving the gpio bank nodes
outside the pinctrl node. Together with the HW limitation that a gpio
bank only can handle up to 32 pins, using the gpio-ranges makes more sense.
I will send a v2 of my series with minor changes to the probe function.
[2] https://patchwork.ozlabs.org/project/linux-gpio/list/?series=343421
Regards,
Jonas
>
> 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
>>>
>
More information about the U-Boot
mailing list