Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
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 0x7E20
+#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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
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_BASE0x7E20
> +#define BCM2835_NUM_GPIOS53
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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
Driver for BCM2835 SoC. This gives the basic functionality of
setting/clearing the output.
Signed-off-by: Vikram Narayanan
Cc: Stephen Warren
Cc: Albert Aribaud
---
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 000..3ab06e0
--- /dev/null
+++ b/arch/arm/include/asm/arch-bcm2835/gpio.h
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2012 Vikram Narayananan
+ *
+ *
+ * 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 0x7E20
+#define BCM2835_NUM_GPIOS 53
+
+#define BCM2835_GPIO_FSEL_MASK 0x7
+#define BCM2835_GPIO_INPUT 0x0
+#define BCM2835_GPIO_OUTPUT0x1
+#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 000..23cdd90
--- /dev/null
+++ b/drivers/gpio/gpio_bcm2835.c
@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2012 Vikram Narayananan
+ *
+ *
+ * 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
+#include
+#include
+
+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 &= ~(BCM283
