[v2,5/9] gpiolib: Get rid of ARCH_NR_GPIOS

Since commit 14e85c0e69d5 ("gpio: remove gpio_descs global array")
there is no limitation on the number of GPIOs that can be allocated
in the system since the allocation is fully dynamic.

ARCH_NR_GPIOS is today only used in order to provide downwards
gpiobase allocation from that value, while static allocation is
performed upwards from 0. However that has the disadvantage of
limiting the number of GPIOs that can be registered in the system.

To overcome this limitation without requiring each and every
platform to provide its 'best-guess' maximum number, rework the
allocation to allocate upwards, allowing approx 2 millions of
GPIOs.

In order to still allow static allocation for legacy drivers, define
GPIO_DYNAMIC_BASE with the value 512 as the start for dynamic
allocation. The 512 value is chosen because it is the end of
the current default range so all current static allocations are
expected to be below that value. Of course that's just a rough
estimate based on the default value, but assuming static
allocations come first, even if there are more static allocations
it should fit under the 512 value.

In the future, it is expected that all static allocations go away
and then dynamic allocation will be patched to start at 0.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Enhanced commit description and change from 256 to 512.
---
 arch/arm/include/asm/gpio.h |  1 -
 drivers/gpio/gpiolib.c      | 10 +++----
 include/asm-generic/gpio.h  | 55 ++++++++++++++-----------------------
 3 files changed, 26 insertions(+), 40 deletions(-)

On Fri, Sep 2, 2022 at 4:57 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Since commit 14e85c0e69d5 ("gpio: remove gpio_descs global array")
> there is no limitation on the number of GPIOs that can be allocated
> in the system since the allocation is fully dynamic.
>
> ARCH_NR_GPIOS is today only used in order to provide downwards
> gpiobase allocation from that value, while static allocation is
> performed upwards from 0. However that has the disadvantage of
> limiting the number of GPIOs that can be registered in the system.
>
> To overcome this limitation without requiring each and every
> platform to provide its 'best-guess' maximum number, rework the
> allocation to allocate upwards, allowing approx 2 millions of
> GPIOs.
>
> In order to still allow static allocation for legacy drivers, define
> GPIO_DYNAMIC_BASE with the value 512 as the start for dynamic
> allocation. The 512 value is chosen because it is the end of
> the current default range so all current static allocations are
> expected to be below that value. Of course that's just a rough
> estimate based on the default value, but assuming static
> allocations come first, even if there are more static allocations
> it should fit under the 512 value.
>
> In the future, it is expected that all static allocations go away
> and then dynamic allocation will be patched to start at 0.

Eventually we have to get rid of gpio_is_valid() completely...
But this is another story.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: Enhanced commit description and change from 256 to 512.
> ---
>  arch/arm/include/asm/gpio.h |  1 -
>  drivers/gpio/gpiolib.c      | 10 +++----
>  include/asm-generic/gpio.h  | 55 ++++++++++++++-----------------------
>  3 files changed, 26 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/include/asm/gpio.h b/arch/arm/include/asm/gpio.h
> index f3bb8a2bf788..4ebbb58f06ea 100644
> --- a/arch/arm/include/asm/gpio.h
> +++ b/arch/arm/include/asm/gpio.h
> @@ -2,7 +2,6 @@
>  #ifndef _ARCH_ARM_GPIO_H
>  #define _ARCH_ARM_GPIO_H
>
> -/* Note: this may rely upon the value of ARCH_NR_GPIOS set in mach/gpio.h */
>  #include <asm-generic/gpio.h>
>
>  /* The trivial gpiolib dispatchers */
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4e2fcb7b0a01..1846f24971e3 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -183,14 +183,14 @@ EXPORT_SYMBOL_GPL(gpiod_to_chip);
>  static int gpiochip_find_base(int ngpio)
>  {
>         struct gpio_device *gdev;
> -       int base = ARCH_NR_GPIOS - ngpio;
> +       int base = GPIO_DYNAMIC_BASE;
>
> -       list_for_each_entry_reverse(gdev, &gpio_devices, list) {
> +       list_for_each_entry(gdev, &gpio_devices, list) {
>                 /* found a free space? */
> -               if (gdev->base + gdev->ngpio <= base)
> +               if (gdev->base >= base + ngpio)
>                         break;
> -               /* nope, check the space right before the chip */
> -               base = gdev->base - ngpio;
> +               /* nope, check the space right after the chip */
> +               base = gdev->base + gdev->ngpio;
>         }
>
>         if (gpio_is_valid(base)) {
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index aea9aee1f3e9..a7752cf152ce 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -11,40 +11,18 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>
>
> -/* Platforms may implement their GPIO interface with library code,
> +/*
> + * Platforms may implement their GPIO interface with library code,
>   * at a small performance cost for non-inlined operations and some
>   * extra memory (for code and for per-GPIO table entries).
> - *
> - * While the GPIO programming interface defines valid GPIO numbers
> - * to be in the range 0..MAX_INT, this library restricts them to the
> - * smaller range 0..ARCH_NR_GPIOS-1.
> - *
> - * ARCH_NR_GPIOS is somewhat arbitrary; it usually reflects the sum of
> - * builtin/SoC GPIOs plus a number of GPIOs on expanders; the latter is
> - * actually an estimate of a board-specific value.
>   */
>
> -#ifndef ARCH_NR_GPIOS
> -#if defined(CONFIG_ARCH_NR_GPIO) && CONFIG_ARCH_NR_GPIO > 0
> -#define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
> -#else
> -#define ARCH_NR_GPIOS          512
> -#endif
> -#endif
> -
>  /*
> - * "valid" GPIO numbers are nonnegative and may be passed to
> - * setup routines like gpio_request().  only some valid numbers
> - * can successfully be requested and used.
> - *
> - * Invalid GPIO numbers are useful for indicating no-such-GPIO in
> - * platform data and other tables.
> + * At the end we want all GPIOs to be dynamically allocated from 0.
> + * However, some legacy drivers still perform fixed allocation.
> + * Until they are all fixed, leave 0-512 space for them.
>   */
> -
> -static inline bool gpio_is_valid(int number)
> -{
> -       return number >= 0 && number < ARCH_NR_GPIOS;
> -}
> +#define GPIO_DYNAMIC_BASE      512
>
>  struct device;
>  struct gpio;
> @@ -140,12 +118,6 @@ static inline void gpio_unexport(unsigned gpio)
>
>  #include <linux/kernel.h>
>
> -static inline bool gpio_is_valid(int number)
> -{
> -       /* only non-negative numbers are valid */
> -       return number >= 0;
> -}
> -
>  /* platforms that don't directly support access to GPIOs through I2C, SPI,
>   * or other blocking infrastructure can use these wrappers.
>   */
> @@ -169,4 +141,19 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
>
>  #endif /* !CONFIG_GPIOLIB */
>
> +/*
> + * "valid" GPIO numbers are nonnegative and may be passed to
> + * setup routines like gpio_request().  only some valid numbers
> + * can successfully be requested and used.
> + *
> + * Invalid GPIO numbers are useful for indicating no-such-GPIO in
> + * platform data and other tables.
> + */
> +
> +static inline bool gpio_is_valid(int number)
> +{
> +       /* only non-negative numbers are valid */
> +       return number >= 0;
> +}
> +
>  #endif /* _ASM_GENERIC_GPIO_H */
> --
> 2.37.1
>

Le 02/09/2022 à 16:58, Andy Shevchenko a écrit :
> On Fri, Sep 2, 2022 at 4:57 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>> Since commit 14e85c0e69d5 ("gpio: remove gpio_descs global array")
>> there is no limitation on the number of GPIOs that can be allocated
>> in the system since the allocation is fully dynamic.
>>
>> ARCH_NR_GPIOS is today only used in order to provide downwards
>> gpiobase allocation from that value, while static allocation is
>> performed upwards from 0. However that has the disadvantage of
>> limiting the number of GPIOs that can be registered in the system.
>>
>> To overcome this limitation without requiring each and every
>> platform to provide its 'best-guess' maximum number, rework the
>> allocation to allocate upwards, allowing approx 2 millions of
>> GPIOs.
>>
>> In order to still allow static allocation for legacy drivers, define
>> GPIO_DYNAMIC_BASE with the value 512 as the start for dynamic
>> allocation. The 512 value is chosen because it is the end of
>> the current default range so all current static allocations are
>> expected to be below that value. Of course that's just a rough
>> estimate based on the default value, but assuming static
>> allocations come first, even if there are more static allocations
>> it should fit under the 512 value.
>>
>> In the future, it is expected that all static allocations go away
>> and then dynamic allocation will be patched to start at 0.
> 
> Eventually we have to get rid of gpio_is_valid() completely...
> But this is another story.
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Yes that could be done as a follow-up.

There are about 300 call sites.

Should simply replace gpio_is_valid(gpio) by gpio >= 0. And then verify 
that the check is really required. But needs to check signness of gpio 
at every place.

First look seems already promissing:


int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
{
	struct gpio_desc *desc;
	int err;

	desc = gpio_to_desc(gpio);

	/* Compatibility: assume unavailable "valid" GPIOs will appear later */
	if (!desc && gpio_is_valid(gpio))
		return -EPROBE_DEFER;