Skip to contentSkip to conversation

dm: gpio: mxc: fix mxc_gpio_bank_get_value

PATCH New Peng Fan 5 participants 12 messages Mar 15, 2016 – Apr 11, 2016

Subsystems (2)
Arm i.MX and Layerscape GPIO
lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

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 <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/gpio/mxc_gpio.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/gpio/mxc_gpio.c +3 -0
index 70fe5b6..b6ae3fc 100644
@@ -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(&regs->gpio_dr) >> offset) & 0x01;
+
 	return (readl(&regs->gpio_psr) >> offset) & 0x01;
 }
Signature · 1 line
2.6.2
author
#1 lore raw

PATCH 2/2 dm: gpio: mxc: implement xlate function

To i.MX controller, we use such as "<&gpio1 3 GPIO_ACTIVE_LOW>" for a device to refer a gpio pin in device tree. So need to implement xlate function, to correctly handle gpio flags and offset.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/gpio/mxc_gpio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/gpio/mxc_gpio.c +12 -0
index b6ae3fc..15449d7 100644
@@ -10,10 +10,12 @@
 #include <common.h>
 #include <errno.h>
 #include <dm.h>
+#include <fdtdec.h>
 #include <malloc.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/gpio.h>
 #include <asm/io.h>
+#include <dt-bindings/gpio/gpio.h>
 
 enum mxc_gpio_direction {
 	MXC_GPIO_DIRECTION_IN,

@@ -263,12 +265,22 @@ static int mxc_gpio_get_function(struct udevice *dev, unsigned offset)
 		return GPIOF_INPUT;
 }
 
+static int mxc_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
+			  struct fdtdec_phandle_args *args)
+{
+	desc->offset = args->args[0];
+	desc->flags = args->args[1] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
+
+	return 0;
+}
+
 static const struct dm_gpio_ops gpio_mxc_ops = {
 	.direction_input	= mxc_gpio_direction_input,
 	.direction_output	= mxc_gpio_direction_output,
 	.get_value		= mxc_gpio_get_value,
 	.set_value		= mxc_gpio_set_value,
 	.get_function		= mxc_gpio_get_function,
+	.xlate			= mxc_gpio_xlate,
 };
 
 static int mxc_gpio_probe(struct udevice *dev)
Signature · 1 line
2.6.2
#2 lore raw

PATCH 2/2 dm: gpio: mxc: implement xlate function

Quoted 11 lines · Peng Fan

To i.MX controller, we use such as "<&gpio1 3 GPIO_ACTIVE_LOW>" for a device to refer a gpio pin in device tree. So need to implement xlate function, to correctly handle gpio flags and offset.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/gpio/mxc_gpio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Reviewed-by Simon Glass <[email protected]>
author
#3 lore raw

PATCH 2/2 dm: gpio: mxc: implement xlate function

Quoted 13 lines · Simon Glass
Quoted 11 lines · Peng Fan

To i.MX controller, we use such as "<&gpio1 3 GPIO_ACTIVE_LOW>" for a device to refer a gpio pin in device tree. So need to implement xlate function, to correctly handle gpio flags and offset.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/gpio/mxc_gpio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Quoted 1 line
Reviewed-by Simon Glass <[email protected]>

Will this patch be picked up? Or droped, since Eric is trying to add a common way.

Thanks,
Peng.

#4 lore raw

PATCH 2/2 dm: gpio: mxc: implement xlate function

Hi Peng,

Quoted 14 lines · Peng Fan
Quoted 13 lines · Simon Glass
Quoted 11 lines · Peng Fan

To i.MX controller, we use such as "<&gpio1 3 GPIO_ACTIVE_LOW>" for a device to refer a gpio pin in device tree. So need to implement xlate function, to correctly handle gpio flags and offset.

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/gpio/mxc_gpio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Quoted 5 lines
Quoted 1 line
Reviewed-by Simon Glass <[email protected]>

Will this patch be picked up? Or droped, since Eric is trying to add a common way.

I think this patch should be dropped.

reviewer
#5 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

In reply to Peng Fan
Quoted 3 lines · Peng Fan

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?

author
#6 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Hi Fabio,

Quoted 6 lines · Fabio Estevam
Quoted 3 lines · Peng Fan

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.

reviewer
#7 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Hi Peng,

Quoted 5 lines · Peng Fan

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?

author
#8 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Quoted 4 lines · Fabio Estevam

Hi Peng,

On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <[email protected]> wrote:

Quoted 8 lines
Quoted 5 lines

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.

#9 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Hi Peng,

Quoted 30 lines · Peng Fan
Quoted 4 lines · Fabio Estevam

Hi Peng,

On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <[email protected]> wrote:

Quoted 8 lines
Quoted 5 lines

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,

Quoted 9 lines
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 <[email protected]>
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.

Quoted 4 lines

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

Signature · 5 lines
=====================================================================
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 at denx.de
=====================================================================
author
#10 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Hi Stefano,

Quoted 50 lines · Stefano Babic

Hi Peng,

Quoted 30 lines · Peng Fan
Quoted 4 lines · Fabio Estevam

Hi Peng,

On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <[email protected]> wrote:

Quoted 8 lines
Quoted 5 lines

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,

Quoted 9 lines
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 <[email protected]>
Date:   Mon Aug 20 10:55:41 2012 +0000
Quoted 8 lines

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.

Quoted 16 lines
Quoted 6 lines

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

Signature · 5 lines
=====================================================================
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 at denx.de
=====================================================================
#11 lore raw

PATCH 1/2 dm: gpio: mxc: fix mxc_gpio_bank_get_value

Hi Peng,

Quoted 56 lines · Peng Fan

Hi Stefano,

Quoted 53 lines · Stefano Babic

Hi Peng,

Quoted 30 lines · Peng Fan
Quoted 4 lines · Fabio Estevam

Hi Peng,

On Tue, Mar 15, 2016 at 10:27 PM, Peng Fan <[email protected]> wrote:

Quoted 8 lines
Quoted 5 lines

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,

Quoted 9 lines
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 <[email protected]>
Date:   Mon Aug 20 10:55:41 2012 +0000

mxc: Make gpio_get_value() use PSR

Quoted 7 lines
Quoted 5 lines

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)

Quoted 19 lines

Regards,
Peng.

Quoted 16 lines
Quoted 6 lines

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 at denx.de
=====================================================================

Regards,
Simon