gpio: omap_gpio: Fix valid gpio range for AM33XX

PATCH Superseded Axel Lin 4 participants 9 messages Jun 21, 2013 GPIO

lore

Share & tools

lore

Axel Lin

author

2013-06-21 03:07 UTC #0 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

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

Signed-off-by Axel Lin <[email protected]>

---
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(-)

drivers/gpio/omap_gpio.c +7 -1

index a30d7f0..6fa57c9 100644
@@ -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)

Signature · 1 line

1.8.1.2

Stefan Roese

2013-06-21 05:23 UTC #1 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

Quoted 3 lines · Axel Lin

AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
Signed-off-by Axel Lin <[email protected]>

Acked-by Stefan Roese <[email protected]>

Thanks,
Stefan

Lubomir Popov

2013-06-21 06:59 UTC #2 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

In reply to Axel Lin

Hi Axel,

Quoted 17 lines · Axel Lin

AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
Signed-off-by Axel Lin <[email protected]>
---
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(-)
drivers/gpio/omap_gpio.c +0 -0
index a30d7f0..6fa57c9 100644

drivers/gpio/omap_gpio.c line 47

Quoted 8 lines

drivers/gpio/omap_gpio.c +5 -0

@@ -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

Quoted 6 lines

drivers/gpio/omap_gpio.c +1 -0

+
 #define OMAP_GPIO_DIR_OUT	0
 #define OMAP_GPIO_DIR_IN	1

drivers/gpio/omap_gpio.c static inline int get_gpio_index(int gpio) · line 67

Quoted 8 lines

drivers/gpio/omap_gpio.c +1 -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

Axel Lin

author

2013-06-21 07:13 UTC #3 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

2013/6/21 Lubomir Popov <[email protected]>:

Quoted 35 lines

Hi Axel,
Quoted 17 lines · Axel Lin
AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
Signed-off-by Axel Lin <[email protected]>
---
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(-)
drivers/gpio/omap_gpio.c +0 -0
index a30d7f0..6fa57c9 100644
drivers/gpio/omap_gpio.c line 47
Quoted 8 lines
drivers/gpio/omap_gpio.c +5 -0
@@ -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:

Quoted 2 lines

#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

Lubomir Popov

2013-06-21 07:29 UTC #4 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

Hi Axel,

Quoted 51 lines · Axel Lin

2013/6/21 Lubomir Popov <[email protected]>:
Quoted 35 lines
Hi Axel,
Quoted 17 lines · Axel Lin
AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
Signed-off-by Axel Lin <[email protected]>
---
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(-)
drivers/gpio/omap_gpio.c +0 -0
index a30d7f0..6fa57c9 100644
drivers/gpio/omap_gpio.c line 47
Quoted 8 lines
drivers/gpio/omap_gpio.c +5 -0
@@ -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:
Quoted 2 lines
#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 },
};

Quoted 4 lines

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

Quoted 4 lines

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?

Quoted 4 lines

Regards,
Axel

Thanks,
Lubo

Lubomir Popov

2013-06-21 07:44 UTC #5 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

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

Quoted 81 lines · Lubomir Popov

Hi Axel,
Quoted 51 lines · Axel Lin
2013/6/21 Lubomir Popov <[email protected]>:
Quoted 35 lines
Hi Axel,
Quoted 17 lines · Axel Lin
AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.
Signed-off-by Axel Lin <[email protected]>
---
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(-)
drivers/gpio/omap_gpio.c +0 -0
index a30d7f0..6fa57c9 100644
drivers/gpio/omap_gpio.c line 47
Quoted 8 lines
drivers/gpio/omap_gpio.c +5 -0
@@ -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:
Quoted 2 lines
#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 },
};
Quoted 4 lines
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
Quoted 4 lines
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?
Quoted 4 lines
Regards,
Axel
Thanks,
Lubo

Heiko Schocher

2013-06-21 08:44 UTC #6 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

Hello Lubomir,

Am 21.06.2013 09:44, schrieb Lubomir Popov:

Quoted 5 lines

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

Signature · 2 lines

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

Axel Lin

author

2013-06-21 08:45 UTC #7 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

2013/6/21 Heiko Schocher <[email protected]>:

Quoted 10 lines

Hello Lubomir,
Am 21.06.2013 09:44, schrieb Lubomir Popov:
Quoted 5 lines
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

Lubomir Popov

2013-06-21 10:04 UTC #8 lore raw

PATCH v2 1/1 gpio: omap_gpio: Fix valid gpio range for AM33XX

In reply to Heiko Schocher

Hi Heiko, Axel,

Quoted 10 lines · Heiko Schocher

Hello Lubomir,
Am 21.06.2013 09:44, schrieb Lubomir Popov:
Quoted 5 lines
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.

Quoted 4 lines

bye,
Heiko

Regards,
Lubo