[U-Boot] [PATCH v2 0/2] GPIO driver for BCM2835 SoC
Add GPIO driver for BCM2835 Soc. Also add the driver to the raspberrypi default config
Cc: Stephen Warren swarren@wwwdotorg.org Cc: Albert Aribaud albert.u.boot@aribaud.net
Vikram Narayanan (2): bcm: Add GPIO driver rbpi: Add BCM2835 GPIO driver for raspberry pi
Changes from v1: Incorporate comments from Stephen Warren.
arch/arm/include/asm/arch-bcm2835/gpio.h | 71 ++++++++++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio_bcm2835.c | 87 ++++++++++++++++++++++++++++++ include/configs/rpi_b.h | 3 +- 4 files changed, 161 insertions(+), 1 deletions(-) create mode 100644 arch/arm/include/asm/arch-bcm2835/gpio.h create mode 100644 drivers/gpio/gpio_bcm2835.c
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
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 | 71 ++++++++++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio_bcm2835.c | 87 ++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-bcm2835/gpio.h create mode 100644 drivers/gpio/gpio_bcm2835.c
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..3ab06e0 --- /dev/null +++ b/arch/arm/include/asm/arch-bcm2835/gpio.h @@ -0,0 +1,71 @@ +/* + * 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_NUM_GPIOS 53 + +#define BCM2835_GPIO_FSEL_MASK 0x7 +#define BCM2835_GPIO_INPUT 0x0 +#define BCM2835_GPIO_OUTPUT 0x1 +#define BCM2835_GPIO_ALT0 0x2 +#define BCM2835_GPIO_ALT1 0x3 +#define BCM2835_GPIO_ALT2 0x4 +#define BCM2835_GPIO_ALT3 0x5 +#define BCM2835_GPIO_ALT4 0x6 +#define BCM2835_GPIO_ALT5 0x7 + +#define BCM2835_GPIO_COMMON_BANK(gpio) ((gpio < 32) ? 0 : 1) +#define BCM2835_GPIO_COMMON_MASK(gpio) (gpio & 0x1f) + +#define BCM2835_GPIO_FSEL_BANK(gpio) (gpio / 10) +#define BCM2835_GPIO_FSEL_SHIFT(gpio) ((gpio % 10) * 3) + +struct bcm_gpio_regs { + u32 gpfsel[6]; + u32 reserved1; + u32 gpset[2]; + u32 reserved2; + u32 gpclr[2]; + u32 reserved3; + u32 gplev[2]; + u32 reserved4; + u32 gpeds[2]; + u32 reserved5; + u32 gpren[2]; + u32 reserved6; + u32 gpfen[2]; + u32 reserved7; + u32 gphen[2]; + u32 reserved8; + u32 gplen[2]; + u32 reserved9; + u32 gparen[2]; + u32 reserved10; + u32 gppud; + u32 gppudclk[2]; +}; + +#endif /* _BCM2835_GPIO_H_ */ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index fb3b09a..7653e84 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) += gpio_bcm2835.o
COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c new file mode 100644 index 0000000..23cdd90 --- /dev/null +++ b/drivers/gpio/gpio_bcm2835.c @@ -0,0 +1,87 @@ +/* + * 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> + +inline int gpio_is_valid(unsigned gpio) +{ + return (gpio > BCM2835_NUM_GPIOS) ? 0 : 1; +} + +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_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + unsigned val; + + val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); + val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio)); + writel(val, reg->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); +} + +int gpio_direction_output(unsigned gpio, int value) +{ + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + unsigned val; + + val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); + val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio)); + val |= (BCM2835_GPIO_OUTPUT << BCM2835_GPIO_FSEL_SHIFT(gpio)); + writel(val, reg->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); + + 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; + + val = readl(®->gplev[BCM2835_GPIO_COMMON_BANK(gpio)]); + + return (val >> BCM2835_GPIO_COMMON_MASK(gpio)) & 0x1; +} + +int gpio_set_value(unsigned gpio, int value) +{ + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; + u32 *output_reg = value ? reg->gpset : reg->gpclr; + + writel(1 << BCM2835_GPIO_COMMON_MASK(gpio), + output_reg[BCM2835_GPIO_COMMON_BANK(gpio)]); + + return 0; +} +
On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
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 +#define BCM2835_NUM_GPIOS 53
For consistency, that might be better as BCM2835_GPIO_COUNT, but not a big deal.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
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) += gpio_bcm2835.o
It looks like the name bcm2835_gpio.c would be more consistent with existing drivers, but not a big deal.
diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c
+inline int gpio_is_valid(unsigned gpio) +{
- return (gpio > BCM2835_NUM_GPIOS) ? 0 : 1;
Presumably gpio==0 is a valid GPIO, so that should be >= not >. It'd be simpler to write it as:
return gpio < BCM2835_NUM_GPIOS;
+int gpio_request(unsigned gpio, const char *label) +{
- return (gpio_is_valid(gpio)) ? 1 : 0;
Why not just return gpio_is_valid_(gpio) directly?
+int gpio_direction_input(unsigned gpio)
- val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
- val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio));
Even if BCM2835_GPIO_OUTPUT==0, it seems better to | it in here for documentation purposes, so add:
val |= (BCM2835_GPIO_INPUT << BCM2835_GPIO_FSEL_SHIFT(gpio));
Otherwise, there's not much point creating the #define BCM2835_GPIO_INPUT.
+int gpio_direction_output(unsigned gpio, int value) +{
- struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
- unsigned val;
- val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
- val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio));
- val |= (BCM2835_GPIO_OUTPUT << BCM2835_GPIO_FSEL_SHIFT(gpio));
- writel(val, reg->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
This (setting the direction) should happen after the following to set the value:
- if (value)
gpio_set_value(gpio, value);
That way, when the GPIO is set to output, the correct value will immediately be driven onto the GPIO, so a glitch may be avoided.
+int gpio_get_value(unsigned gpio)
- return (val >> BCM2835_GPIO_COMMON_MASK(gpio)) & 0x1;
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
+int gpio_set_value(unsigned gpio, int value) +{
- struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
- u32 *output_reg = value ? reg->gpset : reg->gpclr;
- writel(1 << BCM2835_GPIO_COMMON_MASK(gpio),
output_reg[BCM2835_GPIO_COMMON_BANK(gpio)]);
Same comment here.
Hello Stephen,
On 7/15/2012 10:53 PM, Stephen Warren wrote:
On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
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 +#define BCM2835_NUM_GPIOS 53
For consistency, that might be better as BCM2835_GPIO_COUNT, but not a big deal.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
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) += gpio_bcm2835.o
It looks like the name bcm2835_gpio.c would be more consistent with existing drivers, but not a big deal.
diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c
Linux kernel follows this naming, to be exact, it should've been gpio-bcm2835.c. Having a thought in mind that one day the namings would be made consistent with the kernel. That is the reason for this naming, but isn't a big deal to change it.
+inline int gpio_is_valid(unsigned gpio) +{
- return (gpio> BCM2835_NUM_GPIOS) ? 0 : 1;
Presumably gpio==0 is a valid GPIO, so that should be>= not>. It'd be simpler to write it as:
return gpio< BCM2835_NUM_GPIOS;
+int gpio_request(unsigned gpio, const char *label) +{
- return (gpio_is_valid(gpio)) ? 1 : 0;
Why not just return gpio_is_valid_(gpio) directly?
+int gpio_direction_input(unsigned gpio)
- val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
- val&= ~(BCM2835_GPIO_FSEL_MASK<< BCM2835_GPIO_FSEL_SHIFT(gpio));
Even if BCM2835_GPIO_OUTPUT==0, it seems better to | it in here for documentation purposes, so add:
val |= (BCM2835_GPIO_INPUT<< BCM2835_GPIO_FSEL_SHIFT(gpio));
Otherwise, there's not much point creating the #define BCM2835_GPIO_INPUT.
+int gpio_direction_output(unsigned gpio, int value) +{
- struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
- unsigned val;
- val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
- val&= ~(BCM2835_GPIO_FSEL_MASK<< BCM2835_GPIO_FSEL_SHIFT(gpio));
- val |= (BCM2835_GPIO_OUTPUT<< BCM2835_GPIO_FSEL_SHIFT(gpio));
- writel(val, reg->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]);
This (setting the direction) should happen after the following to set the value:
- if (value)
gpio_set_value(gpio, value);That way, when the GPIO is set to output, the correct value will immediately be driven onto the GPIO, so a glitch may be avoided.
+int gpio_get_value(unsigned gpio)
- return (val>> BCM2835_GPIO_COMMON_MASK(gpio))& 0x1;
Agree for all the above. Will get reflected in the v3.
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then it shall be COMMON_SHIFT.
But it doesn't do any shifting like the FSEL_SHIFT, rather it does only masking of bits. So, it makes more sense for me to name it as MASK and not SHIFT.
~Vikram
On 07/31/2012 09:46 AM, Vikram Narayanan wrote:
On 7/15/2012 10:53 PM, Stephen Warren wrote:
On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h b/arch/arm/include/asm/arch-bcm2835/gpio.h
One more comment on the patch subject; it probably should be "gpio: bcm2835:" not "bcm:" since (a) it's in the GPIO directory and (b) the GPIO module is specifically for a BCM2835, and probably doesn't apply to any/all Broadcom devices.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
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) += gpio_bcm2835.o
It looks like the name bcm2835_gpio.c would be more consistent with existing drivers, but not a big deal.
diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c
Linux kernel follows this naming, to be exact, it should've been gpio-bcm2835.c. Having a thought in mind that one day the namings would be made consistent with the kernel. That is the reason for this naming, but isn't a big deal to change it.
Hmmm. It seems better to be internally consistent with U-Boot rather than keeping (onyl part of) U-Boot consistent with the kernel...
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then it shall be COMMON_SHIFT.
But it doesn't do any shifting like the FSEL_SHIFT, rather it does only masking of bits. So, it makes more sense for me to name it as MASK and not SHIFT.
The full quote you're replying to was:
+int gpio_get_value(unsigned gpio)
- return (val >> BCM2835_GPIO_COMMON_MASK(gpio)) & 0x1;
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
... so that macro is being used as a shift not as a mask.
On 7/31/2012 9:22 PM, Stephen Warren wrote:
On 07/31/2012 09:46 AM, Vikram Narayanan wrote:
On 7/15/2012 10:53 PM, Stephen Warren wrote:
On 07/11/2012 02:37 PM, Vikram Narayanan wrote:
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h b/arch/arm/include/asm/arch-bcm2835/gpio.h
One more comment on the patch subject; it probably should be "gpio: bcm2835:" not "bcm:" since (a) it's in the GPIO directory and (b) the GPIO module is specifically for a BCM2835, and probably doesn't apply to any/all Broadcom devices.
Linux kernel follows this naming, to be exact, it should've been gpio-bcm2835.c. Having a thought in mind that one day the namings would be made consistent with the kernel. That is the reason for this naming, but isn't a big deal to change it.
Hmmm. It seems better to be internally consistent with U-Boot rather than keeping (onyl part of) U-Boot consistent with the kernel...
Yes.
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then it shall be COMMON_SHIFT.
But it doesn't do any shifting like the FSEL_SHIFT, rather it does only masking of bits. So, it makes more sense for me to name it as MASK and not SHIFT.
The full quote you're replying to was:
+int gpio_get_value(unsigned gpio)
- return (val>> BCM2835_GPIO_COMMON_MASK(gpio))& 0x1;
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
... so that macro is being used as a shift not as a mask.
Naming isn't really a problem for me. If you want it to be SHIFT, I'd go with it.
Add the driver to the default config
Signed-off-by: Vikram Narayanan vikram186@gmail.com --- include/configs/rpi_b.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/include/configs/rpi_b.h b/include/configs/rpi_b.h index f547027..d4bbccc 100644 --- a/include/configs/rpi_b.h +++ b/include/configs/rpi_b.h @@ -43,7 +43,8 @@ #define CONFIG_SYS_NO_FLASH
/* Devices */ -/* None yet */ +/* GPIO */ +#define CONFIG_BCM2835_GPIO
/* Console UART */ #define CONFIG_PL011_SERIAL
participants (2)
-
Stephen Warren -
Vikram Narayanan