Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-27 Thread Ian Campbell 
On Wed, 2014-03-26 at 10:01 +0100, Wolfgang Denk wrote:
 I'm not an expert for ARM, but this indeed looks suspiscious - thanks
 for reporting this.

FYI I made the change which prompted this and the resulting code was the
same see https://groups.google.com/forum/#!topic/linux-sunxi/REZ18q0wcDY
The barriers were only ever compile barrier() type, not cpu barriers.

So the open coded thing was effectively:
v = read();
barrier();
v = fiddlebits(b)
barrier();
write(v);

Whereas the code using clrsetbits is basically:
write(fiddlebits(read()))

Which I think is OK in this case at least. Not sure about the general
case.

Ian.

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

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Ian Campbell 
On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
  + cfg = readl(pio-cfg[0] + index);
  + cfg = ~(0xf  offset);
  + cfg |= val  offset;
  +
  + writel(cfg, pio-cfg[0] + index);
 
 clrsetbits_le32() here.

I looked at this transform in a few different contexts and one concern I
had was that readl and writel have barriers in them (after the read and
before the write respectively) while clrsetbits and friends do not. I
don't think this will matter for the read/modify/write bit twiddling
itself (since there are register dependencies) but I was slightly
concerned that the barriers were hiding the lack of explicit barriers
which would be required between the various reads/writes. 

But I think I am probably being overly cautious here and the obvious
transformation can be made. Anyone got any thoughts?

Ian.

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

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Ian Campbell 
On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:

  +int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
  +int sunxi_gpio_get_cfgpin(u32 pin);
  +int sunxi_gpio_set_drv(u32 pin, u32 val);
  +int sunxi_gpio_set_pull(u32 pin, u32 val);
  +int name_to_gpio(const char *name);
  +#define name_to_gpio name_to_gpio
 
 What is this ugly define doing here ?

common/cmd_gpio.c uses the #ifndef name_to_gpio pattern to provide (or
not) a default fallback implementation. I think this is a reasonably
(but not very) common idiom for such cases where the non-default variant
is not best expressed as a macro.

Ian.

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

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Marek Vasut 
On Wednesday, March 26, 2014 at 09:30:38 AM, Ian Campbell wrote:
 On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
   + cfg = readl(pio-cfg[0] + index);
   + cfg = ~(0xf  offset);
   + cfg |= val  offset;
   +
   + writel(cfg, pio-cfg[0] + index);
  
  clrsetbits_le32() here.
 
 I looked at this transform in a few different contexts and one concern I
 had was that readl and writel have barriers in them (after the read and
 before the write respectively) while clrsetbits and friends do not. I
 don't think this will matter for the read/modify/write bit twiddling
 itself (since there are register dependencies) but I was slightly
 concerned that the barriers were hiding the lack of explicit barriers
 which would be required between the various reads/writes.
 
 But I think I am probably being overly cautious here and the obvious
 transformation can be made. Anyone got any thoughts?

+CC Tom, Albert .

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Marek Vasut 
On Wednesday, March 26, 2014 at 09:33:01 AM, Ian Campbell wrote:
 On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
   +int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
   +int sunxi_gpio_get_cfgpin(u32 pin);
   +int sunxi_gpio_set_drv(u32 pin, u32 val);
   +int sunxi_gpio_set_pull(u32 pin, u32 val);
   +int name_to_gpio(const char *name);
   +#define name_to_gpio name_to_gpio
  
  What is this ugly define doing here ?
 
 common/cmd_gpio.c uses the #ifndef name_to_gpio pattern to provide (or
 not) a default fallback implementation. I think this is a reasonably
 (but not very) common idiom for such cases where the non-default variant
 is not best expressed as a macro.

This should be fixed, the name_to_gpio() there should be replaced by a __weak 
function. (patch is welcome)

Nonetheless, in your case, please don't do #define FOO FOO, but choose some 
sensible name for the function.

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Wolfgang Denk 
Dear Ian,

[Cc: list truncated / changed]

In message 1395822638.29683.9.ca...@dagon.hellion.org.uk you wrote:

 I looked at this transform in a few different contexts and one concern I
 had was that readl and writel have barriers in them (after the read and
 before the write respectively) while clrsetbits and friends do not. I

They are supposed to.  They map to the out_##type() / in_##type()
standard I/O accessors which are supposed to be suitable to access
device registers.

I can see that the ARM implementation maps this to __raw_write##type()
/ __raw_read##type() and then to __arch_put##type() /
__arch_get##type() which indeed do not include MBs.

 But I think I am probably being overly cautious here and the obvious
 transformation can be made. Anyone got any thoughts?

I'm not an expert for ARM, but this indeed looks suspiscious - thanks
for reporting this.

It is conspicuous that Linux does not use out_##type() / in_##type()
for ARM, and instead uses iowrite##type() / ioread##type() - which do
include MBs.

Albert - what do you think?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Drun'? 'm not drun'! You woudn' dare call m' drun' if I was sober!
 - Terry Pratchett, _Men at Arms_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Wolfgang Denk 
Dear Ian Campbell,

In message 1395822781.29683.12.ca...@dagon.hellion.org.uk you wrote:
 On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
 
   +int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
   +int sunxi_gpio_get_cfgpin(u32 pin);
   +int sunxi_gpio_set_drv(u32 pin, u32 val);
   +int sunxi_gpio_set_pull(u32 pin, u32 val);
   +int name_to_gpio(const char *name);
   +#define name_to_gpio name_to_gpio
  
  What is this ugly define doing here ?
 
 common/cmd_gpio.c uses the #ifndef name_to_gpio pattern to provide (or
 not) a default fallback implementation. I think this is a reasonably
 (but not very) common idiom for such cases where the non-default variant
 is not best expressed as a macro.

Please add a comment to explain that.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
EMACS belongs in sys/errno.h: Editor too big!
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Ian Campbell 
On Wed, 2014-03-26 at 10:03 +0100, Wolfgang Denk wrote:
 Dear Ian Campbell,
 
 In message 1395822781.29683.12.ca...@dagon.hellion.org.uk you wrote:
  On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
  
+int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
+int sunxi_gpio_get_cfgpin(u32 pin);
+int sunxi_gpio_set_drv(u32 pin, u32 val);
+int sunxi_gpio_set_pull(u32 pin, u32 val);
+int name_to_gpio(const char *name);
+#define name_to_gpio name_to_gpio
   
   What is this ugly define doing here ?
  
  common/cmd_gpio.c uses the #ifndef name_to_gpio pattern to provide (or
  not) a default fallback implementation. I think this is a reasonably
  (but not very) common idiom for such cases where the non-default variant
  is not best expressed as a macro.
 
 Please add a comment to explain that.

Unless you object I think I'll do as Marek suggested name the function
sunxi_name_to_gpio and make the #define to that, it seems more
consistent that way.

Ian.

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

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Marek Vasut 
On Wednesday, March 26, 2014 at 10:39:16 AM, Ian Campbell wrote:
 On Wed, 2014-03-26 at 10:03 +0100, Wolfgang Denk wrote:
  Dear Ian Campbell,
  
  In message 1395822781.29683.12.ca...@dagon.hellion.org.uk you wrote:
   On Mon, 2014-03-24 at 21:54 +0100, Marek Vasut wrote:
 +int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
 +int sunxi_gpio_get_cfgpin(u32 pin);
 +int sunxi_gpio_set_drv(u32 pin, u32 val);
 +int sunxi_gpio_set_pull(u32 pin, u32 val);
 +int name_to_gpio(const char *name);
 +#define name_to_gpio name_to_gpio

What is this ugly define doing here ?
   
   common/cmd_gpio.c uses the #ifndef name_to_gpio pattern to provide (or
   not) a default fallback implementation. I think this is a reasonably
   (but not very) common idiom for such cases where the non-default
   variant is not best expressed as a macro.
  
  Please add a comment to explain that.
 
 Unless you object I think I'll do as Marek suggested name the function
 sunxi_name_to_gpio and make the #define to that, it seems more
 consistent that way.

I'd suggest you fix cmd_gpio.c while at it too ;-)

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-26 Thread Wolfgang Denk 
Dear Ian Campbell,

In message 1395826756.22808.13.ca...@kazak.uk.xensource.com you wrote:

  Please add a comment to explain that.
 
 Unless you object I think I'll do as Marek suggested name the function
 sunxi_name_to_gpio and make the #define to that, it seems more
 consistent that way.

That's better, indeed.  Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
In any group of employed individuals the only naturally  early  riser
is  _always_  the office manager, who will _always_ leave reproachful
little notes ... on the desks of their subordinates.
- Terry Pratchett, _Lords and Ladies_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-24 Thread Marek Vasut 
On Friday, March 21, 2014 at 10:54:19 PM, Ian Campbell wrote:
[...]

 diff --git a/arch/arm/cpu/armv7/sunxi/pinmux.c
 b/arch/arm/cpu/armv7/sunxi/pinmux.c new file mode 100644
 index 000..8f5cbfe
 --- /dev/null
 +++ b/arch/arm/cpu/armv7/sunxi/pinmux.c
 @@ -0,0 +1,80 @@
 +/*
 + * (C) Copyright 2007-2011
 + * Allwinner Technology Co., Ltd. www.allwinnertech.com
 + * Tom Cubie tangli...@allwinnertech.com
 + *
 + * SPDX-License-Identifier:  GPL-2.0+
 + */
 +
 +#include common.h
 +#include asm/io.h
 +#include asm/arch/gpio.h
 +
 +int sunxi_gpio_set_cfgpin(u32 pin, u32 val)
 +{
 + u32 cfg;
 + u32 bank = GPIO_BANK(pin);
 + u32 index = GPIO_CFG_INDEX(pin);
 + u32 offset = GPIO_CFG_OFFSET(pin);
 + struct sunxi_gpio *pio =
 + ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
 +
 + cfg = readl(pio-cfg[0] + index);
 + cfg = ~(0xf  offset);
 + cfg |= val  offset;
 +
 + writel(cfg, pio-cfg[0] + index);

clrsetbits_le32() here.

 + return 0;
 +}
 +
 +int sunxi_gpio_get_cfgpin(u32 pin)
 +{
 + u32 cfg;
 + u32 bank = GPIO_BANK(pin);
 + u32 index = GPIO_CFG_INDEX(pin);
 + u32 offset = GPIO_CFG_OFFSET(pin);
 + struct sunxi_gpio *pio =
 + ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
 +
 + cfg = readl(pio-cfg[0] + index);
 + cfg = offset;
 +
 + return cfg  0xf;
 +}
 +
 +int sunxi_gpio_set_drv(u32 pin, u32 val)
 +{
 + u32 drv;
 + u32 bank = GPIO_BANK(pin);
 + u32 index = GPIO_DRV_INDEX(pin);
 + u32 offset = GPIO_DRV_OFFSET(pin);
 + struct sunxi_gpio *pio =
 + ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
 +
 + drv = readl(pio-drv[0] + index);
 + drv = ~(0x3  offset);
 + drv |= val  offset;
 +
 + writel(drv, pio-drv[0] + index);

Here as well.

 + return 0;
 +}
 +
 +int sunxi_gpio_set_pull(u32 pin, u32 val)
 +{
 + u32 pull;
 + u32 bank = GPIO_BANK(pin);
 + u32 index = GPIO_PULL_INDEX(pin);
 + u32 offset = GPIO_PULL_OFFSET(pin);
 + struct sunxi_gpio *pio =
 + ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
 +
 + pull = readl(pio-pull[0] + index);
 + pull = ~(0x3  offset);
 + pull |= val  offset;
 +
 + writel(pull, pio-pull[0] + index);

Same here.

 + return 0;
 +}

[...]

 +int sunxi_gpio_set_cfgpin(u32 pin, u32 val);
 +int sunxi_gpio_get_cfgpin(u32 pin);
 +int sunxi_gpio_set_drv(u32 pin, u32 val);
 +int sunxi_gpio_set_pull(u32 pin, u32 val);
 +int name_to_gpio(const char *name);
 +#define name_to_gpio name_to_gpio

What is this ugly define doing here ?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support

2014-03-21 Thread Ian Campbell 
This has been stripped back for mainlining and supports only sun7i. These
changes are not useful by themselves but are split out to make the patch sizes
more manageable.

As well as the following signed-off-by the sunxi branch shows commits to these
files authored by the following:
  Carl van Schaik
  Henrik Nordstrom
  Stefan Roese
  Tom Cubie

Signed-off-by: Chen-Yu Tsai w...@csie.org
Signed-off-by: Hans de Goede hdego...@redhat.com
Signed-off-by: Ma Haijun mahaij...@gmail.com
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
Signed-off-by: Ian Campbell i...@hellion.org.uk
Reviewed-by: Tom Rini tr...@ti.com
---
v2: Based on u-boot-sunxi.git#sunxi d9aa5dd3d15c sunxi: mmc:
checkpatch whitespace fixes with v2014.04-rc2 merged in:
  - Additional pin definitions

v1: Based on u-boot-sunxi.git#sunxi commit d854c4de2f57 arm: Handle .gnu.hash
section in ldscripts vs v2014.01.
---
 arch/arm/cpu/armv7/sunxi/Makefile  |   1 +
 arch/arm/cpu/armv7/sunxi/pinmux.c  |  80 ++
 arch/arm/include/asm/arch-sunxi/gpio.h | 145 +
 3 files changed, 226 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/sunxi/pinmux.c
 create mode 100644 arch/arm/include/asm/arch-sunxi/gpio.h

diff --git a/arch/arm/cpu/armv7/sunxi/Makefile 
b/arch/arm/cpu/armv7/sunxi/Makefile
index 787a127..b3ef8a0 100644
--- a/arch/arm/cpu/armv7/sunxi/Makefile
+++ b/arch/arm/cpu/armv7/sunxi/Makefile
@@ -9,3 +9,4 @@
 #
 obj-y  += timer.o
 obj-y  += clock.o
+obj-y  += pinmux.o
diff --git a/arch/arm/cpu/armv7/sunxi/pinmux.c 
b/arch/arm/cpu/armv7/sunxi/pinmux.c
new file mode 100644
index 000..8f5cbfe
--- /dev/null
+++ b/arch/arm/cpu/armv7/sunxi/pinmux.c
@@ -0,0 +1,80 @@
+/*
+ * (C) Copyright 2007-2011
+ * Allwinner Technology Co., Ltd. www.allwinnertech.com
+ * Tom Cubie tangli...@allwinnertech.com
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include common.h
+#include asm/io.h
+#include asm/arch/gpio.h
+
+int sunxi_gpio_set_cfgpin(u32 pin, u32 val)
+{
+   u32 cfg;
+   u32 bank = GPIO_BANK(pin);
+   u32 index = GPIO_CFG_INDEX(pin);
+   u32 offset = GPIO_CFG_OFFSET(pin);
+   struct sunxi_gpio *pio =
+   ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
+
+   cfg = readl(pio-cfg[0] + index);
+   cfg = ~(0xf  offset);
+   cfg |= val  offset;
+
+   writel(cfg, pio-cfg[0] + index);
+
+   return 0;
+}
+
+int sunxi_gpio_get_cfgpin(u32 pin)
+{
+   u32 cfg;
+   u32 bank = GPIO_BANK(pin);
+   u32 index = GPIO_CFG_INDEX(pin);
+   u32 offset = GPIO_CFG_OFFSET(pin);
+   struct sunxi_gpio *pio =
+   ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
+
+   cfg = readl(pio-cfg[0] + index);
+   cfg = offset;
+
+   return cfg  0xf;
+}
+
+int sunxi_gpio_set_drv(u32 pin, u32 val)
+{
+   u32 drv;
+   u32 bank = GPIO_BANK(pin);
+   u32 index = GPIO_DRV_INDEX(pin);
+   u32 offset = GPIO_DRV_OFFSET(pin);
+   struct sunxi_gpio *pio =
+   ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
+
+   drv = readl(pio-drv[0] + index);
+   drv = ~(0x3  offset);
+   drv |= val  offset;
+
+   writel(drv, pio-drv[0] + index);
+
+   return 0;
+}
+
+int sunxi_gpio_set_pull(u32 pin, u32 val)
+{
+   u32 pull;
+   u32 bank = GPIO_BANK(pin);
+   u32 index = GPIO_PULL_INDEX(pin);
+   u32 offset = GPIO_PULL_OFFSET(pin);
+   struct sunxi_gpio *pio =
+   ((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)-gpio_bank[bank];
+
+   pull = readl(pio-pull[0] + index);
+   pull = ~(0x3  offset);
+   pull |= val  offset;
+
+   writel(pull, pio-pull[0] + index);
+
+   return 0;
+}
diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h 
b/arch/arm/include/asm/arch-sunxi/gpio.h
new file mode 100644
index 000..802f347
--- /dev/null
+++ b/arch/arm/include/asm/arch-sunxi/gpio.h
@@ -0,0 +1,145 @@
+/*
+ * (C) Copyright 2007-2012
+ * Allwinner Technology Co., Ltd. www.allwinnertech.com
+ * Tom Cubie tangli...@allwinnertech.com
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#ifndef _SUNXI_GPIO_H
+#define _SUNXI_GPIO_H
+
+#include linux/types.h
+
+/*
+ * sunxi has 9 banks of gpio, they are:
+ * PA0 - PA17 | PB0 - PB23 | PC0 - PC24
+ * PD0 - PD27 | PE0 - PE31 | PF0 - PF5
+ * PG0 - PG9  | PH0 - PH27 | PI0 - PI12
+ */
+
+#define SUNXI_GPIO_A   0
+#define SUNXI_GPIO_B   1
+#define SUNXI_GPIO_C   2
+#define SUNXI_GPIO_D   3
+#define SUNXI_GPIO_E   4
+#define SUNXI_GPIO_F   5
+#define SUNXI_GPIO_G   6
+#define SUNXI_GPIO_H   7
+#define SUNXI_GPIO_I   8
+
+struct sunxi_gpio {
+   u32 cfg[4];
+   u32 dat;
+   u32 drv[2];
+   u32 pull[2];
+};
+
+/* gpio interrupt control */
+struct sunxi_gpio_int {
+   u32 cfg[3];
+   u32 ctl;
+   u32 sta;
+   u32 deb;/* interrupt debounce */
+};
+
+struct sunxi_gpio_reg {
+   struct sunxi_gpio gpio_bank[9];
+   u8 res[0xbc];
+   struct sunxi_gpio_int gpio_int;
+};
+
+#define