| Message ID | e5d4d2d9-f1f8-3ff5-ab08-ad00c8814aee@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v1,1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider | expand |
Hi Johan, On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@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? > > Signed-off-by: Johan Jonker <jbx6244@gmail.com> > --- > drivers/gpio/rk_gpio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@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
On 3/18/23 21:20, Simon Glass wrote: > Hi Johan, > > On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6244@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. === 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@gmail.com> >> --- >> drivers/gpio/rk_gpio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@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
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@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? [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@gmail.com> >>> --- >>> drivers/gpio/rk_gpio.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Reviewed-by: Simon Glass <sjg@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
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@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. 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) 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) > > [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@gmail.com> >>>> --- >>>> drivers/gpio/rk_gpio.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Reviewed-by: Simon Glass <sjg@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 >
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@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? 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@gmail.com> >>>>> --- >>>>> drivers/gpio/rk_gpio.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> Reviewed-by: Simon Glass <sjg@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 >>
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@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-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@gmail.com> >>>>>> --- >>>>>> drivers/gpio/rk_gpio.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> Reviewed-by: Simon Glass <sjg@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 >>> >
Hi Johan, On 2023/3/19 19: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@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. Could you share which TRM did you find gpio-banks is not 32 pins? The design should be 32 pins per bank for all the SoCs, although some banks may not have full 32 pins, but all the pin ID should follow the rules with 32pin per bank. Thanks, - Kever > 2: The "gpio-ranges" syntax allows multiple items with variable number of pins. > > === > > 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@gmail.com> >>> --- >>> drivers/gpio/rk_gpio.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> Reviewed-by: Simon Glass <sjg@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
On 3/20/23 02:32, Kever Yang wrote: > Hi Johan, > > On 2023/3/19 19: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@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. > > Could you share which TRM did you find gpio-banks is not 32 pins? px2==rk3066: Rockchip PX2 TRM V1.0.pdf page 20 GPIO 6 groups of GPIO (GPIO0~GPIO4, GPIO6) , 32 GPIOs per group in GPIO0~GPIO4, and 16 GPIOs in GPIO6, totally have 176 GPIOs gpio6 is incomplete, mostly only A+B registers but never 32 pin. === rk3288-chapter-01-introduction.pdf page 15 GPIO Totally 160 GPIOs gpio0 only A+B+C gpio8 only A+B === rk3066 : 16 : https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3066.c#L83 rk3288: 24: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3288.c#L185 16: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/rockchip/pinctrl-rk3288.c#L213 > > The design should be 32 pins per bank for all the SoCs, although some banks may not have full 32 pins, > > but all the pin ID should follow the rules with 32pin per bank. That's correct. This patch does not break that formula. Just don't use args[2]. number of pins != divider > > > Thanks, > > - Kever > >> 2: The "gpio-ranges" syntax allows multiple items with variable number of pins. >> >> === >> >> 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@gmail.com> >>>> --- >>>> drivers/gpio/rk_gpio.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> Reviewed-by: Simon Glass <sjg@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
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, '@');
The current divider to calculate the bank ID can change. Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider. Signed-off-by: Johan Jonker <jbx6244@gmail.com> --- drivers/gpio/rk_gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.1