Message ID | 1395438866-1193-2-git-send-email-ijc@hellion.org.uk |
---|---|
State | Changes Requested |
Delegated to: | Albert ARIBAUD |
Headers | show |
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 0000000..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 <tangliang@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 ?
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.
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.
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
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
Dear Ian, [Cc: list truncated / changed] In message <1395822638.29683.9.camel@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
Dear Ian Campbell, In message <1395822781.29683.12.camel@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
On Wed, 2014-03-26 at 10:03 +0100, Wolfgang Denk wrote: > Dear Ian Campbell, > > In message <1395822781.29683.12.camel@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.
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.camel@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
Dear Ian Campbell, In message <1395826756.22808.13.camel@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
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.
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 0000000..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 <tangliang@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 0000000..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 <tangliang@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 GPIO_BANK(pin) ((pin) >> 5) +#define GPIO_NUM(pin) ((pin) & 0x1f) + +#define GPIO_CFG_INDEX(pin) (((pin) & 0x1f) >> 3) +#define GPIO_CFG_OFFSET(pin) ((((pin) & 0x1f) & 0x7) << 2) + +#define GPIO_DRV_INDEX(pin) (((pin) & 0x1f) >> 4) +#define GPIO_DRV_OFFSET(pin) ((((pin) & 0x1f) & 0xf) << 1) + +#define GPIO_PULL_INDEX(pin) (((pin) & 0x1f) >> 4) +#define GPIO_PULL_OFFSET(pin) ((((pin) & 0x1f) & 0xf) << 1) + +/* GPIO bank sizes */ +#define SUNXI_GPIO_A_NR 32 +#define SUNXI_GPIO_B_NR 32 +#define SUNXI_GPIO_C_NR 32 +#define SUNXI_GPIO_D_NR 32 +#define SUNXI_GPIO_E_NR 32 +#define SUNXI_GPIO_F_NR 32 +#define SUNXI_GPIO_G_NR 32 +#define SUNXI_GPIO_H_NR 32 +#define SUNXI_GPIO_I_NR 32 + +#define SUNXI_GPIO_NEXT(__gpio) \ + ((__gpio##_START) + (__gpio##_NR) + 0) + +enum sunxi_gpio_number { + SUNXI_GPIO_A_START = 0, + SUNXI_GPIO_B_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_A), + SUNXI_GPIO_C_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_B), + SUNXI_GPIO_D_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_C), + SUNXI_GPIO_E_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_D), + SUNXI_GPIO_F_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_E), + SUNXI_GPIO_G_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_F), + SUNXI_GPIO_H_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_G), + SUNXI_GPIO_I_START = SUNXI_GPIO_NEXT(SUNXI_GPIO_H), +}; + +/* SUNXI GPIO number definitions */ +#define SUNXI_GPA(_nr) (SUNXI_GPIO_A_START + (_nr)) +#define SUNXI_GPB(_nr) (SUNXI_GPIO_B_START + (_nr)) +#define SUNXI_GPC(_nr) (SUNXI_GPIO_C_START + (_nr)) +#define SUNXI_GPD(_nr) (SUNXI_GPIO_D_START + (_nr)) +#define SUNXI_GPE(_nr) (SUNXI_GPIO_E_START + (_nr)) +#define SUNXI_GPF(_nr) (SUNXI_GPIO_F_START + (_nr)) +#define SUNXI_GPG(_nr) (SUNXI_GPIO_G_START + (_nr)) +#define SUNXI_GPH(_nr) (SUNXI_GPIO_H_START + (_nr)) +#define SUNXI_GPI(_nr) (SUNXI_GPIO_I_START + (_nr)) + +/* GPIO pin function config */ +#define SUNXI_GPIO_INPUT 0 +#define SUNXI_GPIO_OUTPUT 1 + +#define SUNXI_GPA0_EMAC 2 +#define SUN7I_GPA0_GMAC 5 + +#define SUNXI_GPB0_TWI0 2 + +#define SUN4I_GPB22_UART0_TX 2 +#define SUN4I_GPB23_UART0_RX 2 + +#define SUN5I_GPB19_UART0_TX 2 +#define SUN5I_GPB20_UART0_RX 2 + +#define SUN5I_GPG3_UART1_TX 4 +#define SUN5I_GPG4_UART1_RX 4 + +#define SUNXI_GPC6_SDC2 3 + +#define SUNXI_GPF0_SDC0 2 + +#define SUNXI_GPF2_SDC0 2 +#define SUNXI_GPF2_UART0_TX 4 +#define SUNXI_GPF4_UART0_RX 4 + +#define SUN4I_GPG0_SDC1 4 + +#define SUN4I_GPH22_SDC1 5 + +#define SUN4I_GPI4_SDC3 2 + +/* GPIO pin pull-up/down config */ +#define SUNXI_GPIO_PULL_DISABLE 0 +#define SUNXI_GPIO_PULL_UP 1 +#define SUNXI_GPIO_PULL_DOWN 2 + +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 + +#endif /* _SUNXI_GPIO_H */