[PATCH 0/3] gpio: intel_gpio: Fix Intel gpio driver

[PATCH 0/3] gpio: intel_gpio: Fix Intel gpio driver

[Wolfgang Wallner]

This series fixes some issues in the Intel gpio driver.
I have tested it on an Apollo Lake device, where U-Boot is booted as a
coreboot payload.


Wolfgang Wallner (3):
  gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()
  gpio: intel_gpio: Clear tx state bit when setting output
  gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

 drivers/gpio/intel_gpio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.24.1

[PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Wolfgang Wallner]

The function pcr_clrsetbits32() expects a device with a P2SB parent
device.

The currently passed device 'dev' is a gpio-controller with a device
'pinctrl' as parent. This does not match the expectations of
pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.

Pass the 'pinctrl' device instead of the 'dev' device to
pcr_clrsetbits32().

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
---

 drivers/gpio/intel_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index 4bf1c9ddc4..db63115628 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
 	struct udevice *pinctrl = dev_get_parent(dev);
 	uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
 
-	pcr_clrsetbits32(dev, config_offset,
+	pcr_clrsetbits32(pinctrl, config_offset,
 			 PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
 				  PAD_CFG0_TX_DISABLE,
 			 PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
-- 
2.24.1

[PATCH 2/3] gpio: intel_gpio: Clear tx state bit when setting output

[Wolfgang Wallner]

Add missing 'PAD_CFG0_TX_STATE' to the clear mask for pcr_clrsetbits32().
Otherwise this bit cannot be cleared again after it has been set once.

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
---

 drivers/gpio/intel_gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index db63115628..ec1a315ae2 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -41,7 +41,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
 
 	pcr_clrsetbits32(pinctrl, config_offset,
 			 PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
-				  PAD_CFG0_TX_DISABLE,
+				  PAD_CFG0_TX_DISABLE | PAD_CFG0_TX_STATE,
 			 PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
 				  (value ? PAD_CFG0_TX_STATE : 0));
 
-- 
2.24.1

[PATCH 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Wolfgang Wallner]

Fix the following in intel_gpio_get_value():

 * The value of the register is contained in the variable 'reg', not in
   'mode'. The variable 'mode' contains only the configuration whether
   the gpio is currently an input or an output.

 * The correct bitmasks for the input and output value are
   PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
   Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
   PAD_CFG0_TX_STATE_BIT.

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>

---

 drivers/gpio/intel_gpio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
index ec1a315ae2..ef47b92ca4 100644
--- a/drivers/gpio/intel_gpio.c
+++ b/drivers/gpio/intel_gpio.c
@@ -59,9 +59,9 @@ static int intel_gpio_get_value(struct udevice *dev, uint offset)
 	if (!mode) {
 		rx_tx = reg & (PAD_CFG0_TX_DISABLE | PAD_CFG0_RX_DISABLE);
 		if (rx_tx == PAD_CFG0_TX_DISABLE)
-			return mode & PAD_CFG0_RX_STATE_BIT ? 1 : 0;
+			return reg & PAD_CFG0_RX_STATE ? 1 : 0;
 		else if (rx_tx == PAD_CFG0_RX_DISABLE)
-			return mode & PAD_CFG0_TX_STATE_BIT ? 1 : 0;
+			return reg & PAD_CFG0_TX_STATE ? 1 : 0;
 	}
 
 	return 0;
-- 
2.24.1

[PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Simon Glass]

On Fri, 10 Jan 2020 at 00:35, Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> The function pcr_clrsetbits32() expects a device with a P2SB parent
> device.
>
> The currently passed device 'dev' is a gpio-controller with a device
> 'pinctrl' as parent. This does not match the expectations of
> pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.
>
> Pass the 'pinctrl' device instead of the 'dev' device to
> pcr_clrsetbits32().
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
>
>  drivers/gpio/intel_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/3] gpio: intel_gpio: Clear tx state bit when setting output

[Simon Glass]

On Fri, 10 Jan 2020 at 00:35, Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Add missing 'PAD_CFG0_TX_STATE' to the clear mask for pcr_clrsetbits32().
> Otherwise this bit cannot be cleared again after it has been set once.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
>
>  drivers/gpio/intel_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Simon Glass]

On Fri, 10 Jan 2020 at 00:35, Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Fix the following in intel_gpio_get_value():
>
>  * The value of the register is contained in the variable 'reg', not in
>    'mode'. The variable 'mode' contains only the configuration whether
>    the gpio is currently an input or an output.
>
>  * The correct bitmasks for the input and output value are
>    PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
>    Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
>    PAD_CFG0_TX_STATE_BIT.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>
> ---
>
>  drivers/gpio/intel_gpio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Bin Meng]

Hi Wolfgang,

On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> The function pcr_clrsetbits32() expects a device with a P2SB parent
> device.
>
> The currently passed device 'dev' is a gpio-controller with a device
> 'pinctrl' as parent. This does not match the expectations of
> pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.

typo: the 'pinctrl' device

>
> Pass the 'pinctrl' device instead of the 'dev' device to
> pcr_clrsetbits32().
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
>
>  drivers/gpio/intel_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> index 4bf1c9ddc4..db63115628 100644
> --- a/drivers/gpio/intel_gpio.c
> +++ b/drivers/gpio/intel_gpio.c
> @@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
>         struct udevice *pinctrl = dev_get_parent(dev);
>         uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
>
> -       pcr_clrsetbits32(dev, config_offset,
> +       pcr_clrsetbits32(pinctrl, config_offset,
>                          PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
>                                   PAD_CFG0_TX_DISABLE,
>                          PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

But I think we should also fix intel_gpio_set_value() where dev is
passed instead of pinctrl.

Regards,
Bin

[PATCH 2/3] gpio: intel_gpio: Clear tx state bit when setting output

[Bin Meng]

On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Add missing 'PAD_CFG0_TX_STATE' to the clear mask for pcr_clrsetbits32().
> Otherwise this bit cannot be cleared again after it has been set once.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
>
>  drivers/gpio/intel_gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 3/3] gpio: intel_gpio: Fix register/bit offsets intel_gpio_get_value()

[Bin Meng]

On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Fix the following in intel_gpio_get_value():
>
>  * The value of the register is contained in the variable 'reg', not in
>    'mode'. The variable 'mode' contains only the configuration whether
>    the gpio is currently an input or an output.
>
>  * The correct bitmasks for the input and output value are
>    PAD_CFG0_RX_STATE and PAD_CFG0_TX_STATE.
>    Use them instead of the currently used PAD_CFG0_RX_STATE_BIT and
>    PAD_CFG0_TX_STATE_BIT.
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>
> ---
>
>  drivers/gpio/intel_gpio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Antwort: Re: [PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Wolfgang Wallner]

Hi Bin,

-----"Bin Meng" <bmeng.cn@gmail.com> schrieb: -----
> Hi Wolfgang,
>
> On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > The function pcr_clrsetbits32() expects a device with a P2SB parent
> > device.
> >
> > The currently passed device 'dev' is a gpio-controller with a device
> > 'pinctrl' as parent. This does not match the expectations of
> > pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.
>
> typo: the 'pinctrl' device

Thanks, I will fix that in V2.

> >
> > Pass the 'pinctrl' device instead of the 'dev' device to
> > pcr_clrsetbits32().
> >
> > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > ---
> >
> >  drivers/gpio/intel_gpio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > index 4bf1c9ddc4..db63115628 100644
> > --- a/drivers/gpio/intel_gpio.c
> > +++ b/drivers/gpio/intel_gpio.c
> > @@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
> >         struct udevice *pinctrl = dev_get_parent(dev);
> >         uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
> >
> > -       pcr_clrsetbits32(dev, config_offset,
> > +       pcr_clrsetbits32(pinctrl, config_offset,
> >                          PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
> >                                   PAD_CFG0_TX_DISABLE,
> >                          PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> But I think we should also fix intel_gpio_set_value() where dev is
> passed instead of pinctrl.

Good catch, I will also include a fix for that in V2.

Those errors are easy to make, hard to find, and currently they are not caught,
except when something goes wrong during runtime. Should we add additional
assert()-lines or checks to catch such errors? If so, where?
For the P2SB driver I would say _pcr_reg_address() would be a good place, as
this is the function that relies on the uclass of the parent device.

regards, Wolfgang

[PATCH 1/3] gpio: intel_gpio: Pass pinctrl device to pcr_clrsetbits32()

[Bin Meng]

Hi Wolfgang,

On Mon, Feb 3, 2020 at 5:44 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Hi Bin,
>
> -----"Bin Meng" <bmeng.cn@gmail.com> schrieb: -----
> > Hi Wolfgang,
> >
> > On Fri, Jan 10, 2020 at 3:35 PM Wolfgang Wallner
> > <wolfgang.wallner@br-automation.com> wrote:
> > >
> > > The function pcr_clrsetbits32() expects a device with a P2SB parent
> > > device.
> > >
> > > The currently passed device 'dev' is a gpio-controller with a device
> > > 'pinctrl' as parent. This does not match the expectations of
> > > pcr_clrsetbits32(). But he 'pinctrl'-device has a P2SB as parent.
> >
> > typo: the 'pinctrl' device
>
> Thanks, I will fix that in V2.
>
> > >
> > > Pass the 'pinctrl' device instead of the 'dev' device to
> > > pcr_clrsetbits32().
> > >
> > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > > ---
> > >
> > >  drivers/gpio/intel_gpio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > > index 4bf1c9ddc4..db63115628 100644
> > > --- a/drivers/gpio/intel_gpio.c
> > > +++ b/drivers/gpio/intel_gpio.c
> > > @@ -39,7 +39,7 @@ static int intel_gpio_direction_output(struct udevice *dev, uint offset,
> > >         struct udevice *pinctrl = dev_get_parent(dev);
> > >         uint config_offset = intel_pinctrl_get_config_reg_addr(pinctrl, offset);
> > >
> > > -       pcr_clrsetbits32(dev, config_offset,
> > > +       pcr_clrsetbits32(pinctrl, config_offset,
> > >                          PAD_CFG0_MODE_MASK | PAD_CFG0_RX_STATE |
> > >                                   PAD_CFG0_TX_DISABLE,
> > >                          PAD_CFG0_MODE_GPIO | PAD_CFG0_RX_DISABLE |
> >
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > But I think we should also fix intel_gpio_set_value() where dev is
> > passed instead of pinctrl.
>
> Good catch, I will also include a fix for that in V2.
>
> Those errors are easy to make, hard to find, and currently they are not caught,
> except when something goes wrong during runtime. Should we add additional
> assert()-lines or checks to catch such errors? If so, where?
> For the P2SB driver I would say _pcr_reg_address() would be a good place, as

That sounds good to me. Thanks!

> this is the function that relies on the uclass of the parent device.

Regards,
Bin