gpio-mxc gpio get always returns 0 for outputs for IMX6

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Tim Harvey]

Shawn,

I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
for gpio's configured as outputs regardless of if they are outputing
high or low.

Digging into gpio-mxc.c I see that the basic memory-mapped generic
gpio controller is used, but its configured to use GPIO_PSR to obtain
the gpio value. For the IMX6 (I can't speak for the other MXC/IMX
users of this driver) the reference manual indicates that GPIO_PSR is
appropriate only if the GPIO is an input and that GPIO_DR will read
the proper state if its configured as an output or an input.

Even before the driver was converted to use the basic memory-mapped
generic gpio controller this was the case (GPIO_PSR used for get). Is
there a reason for this, or is it simply a bug that has been around
since the driver's original creation as I suspect?

The following patch resolves the issue and causes gpio get to always
return the proper setting when configured for an input or output:

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 7176743..25b97db 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -458,8 +458,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
        }

        err = bgpio_init(&port->bgc, &pdev->dev, 4,
-                        port->base + GPIO_PSR,
-                        port->base + GPIO_DR, NULL,
+                        port->base + GPIO_DR,
+                        NULL, NULL,
                         port->base + GPIO_GDIR, NULL, 0);
        if (err)
                goto out_iounmap;

If this looks right to you for all IMX users let me know and I'll post
a properly formatted patch with commit message.

Regards,

Tim

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Russell King - ARM Linux]

On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
> I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
> for gpio's configured as outputs regardless of if they are outputing
> high or low.

There's a different way which this can be solved - set the SION but on
your GPIOs.  You will then be able to read back the state of the
output pin.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: gpio-mxc gpio get always returns 0 for outputs for IMX6

[Alexander Shiyan]

Fri, 11 Jul 2014 17:02:38 +0100 ?? Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
> > I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
> > for gpio's configured as outputs regardless of if they are outputing
> > high or low.
> 
> There's a different way which this can be solved - set the SION but on
> your GPIOs.  You will then be able to read back the state of the
> output pin.

AFAIK, SION is not affected for pins configured as GPIO.
Please FIX ME if my opinion is wrong.

---

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Russell King - ARM Linux]

On Fri, Jul 11, 2014 at 08:16:43PM +0400, Alexander Shiyan wrote:
> Fri, 11 Jul 2014 17:02:38 +0100 ?? Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
> > > I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
> > > for gpio's configured as outputs regardless of if they are outputing
> > > high or low.
> > 
> > There's a different way which this can be solved - set the SION but on
> > your GPIOs.  You will then be able to read back the state of the
> > output pin.
> 
> AFAIK, SION is not affected for pins configured as GPIO.
> Please FIX ME if my opinion is wrong.

SION forces the input path to the GPIOs to be enabled, meaning you can
read the actual state of the pin.

I've ended up setting that on several GPIOs used for regulator control
on iMX6 so that I can see their output state in /sys/kernel/debug/gpio,
otherwise I can't read back their output states.  It appears to work
there:

GPIOs 0-31, platform/209c000.gpio, 209c000.gpio:
 gpio-0   (usb_h1_vbus         ) out lo
 gpio-2   (gpio-ir-recv        ) in  hi
 gpio-4   (2194000.usdhc cd    ) in  lo

GPIOs 32-63, platform/20a0000.gpio, 20a0000.gpio:

GPIOs 64-95, platform/20a4000.gpio, 20a4000.gpio:
 gpio-83  (brcm_reg            ) out hi
 gpio-86  (usb_otg_vbus        ) out lo

GPIOs 96-127, platform/20a8000.gpio, 20a8000.gpio:
 gpio-111 (phy-reset           ) out lo

GPIOs 128-159, platform/20ac000.gpio, 20ac000.gpio:
 gpio-133 (brcm_osc_reg        ) out hi
 gpio-154 (card-reset          ) out hi

GPIOs 160-191, platform/20b0000.gpio, 20b0000.gpio:
 gpio-160 (card-reset          ) out hi

GPIOs 192-223, platform/20b4000.gpio, 20b4000.gpio:


-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Fabio Estevam]

On Fri, Jul 11, 2014 at 1:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
>> I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
>> for gpio's configured as outputs regardless of if they are outputing
>> high or low.
>
> There's a different way which this can be solved - set the SION but on
> your GPIOs.  You will then be able to read back the state of the
> output pin.

Yes, setting the SION bit is the correct way to solve this.

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Shawn Guo]

Copy more i.MX people ...

On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
> Shawn,
> 
> I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
> for gpio's configured as outputs regardless of if they are outputing
> high or low.
> 
> Digging into gpio-mxc.c I see that the basic memory-mapped generic
> gpio controller is used, but its configured to use GPIO_PSR to obtain
> the gpio value. For the IMX6 (I can't speak for the other MXC/IMX
> users of this driver) the reference manual indicates that GPIO_PSR is
> appropriate only if the GPIO is an input and that GPIO_DR will read
> the proper state if its configured as an output or an input.


The following note can be found in i.MX Reference Manual.

			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.

That said for an input GPIO, reading GPIO_DR functionally equals to
reading GPIO_PSR.  So reading either GPIO_DR or GPIO_PSR returns what we
want to get.

The remain question is what we want to get from an output GPIO, the
value that register GPIO_DR holds or the one driven on pad.  I *think*
both should be identical (but not so sure).  If that's the case, reading
GPIO_DR should just work.  But otherwise, we should read GPIO_PSR with
SION bit set to get what is actually driven on pad.

> 
> Even before the driver was converted to use the basic memory-mapped
> generic gpio controller this was the case (GPIO_PSR used for get). Is
> there a reason for this, or is it simply a bug that has been around
> since the driver's original creation as I suspect?
> 
> The following patch resolves the issue and causes gpio get to always
> return the proper setting when configured for an input or output:
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 7176743..25b97db 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -458,8 +458,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>         }
> 
>         err = bgpio_init(&port->bgc, &pdev->dev, 4,
> -                        port->base + GPIO_PSR,
> -                        port->base + GPIO_DR, NULL,
> +                        port->base + GPIO_DR,
> +                        NULL, NULL,
>                          port->base + GPIO_GDIR, NULL, 0);
>         if (err)
>                 goto out_iounmap;
> 
> If this looks right to you for all IMX users let me know and I'll post
> a properly formatted patch with commit message.

So the question comes down to, for an output GPIO, whether the value in
GPIO_DR register is always identical to what we see on the pad.  If yes,
your patch makes sense to me.  Otherwise, we have to set SION bit to
read the value of an output GPIO from GPIO_PSR.

Shawn

> 
> Regards,
> 
> Tim

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Benoît Thébaudeau]

Dear Shawn Guo,

On Mon, Jul 14, 2014 at 9:04 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Copy more i.MX people ...
>
> On Fri, Jul 11, 2014 at 08:34:19AM -0700, Tim Harvey wrote:
>> Shawn,
>>
>> I've noticed that for IMX6 the gpio-mxc gpio driver always returns 0
>> for gpio's configured as outputs regardless of if they are outputing
>> high or low.
>>
>> Digging into gpio-mxc.c I see that the basic memory-mapped generic
>> gpio controller is used, but its configured to use GPIO_PSR to obtain
>> the gpio value. For the IMX6 (I can't speak for the other MXC/IMX
>> users of this driver) the reference manual indicates that GPIO_PSR is
>> appropriate only if the GPIO is an input and that GPIO_DR will read
>> the proper state if its configured as an output or an input.
>
>
> The following note can be found in i.MX Reference Manual.
>
>                         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.
>
> That said for an input GPIO, reading GPIO_DR functionally equals to
> reading GPIO_PSR.  So reading either GPIO_DR or GPIO_PSR returns what we
> want to get.
>
> The remain question is what we want to get from an output GPIO, the
> value that register GPIO_DR holds or the one driven on pad.  I *think*
> both should be identical (but not so sure).  If that's the case, reading
> GPIO_DR should just work.  But otherwise, we should read GPIO_PSR with
> SION bit set to get what is actually driven on pad.
>
>>
>> Even before the driver was converted to use the basic memory-mapped
>> generic gpio controller this was the case (GPIO_PSR used for get). Is
>> there a reason for this, or is it simply a bug that has been around
>> since the driver's original creation as I suspect?
>>
>> The following patch resolves the issue and causes gpio get to always
>> return the proper setting when configured for an input or output:
>>
>> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
>> index 7176743..25b97db 100644
>> --- a/drivers/gpio/gpio-mxc.c
>> +++ b/drivers/gpio/gpio-mxc.c
>> @@ -458,8 +458,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>>         }
>>
>>         err = bgpio_init(&port->bgc, &pdev->dev, 4,
>> -                        port->base + GPIO_PSR,
>> -                        port->base + GPIO_DR, NULL,
>> +                        port->base + GPIO_DR,
>> +                        NULL, NULL,
>>                          port->base + GPIO_GDIR, NULL, 0);
>>         if (err)
>>                 goto out_iounmap;
>>
>> If this looks right to you for all IMX users let me know and I'll post
>> a properly formatted patch with commit message.
>
> So the question comes down to, for an output GPIO, whether the value in
> GPIO_DR register is always identical to what we see on the pad.  If yes,
> your patch makes sense to me.  Otherwise, we have to set SION bit to
> read the value of an output GPIO from GPIO_PSR.

Both are not always the same. They should be the same only for sane
hardware. GPIO_DR indicates the level that the output pad tries to
drive, while GPIO_PSR indicates the level sensed on the pad. Thus,
they may differ if there is an external level conflict on the pin.
This difference may be useful in order to test if the board is
behaving as expected or if there is anything going wrong, which is
typically useful for board prototyping.

The software "knows" what it has set on the output pad, so having a
get accessor for GPIO_PSR is more useful than for GPIO_DR, and it
better fits the actual meaning of this function. GPIO_DR would
correspond to the non-existing gpio_get_set_value() function.

Best regards,
Beno?t

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Shawn Guo]

On Mon, Jul 14, 2014 at 02:56:06PM +0200, Beno?t Th?baudeau wrote:
> > So the question comes down to, for an output GPIO, whether the value in
> > GPIO_DR register is always identical to what we see on the pad.  If yes,
> > your patch makes sense to me.  Otherwise, we have to set SION bit to
> > read the value of an output GPIO from GPIO_PSR.
> 
> Both are not always the same. They should be the same only for sane
> hardware. GPIO_DR indicates the level that the output pad tries to
> drive, while GPIO_PSR indicates the level sensed on the pad. Thus,
> they may differ if there is an external level conflict on the pin.
> This difference may be useful in order to test if the board is
> behaving as expected or if there is anything going wrong, which is
> typically useful for board prototyping.
> 
> The software "knows" what it has set on the output pad, so having a
> get accessor for GPIO_PSR is more useful than for GPIO_DR, and it
> better fits the actual meaning of this function. GPIO_DR would
> correspond to the non-existing gpio_get_set_value() function.

Thanks for the input, Beno?t.  That's helpful.

Shawn

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Tim Harvey]

On Mon, Jul 14, 2014 at 7:05 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, Jul 14, 2014 at 02:56:06PM +0200, Beno?t Th?baudeau wrote:
>> > So the question comes down to, for an output GPIO, whether the value in
>> > GPIO_DR register is always identical to what we see on the pad.  If yes,
>> > your patch makes sense to me.  Otherwise, we have to set SION bit to
>> > read the value of an output GPIO from GPIO_PSR.
>>
>> Both are not always the same. They should be the same only for sane
>> hardware. GPIO_DR indicates the level that the output pad tries to
>> drive, while GPIO_PSR indicates the level sensed on the pad. Thus,
>> they may differ if there is an external level conflict on the pin.
>> This difference may be useful in order to test if the board is
>> behaving as expected or if there is anything going wrong, which is
>> typically useful for board prototyping.
>>
>> The software "knows" what it has set on the output pad, so having a
>> get accessor for GPIO_PSR is more useful than for GPIO_DR, and it
>> better fits the actual meaning of this function. GPIO_DR would
>> correspond to the non-existing gpio_get_set_value() function.
>
> Thanks for the input, Beno?t.  That's helpful.
>
> Shawn

Great - thanks for the clarification everyone. I will not submit such
a patch and instead work this from the pinmux perspective setting SION
on GPIO outputs.

That said, can anyone shed light on the subject of what is 'proper'
for GPIO pinmux in the bootloader vs the kernel device-tree? I see
that among the various imx6qdl*.dtsi files, some boards configure
MX6QDL_PAD_GPIO* as 0x80000000 (no config, keeping power-on or
bootloader settings) instead of manually configuring them. Perhaps
there is some rule of thumb that should be followed?

The boards I maintain have several GPIO's that fall under the categories:
 a) go to a connector for off-board use as GPIO
 b) used for a chip enable that no driver may use (like an i2c buffer
for expanding/isolating i2c to an off-board connector for example)
 c) user leds (which 'are' configured for use by the gpio-led driver)
 d) USB hub reset (which the bootloader configures and de-asserts)
 e) probably a few more less-used cases (see
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi#n348
for a good example)

Thanks,

Tim

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Shawn Guo]

On Tue, Jul 15, 2014 at 10:28:52AM -0700, Tim Harvey wrote:
> On Mon, Jul 14, 2014 at 7:05 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> > On Mon, Jul 14, 2014 at 02:56:06PM +0200, Beno?t Th?baudeau wrote:
> >> > So the question comes down to, for an output GPIO, whether the value in
> >> > GPIO_DR register is always identical to what we see on the pad.  If yes,
> >> > your patch makes sense to me.  Otherwise, we have to set SION bit to
> >> > read the value of an output GPIO from GPIO_PSR.
> >>
> >> Both are not always the same. They should be the same only for sane
> >> hardware. GPIO_DR indicates the level that the output pad tries to
> >> drive, while GPIO_PSR indicates the level sensed on the pad. Thus,
> >> they may differ if there is an external level conflict on the pin.
> >> This difference may be useful in order to test if the board is
> >> behaving as expected or if there is anything going wrong, which is
> >> typically useful for board prototyping.
> >>
> >> The software "knows" what it has set on the output pad, so having a
> >> get accessor for GPIO_PSR is more useful than for GPIO_DR, and it
> >> better fits the actual meaning of this function. GPIO_DR would
> >> correspond to the non-existing gpio_get_set_value() function.
> >
> > Thanks for the input, Beno?t.  That's helpful.
> >
> > Shawn
> 
> Great - thanks for the clarification everyone. I will not submit such
> a patch and instead work this from the pinmux perspective setting SION
> on GPIO outputs.
> 
> That said, can anyone shed light on the subject of what is 'proper'
> for GPIO pinmux in the bootloader vs the kernel device-tree? I see
> that among the various imx6qdl*.dtsi files, some boards configure
> MX6QDL_PAD_GPIO* as 0x80000000 (no config, keeping power-on or
> bootloader settings) instead of manually configuring them. Perhaps
> there is some rule of thumb that should be followed?

Although there are quite some cases where 0x80000000 is being used, we
should really configure it properly to avoid the dependency on
bootloader or reset value.

Instead of specifying SION setup in the config cell case by case, I
think we can do that directly in pin function ID for all cases where
GPIO mode is selected.  I just sent a patch to do that.  Please take
a look to see if it works for you.

Shawn

> 
> The boards I maintain have several GPIO's that fall under the categories:
>  a) go to a connector for off-board use as GPIO
>  b) used for a chip enable that no driver may use (like an i2c buffer
> for expanding/isolating i2c to an off-board connector for example)
>  c) user leds (which 'are' configured for use by the gpio-led driver)
>  d) USB hub reset (which the bootloader configures and de-asserts)
>  e) probably a few more less-used cases (see
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi#n348
> for a good example)
> 
> Thanks,
> 
> Tim

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Benoît Thébaudeau]

On Wed, Jul 16, 2014 at 8:01 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Jul 15, 2014 at 10:28:52AM -0700, Tim Harvey wrote:
>> On Mon, Jul 14, 2014 at 7:05 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
>> > On Mon, Jul 14, 2014 at 02:56:06PM +0200, Beno?t Th?baudeau wrote:
>> >> > So the question comes down to, for an output GPIO, whether the value in
>> >> > GPIO_DR register is always identical to what we see on the pad.  If yes,
>> >> > your patch makes sense to me.  Otherwise, we have to set SION bit to
>> >> > read the value of an output GPIO from GPIO_PSR.
>> >>
>> >> Both are not always the same. They should be the same only for sane
>> >> hardware. GPIO_DR indicates the level that the output pad tries to
>> >> drive, while GPIO_PSR indicates the level sensed on the pad. Thus,
>> >> they may differ if there is an external level conflict on the pin.
>> >> This difference may be useful in order to test if the board is
>> >> behaving as expected or if there is anything going wrong, which is
>> >> typically useful for board prototyping.
>> >>
>> >> The software "knows" what it has set on the output pad, so having a
>> >> get accessor for GPIO_PSR is more useful than for GPIO_DR, and it
>> >> better fits the actual meaning of this function. GPIO_DR would
>> >> correspond to the non-existing gpio_get_set_value() function.
>> >
>> > Thanks for the input, Beno?t.  That's helpful.
>> >
>> > Shawn
>>
>> Great - thanks for the clarification everyone. I will not submit such
>> a patch and instead work this from the pinmux perspective setting SION
>> on GPIO outputs.
>>
>> That said, can anyone shed light on the subject of what is 'proper'
>> for GPIO pinmux in the bootloader vs the kernel device-tree? I see
>> that among the various imx6qdl*.dtsi files, some boards configure
>> MX6QDL_PAD_GPIO* as 0x80000000 (no config, keeping power-on or
>> bootloader settings) instead of manually configuring them. Perhaps
>> there is some rule of thumb that should be followed?
>
> Although there are quite some cases where 0x80000000 is being used, we
> should really configure it properly to avoid the dependency on
> bootloader or reset value.
>
> Instead of specifying SION setup in the config cell case by case, I
> think we can do that directly in pin function ID for all cases where
> GPIO mode is selected.  I just sent a patch to do that.  Please take
> a look to see if it works for you.

I don't find this patch on the ML.

Note that enabling SION for all pads in GPIO mode will have some
impact on the power consumption. That's why SION is designed to be
used on a per-pad basis, only when an output GPIO needs to be sensed.

Beno?t

gpio-mxc gpio get always returns 0 for outputs for IMX6

[Shawn Guo]

On Wed, Jul 16, 2014 at 09:14:23PM +0200, Beno?t Th?baudeau wrote:
> > Instead of specifying SION setup in the config cell case by case, I
> > think we can do that directly in pin function ID for all cases where
> > GPIO mode is selected.  I just sent a patch to do that.  Please take
> > a look to see if it works for you.
> 
> I don't find this patch on the ML.

The patch is too big to hit the list.

> 
> Note that enabling SION for all pads in GPIO mode will have some
> impact on the power consumption. That's why SION is designed to be
> used on a per-pad basis, only when an output GPIO needs to be sensed.

Okay, in that case, we will have to leave the configuration option to
board DTS author.

Shawn