[PATCH 0/2] couple of fixes on IMX gpio driver

[PATCH 0/2] couple of fixes on IMX gpio driver

[Eduardo Valentin]

Hello all,

Here are two fixes on IMX GPIO driver. One is on
the way we handle GPIO irqs while going into suspend.
Another one is to be able to read the value
of an output GPIO.

Eduardo Valentin (1):
  gpio/mxc: implement reading output gpio value

Ulises Brindis (1):
  gpio/mxc: mask gpio interrupts in suspend

 drivers/gpio/gpio-mxc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.5.0

[PATCH 1/2] gpio/mxc: mask gpio interrupts in suspend

[Eduardo Valentin]

From: Ulises Brindis <ubrindis56@gmail.com>

Currently in the FSL platform all GPIO interrupts in a bank are muxed
into two GPIO lines to the GPC interrupt controller. In each GPIO bank
GPIOs 0-15 are OR'ed into one GPC interrupt controller interrupt and 16-31
are OR'ed into another. With the current code, if any of the 0-15 or
16-31 interrupts are marked as wakeup capable, all interrupts belonging
to that sub-bank (either 0-15 or 16-31) will wake up the device. This is
because interrupts are only being masked at the interrupt controller
and not at the GPIO controller.

This patch allows masking of GPIO interrupts at the GPIO controller during
suspend if they have not been labeled wakeup capable. This patch uses
preexisting IRQCHIP_MASK_ON_SUSPEND flag while initializing the GPIO
interrupts to get the desired behavior.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Ulises Brindis <ubrindis56@gmail.com>
---
 drivers/gpio/gpio-mxc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index ec1eb1b..9e5bdbd 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -354,6 +354,7 @@ static void __init mxc_gpio_init_gc(struct mxc_gpio_port *port, int irq_base)
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_set_irq_type;
 	ct->chip.irq_set_wake = gpio_set_wake_irq;
+	ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND;
 	ct->regs.ack = GPIO_ISR;
 	ct->regs.mask = GPIO_IMR;
 
-- 
2.5.0

[PATCH 2/2] gpio/mxc: implement reading output gpio value

[Eduardo Valentin]

In current implementation, reading the value of an output gpio
always return 0. The reason is because when a gpio is configured
as output, its output status can be read from the GPIO_DR register,
and when it is configure as input, its value can be read from
GPIO_PSR. With current implementation of basic mmio gpio driver,
we can only read the value from one single register.

Therefore, to workaround the basic mmio gpio driver limitation,
this patch changes the gpio-mxc driver to provide its own .get
callback. In the callback, we check the current status of the
gpio direction, and read the correct register based on its
current gpio direction status.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ulises Brindis <ubrindis56@gmail.com>
Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/gpio/gpio-mxc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 9e5bdbd..8822823 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -401,6 +401,18 @@ static int mxc_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 	return irq_find_mapping(port->domain, offset);
 }
 
+static int mxc_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	if (!!(bgc->read_reg(bgc->reg_dir) & bgc->pin2mask(bgc, gpio)))
+		return !!(bgc->read_reg(bgc->reg_set) &
+						bgc->pin2mask(bgc, gpio));
+	else
+		return !!(bgc->read_reg(bgc->reg_dat) &
+						bgc->pin2mask(bgc, gpio));
+}
+
 static int mxc_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -455,6 +467,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (err)
 		goto out_bgio;
 
+	port->bgc.gc.get = mxc_gpio_get;
 	port->bgc.gc.to_irq = mxc_gpio_to_irq;
 	port->bgc.gc.base = (pdev->id < 0) ? of_alias_get_id(np, "gpio") * 32 :
 					     pdev->id * 32;
-- 
2.5.0

Re: [PATCH 2/2] gpio/mxc: implement reading output gpio value

[Fabio Estevam]

Hi Eduardo,

On Wed, Aug 5, 2015 at 2:23 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
> In current implementation, reading the value of an output gpio
> always return 0. The reason is because when a gpio is configured

Have you tried setting the SION bit for the pad?

This should fix the problem.

Regards,

Fabio Estevam

Re: [PATCH 2/2] gpio/mxc: implement reading output gpio value

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]


Hello Fabio,

On Wed, Aug 05, 2015 at 02:43:05PM -0300, Fabio Estevam wrote:
> Hi Eduardo,
> 
> On Wed, Aug 5, 2015 at 2:23 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
> > In current implementation, reading the value of an output gpio
> > always return 0. The reason is because when a gpio is configured
> 
> Have you tried setting the SION bit for the pad?

No, I haven't. From the bit description, it looks like it does a
different thing of what we would achieve with this patch. The SION bit
is a overwrite to the pad configuration. That is, the pin will be INPUT
always.

We don't want to force it to be input. We simply want to be able to read
the value of an output GPIO. That is achievable by reading a different
register. In case the GPIO is configured as output, the GPIO block
documentation states its value can be read from GPIO_PSR.
This approach works fine, as the GPIO will be output and we can still
read its value.

Spotting this is very simple. A quick try on /sys/class/gpio/export on
a spare gpio would do it:

# echo $SPARE_GPIO > /sys/class/gpio/export
# echo out > /sys/class/gpio/gpio$SPARE_GPIO/direction
# echo 1 > /sys/class/gpio/gpio$SPARE_GPIO/value
# cat /sys/class/gpio/gpio$SPARE_GPIO/value

without the patch it would always return 0. With the patch in, you see
the read value corresponding the output of that pin (checked on scope,
for instance).



> 
> This should fix the problem.
> 
> Regards,
> 
> Fabio Estevam

BR,

Eduardo Valentin

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] gpio/mxc: implement reading output gpio value

[Fabio Estevam]

Hi Eduardo,

On Wed, Aug 5, 2015 at 3:02 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
>
> Hello Fabio,
>
> On Wed, Aug 05, 2015 at 02:43:05PM -0300, Fabio Estevam wrote:
>> Hi Eduardo,
>>
>> On Wed, Aug 5, 2015 at 2:23 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
>> > In current implementation, reading the value of an output gpio
>> > always return 0. The reason is because when a gpio is configured
>>
>> Have you tried setting the SION bit for the pad?
>
> No, I haven't. From the bit description, it looks like it does a
> different thing of what we would achieve with this patch. The SION bit
> is a overwrite to the pad configuration. That is, the pin will be INPUT
> always.
>
> We don't want to force it to be input. We simply want to be able to read
> the value of an output GPIO. That is achievable by reading a different
> register. In case the GPIO is configured as output, the GPIO block
> documentation states its value can be read from GPIO_PSR.
> This approach works fine, as the GPIO will be output and we can still
> read its value.

Setting the SION bit allows you to do exactly that. Please see:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/271774.html

Regards,

Fabio Estevam

Re: [PATCH 2/2] gpio/mxc: implement reading output gpio value

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

On Wed, Aug 05, 2015 at 03:11:40PM -0300, Fabio Estevam wrote:
> Hi Eduardo,
> 
> On Wed, Aug 5, 2015 at 3:02 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
> >
> > Hello Fabio,
> >
> > On Wed, Aug 05, 2015 at 02:43:05PM -0300, Fabio Estevam wrote:
> >> Hi Eduardo,
> >>
> >> On Wed, Aug 5, 2015 at 2:23 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
> >> > In current implementation, reading the value of an output gpio
> >> > always return 0. The reason is because when a gpio is configured
> >>
> >> Have you tried setting the SION bit for the pad?
> >
> > No, I haven't. From the bit description, it looks like it does a
> > different thing of what we would achieve with this patch. The SION bit
> > is a overwrite to the pad configuration. That is, the pin will be INPUT
> > always.
> >
> > We don't want to force it to be input. We simply want to be able to read
> > the value of an output GPIO. That is achievable by reading a different
> > register. In case the GPIO is configured as output, the GPIO block
> > documentation states its value can be read from GPIO_PSR.
> > This approach works fine, as the GPIO will be output and we can still
> > read its value.
> 
> Setting the SION bit allows you to do exactly that. Please see:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/271774.html

OK. Then, what is the recommendation? Do we set this by default in the
driver code or this is left for DTS pinmux configuration?

To me seams like a bug in the gpio driver still, as the possibility to
read the value of an output gpio is missing/inconsistent.

> 
> Regards,
> 
> Fabio Estevam

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] gpio/mxc: implement reading output gpio value

[Fabio Estevam]

On Wed, Aug 5, 2015 at 4:46 PM, Eduardo Valentin <edubezval@gmail.com> wrote:

> OK. Then, what is the recommendation? Do we set this by default in the
> driver code or this is left for DTS pinmux configuration?

You just need to set the SION bit in the pad you need to read the output.

SION is bit 30 according to
Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt

> To me seams like a bug in the gpio driver still, as the possibility to
> read the value of an output gpio is missing/inconsistent.

No, it is not a bug.

The Reference Manual explains the need of setting the SION bit:

>From the mx6q reference manual:

"28.4.3.2 GPIO Write Mode
The programming sequence for driving output signals should be as follows:
1. Configure IOMUX to select GPIO mode (Via IOMUXC), also enable SION if need
to read loopback pad value through PSR"

Re: [PATCH 1/2] gpio/mxc: mask gpio interrupts in suspend

[Linus Walleij]

On Wed, Aug 5, 2015 at 7:23 PM, Eduardo Valentin <edubezval@gmail.com> wrote:

> From: Ulises Brindis <ubrindis56@gmail.com>
>
> Currently in the FSL platform all GPIO interrupts in a bank are muxed
> into two GPIO lines to the GPC interrupt controller. In each GPIO bank
> GPIOs 0-15 are OR'ed into one GPC interrupt controller interrupt and 16-31
> are OR'ed into another. With the current code, if any of the 0-15 or
> 16-31 interrupts are marked as wakeup capable, all interrupts belonging
> to that sub-bank (either 0-15 or 16-31) will wake up the device. This is
> because interrupts are only being masked at the interrupt controller
> and not at the GPIO controller.
>
> This patch allows masking of GPIO interrupts at the GPIO controller during
> suspend if they have not been labeled wakeup capable. This patch uses
> preexisting IRQCHIP_MASK_ON_SUSPEND flag while initializing the GPIO
> interrupts to get the desired behavior.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: linux-gpio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Ulises Brindis <ubrindis56@gmail.com>

Patch applied.

Yours,
Linus Walleij