[U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

Axel Lin

19 Jun 2013 19 Jun '13

3:44 p.m.

Current code looks strange because no matter the value argument is 0 or 1 it always calls writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;

- writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]); + if (value) + writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]); + else + writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

return 0; }

Show replies by date

Otavio Salvador

19 Jun 19 Jun

3:48 p.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

On Wed, Jun 19, 2013 at 10:44 AM, Axel Lin axel.lin@ingics.com wrote:

...

Current code looks strange because no matter the value argument is 0 or 1 it always calls writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
  writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
  if (value)
          writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
  else
          writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

  return 0;
}

writel(value << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

Should do no? This would avoid the if block.

-- Otavio Salvador O.S. Systems http://www.ossystems.com.br http://projetos.ossystems.com.br Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750

Marek Vasut

20 Jun 20 Jun

1:51 a.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

Dear Otavio Salvador,

...

On Wed, Jun 19, 2013 at 10:44 AM, Axel Lin axel.lin@ingics.com wrote:

...
Current code looks strange because no matter the value argument is 0 or 1 it always calls
    writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value)

{
    struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
  writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
  if (value)
          writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
  else
          writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

  return 0;
}
writel(value << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

Should do no? This would avoid the if block.

No, you need clrbits_le32() to unset the GPIO

Best regards, Marek Vasut

Marek Vasut

19 Jun 19 Jun

3:49 p.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

Dear Axel Lin,

...

Current code looks strange because no matter the value argument is 0 or 1 it always calls writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;

writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
if (value)
writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
else
writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

You might want to use clrbits_le32() and setbits_le32() here, no?

...

return 0; }

Best regards, Marek Vasut

Axel Lin

4:01 p.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

2013/6/19 Marek Vasut marex@denx.de:

...

Dear Axel Lin,

...
Current code looks strange because no matter the value argument is 0 or 1 it always calls writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;
writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
if (value)
        writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
else
        writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
You might want to use clrbits_le32() and setbits_le32() here, no?

Honestly, I ask this because I cannot find the detail datasheet for the gpio control of this device. ( Just found the code looks strange).

Usually we can use clrbits_le32() and setbits_le32() helpers to set/clear register bits. But if the only meaningful bit is "1 << gpio", simply use "write 1 << gpio" and "write 0" saves a register read operation.

So maybe Stefan or someone from ST can provide the information about gpio control on this hardware.

Regards, Axel

Stefan Roese

4:40 p.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

On 19.06.2013 16:01, Axel Lin wrote:

...

...
...
writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
if (value)
        writel(1 << gpio, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
else
        writel(0, &regs->gpiodata[DATA_REG_ADDR(gpio)]);
You might want to use clrbits_le32() and setbits_le32() here, no?
Honestly, I ask this because I cannot find the detail datasheet for the gpio control of this device. ( Just found the code looks strange).

Usually we can use clrbits_le32() and setbits_le32() helpers to set/clear register bits. But if the only meaningful bit is "1 << gpio", simply use "write 1 << gpio" and "write 0" saves a register read operation.

So maybe Stefan or someone from ST can provide the information about gpio control on this hardware.

I honestly have no idea how I tested this GPIO driver. That was too long ago. And I don't have the time to dig out the hardware to do some re-testing. But the code definitely looks wrong, so thanks for catching this. Let's wait what the ST engineers have to comment here.

Thanks, Stefan

Vipin Kumar

20 Jun 20 Jun

5:52 a.m.

New subject: [U-Boot] gpio: Question about gpio_set_value() implementation in spear_gpio.c

On 6/19/2013 7:14 PM, Axel Lin wrote:

...

Current code looks strange because no matter the value argument is 0 or 1 it always calls writel(1<< gpio,&regs->gpiodata[DATA_REG_ADDR(gpio)]);

And then gpio_get_value() always return 1.

I'm wondering if it needs to be fixed, something like below change:

diff --git a/drivers/gpio/spear_gpio.c b/drivers/gpio/spear_gpio.c index d3c728e..8878608 100644 --- a/drivers/gpio/spear_gpio.c +++ b/drivers/gpio/spear_gpio.c @@ -52,7 +52,10 @@ int gpio_set_value(unsigned gpio, int value) { struct gpio_regs *regs = (struct gpio_regs *)CONFIG_GPIO_BASE;

writel(1<< gpio,&regs->gpiodata[DATA_REG_ADDR(gpio)]);
if (value)
writel(1<<  gpio,&regs->gpiodata[DATA_REG_ADDR(gpio)]);
else
writel(0,&regs->gpiodata[DATA_REG_ADDR(gpio)]);

Yes, this is the right way. It was a blunder. I am wondering no one ever tried to set a ZERO on any GPIO..

Thanks for pointing out

Regards Vipin

...

return 0; }

4346

Age (days ago)

4347

Last active (days ago)

List overview

Download

6 comments

5 participants

tags (0)

participants (5)

Axel Lin
Marek Vasut
Otavio Salvador
Stefan Roese
Vipin Kumar