[U-Boot] [PATCH v2 0/2] GPIO driver for BCM2835 SoC

[U-Boot] [PATCH v2 0/2] GPIO driver for BCM2835 SoC

[Vikram Narayanan]

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

-- 
1.7.4.1

[U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan]

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(&reg->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(&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)]);
+
+	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(&reg->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;
+}
+
-- 
1.7.4.1

[U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Stephen Warren]

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(&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.

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

> +	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] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan]

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(&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.
>
>> +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:
>
>> +	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] [PATCH v2 1/2] bcm: Add GPIO driver

[Stephen Warren]

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] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan]

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] [PATCH v2 2/2] rbpi: Add BCM2835 GPIO driver for raspberry pi

[Vikram Narayanan]

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