[[RFC] ] gpio: gpio-mxc: make sure gpio is input when request IRQ

Commit Message

Markus Niebel July 16, 2014, 1:51 p.m. UTC

From: Markus Niebel <[e-mail]>

When requesting an GPIO irq for imx Soc two things are missing:
- there is no check, if the GPIO is already requested
- there is no check, if the GPIO is configured as input

The first case can lead to reconfiguring the GPIO pin from third
party while it is used as IRQ pin, second case will (eventually)
prevent IRQ from being seen by SOC becaus the pin is driven by
Soc

This patch tries to implement (logic taken roughly from gpio-omap)
- basic check if gpio already requested
- if needed requests the gpio and configures as IN.
- if gpio is already requested it is only verified if pin is IN
- gpio is locked as irq

Tested on a not mainlined i.MX6 based hardware with pin configured
by bootloader as OUT HIGH and expecting a low active IRQ.

Signed-off-by: Markus Niebel <[e-mail]>
---
 drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Shawn Guo July 22, 2014, 6:28 a.m. UTC | #1

On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:
> From: Markus Niebel <[e-mail]>
> 
> When requesting an GPIO irq for imx Soc two things are missing:
> - there is no check, if the GPIO is already requested
> - there is no check, if the GPIO is configured as input
> 
> The first case can lead to reconfiguring the GPIO pin from third
> party while it is used as IRQ pin, second case will (eventually)
> prevent IRQ from being seen by SOC becaus the pin is driven by
> Soc
> 
> This patch tries to implement (logic taken roughly from gpio-omap)
> - basic check if gpio already requested
> - if needed requests the gpio and configures as IN.
> - if gpio is already requested it is only verified if pin is IN
> - gpio is locked as irq
> 
> Tested on a not mainlined i.MX6 based hardware with pin configured
> by bootloader as OUT HIGH and expecting a low active IRQ.
> 
> Signed-off-by: Markus Niebel <[e-mail]>
> ---
>  drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index db83b3c..4316a38 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  	u32 gpio = port->bgc.gc.base + gpio_idx;
>  	int edge;
>  	void __iomem *reg = port->base;
> +	int ret = 0;
> +
> +	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
> +		char label[32];
> +
> +		snprintf(label, 32, "gpio%u-irq", gpio);
> +		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);

I'm not sure it's correct to call gpio_request_one() from .set_irq_type
hook.  It looks like a API usage violation to me.  It should really be
called from client driver.

> +	} else {
> +		val = readl(port->base + GPIO_GDIR);
> +		if (val & BIT(gpio_idx))
> +			ret = -EINVAL;

It says that the GPIO is requested by someone, but we're not really sure
if it's the correct one, i.e. the one is requesting set_irq_type.

> +	}
> +
> +	if (ret) {
> +		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
> +			gpio_idx);
> +		return ret;
> +	}

Having said that, I'm not sure any above changes is really necessary.
If any, I would say only gpiochip_is_requested() check makes some sense,
but we should just fail out if the GPIO hasn't been requested.  Nothing
more can be done in there.

> +
> +	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
> +	if (ret) {
> +		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
> +			gpio_idx);
> +		return ret;
> +	}

This and the following changes do make sense to me.

Shawn

>  
>  	port->both_edges &= ~(1 << gpio_idx);
>  	switch (type) {
> @@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  	return 0;
>  }
>  
> +static void gpio_irq_shutdown(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct mxc_gpio_port *port = gc->private;
> +	u32 gpio_idx = d->hwirq;
> +
> +	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
> +}
> +
>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  {
>  	void __iomem *reg = port->base;
> @@ -353,6 +387,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
>  	ct->chip.irq_mask = irq_gc_mask_clr_bit;
>  	ct->chip.irq_unmask = irq_gc_mask_set_bit;
>  	ct->chip.irq_set_type = gpio_set_irq_type;
> +	ct->chip.irq_shutdown = gpio_irq_shutdown;
>  	ct->chip.irq_set_wake = gpio_set_wake_irq;
>  	ct->regs.ack = GPIO_ISR;
>  	ct->regs.mask = GPIO_IMR;
> -- 
> 1.7.9.5
>

Markus Niebel July 22, 2014, 7:19 a.m. UTC | #2

Am 22.07.2014 08:28, wrote Shawn Guo:
> On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:
>> From: Markus Niebel <[e-mail]>
>>
>> When requesting an GPIO irq for imx Soc two things are missing:
>> - there is no check, if the GPIO is already requested
>> - there is no check, if the GPIO is configured as input
>>
>> The first case can lead to reconfiguring the GPIO pin from third
>> party while it is used as IRQ pin, second case will (eventually)
>> prevent IRQ from being seen by SOC becaus the pin is driven by
>> Soc
>>
>> This patch tries to implement (logic taken roughly from gpio-omap)
>> - basic check if gpio already requested
>> - if needed requests the gpio and configures as IN.
>> - if gpio is already requested it is only verified if pin is IN
>> - gpio is locked as irq
>>
>> Tested on a not mainlined i.MX6 based hardware with pin configured
>> by bootloader as OUT HIGH and expecting a low active IRQ.
>>
>> Signed-off-by: Markus Niebel <[e-mail]>
>> ---
>>  drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
>> index db83b3c..4316a38 100644
>> --- a/drivers/gpio/gpio-mxc.c
>> +++ b/drivers/gpio/gpio-mxc.c
>> @@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>>  	u32 gpio = port->bgc.gc.base + gpio_idx;
>>  	int edge;
>>  	void __iomem *reg = port->base;
>> +	int ret = 0;
>> +
>> +	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
>> +		char label[32];
>> +
>> +		snprintf(label, 32, "gpio%u-irq", gpio);
>> +		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);
> 
> I'm not sure it's correct to call gpio_request_one() from .set_irq_type
> hook.  It looks like a API usage violation to me.  It should really be
> called from client driver.

Thats why it is an RFC. I add Linus Walleij to the cc-list.

Let me describe the problem:

Currently client drivers have simply interrupts and interrupt-parent
in their bindings, but no interrupt-gpios. Therefore in this case a
client does not know about a dedicated gpio which is to be requested
and configured.

> 
>> +	} else {
>> +		val = readl(port->base + GPIO_GDIR);
>> +		if (val & BIT(gpio_idx))
>> +			ret = -EINVAL;
> 
> It says that the GPIO is requested by someone, but we're not really sure
> if it's the correct one, i.e. the one is requesting set_irq_type.
> 

Yes, but the current situation is even worse (in my eyes): an IRQ can be requested and an
independend party can request and configure the gpio as output ...

>> +	}
>> +
>> +	if (ret) {
>> +		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
>> +			gpio_idx);
>> +		return ret;
>> +	}
> 
> Having said that, I'm not sure any above changes is really necessary.
> If any, I would say only gpiochip_is_requested() check makes some sense,
> but we should just fail out if the GPIO hasn't been requested.  Nothing
> more can be done in there.

Going that way as a consequence a reworked device tree binding for gpio irq is needed
(just like in the platform data days) when providing a gpio number and the client has
to request gpio and irq - correct me if I'm wrong.

Maybe here is something needed with deeper knowledge in the gpio subsystem.

> 
>> +
>> +	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
>> +	if (ret) {
>> +		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
>> +			gpio_idx);
>> +		return ret;
>> +	}
> 
> This and the following changes do make sense to me.
> 
> Shawn
> 
>>  
>>  	port->both_edges &= ~(1 << gpio_idx);
>>  	switch (type) {
>> @@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>>  	return 0;
>>  }
>>  
>> +static void gpio_irq_shutdown(struct irq_data *d)
>> +{
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct mxc_gpio_port *port = gc->private;
>> +	u32 gpio_idx = d->hwirq;
>> +
>> +	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
>> +}
>> +
>>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>>  {
>>  	void __iomem *reg = port->base;
>> @@ -353,6 +387,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
>>  	ct->chip.irq_mask = irq_gc_mask_clr_bit;
>>  	ct->chip.irq_unmask = irq_gc_mask_set_bit;
>>  	ct->chip.irq_set_type = gpio_set_irq_type;
>> +	ct->chip.irq_shutdown = gpio_irq_shutdown;
>>  	ct->chip.irq_set_wake = gpio_set_wake_irq;
>>  	ct->regs.ack = GPIO_ISR;
>>  	ct->regs.mask = GPIO_IMR;
>> -- 
>> 1.7.9.5
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Shawn Guo July 22, 2014, 7:42 a.m. UTC | #3

+ Grant and devicetree list

On Tue, Jul 22, 2014 at 09:19:22AM +0200, Markus Niebel wrote:
> Am 22.07.2014 08:28, wrote Shawn Guo:
> > On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:
> >> From: Markus Niebel <[e-mail]>
> >>
> >> When requesting an GPIO irq for imx Soc two things are missing:
> >> - there is no check, if the GPIO is already requested
> >> - there is no check, if the GPIO is configured as input
> >>
> >> The first case can lead to reconfiguring the GPIO pin from third
> >> party while it is used as IRQ pin, second case will (eventually)
> >> prevent IRQ from being seen by SOC becaus the pin is driven by
> >> Soc
> >>
> >> This patch tries to implement (logic taken roughly from gpio-omap)
> >> - basic check if gpio already requested
> >> - if needed requests the gpio and configures as IN.
> >> - if gpio is already requested it is only verified if pin is IN
> >> - gpio is locked as irq
> >>
> >> Tested on a not mainlined i.MX6 based hardware with pin configured
> >> by bootloader as OUT HIGH and expecting a low active IRQ.
> >>
> >> Signed-off-by: Markus Niebel <[e-mail]>
> >> ---
> >>  drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 35 insertions(+)
> >>
> >> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> >> index db83b3c..4316a38 100644
> >> --- a/drivers/gpio/gpio-mxc.c
> >> +++ b/drivers/gpio/gpio-mxc.c
> >> @@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
> >>  	u32 gpio = port->bgc.gc.base + gpio_idx;
> >>  	int edge;
> >>  	void __iomem *reg = port->base;
> >> +	int ret = 0;
> >> +
> >> +	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
> >> +		char label[32];
> >> +
> >> +		snprintf(label, 32, "gpio%u-irq", gpio);
> >> +		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);
> > 
> > I'm not sure it's correct to call gpio_request_one() from .set_irq_type
> > hook.  It looks like a API usage violation to me.  It should really be
> > called from client driver.
> 
> Thats why it is an RFC. I add Linus Walleij to the cc-list.
> 
> Let me describe the problem:
> 
> Currently client drivers have simply interrupts and interrupt-parent
> in their bindings, but no interrupt-gpios. Therefore in this case a
> client does not know about a dedicated gpio which is to be requested
> and configured.

Okay.  I understand your problem now.  This is an issue in case we
specify a GPIO to be used as interrupt from device tree.  In non-DT
world, client driver knows this is an interrupt on GPIO, and therefore
takes the responsibility to request and configure the GPIO to work as
interrupt.  But in DT case, client driver does not know whether it's an
IRQ line or GPIO based interrupt.  The consequence is that GPIO
subsystem, OF subsystem, client driver, none of the three is requesting
and configuring the GPIO to be used as interrupt.

This is a common issue instead of i.MX specific, and should be addressed
globally, I think.

Shawn

> 
> > 
> >> +	} else {
> >> +		val = readl(port->base + GPIO_GDIR);
> >> +		if (val & BIT(gpio_idx))
> >> +			ret = -EINVAL;
> > 
> > It says that the GPIO is requested by someone, but we're not really sure
> > if it's the correct one, i.e. the one is requesting set_irq_type.
> > 
> 
> Yes, but the current situation is even worse (in my eyes): an IRQ can be requested and an
> independend party can request and configure the gpio as output ...
> 
> >> +	}
> >> +
> >> +	if (ret) {
> >> +		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
> >> +			gpio_idx);
> >> +		return ret;
> >> +	}
> > 
> > Having said that, I'm not sure any above changes is really necessary.
> > If any, I would say only gpiochip_is_requested() check makes some sense,
> > but we should just fail out if the GPIO hasn't been requested.  Nothing
> > more can be done in there.
> 
> Going that way as a consequence a reworked device tree binding for gpio irq is needed
> (just like in the platform data days) when providing a gpio number and the client has
> to request gpio and irq - correct me if I'm wrong.
> 
> Maybe here is something needed with deeper knowledge in the gpio subsystem.
> 
> > 
> >> +
> >> +	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
> >> +	if (ret) {
> >> +		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
> >> +			gpio_idx);
> >> +		return ret;
> >> +	}
> > 
> > This and the following changes do make sense to me.
> > 
> > Shawn
> > 
> >>  
> >>  	port->both_edges &= ~(1 << gpio_idx);
> >>  	switch (type) {
> >> @@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
> >>  	return 0;
> >>  }
> >>  
> >> +static void gpio_irq_shutdown(struct irq_data *d)
> >> +{
> >> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> >> +	struct mxc_gpio_port *port = gc->private;
> >> +	u32 gpio_idx = d->hwirq;
> >> +
> >> +	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
> >> +}
> >> +
> >>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
> >>  {
> >>  	void __iomem *reg = port->base;
> >> @@ -353,6 +387,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
> >>  	ct->chip.irq_mask = irq_gc_mask_clr_bit;
> >>  	ct->chip.irq_unmask = irq_gc_mask_set_bit;
> >>  	ct->chip.irq_set_type = gpio_set_irq_type;
> >> +	ct->chip.irq_shutdown = gpio_irq_shutdown;
> >>  	ct->chip.irq_set_wake = gpio_set_wake_irq;
> >>  	ct->regs.ack = GPIO_ISR;
> >>  	ct->regs.mask = GPIO_IMR;
> >> -- 
> >> 1.7.9.5
> >>
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
>

Linus Walleij July 23, 2014, 4:10 p.m. UTC | #4

On Wed, Jul 16, 2014 at 3:51 PM, Markus Niebel
<[e-mail]> wrote:

> From: Markus Niebel <[e-mail]>
>
> When requesting an GPIO irq for imx Soc two things are missing:
> - there is no check, if the GPIO is already requested
> - there is no check, if the GPIO is configured as input
>
> The first case can lead to reconfiguring the GPIO pin from third
> party while it is used as IRQ pin, second case will (eventually)
> prevent IRQ from being seen by SOC becaus the pin is driven by
> Soc
>
> This patch tries to implement (logic taken roughly from gpio-omap)
> - basic check if gpio already requested
> - if needed requests the gpio and configures as IN.
> - if gpio is already requested it is only verified if pin is IN
> - gpio is locked as irq
>
> Tested on a not mainlined i.MX6 based hardware with pin configured
> by bootloader as OUT HIGH and expecting a low active IRQ.
>
> Signed-off-by: Markus Niebel <[e-mail]>
> ---
>  drivers/gpio/gpio-mxc.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index db83b3c..4316a38 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -175,6 +175,31 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>         u32 gpio = port->bgc.gc.base + gpio_idx;
>         int edge;
>         void __iomem *reg = port->base;
> +       int ret = 0;
> +
> +       if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
> +               char label[32];
> +
> +               snprintf(label, 32, "gpio%u-irq", gpio);
> +               ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);

Wut? Auto-request here? This can not be right.

> +       } else {
> +               val = readl(port->base + GPIO_GDIR);
> +               if (val & BIT(gpio_idx))
> +                       ret = -EINVAL;
> +       }
> +
> +       if (ret) {
> +               dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
> +                       gpio_idx);
> +               return ret;
> +       }
> +
> +       ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);

This call should be done in the new
.irq_request_resources callback in struct irq_chip.

> +       if (ret) {
> +               dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
> +                       gpio_idx);
> +               return ret;
> +       }
>
>         port->both_edges &= ~(1 << gpio_idx);
>         switch (type) {
> @@ -231,6 +256,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>         return 0;
>  }
>
> +static void gpio_irq_shutdown(struct irq_data *d)
> +{
> +       struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +       struct mxc_gpio_port *port = gc->private;
> +       u32 gpio_idx = d->hwirq;
> +
> +       gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
> +}

This call should be done in the new
.irq_release_resources callback in struct irq_chip.

Looking at the driver, it should be switched to using the new
irqchip helpers inside gpiolib, see e.g.
commit 7bcbce55f20e41c014df4d5d9c8448f7b5e49d79
"gpio: pca953x: use gpiolib irqchip helpers"

Yours,
Linus Walleij

Linus Walleij July 23, 2014, 4:14 p.m. UTC | #5

On Tue, Jul 22, 2014 at 9:42 AM, Shawn Guo <[e-mail]> wrote:
> On Tue, Jul 22, 2014 at 09:19:22AM +0200, Markus Niebel wrote:

>> Currently client drivers have simply interrupts and interrupt-parent
>> in their bindings, but no interrupt-gpios. Therefore in this case a
>> client does not know about a dedicated gpio which is to be requested
>> and configured.
>
> Okay.  I understand your problem now.  This is an issue in case we
> specify a GPIO to be used as interrupt from device tree.  In non-DT
> world, client driver knows this is an interrupt on GPIO, and therefore
> takes the responsibility to request and configure the GPIO to work as
> interrupt.  But in DT case, client driver does not know whether it's an
> IRQ line or GPIO based interrupt.  The consequence is that GPIO
> subsystem, OF subsystem, client driver, none of the three is requesting
> and configuring the GPIO to be used as interrupt.
>
> This is a common issue instead of i.MX specific, and should be addressed
> globally, I think.

This is very well documented in Documentation/gpio/driver.txt these
days. Quoting:

"
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

So always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
been called first.

This orthogonality leads to ambiguities that we need to solve: if there is
competition inside the subsystem which side is using the resource (a certain
GPIO line and register for example) it needs to deny certain operations and
keep track of usage inside of the gpiolib subsystem. This is why the API
below exists.


Locking IRQ usage
-----------------
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
to mark the GPIO as being used as an IRQ:

        int gpiod_lock_as_irq(struct gpio_desc *desc)

This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
is released:

        void gpiod_unlock_as_irq(struct gpio_desc *desc)

When implementing an irqchip inside a GPIO driver, these two functions should
typically be called in the .startup() and .shutdown() callbacks from the
irqchip.
"

So: make sure each API can be used orthogonally by just poking
into the hardware registers. Do not call gpio_request_one().

Yours,
Linus Walleij

Markus Niebel July 24, 2014, 8:12 a.m. UTC | #6

Am 23.07.2014 18:14, wrote Linus Walleij:
> On Tue, Jul 22, 2014 at 9:42 AM, Shawn Guo <[e-mail]> wrote:
>> On Tue, Jul 22, 2014 at 09:19:22AM +0200, Markus Niebel wrote:
> 
>>> Currently client drivers have simply interrupts and interrupt-parent
>>> in their bindings, but no interrupt-gpios. Therefore in this case a
>>> client does not know about a dedicated gpio which is to be requested
>>> and configured.
>>
>> Okay.  I understand your problem now.  This is an issue in case we
>> specify a GPIO to be used as interrupt from device tree.  In non-DT
>> world, client driver knows this is an interrupt on GPIO, and therefore
>> takes the responsibility to request and configure the GPIO to work as
>> interrupt.  But in DT case, client driver does not know whether it's an
>> IRQ line or GPIO based interrupt.  The consequence is that GPIO
>> subsystem, OF subsystem, client driver, none of the three is requesting
>> and configuring the GPIO to be used as interrupt.
>>
>> This is a common issue instead of i.MX specific, and should be addressed
>> globally, I think.
> 
> This is very well documented in Documentation/gpio/driver.txt these
> days. Quoting:
> 

Sorry for not carefully reading the docs and many thanks for the helpful comments.

> "
> It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
> if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> irq_chip are orthogonal, and offering their services independent of each
> other.
> 
> gpiod_to_irq() is just a convenience function to figure out the IRQ for a
> certain GPIO line and should not be relied upon to have been called before
> the IRQ is used.
> 
> So always prepare the hardware and make it ready for action in respective
> callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
> been called first.
> 

So a gpio driver is responsible to read status of gpio lines and flag any gpio line 
currently configured as out (base on the information read from hardware registers) 
on driver probe time - correct? If yes is the driver allowed to call 
gpiod_get_direction() to have the FLAG_IS_OUT set in the gpiolib layer?

> This orthogonality leads to ambiguities that we need to solve: if there is
> competition inside the subsystem which side is using the resource (a certain
> GPIO line and register for example) it needs to deny certain operations and
> keep track of usage inside of the gpiolib subsystem. This is why the API
> below exists.
> 
> 
> Locking IRQ usage
> -----------------
> Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
> to mark the GPIO as being used as an IRQ:
> 
>         int gpiod_lock_as_irq(struct gpio_desc *desc)
> 
> This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
> is released:
> 
>         void gpiod_unlock_as_irq(struct gpio_desc *desc)
> 
> When implementing an irqchip inside a GPIO driver, these two functions should
> typically be called in the .startup() and .shutdown() callbacks from the
> irqchip.
> "
> 
> So: make sure each API can be used orthogonally by just poking
> into the hardware registers. Do not call gpio_request_one().

Makes sense.

Thanks again

Markus

> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Linus Walleij July 25, 2014, 11:38 a.m. UTC | #7

On Thu, Jul 24, 2014 at 10:12 AM, Markus Niebel
<[e-mail]> wrote:
> Am 23.07.2014 18:14, wrote Linus Walleij:

>> So always prepare the hardware and make it ready for action in respective
>> callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
>> been called first.
>>
>
> So a gpio driver is responsible to read status of gpio lines and flag any gpio line
> currently configured as out (base on the information read from hardware registers)
> on driver probe time - correct?

I don't think anyone reads that information explicitly to set up
these flags.

Drivers just leave the pins in their power-on maiden state without
trying to figure out how they're set-up. But as you say, if you call
gpiod_get_direction() on them, the flag gets set up indeed.

So usually these flags are set by code, calling
gpiod_[get/set]_direction().
Then they do get flagged as outputs or inputs.

> If yes is the driver allowed to call
> gpiod_get_direction() to have the FLAG_IS_OUT set in the gpiolib layer?

I don't know if it'd be a good idea to loop over all gpios in a new
irqchip and fetch the direction just to get the flags right, so far
we haven't done that and I don't know what the usecase would be.

If we need that we should do it in gpiolib for all drivers don't you
think?

But then we need a rationale for doing it, other than it's nice :-)
It is already called on-the-fly by debugfs when a user needs that
info.

Yours,
Linus Walleij

Markus Niebel July 25, 2014, 12:52 p.m. UTC | #8

Am 25.07.2014 13:38, wrote Linus Walleij:
> On Thu, Jul 24, 2014 at 10:12 AM, Markus Niebel
> <[e-mail]> wrote:
>> Am 23.07.2014 18:14, wrote Linus Walleij:
> 
>>> So always prepare the hardware and make it ready for action in respective
>>> callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
>>> been called first.
>>>
>>
>> So a gpio driver is responsible to read status of gpio lines and flag any gpio line
>> currently configured as out (base on the information read from hardware registers)
>> on driver probe time - correct?
> 
> I don't think anyone reads that information explicitly to set up
> these flags.
> 
> Drivers just leave the pins in their power-on maiden state without
> trying to figure out how they're set-up. But as you say, if you call
> gpiod_get_direction() on them, the flag gets set up indeed.
> 
> So usually these flags are set by code, calling
> gpiod_[get/set]_direction().
> Then they do get flagged as outputs or inputs.
> 
>> If yes is the driver allowed to call
>> gpiod_get_direction() to have the FLAG_IS_OUT set in the gpiolib layer?
> 
> I don't know if it'd be a good idea to loop over all gpios in a new
> irqchip and fetch the direction just to get the flags right, so far
> we haven't done that and I don't know what the usecase would be.
> 

I've came to a situation where it would have been helpful to know - bootloader
configured a pin as output and linux tried to configure the pin as IRQ input.
*YES I KNOW THIS IS NOT CORRECT* but it took some time to see, what happened.

> If we need that we should do it in gpiolib for all drivers don't you
> think?

A pragmatic / lean solution would be to deny the IRQ configuration when
seeing the pin configured as output in hardware and print out an error
on the gpio driver level.

> 
> But then we need a rationale for doing it, other than it's nice :-)
> It is already called on-the-fly by debugfs when a user needs that
> info.

See above, I think an error on a misconfigured system would be enough.
Maybe the documentation for gpio drivers should have a hint for driver
implementors.

So first step would be to convert the driver to the irqchip helper and then
add calls to gpiod_[un]lock_as_irq at the right places.

Will try to do that after my holiday.

> 
> Yours,
> Linus Walleij
> 
Yours,

Markus Niebel

Linus Walleij July 25, 2014, 1:52 p.m. UTC | #9

On Fri, Jul 25, 2014 at 2:52 PM, Markus Niebel
<[e-mail]> wrote:
> Am 25.07.2014 13:38, wrote Linus Walleij:

>> I don't know if it'd be a good idea to loop over all gpios in a new
>> irqchip and fetch the direction just to get the flags right, so far
>> we haven't done that and I don't know what the usecase would be.
>
> I've came to a situation where it would have been helpful to know - bootloader
> configured a pin as output and linux tried to configure the pin as IRQ input.
> *YES I KNOW THIS IS NOT CORRECT* but it took some time to see, what happened.

Hm, maybe we should call gpiod_get_direction() first in
gpio_lock_as_irq(), so we check the hardware and know there that the
flag is correctly set here?

>> If we need that we should do it in gpiolib for all drivers don't you
>> think?
>
> A pragmatic / lean solution would be to deny the IRQ configuration when
> seeing the pin configured as output in hardware and print out an error
> on the gpio driver level.

I guess that is what I'm suggesting, not sure if I follow
correctly.

Yours,
Linus Walleij

Patch

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index db83b3c..4316a38 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -175,6 +175,31 @@  static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	u32 gpio = port->bgc.gc.base + gpio_idx;
 	int edge;
 	void __iomem *reg = port->base;
+	int ret = 0;
+
+	if (!gpiochip_is_requested(&port->bgc.gc, gpio_idx)) {
+		char label[32];
+
+		snprintf(label, 32, "gpio%u-irq", gpio);
+		ret = gpio_request_one(gpio, GPIOF_DIR_IN, label);
+	} else {
+		val = readl(port->base + GPIO_GDIR);
+		if (val & BIT(gpio_idx))
+			ret = -EINVAL;
+	}
+
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to set gpio_idx %u as IN\n",
+			gpio_idx);
+		return ret;
+	}
+
+	ret = gpio_lock_as_irq(&port->bgc.gc, gpio_idx);
+	if (ret) {
+		dev_err(port->bgc.gc.dev, "unable to lock gpio_idx %u for IRQ\n",
+			gpio_idx);
+		return ret;
+	}
 
 	port->both_edges &= ~(1 << gpio_idx);
 	switch (type) {
@@ -231,6 +256,15 @@  static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	return 0;
 }
 
+static void gpio_irq_shutdown(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct mxc_gpio_port *port = gc->private;
+	u32 gpio_idx = d->hwirq;
+
+	gpio_unlock_as_irq(&port->bgc.gc, gpio_idx);
+}
+
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 {
 	void __iomem *reg = port->base;
@@ -353,6 +387,7 @@  static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
 	ct->chip.irq_mask = irq_gc_mask_clr_bit;
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_set_irq_type;
+	ct->chip.irq_shutdown = gpio_irq_shutdown;
 	ct->chip.irq_set_wake = gpio_set_wake_irq;
 	ct->regs.ack = GPIO_ISR;
 	ct->regs.mask = GPIO_IMR;