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

2013-06-21 Thread 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@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_GPIO128
 +#else
 +#define OMAP_MAX_GPIO192
 +#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_OUT0
  #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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread Axel Lin 
2013/6/21 Lubomir Popov lpo...@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@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_GPIO128
 +#else
 +#define OMAP_MAX_GPIO192
 +#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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread Lubomir Popov 
Hi Axel,

On 21/06/13 10:13, Axel Lin wrote:
 2013/6/21 Lubomir Popov lpo...@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@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_GPIO128
 +#else
 +#define OMAP_MAX_GPIO192
 +#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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread 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 lpo...@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@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_GPIO128
 +#else
 +#define OMAP_MAX_GPIO192
 +#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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread Axel Lin 
2013/6/21 Heiko Schocher h...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-21 Thread 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-20 Thread Axel Lin 
AM33XX has 4 gpio banks, thus the valid gpio range should be 0 ... 127.

Signed-off-by: Axel Lin axel@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-06-20 Thread 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@ingics.com

Acked-by: Stefan Roese s...@denx.de

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot