GPIO driver for BCM2835 SoC

PATCH Superseded Vikram Narayanan 2 participants 7 messages Jul 11, 2012 – Jul 31, 2012

Subsystems (4)

Arm (32-bit) GPIO Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture

lore

Share & tools

lore

Vikram Narayanan

author

2012-07-11 20:35 UTC #0 lore raw

v2 cover GPIO driver for BCM2835 SoC

Add GPIO driver for BCM2835 Soc. Also add the driver to the raspberrypi default config

Cc Stephen Warren <[email protected]>

Cc Albert Aribaud <[email protected]>

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

Signature · 1 line

1.7.4.1

Vikram Narayanan

author

2012-07-11 20:37 UTC #1 lore raw

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 <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 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

arch/arm/include/asm/arch-bcm2835/gpio.h new file +71 -0

new file mode 100644
index 0000000..3ab06e0
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2012 Vikram Narayananan
+ * <[email protected]>
+ *
+ * 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_ */

drivers/gpio/Makefile +1 -0

index fb3b09a..7653e84 100644
@@ -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)

drivers/gpio/gpio_bcm2835.c new file +87 -0

87 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Vikram Narayanan

author

2012-07-31 15:46 UTC #3 lore raw

PATCH v2 1/2 bcm: Add GPIO driver

Hello Stephen,

Quoted 19 lines · Stephen Warren

arch/arm/include/asm/arch-bcm2835/gpio.h line 28
Quoted 7 lines · Vikram Narayanan
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
arch/arm/include/asm/arch-bcm2835/gpio.h +2 -0
 
+#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.
drivers/gpio/Makefile COBJS-$(CONFIG_TEGRA2_GPIO) += tegra2_gpio.o · line 42
Quoted 6 lines
drivers/gpio/Makefile +1 -0
 
  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

Quoted 5 lines

It looks like the name bcm2835_gpio.c would be more consistent with existing drivers, but not a big deal.
drivers/gpio/gpio_bcm2835.c
Quoted 1 line
drivers/gpio/gpio_bcm2835.c +0 -0

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.

Quoted 46 lines

drivers/gpio/gpio_bcm2835.c
Quoted 3 lines
drivers/gpio/gpio_bcm2835.c +3 -0
+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;
drivers/gpio/gpio_bcm2835.c line 35
Quoted 3 lines
drivers/gpio/gpio_bcm2835.c +3 -0
+int gpio_request(unsigned gpio, const char *label)
+{
+	return (gpio_is_valid(gpio)) ? 1 : 0;
Why not just return gpio_is_valid_(gpio) directly?
drivers/gpio/gpio_bcm2835.c
Quoted 4 lines
drivers/gpio/gpio_bcm2835.c +3 -0
+int gpio_direction_input(unsigned gpio)
 
+	val = readl(&reg->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.
drivers/gpio/gpio_bcm2835.c line 61
Quoted 9 lines
drivers/gpio/gpio_bcm2835.c +9 -0
+int gpio_direction_output(unsigned gpio, int value)
+{
+	struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
+	unsigned val;
+
+	val = readl(&reg->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:
drivers/gpio/gpio_bcm2835.c line 64
Quoted 2 lines
drivers/gpio/gpio_bcm2835.c +2 -0
+	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.

Quoted 4 lines

drivers/gpio/gpio_bcm2835.c
Quoted 3 lines
drivers/gpio/gpio_bcm2835.c +2 -0
+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.

Quoted 1 line

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

Stephen Warren

2012-07-31 15:52 UTC #4 lore raw

PATCH v2 1/2 bcm: Add GPIO driver

Quoted 7 lines · Vikram Narayanan

Quoted 6 lines · Stephen Warren
arch/arm/include/asm/arch-bcm2835/gpio.h
Quoted 5 lines · Vikram Narayanan
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
arch/arm/include/asm/arch-bcm2835/gpio.h +0 -0

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.

Quoted 10 lines

Quoted 10 lines
drivers/gpio/Makefile COBJS-$(CONFIG_TEGRA2_GPIO) += tegra2_gpio.o · line 42
Quoted 6 lines
drivers/gpio/Makefile +1 -0
 
  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.

Quoted 6 lines

Quoted 1 line
drivers/gpio/gpio_bcm2835.c
Quoted 1 line
drivers/gpio/gpio_bcm2835.c +0 -0
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...

Quoted 8 lines

Quoted 1 line
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:

Quoted 5 lines

drivers/gpio/gpio_bcm2835.c line 74
Quoted 3 lines
drivers/gpio/gpio_bcm2835.c new file +2 -0
+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.

Vikram Narayanan

author

2012-07-31 16:09 UTC #5 lore raw

PATCH v2 1/2 bcm: Add GPIO driver

Quoted 15 lines · Stephen Warren

Quoted 7 lines · Vikram Narayanan
Quoted 6 lines · Stephen Warren
arch/arm/include/asm/arch-bcm2835/gpio.h
Quoted 5 lines · Vikram Narayanan
Driver for BCM2835 SoC. This gives the basic functionality of setting/clearing the output.
arch/arm/include/asm/arch-bcm2835/gpio.h +0 -0
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.
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
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.

Quoted 12 lines

Quoted 9 lines
Quoted 2 lines
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:

Quoted 7 lines

Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+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.

Vikram Narayanan

author

2012-07-11 20:38 UTC #6 lore raw

PATCH v2 2/2 rbpi: Add BCM2835 GPIO driver for raspberry pi

In reply to Vikram Narayanan

Add the driver to the default config

Signed-off-by Vikram Narayanan <[email protected]>

---
 include/configs/rpi_b.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

include/configs/rpi_b.h +2 -1

index f547027..d4bbccc 100644
@@ -43,7 +43,8 @@
 #define CONFIG_SYS_NO_FLASH
 
 /* Devices */
-/* None yet */
+/* GPIO */
+#define CONFIG_BCM2835_GPIO
 
 /* Console UART */
 #define CONFIG_PL011_SERIAL

Signature · 1 line

1.7.4.1