Re: [U-Boot] [PATCH v2 2/9] sunxi: initial sun7i pinmux and gpio support
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
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
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
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
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
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
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
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
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
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
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
