[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Axel Lin]

AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
v2: define OMAP_MAX_GPIO and use it.
This change is mainly based on Stefan's comment, however I use
OMAP_MAX_GPIO instead of CONFIG_OMAP_MAX_GPIO because having CONFIG_ prefix
seems meaning it can be configurable in configs.

 drivers/gpio/omap_gpio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
index a30d7f0..6fa57c9 100644
--- a/drivers/gpio/omap_gpio.c
+++ b/drivers/gpio/omap_gpio.c
@@ -40,6 +40,12 @@
 #include <asm/io.h>
 #include <asm/errno.h>
 
+#if defined(CONFIG_AM33XX)
+#define OMAP_MAX_GPIO		128
+#else
+#define OMAP_MAX_GPIO		192
+#endif
+
 #define OMAP_GPIO_DIR_OUT	0
 #define OMAP_GPIO_DIR_IN	1
 
@@ -55,7 +61,7 @@ static inline int get_gpio_index(int gpio)
 
 int gpio_is_valid(int gpio)
 {
-	return (gpio >= 0) && (gpio < 192);
+	return (gpio >= 0) && (gpio < OMAP_MAX_GPIO);
 }
 
 static int check_gpio(int gpio)
-- 
1.8.1.2

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Stefan Roese]

On 21.06.2013 05:07, Axel Lin wrote:
> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Acked-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Lubomir Popov]

Hi Axel,

On 21/06/13 06:07, Axel Lin wrote:
> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> v2: define OMAP_MAX_GPIO and use it.
> This change is mainly based on Stefan's comment, however I use
> OMAP_MAX_GPIO instead of CONFIG_OMAP_MAX_GPIO because having CONFIG_ prefix
> seems meaning it can be configurable in configs.
> 
>  drivers/gpio/omap_gpio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
> index a30d7f0..6fa57c9 100644
> --- a/drivers/gpio/omap_gpio.c
> +++ b/drivers/gpio/omap_gpio.c
> @@ -40,6 +40,12 @@
>  #include <asm/io.h>
>  #include <asm/errno.h>
>  
> +#if defined(CONFIG_AM33XX)
> +#define OMAP_MAX_GPIO		128
> +#else
> +#define OMAP_MAX_GPIO		192
> +#endif

Please be aware that OMAP54XX and DRA7XX SoCs have 8 banks per 32 GPIOs,
that is, 256 in total. The DRA7xx config defines CONFIG_DRA7XX, but also
includes omap5_common.h, where CONFIG_OMAP54XX is defined (due to sharing
of many internal IPs with the OMAP5, including GPIO). The above definitions
should be changed to something like:

#if defined(CONFIG_OMAP54XX)
#define	OMAP_MAX_GPIO		256	/* OMAP54XX and DRA7XX */
#else
#if defined (CONFIG_AM33XX)
#define OMAP_MAX_GPIO		128
#else
#define OMAP_MAX_GPIO		192	/* All other OMAP3/4 */
#endif
#endif

> +
>  #define OMAP_GPIO_DIR_OUT	0
>  #define OMAP_GPIO_DIR_IN	1
>  
> @@ -55,7 +61,7 @@ static inline int get_gpio_index(int gpio)
>  
>  int gpio_is_valid(int gpio)
>  {
> -	return (gpio >= 0) && (gpio < 192);
> +	return (gpio >= 0) && (gpio < OMAP_MAX_GPIO);
>  }
>  
>  static int check_gpio(int gpio)
> 

Regards,
Lubo

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Axel Lin]

2013/6/21 Lubomir Popov <lpopov@mm-sol.com>:
> Hi Axel,
>
> On 21/06/13 06:07, Axel Lin wrote:
>> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> ---
>> v2: define OMAP_MAX_GPIO and use it.
>> This change is mainly based on Stefan's comment, however I use
>> OMAP_MAX_GPIO instead of CONFIG_OMAP_MAX_GPIO because having CONFIG_ prefix
>> seems meaning it can be configurable in configs.
>>
>>  drivers/gpio/omap_gpio.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
>> index a30d7f0..6fa57c9 100644
>> --- a/drivers/gpio/omap_gpio.c
>> +++ b/drivers/gpio/omap_gpio.c
>> @@ -40,6 +40,12 @@
>>  #include <asm/io.h>
>>  #include <asm/errno.h>
>>
>> +#if defined(CONFIG_AM33XX)
>> +#define OMAP_MAX_GPIO                128
>> +#else
>> +#define OMAP_MAX_GPIO                192
>> +#endif
>
> Please be aware that OMAP54XX and DRA7XX SoCs have 8 banks per 32 GPIOs,
> that is, 256 in total. The DRA7xx config defines CONFIG_DRA7XX, but also
> includes omap5_common.h, where CONFIG_OMAP54XX is defined (due to sharing
> of many internal IPs with the OMAP5, including GPIO). The above definitions
> should be changed to something like:
>
> #if defined(CONFIG_OMAP54XX)
> #define OMAP_MAX_GPIO           256     /* OMAP54XX and DRA7XX */

In arch/arm/cpu/armv7/omap5/hwinit.c
we have below settings:

static struct gpio_bank gpio_bank_54xx[6] = {
        { (void *)OMAP54XX_GPIO1_BASE, METHOD_GPIO_24XX },
        { (void *)OMAP54XX_GPIO2_BASE, METHOD_GPIO_24XX },
        { (void *)OMAP54XX_GPIO3_BASE, METHOD_GPIO_24XX },
        { (void *)OMAP54XX_GPIO4_BASE, METHOD_GPIO_24XX },
        { (void *)OMAP54XX_GPIO5_BASE, METHOD_GPIO_24XX },
        { (void *)OMAP54XX_GPIO6_BASE, METHOD_GPIO_24XX },
};

const struct gpio_bank *const omap_gpio_bank = gpio_bank_54xx;

So gpio_bank_54xx only has 6 entries rather than 8 entries.
Seems need to fix this before setting OMAP_MAX_GPIO to 256.

I'm wondering if it's ok to have this patch as is, and then an incremental
patch to set gpio_bank_54xx[] and OMAP_MAX_GPIO for
OMAP54XX and DRA7XX.

Regards,
Axel

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Lubomir Popov]

Hi Axel,

On 21/06/13 10:13, Axel Lin wrote:
> 2013/6/21 Lubomir Popov <lpopov@mm-sol.com>:
>> Hi Axel,
>>
>> On 21/06/13 06:07, Axel Lin wrote:
>>> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
>>>
>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>> ---
>>> v2: define OMAP_MAX_GPIO and use it.
>>> This change is mainly based on Stefan's comment, however I use
>>> OMAP_MAX_GPIO instead of CONFIG_OMAP_MAX_GPIO because having CONFIG_ prefix
>>> seems meaning it can be configurable in configs.
>>>
>>>  drivers/gpio/omap_gpio.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
>>> index a30d7f0..6fa57c9 100644
>>> --- a/drivers/gpio/omap_gpio.c
>>> +++ b/drivers/gpio/omap_gpio.c
>>> @@ -40,6 +40,12 @@
>>>  #include <asm/io.h>
>>>  #include <asm/errno.h>
>>>
>>> +#if defined(CONFIG_AM33XX)
>>> +#define OMAP_MAX_GPIO                128
>>> +#else
>>> +#define OMAP_MAX_GPIO                192
>>> +#endif
>>
>> Please be aware that OMAP54XX and DRA7XX SoCs have 8 banks per 32 GPIOs,
>> that is, 256 in total. The DRA7xx config defines CONFIG_DRA7XX, but also
>> includes omap5_common.h, where CONFIG_OMAP54XX is defined (due to sharing
>> of many internal IPs with the OMAP5, including GPIO). The above definitions
>> should be changed to something like:
>>
>> #if defined(CONFIG_OMAP54XX)
>> #define OMAP_MAX_GPIO           256     /* OMAP54XX and DRA7XX */
> 
> In arch/arm/cpu/armv7/omap5/hwinit.c
> we have below settings:
> 
> static struct gpio_bank gpio_bank_54xx[6] = {
>         { (void *)OMAP54XX_GPIO1_BASE, METHOD_GPIO_24XX },
>         { (void *)OMAP54XX_GPIO2_BASE, METHOD_GPIO_24XX },
>         { (void *)OMAP54XX_GPIO3_BASE, METHOD_GPIO_24XX },
>         { (void *)OMAP54XX_GPIO4_BASE, METHOD_GPIO_24XX },
>         { (void *)OMAP54XX_GPIO5_BASE, METHOD_GPIO_24XX },
>         { (void *)OMAP54XX_GPIO6_BASE, METHOD_GPIO_24XX },
> };
> 
> const struct gpio_bank *const omap_gpio_bank = gpio_bank_54xx;
> 
> So gpio_bank_54xx only has 6 entries rather than 8 entries.
> Seems need to fix this before setting OMAP_MAX_GPIO to 256.
Sure. File arch/arm/include/asm/arch-omap5/gpio.h shall need
fixing as well, by adding:

#define OMAP54XX_GPIO7_BASE		0x48051000
#define OMAP54XX_GPIO8_BASE		0x48053000

> 
> I'm wondering if it's ok to have this patch as is, and then an incremental
> patch to set gpio_bank_54xx[] and OMAP_MAX_GPIO for
> OMAP54XX and DRA7XX.
Feel free to proceed as you wish. If you modify hwint.c and gpio.h, then
the three patches could be combined in a series, e.g. "Fix valid GPIO
range for OMAP".

Tom?

> 
> Regards,
> Axel
> 
Thanks,
Lubo

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Lubomir Popov]

One more thing that perhaps seems more reasonable in general:

These OMAP_MAX_GPIO defines could go into the corresponding .../arch-omap*.h
files, where the base addresses are defined, and the number of GPIOs is
implicitly obvious. And we shall have no ugly #ifdefs in the GPIO driver.

Tom, what do you think?

Thanks,
Lubo

On 21/06/13 10:29, Lubomir Popov wrote:
> Hi Axel,
> 
> On 21/06/13 10:13, Axel Lin wrote:
>> 2013/6/21 Lubomir Popov <lpopov@mm-sol.com>:
>>> Hi Axel,
>>>
>>> On 21/06/13 06:07, Axel Lin wrote:
>>>> AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
>>>>
>>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>>> ---
>>>> v2: define OMAP_MAX_GPIO and use it.
>>>> This change is mainly based on Stefan's comment, however I use
>>>> OMAP_MAX_GPIO instead of CONFIG_OMAP_MAX_GPIO because having CONFIG_ prefix
>>>> seems meaning it can be configurable in configs.
>>>>
>>>>  drivers/gpio/omap_gpio.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpio/omap_gpio.c b/drivers/gpio/omap_gpio.c
>>>> index a30d7f0..6fa57c9 100644
>>>> --- a/drivers/gpio/omap_gpio.c
>>>> +++ b/drivers/gpio/omap_gpio.c
>>>> @@ -40,6 +40,12 @@
>>>>  #include <asm/io.h>
>>>>  #include <asm/errno.h>
>>>>
>>>> +#if defined(CONFIG_AM33XX)
>>>> +#define OMAP_MAX_GPIO                128
>>>> +#else
>>>> +#define OMAP_MAX_GPIO                192
>>>> +#endif
>>>
>>> Please be aware that OMAP54XX and DRA7XX SoCs have 8 banks per 32 GPIOs,
>>> that is, 256 in total. The DRA7xx config defines CONFIG_DRA7XX, but also
>>> includes omap5_common.h, where CONFIG_OMAP54XX is defined (due to sharing
>>> of many internal IPs with the OMAP5, including GPIO). The above definitions
>>> should be changed to something like:
>>>
>>> #if defined(CONFIG_OMAP54XX)
>>> #define OMAP_MAX_GPIO           256     /* OMAP54XX and DRA7XX */
>>
>> In arch/arm/cpu/armv7/omap5/hwinit.c
>> we have below settings:
>>
>> static struct gpio_bank gpio_bank_54xx[6] = {
>>         { (void *)OMAP54XX_GPIO1_BASE, METHOD_GPIO_24XX },
>>         { (void *)OMAP54XX_GPIO2_BASE, METHOD_GPIO_24XX },
>>         { (void *)OMAP54XX_GPIO3_BASE, METHOD_GPIO_24XX },
>>         { (void *)OMAP54XX_GPIO4_BASE, METHOD_GPIO_24XX },
>>         { (void *)OMAP54XX_GPIO5_BASE, METHOD_GPIO_24XX },
>>         { (void *)OMAP54XX_GPIO6_BASE, METHOD_GPIO_24XX },
>> };
>>
>> const struct gpio_bank *const omap_gpio_bank = gpio_bank_54xx;
>>
>> So gpio_bank_54xx only has 6 entries rather than 8 entries.
>> Seems need to fix this before setting OMAP_MAX_GPIO to 256.
> Sure. File arch/arm/include/asm/arch-omap5/gpio.h shall need
> fixing as well, by adding:
> 
> #define OMAP54XX_GPIO7_BASE		0x48051000
> #define OMAP54XX_GPIO8_BASE		0x48053000
> 
>>
>> I'm wondering if it's ok to have this patch as is, and then an incremental
>> patch to set gpio_bank_54xx[] and OMAP_MAX_GPIO for
>> OMAP54XX and DRA7XX.
> Feel free to proceed as you wish. If you modify hwint.c and gpio.h, then
> the three patches could be combined in a series, e.g. "Fix valid GPIO
> range for OMAP".
> 
> Tom?
> 
>>
>> Regards,
>> Axel
>>
> Thanks,
> Lubo
> 

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Heiko Schocher]

Hello Lubomir,

Am 21.06.2013 09:44, schrieb Lubomir Popov:
> One more thing that perhaps seems more reasonable in general:
> 
> These OMAP_MAX_GPIO defines could go into the corresponding .../arch-omap*.h
> files, where the base addresses are defined, and the number of GPIOs is
> implicitly obvious. And we shall have no ugly #ifdefs in the GPIO driver.

This sounds good. I vote for doing it this way ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Axel Lin]

2013/6/21 Heiko Schocher <hs@denx.de>:
> Hello Lubomir,
>
> Am 21.06.2013 09:44, schrieb Lubomir Popov:
>> One more thing that perhaps seems more reasonable in general:
>>
>> These OMAP_MAX_GPIO defines could go into the corresponding .../arch-omap*.h
>> files, where the base addresses are defined, and the number of GPIOs is
>> implicitly obvious. And we shall have no ugly #ifdefs in the GPIO driver.
>
> This sounds good. I vote for doing it this way ...

I'm sending patches base on this idea now...

Thanks,
Axel

[U-Boot] [PATCH v2] gpio: omap_gpio: Fix valid gpio range for AM33XX

[Lubomir Popov]

Hi Heiko, Axel,

On 21/06/13 11:44, Heiko Schocher wrote:
> Hello Lubomir,
> 
> Am 21.06.2013 09:44, schrieb Lubomir Popov:
>> One more thing that perhaps seems more reasonable in general:
>>
>> These OMAP_MAX_GPIO defines could go into the corresponding .../arch-omap*.h
>> files, where the base addresses are defined, and the number of GPIOs is
>> implicitly obvious. And we shall have no ugly #ifdefs in the GPIO driver.
> 
> This sounds good. I vote for doing it this way ...

Sure, but for things to work on the OMAP5 SoCs, the GPIO modules 7 & 8
clocks have to be enabled as well, otherwise we get a Data Abort
exception. This requires one more fix in arch/arm/cpu/armv7/omap5/hw_data.c:

		(*prcm)->cm_l4per_gpio7_clkctrl,
		(*prcm)->cm_l4per_gpio8_clkctrl,

have to be added to the clk_modules_hw_auto_essential[] array.

I'm not mentioning here that, in order to test this in U-Boot, the pads that
are intended to be used as GPIO have to be properly configured (the GPIO mux
mode selected). For me the easiest way to do this was by again defining
CONFIG_SYS_ENABLE_PADS_ALL (which is being descoped) because my board mux
file has quite a lot of pads set to GPIO in the non_essential array.

I have some additional concerns as well: on the different SoCs we have
different GPIOs that are input-only or output-only, and this is currently
not handled by the driver in any way. We have just to hope that users are
educated enough... Unfortunately my experience tells me that very few
people care to read these thousands of pages long tech docs :( .
Anyway, I guess we shall have to leave this as is for now...

I would suggest that Axel withdraws V3 and makes a V4 (and, please,
cleanly based on master branch, not incremental), where:

1. The OMAP_MAX_GPIO defines are moved to all
arch/arm/include/asm/arch-omap*/gpio.h files, and

2. The fix to arch/arm/cpu/armv7/omap5/hw_data.c is made - this
affects OMAP54XX and DRA7XX.

Or perhaps an entirely new patch subject (that is, new V1 patch), so that
no confusion happens - we have actually shifted the scope a bit with
these additional fixes. Something like

ARM: OMAP: GPIO: Fix valid range and enable usage of all GPIOs on OMAP5

Otherwise, functionally tested on a custom OMAP5430 board with some
GPIOs > 191, works OK (with all discussed fixes applied manually).
Shall give my "Tested-by:" when I apply the final version as a patch
and test again.

> 
> bye,
> Heiko
> 
Regards,
Lubo