All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
@ 2013-09-28 17:22 Fabio Estevam
  2013-09-28 19:04 ` Otavio Salvador
  2013-09-29 13:21 ` Benoît Thébaudeau
  0 siblings, 2 replies; 18+ messages in thread
From: Fabio Estevam @ 2013-09-28 17:22 UTC (permalink / raw)
  To: u-boot

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(-)

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)
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-28 17:22 [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output Fabio Estevam
@ 2013-09-28 19:04 ` Otavio Salvador
  2013-09-29 13:21 ` Benoît Thébaudeau
  1 sibling, 0 replies; 18+ messages in thread
From: Otavio Salvador @ 2013-09-28 19:04 UTC (permalink / raw)
  To: u-boot

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>

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-28 17:22 [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output Fabio Estevam
  2013-09-28 19:04 ` Otavio Salvador
@ 2013-09-29 13:21 ` Benoît Thébaudeau
  2013-09-29 17:09   ` Eric Bénard
  1 sibling, 1 reply; 18+ messages in thread
From: Benoît Thébaudeau @ 2013-09-29 13:21 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 13:21 ` Benoît Thébaudeau
@ 2013-09-29 17:09   ` Eric Bénard
  2013-09-29 17:48     ` Otavio Salvador
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Bénard @ 2013-09-29 17:09 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 17:09   ` Eric Bénard
@ 2013-09-29 17:48     ` Otavio Salvador
  2013-09-29 18:19       ` Eric Bénard
  0 siblings, 1 reply; 18+ messages in thread
From: Otavio Salvador @ 2013-09-29 17:48 UTC (permalink / raw)
  To: u-boot

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.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 17:48     ` Otavio Salvador
@ 2013-09-29 18:19       ` Eric Bénard
  2013-09-29 18:22         ` Fabio Estevam
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Bénard @ 2013-09-29 18:19 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:19       ` Eric Bénard
@ 2013-09-29 18:22         ` Fabio Estevam
  2013-09-29 18:26           ` Eric Bénard
  0 siblings, 1 reply; 18+ messages in thread
From: Fabio Estevam @ 2013-09-29 18:22 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:22         ` Fabio Estevam
@ 2013-09-29 18:26           ` Eric Bénard
  2013-09-29 18:48             ` Fabio Estevam
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Bénard @ 2013-09-29 18:26 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:26           ` Eric Bénard
@ 2013-09-29 18:48             ` Fabio Estevam
  2013-09-29 18:50               ` Benoît Thébaudeau
  0 siblings, 1 reply; 18+ messages in thread
From: Fabio Estevam @ 2013-09-29 18:48 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:48             ` Fabio Estevam
@ 2013-09-29 18:50               ` Benoît Thébaudeau
  2013-09-29 18:58                 ` Fabio Estevam
  0 siblings, 1 reply; 18+ messages in thread
From: Benoît Thébaudeau @ 2013-09-29 18:50 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:50               ` Benoît Thébaudeau
@ 2013-09-29 18:58                 ` Fabio Estevam
  2013-09-29 19:25                   ` Benoît Thébaudeau
  0 siblings, 1 reply; 18+ messages in thread
From: Fabio Estevam @ 2013-09-29 18:58 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 18:58                 ` Fabio Estevam
@ 2013-09-29 19:25                   ` Benoît Thébaudeau
  2013-09-29 19:42                     ` Otavio Salvador
  0 siblings, 1 reply; 18+ messages in thread
From: Benoît Thébaudeau @ 2013-09-29 19:25 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 19:25                   ` Benoît Thébaudeau
@ 2013-09-29 19:42                     ` Otavio Salvador
  2013-09-29 19:45                       ` Benoît Thébaudeau
  0 siblings, 1 reply; 18+ messages in thread
From: Otavio Salvador @ 2013-09-29 19:42 UTC (permalink / raw)
  To: u-boot

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?

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 19:42                     ` Otavio Salvador
@ 2013-09-29 19:45                       ` Benoît Thébaudeau
  2013-09-29 19:49                         ` Otavio Salvador
  0 siblings, 1 reply; 18+ messages in thread
From: Benoît Thébaudeau @ 2013-09-29 19:45 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 19:45                       ` Benoît Thébaudeau
@ 2013-09-29 19:49                         ` Otavio Salvador
  2013-09-29 22:24                           ` Otavio Salvador
  0 siblings, 1 reply; 18+ messages in thread
From: Otavio Salvador @ 2013-09-29 19:49 UTC (permalink / raw)
  To: u-boot

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                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 19:49                         ` Otavio Salvador
@ 2013-09-29 22:24                           ` Otavio Salvador
  2013-09-30  1:32                             ` Fabio Estevam
  0 siblings, 1 reply; 18+ messages in thread
From: Otavio Salvador @ 2013-09-29 22:24 UTC (permalink / raw)
  To: u-boot

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.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-29 22:24                           ` Otavio Salvador
@ 2013-09-30  1:32                             ` Fabio Estevam
  2013-09-30 11:08                               ` Benoît Thébaudeau
  0 siblings, 1 reply; 18+ messages in thread
From: Fabio Estevam @ 2013-09-30  1:32 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output
  2013-09-30  1:32                             ` Fabio Estevam
@ 2013-09-30 11:08                               ` Benoît Thébaudeau
  0 siblings, 0 replies; 18+ messages in thread
From: Benoît Thébaudeau @ 2013-09-30 11:08 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2013-09-30 11:08 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-28 17:22 [U-Boot] [PATCH] gpio: mxc_gpio: Fix gpio_get_value() when the GPIO is an output Fabio Estevam
2013-09-28 19:04 ` Otavio Salvador
2013-09-29 13:21 ` Benoît Thébaudeau
2013-09-29 17:09   ` Eric Bénard
2013-09-29 17:48     ` Otavio Salvador
2013-09-29 18:19       ` Eric Bénard
2013-09-29 18:22         ` Fabio Estevam
2013-09-29 18:26           ` Eric Bénard
2013-09-29 18:48             ` Fabio Estevam
2013-09-29 18:50               ` Benoît Thébaudeau
2013-09-29 18:58                 ` Fabio Estevam
2013-09-29 19:25                   ` Benoît Thébaudeau
2013-09-29 19:42                     ` Otavio Salvador
2013-09-29 19:45                       ` Benoît Thébaudeau
2013-09-29 19:49                         ` Otavio Salvador
2013-09-29 22:24                           ` Otavio Salvador
2013-09-30  1:32                             ` Fabio Estevam
2013-09-30 11:08                               ` Benoît Thébaudeau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.