| Message ID | CAJ+vNU3w9Oi+dErmy9x8g6ps=eLHLNLO-w7=gn_8JtY4Kab6JQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. ---
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:
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.
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
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
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
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
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
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
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
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;