Skip to contentSkip to conversation

Add GPIO driver for BCM2835 SoC

PATCH New Vikram Narayanan 2 participants 11 messages Jun 24, 2012 – Jul 4, 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
#0 lore raw

cover Add GPIO driver for BCM2835 SoC

Add a GPIO driver for the BCM2835 Soc.
Refer Datasheet http://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
Also, add the driver to the raspberrypi's default config

Vikram Narayanan (2):
bcm: Add GPIO driver for BCM2835 SoC
rbpi: Add BCM2835 GPIO driver for raspberry pi

 arch/arm/include/asm/arch-bcm2835/gpio.h |   49 +++++++++++
 drivers/gpio/Makefile                    |    1 +
 drivers/gpio/rpi_gpio.c                  |  128 ++++++++++++++++++++++++++++++
 include/configs/rpi_b.h                  |    4 +-
 4 files changed, 181 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-bcm2835/gpio.h
 create mode 100644 drivers/gpio/rpi_gpio.c
Signature · 1 line
1.7.4.1
author
#1 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

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 |   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
arch/arm/include/asm/arch-bcm2835/gpio.h new file +49 -0
new file mode 100644
index 0000000..3cba0fb
@@ -0,0 +1,49 @@
+/*
+ * 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_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_ */
drivers/gpio/Makefile +1 -0
index fb3b09a..b042c46 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)	+= rpi_gpio.o
 
 COBJS	:= $(COBJS-y)
 SRCS 	:= $(COBJS:.o=.c)
drivers/gpio/rpi_gpio.c new file +128 -0

128 lines not on this page · raw message

Signature · 1 line
1.7.4.1
author
#7 lore raw

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

In reply to Vikram Narayanan

Add the driver to the rpi_b's default config

Signed-off-by Vikram Narayanan <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Albert Aribaud <[email protected]>
---
 include/configs/rpi_b.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
include/configs/rpi_b.h +3 -1
index f547027..fb45a18 100644
@@ -43,7 +43,9 @@
 #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
#2 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

In reply to Vikram Narayanan

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.

arch/arm/include/asm/arch-bcm2835/gpio.h

Quoted 3 lines
arch/arm/include/asm/arch-bcm2835/gpio.h +1 -0
 
+#define BCM2835_GPIO_BASE	(0x7E200000)

There's no need for brackets around simple numbers.

Quoted 1 line

+#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.

Quoted 1 line

+#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.

Quoted 1 line

+#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.

arch/arm/include/asm/arch-bcm2835/gpio.h line 34

Quoted 2 lines
arch/arm/include/asm/arch-bcm2835/gpio.h +2 -0
+#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?)

arch/arm/include/asm/arch-bcm2835/gpio.h line 40

Quoted 6 lines
arch/arm/include/asm/arch-bcm2835/gpio.h +5 -0
+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.

arch/arm/include/asm/arch-bcm2835/gpio.h line 46

Quoted 6 lines
arch/arm/include/asm/arch-bcm2835/gpio.h +6 -0
+	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.

drivers/gpio/rpi_gpio.c

Quoted 3 lines
arch/arm/include/asm/arch-bcm2835/gpio.h +1 -0
+};
drivers/gpio/rpi_gpio.c +0 -0

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.

Quoted 4 lines
drivers/gpio/rpi_gpio.c +4 -0
+struct bcm_gpio {
+	u32 bank;
+	u32 pin;
+};

...

drivers/gpio/rpi_gpio.c line 46

Quoted 9 lines
drivers/gpio/rpi_gpio.c +9 -0
+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.

drivers/gpio/rpi_gpio.c line 50

Quoted 4 lines
drivers/gpio/rpi_gpio.c +4 -0
+	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.

drivers/gpio/rpi_gpio.c line 60

Quoted 9 lines
drivers/gpio/rpi_gpio.c +9 -0
+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.

drivers/gpio/rpi_gpio.c line 66

Quoted 5 lines
drivers/gpio/rpi_gpio.c +5 -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;

There's no need to initialize val; it's unconditionally written to below before it's used.

drivers/gpio/rpi_gpio.c line 69

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

drivers/gpio/rpi_gpio.c

Quoted 2 lines
drivers/gpio/rpi_gpio.c +2 -0
+	val = readl(&reg->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.

drivers/gpio/rpi_gpio.c line 102

Quoted 11 lines
drivers/gpio/rpi_gpio.c +10 -0
+	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(&reg->gplev0) : readl(&reg->gplev1);

How about:

val = readl(&reg->gplev[BCM2835_GPIO_OTHER_BANK(gpio)]);

drivers/gpio/rpi_gpio.c

Quoted 1 line
drivers/gpio/rpi_gpio.c +1 -0
+	gpio &= 0x1f;

That's not needed, since we extract only the bits we care about below anyway.

drivers/gpio/rpi_gpio.c line 105

Quoted 1 line
drivers/gpio/rpi_gpio.c +1 -0
+	return (val & (1 << gpio)) >> gpio;

That would be simpler as:

return (val >> BCM2835_GPIO_OTHER_SHIFT(gpio)) & 1;

Quoted 11 lines
drivers/gpio/rpi_gpio.c +11 -0
+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

drivers/gpio/rpi_gpio.c line 128

Quoted 10 lines
drivers/gpio/rpi_gpio.c +10 -0
+			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.

#8 lore raw

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

In reply to Vikram Narayanan
Quoted 2 lines · Vikram Narayanan

Add the driver to the rpi_b's default config

It looks like there's a blank line before the patch description there.

Aside from that and the nit below,

Acked-by Stephen Warren <[email protected]>

include/configs/rpi_b.h line 48

Quoted 8 lines
include/configs/rpi_b.h +3 -1
 
 /* Devices */
-/* None yet */
+/* GPIO */
+#define CONFIG_BCM2835_GPIO
+

No need to add an extra blank line there.

author
#3 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

In reply to Stephen Warren

Hello Stephen,

Quoted 4 lines · Stephen Warren

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.

Quoted 2 lines

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

#4 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

Quoted 11 lines · Vikram Narayanan

Hello Stephen,

Quoted 4 lines · Stephen Warren

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.

Quoted 4 lines

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

author
#5 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

Quoted 13 lines · Stephen Warren
Quoted 11 lines · Vikram Narayanan

Hello Stephen,

Quoted 4 lines · Stephen Warren

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.

Quoted 6 lines

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.

Quoted 12 lines
Quoted 4 lines

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:

Quoted 5 lines

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.

#6 lore raw

PATCH 1/2 bcm: Add GPIO driver for BCM2835 SoC

Quoted 2 lines · Vikram Narayanan
Quoted 1 line · Stephen Warren

On 06/27/2012 11:32 AM, Vikram Narayanan wrote:

...

Quoted 15 lines
Quoted 15 lines
Quoted 4 lines

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

Quoted 8 lines
Quoted 2 lines

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.

#9 lore raw

cover Add GPIO driver for BCM2835 SoC

In reply to Vikram Narayanan
Quoted 3 lines · Vikram Narayanan

Add a GPIO driver for the BCM2835 Soc.
Refer Datasheet http://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
Also, add the driver to the raspberrypi's default config

Vikram,

Are you planning on revising this driver and re-posting? I was hoping to enhance it to add support for the pin muxing functionality of the HW, but obviously want to wait for any revised version before doing so.

Thanks.

author
#10 lore raw

cover Add GPIO driver for BCM2835 SoC

Hello Stephen,

Quoted 10 lines · Stephen Warren
Quoted 3 lines · Vikram Narayanan

Add a GPIO driver for the BCM2835 Soc.
Refer Datasheet http://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
Also, add the driver to the raspberrypi's default config

Vikram,

Are you planning on revising this driver and re-posting? I was hoping to enhance it to add support for the pin muxing functionality of the HW, but obviously want to wait for any revised version before doing so.

Yes. I'm on it. I'm quite busy on other tasks.
I'll post the v2 by the end of this week.
Hope this should be ok for you.

Thanks,
Vikram