| Message ID | 1458020937-994-1-git-send-email-van.freenix@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
On Tue, Mar 15, 2016 at 2:48 AM, Peng Fan <van.freenix@gmail.com> wrote: > When configured a gpio to output direction, directly reading PSR register > can not return the output value, since we did not set SION bit for gpio > iomux. So, we can use data register to reflect what value is outputed. Then why not simply set the SION bit instead?
Hi Fabio, On Tue, Mar 15, 2016 at 09:53:15AM -0300, Fabio Estevam wrote: >On Tue, Mar 15, 2016 at 2:48 AM, Peng Fan <van.freenix@gmail.com> wrote: >> When configured a gpio to output direction, directly reading PSR register >> can not return the output value, since we did not set SION bit for gpio >> iomux. So, we can use data register to reflect what value is outputed. > >Then why not simply set the SION bit instead? If set the SION bit, we need to change the pinmux settings in device tree, however device tree are introduced from linux kernel. I would not like to change the pinmux setting value. So I choose to use data register but not PSR register. Regards, Peng.
Hi Peng, On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <van.freenix@gmail.com> wrote: > If set the SION bit, we need to change the pinmux settings in device tree, > however device tree are introduced from linux kernel. I would not like > to change the pinmux setting value. > > So I choose to use data register but not PSR register. Haven't checked the kernel driver, but just curious: how does the kernel handle this?
On Wed, Mar 16, 2016 at 09:48:24AM -0300, Fabio Estevam wrote: >Hi Peng, > >On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <van.freenix@gmail.com> wrote: > >> If set the SION bit, we need to change the pinmux settings in device tree, >> however device tree are introduced from linux kernel. I would not like >> to change the pinmux setting value. >> >> So I choose to use data register but not PSR register. > >Haven't checked the kernel driver, but just curious: how does the >kernel handle this? bgc->gc.get = bgpio_get; The get method: * @get: returns value for signal "offset"; for output signals this * returns either the value actually sensed, or zero 138 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio) 139 { 140 struct bgpio_chip *bgc = to_bgpio_chip(gc); 141 142 return !!(bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio)); 143 } The gpio subsystem use reg_data. The reg_data is GPIO_PSR for gpio-mxc.c: 450 err = bgpio_init(&port->bgc, &pdev->dev, 4, 451 port->base + GPIO_PSR, 452 port->base + GPIO_DR, NULL, 453 port->base + GPIO_GDIR, NULL, 0); So we know that kernel gpio-mxc use GPIO_PSR for reading data, but as the comments for get method "for output signals this returns either the value actually sensed, or zero", if no SION bit set, reading PSR will return 0 with direction output. But in U-Boot, the regulator driver needs to get the value when direction is configured as output, we can use data register here or configure SION in pinmux. But I prefer using data register here. Thanks, Peng.
Hi Peng, On 17/03/2016 02:44, Peng Fan wrote: > On Wed, Mar 16, 2016 at 09:48:24AM -0300, Fabio Estevam wrote: >> Hi Peng, >> >> On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <van.freenix@gmail.com> wrote: >> >>> If set the SION bit, we need to change the pinmux settings in device tree, >>> however device tree are introduced from linux kernel. I would not like >>> to change the pinmux setting value. >>> >>> So I choose to use data register but not PSR register. >> >> Haven't checked the kernel driver, but just curious: how does the >> kernel handle this? > > bgc->gc.get = bgpio_get; > > The get method: > * @get: returns value for signal "offset"; for output signals this > * returns either the value actually sensed, or zero > > 138 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio) > 139 { > 140 struct bgpio_chip *bgc = to_bgpio_chip(gc); > 141 > 142 return !!(bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio)); > 143 } > > The gpio subsystem use reg_data. The reg_data is GPIO_PSR for gpio-mxc.c: > > 450 err = bgpio_init(&port->bgc, &pdev->dev, 4, > 451 port->base + GPIO_PSR, > 452 port->base + GPIO_DR, NULL, > 453 port->base + GPIO_GDIR, NULL, 0); > > So we know that kernel gpio-mxc use GPIO_PSR for reading data, but > as the comments for get method "for output signals this returns > either the value actually sensed, or zero", if no SION bit set, reading > PSR will return 0 with direction output. > There was already a debate in the past about this point. You can take a look at the commit: commit 5dafa4543c399d329c7b01df1afa98437861cac0 Author: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Date: Mon Aug 20 10:55:41 2012 +0000 mxc: Make gpio_get_value() use PSR There were good reasons to switch to PSR (mxc-gpio used DR before) - we cannot switch any time back to solve a single issue. I would like to remain compliant with Linux - your change breaks also boards that rely on this behavior. > But in U-Boot, the regulator driver needs to get the value when direction is > configured as output, we can use data register here or configure SION in pinmux. > But I prefer using data register here. > Best regards, Stefano
Hi Stefano, On Sun, Apr 03, 2016 at 08:09:35PM +0200, Stefano Babic wrote: >Hi Peng, > >On 17/03/2016 02:44, Peng Fan wrote: >> On Wed, Mar 16, 2016 at 09:48:24AM -0300, Fabio Estevam wrote: >>> Hi Peng, >>> >>> On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <van.freenix@gmail.com> wrote: >>> >>>> If set the SION bit, we need to change the pinmux settings in device tree, >>>> however device tree are introduced from linux kernel. I would not like >>>> to change the pinmux setting value. >>>> >>>> So I choose to use data register but not PSR register. >>> >>> Haven't checked the kernel driver, but just curious: how does the >>> kernel handle this? >> >> bgc->gc.get = bgpio_get; >> >> The get method: >> * @get: returns value for signal "offset"; for output signals this >> * returns either the value actually sensed, or zero >> >> 138 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio) >> 139 { >> 140 struct bgpio_chip *bgc = to_bgpio_chip(gc); >> 141 >> 142 return !!(bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio)); >> 143 } >> >> The gpio subsystem use reg_data. The reg_data is GPIO_PSR for gpio-mxc.c: >> >> 450 err = bgpio_init(&port->bgc, &pdev->dev, 4, >> 451 port->base + GPIO_PSR, >> 452 port->base + GPIO_DR, NULL, >> 453 port->base + GPIO_GDIR, NULL, 0); >> >> So we know that kernel gpio-mxc use GPIO_PSR for reading data, but >> as the comments for get method "for output signals this returns >> either the value actually sensed, or zero", if no SION bit set, reading >> PSR will return 0 with direction output. >> > >There was already a debate in the past about this point. You can take a >look at the commit: > >commit 5dafa4543c399d329c7b01df1afa98437861cac0 >Author: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> >Date: Mon Aug 20 10:55:41 2012 +0000 > > mxc: Make gpio_get_value() use PSR > > >There were good reasons to switch to PSR (mxc-gpio used DR before) - we >cannot switch any time back to solve a single issue. > >I would like to remain compliant with Linux - your change breaks also >boards that rely on this behavior. Thanks for this. I'll look at whether I can have some new way. Regards, Peng. > > >> But in U-Boot, the regulator driver needs to get the value when direction is >> configured as output, we can use data register here or configure SION in pinmux. >> But I prefer using data register here. >> > >Best regards, >Stefano > >-- >===================================================================== >DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de >=====================================================================
Hi Peng, On 4 April 2016 at 23:04, Peng Fan <van.freenix@gmail.com> wrote: > Hi Stefano, > > On Sun, Apr 03, 2016 at 08:09:35PM +0200, Stefano Babic wrote: >>Hi Peng, >> >>On 17/03/2016 02:44, Peng Fan wrote: >>> On Wed, Mar 16, 2016 at 09:48:24AM -0300, Fabio Estevam wrote: >>>> Hi Peng, >>>> >>>> On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <van.freenix@gmail.com> wrote: >>>> >>>>> If set the SION bit, we need to change the pinmux settings in device tree, >>>>> however device tree are introduced from linux kernel. I would not like >>>>> to change the pinmux setting value. >>>>> >>>>> So I choose to use data register but not PSR register. >>>> >>>> Haven't checked the kernel driver, but just curious: how does the >>>> kernel handle this? >>> >>> bgc->gc.get = bgpio_get; >>> >>> The get method: >>> * @get: returns value for signal "offset"; for output signals this >>> * returns either the value actually sensed, or zero >>> >>> 138 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio) >>> 139 { >>> 140 struct bgpio_chip *bgc = to_bgpio_chip(gc); >>> 141 >>> 142 return !!(bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio)); >>> 143 } >>> >>> The gpio subsystem use reg_data. The reg_data is GPIO_PSR for gpio-mxc.c: >>> >>> 450 err = bgpio_init(&port->bgc, &pdev->dev, 4, >>> 451 port->base + GPIO_PSR, >>> 452 port->base + GPIO_DR, NULL, >>> 453 port->base + GPIO_GDIR, NULL, 0); >>> >>> So we know that kernel gpio-mxc use GPIO_PSR for reading data, but >>> as the comments for get method "for output signals this returns >>> either the value actually sensed, or zero", if no SION bit set, reading >>> PSR will return 0 with direction output. >>> >> >>There was already a debate in the past about this point. You can take a >>look at the commit: >> >>commit 5dafa4543c399d329c7b01df1afa98437861cac0 >>Author: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> >>Date: Mon Aug 20 10:55:41 2012 +0000 >> >> mxc: Make gpio_get_value() use PSR >> >> >>There were good reasons to switch to PSR (mxc-gpio used DR before) - we >>cannot switch any time back to solve a single issue. >> >>I would like to remain compliant with Linux - your change breaks also >>boards that rely on this behavior. > > Thanks for this. I'll look at whether I can have some new way. Two possible ideas: 1. Cache the output state in the driver so you can return it 2. Add a new get_output_value() method to the uclass (although maybe I misunderstand the problem here) > > Regards, > Peng. >> >> >>> But in U-Boot, the regulator driver needs to get the value when direction is >>> configured as output, we can use data register here or configure SION in pinmux. >>> But I prefer using data register here. >>> >> >>Best regards, >>Stefano >> >>-- >>===================================================================== >>DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >>HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >>Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de >>===================================================================== Regards, Simon
diff --git a/drivers/gpio/mxc_gpio.c b/drivers/gpio/mxc_gpio.c index 70fe5b6..b6ae3fc 100644 --- a/drivers/gpio/mxc_gpio.c +++ b/drivers/gpio/mxc_gpio.c @@ -201,6 +201,9 @@ static void mxc_gpio_bank_set_value(struct gpio_regs *regs, int offset, static int mxc_gpio_bank_get_value(struct gpio_regs *regs, int offset) { + if (mxc_gpio_is_output(regs, offset)) + return (readl(®s->gpio_dr) >> offset) & 0x01; + return (readl(®s->gpio_psr) >> offset) & 0x01; }
When configured a gpio to output direction, directly reading PSR register can not return the output value, since we did not set SION bit for gpio iomux. So, we can use data register to reflect what value is outputed. If not, "regulator status" always return disabled, even if already "regulator enable": " => regulator enable => regulator status Regulator VSD_3V3 status: * enable: 0 (false) * value uV: 3300000 * current uA: No data available (err: -61) * mode id: Function not implemented (err: -38) " Signed-off-by: Peng Fan <van.freenix@gmail.com> Cc: Simon Glass <sjg@chromium.org> Cc: Stefano Babic <sbabic@denx.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> --- drivers/gpio/mxc_gpio.c | 3 +++ 1 file changed, 3 insertions(+)