Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs

Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs

[Linus Walleij]

On Mon, Aug 5, 2013 at 11:59 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:

> From: Fabian Vogt <fabian@ritter-vogt.de>
>
> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
> It has been successfully tested on a TI nspire CX calculator.
>
> Signed-off-by: Fabian Vogt <fabian@ritter-vogt.de>

Hi Fabian, sorry for taking so long for getting around to review this.

> +++ b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt

I want an ACK from one of the DT bindings maintainers for this
portion of the driver ideally. (It looks all right to me.)

> +config GPIO_ZEVIO
> +       bool "LSI ZEVIO SoC memory mapped GPIOs"
> +       depends on ARCH_NSPIRE

Can't this appear in some other SoC?

It doesn't seem very arch-dependent in itself.

I would rather have this depend on OF so any device-tree enabled
SoC may use it, plus we get compile coverage by allmodconfig.

> diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c
(...)
> +/*
> + * Memory layout:
> + * This chip has four gpio sections, each controls 8 GPIOs.
> + * Bit 0 in section 0 is GPIO 0, bit 2 in section 1 is GPIO 10.
> + * Disclaimer: Reverse engineered!
> + * For more information refer to:
> + *
> http://hackspire.unsads.com/wiki/index.php/Memory-mapped_I/O_ports#90000000_-_General_Purpose_I.2FO_.28GPIO.29

Very ambitious. Impressive!

> + * 0x00-0x3F: Section 0
> + *     +0x00: Masked interrupt status (read-only)
> + *     +0x04: R: Interrupt status W: Reset interrupt status
> + *     +0x08: R: Interrupt mask W: Mask interrupt
> + *     +0x0C: W: Unmask interrupt (write-only)
> + *     +0x10: Direction: I/O=1/0
> + *     +0x14: Output
> + *     +0x18: Input (read-only)
> + *     +0x20: R: Sticky interrupts W: Set sticky interrupt

What is a sticky interrupt? Do you mean it is a level IRQ?
Then it's edge triggered if zero and level triggered if "sticky"
is set to 1, right?

> +/* Functions for struct gpio_chip */
> +static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
> +{
> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
> +
> +       /* Only reading allowed, so no spinlock needed */
> +       uint16_t val = readw(ZEVIO_GPIO(controller, pin, INPUT));

Use just u16 please. uint16_t is some portable C type.

Please replace uint16_t with u16 everywhere.

> +
> +       return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;

Use this construct:
return !!(val & ZEVIO_GPIO_BIT(pin));

> +static void zevio_gpio_set(struct gpio_chip *chip, unsigned pin, int value)
> +{
> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
> +       uint16_t val;
> +
> +       spin_lock(&controller->lock);
> +       val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
> +       if (value)
> +               val |= 1<<ZEVIO_GPIO_BIT(pin);

I usually do this:

#include <linux/bitops.h>

val |= BIT(ZEVIO_GPIO_BIT(pin));

> +       else
> +               val &= ~(1<<ZEVIO_GPIO_BIT(pin));

Dito, sort of...

> +
> +       writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
> +       spin_unlock(&controller->lock);
> +}
> +
> +static int zevio_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
> +{
> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
> +       uint16_t val;
> +
> +       spin_lock(&controller->lock);
> +
> +       val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
> +       val |= 1<<ZEVIO_GPIO_BIT(pin);

Same idea as above.

> +       writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));
> +
> +       spin_unlock(&controller->lock);
> +
> +       return 0;
> +}
> +
> +static int zevio_gpio_direction_output(struct gpio_chip *chip,
> +                                      unsigned pin, int value)
> +{
> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
> +       uint16_t val;
> +
> +       spin_lock(&controller->lock);
> +       val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
> +       if (value)
> +               val |= 1<<ZEVIO_GPIO_BIT(pin);
> +       else
> +               val &= ~(1<<ZEVIO_GPIO_BIT(pin));

And here too.

> +
> +       writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
> +       val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
> +       val &= ~(1<<ZEVIO_GPIO_BIT(pin));
> +       writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));

And here.

> +
> +       spin_unlock(&controller->lock);
> +
> +       return 0;
> +}
> +
> +static int zevio_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
> +{
> +       /* Not implemented due to weird lockups */
> +       return -ENXIO;

Hm. I guess this should be marked TODO: or something.

So when you figure this out you also add an irqchip.

The way this looks I was thinking it could use the
drivers/gpio/gpio-generic.c driver, but maybe not?

Yours,
Linus Walleij

Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs

[Fabian Vogt]

On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>  
wrote:
> On Mon, Aug 5, 2013 at 11:59 PM, Fabian Vogt <fabian@ritter-vogt.de>  
> wrote:
>
>> From: Fabian Vogt <fabian@ritter-vogt.de>
>>
>> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
>> It has been successfully tested on a TI nspire CX calculator.
>>
>> Signed-off-by: Fabian Vogt <fabian@ritter-vogt.de>
>
> Hi Fabian, sorry for taking so long for getting around to review this.
No problem :)

>> +++ b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
>
> I want an ACK from one of the DT bindings maintainers for this
> portion of the driver ideally. (It looks all right to me.)
Any ideas which mail addresses I should add to CC:?

>> +config GPIO_ZEVIO
>> +       bool "LSI ZEVIO SoC memory mapped GPIOs"
>> +       depends on ARCH_NSPIRE
>
> Can't this appear in some other SoC?
The problem is, no documents about this SoC are available at all.
Everything we know about this chip has been found out by disassembling the
nspire software (nucleus PLUS), so I have no idea, but probably not.
Also, it can't be tested during bootup, as the only platform we can test  
it on
boots from an internal read-only flash, which loads boot2,
which itself is exploitable to start linux.
The entire hardware has already been initialized before booting linux.

> It doesn't seem very arch-dependent in itself.
>
> I would rather have this depend on OF so any device-tree enabled
> SoC may use it, plus we get compile coverage by allmodconfig.
>
>> diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c
> (...)
>> +/*
>> + * Memory layout:
>> + * This chip has four gpio sections, each controls 8 GPIOs.
>> + * Bit 0 in section 0 is GPIO 0, bit 2 in section 1 is GPIO 10.
>> + * Disclaimer: Reverse engineered!
>> + * For more information refer to:
>> + *
>> http://hackspire.unsads.com/wiki/index.php/Memory-mapped_I/O_ports#90000000_-_General_Purpose_I.2FO_.28GPIO.29
>
> Very ambitious. Impressive!
Not my work, but indeed, it's very impressive!
(Except for the newer ("complicated") touchpad interface which was indeed  
an I2C controller)

>> + * 0x00-0x3F: Section 0
>> + *     +0x00: Masked interrupt status (read-only)
>> + *     +0x04: R: Interrupt status W: Reset interrupt status
>> + *     +0x08: R: Interrupt mask W: Mask interrupt
>> + *     +0x0C: W: Unmask interrupt (write-only)
>> + *     +0x10: Direction: I/O=1/0
>> + *     +0x14: Output
>> + *     +0x18: Input (read-only)
>> + *     +0x20: R: Sticky interrupts W: Set sticky interrupt
>
> What is a sticky interrupt? Do you mean it is a level IRQ?
> Then it's edge triggered if zero and level triggered if "sticky"
> is set to 1, right?
It's how the GPIO controller signals the VIC.
On sticky it keeps the IRQ high until it has been handled (W to 0x4),
if not sticky, it sets the IRQ line at the same state as the GPIO pin is.
If GPIO high => IRQ high, if GPIO gets low again => IRQ low, which is not  
VERY useful..

>> +/* Functions for struct gpio_chip */
>> +static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
>> +{
>> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
>> +
>> +       /* Only reading allowed, so no spinlock needed */
>> +       uint16_t val = readw(ZEVIO_GPIO(controller, pin, INPUT));
>
> Use just u16 please. uint16_t is some portable C type.
>
> Please replace uint16_t with u16 everywhere.
It's used VERY often and I couldn't find any coding style document which  
prefers u16..

>> +
>> +       return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
>
> Use this construct:
> return !!(val & ZEVIO_GPIO_BIT(pin));
Ok.

>> +static void zevio_gpio_set(struct gpio_chip *chip, unsigned pin, int  
>> value)
>> +{
>> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
>> +       uint16_t val;
>> +
>> +       spin_lock(&controller->lock);
>> +       val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
>> +       if (value)
>> +               val |= 1<<ZEVIO_GPIO_BIT(pin);
>
> I usually do this:
>
> #include <linux/bitops.h>
>
> val |= BIT(ZEVIO_GPIO_BIT(pin));
Oh, it seems I have overseen that..

>> +       else
>> +               val &= ~(1<<ZEVIO_GPIO_BIT(pin));
>
> Dito, sort of...
>
>> +
>> +       writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
>> +       spin_unlock(&controller->lock);
>> +}
>> +
>> +static int zevio_gpio_direction_input(struct gpio_chip *chip, unsigned  
>> pin)
>> +{
>> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
>> +       uint16_t val;
>> +
>> +       spin_lock(&controller->lock);
>> +
>> +       val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
>> +       val |= 1<<ZEVIO_GPIO_BIT(pin);
>
> Same idea as above.
>
>> +       writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));
>> +
>> +       spin_unlock(&controller->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int zevio_gpio_direction_output(struct gpio_chip *chip,
>> +                                      unsigned pin, int value)
>> +{
>> +       struct zevio_gpio *controller = to_zevio_gpio(chip);
>> +       uint16_t val;
>> +
>> +       spin_lock(&controller->lock);
>> +       val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
>> +       if (value)
>> +               val |= 1<<ZEVIO_GPIO_BIT(pin);
>> +       else
>> +               val &= ~(1<<ZEVIO_GPIO_BIT(pin));
>
> And here too.
>
>> +
>> +       writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
>> +       val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
>> +       val &= ~(1<<ZEVIO_GPIO_BIT(pin));
>> +       writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));
>
> And here.
>
>> +
>> +       spin_unlock(&controller->lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int zevio_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>> +{
>> +       /* Not implemented due to weird lockups */
>> +       return -ENXIO;
>
> Hm. I guess this should be marked TODO: or something.
>
> So when you figure this out you also add an irqchip.
>
> The way this looks I was thinking it could use the
> drivers/gpio/gpio-generic.c driver, but maybe not?
Would be great, but it doesn't support multiple registers (4*8 GPIOs),
so I would have to register 4 of them. Then, they had to share one  
interrupt,
which would have to be implemented seperately (or not at all) and then it  
has
to be working with device tree without any extra (struct platform_)data.
I prefer two bitops (pin&7 and pin>>3), which are present anyways (for  
IRQs)
over an array of four gpio_chips.

I'll resubmit the improved version as V4 after you told me which devicetree
mail addresses I should add.

Bye,
Fabian

Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs

[Linus Walleij]

On Fri, Aug 16, 2013 at 6:34 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
> On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>
> wrote:

>> I want an ACK from one of the DT bindings maintainers for this
>> portion of the driver ideally. (It looks all right to me.)
>
> Any ideas which mail addresses I should add to CC:?

No. devicetree@vger.kernel.org is the place.

But if nothing happens you might want to ask Stephen Warren.

>>> +config GPIO_ZEVIO
>>> +       bool "LSI ZEVIO SoC memory mapped GPIOs"
>>> +       depends on ARCH_NSPIRE
>>
>>
>> Can't this appear in some other SoC?
>
> The problem is, no documents about this SoC are available at all.
> Everything we know about this chip has been found out by disassembling the
> nspire software (nucleus PLUS), so I have no idea, but probably not.
> Also, it can't be tested during bootup, as the only platform we can test it
> on
> boots from an internal read-only flash, which loads boot2,
> which itself is exploitable to start linux.
> The entire hardware has already been initialized before booting linux.

The thing is that if we don't put in the dependency, we can get
some nice compile coverage on a few different compilers and
platforms.

>>> + * 0x00-0x3F: Section 0
>>> + *     +0x00: Masked interrupt status (read-only)
>>> + *     +0x04: R: Interrupt status W: Reset interrupt status
>>> + *     +0x08: R: Interrupt mask W: Mask interrupt
>>> + *     +0x0C: W: Unmask interrupt (write-only)
>>> + *     +0x10: Direction: I/O=1/0
>>> + *     +0x14: Output
>>> + *     +0x18: Input (read-only)
>>> + *     +0x20: R: Sticky interrupts W: Set sticky interrupt
>>
>>
>> What is a sticky interrupt? Do you mean it is a level IRQ?
>> Then it's edge triggered if zero and level triggered if "sticky"
>> is set to 1, right?
>
> It's how the GPIO controller signals the VIC.
> On sticky it keeps the IRQ high until it has been handled (W to 0x4),

This is what is called a level interrupt.

So please update the terminology to match the common
one.

> if not sticky, it sets the IRQ line at the same state as the GPIO pin is.
> If GPIO high => IRQ high, if GPIO gets low again => IRQ low, which is not
> VERY useful..

Hm doesn't seem like an interrupt at all, rather some
test operation to test the IRQ line. Well whatever...

>> Use just u16 please. uint16_t is some portable C type.
>>
>> Please replace uint16_t with u16 everywhere.
>
> It's used VERY often and I couldn't find any coding style document which
> prefers u16..

There is no consensus around this.
It is up to the subsystem maintainer to decide this I think.


>>> +static int zevio_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>>> +{
>>> +       /* Not implemented due to weird lockups */
>>> +       return -ENXIO;
>>
>>
>> Hm. I guess this should be marked TODO: or something.
>>
>> So when you figure this out you also add an irqchip.
>>
>> The way this looks I was thinking it could use the
>> drivers/gpio/gpio-generic.c driver, but maybe not?
>
> Would be great, but it doesn't support multiple registers (4*8 GPIOs),
> so I would have to register 4 of them. Then, they had to share one
> interrupt,
> which would have to be implemented seperately (or not at all) and then it
> has
> to be working with device tree without any extra (struct platform_)data.
> I prefer two bitops (pin&7 and pin>>3), which are present anyways (for IRQs)
> over an array of four gpio_chips.

OK I buy this, but you know you can reuse only part of the
generic MMIO driver, and do not have to use all of it?

> I'll resubmit the improved version as V4 after you told me which devicetree
> mail addresses I should add.

OK thanks.

Yours,
Linus Walleij

Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs

[Stephen Warren]

On 08/23/2013 08:09 AM, Linus Walleij wrote:
> On Fri, Aug 16, 2013 at 6:34 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
>> On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>
>> wrote:
> 
>>> I want an ACK from one of the DT bindings maintainers for this
>>> portion of the driver ideally. (It looks all right to me.)
>>
>> Any ideas which mail addresses I should add to CC:?
> 
> No. devicetree@vger.kernel.org is the place.
> 
> But if nothing happens you might want to ask Stephen Warren.

The full list of CC's is:

OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
M:      Rob Herring <rob.herring@calxeda.com>
M:      Pawel Moll <pawel.moll@arm.com>
M:      Mark Rutland <mark.rutland@arm.com>
M:      Stephen Warren <swarren@wwwdotorg.org>
M:      Ian Campbell <ian.campbell@citrix.com>
L:      devicetree@vger.kernel.org