[PATCH] Allow configuration of ARCH_NR_GPIO

[PATCH] Allow configuration of ARCH_NR_GPIO

[Billie Alsup]

From: Billie R Alsup <balsup@cisco.com>

Problem: Some systems support a high number of GPIO pins.  Allow
the kernel builder to configure the maximum number of pins, rather
than forcing a large value on everyone.

Impact: Once a .config is generated, the ARCH_NR_GPIO setting
will persist.  To return to a default setting, the CONFIG_ARCH_NR_GPIO
must be removed from .config file first.

Trade-offs: It is possible to achieve similar via command line
parameters, e.g.

    make KBUILD_CFLAGS_KERNEL=-DARCH_NR_GPIOS=16384

to the build. This is problematic because the setting does not
show up in /proc/config.gz.  It is also problematic for out-of-tree
module builds, which require similar if they invoke the API
gpio_is_valid().  In both cases, one could envision one system
working normally, and another failing, even though they both have
the same kernel version and /proc/config.gz.  Therefore, it is
better to have the setting available in .config

Signed-off-by: Billie R Alsup <balsup@cisco.com>
---
 arch/arm/Kconfig | 2 +-
 arch/x86/Kconfig | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 7630ba9cb6cc..7fc6e52d1d3c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1226,7 +1226,7 @@ config ARM_PSCI
 # a multiplatform kernel, we just want the highest value required by the
 # selected platforms.
 config ARCH_NR_GPIO
-	int
+	int "Maximum number of GPIOs supported"
 	default 2048 if ARCH_INTEL_SOCFPGA
 	default 1024 if ARCH_BRCMSTB || ARCH_RENESAS || ARCH_TEGRA || \
 		ARCH_ZYNQ || ARCH_ASPEED
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 52a7f91527fe..a59cef517f56 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -347,9 +347,13 @@ config ARCH_HIBERNATION_POSSIBLE
 	def_bool y
 
 config ARCH_NR_GPIO
-	int
+	int "Maximum number of GPIOs supported"
 	default 1024 if X86_64
 	default 512
+	help
+	  Maximum number of GPIOs in the system.
+
+	  If unsure, leave the default value.
 
 config ARCH_SUSPEND_POSSIBLE
 	def_bool y
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Allow configuration of ARCH_NR_GPIO

[Arnd Bergmann]

On Sat, Jul 30, 2022 at 2:38 AM Billie Alsup <balsup@cisco.com> wrote:
>
> From: Billie R Alsup <balsup@cisco.com>
>
> Problem: Some systems support a high number of GPIO pins.  Allow
> the kernel builder to configure the maximum number of pins, rather
> than forcing a large value on everyone.
>
> Impact: Once a .config is generated, the ARCH_NR_GPIO setting
> will persist.  To return to a default setting, the CONFIG_ARCH_NR_GPIO
> must be removed from .config file first.
>
> Trade-offs: It is possible to achieve similar via command line
> parameters, e.g.
>
>     make KBUILD_CFLAGS_KERNEL=-DARCH_NR_GPIOS=16384
>
> to the build. This is problematic because the setting does not
> show up in /proc/config.gz.  It is also problematic for out-of-tree
> module builds, which require similar if they invoke the API
> gpio_is_valid().  In both cases, one could envision one system
> working normally, and another failing, even though they both have
> the same kernel version and /proc/config.gz.  Therefore, it is
> better to have the setting available in .config
>
> Signed-off-by: Billie R Alsup <balsup@cisco.com>

What is the use case for manually setting this rather than deriving
it from the selected platforms?

Have you tried to use both a platform-specific option for the minimum
number of this setting, and then restictricting the CONFIG_ARCH_NR_GPIO
setting with a 'range' statement?

         Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Allow configuration of ARCH_NR_GPIO

[Billie Alsup (balsup)]

  >On 7/30/22, 4:54 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:

  >What is the use case for manually setting this rather than deriving
  >it from the selected platforms?

A Cisco GPIO IP block supports 1022 pins.  A single FPGA can support 
multiple GPIO IP blocks (although typically 1 or 2).  A system may 
have two or three FPGAs directly accessible through the PCI bus, and 
an additional 8 to 36 FPGAS accessible through alternate means (e.g., 
SLPC, i2c, or a proprietary point-to-point bus).  Although typically 
the full set of pins is not used, in total, it is very easy to exceed 
the limit.  Previous patches have simply bumped up the number, and I 
thought a more generic approach that would solve the use case for 
customer devices would be more appropriate.  I am also trying to keep 
the ARM and X86 implementations similar, as the devices are 
accessible from both an ARM-based BMC as well as an X86 type CPU.  It 
is desirable to use the same kernel configuration for multiple 
products, even though the number of FPGAS, and therefore the number 
of supported GPIO pins can vary.  Such is also the case for Open NOS 
environments, such as SONiC and FBOSS, which attempt to use a single 
kernel configuration across multiple (multi-vendor) products. A 
platform specific option would not seem to work in such cases.  
Ideally, I would like to see this configuration knob go away 
completely, but that might be a more complicated and controversial 
change.  It is not used in very many places, but the problematic use 
is within the inline function gpio_is_valid.

  >Have you tried to use both a platform-specific option for the minimum
  >number of this setting, and then restictricting the 
  >CONFIG_ARCH_NR_GPIO
  >setting with a 'range' statement?

I have not tried a platform specific option, as we would need to 
choose the worst case platform for such a configuration (in order to 
share a kernel build across products), but this in turn may conflict 
with the chosen platform for some other “worst-case” setting of 
another variable.  It seems prudent to simply allow the kernel 
builder to choose the options they want.  This is just one example
where the configuration is not directly available to the kernel 
builder, but must be set indirectly.  We have other configuration 
options that cannot be set directly, and instead require us to enable 
an unrelated (from a product standpoint) configuration item purely 
for the side-effect of enabling such an option.  CONFIG_ARCH_NR_GPIO 
is a similar issue, where you choose a platform to indirectly set the 
value, but that value really needs to be configurable by the builder. 
The last such patch for X86 simply bumped up the number.  I thought 
it better to simply allow configuration.  I have no issue with 
restricting the value to a minimum, if that is a worry.

I note that it doesn't seem that this setting has a negative impact on
resources within the system.  Only one driver (gpio-aggregator) performs
memory allocation based on this value, and that is only for the duration
of a single function call to aggr_parse.  Otherwise, there does not seem
to be any negative consequence to having a higher limit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Allow configuration of ARCH_NR_GPIO

[Arnd Bergmann]

On Sun, Jul 31, 2022 at 12:33 AM Billie Alsup (balsup) <balsup@cisco.com> wrote:
>   >On 7/30/22, 4:54 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:
>
>   >What is the use case for manually setting this rather than deriving
>   >it from the selected platforms?
>
> A Cisco GPIO IP block supports 1022 pins.  A single FPGA can support
> multiple GPIO IP blocks (although typically 1 or 2).  A system may
> have two or three FPGAs directly accessible through the PCI bus, and
> an additional 8 to 36 FPGAS accessible through alternate means (e.g.,
> SLPC, i2c, or a proprietary point-to-point bus).  Although typically
> the full set of pins is not used, in total, it is very easy to exceed
> the limit.  Previous patches have simply bumped up the number, and I
> thought a more generic approach that would solve the use case for
> customer devices would be more appropriate.  I am also trying to keep
> the ARM and X86 implementations similar, as the devices are
> accessible from both an ARM-based BMC as well as an X86 type CPU.

It would be nice to keep it consistent between the major architectures,
at least arm64, powerpc, mips and riscv in addition to the two you already
have.

One way we could do this is to keep the existing
CONFIG_ARCH_NR_GPIO for on-chip GPIOs, and then add
a new Kconfig option for extra GPIOs on things like PCI cards or
programmable logic.

>  It is desirable to use the same kernel configuration for multiple
> products, even though the number of FPGAS, and therefore the number
> of supported GPIO pins can vary.  Such is also the case for Open NOS
> environments, such as SONiC and FBOSS, which attempt to use a single
> kernel configuration across multiple (multi-vendor) products. A
> platform specific option would not seem to work in such cases.
> Ideally, I would like to see this configuration knob go away
> completely, but that might be a more complicated and controversial
> change.  It is not used in very many places, but the problematic use
> is within the inline function gpio_is_valid.

I think the plan for making the option obsolete is to convert every user
of the legacy GPIO interface to use GPIO descriptors, which don't
have the limitation and don't need gpio_is_valid().

In your use case, can you identify a set of drivers that need access
to old-style GPIO numbers above the hardcoded on-chip numbers?
Maybe we can prioritize fixing those drivers.

>   >Have you tried to use both a platform-specific option for the minimum
>   >number of this setting, and then restictricting the
>   >CONFIG_ARCH_NR_GPIO
>   >setting with a 'range' statement?
>
> I have not tried a platform specific option, as we would need to
> choose the worst case platform for such a configuration (in order to
> share a kernel build across products), but this in turn may conflict
> with the chosen platform for some other “worst-case” setting of
> another variable.  It seems prudent to simply allow the kernel
> builder to choose the options they want.  This is just one example
> where the configuration is not directly available to the kernel
> builder, but must be set indirectly.  We have other configuration
> options that cannot be set directly, and instead require us to enable
> an unrelated (from a product standpoint) configuration item purely
> for the side-effect of enabling such an option.  CONFIG_ARCH_NR_GPIO
> is a similar issue, where you choose a platform to indirectly set the
> value, but that value really needs to be configurable by the builder.
> The last such patch for X86 simply bumped up the number.  I thought
> it better to simply allow configuration.  I have no issue with
> restricting the value to a minimum, if that is a worry.

I have no problem with allowing a kernel build to use larger values
than the default, I'm just worried that allowing smaller values than what
we know is needed for the existing platforms can break distro kernels
that have to work across many machines.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Allow configuration of ARCH_NR_GPIO

[Billie Alsup (balsup)]

On 7/31/22, 4:44 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:

    >One way we could do this is to keep the existing
    >CONFIG_ARCH_NR_GPIO for on-chip GPIOs, and then add
    >a new Kconfig option for extra GPIOs on things like PCI cards or
    >programmable logic.

Thank you for your feedback.  This makes sense to me, but your
next statement has me rethinking the problem.

   > In your use case, can you identify a set of drivers that need access
    >to old-style GPIO numbers above the hardcoded on-chip numbers?
    >Maybe we can prioritize fixing those drivers.

For FBOSS, libgpio is used, so controller specific offsets apply.  The only
issue is that our gpio controller allows the kernel to set the base pin
number, and that will fail with a small ARCH_NR_GPIOS. We can workaround
this issue by explicitly assigning a base pin number (as a property
read from device-tree or ACPI).  For SONiC however, GPIO_SYSFS is still
used.  From code inspection, the gpio_is_valid call would only apply
if a pin number were used that does not yet exist.  Such would be the
case where the pin numbers are predefined, but perhaps the controller
has not yet been hot-plugged.  In such cases a pr_warn is emitted from
gpio_to_desc.  However, I think this should be ok (as it would be only
an occasional race condition when the application still thought a device
was plugged in and tried to access the device, while a user was in the
process of removing said device).

Let me run some experiments with our controllers passing in an explicit
base pin number instead of letting the kernel auto-assign it.  This will of
course be outside of the ARCH_NR_GPIO setting.  If this is successful,
then no Kconfig changes are necessary.  If there are still issues, then
I will follow up with a V2 that sets an EXTRA_NR_GPIO or similar in
drivers/gpio/Kconfig as you have suggested.  This can default to 0.
This ensures that the platform specified CONFIG_ARCH_NR_GPIO is not
set too low (inconsistent with the platform requirements), and gives
us the flexibility to support additional controllers.  This might be
necessary if we hit the ARCH_NR_GPIO limit with generic i2c gpio
expanders (although even in that case, reading the base pin from 
device-tree or ACPI seems like a better solution).  gpio-pca953x does
allow setting the base today from platform data, but we prefer
device-tree and ACPI over the platform data technique.

Thank again for your feedback!


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel