| Message ID | 4FE74C8E.2070306@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 06/24/2012 11:21 AM, Vikram Narayanan wrote: First off, it's great to see some patches for the chip. Thanks. Sorry for being so nit-picky below; it's a tendency of mine... It'd be nice to include a patch description here, so that something shows up besides just the patch subject. > diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h b/arch/arm/include/asm/arch-bcm2835/gpio.h > +#define BCM2835_GPIO_BASE (0x7E200000) There's no need for brackets around simple numbers. > +#define BCM2835_MAX_GPIOS 53 I think that should be named either BCM2835_NUM_GPIOS or BCM2835_MAX_GPIO; you can have a number of GPIOs or a maximum GPIO number, but not a maximum GPIOs. > +#define BCM2835_GPIO_FSEL_CLR_MASK (0x7) The mask applies equally to set or clear; I'd rename this to just BCM2835_GPIO_FSEL_MASK. > +#define BCM2835_GPIO_OUTPUT (0x1) I'd like to see a matching BCM2835_GPIO_INPUT function select value too; even if the value is actually 0 so it doesn't matter, it's still good to document it. It would also be useful to define all the values for the function selector, in order to extend this driver to provide pinmuxing in the future, e.g. #define BCM2835_GPIO_FSEL_ALT0 4 etc. > +#define GPIO_TO_BANK(n) (n / 10) > +#define GPIO_TO_PIN(n) (n % 10) There are two kinds of bank relevant to the current driver, and perhaps more in the HW. The macros should also be prefixed with the module name to avoid conflicts. PIN isn't really a good name, since the GPIOs themselves /are/ pins; how about SHIFT? I'd like to see these renamed BCM2835_GPIO_FSEL_BANK/SHIFT, and also to add BCM2835_GPIO_OTHER_BANK/SHIFT macros. (or perhaps GENERAL or 1BIT instead of OTHER?) > +struct bcm_gpio_regs { > + u32 gpfsel[6]; > + u32 reserved1; > + u32 gpset0; > + u32 gpset1; You could replace those two with gpset[2], and then use BCM2835_GPIO_OTHER_BANK() to index into the array. That will simplify the driver code a bit. The same applies to gpclr* and gplev* below. > + u32 reserved2; > + u32 gpclr0; > + u32 gpclr1; > + u32 reserved3; > + u32 gplev0; > + u32 gplev1; There are quite a few more registers defined in the datasheet after this. Even if the driver doesn't use them yet, it'd still be good to add them here. > +}; > diff --git a/drivers/gpio/rpi_gpio.c b/drivers/gpio/rpi_gpio.c I think this should be named bcm2835_gpio.c, since it's a driver for the SoC in general, rather than a particular board that uses the SoC. > +struct bcm_gpio { > + u32 bank; > + u32 pin; > +}; ... > +static int get_bank_pin(unsigned gpio, struct bcm_gpio *pio) > +{ > + int bank = GPIO_TO_BANK(gpio); > + int pin = GPIO_TO_PIN(gpio); > + > + if (!gpio_is_valid(gpio)) > + return -1; > + > + pin &= 0x09; GPIO_TO_PIN already performs any required masking. Also, this line doesn't work, because you want mod 9, not bitwise-and with 9. > + pio->pin = pin; > + pio->bank = bank; > + return 0; > +} I'm not really sure that structure or function are useful; you can just use macros BCM2835_GPIO_*_BANK/SHIFT directly in the code. > +int gpio_request(unsigned gpio, const char *label) > +{ > + return (gpio_is_valid(gpio)) ? 1 : 0; > +} > + > +int gpio_free(unsigned gpio) > +{ > + return 0; > +} Hmmm. Don't you want to do something like save the label away so you know who requested the pin for what, and mark it requested so you can't request it twice? IIRC, there's some "gpio info" command that will list out all the GPIO owners, at least for some pre-existing GPIO drivers. > +int gpio_direction_input(unsigned gpio) > +{ > + struct bcm_gpio pio; > + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; > + unsigned val = 0; There's no need to initialize val; it's unconditionally written to below before it's used. > + > + if (get_bank_pin(gpio, &pio)) > + return -1; I would drop that function call and the pio variable, and just use the macros directly where needed. > + val = readl(®->gpfsel[pio.bank]); > + val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << (pio.pin * 3)); That might be clearer as: val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << BCM2835_GPIO_FSEL_SHIFT(pin)); val |= ~(BCM2835_GPIO_OUTPUT << BCM2835_GPIO_FSEL_SHIFT(pin)); Similar for gpio_direction_output() below. > + writel(val, reg->gpfsel[pio.bank]); > +int gpio_get_value(unsigned gpio) > +{ > + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; > + unsigned val = 0; > + > + if (!gpio_is_valid(gpio)) > + return -1; > + > + val = (gpio < 32) ? readl(®->gplev0) : readl(®->gplev1); How about: val = readl(®->gplev[BCM2835_GPIO_OTHER_BANK(gpio)]); > + gpio &= 0x1f; That's not needed, since we extract only the bits we care about below anyway. > + return (val & (1 << gpio)) >> gpio; That would be simpler as: return (val >> BCM2835_GPIO_OTHER_SHIFT(gpio)) & 1; > +int gpio_set_value(unsigned gpio, int value) > +{ > + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; > + > + if (!gpio_is_valid(gpio)) > + return -1; > + > + if (gpio < 32) { > + if (value) > + writel((1 << gpio), reg->gpset0); > + else > + writel((1 << gpio), reg->gpclr0); > + } else { > + gpio &= 0x1f; > + if (value) > + writel((1 << pin), reg->gpset1); > + else > + writel((1 << pin), reg->gpclr1); > + } > + return 0; > +} How about: u32 *reg = value ? reg->gpset : reg->gpclr; writel(1 << BCM2835_GPIO_VALUE_SHIFT(gpio), reg[BCM2835_GPIO_VALUE_BANK(gpio)]); BTW, I don't think this patch compiles; gpio_set_value() references variable "pin", which doesn't exist.
Hello Stephen, On 6/27/2012 7:09 AM, Stephen Warren wrote: > On 06/24/2012 11:21 AM, Vikram Narayanan wrote: > > First off, it's great to see some patches for the chip. Thanks. Sorry > for being so nit-picky below; it's a tendency of mine... Thanks for the detailed review. I'd make a v2 for this. And I might probably include you in the signed-off-by line. Hope you don't mind that. Do you have a hosted repo somewhere for this rpi_b stuff? If so, please post it here or we shall have one, which has all the patches queued in for the mainline. What do you say? Share your opinions about this. > BTW, I don't think this patch compiles; gpio_set_value() references > variable "pin", which doesn't exist. Sorry about that, Late night submission! Thanks, Vikram
On 06/27/2012 11:32 AM, Vikram Narayanan wrote: > Hello Stephen, > > On 6/27/2012 7:09 AM, Stephen Warren wrote: >> On 06/24/2012 11:21 AM, Vikram Narayanan wrote: >> >> First off, it's great to see some patches for the chip. Thanks. Sorry >> for being so nit-picky below; it's a tendency of mine... > > Thanks for the detailed review. I'd make a v2 for this. > And I might probably include you in the signed-off-by line. > Hope you don't mind that. No, you shouldn't add any tags to the patch that refer to other people, except perhaps a Reported-By, without their explicitly giving those tags. Also, Signed-off-by wouldn't make sense here since I'm not vouching for the code or passing it along. Once V2 is posted, I may give an ack or review tag. > Do you have a hosted repo somewhere for this rpi_b stuff? > If so, please post it here or we shall have one, which has all the > patches queued in for the mainline. What do you say? Share your opinions > about this. I do have a repo. It's at: https://github.com/swarren/u-boot However, that's my personal work-space. The RPi patches should eventually make it into the official U-Boot repositories through the standard review process. They are: ARM repo: git://git.denx.de/u-boot-arm.git Main repo: git://git.denx.de/u-boot.git
On 6/27/2012 11:36 PM, Stephen Warren wrote: > On 06/27/2012 11:32 AM, Vikram Narayanan wrote: >> Hello Stephen, >> >> On 6/27/2012 7:09 AM, Stephen Warren wrote: >>> On 06/24/2012 11:21 AM, Vikram Narayanan wrote: >>> >>> First off, it's great to see some patches for the chip. Thanks. Sorry >>> for being so nit-picky below; it's a tendency of mine... >> >> Thanks for the detailed review. I'd make a v2 for this. >> And I might probably include you in the signed-off-by line. >> Hope you don't mind that. > > No, you shouldn't add any tags to the patch that refer to other people, > except perhaps a Reported-By, without their explicitly giving those tags. > > Also, Signed-off-by wouldn't make sense here since I'm not vouching for > the code or passing it along. Once V2 is posted, I may give an ack or > review tag. I'm aware of it. :) Just for the level of details you said to change in the code I said so. Don't mind that. >> Do you have a hosted repo somewhere for this rpi_b stuff? >> If so, please post it here or we shall have one, which has all the >> patches queued in for the mainline. What do you say? Share your opinions >> about this. > > I do have a repo. It's at: > https://github.com/swarren/u-boot > > However, that's my personal work-space. The RPi patches should > eventually make it into the official U-Boot repositories through the > standard review process. They are: > > ARM repo: > git://git.denx.de/u-boot-arm.git > > Main repo: > git://git.denx.de/u-boot.git I'm aware of this too. I'm referring to a public hosting of these RPi patches somewhere, so that it could easily be submitted to the mainline in _one_ shot. Since your initial SoC support patches aren't added to the u-boot-arm, I planned to have everything queued up for submission into the mainline *via* the mailing list. Hope you got my point.
On 06/27/2012 08:59 PM, Vikram Narayanan wrote: > On 6/27/2012 11:36 PM, Stephen Warren wrote: >> On 06/27/2012 11:32 AM, Vikram Narayanan wrote: ... >>> Do you have a hosted repo somewhere for this rpi_b stuff? >>> If so, please post it here or we shall have one, which has all the >>> patches queued in for the mainline. What do you say? Share your opinions >>> about this. >> >> I do have a repo. It's at: >> https://github.com/swarren/u-boot >> >> However, that's my personal work-space. The RPi patches should >> eventually make it into the official U-Boot repositories through the >> standard review process. They are: >> >> ARM repo: >> git://git.denx.de/u-boot-arm.git >> >> Main repo: >> git://git.denx.de/u-boot.git > > I'm aware of this too. I'm referring to a public hosting of these RPi > patches somewhere, so that it could easily be submitted to the mainline > in _one_ shot. Since your initial SoC support patches aren't added to > the u-boot-arm, I planned to have everything queued up for submission > into the mainline *via* the mailing list. Hope you got my point. I can certainly apply the GPIO patch to my github tree too if that helps. However, the U-Boot maintainers have indicated that BCM2835 support is too small to warrant their taking patches via git pull rather than mailing list posts.
diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h b/arch/arm/include/asm/arch-bcm2835/gpio.h new file mode 100644 index 0000000..3cba0fb --- /dev/null +++ b/arch/arm/include/asm/arch-bcm2835/gpio.h @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2012 Vikram Narayananan + * <vikram186@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _BCM2835_GPIO_H_ +#define _BCM2835_GPIO_H_ + +#define BCM2835_GPIO_BASE (0x7E200000) +#define BCM2835_MAX_GPIOS 53 + +#define BCM2835_GPIO_FSEL_CLR_MASK (0x7) +#define BCM2835_GPIO_OUTPUT (0x1) + +#define GPIO_TO_BANK(n) (n / 10) +#define GPIO_TO_PIN(n) (n % 10) + +struct bcm_gpio_regs { + u32 gpfsel[6]; + u32 reserved1; + u32 gpset0; + u32 gpset1; + u32 reserved2; + u32 gpclr0; + u32 gpclr1; + u32 reserved3; + u32 gplev0; + u32 gplev1; +}; + +#endif /* _BCM2835_GPIO_H_ */ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fb3b09a..b042c46 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -39,6 +39,7 @@ COBJS-$(CONFIG_TEGRA2_GPIO) += tegra2_gpio.o COBJS-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o COBJS-$(CONFIG_ALTERA_PIO) += altera_pio.o COBJS-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o +COBJS-$(CONFIG_BCM2835_GPIO) += rpi_gpio.o COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/gpio/rpi_gpio.c b/drivers/gpio/rpi_gpio.c new file mode 100644 index 0000000..c2b547f --- /dev/null +++ b/drivers/gpio/rpi_gpio.c @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2012 Vikram Narayananan + * <vikram186@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <asm/gpio.h> +#include <asm/io.h> + +struct bcm_gpio { + u32 bank; + u32 pin; +}; + +inline int gpio_is_valid(unsigned gpio) +{ + return (gpio > BCM2835_MAX_GPIOS) ? 0 : 1; +} + +static int get_bank_pin(unsigned gpio, struct bcm_gpio *pio) +{ + int bank = GPIO_TO_BANK(gpio); + int pin = GPIO_TO_PIN(gpio); + + if (!gpio_is_valid(gpio)) + return -1; + + pin &= 0x09; + pio->pin = pin; + pio->bank = bank; + return 0; +} + +int gpio_request(unsigned gpio, const char *label) +{ + return (gpio_is_valid(gpio)) ? 1 : 0; +} + +int gpio_free(unsigned gpio) +{ + return 0; +} + +int gpio_direction_input(unsigned gpio) +{ + struct bcm_gpio pio; + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + unsigned val = 0; + + if (get_bank_pin(gpio, &pio)) + return -1; + + val = readl(®->gpfsel[pio.bank]); + val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << (pio.pin * 3)); + writel(val, reg->gpfsel[pio.bank]); +} + +int gpio_direction_output(unsigned gpio, int value) +{ + struct bcm_gpio pio; + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + unsigned val = 0; + + if (get_bank_pin(gpio, &pio)) + return -1; + + val = readl(®->gpfsel[pio.bank]); + val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << (pio.pin * 3)); + val |= (BCM2835_GPIO_OUTPUT << (pio.pin * 3)); + writel(val, reg->gpfsel[pio.bank]); + + if (value) + gpio_set_value(gpio, value); +} + +int gpio_get_value(unsigned gpio) +{ + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + unsigned val = 0; + + if (!gpio_is_valid(gpio)) + return -1; + + val = (gpio < 32) ? readl(®->gplev0) : readl(®->gplev1); + gpio &= 0x1f; + + return (val & (1 << gpio)) >> gpio; +} + +int gpio_set_value(unsigned gpio, int value) +{ + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + + if (!gpio_is_valid(gpio)) + return -1; + + if (gpio < 32) { + if (value) + writel((1 << gpio), reg->gpset0); + else + writel((1 << gpio), reg->gpclr0); + } else { + gpio &= 0x1f; + if (value) + writel((1 << pin), reg->gpset1); + else + writel((1 << pin), reg->gpclr1); + } + return 0; +}
Signed-off-by: Vikram Narayanan <vikram186@gmail.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: Albert Aribaud <albert.u.boot@aribaud.net> --- arch/arm/include/asm/arch-bcm2835/gpio.h | 49 +++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/rpi_gpio.c | 128 ++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-bcm2835/gpio.h create mode 100644 drivers/gpio/rpi_gpio.c