Re: [U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

2015-08-12 Thread Fabio Estevam 
On Wed, Aug 12, 2015 at 11:15 AM, Simon Glass s...@chromium.org wrote:

 +#define GPIO_SWPORTA_DR0x00
 +#define GPIO_SWPORTA_DDR   0x04
 +#define GPIO_INTEN 0x30
 +#define GPIO_INTMASK   0x34
 +#define GPIO_INTTYPE_LEVEL 0x38
 +#define GPIO_INT_POLARITY  0x3c
 +#define GPIO_INTSTATUS 0x40
 +#define GPIO_PORTA_DEBOUNCE0x48
 +#define GPIO_PORTA_EOI 0x4c
 +#define GPIO_EXT_PORTA 0x50

 What's the deal with C structures? Has the policy on this changed? I

I thought we no longer need to access registers via structs, and
accessing them via base + offset, like the kernel does is OK in
U-boot:
https://www.marc.info/?l=u-bootm=142609602127309w=2

Regards,

Fabio Estevam
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

2015-08-12 Thread Marek Vasut 
On Wednesday, August 12, 2015 at 04:15:00 PM, Simon Glass wrote:
 Hi Marek,
 
 On 10 August 2015 at 09:30, Marek Vasut ma...@denx.de wrote:
  Add driver for the DesignWare APB GPIO IP block.
  This driver is DM capable and probes from DT.
  
  Signed-off-by: Marek Vasut ma...@denx.de
  Cc: Simon Glass s...@chromium.org
  ---
  
   drivers/gpio/Kconfig  |   7 ++
   drivers/gpio/Makefile |   1 +
   drivers/gpio/dwapb_gpio.c | 167
   ++ 3 files changed, 175
   insertions(+)
   create mode 100644 drivers/gpio/dwapb_gpio.c
 
 Reviewed-by: Simon Glass s...@chromium.org
 
 Please see nits/suggestions below.
 
 Are you going to submit a GPIO binding change for this?

You mean for the bank-name ? Yes, I think it only makes sense to do it.

  V2: Obtain the bank name from the bank-name property instead.
  
  diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
  index 0c43777..c04388d 100644
  --- a/drivers/gpio/Kconfig
  +++ b/drivers/gpio/Kconfig
  @@ -8,6 +8,13 @@ config DM_GPIO
  
particular GPIOs that they provide. The uclass interface
is defined in include/asm-generic/gpio.h.
  
  +config DWAPB_GPIO
  +   bool DWAPB GPIO driver
  +   depends on DM  DM_GPIO
  +   default n
  +   help
  + Support for the Designware APB GPIO driver.
 
 You could expand this to talk about bank naming, features supported,
 chips which use it.

Done

  +
  
   config LPC32XX_GPIO
   
  bool LPC32XX GPIO driver
  depends on DM
  
  diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
  index 67c6374..603c96b 100644
  --- a/drivers/gpio/Makefile
  +++ b/drivers/gpio/Makefile
  @@ -6,6 +6,7 @@
  
   #
   
   ifndef CONFIG_SPL_BUILD
  
  +obj-$(CONFIG_DWAPB_GPIO)   += dwapb_gpio.o
  
   obj-$(CONFIG_AXP_GPIO) += axp_gpio.o
   endif
   obj-$(CONFIG_DM_GPIO)  += gpio-uclass.o
  
  diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
  new file mode 100644
  index 000..72cec48
  --- /dev/null
  +++ b/drivers/gpio/dwapb_gpio.c
  @@ -0,0 +1,167 @@
  +/*
  + * (C) Copyright 2015 Marek Vasut ma...@denx.de
  + *
  + * DesignWare APB GPIO driver
  + *
  + * SPDX-License-Identifier:GPL-2.0+
  + */
  +
  +#include common.h
  +#include malloc.h
  +#include asm/arch/gpio.h
  +#include asm/gpio.h
  +#include asm/io.h
  +#include dm.h
  +#include dm/device-internal.h
  +#include dm/lists.h
  +#include dm/root.h
  +#include errno.h
 
 should go just below common.h

Yup

  +
  +DECLARE_GLOBAL_DATA_PTR;
  +
  +#define GPIO_SWPORTA_DR0x00
  +#define GPIO_SWPORTA_DDR   0x04
  +#define GPIO_INTEN 0x30
  +#define GPIO_INTMASK   0x34
  +#define GPIO_INTTYPE_LEVEL 0x38
  +#define GPIO_INT_POLARITY  0x3c
  +#define GPIO_INTSTATUS 0x40
  +#define GPIO_PORTA_DEBOUNCE0x48
  +#define GPIO_PORTA_EOI 0x4c
  +#define GPIO_EXT_PORTA 0x50
 
 What's the deal with C structures? Has the policy on this changed? I
 can't help thinking that your GPIO_ prefix is unnecessary.

I think we don't do that anymore. The GPIO_ stuff is copied from Linux.

  +
  +struct gpio_dwapb_platdata {
  +   const char  *name;
  +   int bank;
  +   int pins;
  +   fdt_addr_t  base;
  +};
  +
  +static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
  +{
  +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
  +
  +   clrbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
  +   return 0;
  +}
  +
  +static int dwapb_gpio_direction_output(struct udevice *dev, unsigned
  pin, +int val)
  +{
  +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
  +
  +   setbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
  +
  +   if (val)
  +   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
  +   else
  +   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
  +
  +   return 0;
  +}
  +
  +static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
  +{
  +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
  +   return !!(readl(plat-base + GPIO_EXT_PORTA)  (1  pin));
  +}
  +
  +
  +static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int
  val) +{
  +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
  +
  +   if (val)
  +   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
  +   else
  +   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
  +
  +   return 0;
  +}
  +
  +static const struct dm_gpio_ops gpio_dwapb_ops = {
  +   .direction_input= dwapb_gpio_direction_input,
  +   .direction_output   = dwapb_gpio_direction_output,
  +   .get_value  = dwapb_gpio_get_value,
  +   .set_value  = dwapb_gpio_set_value,
  +};
  +
  +static int gpio_dwapb_probe(struct

Re: [U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

2015-08-12 Thread Simon Glass 
Hi Marek,

On 10 August 2015 at 09:30, Marek Vasut ma...@denx.de wrote:
 Add driver for the DesignWare APB GPIO IP block.
 This driver is DM capable and probes from DT.

 Signed-off-by: Marek Vasut ma...@denx.de
 Cc: Simon Glass s...@chromium.org
 ---
  drivers/gpio/Kconfig  |   7 ++
  drivers/gpio/Makefile |   1 +
  drivers/gpio/dwapb_gpio.c | 167 
 ++
  3 files changed, 175 insertions(+)
  create mode 100644 drivers/gpio/dwapb_gpio.c

Reviewed-by: Simon Glass s...@chromium.org

Please see nits/suggestions below.

Are you going to submit a GPIO binding change for this?


 V2: Obtain the bank name from the bank-name property instead.

 diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
 index 0c43777..c04388d 100644
 --- a/drivers/gpio/Kconfig
 +++ b/drivers/gpio/Kconfig
 @@ -8,6 +8,13 @@ config DM_GPIO
   particular GPIOs that they provide. The uclass interface
   is defined in include/asm-generic/gpio.h.

 +config DWAPB_GPIO
 +   bool DWAPB GPIO driver
 +   depends on DM  DM_GPIO
 +   default n
 +   help
 + Support for the Designware APB GPIO driver.

You could expand this to talk about bank naming, features supported,
chips which use it.

 +
  config LPC32XX_GPIO
 bool LPC32XX GPIO driver
 depends on DM
 diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
 index 67c6374..603c96b 100644
 --- a/drivers/gpio/Makefile
 +++ b/drivers/gpio/Makefile
 @@ -6,6 +6,7 @@
  #

  ifndef CONFIG_SPL_BUILD
 +obj-$(CONFIG_DWAPB_GPIO)   += dwapb_gpio.o
  obj-$(CONFIG_AXP_GPIO) += axp_gpio.o
  endif
  obj-$(CONFIG_DM_GPIO)  += gpio-uclass.o
 diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
 new file mode 100644
 index 000..72cec48
 --- /dev/null
 +++ b/drivers/gpio/dwapb_gpio.c
 @@ -0,0 +1,167 @@
 +/*
 + * (C) Copyright 2015 Marek Vasut ma...@denx.de
 + *
 + * DesignWare APB GPIO driver
 + *
 + * SPDX-License-Identifier:GPL-2.0+
 + */
 +
 +#include common.h
 +#include malloc.h
 +#include asm/arch/gpio.h
 +#include asm/gpio.h
 +#include asm/io.h
 +#include dm.h
 +#include dm/device-internal.h
 +#include dm/lists.h
 +#include dm/root.h
 +#include errno.h

should go just below common.h

 +
 +DECLARE_GLOBAL_DATA_PTR;
 +
 +#define GPIO_SWPORTA_DR0x00
 +#define GPIO_SWPORTA_DDR   0x04
 +#define GPIO_INTEN 0x30
 +#define GPIO_INTMASK   0x34
 +#define GPIO_INTTYPE_LEVEL 0x38
 +#define GPIO_INT_POLARITY  0x3c
 +#define GPIO_INTSTATUS 0x40
 +#define GPIO_PORTA_DEBOUNCE0x48
 +#define GPIO_PORTA_EOI 0x4c
 +#define GPIO_EXT_PORTA 0x50

What's the deal with C structures? Has the policy on this changed? I
can't help thinking that your GPIO_ prefix is unnecessary.

 +
 +struct gpio_dwapb_platdata {
 +   const char  *name;
 +   int bank;
 +   int pins;
 +   fdt_addr_t  base;
 +};
 +
 +static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
 +{
 +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
 +
 +   clrbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
 +   return 0;
 +}
 +
 +static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin,
 +int val)
 +{
 +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
 +
 +   setbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
 +
 +   if (val)
 +   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
 +   else
 +   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
 +
 +   return 0;
 +}
 +
 +static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
 +{
 +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
 +   return !!(readl(plat-base + GPIO_EXT_PORTA)  (1  pin));
 +}
 +
 +
 +static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val)
 +{
 +   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
 +
 +   if (val)
 +   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
 +   else
 +   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
 +
 +   return 0;
 +}
 +
 +static const struct dm_gpio_ops gpio_dwapb_ops = {
 +   .direction_input= dwapb_gpio_direction_input,
 +   .direction_output   = dwapb_gpio_direction_output,
 +   .get_value  = dwapb_gpio_get_value,
 +   .set_value  = dwapb_gpio_set_value,
 +};
 +
 +static int gpio_dwapb_probe(struct udevice *dev)
 +{
 +   struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
 +   struct gpio_dwapb_platdata *plat = dev-platdata;
 +
 +   if (!plat)
 +   return 0;
 +
 +   priv-gpio_count = plat-pins;
 +   priv-bank_name = plat-name;
 +
 +   return 0;
 +}
 +
 +static int gpio_dwapb_bind(struct udevice *dev)
 +{
 +   struct gpio_dwapb_platdata *plat =

Re: [U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

2015-08-12 Thread Simon Glass 
+Tom

Hi Fabio,

On 12 August 2015 at 08:24, Fabio Estevam feste...@gmail.com wrote:

 On Wed, Aug 12, 2015 at 11:15 AM, Simon Glass s...@chromium.org wrote:

  +#define GPIO_SWPORTA_DR0x00
  +#define GPIO_SWPORTA_DDR   0x04
  +#define GPIO_INTEN 0x30
  +#define GPIO_INTMASK   0x34
  +#define GPIO_INTTYPE_LEVEL 0x38
  +#define GPIO_INT_POLARITY  0x3c
  +#define GPIO_INTSTATUS 0x40
  +#define GPIO_PORTA_DEBOUNCE0x48
  +#define GPIO_PORTA_EOI 0x4c
  +#define GPIO_EXT_PORTA 0x50
 
  What's the deal with C structures? Has the policy on this changed? I

 I thought we no longer need to access registers via structs, and
 accessing them via base + offset, like the kernel does is OK in
 U-boot:
 https://www.marc.info/?l=u-bootm=142609602127309w=2

Thanks for the link. No I did not know that.

Perhaps we should add the type-checking referred to in that thread?
What is it and who is doing it?

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

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

2015-08-10 Thread Marek Vasut 
Add driver for the DesignWare APB GPIO IP block.
This driver is DM capable and probes from DT.

Signed-off-by: Marek Vasut ma...@denx.de
Cc: Simon Glass s...@chromium.org
---
 drivers/gpio/Kconfig  |   7 ++
 drivers/gpio/Makefile |   1 +
 drivers/gpio/dwapb_gpio.c | 167 ++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/gpio/dwapb_gpio.c

V2: Obtain the bank name from the bank-name property instead.

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0c43777..c04388d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -8,6 +8,13 @@ config DM_GPIO
  particular GPIOs that they provide. The uclass interface
  is defined in include/asm-generic/gpio.h.
 
+config DWAPB_GPIO
+   bool DWAPB GPIO driver
+   depends on DM  DM_GPIO
+   default n
+   help
+ Support for the Designware APB GPIO driver.
+
 config LPC32XX_GPIO
bool LPC32XX GPIO driver
depends on DM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 67c6374..603c96b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,6 +6,7 @@
 #
 
 ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_DWAPB_GPIO)   += dwapb_gpio.o
 obj-$(CONFIG_AXP_GPIO) += axp_gpio.o
 endif
 obj-$(CONFIG_DM_GPIO)  += gpio-uclass.o
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
new file mode 100644
index 000..72cec48
--- /dev/null
+++ b/drivers/gpio/dwapb_gpio.c
@@ -0,0 +1,167 @@
+/*
+ * (C) Copyright 2015 Marek Vasut ma...@denx.de
+ *
+ * DesignWare APB GPIO driver
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include common.h
+#include malloc.h
+#include asm/arch/gpio.h
+#include asm/gpio.h
+#include asm/io.h
+#include dm.h
+#include dm/device-internal.h
+#include dm/lists.h
+#include dm/root.h
+#include errno.h
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define GPIO_SWPORTA_DR0x00
+#define GPIO_SWPORTA_DDR   0x04
+#define GPIO_INTEN 0x30
+#define GPIO_INTMASK   0x34
+#define GPIO_INTTYPE_LEVEL 0x38
+#define GPIO_INT_POLARITY  0x3c
+#define GPIO_INTSTATUS 0x40
+#define GPIO_PORTA_DEBOUNCE0x48
+#define GPIO_PORTA_EOI 0x4c
+#define GPIO_EXT_PORTA 0x50
+
+struct gpio_dwapb_platdata {
+   const char  *name;
+   int bank;
+   int pins;
+   fdt_addr_t  base;
+};
+
+static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
+{
+   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+   clrbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
+   return 0;
+}
+
+static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin,
+int val)
+{
+   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+   setbits_le32(plat-base + GPIO_SWPORTA_DDR, 1  pin);
+
+   if (val)
+   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
+   else
+   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
+
+   return 0;
+}
+
+static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
+{
+   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+   return !!(readl(plat-base + GPIO_EXT_PORTA)  (1  pin));
+}
+
+
+static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val)
+{
+   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+   if (val)
+   setbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
+   else
+   clrbits_le32(plat-base + GPIO_SWPORTA_DR, 1  pin);
+
+   return 0;
+}
+
+static const struct dm_gpio_ops gpio_dwapb_ops = {
+   .direction_input= dwapb_gpio_direction_input,
+   .direction_output   = dwapb_gpio_direction_output,
+   .get_value  = dwapb_gpio_get_value,
+   .set_value  = dwapb_gpio_set_value,
+};
+
+static int gpio_dwapb_probe(struct udevice *dev)
+{
+   struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
+   struct gpio_dwapb_platdata *plat = dev-platdata;
+
+   if (!plat)
+   return 0;
+
+   priv-gpio_count = plat-pins;
+   priv-bank_name = plat-name;
+
+   return 0;
+}
+
+static int gpio_dwapb_bind(struct udevice *dev)
+{
+   struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+   const void *blob = gd-fdt_blob;
+   struct udevice *subdev;
+   fdt_addr_t base;
+   int ret, node, bank = 0;
+
+   /* If this is a child device, there is nothing to do here */
+   if (plat)
+   return 0;
+
+   base = fdtdec_get_addr(blob, dev-of_offset, reg);
+   if (base == FDT_ADDR_T_NONE) {
+   debug(Can't get the GPIO register base address\n);
+   return -ENXIO;
+   }
+
+   for (node = fdt_first_subnode(blob, dev-of_offset);
+node  0;
+node = fdt_next_subnode(blob, node)) {
+   if