Patchwork [U-Boot] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output

login
register
mail settings
Submitter Fabio Estevam
Date Sept. 28, 2013, 5:22 p.m.
Message ID <1380388964-26009-1-git-send-email-festevam@gmail.com>
Download mbox | patch
Permalink /patch/278752/
State Rejected
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - Sept. 28, 2013, 5:22 p.m.
From: Fabio Estevam <fabio.estevam@freescale.com>

When the GPIO is configured as an output, we should return the value from the DR
register.

This implements the same fix as in the following kernel commit from FSL BSP:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Otavio,

I don't have i.mx hardware access at the moment to try it, but this should fix 
your LED problem.

 drivers/gpio/mxc_gpio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Otavio Salvador - Sept. 28, 2013, 7:04 p.m.
On Sat, Sep 28, 2013 at 2:22 PM, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> When the GPIO is configured as an output, we should return the value from the DR
> register.
>
> This implements the same fix as in the following kernel commit from FSL BSP:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---

It does indeed; I redid the series using this and simplified the code.

Thanks by looking at this.

Acked-by: Otavio Salvador <otavio@ossystems.com.br>
Benoît Thébaudeau - Sept. 29, 2013, 1:21 p.m.
Hi Fabio,

On Saturday, September 28, 2013 7:22:44 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When the GPIO is configured as an output, we should return the value from the
> DR
> register.
> 
> This implements the same fix as in the following kernel commit from FSL BSP:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45

Why is this required? Is it because there is a different behavior of the PSR
register on one of the i.MXs?

See my commit message here:
http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=5dafa4543c399d329c7b01df1afa98437861cac0

In case the registers are configured to output some level on a GPIO but there is
a level conflict with other hardware, the general assumption about
gpio_get_value() would probably be that it returns the actual GPIO level, not
the level that the registers try to apply. For the latter, another function
accessing DR could be implemented.

One could also argue that such GPIO level conflicts are just the result of a
flawed hardware design, and so that normal software should not care about such a
case. But it's good to be able to detect hardware issues from software, and this
patch seems to change the meaning of gpio_get_value() to cover some hardware
issue. Please give more details.

In any case, there should probably be a comment in the code for this function to
explain the various issues and how they are addressed.

Best regards,
Benoît
Eric Benard - Sept. 29, 2013, 5:09 p.m.
Hi Benoît,

Le Sun, 29 Sep 2013 15:21:52 +0200 (CEST),
Benoît Thébaudeau <benoit.thebaudeau@advansee.com> a écrit :
> Why is this required? Is it because there is a different behavior of the PSR
> register on one of the i.MXs?
> 
> See my commit message here:
> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=5dafa4543c399d329c7b01df1afa98437861cac0
> 
> In case the registers are configured to output some level on a GPIO but there is
> a level conflict with other hardware, the general assumption about
> gpio_get_value() would probably be that it returns the actual GPIO level, not
> the level that the registers try to apply. For the latter, another function
> accessing DR could be implemented.
> 
you are right and if that works in the kernel, that should also work
in u-boot. It would be interesting to know if the original patch was
really fixing a problem as it would be surprising that setting the pin
as an input could fix the level sampling problem reliably : Otavio was
that tested on real hardware ?

BTW Otavio if you read that email through the ML, your MX server rejects
my emails :
<otavio@ossystems.com.br>: host mx.ossystems.com.br[66.7.219.172] said:
450 4.1.8 <eric@eukrea.com>: Sender address rejected: Domain not found
(in reply to RCPT TO command)

Eric
Otavio Salvador - Sept. 29, 2013, 5:48 p.m.
On Sun, Sep 29, 2013 at 2:09 PM, Eric Bénard <eric@eukrea.com> wrote:
> Hi Benoît,
>
> Le Sun, 29 Sep 2013 15:21:52 +0200 (CEST),
> Benoît Thébaudeau <benoit.thebaudeau@advansee.com> a écrit :
>> Why is this required? Is it because there is a different behavior of the PSR
>> register on one of the i.MXs?
>>
>> See my commit message here:
>> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=5dafa4543c399d329c7b01df1afa98437861cac0
>>
>> In case the registers are configured to output some level on a GPIO but there is
>> a level conflict with other hardware, the general assumption about
>> gpio_get_value() would probably be that it returns the actual GPIO level, not
>> the level that the registers try to apply. For the latter, another function
>> accessing DR could be implemented.
>>
> you are right and if that works in the kernel, that should also work
> in u-boot. It would be interesting to know if the original patch was
> really fixing a problem as it would be surprising that setting the pin
> as an input could fix the level sampling problem reliably : Otavio was
> that tested on real hardware ?

Yes; it did.

Both my original patch (setting it as input) and Fabio's one checking
the other register when in output worked fine.

> BTW Otavio if you read that email through the ML, your MX server rejects
> my emails :
> <otavio@ossystems.com.br>: host mx.ossystems.com.br[66.7.219.172] said:
> 450 4.1.8 <eric@eukrea.com>: Sender address rejected: Domain not found
> (in reply to RCPT TO command)

Indeed. I am trying to fix it :-( my bad.
Eric Benard - Sept. 29, 2013, 6:19 p.m.
Le Sun, 29 Sep 2013 14:48:32 -0300,
Otavio Salvador <otavio@ossystems.com.br> a écrit :

> On Sun, Sep 29, 2013 at 2:09 PM, Eric Bénard <eric@eukrea.com> wrote:
> > Hi Benoît,
> >
> > Le Sun, 29 Sep 2013 15:21:52 +0200 (CEST),
> > Benoît Thébaudeau <benoit.thebaudeau@advansee.com> a écrit :
> >> Why is this required? Is it because there is a different behavior of the PSR
> >> register on one of the i.MXs?
> >>
> >> See my commit message here:
> >> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=5dafa4543c399d329c7b01df1afa98437861cac0
> >>
> >> In case the registers are configured to output some level on a GPIO but there is
> >> a level conflict with other hardware, the general assumption about
> >> gpio_get_value() would probably be that it returns the actual GPIO level, not
> >> the level that the registers try to apply. For the latter, another function
> >> accessing DR could be implemented.
> >>
> > you are right and if that works in the kernel, that should also work
> > in u-boot. It would be interesting to know if the original patch was
> > really fixing a problem as it would be surprising that setting the pin
> > as an input could fix the level sampling problem reliably : Otavio was
> > that tested on real hardware ?
> 
> Yes; it did.
> 
> Both my original patch (setting it as input) and Fabio's one checking
> the other register when in output worked fine.
> 
on which CPU is that ?
It's strange reading PSR works in the kernel and not in u-boot.

Eric
Fabio Estevam - Sept. 29, 2013, 6:22 p.m.
On Sun, Sep 29, 2013 at 3:19 PM, Eric Bénard <eric@eukrea.com> wrote:

> on which CPU is that ?

Otavio tested it on mx6.

> It's strange reading PSR works in the kernel and not in u-boot.

The patch I sent aligns with the kernel behaviour as well:

http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45

Will investige it in more details tomorrow at FSL.

Regards,

Fabio Estevam
Eric Benard - Sept. 29, 2013, 6:26 p.m.
Hi Fabio,

Le Sun, 29 Sep 2013 15:22:36 -0300,
Fabio Estevam <festevam@gmail.com> a écrit :

> On Sun, Sep 29, 2013 at 3:19 PM, Eric Bénard <eric@eukrea.com> wrote:
> 
> > on which CPU is that ?
> 
> Otavio tested it on mx6.
> 
> > It's strange reading PSR works in the kernel and not in u-boot.
> 
> The patch I sent aligns with the kernel behaviour as well:
> 
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45
> 
mainline kernel doesn't seem to have this fix.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-mxc.c

Eric
Fabio Estevam - Sept. 29, 2013, 6:48 p.m.
On Sun, Sep 29, 2013 at 3:26 PM, Eric Bénard <eric@eukrea.com> wrote:
> Hi Fabio,
>
> Le Sun, 29 Sep 2013 15:22:36 -0300,
> Fabio Estevam <festevam@gmail.com> a écrit :
>
>> On Sun, Sep 29, 2013 at 3:19 PM, Eric Bénard <eric@eukrea.com> wrote:
>>
>> > on which CPU is that ?
>>
>> Otavio tested it on mx6.
>>
>> > It's strange reading PSR works in the kernel and not in u-boot.
>>
>> The patch I sent aligns with the kernel behaviour as well:
>>
>> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45
>>
> mainline kernel doesn't seem to have this fix.
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-mxc.c

Yes, I saw that too. Maybe it will also need to be fixed.

First I want to run it on real hardware and understand the issue in more detail.

Regards,

Fabio Estevam
Benoît Thébaudeau - Sept. 29, 2013, 6:50 p.m.
Hi Fabio,

On Sunday, September 29, 2013 8:48:46 PM, Fabio Estevam wrote:
> On Sun, Sep 29, 2013 at 3:26 PM, Eric Bénard <eric@eukrea.com> wrote:
> > Hi Fabio,
> >
> > Le Sun, 29 Sep 2013 15:22:36 -0300,
> > Fabio Estevam <festevam@gmail.com> a écrit :
> >
> >> On Sun, Sep 29, 2013 at 3:19 PM, Eric Bénard <eric@eukrea.com> wrote:
> >>
> >> > on which CPU is that ?
> >>
> >> Otavio tested it on mx6.
> >>
> >> > It's strange reading PSR works in the kernel and not in u-boot.
> >>
> >> The patch I sent aligns with the kernel behaviour as well:
> >>
> >> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/arch/arm/plat-mxc/gpio.c?h=imx_3.0.35_4.1.0&id=d6f32393eaf455ce3c32d4e9bafd34d9091eaf45
> >>
> > mainline kernel doesn't seem to have this fix.
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-mxc.c
> 
> Yes, I saw that too. Maybe it will also need to be fixed.
> 
> First I want to run it on real hardware and understand the issue in more
> detail.

Can you test again without any GPIO patch, but with SION set for this pin in the
IOMUXC? According to the reference manual, SION not being set in the IOMUXC is
the only reason that would prevent PSR from reading the pin level in GPIO output
mode.

Best regards,
Benoît
Fabio Estevam - Sept. 29, 2013, 6:58 p.m.
Hi Benoît,

On Sun, Sep 29, 2013 at 3:50 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Can you test again without any GPIO patch, but with SION set for this pin in the
> IOMUXC? According to the reference manual, SION not being set in the IOMUXC is
> the only reason that would prevent PSR from reading the pin level in GPIO output
> mode.

Yes, from the feedback from a colleague the SION bit plays a role here:

"The RM does not describe this clear. The fact should be:

PSR for input, DR for output.
PSR can be for output only when SION bit is set (now PSR = DR). "

Regards,

Fabio Estevam
Benoît Thébaudeau - Sept. 29, 2013, 7:25 p.m.
Hi Fabio,

On Sunday, September 29, 2013 8:58:09 PM, Fabio Estevam wrote:
> Hi Benoît,
> 
> On Sun, Sep 29, 2013 at 3:50 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Can you test again without any GPIO patch, but with SION set for this pin
> > in the
> > IOMUXC? According to the reference manual, SION not being set in the IOMUXC
> > is
> > the only reason that would prevent PSR from reading the pin level in GPIO
> > output
> > mode.
> 
> Yes, from the feedback from a colleague the SION bit plays a role here:
> 
> "The RM does not describe this clear. The fact should be:
> 
> PSR for input, DR for output.
> PSR can be for output only when SION bit is set (now PSR = DR). "

Yes, that's well explained in the i.MX6Q RM:
"
28.4.2.1 GPIO Read Mode
The programming sequence for reading input signals should be as follows:
1. Configure IOMUX to select GPIO mode (Via IOMUX Controller (IOMUXC) ).
2. Configure GPIO direction register to input (GPIO_GDIR[GDIR] set to 0b).
3. Read value from data register/pad status register.
[...]
NOTE
While the GPIO direction is set to input (GPIO_GDIR = 0), a
read access to GPIO_DR does not return GPIO_DR data.
Instead, it returns the GPIO_PSR data, which is the
corresponding input signal value.

28.4.2.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
2. Configure GPIO direction register to output (GPIO_GDIR[GDIR] set to 1b).
3. Write value to data register (GPIO_DR).
"

So:
 - in input mode: DR = PSR = pin,
 - in output mode: DR is applied to the pin, and the pin level can be read back
   through PSR if SION is set (i.e. PSR = DR unless there is a hardware
   conflict).

Hence, gpio_get_value() should be left unchanged (using PSR in all cases), and
SION should be set for all GPIOs in the i.MX6 pin definition header files.

Best regards,
Benoît
Otavio Salvador - Sept. 29, 2013, 7:42 p.m.
On Sun, Sep 29, 2013 at 4:25 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
...
> Hence, gpio_get_value() should be left unchanged (using PSR in all cases), and
> SION should be set for all GPIOs in the i.MX6 pin definition header files.

I just does not follow why this preferred against Fabio's proposed
patch to read from DR?
Benoît Thébaudeau - Sept. 29, 2013, 7:45 p.m.
Hi Otavio,

On Sunday, September 29, 2013 9:42:44 PM, Otavio Salvador wrote:
> On Sun, Sep 29, 2013 at 4:25 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> ...
> > Hence, gpio_get_value() should be left unchanged (using PSR in all cases),
> > and
> > SION should be set for all GPIOs in the i.MX6 pin definition header files.
> 
> I just does not follow why this preferred against Fabio's proposed
> patch to read from DR?

Because in case of a level conflict between the GPIO output and some other
hardware on the board, one would expect gpio_get_value() to return the actual
pin level, and not the level that the GPIO output tries (but possibly fails) to
apply on this pin.

Best regards,
Benoît
Otavio Salvador - Sept. 29, 2013, 7:49 p.m.
On Sun, Sep 29, 2013 at 4:45 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> On Sunday, September 29, 2013 9:42:44 PM, Otavio Salvador wrote:
>> On Sun, Sep 29, 2013 at 4:25 PM, Benoît Thébaudeau
>> <benoit.thebaudeau@advansee.com> wrote:
>> ...
>> > Hence, gpio_get_value() should be left unchanged (using PSR in all cases),
>> > and
>> > SION should be set for all GPIOs in the i.MX6 pin definition header files.
>>
>> I just does not follow why this preferred against Fabio's proposed
>> patch to read from DR?
>
> Because in case of a level conflict between the GPIO output and some other
> hardware on the board, one would expect gpio_get_value() to return the actual
> pin level, and not the level that the GPIO output tries (but possibly fails) to
> apply on this pin.

Ahh now I see. I agree :-)
Otavio Salvador - Sept. 29, 2013, 10:24 p.m.
On Sun, Sep 29, 2013 at 4:49 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:
> On Sun, Sep 29, 2013 at 4:45 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
>> On Sunday, September 29, 2013 9:42:44 PM, Otavio Salvador wrote:
>>> On Sun, Sep 29, 2013 at 4:25 PM, Benoît Thébaudeau
>>> <benoit.thebaudeau@advansee.com> wrote:
>>> ...
>>> > Hence, gpio_get_value() should be left unchanged (using PSR in all cases),
>>> > and
>>> > SION should be set for all GPIOs in the i.MX6 pin definition header files.
>>>
>>> I just does not follow why this preferred against Fabio's proposed
>>> patch to read from DR?
>>
>> Because in case of a level conflict between the GPIO output and some other
>> hardware on the board, one would expect gpio_get_value() to return the actual
>> pin level, and not the level that the GPIO output tries (but possibly fails) to
>> apply on this pin.
>
> Ahh now I see. I agree :-)

I sent the patch to fix this adding the flag to the GPIO pins.

I tested it and it works fine indeed. The patch is awaiting for
approval as it is a little big. The commitlog is below for reference:

    mx6: Add IOMUX_CONFIG_SION flag to all GPIO pins

    The IOMUX_CONFIG_SION allows for reading PAD value from PSR register.

    The following quote from the datasheet:

    ,----
    | ...
    | 28.4.2.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
    | 2. Configure GPIO direction register to output (GPIO_GDIR[GDIR]
set to 1b).
    | 3. Write value to data register (GPIO_DR).
    | ...
    `----

    This fixes the gpio_get_value to properly work when a GPIO is set for
    output and has no conflicts.

    Thanks for Benoît Thébaudeau <benoit.thebaudeau@advansee.com>, Fabio
    Estevam <fabio.estevam@freescale.com> and Eric Bénard
    <eric@eukrea.com> for helping to properly trace this down.

    Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Again, thanks for the help on this.
Fabio Estevam - Sept. 30, 2013, 1:32 a.m.
On Sun, Sep 29, 2013 at 7:24 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> I sent the patch to fix this adding the flag to the GPIO pins.
>
> I tested it and it works fine indeed. The patch is awaiting for
> approval as it is a little big. The commitlog is below for reference:
>
>     mx6: Add IOMUX_CONFIG_SION flag to all GPIO pins

Ok, this fixes mx6, but other i.mx SoC would need the same fix as well, right?

Also, if you have a chance please send a fix for the mainline kernel.

Regards,

Fabio Estevam
Benoît Thébaudeau - Sept. 30, 2013, 11:08 a.m.
Hi Fabio,

On Monday, September 30, 2013 3:32:39 AM, Fabio Estevam wrote:
> On Sun, Sep 29, 2013 at 7:24 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
> 
> > I sent the patch to fix this adding the flag to the GPIO pins.
> >
> > I tested it and it works fine indeed. The patch is awaiting for
> > approval as it is a little big. The commitlog is below for reference:
> >
> >     mx6: Add IOMUX_CONFIG_SION flag to all GPIO pins
> 
> Ok, this fixes mx6, but other i.mx SoC would need the same fix as well,
> right?

Maybe not. The requirement for SION setting for the various pin functions
differs among i.MXs. E.g., SION is required for I²C on some i.MX (51 I think),
but not for all. So this should be checked for GPIO as well on a per-i.MX basis.

[...]

Best regards,
Benoît

Patch

diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c
index 6a572d5..debbecb 100644
--- a/drivers/gpio/mxc_gpio.c
+++ b/drivers/gpio/mxc_gpio.c
@@ -93,7 +93,7 @@  int gpio_get_value(unsigned gpio)
 {
 	unsigned int port = GPIO_TO_PORT(gpio);
 	struct gpio_regs *regs;
-	u32 val;
+	u32 direction;
 
 	if (port >= ARRAY_SIZE(gpio_ports))
 		return -1;
@@ -102,9 +102,12 @@  int gpio_get_value(unsigned gpio)
 
 	regs = (struct gpio_regs *)gpio_ports[port];
 
-	val = (readl(&regs->gpio_psr) >> gpio) & 0x01;
+	direction = readl(&regs->gpio_dir);
 
-	return val;
+	if ((direction >> gpio) & 1)
+		return (readl(&regs->gpio_dr) >> gpio) & 1; /* output mode */
+	else
+		return (readl(&regs->gpio_psr) >> gpio) & 1; /* input mode */
 }
 
 int gpio_request(unsigned gpio, const char *label)