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 <jbx6...@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@fdd60000"; > gpio3 = "/pinctrl/gpio@fe760000"; > serial2 = "/serial@fe660000"; > spi0 = "/spi@fe300000"; > mmc0 = "/mmc@fe310000"; > mmc1 = "/mmc@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-jo...@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 <jbx6...@gmail.com> >>>>>> --- >>>>>> drivers/gpio/rk_gpio.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> Reviewed-by: Simon Glass <s...@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 >>> >