| Message ID | 1371784037.19200.1.camel@phoenix |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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 >
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
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
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
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)
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(-)