At the time being, the default maximum number of GPIOs is set to 512
and can only get customised via an architecture specific
CONFIG_ARCH_NR_GPIO.
The maximum number of GPIOs might be dependent on the number of
interface boards and is somewhat independent of architecture.
Allow the user to select that maximum number outside of any
architecture configuration. To enable that, re-define a
core CONFIG_ARCH_NR_GPIO for architectures which don't already
define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
On arm, do like x86 and set 512 as the default instead of 0, that
allows simplifying the logic in asm-generic/gpio.h
Signed-off-by: Christophe Leroy <[email protected]>
---
Documentation/driver-api/gpio/legacy.rst | 2 +-
arch/arm/Kconfig | 3 ++-
arch/arm/include/asm/gpio.h | 1 -
arch/x86/Kconfig | 1 +
drivers/gpio/Kconfig | 14 ++++++++++++++
include/asm-generic/gpio.h | 6 ------
6 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
index 9b12eeb89170..566b06a584cf 100644
--- a/Documentation/driver-api/gpio/legacy.rst
+++ b/Documentation/driver-api/gpio/legacy.rst
@@ -558,7 +558,7 @@ Platform Support
To force-enable this framework, a platform's Kconfig will "select" GPIOLIB,
else it is up to the user to configure support for GPIO.
-It may also provide a custom value for ARCH_NR_GPIOS, so that it better
+It may also provide a custom value for CONFIG_ARCH_NR_GPIO, so that it better
reflects the number of GPIOs in actual use on that platform, without
wasting static table space. (It should count both built-in/SoC GPIOs and
also ones on GPIO expanders.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 53e6a1da9af5..e55b6560fe4f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -14,6 +14,7 @@ config ARM
select ARCH_HAS_KCOV
select ARCH_HAS_MEMBARRIER_SYNC_CORE
select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
+ select ARCH_HAS_NR_GPIO
select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
select ARCH_HAS_PHYS_TO_DMA
select ARCH_HAS_SETUP_DMA_OPS
@@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
default 352 if ARCH_VT8500
default 288 if ARCH_ROCKCHIP
default 264 if MACH_H4700
- default 0
+ default 512
help
Maximum number of GPIOs in the system.
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/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..a8c956abc21e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -82,6 +82,7 @@ config X86
select ARCH_HAS_MEM_ENCRYPT
select ARCH_HAS_MEMBARRIER_SYNC_CORE
select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
+ select ARCH_HAS_NR_GPIO
select ARCH_HAS_PMEM_API if X86_64
select ARCH_HAS_PTE_DEVMAP if X86_64
select ARCH_HAS_PTE_SPECIAL
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0642f579196f..250b50ed44e1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -11,6 +11,9 @@ config ARCH_HAVE_CUSTOM_GPIO_H
overriding the default implementations. New uses of this are
strongly discouraged.
+config ARCH_HAS_NR_GPIO
+ bool
+
menuconfig GPIOLIB
bool "GPIO Support"
help
@@ -22,6 +25,17 @@ menuconfig GPIOLIB
if GPIOLIB
+config ARCH_NR_GPIO
+ int "Maximum number of GPIOs" if EXPERT
+ depends on !ARCH_HAS_NR_GPIO
+ default 512
+ help
+ This allows the user to select the maximum number of GPIOs the
+ kernel must support. When the architecture defines a number
+ through CONFIG_ARCH_NR_GPIO, that value is taken and the user
+ cannot change it. Otherwise it defaults to 512 and the user
+ can change it when CONFIG_EXPERT is set.
+
config GPIOLIB_FASTPATH_LIMIT
int "Maximum number of GPIOs for fast path"
range 32 512
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index aea9aee1f3e9..ee090f534ab8 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -24,13 +24,7 @@
* 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
--
2.37.1
On August 9, 2022 3:40:38 AM PDT, Christophe Leroy <[email protected]> wrote:
>At the time being, the default maximum number of GPIOs is set to 512
>and can only get customised via an architecture specific
>CONFIG_ARCH_NR_GPIO.
>
>The maximum number of GPIOs might be dependent on the number of
>interface boards and is somewhat independent of architecture.
>
>Allow the user to select that maximum number outside of any
>architecture configuration. To enable that, re-define a
>core CONFIG_ARCH_NR_GPIO for architectures which don't already
>define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>
>Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>
>On arm, do like x86 and set 512 as the default instead of 0, that
>allows simplifying the logic in asm-generic/gpio.h
>
>Signed-off-by: Christophe Leroy <[email protected]>
>---
> Documentation/driver-api/gpio/legacy.rst | 2 +-
> arch/arm/Kconfig | 3 ++-
> arch/arm/include/asm/gpio.h | 1 -
> arch/x86/Kconfig | 1 +
> drivers/gpio/Kconfig | 14 ++++++++++++++
> include/asm-generic/gpio.h | 6 ------
> 6 files changed, 18 insertions(+), 9 deletions(-)
>
>diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
>index 9b12eeb89170..566b06a584cf 100644
>--- a/Documentation/driver-api/gpio/legacy.rst
>+++ b/Documentation/driver-api/gpio/legacy.rst
>@@ -558,7 +558,7 @@ Platform Support
> To force-enable this framework, a platform's Kconfig will "select" GPIOLIB,
> else it is up to the user to configure support for GPIO.
>
>-It may also provide a custom value for ARCH_NR_GPIOS, so that it better
>+It may also provide a custom value for CONFIG_ARCH_NR_GPIO, so that it better
> reflects the number of GPIOs in actual use on that platform, without
> wasting static table space. (It should count both built-in/SoC GPIOs and
> also ones on GPIO expanders.
>diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>index 53e6a1da9af5..e55b6560fe4f 100644
>--- a/arch/arm/Kconfig
>+++ b/arch/arm/Kconfig
>@@ -14,6 +14,7 @@ config ARM
> select ARCH_HAS_KCOV
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>+ select ARCH_HAS_NR_GPIO
> select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
> select ARCH_HAS_PHYS_TO_DMA
> select ARCH_HAS_SETUP_DMA_OPS
>@@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
> default 352 if ARCH_VT8500
> default 288 if ARCH_ROCKCHIP
> default 264 if MACH_H4700
>- default 0
>+ default 512
> help
> Maximum number of GPIOs in the system.
>
>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/arch/x86/Kconfig b/arch/x86/Kconfig
>index f9920f1341c8..a8c956abc21e 100644
>--- a/arch/x86/Kconfig
>+++ b/arch/x86/Kconfig
>@@ -82,6 +82,7 @@ config X86
> select ARCH_HAS_MEM_ENCRYPT
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>+ select ARCH_HAS_NR_GPIO
> select ARCH_HAS_PMEM_API if X86_64
> select ARCH_HAS_PTE_DEVMAP if X86_64
> select ARCH_HAS_PTE_SPECIAL
>diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>index 0642f579196f..250b50ed44e1 100644
>--- a/drivers/gpio/Kconfig
>+++ b/drivers/gpio/Kconfig
>@@ -11,6 +11,9 @@ config ARCH_HAVE_CUSTOM_GPIO_H
> overriding the default implementations. New uses of this are
> strongly discouraged.
>
>+config ARCH_HAS_NR_GPIO
>+ bool
>+
> menuconfig GPIOLIB
> bool "GPIO Support"
> help
>@@ -22,6 +25,17 @@ menuconfig GPIOLIB
>
> if GPIOLIB
>
>+config ARCH_NR_GPIO
>+ int "Maximum number of GPIOs" if EXPERT
>+ depends on !ARCH_HAS_NR_GPIO
>+ default 512
>+ help
>+ This allows the user to select the maximum number of GPIOs the
>+ kernel must support. When the architecture defines a number
>+ through CONFIG_ARCH_NR_GPIO, that value is taken and the user
>+ cannot change it. Otherwise it defaults to 512 and the user
>+ can change it when CONFIG_EXPERT is set.
>+
> config GPIOLIB_FASTPATH_LIMIT
> int "Maximum number of GPIOs for fast path"
> range 32 512
>diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>index aea9aee1f3e9..ee090f534ab8 100644
>--- a/include/asm-generic/gpio.h
>+++ b/include/asm-generic/gpio.h
>@@ -24,13 +24,7 @@
> * 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
This seems very odd to me. GPIOs can be, and often are, attached to peripheral buses which means that the *same system* can have anything from none to thousands of gpios ..
On Thu, Aug 11, 2022 at 11:12 PM H. Peter Anvin <[email protected]> wrote:
>
> On August 9, 2022 3:40:38 AM PDT, Christophe Leroy <[email protected]> wrote:
> >At the time being, the default maximum number of GPIOs is set to 512
> >and can only get customised via an architecture specific
> >CONFIG_ARCH_NR_GPIO.
> >
> >The maximum number of GPIOs might be dependent on the number of
> >interface boards and is somewhat independent of architecture.
> >
> >Allow the user to select that maximum number outside of any
> >architecture configuration. To enable that, re-define a
> >core CONFIG_ARCH_NR_GPIO for architectures which don't already
> >define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
> >
> >Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
> >
> >On arm, do like x86 and set 512 as the default instead of 0, that
> >allows simplifying the logic in asm-generic/gpio.h
...
> This seems very odd to me. GPIOs can be, and often are, attached to peripheral buses which means that the *same system* can have anything from none to thousands of gpios ..
Basically this setting should give us a *minimum* GPIO lines that are
present on the system. And that is perfectly SoC dependent. The real
issue is that the GPIO framework has these global arrays that (still?)
can't be initialized from the heap due to too early initialization (is
it the true reason?).
--
With Best Regards,
Andy Shevchenko
On August 12, 2022 2:58:36 PM PDT, Andy Shevchenko <[email protected]> wrote:
>On Thu, Aug 11, 2022 at 11:12 PM H. Peter Anvin <[email protected]> wrote:
>>
>> On August 9, 2022 3:40:38 AM PDT, Christophe Leroy <[email protected]> wrote:
>> >At the time being, the default maximum number of GPIOs is set to 512
>> >and can only get customised via an architecture specific
>> >CONFIG_ARCH_NR_GPIO.
>> >
>> >The maximum number of GPIOs might be dependent on the number of
>> >interface boards and is somewhat independent of architecture.
>> >
>> >Allow the user to select that maximum number outside of any
>> >architecture configuration. To enable that, re-define a
>> >core CONFIG_ARCH_NR_GPIO for architectures which don't already
>> >define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>> >
>> >Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>> >
>> >On arm, do like x86 and set 512 as the default instead of 0, that
>> >allows simplifying the logic in asm-generic/gpio.h
>
>...
>
>> This seems very odd to me. GPIOs can be, and often are, attached to peripheral buses which means that the *same system* can have anything from none to thousands of gpios ..
>
>Basically this setting should give us a *minimum* GPIO lines that are
>present on the system. And that is perfectly SoC dependent. The real
>issue is that the GPIO framework has these global arrays that (still?)
>can't be initialized from the heap due to too early initialization (is
>it the true reason?).
>
Ok that makes more sense... but in that case, it would also be good to reclaim excess storage that turns out to not be needed.
I am a bit skeptical, though – we get basic memory allocation very early.
Le 11/08/2022 à 21:57, H. Peter Anvin a écrit :
> On August 9, 2022 3:40:38 AM PDT, Christophe Leroy <[email protected]> wrote:
>> At the time being, the default maximum number of GPIOs is set to 512
>> and can only get customised via an architecture specific
>> CONFIG_ARCH_NR_GPIO.
>>
>> The maximum number of GPIOs might be dependent on the number of
>> interface boards and is somewhat independent of architecture.
>>
>> Allow the user to select that maximum number outside of any
>> architecture configuration. To enable that, re-define a
>> core CONFIG_ARCH_NR_GPIO for architectures which don't already
>> define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>>
>> Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>>
>> On arm, do like x86 and set 512 as the default instead of 0, that
>> allows simplifying the logic in asm-generic/gpio.h
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> Documentation/driver-api/gpio/legacy.rst | 2 +-
>> arch/arm/Kconfig | 3 ++-
>> arch/arm/include/asm/gpio.h | 1 -
>> arch/x86/Kconfig | 1 +
>> drivers/gpio/Kconfig | 14 ++++++++++++++
>> include/asm-generic/gpio.h | 6 ------
>> 6 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
>> index 9b12eeb89170..566b06a584cf 100644
>> --- a/Documentation/driver-api/gpio/legacy.rst
>> +++ b/Documentation/driver-api/gpio/legacy.rst
>> @@ -558,7 +558,7 @@ Platform Support
>> To force-enable this framework, a platform's Kconfig will "select" GPIOLIB,
>> else it is up to the user to configure support for GPIO.
>>
>> -It may also provide a custom value for ARCH_NR_GPIOS, so that it better
>> +It may also provide a custom value for CONFIG_ARCH_NR_GPIO, so that it better
>> reflects the number of GPIOs in actual use on that platform, without
>> wasting static table space. (It should count both built-in/SoC GPIOs and
>> also ones on GPIO expanders.
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 53e6a1da9af5..e55b6560fe4f 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -14,6 +14,7 @@ config ARM
>> select ARCH_HAS_KCOV
>> select ARCH_HAS_MEMBARRIER_SYNC_CORE
>> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>> + select ARCH_HAS_NR_GPIO
>> select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
>> select ARCH_HAS_PHYS_TO_DMA
>> select ARCH_HAS_SETUP_DMA_OPS
>> @@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
>> default 352 if ARCH_VT8500
>> default 288 if ARCH_ROCKCHIP
>> default 264 if MACH_H4700
>> - default 0
>> + default 512
>> help
>> Maximum number of GPIOs in the system.
>>
>> 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/arch/x86/Kconfig b/arch/x86/Kconfig
>> index f9920f1341c8..a8c956abc21e 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -82,6 +82,7 @@ config X86
>> select ARCH_HAS_MEM_ENCRYPT
>> select ARCH_HAS_MEMBARRIER_SYNC_CORE
>> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>> + select ARCH_HAS_NR_GPIO
>> select ARCH_HAS_PMEM_API if X86_64
>> select ARCH_HAS_PTE_DEVMAP if X86_64
>> select ARCH_HAS_PTE_SPECIAL
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 0642f579196f..250b50ed44e1 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -11,6 +11,9 @@ config ARCH_HAVE_CUSTOM_GPIO_H
>> overriding the default implementations. New uses of this are
>> strongly discouraged.
>>
>> +config ARCH_HAS_NR_GPIO
>> + bool
>> +
>> menuconfig GPIOLIB
>> bool "GPIO Support"
>> help
>> @@ -22,6 +25,17 @@ menuconfig GPIOLIB
>>
>> if GPIOLIB
>>
>> +config ARCH_NR_GPIO
>> + int "Maximum number of GPIOs" if EXPERT
>> + depends on !ARCH_HAS_NR_GPIO
>> + default 512
>> + help
>> + This allows the user to select the maximum number of GPIOs the
>> + kernel must support. When the architecture defines a number
>> + through CONFIG_ARCH_NR_GPIO, that value is taken and the user
>> + cannot change it. Otherwise it defaults to 512 and the user
>> + can change it when CONFIG_EXPERT is set.
>> +
>> config GPIOLIB_FASTPATH_LIMIT
>> int "Maximum number of GPIOs for fast path"
>> range 32 512
>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>> index aea9aee1f3e9..ee090f534ab8 100644
>> --- a/include/asm-generic/gpio.h
>> +++ b/include/asm-generic/gpio.h
>> @@ -24,13 +24,7 @@
>> * 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
>
> This seems very odd to me. GPIOs can be, and often are, attached to peripheral buses which means that the *same system* can have anything from none to thousands of gpios ..
Exactly, that's the reason why it needs to be configurable independently
of the architecture.
The required number of GPIOs depends on the number of boards I have in
the system, not on whether it is a x86_64, an arm or a powerpc CPU.
I did this patch because I faced a problem after I added a board in my
powerpc system. At the time being powerpc has the default, while x86_64
has 1024, and I thought that it would be better to get it configurable
rather than hard coding a higher limit on powerpc as done on x86_64 or
several specific ARM systems.
On the long run the best would probably be to have it dynamic, but it
means more changes, I think having it configurable at build time is a
good compromise for the time being, isn't it ?
Thanks
Christophe
On Tue, Aug 9, 2022 at 12:40 PM Christophe Leroy
<[email protected]> wrote:
>
> At the time being, the default maximum number of GPIOs is set to 512
> and can only get customised via an architecture specific
> CONFIG_ARCH_NR_GPIO.
>
> The maximum number of GPIOs might be dependent on the number of
> interface boards and is somewhat independent of architecture.
>
> Allow the user to select that maximum number outside of any
> architecture configuration. To enable that, re-define a
> core CONFIG_ARCH_NR_GPIO for architectures which don't already
> define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>
> Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>
> On arm, do like x86 and set 512 as the default instead of 0, that
> allows simplifying the logic in asm-generic/gpio.h
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> Documentation/driver-api/gpio/legacy.rst | 2 +-
> arch/arm/Kconfig | 3 ++-
> arch/arm/include/asm/gpio.h | 1 -
> arch/x86/Kconfig | 1 +
> drivers/gpio/Kconfig | 14 ++++++++++++++
> include/asm-generic/gpio.h | 6 ------
> 6 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
> index 9b12eeb89170..566b06a584cf 100644
> --- a/Documentation/driver-api/gpio/legacy.rst
> +++ b/Documentation/driver-api/gpio/legacy.rst
> @@ -558,7 +558,7 @@ Platform Support
> To force-enable this framework, a platform's Kconfig will "select" GPIOLIB,
> else it is up to the user to configure support for GPIO.
>
> -It may also provide a custom value for ARCH_NR_GPIOS, so that it better
> +It may also provide a custom value for CONFIG_ARCH_NR_GPIO, so that it better
> reflects the number of GPIOs in actual use on that platform, without
> wasting static table space. (It should count both built-in/SoC GPIOs and
> also ones on GPIO expanders.
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 53e6a1da9af5..e55b6560fe4f 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -14,6 +14,7 @@ config ARM
> select ARCH_HAS_KCOV
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> + select ARCH_HAS_NR_GPIO
> select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
> select ARCH_HAS_PHYS_TO_DMA
> select ARCH_HAS_SETUP_DMA_OPS
> @@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
> default 352 if ARCH_VT8500
> default 288 if ARCH_ROCKCHIP
> default 264 if MACH_H4700
> - default 0
> + default 512
This list should be kept sorted, otherwise you still get e.g. the '264' default
value. If you have a GPIO extender that provides hardcoded GPIO
numbers on your machine, there should be a configuration option for
that driver.
Which driver is it that needs extra hardcoded GPIO numbers for you?
Have you tried converting it to use GPIO descriptors so you don't
need the number assignment?
Arnd
Le 17/08/2022 à 19:46, Arnd Bergmann a écrit :
> On Tue, Aug 9, 2022 at 12:40 PM Christophe Leroy
> <[email protected]> wrote:
>>
>> At the time being, the default maximum number of GPIOs is set to 512
>> and can only get customised via an architecture specific
>> CONFIG_ARCH_NR_GPIO.
>>
>> The maximum number of GPIOs might be dependent on the number of
>> interface boards and is somewhat independent of architecture.
>>
>> Allow the user to select that maximum number outside of any
>> architecture configuration. To enable that, re-define a
>> core CONFIG_ARCH_NR_GPIO for architectures which don't already
>> define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>>
>> Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>>
>> On arm, do like x86 and set 512 as the default instead of 0, that
>> allows simplifying the logic in asm-generic/gpio.h
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> Documentation/driver-api/gpio/legacy.rst | 2 +-
>> arch/arm/Kconfig | 3 ++-
>> arch/arm/include/asm/gpio.h | 1 -
>> arch/x86/Kconfig | 1 +
>> drivers/gpio/Kconfig | 14 ++++++++++++++
>> include/asm-generic/gpio.h | 6 ------
>> 6 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
>> index 9b12eeb89170..566b06a584cf 100644
>> --- a/Documentation/driver-api/gpio/legacy.rst
>> +++ b/Documentation/driver-api/gpio/legacy.rst
>> @@ -558,7 +558,7 @@ Platform Support
>> To force-enable this framework, a platform's Kconfig will "select" GPIOLIB,
>> else it is up to the user to configure support for GPIO.
>>
>> -It may also provide a custom value for ARCH_NR_GPIOS, so that it better
>> +It may also provide a custom value for CONFIG_ARCH_NR_GPIO, so that it better
>> reflects the number of GPIOs in actual use on that platform, without
>> wasting static table space. (It should count both built-in/SoC GPIOs and
>> also ones on GPIO expanders.
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 53e6a1da9af5..e55b6560fe4f 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -14,6 +14,7 @@ config ARM
>> select ARCH_HAS_KCOV
>> select ARCH_HAS_MEMBARRIER_SYNC_CORE
>> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>> + select ARCH_HAS_NR_GPIO
>> select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
>> select ARCH_HAS_PHYS_TO_DMA
>> select ARCH_HAS_SETUP_DMA_OPS
>> @@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
>> default 352 if ARCH_VT8500
>> default 288 if ARCH_ROCKCHIP
>> default 264 if MACH_H4700
>> - default 0
>> + default 512
>
> This list should be kept sorted, otherwise you still get e.g. the '264' default
> value. If you have a GPIO extender that provides hardcoded GPIO
> numbers on your machine, there should be a configuration option for
> that driver.
I don't want to change the behaviour for existing configurations. If the
unconditional default goes before conditional ones, then all following
defaults will be ignored and you'll get 512 instead of 264 if MAC_H4700
is selected for instance.
At the time being, you get 0 only when no other default was selected,
then that 0 implies 512 in asm-generic/gpio.h by:
#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
>
> Which driver is it that needs extra hardcoded GPIO numbers for you?
> Have you tried converting it to use GPIO descriptors so you don't
> need the number assignment?
It is a max7301 (drivers/gpio/gpio-max730x.c) but I can't understand
what you mean. GPIO descriptors are for consumers, aren't they ?
During boot I get :
[ 0.601942] gpiochip_find_base: found new base at 496
[ 0.606337] gpiochip_find_base: found new base at 464
[ 0.616408] gpiochip_find_base: found new base at 448
[ 0.621826] gpiochip_find_base: found new base at 432
[ 0.627228] gpiochip_find_base: found new base at 400
[ 0.660984] gpiochip_find_base: found new base at 384
[ 0.669631] gpiochip_find_base: found new base at 368
[ 0.672713] gpiochip_find_base: found new base at 352
[ 0.675805] gpiochip_find_base: found new base at 336
[ 0.678885] gpiochip_find_base: found new base at 320
[ 0.682178] gpiochip_find_base: found new base at 304
[ 0.685275] gpiochip_find_base: found new base at 288
[ 0.688366] gpiochip_find_base: found new base at 272
[ 0.691678] gpiochip_find_base: found new base at 256
[ 0.694762] gpiochip_find_base: found new base at 240
[ 0.697847] gpiochip_find_base: found new base at 224
[ 0.701441] gpiochip_find_base: found new base at 208
[ 0.709427] gpiochip_find_base: found new base at 192
[ 0.713859] gpiochip_find_base: found new base at 176
[ 0.718002] gpiochip_find_base: found new base at 160
[ 0.723316] gpiochip_find_base: found new base at 144
[ 0.731105] gpiochip_find_base: found new base at 128
[ 0.737403] gpiochip_find_base: found new base at 112
[ 0.740614] gpiochip_find_base: found new base at 96
[ 0.743701] gpiochip_find_base: found new base at 80
[ 0.747246] gpiochip_find_base: found new base at 64
[ 4.663677] gpiochip_find_base: found new base at 36
[ 5.050792] gpiochip_find_base: found new base at 16
[ 5.064892] gpiochip_find_base: cannot find free range
[ 5.095527] gpiochip_find_base: cannot find free range
gpiochip_find_base() is called for any GPIO driver, by gpiochip_add() /
gpiochip_add_data_with_key(), and there is the following comment:
/*
* TODO: this allocates a Linux GPIO number base in the global
* GPIO numberspace for this chip. In the long run we want to
* get *rid* of this numberspace and use only descriptors, but
* it may be a pipe dream. It will not happen before we get rid
* of the sysfs interface anyways.
*/
So, what did I miss ?
Thanks
Christophe
On Thu, Aug 18, 2022 at 8:00 AM Christophe Leroy
<[email protected]> wrote:
> Le 17/08/2022 à 19:46, Arnd Bergmann a écrit :
> > On Tue, Aug 9, 2022 at 12:40 PM Christophe Leroy <[email protected]> wrote:
> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> index 53e6a1da9af5..e55b6560fe4f 100644
> >> --- a/arch/arm/Kconfig
> >> +++ b/arch/arm/Kconfig
> >> @@ -14,6 +14,7 @@ config ARM
> >> select ARCH_HAS_KCOV
> >> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> >> select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> >> + select ARCH_HAS_NR_GPIO
> >> select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
> >> select ARCH_HAS_PHYS_TO_DMA
> >> select ARCH_HAS_SETUP_DMA_OPS
> >> @@ -1243,7 +1244,7 @@ config ARCH_NR_GPIO
> >> default 352 if ARCH_VT8500
> >> default 288 if ARCH_ROCKCHIP
> >> default 264 if MACH_H4700
> >> - default 0
> >> + default 512
> >
> > This list should be kept sorted, otherwise you still get e.g. the '264' default
> > value. If you have a GPIO extender that provides hardcoded GPIO
> > numbers on your machine, there should be a configuration option for
> > that driver.
>
> I don't want to change the behaviour for existing configurations. If the
> unconditional default goes before conditional ones, then all following
> defaults will be ignored and you'll get 512 instead of 264 if MAC_H4700
> is selected for instance.
>
> At the time being, you get 0 only when no other default was selected,
> then that 0 implies 512 in asm-generic/gpio.h by:
>
> #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
Ok, I see what you are doing now. I'm not sure this is actually intentional
behavior of the current implementation though, my guess would be that
the 'default 0' was intended as a fallback for platforms that have no
GPIO providers at all.
This is of course not entirely appropriate any more as
> > Which driver is it that needs extra hardcoded GPIO numbers for you?
> > Have you tried converting it to use GPIO descriptors so you don't
> > need the number assignment?
>
> It is a max7301 (drivers/gpio/gpio-max730x.c) but I can't understand
> what you mean. GPIO descriptors are for consumers, aren't they ?
I meant the consumers, yes.
> During boot I get :
>
> [ 0.601942] gpiochip_find_base: found new base at 496
> [ 0.606337] gpiochip_find_base: found new base at 464
> [ 0.616408] gpiochip_find_base: found new base at 448
> [ 0.621826] gpiochip_find_base: found new base at 432
> [ 0.627228] gpiochip_find_base: found new base at 400
> [ 0.660984] gpiochip_find_base: found new base at 384
> [ 0.669631] gpiochip_find_base: found new base at 368
> [ 0.672713] gpiochip_find_base: found new base at 352
> [ 0.675805] gpiochip_find_base: found new base at 336
> [ 0.678885] gpiochip_find_base: found new base at 320
> [ 0.682178] gpiochip_find_base: found new base at 304
> [ 0.685275] gpiochip_find_base: found new base at 288
> [ 0.688366] gpiochip_find_base: found new base at 272
> [ 0.691678] gpiochip_find_base: found new base at 256
> [ 0.694762] gpiochip_find_base: found new base at 240
> [ 0.697847] gpiochip_find_base: found new base at 224
> [ 0.701441] gpiochip_find_base: found new base at 208
> [ 0.709427] gpiochip_find_base: found new base at 192
> [ 0.713859] gpiochip_find_base: found new base at 176
> [ 0.718002] gpiochip_find_base: found new base at 160
> [ 0.723316] gpiochip_find_base: found new base at 144
> [ 0.731105] gpiochip_find_base: found new base at 128
> [ 0.737403] gpiochip_find_base: found new base at 112
> [ 0.740614] gpiochip_find_base: found new base at 96
> [ 0.743701] gpiochip_find_base: found new base at 80
> [ 0.747246] gpiochip_find_base: found new base at 64
> [ 4.663677] gpiochip_find_base: found new base at 36
> [ 5.050792] gpiochip_find_base: found new base at 16
> [ 5.064892] gpiochip_find_base: cannot find free range
> [ 5.095527] gpiochip_find_base: cannot find free range
>
> gpiochip_find_base() is called for any GPIO driver, by gpiochip_add() /
> gpiochip_add_data_with_key(), and there is the following comment:
>
> /*
> * TODO: this allocates a Linux GPIO number base in the global
> * GPIO numberspace for this chip. In the long run we want to
> * get *rid* of this numberspace and use only descriptors, but
> * it may be a pipe dream. It will not happen before we get rid
> * of the sysfs interface anyways.
> */
>
> So, what did I miss ?
I missed the fact that the registration fails if it runs out of gpio numbers,
as I was assuming that you could still use the additional gpio chips
with the descriptor based API as long as all of the consumers on the
system use that and you don't use CONFIG_GPIO_SYSFS.
I see that this does not work today, but maybe it wouldn't be too hard to
change? I see that CONFIG_GPIO_SYSFS continued to move towards
deprecation after the comment was added in the code, and these days it
can only be enabled if CONFIG_EXPERT=y is set.
Arnd
On Tue, Aug 9, 2022 at 12:41 PM Christophe Leroy
<[email protected]> wrote:
> At the time being, the default maximum number of GPIOs is set to 512
> and can only get customised via an architecture specific
> CONFIG_ARCH_NR_GPIO.
>
> The maximum number of GPIOs might be dependent on the number of
> interface boards and is somewhat independent of architecture.
>
> Allow the user to select that maximum number outside of any
> architecture configuration. To enable that, re-define a
> core CONFIG_ARCH_NR_GPIO for architectures which don't already
> define one. Guard it with a new hidden CONFIG_ARCH_HAS_NR_GPIO.
>
> Only two architectures will need CONFIG_ARCH_HAS_NR_GPIO: x86 and arm.
>
> On arm, do like x86 and set 512 as the default instead of 0, that
> allows simplifying the logic in asm-generic/gpio.h
>
> Signed-off-by: Christophe Leroy <[email protected]>
I see what you're trying to do and this might be a possible interim solution.
I would like some comments sprinkled into the patched sites that this is
supposed to go away.
The GPIO descriptor refactoring which has been ongoing for a few
years, see drivers/gpio/TODO (please read), has the end goal of making
descriptor allocation fully dynamic. Once we free ourselves from
the fixed GPIO numberspace, there is nothing preventing us from
just kmalloc() ing a new descriptor whenever one is needed.
Help with rooting out the remaining fixed GPIO number clients
is much appreciated!
The per-arch GPIO number only exist for one reason: embedded
GPIOs (think SoC:s) that refer to fixed numbers in numberspace in
the board support code. This makes it necessary to allocate
descriptors up front in some compiled-in GPIO chips.
Yours,
Linus Walleij
On Thu, Aug 18, 2022 at 11:33 AM Linus Walleij <[email protected]> wrote:
>
> The per-arch GPIO number only exist for one reason: embedded
> GPIOs (think SoC:s) that refer to fixed numbers in numberspace in
> the board support code. This makes it necessary to allocate
> descriptors up front in some compiled-in GPIO chips.
As I understood, the problem that Christophe ran into is that the
dynamic registration of additional gpio chips is broken because
it unregisters the chip if the number space is exhausted:
base = gpiochip_find_base(gc->ngpio);
if (base < 0) {
ret = base;
spin_unlock_irqrestore(&gpio_lock, flags);
goto err_free_label;
}
From the git history, it looks like this error was never handled gracefully
even if the intention was to keep going without a number assignment,
so there are probably other bugs one runs into after changing this.
Arnd
On Thu, Aug 18, 2022 at 11:48 AM Arnd Bergmann <[email protected]> wrote:
> As I understood, the problem that Christophe ran into is that the
> dynamic registration of additional gpio chips is broken because
> it unregisters the chip if the number space is exhausted:
>
> base = gpiochip_find_base(gc->ngpio);
> if (base < 0) {
> ret = base;
> spin_unlock_irqrestore(&gpio_lock, flags);
> goto err_free_label;
> }
>
> From the git history, it looks like this error was never handled gracefully
> even if the intention was to keep going without a number assignment,
> so there are probably other bugs one runs into after changing this.
Hm that should be possible to get rid of altogether? I suppose it is only
there to satisfy
static inline bool gpio_is_valid(int number)
{
return number >= 0 && number < ARCH_NR_GPIOS;
}
?
If using GPIO descriptors, any descriptor != NULL is valid,
this one is just used with legacy GPIOs. Maybe we should just
delete gpio_is_valid() everywhere and then drop the cap?
I think there may be systems and users that still depend on GPIO base
numbers being assigned from ARCH_NR_GPIOS and
downwards (userspace GPIO numbers in sysfs will also change...)
otherwise we could assign from 0 and up.
Right now the safest would be:
Assign from 512 and downwards until we hit 0 then assign
from something high, like U32_MAX and downward.
That requires dropping gpio_is_valid() everywhere.
If we wanna be bold, just delete gpio_is_valid() and assign
bases from 0 and see what happens. But I think that will
lead to regressions.
Yours,
Linus Walleij
On Thu, Aug 18, 2022 at 1:13 PM Linus Walleij <[email protected]> wrote:
>
> On Thu, Aug 18, 2022 at 11:48 AM Arnd Bergmann <[email protected]> wrote:
>
> > As I understood, the problem that Christophe ran into is that the
> > dynamic registration of additional gpio chips is broken because
> > it unregisters the chip if the number space is exhausted:
> >
> > base = gpiochip_find_base(gc->ngpio);
> > if (base < 0) {
> > ret = base;
> > spin_unlock_irqrestore(&gpio_lock, flags);
> > goto err_free_label;
> > }
> >
> > From the git history, it looks like this error was never handled gracefully
> > even if the intention was to keep going without a number assignment,
> > so there are probably other bugs one runs into after changing this.
>
> Hm that should be possible to get rid of altogether? I suppose it is only
> there to satisfy
>
> static inline bool gpio_is_valid(int number)
> {
> return number >= 0 && number < ARCH_NR_GPIOS;
> }
>
> ?
>
> If using GPIO descriptors, any descriptor != NULL is valid,
> this one is just used with legacy GPIOs. Maybe we should just
> delete gpio_is_valid() everywhere and then drop the cap?
I think it makes sense to keep gpio_is_valid() for as long as we
support the numbers.
> I think there may be systems and users that still depend on GPIO base
> numbers being assigned from ARCH_NR_GPIOS and
> downwards (userspace GPIO numbers in sysfs will also change...)
> otherwise we could assign from 0 and up.
Is it possible to find in-kernel users that depend on well-known
numbers for dynamically assigned gpios? I would argue
that those are always broken.
Even for the sysfs interface, it is questionable to rely on
specific numbers because at least in an arm multiplatform
kernel the top number changes based on kernel configuration.
> Right now the safest would be:
> Assign from 512 and downwards until we hit 0 then assign
> from something high, like U32_MAX and downward.
>
> That requires dropping gpio_is_valid() everywhere.
>
> If we wanna be bold, just delete gpio_is_valid() and assign
> bases from 0 and see what happens. But I think that will
> lead to regressions.
I'm still unsure how removing gpio_is_valid() would help.
What I could imagine as a next step would be to mark all
consumer drivers and the sysfs interface that use gpio
numbers as 'depends on GPIO_LEGACY' and then only
provide the corresponding drivers if that option is set.
After that, we could try to make sure we can run the
defconfigs for modern architectures (arm64, riscv,
x86-64, ...) without the option by converting the drivers
that are most commonly used.
Arnd
On Thu, Aug 18, 2022 at 1:33 PM Arnd Bergmann <[email protected]> wrote:
> On Thu, Aug 18, 2022 at 1:13 PM Linus Walleij <[email protected]> wrote:
> > static inline bool gpio_is_valid(int number)
> > {
> > return number >= 0 && number < ARCH_NR_GPIOS;
> > }
> >
> > ?
> >
> > If using GPIO descriptors, any descriptor != NULL is valid,
> > this one is just used with legacy GPIOs. Maybe we should just
> > delete gpio_is_valid() everywhere and then drop the cap?
>
> I think it makes sense to keep gpio_is_valid() for as long as we
> support the numbers.
Hmmm....
> > I think there may be systems and users that still depend on GPIO base
> > numbers being assigned from ARCH_NR_GPIOS and
> > downwards (userspace GPIO numbers in sysfs will also change...)
> > otherwise we could assign from 0 and up.
>
> Is it possible to find in-kernel users that depend on well-known
> numbers for dynamically assigned gpios? I would argue
> that those are always broken.
Most in-kernel users hard-code the base to something like
0 etc it's only the ones that code -1 into .base that are in
trouble because that will activate dynamic assignment for the
base.
git grep 'base = -1' yields these suspects:
arch/arm/common/sa1111.c: sachip->gc.base = -1;
arch/arm/common/scoop.c: devptr->gpio.base = -1;
arch/powerpc/platforms/52xx/mpc52xx_gpt.c: gpt->gc.base = -1;
arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c: gc->base = -1;
That's all! We could just calculate these to 512-ngpios and
hardcode that instead.
> Even for the sysfs interface, it is questionable to rely on
> specific numbers because at least in an arm multiplatform
> kernel the top number changes based on kernel configuration.
Yeah :/ still these users tend to angrily report any breakage
due to expected (fragile) behaviour.
> > Right now the safest would be:
> > Assign from 512 and downwards until we hit 0 then assign
> > from something high, like U32_MAX and downward.
> >
> > That requires dropping gpio_is_valid() everywhere.
> >
> > If we wanna be bold, just delete gpio_is_valid() and assign
> > bases from 0 and see what happens. But I think that will
> > lead to regressions.
>
> I'm still unsure how removing gpio_is_valid() would help.
If we allow GPIO base all the way to U32_MAX
this function becomes:
static inline bool gpio_is_valid(int number)
{
return number >= 0 && number < U32_MAX;
}
and we can then just
#define gpio_is_valid true
and in that case it is better to delete the use of this function
altogether since it can not fail.
> What I could imagine as a next step would be to mark all
> consumer drivers and the sysfs interface that use gpio
> numbers as 'depends on GPIO_LEGACY' and then only
> provide the corresponding drivers if that option is set.
Hm I wonder what Bartosz and Alexandre Courbot and thinks
about a GPIO_LEGACY symbol to phase out the global
GPIO numberspace. I kind of like the idea.
I made the sysfs depend on CONFIG_EXPERT to at least make it less
accessible and not provide users with guns to shoot themselves
in the foot.
Yours,
Linus Walleij
On Thu, Aug 18, 2022 at 2:25 PM Linus Walleij <[email protected]> wrote:
> On Thu, Aug 18, 2022 at 1:33 PM Arnd Bergmann <[email protected]> wrote:
> > On Thu, Aug 18, 2022 at 1:13 PM Linus Walleij <[email protected]> wrote:
> > > I think there may be systems and users that still depend on GPIO base
> > > numbers being assigned from ARCH_NR_GPIOS and
> > > downwards (userspace GPIO numbers in sysfs will also change...)
> > > otherwise we could assign from 0 and up.
> >
> > Is it possible to find in-kernel users that depend on well-known
> > numbers for dynamically assigned gpios? I would argue
> > that those are always broken.
>
> Most in-kernel users hard-code the base to something like
> 0 etc it's only the ones that code -1 into .base that are in
> trouble because that will activate dynamic assignment for the
> base.
>
> git grep 'base = -1' yields these suspects:
>
> arch/arm/common/sa1111.c: sachip->gc.base = -1;
> arch/arm/common/scoop.c: devptr->gpio.base = -1;
> arch/powerpc/platforms/52xx/mpc52xx_gpt.c: gpt->gc.base = -1;
> arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c: gc->base = -1;
>
> That's all! We could just calculate these to 512-ngpios and
> hardcode that instead.
How do the consumers find the numbers for these four?
> > > Right now the safest would be:
> > > Assign from 512 and downwards until we hit 0 then assign
> > > from something high, like U32_MAX and downward.
> > >
> > > That requires dropping gpio_is_valid() everywhere.
> > >
> > > If we wanna be bold, just delete gpio_is_valid() and assign
> > > bases from 0 and see what happens. But I think that will
> > > lead to regressions.
> >
> > I'm still unsure how removing gpio_is_valid() would help.
>
> If we allow GPIO base all the way to U32_MAX
> this function becomes:
>
> static inline bool gpio_is_valid(int number)
> {
> return number >= 0 && number < U32_MAX;
> }
>
> and we can then just
>
> #define gpio_is_valid true
>
> and in that case it is better to delete the use of this function
> altogether since it can not fail.
S32_MAX might be a better upper bound. That allows to
just have no number assigned to a gpio chip. Any driver
code calling desc_to_gpio() could then get back -1
or a negative error code.
Making the ones that are invalid today valid sounds like
a step backwards to me if the goal is to stop using
gpio numbers and most consumers no longer need them.
Arnd
Le 18/08/2022 à 14:46, Arnd Bergmann a écrit :
> On Thu, Aug 18, 2022 at 2:25 PM Linus Walleij <[email protected]> wrote:
>> On Thu, Aug 18, 2022 at 1:33 PM Arnd Bergmann <[email protected]> wrote:
>>> On Thu, Aug 18, 2022 at 1:13 PM Linus Walleij <[email protected]> wrote:
>>>> I think there may be systems and users that still depend on GPIO base
>>>> numbers being assigned from ARCH_NR_GPIOS and
>>>> downwards (userspace GPIO numbers in sysfs will also change...)
>>>> otherwise we could assign from 0 and up.
>>>
>>> Is it possible to find in-kernel users that depend on well-known
>>> numbers for dynamically assigned gpios? I would argue
>>> that those are always broken.
>>
>> Most in-kernel users hard-code the base to something like
>> 0 etc it's only the ones that code -1 into .base that are in
>> trouble because that will activate dynamic assignment for the
>> base.
>>
>> git grep 'base = -1' yields these suspects:
>>
>> arch/arm/common/sa1111.c: sachip->gc.base = -1;
>> arch/arm/common/scoop.c: devptr->gpio.base = -1;
>> arch/powerpc/platforms/52xx/mpc52xx_gpt.c: gpt->gc.base = -1;
>> arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c: gc->base = -1;
>>
>> That's all! We could just calculate these to 512-ngpios and
>> hardcode that instead.
>
> How do the consumers find the numbers for these four?
>
>>>> Right now the safest would be:
>>>> Assign from 512 and downwards until we hit 0 then assign
>>>> from something high, like U32_MAX and downward.
>>>>
>>>> That requires dropping gpio_is_valid() everywhere.
>>>>
>>>> If we wanna be bold, just delete gpio_is_valid() and assign
>>>> bases from 0 and see what happens. But I think that will
>>>> lead to regressions.
>>>
>>> I'm still unsure how removing gpio_is_valid() would help.
>>
>> If we allow GPIO base all the way to U32_MAX
>> this function becomes:
>>
>> static inline bool gpio_is_valid(int number)
>> {
>> return number >= 0 && number < U32_MAX;
>> }
>>
>> and we can then just
>>
>> #define gpio_is_valid true
>>
>> and in that case it is better to delete the use of this function
>> altogether since it can not fail.
>
> S32_MAX might be a better upper bound. That allows to
> just have no number assigned to a gpio chip. Any driver
> code calling desc_to_gpio() could then get back -1
> or a negative error code.
>
> Making the ones that are invalid today valid sounds like
> a step backwards to me if the goal is to stop using
> gpio numbers and most consumers no longer need them.
>
What about GPIO AGGREGATOR, drivers/gpio/gpio-aggregator.c
bitmap = bitmap_alloc(ARCH_NR_GPIOS, GFP_KERNEL);
Christophe
Le 25/08/2022 à 15:36, Linus Walleij a écrit :
> On Thu, Aug 18, 2022 at 2:46 PM Arnd Bergmann <[email protected]> wrote:
>> On Thu, Aug 18, 2022 at 2:25 PM Linus Walleij <[email protected]> wrote:
>
>>> git grep 'base = -1' yields these suspects:
>>>
>>> arch/arm/common/sa1111.c: sachip->gc.base = -1;
>>> arch/arm/common/scoop.c: devptr->gpio.base = -1;
>>> arch/powerpc/platforms/52xx/mpc52xx_gpt.c: gpt->gc.base = -1;
>>> arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c: gc->base = -1;
>>>
>>> That's all! We could just calculate these to 512-ngpios and
>>> hardcode that instead.
>>
>> How do the consumers find the numbers for these four?
>
> For SA1111 the chip gets named "sa1111" and some consumers actually
> use proper machine descriptions, maybe all?
>
> arch/arm/mach-sa1100/jornada720.c: GPIO_LOOKUP("sa1111",
> 0, "s0-power", GPIO_ACTIVE_HIGH),
> arch/arm/mach-sa1100/jornada720.c: GPIO_LOOKUP("sa1111",
> 1, "s1-power", GPIO_ACTIVE_HIGH),
> (...)
>
> For Scoop it is conditionally overridden in the code. I guess always
> overridden.
>
> For powerpc these seem to be using (old but working) device tree
> lookups, so should not be an issue.
>
> Sadly I'm not 100% sure that there are no random hard-coded
> GPIO numbers referring to whatever the framework gave them
> at the time the code was written :(
On my PPC board, the one before the last looks suspicious ....
[ 0.573261] gpio gpiochip0: registered GPIOs 496 to 511 on
/soc@ff000000/cpm@9c0/gpio-controller@950
[ 0.577460] gpio gpiochip1: registered GPIOs 464 to 495 on
/soc@ff000000/cpm@9c0/gpio-controller@ab8
[ 0.586011] gpio gpiochip2: registered GPIOs 448 to 463 on
/soc@ff000000/cpm@9c0/gpio-controller@960
[ 0.591057] gpio gpiochip3: registered GPIOs 432 to 447 on
/soc@ff000000/cpm@9c0/gpio-controller@970
[ 0.595979] gpio gpiochip4: registered GPIOs 400 to 431 on
/soc@ff000000/cpm@9c0/gpio-controller@ac8
[ 0.629292] gpio_stub_drv gpiochip5: registered GPIOs 384 to 399 on
/localbus@ff000100/cpld-cmpc@5,0000000/gpio-controller@2
[ 0.636556] gpio_stub_drv gpiochip6: registered GPIOs 368 to 383 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@00
[ 0.639503] gpio_stub_drv gpiochip7: registered GPIOs 352 to 367 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@02
[ 0.642434] gpio_stub_drv gpiochip8: registered GPIOs 336 to 351 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@04
[ 0.645257] gpio_stub_drv gpiochip9: registered GPIOs 320 to 335 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@10
[ 0.648230] gpio_stub_drv gpiochip10: registered GPIOs 304 to 319 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@20
[ 0.651070] gpio_stub_drv gpiochip11: registered GPIOs 288 to 303 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@22
[ 0.653986] gpio_stub_drv gpiochip12: registered GPIOs 272 to 287 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@24
[ 0.656807] gpio_stub_drv gpiochip13: registered GPIOs 256 to 271 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@26
[ 0.659761] gpio_stub_drv gpiochip14: registered GPIOs 240 to 255 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@28
[ 0.662622] gpio_stub_drv gpiochip15: registered GPIOs 224 to 239 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@2A
[ 0.665454] gpio_stub_drv gpiochip16: registered GPIOs 208 to 223 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@2C
[ 0.673552] gpio_stub_drv gpiochip17: registered GPIOs 192 to 207 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@30
[ 0.677281] gpio_stub_drv gpiochip18: registered GPIOs 176 to 191 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@32
[ 0.680235] gpio_stub_drv gpiochip19: registered GPIOs 160 to 175 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@40
[ 0.685876] gpio_stub_drv gpiochip20: registered GPIOs 144 to 159 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@42
[ 0.694431] gpio_stub_drv gpiochip21: registered GPIOs 128 to 143 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@44
[ 0.697257] gpio_stub_drv gpiochip22: registered GPIOs 112 to 127 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@50
[ 0.700220] gpio_stub_drv gpiochip23: registered GPIOs 96 to 111 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@52
[ 0.703183] gpio_stub_drv gpiochip24: registered GPIOs 80 to 95 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@54
[ 0.708226] gpio_stub_drv gpiochip25: registered GPIOs 64 to 79 on
/localbus@ff000100/fpga-m@4,0000000/gpio-controller@34
[ 0.756817] gpio gpiochip26: registered GPIOs 0 to 2 on generic
[ 4.530397] gpio gpiochip27: registered GPIOs 36 to 63 on max7301
>
> Another reason the base is assigned from above (usually
> from 512 and downward) is that the primary SoC GPIO usually
> want to be at base 0 and there is no guarantee that it will
> get probed first. So hard-coded GPIO bases go from 0 -> n
> and dynamically allocateed GPIO bases from n <- 512.
>
> Then we hope they don't meet and overlap in the middle...
>
>>> and in that case it is better to delete the use of this function
>>> altogether since it can not fail.
>>
>> S32_MAX might be a better upper bound. That allows to
>> just have no number assigned to a gpio chip. Any driver
>> code calling desc_to_gpio() could then get back -1
>> or a negative error code.
>>
>> Making the ones that are invalid today valid sounds like
>> a step backwards to me if the goal is to stop using
>> gpio numbers and most consumers no longer need them.
>
> OK I get it...
>
> Now: who wants to write this patch? :)
>
> Christophe? Will you take a stab at it?
>
Which patch should I write ?
Christophe
On Thu, Aug 18, 2022 at 2:46 PM Arnd Bergmann <[email protected]> wrote:
> On Thu, Aug 18, 2022 at 2:25 PM Linus Walleij <[email protected]> wrote:
> > git grep 'base = -1' yields these suspects:
> >
> > arch/arm/common/sa1111.c: sachip->gc.base = -1;
> > arch/arm/common/scoop.c: devptr->gpio.base = -1;
> > arch/powerpc/platforms/52xx/mpc52xx_gpt.c: gpt->gc.base = -1;
> > arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c: gc->base = -1;
> >
> > That's all! We could just calculate these to 512-ngpios and
> > hardcode that instead.
>
> How do the consumers find the numbers for these four?
For SA1111 the chip gets named "sa1111" and some consumers actually
use proper machine descriptions, maybe all?
arch/arm/mach-sa1100/jornada720.c: GPIO_LOOKUP("sa1111",
0, "s0-power", GPIO_ACTIVE_HIGH),
arch/arm/mach-sa1100/jornada720.c: GPIO_LOOKUP("sa1111",
1, "s1-power", GPIO_ACTIVE_HIGH),
(...)
For Scoop it is conditionally overridden in the code. I guess always
overridden.
For powerpc these seem to be using (old but working) device tree
lookups, so should not be an issue.
Sadly I'm not 100% sure that there are no random hard-coded
GPIO numbers referring to whatever the framework gave them
at the time the code was written :(
Another reason the base is assigned from above (usually
from 512 and downward) is that the primary SoC GPIO usually
want to be at base 0 and there is no guarantee that it will
get probed first. So hard-coded GPIO bases go from 0 -> n
and dynamically allocateed GPIO bases from n <- 512.
Then we hope they don't meet and overlap in the middle...
> > and in that case it is better to delete the use of this function
> > altogether since it can not fail.
>
> S32_MAX might be a better upper bound. That allows to
> just have no number assigned to a gpio chip. Any driver
> code calling desc_to_gpio() could then get back -1
> or a negative error code.
>
> Making the ones that are invalid today valid sounds like
> a step backwards to me if the goal is to stop using
> gpio numbers and most consumers no longer need them.
OK I get it...
Now: who wants to write this patch? :)
Christophe? Will you take a stab at it?
Yours,
Linus Walleij
On Thu, Aug 25, 2022 at 4:00 PM Christophe Leroy
<[email protected]> wrote:
> > Christophe? Will you take a stab at it?
>
> Which patch should I write ?
One that removes CONFIG_ARCH_HAS_NR_GPIO entirely, then
allocate bases for new GPIO chips from 0 and upward instead.
And then see what happens.
Yours,
Linus Walleij
Le 26/08/2022 à 15:49, Linus Walleij a écrit :
> On Thu, Aug 25, 2022 at 4:00 PM Christophe Leroy
> <[email protected]> wrote:
>
>>> Christophe? Will you take a stab at it?
>>
>> Which patch should I write ?
>
> One that removes CONFIG_ARCH_HAS_NR_GPIO entirely, then
> allocate bases for new GPIO chips from 0 and upward instead.
> And then see what happens.
>
Ok, I can give it a try.
But what do I do with:
drivers/gpio/gpio-aggregator.c: bitmap = bitmap_alloc(ARCH_NR_GPIOS,
GFP_KERNEL);
Christophe
On Fri, Aug 26, 2022 at 5:08 PM Christophe Leroy
<[email protected]> wrote:
> Le 26/08/2022 à 15:49, Linus Walleij a écrit :
> > On Thu, Aug 25, 2022 at 4:00 PM Christophe Leroy
> > <[email protected]> wrote:
> >
> >>> Christophe? Will you take a stab at it?
> >>
> >> Which patch should I write ?
> >
> > One that removes CONFIG_ARCH_HAS_NR_GPIO entirely, then
> > allocate bases for new GPIO chips from 0 and upward instead.
> > And then see what happens.
> >
>
> Ok, I can give it a try.
Nice!
> But what do I do with:
>
> drivers/gpio/gpio-aggregator.c: bitmap = bitmap_alloc(ARCH_NR_GPIOS,
> GFP_KERNEL);
That's just used locally in that driver to loop over the arguments to the
aggregator (from the file in sysfs). I would set some arbitrary root
like
#define AGGREGATOR_MAX_GPIOS 512
and just search/replace with that.
Yours,
Linus Walleij
Le 26/08/2022 à 23:54, Linus Walleij a écrit :
> On Fri, Aug 26, 2022 at 5:08 PM Christophe Leroy
> <[email protected]> wrote:
>> Le 26/08/2022 à 15:49, Linus Walleij a écrit :
>>> On Thu, Aug 25, 2022 at 4:00 PM Christophe Leroy
>>> <[email protected]> wrote:
>>>
>>>>> Christophe? Will you take a stab at it?
>>>>
>>>> Which patch should I write ?
>>>
>>> One that removes CONFIG_ARCH_HAS_NR_GPIO entirely, then
>>> allocate bases for new GPIO chips from 0 and upward instead.
>>> And then see what happens.
>>>
>>
>> Ok, I can give it a try.
>
> Nice!
>
>> But what do I do with:
>>
>> drivers/gpio/gpio-aggregator.c: bitmap = bitmap_alloc(ARCH_NR_GPIOS,
>> GFP_KERNEL);
>
> That's just used locally in that driver to loop over the arguments to the
> aggregator (from the file in sysfs). I would set some arbitrary root
> like
> #define AGGREGATOR_MAX_GPIOS 512
> and just search/replace with that.
>
And what about gsta_gpio_setup() that requests base 0 with the following
comment:
/*
* ARCH_NR_GPIOS is currently 256 and dynamic allocation starts
* from the end. However, for compatibility, we need the first
* ConneXt device to start from gpio 0: it's the main chipset
* on most boards so documents and drivers assume gpio0..gpio127
*/
And I guess there might be other drivers like that (I found that one
because of its comment mentionning ARCH_NR_GPIOS.
Another solution could be to leave first GPIOs for static allocation,
and allocate dynamic ones from 256 or from 512 ?
Maybe in two steps:
- First step: Allocate dynamic from 256 upwards and add a pr_warn() for
all static allocations.
- Second step later: Allocate dynamic from 0 and forbid static allocation.
Any opinion ?
Christophe
On Sun, Aug 28, 2022 at 11:06 AM Christophe Leroy
<[email protected]> wrote:
> Le 26/08/2022 à 23:54, Linus Walleij a écrit :
> >> But what do I do with:
> >>
> >> drivers/gpio/gpio-aggregator.c: bitmap = bitmap_alloc(ARCH_NR_GPIOS,
> >> GFP_KERNEL);
> >
> > That's just used locally in that driver to loop over the arguments to the
> > aggregator (from the file in sysfs). I would set some arbitrary root
> > like
> > #define AGGREGATOR_MAX_GPIOS 512
> > and just search/replace with that.
> >
>
> And what about gsta_gpio_setup() that requests base 0 with the following
> comment:
>
> /*
> * ARCH_NR_GPIOS is currently 256 and dynamic allocation starts
> * from the end. However, for compatibility, we need the first
> * ConneXt device to start from gpio 0: it's the main chipset
> * on most boards so documents and drivers assume gpio0..gpio127
> */
>
>
> And I guess there might be other drivers like that (I found that one
> because of its comment mentioning ARCH_NR_GPIOS.
This driver is clearly incomplete: there is an mfd portion in
drivers/mfd/sta2x11-mfd.c, the gpio provider in
drivers/gpio/gpio-sta2x11.c, one gpio consumer in
drivers/media/pci/sta2x11/, and some glue logic in
arch/x86/pci/sta2x11-fixup.c, but nothing that seems to set up
the platform data that these need to communicate with one another.
I think that just means the code that one would have to modify
is in vendor kernels of devices using this chip, but there is no
way to fix those if they are not in mainline. The last meaningful
patches on this SoC support were in 2012 by Davide Ciminaghi
and Alessandro Rubini, though they still Acked patches after that.
I wonder if I was missing the interesting bit about it, if the driver
is just obsolete and can be removed, or if there is something
that is still worth fixing here.
Arnd
On Sun, Aug 28, 2022 at 11:06 AM Christophe Leroy
<[email protected]> wrote:
> And I guess there might be other drivers like that (I found that one
> because of its comment mentionning ARCH_NR_GPIOS.
Yes there are a bunch of GPIO controllers with fixed base.
These only exist because there is boardfile code that uses
these fixed GPIO numbers.
> Another solution could be to leave first GPIOs for static allocation,
> and allocate dynamic ones from 256 or from 512 ?
>
> Maybe in two steps:
> - First step: Allocate dynamic from 256 upwards and add a pr_warn() for
> all static allocations.
OK that is reasonable.
I thought that maybe we could assume the fixed bases to probe first
and thus reserve the GPIO bases they want before we get to the
dynamically allocated drivers.
This could be a good first step.
> - Second step later: Allocate dynamic from 0 and forbid static allocation.
What needs to happen for doing that 100% safe is to get rid of all
board files, mostly in arch/arm/mach* but also elsewhere, or to
augment all boardfiles to use descriptor tables instead.
But you're right, try the two step approach first.
Yours,
Linus Walleij
Thanks davide for the good explanation
(and the link the the old email I forgot about)
> tl;dr: sta2x11 support can be removed.
Confirmed.
The point here is that we talked with the vendor. They are still using
the device (with some extra internal patches), but new development at
kernel level is stopped.
Since the device is not currently available to the general public,
it's ok to save the maintaining efforts.
I can submit patch[es] next week or ack removal if somebody submits
them earlier.
thanks
/alessandro
Le 30/08/2022 à 10:33, Alessandro Rubini a écrit :
> [Vous ne recevez pas souvent de courriers de [email protected]. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>
> Thanks davide for the good explanation
> (and the link the the old email I forgot about)
>
>> tl;dr: sta2x11 support can be removed.
>
> Confirmed.
>
> The point here is that we talked with the vendor. They are still using
> the device (with some extra internal patches), but new development at
> kernel level is stopped.
>
> Since the device is not currently available to the general public,
> it's ok to save the maintaining efforts.
>
> I can submit patch[es] next week or ack removal if somebody submits
> them earlier.
>
I have resurected removal patch from Davide Ciminaghi
https://lore.kernel.org/lkml/64236c96ff620d64aa46d0fc3dac159a98c64709.1375867291.git.rubini@gnudd.com/
And I will add it in my series
https://patchwork.ozlabs.org/project/linux-gpio/list/?series=315826
prior to patch 4 in next re-spin (likely in a few days) because patch 4
is otherwise requiring modification of gpio-sta2x11.c
Thanks
Christophe
tl;dr: sta2x11 support can be removed.
On Sun, Aug 28, 2022 at 12:04:29PM +0200, Arnd Bergmann wrote:
> On Sun, Aug 28, 2022 at 11:06 AM Christophe Leroy
> <[email protected]> wrote:
> > Le 26/08/2022 ?? 23:54, Linus Walleij a ??crit :
....
>
> I think that just means the code that one would have to modify
> is in vendor kernels of devices using this chip, but there is no
> way to fix those if they are not in mainline. The last meaningful
> patches on this SoC support were in 2012 by Davide Ciminaghi
> and Alessandro Rubini, though they still Acked patches after that.
>
> I wonder if I was missing the interesting bit about it, if the driver
> is just obsolete and can be removed, or if there is something
> that is still worth fixing here.
>
Hi,
the sta2x11 was a chip containing AMBA peripherals and a PCIe to AMBA bridge
(it is still in production as far as I know, but deprecated for new designs).
It would typically be installed on x86 machines, so you needed to build and
run AMBA drivers in an x86 environment. The original drivers we started from
had platform data, but then we were told to switch to DTS.
Device trees, though, were not very common under x86 at the time and,
perhaps most important, we had a bunch of amba peripherals "behind" a
pci bus, which is a dynamic thing. Our idea was to build a device
tree at runtime (in user space) and then booting a second kernel via
kexec with the correct DTB, but this was not a complete solution.
For instance we needed to patch the device tree at runtime to
take dynamically assigned IRQ numbers into account.
Also the clocks tree had to be dynamically instantiated, once for each sta2x11
chip. Finally, there were some problems allocating dma buffers because
the AMBA side of the bridge could only reach some ranges of physical
addresses.
We had a more or less working prototype, and you may want to have a look
at some of our work:
https://lore.kernel.org/lkml/[email protected]/t/
Nevertheless the upstreaming effort was eventually too big for Alessandro
and myself.
So the sta2x11 drivers upstreaming project has been abandoned (even though
I like to think of it as one of the funniest failures of my life).
Sta2x11 related drivers can of course be removed.
Thanks and regards
Davide
On Tue, Aug 30, 2022 at 9:58 AM Davide Ciminaghi <[email protected]> wrote:
> the sta2x11 was a chip containing AMBA peripherals and a PCIe to AMBA bridge
> (it is still in production as far as I know, but deprecated for new designs).
> It would typically be installed on x86 machines, so you needed to build and
> run AMBA drivers in an x86 environment. The original drivers we started from
> had platform data, but then we were told to switch to DTS.
For the record I think that was bad advice, I hope it wasn't me.
But the world was different back then I suppose.
Adding DTS to x86 which is inherently ACPI is not a good idea.
Especially if you look at how SBSA ACPI UARTS were done
in drivers/tty/serial/amba-pl011.c.
Today I think we would solve this by simply creating software nodes, which
are the same no matter if you're doing device tree or ACPI, so it would
have been much easier now, I think. :/
Yours,
Linus Walleij
On Wed, Aug 31, 2022 at 03:32:25PM +0200, Linus Walleij wrote:
> On Tue, Aug 30, 2022 at 9:58 AM Davide Ciminaghi <[email protected]> wrote:
>
> > the sta2x11 was a chip containing AMBA peripherals and a PCIe to AMBA bridge
> > (it is still in production as far as I know, but deprecated for new designs).
> > It would typically be installed on x86 machines, so you needed to build and
> > run AMBA drivers in an x86 environment. The original drivers we started from
> > had platform data, but then we were told to switch to DTS.
>
> For the record I think that was bad advice, I hope it wasn't me.
> But the world was different back then I suppose.
> Adding DTS to x86 which is inherently ACPI is not a good idea.
> Especially if you look at how SBSA ACPI UARTS were done
> in drivers/tty/serial/amba-pl011.c.
>
now that I think of it, ACPI was also listed as a possible choice, but the
problem was that we didn't know much about ACPI, and took the DTS way.
So there was no bad advice, just fear of the unknown :-)
Thanks
Davide
On Wed, Aug 31, 2022 at 5:14 PM Davide Ciminaghi <[email protected]> wrote:
> On Wed, Aug 31, 2022 at 03:32:25PM +0200, Linus Walleij wrote:
> > On Tue, Aug 30, 2022 at 9:58 AM Davide Ciminaghi <[email protected]> wrote:
> >
> > > the sta2x11 was a chip containing AMBA peripherals and a PCIe to AMBA bridge
> > > (it is still in production as far as I know, but deprecated for new designs).
> > > It would typically be installed on x86 machines, so you needed to build and
> > > run AMBA drivers in an x86 environment. The original drivers we started from
> > > had platform data, but then we were told to switch to DTS.
> >
> > For the record I think that was bad advice, I hope it wasn't me.
> > But the world was different back then I suppose.
> > Adding DTS to x86 which is inherently ACPI is not a good idea.
> > Especially if you look at how SBSA ACPI UARTS were done
> > in drivers/tty/serial/amba-pl011.c.
> >
> now that I think of it, ACPI was also listed as a possible choice, but the
> problem was that we didn't know much about ACPI, and took the DTS way.
> So there was no bad advice, just fear of the unknown :-)
Feel free to ask, we have experts in the mailing list(s).
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 01, 2022 at 12:07:26AM +0300, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 5:14 PM Davide Ciminaghi <[email protected]> wrote:
> > On Wed, Aug 31, 2022 at 03:32:25PM +0200, Linus Walleij wrote:
> > > On Tue, Aug 30, 2022 at 9:58 AM Davide Ciminaghi <[email protected]> wrote:
> > >
> > > > the sta2x11 was a chip containing AMBA peripherals and a PCIe to AMBA bridge
> > > > (it is still in production as far as I know, but deprecated for new designs).
> > > > It would typically be installed on x86 machines, so you needed to build and
> > > > run AMBA drivers in an x86 environment. The original drivers we started from
> > > > had platform data, but then we were told to switch to DTS.
> > >
> > > For the record I think that was bad advice, I hope it wasn't me.
> > > But the world was different back then I suppose.
> > > Adding DTS to x86 which is inherently ACPI is not a good idea.
> > > Especially if you look at how SBSA ACPI UARTS were done
> > > in drivers/tty/serial/amba-pl011.c.
> > >
> > now that I think of it, ACPI was also listed as a possible choice, but the
> > problem was that we didn't know much about ACPI, and took the DTS way.
> > So there was no bad advice, just fear of the unknown :-)
>
> Feel free to ask, we have experts in the mailing list(s).
>
Thanks ! I'll keep that in mind in case I need some ACPI advice.
I'm afraid it's too late for the sta2x11, though. Its kernel is now a
downstream one, and it's been freezed, as the SOC has been deprecated
for new designs.
As Alessandro said, we'll submit (or ack) patches for removal.
Thanks again and regards
Davide