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.

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)

There's no need for brackets around simple numbers.

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

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

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

+#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?)

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

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

+};

diff --git a/drivers/gpio/rpi_gpio.c b/drivers/gpio/rpi_gpio.c

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.

+struct bcm_gpio {

• u32 bank;
• u32 pin;

+};

...

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

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

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

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

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

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

• 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)]);

• gpio &= 0x1f;

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

• return (val & (1 << gpio)) >> gpio;

That would be simpler as:

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

+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
  

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

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

Hello Stephen,

On 6/27/2012 7:09 AM, Stephen Warren wrote:

...

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.

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

On 06/27/2012 08:59 PM, Vikram Narayanan wrote:

On 6/27/2012 11:36 PM, Stephen Warren wrote:

...

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

...

...

...

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

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.

On 06/24/2012 11:22 AM, Vikram Narayanan wrote:

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 swarren@wwwdotorg.org

diff --git a/include/configs/rpi_b.h b/include/configs/rpi_b.h

/* Devices */ -/* None yet */ +/* GPIO */ +#define CONFIG_BCM2835_GPIO

No need to add an extra blank line there.

Hello Stephen,

On 7/4/2012 7:37 AM, Stephen Warren wrote:

On 06/24/2012 11:19 AM, Vikram Narayanan wrote:

...

Add a GPIO driver for the BCM2835 Soc. Refer Datasheet http://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripheral... 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