| Message ID | 1380388964-26009-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Stefano Babic |
| Headers | show |
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>
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
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
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.
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
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
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
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
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
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
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
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?
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
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 :-)
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.
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
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
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(®s->gpio_psr) >> gpio) & 0x01; + direction = readl(®s->gpio_dir); - return val; + if ((direction >> gpio) & 1) + return (readl(®s->gpio_dr) >> gpio) & 1; /* output mode */ + else + return (readl(®s->gpio_psr) >> gpio) & 1; /* input mode */ } int gpio_request(unsigned gpio, const char *label)