+ 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:
quoted
On Wed, Jul 16, 2014 at 03:51:04PM +0200, Markus Niebel wrote:
quoted
From: Markus Niebel <redacted>
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 <redacted>
---
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
quoted
quoted
+ } 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 ...
quoted
quoted
+ }
+
+ 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.
quoted
quoted
+
+ 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
quoted
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
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jul 22, 2014 at 9:42 AM, Shawn Guo [off-list ref] wrote:
On Tue, Jul 22, 2014 at 09:19:22AM +0200, Markus Niebel wrote:
quoted
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
Am 23.07.2014 18:14, wrote Linus Walleij:
On Tue, Jul 22, 2014 at 9:42 AM, Shawn Guo [off-list ref] wrote:
quoted
On Tue, Jul 22, 2014 at 09:19:22AM +0200, Markus Niebel wrote:
quoted
quoted
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 majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html