[PATCH 0/2] pinctrl: pinctrl-imx: add gpio support for mx7ulp

[PATCH 0/2] pinctrl: pinctrl-imx: add gpio support for mx7ulp

[Dong Aisheng]

This patch series intends to add support for mx7ulp based on exist
code as imx7ulp pinctrl is slightly a bit different from Vybrid.

First, iMX ULP has different IOB/OBE shift from Vibrid, so we make
it as a SoC property.

Second, current code assumes MUX0 is GPIO function. It's true for Vybrid,
but not on ULP as Mux 1 is GPIO on ULP. So the patch corrects it
a bit based on the parsed imx_pin->mux_mode from device tree.

This patch series depends on the below one sent out a few days ago.
[PATCH 0/5] pinctrl: imx: add generic pin config and imx7ulp support
https://www.spinics.net/lists/arm-kernel/msg580737.html

Dong Aisheng (2):
  pinctrl: pinctrl-imx: add IBE and OBE SoC property
  pinctrl: pinctrl-imx: do not assume mux 0 is gpio

 drivers/pinctrl/freescale/pinctrl-imx.c     | 11 ++++++-----
 drivers/pinctrl/freescale/pinctrl-imx.h     |  2 ++
 drivers/pinctrl/freescale/pinctrl-imx7ulp.c |  2 ++
 drivers/pinctrl/freescale/pinctrl-vf610.c   |  2 ++
 4 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 0/2] pinctrl: pinctrl-imx: add gpio support for mx7ulp

[Dong Aisheng]

This patch series intends to add support for mx7ulp based on exist
code as imx7ulp pinctrl is slightly a bit different from Vybrid.

First, iMX ULP has different IOB/OBE shift from Vibrid, so we make
it as a SoC property.

Second, current code assumes MUX0 is GPIO function. It's true for Vybrid,
but not on ULP as Mux 1 is GPIO on ULP. So the patch corrects it
a bit based on the parsed imx_pin->mux_mode from device tree.

This patch series depends on the below one sent out a few days ago.
[PATCH 0/5] pinctrl: imx: add generic pin config and imx7ulp support
https://www.spinics.net/lists/arm-kernel/msg580737.html

Dong Aisheng (2):
  pinctrl: pinctrl-imx: add IBE and OBE SoC property
  pinctrl: pinctrl-imx: do not assume mux 0 is gpio

 drivers/pinctrl/freescale/pinctrl-imx.c     | 11 ++++++-----
 drivers/pinctrl/freescale/pinctrl-imx.h     |  2 ++
 drivers/pinctrl/freescale/pinctrl-imx7ulp.c |  2 ++
 drivers/pinctrl/freescale/pinctrl-vf610.c   |  2 ++
 4 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Dong Aisheng]

iMX ULP has different IOB/OBE shift from Vibrid, so let's make
it a SoC property.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
 drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
 drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
 drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 57e1f7a..0d6aaca 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	u32 reg;
 
 	/*
-	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
-	 * They are part of the shared mux/conf register.
+	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
+	 * (IBE/OBE) They are part of the shared mux/conf register.
 	 */
 	if (!(info->flags & SHARE_MUX_CONF_REG))
 		return 0;
@@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	/* IBE always enabled allows us to read the value "on the wire" */
 	reg = readl(ipctl->base + pin_reg->mux_reg);
 	if (input)
-		reg &= ~0x2;
+		reg = (reg & ~info->obe_bit) | info->ibe_bit;
 	else
-		reg |= 0x2;
+		reg = (reg & ~info->ibe_bit) | info->obe_bit;
 	writel(reg, ipctl->base + pin_reg->mux_reg);
 
 	return 0;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index eb0ce95..9ded65d 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
 	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
 	unsigned int mux_mask;
 	u8 mux_shift;
+	u32 ibe_bit;
+	u32 obe_bit;
 
 	/* generic pinconf */
 	bool generic_pinconf;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
index dead416..f724a01 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
@@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info imx7ulp_pinctrl_info = {
 	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
 	.mux_mask = 0xf00,
 	.mux_shift = 8,
+	.ibe_bit = BIT(16),
+	.obe_bit = BIT(17),
 	.generic_pinconf = true,
 	.custom_params = imx7ulp_cfg_params,
 	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params),
diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c
index 3bd8556..c0823f9 100644
--- a/drivers/pinctrl/freescale/pinctrl-vf610.c
+++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
@@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
 	.mux_mask = 0x700000,
 	.mux_shift = 20,
+	.ibe_bit = BIT(0),
+	.obe_bit = BIT(1),
 };
 
 static const struct of_device_id vf610_pinctrl_of_match[] = {
-- 
2.7.4

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Dong Aisheng]

iMX ULP has different IOB/OBE shift from Vibrid, so let's make
it a SoC property.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
 drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
 drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
 drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 57e1f7a..0d6aaca 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	u32 reg;
 
 	/*
-	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
-	 * They are part of the shared mux/conf register.
+	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
+	 * (IBE/OBE) They are part of the shared mux/conf register.
 	 */
 	if (!(info->flags & SHARE_MUX_CONF_REG))
 		return 0;
@@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 	/* IBE always enabled allows us to read the value "on the wire" */
 	reg = readl(ipctl->base + pin_reg->mux_reg);
 	if (input)
-		reg &= ~0x2;
+		reg = (reg & ~info->obe_bit) | info->ibe_bit;
 	else
-		reg |= 0x2;
+		reg = (reg & ~info->ibe_bit) | info->obe_bit;
 	writel(reg, ipctl->base + pin_reg->mux_reg);
 
 	return 0;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index eb0ce95..9ded65d 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
 	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
 	unsigned int mux_mask;
 	u8 mux_shift;
+	u32 ibe_bit;
+	u32 obe_bit;
 
 	/* generic pinconf */
 	bool generic_pinconf;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
index dead416..f724a01 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
@@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info imx7ulp_pinctrl_info = {
 	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
 	.mux_mask = 0xf00,
 	.mux_shift = 8,
+	.ibe_bit = BIT(16),
+	.obe_bit = BIT(17),
 	.generic_pinconf = true,
 	.custom_params = imx7ulp_cfg_params,
 	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params),
diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c
index 3bd8556..c0823f9 100644
--- a/drivers/pinctrl/freescale/pinctrl-vf610.c
+++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
@@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
 	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
 	.mux_mask = 0x700000,
 	.mux_shift = 20,
+	.ibe_bit = BIT(0),
+	.obe_bit = BIT(1),
 };
 
 static const struct of_device_id vf610_pinctrl_of_match[] = {
-- 
2.7.4

Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Stefan Agner]

On 2017-05-14 23:48, Dong Aisheng wrote:
> iMX ULP has different IOB/OBE shift from Vibrid, so let's make

s/Vibrid/Vybrid

> it a SoC property.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
>  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
>  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
>  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
>  4 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 57e1f7a..0d6aaca 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> pinctrl_dev *pctldev,
>  	u32 reg;
>  
>  	/*
> -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> -	 * They are part of the shared mux/conf register.
> +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> +	 * (IBE/OBE) They are part of the shared mux/conf register.
>  	 */
>  	if (!(info->flags & SHARE_MUX_CONF_REG))
>  		return 0;
> @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> pinctrl_dev *pctldev,
>  	/* IBE always enabled allows us to read the value "on the wire" */
>  	reg = readl(ipctl->base + pin_reg->mux_reg);
>  	if (input)
> -		reg &= ~0x2;
> +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
>  	else
> -		reg |= 0x2;
> +		reg = (reg & ~info->ibe_bit) | info->obe_bit;

Why disabling IBE bit now? As the comment above states, it is really
handy to leave the input buffer enabled so we can read back the true
value on the wire...

--
Stefan

>  	writel(reg, ipctl->base + pin_reg->mux_reg);
>  
>  	return 0;
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> b/drivers/pinctrl/freescale/pinctrl-imx.h
> index eb0ce95..9ded65d 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
>  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
>  	unsigned int mux_mask;
>  	u8 mux_shift;
> +	u32 ibe_bit;
> +	u32 obe_bit;
>  
>  	/* generic pinconf */
>  	bool generic_pinconf;
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> index dead416..f724a01 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info imx7ulp_pinctrl_info = {
>  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
>  	.mux_mask = 0xf00,
>  	.mux_shift = 8,
> +	.ibe_bit = BIT(16),
> +	.obe_bit = BIT(17),
>  	.generic_pinconf = true,
>  	.custom_params = imx7ulp_cfg_params,
>  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params),
> diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c
> b/drivers/pinctrl/freescale/pinctrl-vf610.c
> index 3bd8556..c0823f9 100644
> --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
>  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
>  	.mux_mask = 0x700000,
>  	.mux_shift = 20,
> +	.ibe_bit = BIT(0),
> +	.obe_bit = BIT(1),
>  };
>  
>  static const struct of_device_id vf610_pinctrl_of_match[] = {

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Stefan Agner]

On 2017-05-14 23:48, Dong Aisheng wrote:
> iMX ULP has different IOB/OBE shift from Vibrid, so let's make

s/Vibrid/Vybrid

> it a SoC property.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
>  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
>  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
>  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
>  4 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 57e1f7a..0d6aaca 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> pinctrl_dev *pctldev,
>  	u32 reg;
>  
>  	/*
> -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> -	 * They are part of the shared mux/conf register.
> +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> +	 * (IBE/OBE) They are part of the shared mux/conf register.
>  	 */
>  	if (!(info->flags & SHARE_MUX_CONF_REG))
>  		return 0;
> @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> pinctrl_dev *pctldev,
>  	/* IBE always enabled allows us to read the value "on the wire" */
>  	reg = readl(ipctl->base + pin_reg->mux_reg);
>  	if (input)
> -		reg &= ~0x2;
> +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
>  	else
> -		reg |= 0x2;
> +		reg = (reg & ~info->ibe_bit) | info->obe_bit;

Why disabling IBE bit now? As the comment above states, it is really
handy to leave the input buffer enabled so we can read back the true
value on the wire...

--
Stefan

>  	writel(reg, ipctl->base + pin_reg->mux_reg);
>  
>  	return 0;
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> b/drivers/pinctrl/freescale/pinctrl-imx.h
> index eb0ce95..9ded65d 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
>  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
>  	unsigned int mux_mask;
>  	u8 mux_shift;
> +	u32 ibe_bit;
> +	u32 obe_bit;
>  
>  	/* generic pinconf */
>  	bool generic_pinconf;
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> index dead416..f724a01 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info imx7ulp_pinctrl_info = {
>  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
>  	.mux_mask = 0xf00,
>  	.mux_shift = 8,
> +	.ibe_bit = BIT(16),
> +	.obe_bit = BIT(17),
>  	.generic_pinconf = true,
>  	.custom_params = imx7ulp_cfg_params,
>  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params),
> diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c
> b/drivers/pinctrl/freescale/pinctrl-vf610.c
> index 3bd8556..c0823f9 100644
> --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = {
>  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
>  	.mux_mask = 0x700000,
>  	.mux_shift = 20,
> +	.ibe_bit = BIT(0),
> +	.obe_bit = BIT(1),
>  };
>  
>  static const struct of_device_id vf610_pinctrl_of_match[] = {

RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Tuesday, May 16, 2017 1:11 AM
> To: A.S. Dong
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> On 2017-05-14 23:48, Dong Aisheng wrote:
> > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> 
> s/Vibrid/Vybrid
> 
> > it a SoC property.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > Cc: Bai Ping <ping.bai@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> >  4 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index 57e1f7a..0d6aaca 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > pinctrl_dev *pctldev,
> >  	u32 reg;
> >
> >  	/*
> > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> > -	 * They are part of the shared mux/conf register.
> > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> >  	 */
> >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> >  		return 0;
> > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > pinctrl_dev *pctldev,
> >  	/* IBE always enabled allows us to read the value "on the wire" */
> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >  	if (input)
> > -		reg &= ~0x2;
> > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> >  	else
> > -		reg |= 0x2;
> > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> 
> Why disabling IBE bit now? As the comment above states, it is really handy
> to leave the input buffer enabled so we can read back the true value on
> the wire...
> 

Does Vybrid reply on this bit to read the status of output from
Port Data Input Register (GPIOx_PDIR)?

Then, it is a bit strange that read status from input register when the GPIO is
Actually configured as output.

Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.

For MX7ULP, we will read input or output register accordingly by checking
GPIO direction register (PDDR) and we will not enable Input buffer if the GPIO
is configured as output, this also save a bit power.

I know Vybrid has no PDDR, probably that's why you enable IBE by default
always.

Regards
Dong Aisheng

> --
> Stefan
> 
> >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> >  	return 0;
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> > b/drivers/pinctrl/freescale/pinctrl-imx.h
> > index eb0ce95..9ded65d 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> >  	unsigned int mux_mask;
> >  	u8 mux_shift;
> > +	u32 ibe_bit;
> > +	u32 obe_bit;
> >
> >  	/* generic pinconf */
> >  	bool generic_pinconf;
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > index dead416..f724a01 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> imx7ulp_pinctrl_info = {
> >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> >  	.mux_mask = 0xf00,
> >  	.mux_shift = 8,
> > +	.ibe_bit = BIT(16),
> > +	.obe_bit = BIT(17),
> >  	.generic_pinconf = true,
> >  	.custom_params = imx7ulp_cfg_params,
> >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
> > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > index 3bd8556..c0823f9 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> vf610_pinctrl_info = {
> >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> >  	.mux_mask = 0x700000,
> >  	.mux_shift = 20,
> > +	.ibe_bit = BIT(0),
> > +	.obe_bit = BIT(1),
> >  };
> >
> >  static const struct of_device_id vf610_pinctrl_of_match[] = {

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Tuesday, May 16, 2017 1:11 AM
> To: A.S. Dong
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> On 2017-05-14 23:48, Dong Aisheng wrote:
> > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> 
> s/Vibrid/Vybrid
> 
> > it a SoC property.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > Cc: Bai Ping <ping.bai@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> >  4 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index 57e1f7a..0d6aaca 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > pinctrl_dev *pctldev,
> >  	u32 reg;
> >
> >  	/*
> > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> > -	 * They are part of the shared mux/conf register.
> > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> >  	 */
> >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> >  		return 0;
> > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > pinctrl_dev *pctldev,
> >  	/* IBE always enabled allows us to read the value "on the wire" */
> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >  	if (input)
> > -		reg &= ~0x2;
> > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> >  	else
> > -		reg |= 0x2;
> > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> 
> Why disabling IBE bit now? As the comment above states, it is really handy
> to leave the input buffer enabled so we can read back the true value on
> the wire...
> 

Does Vybrid reply on this bit to read the status of output from
Port Data Input Register (GPIOx_PDIR)?

Then, it is a bit strange that read status from input register when the GPIO is
Actually configured as output.

Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.

For MX7ULP, we will read input or output register accordingly by checking
GPIO direction register (PDDR) and we will not enable Input buffer if the GPIO
is configured as output, this also save a bit power.

I know Vybrid has no PDDR, probably that's why you enable IBE by default
always.

Regards
Dong Aisheng

> --
> Stefan
> 
> >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> >  	return 0;
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> > b/drivers/pinctrl/freescale/pinctrl-imx.h
> > index eb0ce95..9ded65d 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> >  	unsigned int mux_mask;
> >  	u8 mux_shift;
> > +	u32 ibe_bit;
> > +	u32 obe_bit;
> >
> >  	/* generic pinconf */
> >  	bool generic_pinconf;
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > index dead416..f724a01 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> imx7ulp_pinctrl_info = {
> >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> >  	.mux_mask = 0xf00,
> >  	.mux_shift = 8,
> > +	.ibe_bit = BIT(16),
> > +	.obe_bit = BIT(17),
> >  	.generic_pinconf = true,
> >  	.custom_params = imx7ulp_cfg_params,
> >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
> > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > index 3bd8556..c0823f9 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> vf610_pinctrl_info = {
> >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> >  	.mux_mask = 0x700000,
> >  	.mux_shift = 20,
> > +	.ibe_bit = BIT(0),
> > +	.obe_bit = BIT(1),
> >  };
> >
> >  static const struct of_device_id vf610_pinctrl_of_match[] = {

RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: A.S. Dong
> Sent: Wednesday, May 17, 2017 2:14 PM
> To: 'Stefan Agner'
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan@agner.ch]
> > Sent: Tuesday, May 16, 2017 1:11 AM
> > To: A.S. Dong
> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> > kernel@pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > property
> >
> > On 2017-05-14 23:48, Dong Aisheng wrote:
> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> >
> > s/Vibrid/Vybrid
> >
> > > it a SoC property.
> > >
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Stefan Agner <stefan@agner.ch>
> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> > > Cc: Bai Ping <ping.bai@nxp.com>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > > index 57e1f7a..0d6aaca 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > > pinctrl_dev *pctldev,
> > >  	u32 reg;
> > >
> > >  	/*
> > > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> > > -	 * They are part of the shared mux/conf register.
> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> > >  	 */
> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> > >  		return 0;
> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > > pinctrl_dev *pctldev,
> > >  	/* IBE always enabled allows us to read the value "on the wire" */
> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >  	if (input)
> > > -		reg &= ~0x2;
> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> > >  	else
> > > -		reg |= 0x2;
> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> >
> > Why disabling IBE bit now? As the comment above states, it is really
> > handy to leave the input buffer enabled so we can read back the true
> > value on the wire...
> >
> 
> Does Vybrid reply on this bit to read the status of output from Port Data
> Input Register (GPIOx_PDIR)?
> 
> Then, it is a bit strange that read status from input register when the
> GPIO is Actually configured as output.
> 
> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> 
> For MX7ULP, we will read input or output register accordingly by checking
> GPIO direction register (PDDR) and we will not enable Input buffer if the
> GPIO is configured as output, this also save a bit power.
> 
> I know Vybrid has no PDDR, probably that's why you enable IBE by default
> always.
> 

We need make a decision on how to address the difference between Vybrid
And IMX7ULP.

If Vybrid wants to keep input buffer enabled, we probably could invent
A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for IMX?

Do you think it's ok?


Regards
Dong Aisheng


> Regards
> Dong Aisheng
> 
> > --
> > Stefan
> >
> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> > >
> > >  	return 0;
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
> > > index eb0ce95..9ded65d 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> > >  	unsigned int mux_mask;
> > >  	u8 mux_shift;
> > > +	u32 ibe_bit;
> > > +	u32 obe_bit;
> > >
> > >  	/* generic pinconf */
> > >  	bool generic_pinconf;
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > index dead416..f724a01 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> > imx7ulp_pinctrl_info = {
> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> > >  	.mux_mask = 0xf00,
> > >  	.mux_shift = 8,
> > > +	.ibe_bit = BIT(16),
> > > +	.obe_bit = BIT(17),
> > >  	.generic_pinconf = true,
> > >  	.custom_params = imx7ulp_cfg_params,
> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > index 3bd8556..c0823f9 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> > vf610_pinctrl_info = {
> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> > >  	.mux_mask = 0x700000,
> > >  	.mux_shift = 20,
> > > +	.ibe_bit = BIT(0),
> > > +	.obe_bit = BIT(1),
> > >  };
> > >
> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: A.S. Dong
> Sent: Wednesday, May 17, 2017 2:14 PM
> To: 'Stefan Agner'
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan at agner.ch]
> > Sent: Tuesday, May 16, 2017 1:11 AM
> > To: A.S. Dong
> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> > kernel at pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > property
> >
> > On 2017-05-14 23:48, Dong Aisheng wrote:
> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> >
> > s/Vibrid/Vybrid
> >
> > > it a SoC property.
> > >
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Stefan Agner <stefan@agner.ch>
> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> > > Cc: Bai Ping <ping.bai@nxp.com>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > > index 57e1f7a..0d6aaca 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > > pinctrl_dev *pctldev,
> > >  	u32 reg;
> > >
> > >  	/*
> > > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
> > > -	 * They are part of the shared mux/conf register.
> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> > >  	 */
> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> > >  		return 0;
> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > > pinctrl_dev *pctldev,
> > >  	/* IBE always enabled allows us to read the value "on the wire" */
> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >  	if (input)
> > > -		reg &= ~0x2;
> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> > >  	else
> > > -		reg |= 0x2;
> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> >
> > Why disabling IBE bit now? As the comment above states, it is really
> > handy to leave the input buffer enabled so we can read back the true
> > value on the wire...
> >
> 
> Does Vybrid reply on this bit to read the status of output from Port Data
> Input Register (GPIOx_PDIR)?
> 
> Then, it is a bit strange that read status from input register when the
> GPIO is Actually configured as output.
> 
> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> 
> For MX7ULP, we will read input or output register accordingly by checking
> GPIO direction register (PDDR) and we will not enable Input buffer if the
> GPIO is configured as output, this also save a bit power.
> 
> I know Vybrid has no PDDR, probably that's why you enable IBE by default
> always.
> 

We need make a decision on how to address the difference between Vybrid
And IMX7ULP.

If Vybrid wants to keep input buffer enabled, we probably could invent
A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for IMX?

Do you think it's ok?


Regards
Dong Aisheng


> Regards
> Dong Aisheng
> 
> > --
> > Stefan
> >
> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> > >
> > >  	return 0;
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
> > > index eb0ce95..9ded65d 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> > >  	unsigned int mux_mask;
> > >  	u8 mux_shift;
> > > +	u32 ibe_bit;
> > > +	u32 obe_bit;
> > >
> > >  	/* generic pinconf */
> > >  	bool generic_pinconf;
> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > index dead416..f724a01 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> > imx7ulp_pinctrl_info = {
> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> > >  	.mux_mask = 0xf00,
> > >  	.mux_shift = 8,
> > > +	.ibe_bit = BIT(16),
> > > +	.obe_bit = BIT(17),
> > >  	.generic_pinconf = true,
> > >  	.custom_params = imx7ulp_cfg_params,
> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > index 3bd8556..c0823f9 100644
> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> > vf610_pinctrl_info = {
> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> > >  	.mux_mask = 0x700000,
> > >  	.mux_shift = 20,
> > > +	.ibe_bit = BIT(0),
> > > +	.obe_bit = BIT(1),
> > >  };
> > >
> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Stefan Agner]

On 2017-05-23 05:06, A.S. Dong wrote:
> Hi Stefan,
> 
>> -----Original Message-----
>> From: A.S. Dong
>> Sent: Wednesday, May 17, 2017 2:14 PM
>> To: 'Stefan Agner'
>> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
>> kernel@pengutronix.de; Alexandre Courbot
>> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
>> property
>>
>> > -----Original Message-----
>> > From: Stefan Agner [mailto:stefan@agner.ch]
>> > Sent: Tuesday, May 16, 2017 1:11 AM
>> > To: A.S. Dong
>> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
>> > kernel@pengutronix.de; Alexandre Courbot
>> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
>> > property
>> >
>> > On 2017-05-14 23:48, Dong Aisheng wrote:
>> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
>> >
>> > s/Vibrid/Vybrid
>> >
>> > > it a SoC property.
>> > >
>> > > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > > Cc: Shawn Guo <shawnguo@kernel.org>
>> > > Cc: Stefan Agner <stefan@agner.ch>
>> > > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > > Cc: Bai Ping <ping.bai@nxp.com>
>> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > > ---
>> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
>> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
>> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
>> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
>> > >  4 files changed, 10 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > index 57e1f7a..0d6aaca 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
>> > > pinctrl_dev *pctldev,
>> > >  	u32 reg;
>> > >
>> > >  	/*
>> > > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
>> > > -	 * They are part of the shared mux/conf register.
>> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
>> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
>> > >  	 */
>> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
>> > >  		return 0;
>> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
>> > > pinctrl_dev *pctldev,
>> > >  	/* IBE always enabled allows us to read the value "on the wire" */
>> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> > >  	if (input)
>> > > -		reg &= ~0x2;
>> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
>> > >  	else
>> > > -		reg |= 0x2;
>> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
>> >
>> > Why disabling IBE bit now? As the comment above states, it is really
>> > handy to leave the input buffer enabled so we can read back the true
>> > value on the wire...
>> >
>>
>> Does Vybrid reply on this bit to read the status of output from Port Data
>> Input Register (GPIOx_PDIR)?
>>
>> Then, it is a bit strange that read status from input register when the
>> GPIO is Actually configured as output.
>>
>> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
>>
>> For MX7ULP, we will read input or output register accordingly by checking
>> GPIO direction register (PDDR) and we will not enable Input buffer if the
>> GPIO is configured as output, this also save a bit power.
>>
>> I know Vybrid has no PDDR, probably that's why you enable IBE by default
>> always.
>>

The main reason I enable input buffer by default is so that software has
a chance to see the actual state of the GPIO. E.g. when a GPIO is
connected to GND, and you set it high, you can read back a 0... (of
course you should not connect a GPIO to GND and set it high! But that is
exactly the point, with enabling the input buffer you actually see that
something is wrong!)

Can we not do this for iMX7ULP too?

> 
> We need make a decision on how to address the difference between Vybrid
> And IMX7ULP.
> 
> If Vybrid wants to keep input buffer enabled, we probably could invent
> A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for IMX?

That would be one option yes. You also need a flag so you can opt out
from imx_pmx_gpio_enable/disable_free...

The more involved version would be to somehow move imx_pmx_gpio_* to
pinctrl-vf610.c, e.g. store the callbacks in imx_pinctrl_soc_info and
set them dynamically in imx_pinctrl_probe (you have to unconst
pinmux_ops). With that you can provide no
imx_pmx_gpio_enable/disable_free for iMX7ULP and your own
imx_pmx_gpio_set_direction...

--
Stefan

> 
> Do you think it's ok?
> 
> 
> Regards
> Dong Aisheng
> 
> 
>> Regards
>> Dong Aisheng
>>
>> > --
>> > Stefan
>> >
>> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
>> > >
>> > >  	return 0;
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > index eb0ce95..9ded65d 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
>> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
>> > >  	unsigned int mux_mask;
>> > >  	u8 mux_shift;
>> > > +	u32 ibe_bit;
>> > > +	u32 obe_bit;
>> > >
>> > >  	/* generic pinconf */
>> > >  	bool generic_pinconf;
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > index dead416..f724a01 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
>> > imx7ulp_pinctrl_info = {
>> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
>> > >  	.mux_mask = 0xf00,
>> > >  	.mux_shift = 8,
>> > > +	.ibe_bit = BIT(16),
>> > > +	.obe_bit = BIT(17),
>> > >  	.generic_pinconf = true,
>> > >  	.custom_params = imx7ulp_cfg_params,
>> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
>> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > index 3bd8556..c0823f9 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
>> > vf610_pinctrl_info = {
>> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
>> > >  	.mux_mask = 0x700000,
>> > >  	.mux_shift = 20,
>> > > +	.ibe_bit = BIT(0),
>> > > +	.obe_bit = BIT(1),
>> > >  };
>> > >
>> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Stefan Agner]

On 2017-05-23 05:06, A.S. Dong wrote:
> Hi Stefan,
> 
>> -----Original Message-----
>> From: A.S. Dong
>> Sent: Wednesday, May 17, 2017 2:14 PM
>> To: 'Stefan Agner'
>> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
>> kernel at pengutronix.de; Alexandre Courbot
>> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
>> property
>>
>> > -----Original Message-----
>> > From: Stefan Agner [mailto:stefan at agner.ch]
>> > Sent: Tuesday, May 16, 2017 1:11 AM
>> > To: A.S. Dong
>> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
>> > kernel at pengutronix.de; Alexandre Courbot
>> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
>> > property
>> >
>> > On 2017-05-14 23:48, Dong Aisheng wrote:
>> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
>> >
>> > s/Vibrid/Vybrid
>> >
>> > > it a SoC property.
>> > >
>> > > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > > Cc: Shawn Guo <shawnguo@kernel.org>
>> > > Cc: Stefan Agner <stefan@agner.ch>
>> > > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > > Cc: Bai Ping <ping.bai@nxp.com>
>> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > > ---
>> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
>> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
>> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
>> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
>> > >  4 files changed, 10 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > index 57e1f7a..0d6aaca 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
>> > > pinctrl_dev *pctldev,
>> > >  	u32 reg;
>> > >
>> > >  	/*
>> > > -	 * Only Vybrid has the input/output buffer enable flags (IBE/OBE)
>> > > -	 * They are part of the shared mux/conf register.
>> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable flags
>> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
>> > >  	 */
>> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
>> > >  		return 0;
>> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
>> > > pinctrl_dev *pctldev,
>> > >  	/* IBE always enabled allows us to read the value "on the wire" */
>> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> > >  	if (input)
>> > > -		reg &= ~0x2;
>> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
>> > >  	else
>> > > -		reg |= 0x2;
>> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
>> >
>> > Why disabling IBE bit now? As the comment above states, it is really
>> > handy to leave the input buffer enabled so we can read back the true
>> > value on the wire...
>> >
>>
>> Does Vybrid reply on this bit to read the status of output from Port Data
>> Input Register (GPIOx_PDIR)?
>>
>> Then, it is a bit strange that read status from input register when the
>> GPIO is Actually configured as output.
>>
>> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
>>
>> For MX7ULP, we will read input or output register accordingly by checking
>> GPIO direction register (PDDR) and we will not enable Input buffer if the
>> GPIO is configured as output, this also save a bit power.
>>
>> I know Vybrid has no PDDR, probably that's why you enable IBE by default
>> always.
>>

The main reason I enable input buffer by default is so that software has
a chance to see the actual state of the GPIO. E.g. when a GPIO is
connected to GND, and you set it high, you can read back a 0... (of
course you should not connect a GPIO to GND and set it high! But that is
exactly the point, with enabling the input buffer you actually see that
something is wrong!)

Can we not do this for iMX7ULP too?

> 
> We need make a decision on how to address the difference between Vybrid
> And IMX7ULP.
> 
> If Vybrid wants to keep input buffer enabled, we probably could invent
> A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for IMX?

That would be one option yes. You also need a flag so you can opt out
from imx_pmx_gpio_enable/disable_free...

The more involved version would be to somehow move imx_pmx_gpio_* to
pinctrl-vf610.c, e.g. store the callbacks in imx_pinctrl_soc_info and
set them dynamically in imx_pinctrl_probe (you have to unconst
pinmux_ops). With that you can provide no
imx_pmx_gpio_enable/disable_free for iMX7ULP and your own
imx_pmx_gpio_set_direction...

--
Stefan

> 
> Do you think it's ok?
> 
> 
> Regards
> Dong Aisheng
> 
> 
>> Regards
>> Dong Aisheng
>>
>> > --
>> > Stefan
>> >
>> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
>> > >
>> > >  	return 0;
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > index eb0ce95..9ded65d 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
>> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
>> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
>> > >  	unsigned int mux_mask;
>> > >  	u8 mux_shift;
>> > > +	u32 ibe_bit;
>> > > +	u32 obe_bit;
>> > >
>> > >  	/* generic pinconf */
>> > >  	bool generic_pinconf;
>> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > index dead416..f724a01 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
>> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
>> > imx7ulp_pinctrl_info = {
>> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
>> > >  	.mux_mask = 0xf00,
>> > >  	.mux_shift = 8,
>> > > +	.ibe_bit = BIT(16),
>> > > +	.obe_bit = BIT(17),
>> > >  	.generic_pinconf = true,
>> > >  	.custom_params = imx7ulp_cfg_params,
>> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --git
>> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > index 3bd8556..c0823f9 100644
>> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
>> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
>> > vf610_pinctrl_info = {
>> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
>> > >  	.mux_mask = 0x700000,
>> > >  	.mux_shift = 20,
>> > > +	.ibe_bit = BIT(0),
>> > > +	.obe_bit = BIT(1),
>> > >  };
>> > >
>> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Wednesday, May 24, 2017 3:16 AM
> To: A.S. Dong
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> On 2017-05-23 05:06, A.S. Dong wrote:
> > Hi Stefan,
> >
> >> -----Original Message-----
> >> From: A.S. Dong
> >> Sent: Wednesday, May 17, 2017 2:14 PM
> >> To: 'Stefan Agner'
> >> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> >> kernel@pengutronix.de; Alexandre Courbot
> >> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> >> property
> >>
> >> > -----Original Message-----
> >> > From: Stefan Agner [mailto:stefan@agner.ch]
> >> > Sent: Tuesday, May 16, 2017 1:11 AM
> >> > To: A.S. Dong
> >> > Cc: linux-gpio@vger.kernel.org;
> >> > linux-arm-kernel@lists.infradead.org;
> >> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
> >> > Duan; kernel@pengutronix.de; Alexandre Courbot
> >> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> >> > property
> >> >
> >> > On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> >> >
> >> > s/Vibrid/Vybrid
> >> >
> >> > > it a SoC property.
> >> > >
> >> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > > Cc: Stefan Agner <stefan@agner.ch>
> >> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > > Cc: Bai Ping <ping.bai@nxp.com>
> >> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > > ---
> >> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> >> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> >> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> >> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> >> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > index 57e1f7a..0d6aaca 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> >> > > pinctrl_dev *pctldev,
> >> > >  	u32 reg;
> >> > >
> >> > >  	/*
> >> > > -	 * Only Vybrid has the input/output buffer enable flags
> (IBE/OBE)
> >> > > -	 * They are part of the shared mux/conf register.
> >> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable
> flags
> >> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> >> > >  	 */
> >> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> >> > >  		return 0;
> >> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> >> > > pinctrl_dev *pctldev,
> >> > >  	/* IBE always enabled allows us to read the value "on the
> wire" */
> >> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> > >  	if (input)
> >> > > -		reg &= ~0x2;
> >> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> >> > >  	else
> >> > > -		reg |= 0x2;
> >> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> >> >
> >> > Why disabling IBE bit now? As the comment above states, it is
> >> > really handy to leave the input buffer enabled so we can read back
> >> > the true value on the wire...
> >> >
> >>
> >> Does Vybrid reply on this bit to read the status of output from Port
> >> Data Input Register (GPIOx_PDIR)?
> >>
> >> Then, it is a bit strange that read status from input register when
> >> the GPIO is Actually configured as output.
> >>
> >> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> >>
> >> For MX7ULP, we will read input or output register accordingly by
> >> checking GPIO direction register (PDDR) and we will not enable Input
> >> buffer if the GPIO is configured as output, this also save a bit power.
> >>
> >> I know Vybrid has no PDDR, probably that's why you enable IBE by
> >> default always.
> >>
> 
> The main reason I enable input buffer by default is so that software has a
> chance to see the actual state of the GPIO. E.g. when a GPIO is connected
> to GND, and you set it high, you can read back a 0... (of course you
> should not connect a GPIO to GND and set it high! But that is exactly the
> point, with enabling the input buffer you actually see that something is
> wrong!)
> 
> Can we not do this for iMX7ULP too?
> 

Hmm.. I probably would think it reversely.
For me, I'd prefer the gpio value read back reflect the real value set in
GPIO controller, not the actual state on the line.
It at least tells "Hey, your GPIO SW setting is okay, check HW ..".

Users could measure the actual state if something is wrong.

> >
> > We need make a decision on how to address the difference between
> > Vybrid And IMX7ULP.
> >
> > If Vybrid wants to keep input buffer enabled, we probably could invent
> > A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for
> IMX?
> 
> That would be one option yes. You also need a flag so you can opt out from
> imx_pmx_gpio_enable/disable_free...
> 
> The more involved version would be to somehow move imx_pmx_gpio_* to
> pinctrl-vf610.c, e.g. store the callbacks in imx_pinctrl_soc_info and set
> them dynamically in imx_pinctrl_probe (you have to unconst pinmux_ops).
> With that you can provide no imx_pmx_gpio_enable/disable_free for iMX7ULP
> and your own imx_pmx_gpio_set_direction...
> 

Since we've got an agreement to remove gpio_enable/disable_free in another
mail I just replied, I will remove it and only keep gpio_set_direction,
but re-implement it as platform specific callback API.

Regards
Dong Aisheng

> --
> Stefan
> 
> >
> > Do you think it's ok?
> >
> >
> > Regards
> > Dong Aisheng
> >
> >
> >> Regards
> >> Dong Aisheng
> >>
> >> > --
> >> > Stefan
> >> >
> >> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >> > >
> >> > >  	return 0;
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > index eb0ce95..9ded65d 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> >> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> >> > >  	unsigned int mux_mask;
> >> > >  	u8 mux_shift;
> >> > > +	u32 ibe_bit;
> >> > > +	u32 obe_bit;
> >> > >
> >> > >  	/* generic pinconf */
> >> > >  	bool generic_pinconf;
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > index dead416..f724a01 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> >> > imx7ulp_pinctrl_info = {
> >> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> >> > >  	.mux_mask = 0xf00,
> >> > >  	.mux_shift = 8,
> >> > > +	.ibe_bit = BIT(16),
> >> > > +	.obe_bit = BIT(17),
> >> > >  	.generic_pinconf = true,
> >> > >  	.custom_params = imx7ulp_cfg_params,
> >> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --
> git
> >> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > index 3bd8556..c0823f9 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> >> > vf610_pinctrl_info = {
> >> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> >> > >  	.mux_mask = 0x700000,
> >> > >  	.mux_shift = 20,
> >> > > +	.ibe_bit = BIT(0),
> >> > > +	.obe_bit = BIT(1),
> >> > >  };
> >> > >
> >> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Wednesday, May 24, 2017 3:16 AM
> To: A.S. Dong
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> property
> 
> On 2017-05-23 05:06, A.S. Dong wrote:
> > Hi Stefan,
> >
> >> -----Original Message-----
> >> From: A.S. Dong
> >> Sent: Wednesday, May 17, 2017 2:14 PM
> >> To: 'Stefan Agner'
> >> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> >> kernel at pengutronix.de; Alexandre Courbot
> >> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> >> property
> >>
> >> > -----Original Message-----
> >> > From: Stefan Agner [mailto:stefan at agner.ch]
> >> > Sent: Tuesday, May 16, 2017 1:11 AM
> >> > To: A.S. Dong
> >> > Cc: linux-gpio at vger.kernel.org;
> >> > linux-arm-kernel at lists.infradead.org;
> >> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
> >> > Duan; kernel at pengutronix.de; Alexandre Courbot
> >> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> >> > property
> >> >
> >> > On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> >> >
> >> > s/Vibrid/Vybrid
> >> >
> >> > > it a SoC property.
> >> > >
> >> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > > Cc: Stefan Agner <stefan@agner.ch>
> >> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > > Cc: Bai Ping <ping.bai@nxp.com>
> >> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > > ---
> >> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> >> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> >> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> >> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> >> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > index 57e1f7a..0d6aaca 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> >> > > pinctrl_dev *pctldev,
> >> > >  	u32 reg;
> >> > >
> >> > >  	/*
> >> > > -	 * Only Vybrid has the input/output buffer enable flags
> (IBE/OBE)
> >> > > -	 * They are part of the shared mux/conf register.
> >> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable
> flags
> >> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> >> > >  	 */
> >> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> >> > >  		return 0;
> >> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> >> > > pinctrl_dev *pctldev,
> >> > >  	/* IBE always enabled allows us to read the value "on the
> wire" */
> >> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> > >  	if (input)
> >> > > -		reg &= ~0x2;
> >> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> >> > >  	else
> >> > > -		reg |= 0x2;
> >> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> >> >
> >> > Why disabling IBE bit now? As the comment above states, it is
> >> > really handy to leave the input buffer enabled so we can read back
> >> > the true value on the wire...
> >> >
> >>
> >> Does Vybrid reply on this bit to read the status of output from Port
> >> Data Input Register (GPIOx_PDIR)?
> >>
> >> Then, it is a bit strange that read status from input register when
> >> the GPIO is Actually configured as output.
> >>
> >> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> >>
> >> For MX7ULP, we will read input or output register accordingly by
> >> checking GPIO direction register (PDDR) and we will not enable Input
> >> buffer if the GPIO is configured as output, this also save a bit power.
> >>
> >> I know Vybrid has no PDDR, probably that's why you enable IBE by
> >> default always.
> >>
> 
> The main reason I enable input buffer by default is so that software has a
> chance to see the actual state of the GPIO. E.g. when a GPIO is connected
> to GND, and you set it high, you can read back a 0... (of course you
> should not connect a GPIO to GND and set it high! But that is exactly the
> point, with enabling the input buffer you actually see that something is
> wrong!)
> 
> Can we not do this for iMX7ULP too?
> 

Hmm.. I probably would think it reversely.
For me, I'd prefer the gpio value read back reflect the real value set in
GPIO controller, not the actual state on the line.
It at least tells "Hey, your GPIO SW setting is okay, check HW ..".

Users could measure the actual state if something is wrong.

> >
> > We need make a decision on how to address the difference between
> > Vybrid And IMX7ULP.
> >
> > If Vybrid wants to keep input buffer enabled, we probably could invent
> > A IMX_PINCONF_HAVE_PDDR just like GPIO driver and only disable IBE for
> IMX?
> 
> That would be one option yes. You also need a flag so you can opt out from
> imx_pmx_gpio_enable/disable_free...
> 
> The more involved version would be to somehow move imx_pmx_gpio_* to
> pinctrl-vf610.c, e.g. store the callbacks in imx_pinctrl_soc_info and set
> them dynamically in imx_pinctrl_probe (you have to unconst pinmux_ops).
> With that you can provide no imx_pmx_gpio_enable/disable_free for iMX7ULP
> and your own imx_pmx_gpio_set_direction...
> 

Since we've got an agreement to remove gpio_enable/disable_free in another
mail I just replied, I will remove it and only keep gpio_set_direction,
but re-implement it as platform specific callback API.

Regards
Dong Aisheng

> --
> Stefan
> 
> >
> > Do you think it's ok?
> >
> >
> > Regards
> > Dong Aisheng
> >
> >
> >> Regards
> >> Dong Aisheng
> >>
> >> > --
> >> > Stefan
> >> >
> >> > >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >> > >
> >> > >  	return 0;
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > index eb0ce95..9ded65d 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> >> > > @@ -67,6 +67,8 @@ struct imx_pinctrl_soc_info {
> >> > >  	/* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */
> >> > >  	unsigned int mux_mask;
> >> > >  	u8 mux_shift;
> >> > > +	u32 ibe_bit;
> >> > > +	u32 obe_bit;
> >> > >
> >> > >  	/* generic pinconf */
> >> > >  	bool generic_pinconf;
> >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > index dead416..f724a01 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx7ulp.c
> >> > > @@ -324,6 +324,8 @@ static struct imx_pinctrl_soc_info
> >> > imx7ulp_pinctrl_info = {
> >> > >  	.flags = ZERO_OFFSET_VALID | SHARE_MUX_CONF_REG,
> >> > >  	.mux_mask = 0xf00,
> >> > >  	.mux_shift = 8,
> >> > > +	.ibe_bit = BIT(16),
> >> > > +	.obe_bit = BIT(17),
> >> > >  	.generic_pinconf = true,
> >> > >  	.custom_params = imx7ulp_cfg_params,
> >> > >  	.num_custom_params = ARRAY_SIZE(imx7ulp_cfg_params), diff --
> git
> >> > > a/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > b/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > index 3bd8556..c0823f9 100644
> >> > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
> >> > > @@ -301,6 +301,8 @@ static struct imx_pinctrl_soc_info
> >> > vf610_pinctrl_info = {
> >> > >  	.flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID,
> >> > >  	.mux_mask = 0x700000,
> >> > >  	.mux_shift = 20,
> >> > > +	.ibe_bit = BIT(0),
> >> > > +	.obe_bit = BIT(1),
> >> > >  };
> >> > >
> >> > >  static const struct of_device_id vf610_pinctrl_of_match[] = {

Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Lothar Waßmann]

"A.S. Dong" <aisheng.dong@nxp.com> wrote:

> Hi Stefan,
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan@agner.ch]
> > Sent: Wednesday, May 24, 2017 3:16 AM
> > To: A.S. Dong
> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> > kernel@pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > property
> > 
> > On 2017-05-23 05:06, A.S. Dong wrote:
> > > Hi Stefan,
> > >
> > >> -----Original Message-----
> > >> From: A.S. Dong
> > >> Sent: Wednesday, May 17, 2017 2:14 PM
> > >> To: 'Stefan Agner'
> > >> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> > >> kernel@pengutronix.de; Alexandre Courbot
> > >> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > >> property
> > >>
> > >> > -----Original Message-----
> > >> > From: Stefan Agner [mailto:stefan@agner.ch]
> > >> > Sent: Tuesday, May 16, 2017 1:11 AM
> > >> > To: A.S. Dong
> > >> > Cc: linux-gpio@vger.kernel.org;
> > >> > linux-arm-kernel@lists.infradead.org;
> > >> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
> > >> > Duan; kernel@pengutronix.de; Alexandre Courbot
> > >> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > >> > property
> > >> >
> > >> > On 2017-05-14 23:48, Dong Aisheng wrote:
> > >> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> > >> >
> > >> > s/Vibrid/Vybrid
> > >> >
> > >> > > it a SoC property.
> > >> > >
> > >> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> > >> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > >> > > Cc: Stefan Agner <stefan@agner.ch>
> > >> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> > >> > > Cc: Bai Ping <ping.bai@nxp.com>
> > >> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >> > > ---
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> > >> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> > >> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> > >> > >
> > >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > index 57e1f7a..0d6aaca 100644
> > >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > >> > > pinctrl_dev *pctldev,
> > >> > >  	u32 reg;
> > >> > >
> > >> > >  	/*
> > >> > > -	 * Only Vybrid has the input/output buffer enable flags
> > (IBE/OBE)
> > >> > > -	 * They are part of the shared mux/conf register.
> > >> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable
> > flags
> > >> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> > >> > >  	 */
> > >> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> > >> > >  		return 0;
> > >> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > >> > > pinctrl_dev *pctldev,
> > >> > >  	/* IBE always enabled allows us to read the value "on the
> > wire" */
> > >> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >> > >  	if (input)
> > >> > > -		reg &= ~0x2;
> > >> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> > >> > >  	else
> > >> > > -		reg |= 0x2;
> > >> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> > >> >
> > >> > Why disabling IBE bit now? As the comment above states, it is
> > >> > really handy to leave the input buffer enabled so we can read back
> > >> > the true value on the wire...
> > >> >
> > >>
> > >> Does Vybrid reply on this bit to read the status of output from Port
> > >> Data Input Register (GPIOx_PDIR)?
> > >>
> > >> Then, it is a bit strange that read status from input register when
> > >> the GPIO is Actually configured as output.
> > >>
> > >> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> > >>
> > >> For MX7ULP, we will read input or output register accordingly by
> > >> checking GPIO direction register (PDDR) and we will not enable Input
> > >> buffer if the GPIO is configured as output, this also save a bit power.
> > >>
> > >> I know Vybrid has no PDDR, probably that's why you enable IBE by
> > >> default always.
> > >>
> > 
> > The main reason I enable input buffer by default is so that software has a
> > chance to see the actual state of the GPIO. E.g. when a GPIO is connected
> > to GND, and you set it high, you can read back a 0... (of course you
> > should not connect a GPIO to GND and set it high! But that is exactly the
> > point, with enabling the input buffer you actually see that something is
> > wrong!)
> > 
> > Can we not do this for iMX7ULP too?
> > 
> 
> Hmm.. I probably would think it reversely.
> For me, I'd prefer the gpio value read back reflect the real value set in
> GPIO controller, not the actual state on the line.
> It at least tells "Hey, your GPIO SW setting is okay, check HW ..".
> 
> Users could measure the actual state if something is wrong.
> 
This doesn't buy the user anything. You can safely assume, that writing
some value to a register will actually set the designated bits in that
register. There is no need to confirm that by reading back.
What's more interesting is whether the change in the register setting
had the desired effect on the hardware outside the chip!


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC property

[Lothar Waßmann]

"A.S. Dong" <aisheng.dong@nxp.com> wrote:

> Hi Stefan,
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan at agner.ch]
> > Sent: Wednesday, May 24, 2017 3:16 AM
> > To: A.S. Dong
> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> > kernel at pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > property
> > 
> > On 2017-05-23 05:06, A.S. Dong wrote:
> > > Hi Stefan,
> > >
> > >> -----Original Message-----
> > >> From: A.S. Dong
> > >> Sent: Wednesday, May 17, 2017 2:14 PM
> > >> To: 'Stefan Agner'
> > >> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> > >> kernel at pengutronix.de; Alexandre Courbot
> > >> Subject: RE: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > >> property
> > >>
> > >> > -----Original Message-----
> > >> > From: Stefan Agner [mailto:stefan at agner.ch]
> > >> > Sent: Tuesday, May 16, 2017 1:11 AM
> > >> > To: A.S. Dong
> > >> > Cc: linux-gpio at vger.kernel.org;
> > >> > linux-arm-kernel at lists.infradead.org;
> > >> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
> > >> > Duan; kernel at pengutronix.de; Alexandre Courbot
> > >> > Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add IBE and OBE SoC
> > >> > property
> > >> >
> > >> > On 2017-05-14 23:48, Dong Aisheng wrote:
> > >> > > iMX ULP has different IOB/OBE shift from Vibrid, so let's make
> > >> >
> > >> > s/Vibrid/Vybrid
> > >> >
> > >> > > it a SoC property.
> > >> > >
> > >> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> > > Cc: Alexandre Courbot <gnurou@gmail.com>
> > >> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > >> > > Cc: Stefan Agner <stefan@agner.ch>
> > >> > > Cc: Fugang Duan <fugang.duan@nxp.com>
> > >> > > Cc: Bai Ping <ping.bai@nxp.com>
> > >> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >> > > ---
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx.c     | 8 ++++----
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx.h     | 2 ++
> > >> > >  drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 2 ++
> > >> > >  drivers/pinctrl/freescale/pinctrl-vf610.c   | 2 ++
> > >> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> > >> > >
> > >> > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > index 57e1f7a..0d6aaca 100644
> > >> > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > > @@ -331,8 +331,8 @@ static int imx_pmx_gpio_set_direction(struct
> > >> > > pinctrl_dev *pctldev,
> > >> > >  	u32 reg;
> > >> > >
> > >> > >  	/*
> > >> > > -	 * Only Vybrid has the input/output buffer enable flags
> > (IBE/OBE)
> > >> > > -	 * They are part of the shared mux/conf register.
> > >> > > +	 * Only Vybrid and iMX ULP has the input/output buffer enable
> > flags
> > >> > > +	 * (IBE/OBE) They are part of the shared mux/conf register.
> > >> > >  	 */
> > >> > >  	if (!(info->flags & SHARE_MUX_CONF_REG))
> > >> > >  		return 0;
> > >> > > @@ -344,9 +344,9 @@ static int imx_pmx_gpio_set_direction(struct
> > >> > > pinctrl_dev *pctldev,
> > >> > >  	/* IBE always enabled allows us to read the value "on the
> > wire" */
> > >> > >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >> > >  	if (input)
> > >> > > -		reg &= ~0x2;
> > >> > > +		reg = (reg & ~info->obe_bit) | info->ibe_bit;
> > >> > >  	else
> > >> > > -		reg |= 0x2;
> > >> > > +		reg = (reg & ~info->ibe_bit) | info->obe_bit;
> > >> >
> > >> > Why disabling IBE bit now? As the comment above states, it is
> > >> > really handy to leave the input buffer enabled so we can read back
> > >> > the true value on the wire...
> > >> >
> > >>
> > >> Does Vybrid reply on this bit to read the status of output from Port
> > >> Data Input Register (GPIOx_PDIR)?
> > >>
> > >> Then, it is a bit strange that read status from input register when
> > >> the GPIO is Actually configured as output.
> > >>
> > >> Probably it works on Vybrid, but I don't know if it's valid for MX7ULP.
> > >>
> > >> For MX7ULP, we will read input or output register accordingly by
> > >> checking GPIO direction register (PDDR) and we will not enable Input
> > >> buffer if the GPIO is configured as output, this also save a bit power.
> > >>
> > >> I know Vybrid has no PDDR, probably that's why you enable IBE by
> > >> default always.
> > >>
> > 
> > The main reason I enable input buffer by default is so that software has a
> > chance to see the actual state of the GPIO. E.g. when a GPIO is connected
> > to GND, and you set it high, you can read back a 0... (of course you
> > should not connect a GPIO to GND and set it high! But that is exactly the
> > point, with enabling the input buffer you actually see that something is
> > wrong!)
> > 
> > Can we not do this for iMX7ULP too?
> > 
> 
> Hmm.. I probably would think it reversely.
> For me, I'd prefer the gpio value read back reflect the real value set in
> GPIO controller, not the actual state on the line.
> It at least tells "Hey, your GPIO SW setting is okay, check HW ..".
> 
> Users could measure the actual state if something is wrong.
> 
This doesn't buy the user anything. You can safely assume, that writing
some value to a register will actually set the designated bits in that
register. There is no need to confirm that by reading back.
What's more interesting is whether the change in the register setting
had the desired effect on the hardware outside the chip!


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Dong Aisheng]

Do not assume MUX 0 is GPIO function in core driver as a different
SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 0d6aaca..ed8ea32 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 			continue;
 		for (pin = 0; pin < grp->num_pins; pin++) {
 			imx_pin = &((struct imx_pin *)(grp->data))[pin];
-			if (imx_pin->pin == offset && !imx_pin->mux_mode)
+			if (imx_pin->pin == offset)
 				goto mux_pin;
 		}
 	}
@@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	reg = readl(ipctl->base + pin_reg->mux_reg);
 	reg &= ~info->mux_mask;
 	reg |= imx_pin->config;
+	reg |= imx_pin->mux_mode << info->mux_shift;
 	writel(reg, ipctl->base + pin_reg->mux_reg);
 
 	return 0;
-- 
2.7.4

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Dong Aisheng]

Do not assume MUX 0 is GPIO function in core driver as a different
SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Fugang Duan <fugang.duan@nxp.com>
Cc: Bai Ping <ping.bai@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 0d6aaca..ed8ea32 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 			continue;
 		for (pin = 0; pin < grp->num_pins; pin++) {
 			imx_pin = &((struct imx_pin *)(grp->data))[pin];
-			if (imx_pin->pin == offset && !imx_pin->mux_mode)
+			if (imx_pin->pin == offset)
 				goto mux_pin;
 		}
 	}
@@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	reg = readl(ipctl->base + pin_reg->mux_reg);
 	reg &= ~info->mux_mask;
 	reg |= imx_pin->config;
+	reg |= imx_pin->mux_mode << info->mux_shift;
 	writel(reg, ipctl->base + pin_reg->mux_reg);
 
 	return 0;
-- 
2.7.4

Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-14 23:48, Dong Aisheng wrote:
> Do not assume MUX 0 is GPIO function in core driver as a different
> SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 0d6aaca..ed8ea32 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> pinctrl_dev *pctldev,
>  			continue;
>  		for (pin = 0; pin < grp->num_pins; pin++) {
>  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> +			if (imx_pin->pin == offset)
>  				goto mux_pin;

The reason I added that check was to make sure we pick a mux option
which is GPIO... With this change, any pinmux might be picked...

>  		}
>  	}
> @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> pinctrl_dev *pctldev,
>  	reg = readl(ipctl->base + pin_reg->mux_reg);
>  	reg &= ~info->mux_mask;
>  	reg |= imx_pin->config;
> +	reg |= imx_pin->mux_mode << info->mux_shift;

... and muxed...

Not sure if we want that.

I had to control GPIO output/input through pinctrl since Vybrid does not
allow to control that from the GPIO block.

However, according to your GPIO patchset, the i.MX 7ULP has a new
register GPIO_PDDR to control output from the GPIO block. Is controlling
the output driver from IOMUXC still required? If not, I really would
just not use all that "find pinctrl config" hackery... e.g. add a new
flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.

This would also align much better with the other i.MX devices, where
pinmuxing and GPIO is completely orthogonal.

--
Stefan

>  	writel(reg, ipctl->base + pin_reg->mux_reg);
>  
>  	return 0;

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-14 23:48, Dong Aisheng wrote:
> Do not assume MUX 0 is GPIO function in core driver as a different
> SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Fugang Duan <fugang.duan@nxp.com>
> Cc: Bai Ping <ping.bai@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 0d6aaca..ed8ea32 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> pinctrl_dev *pctldev,
>  			continue;
>  		for (pin = 0; pin < grp->num_pins; pin++) {
>  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> +			if (imx_pin->pin == offset)
>  				goto mux_pin;

The reason I added that check was to make sure we pick a mux option
which is GPIO... With this change, any pinmux might be picked...

>  		}
>  	}
> @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> pinctrl_dev *pctldev,
>  	reg = readl(ipctl->base + pin_reg->mux_reg);
>  	reg &= ~info->mux_mask;
>  	reg |= imx_pin->config;
> +	reg |= imx_pin->mux_mode << info->mux_shift;

... and muxed...

Not sure if we want that.

I had to control GPIO output/input through pinctrl since Vybrid does not
allow to control that from the GPIO block.

However, according to your GPIO patchset, the i.MX 7ULP has a new
register GPIO_PDDR to control output from the GPIO block. Is controlling
the output driver from IOMUXC still required? If not, I really would
just not use all that "find pinctrl config" hackery... e.g. add a new
flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.

This would also align much better with the other i.MX devices, where
pinmuxing and GPIO is completely orthogonal.

--
Stefan

>  	writel(reg, ipctl->base + pin_reg->mux_reg);
>  
>  	return 0;

RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Tuesday, May 16, 2017 1:27 AM
> To: A.S. Dong
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-14 23:48, Dong Aisheng wrote:
> > Do not assume MUX 0 is GPIO function in core driver as a different SoC
> > may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > Cc: Bai Ping <ping.bai@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index 0d6aaca..ed8ea32 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  			continue;
> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> > +			if (imx_pin->pin == offset)
> >  				goto mux_pin;
> 
> The reason I added that check was to make sure we pick a mux option which
> is GPIO... With this change, any pinmux might be picked...
> 

First of all, this seems to be wrong to me that GPIO mux mode is SoC
Dependant and should not be put in pinctrl-imx core driver.

Secondly, I think we may be over worried and it's not quite necessary
As we did not do the sanity check for both raw config and mux data read
>From Device tree, why only do it for GPIO?

We may trust the data in device tree.

> >  		}
> >  	}
> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >  	reg &= ~info->mux_mask;
> >  	reg |= imx_pin->config;
> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> 
> ... and muxed...
> 
> Not sure if we want that.
> 
> I had to control GPIO output/input through pinctrl since Vybrid does not
> allow to control that from the GPIO block.
> 
> However, according to your GPIO patchset, the i.MX 7ULP has a new register
> GPIO_PDDR to control output from the GPIO block. Is controlling the output
> driver from IOMUXC still required? 

Yes, it's still required.

> If not, I really would just not use all
> that "find pinctrl config" hackery... e.g. add a new flag,
> USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.
> 
> This would also align much better with the other i.MX devices, where
> pinmuxing and GPIO is completely orthogonal.
> 

Actually this patch came only because to make the exist code
not break MX7ULP.

Actually I'm wondering why we need implement 
imx_pmx_gpio_request_enable function?

Per my understanding, IMX binding already set GPIO mux by
Parsing MUX mode from device tree, so why need gpio_request_enable
which looks like is duplicated.

Can you help explain it?

Regards
Dong Aisheng

> --
> Stefan
> 
> >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> >  	return 0;

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Tuesday, May 16, 2017 1:27 AM
> To: A.S. Dong
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-14 23:48, Dong Aisheng wrote:
> > Do not assume MUX 0 is GPIO function in core driver as a different SoC
> > may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Stefan Agner <stefan@agner.ch>
> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > Cc: Bai Ping <ping.bai@nxp.com>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > index 0d6aaca..ed8ea32 100644
> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  			continue;
> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> > +			if (imx_pin->pin == offset)
> >  				goto mux_pin;
> 
> The reason I added that check was to make sure we pick a mux option which
> is GPIO... With this change, any pinmux might be picked...
> 

First of all, this seems to be wrong to me that GPIO mux mode is SoC
Dependant and should not be put in pinctrl-imx core driver.

Secondly, I think we may be over worried and it's not quite necessary
As we did not do the sanity check for both raw config and mux data read
>From Device tree, why only do it for GPIO?

We may trust the data in device tree.

> >  		}
> >  	}
> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> > pinctrl_dev *pctldev,
> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >  	reg &= ~info->mux_mask;
> >  	reg |= imx_pin->config;
> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> 
> ... and muxed...
> 
> Not sure if we want that.
> 
> I had to control GPIO output/input through pinctrl since Vybrid does not
> allow to control that from the GPIO block.
> 
> However, according to your GPIO patchset, the i.MX 7ULP has a new register
> GPIO_PDDR to control output from the GPIO block. Is controlling the output
> driver from IOMUXC still required? 

Yes, it's still required.

> If not, I really would just not use all
> that "find pinctrl config" hackery... e.g. add a new flag,
> USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.
> 
> This would also align much better with the other i.MX devices, where
> pinmuxing and GPIO is completely orthogonal.
> 

Actually this patch came only because to make the exist code
not break MX7ULP.

Actually I'm wondering why we need implement 
imx_pmx_gpio_request_enable function?

Per my understanding, IMX binding already set GPIO mux by
Parsing MUX mode from device tree, so why need gpio_request_enable
which looks like is duplicated.

Can you help explain it?

Regards
Dong Aisheng

> --
> Stefan
> 
> >  	writel(reg, ipctl->base + pin_reg->mux_reg);
> >
> >  	return 0;

Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-17 00:18, A.S. Dong wrote:
>> -----Original Message-----
>> From: Stefan Agner [mailto:stefan@agner.ch]
>> Sent: Tuesday, May 16, 2017 1:27 AM
>> To: A.S. Dong
>> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
>> kernel@pengutronix.de; Alexandre Courbot
>> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
>>
>> On 2017-05-14 23:48, Dong Aisheng wrote:
>> > Do not assume MUX 0 is GPIO function in core driver as a different SoC
>> > may have different value. e.g. MX7ULP Mux 1 is GPIO.
>> >
>> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > Cc: Shawn Guo <shawnguo@kernel.org>
>> > Cc: Stefan Agner <stefan@agner.ch>
>> > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > Cc: Bai Ping <ping.bai@nxp.com>
>> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > ---
>> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > index 0d6aaca..ed8ea32 100644
>> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > pinctrl_dev *pctldev,
>> >  			continue;
>> >  		for (pin = 0; pin < grp->num_pins; pin++) {
>> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
>> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
>> > +			if (imx_pin->pin == offset)
>> >  				goto mux_pin;
>>
>> The reason I added that check was to make sure we pick a mux option which
>> is GPIO... With this change, any pinmux might be picked...
>>
> 
> First of all, this seems to be wrong to me that GPIO mux mode is SoC
> Dependant and should not be put in pinctrl-imx core driver.

Hm, agree, we should consider to move
imx_pmx_gpio_request_enable/disable_free and imx_pmx_gpio_set_direction
into pinctrl-vf610.c

> 
> Secondly, I think we may be over worried and it's not quite necessary
> As we did not do the sanity check for both raw config and mux data read
> From Device tree, why only do it for GPIO?
> 
> We may trust the data in device tree.

In Vybrid, there is no need to explicitly assign a pinmux to use a pin
as GPIO. So the pinmux could be anything... The implemented semantics
for Vyrbid is really different than i.MX, see below.

> 
>> >  		}
>> >  	}
>> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > pinctrl_dev *pctldev,
>> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> >  	reg &= ~info->mux_mask;
>> >  	reg |= imx_pin->config;
>> > +	reg |= imx_pin->mux_mode << info->mux_shift;
>>
>> ... and muxed...
>>
>> Not sure if we want that.
>>
>> I had to control GPIO output/input through pinctrl since Vybrid does not
>> allow to control that from the GPIO block.
>>
>> However, according to your GPIO patchset, the i.MX 7ULP has a new register
>> GPIO_PDDR to control output from the GPIO block. Is controlling the output
>> driver from IOMUXC still required?
> 
> Yes, it's still required.
> 

That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
enable the output driver to drive the pin, but can I disable output just
using GPIO_PDDR?

Maybe we have a miss understanding here:

Lets assume we want to switch a GPIO between output and input:

echo "output" > /sys/class/gpio/gpioN/direction
..
echo "input" > /sys/class/gpio/gpioN/direction

Do I need to disable the output driver in the IOMUXC or can we just
disable GPIO_PDDR and use the pin as input?

If we can disable the output driver just using GPIO_PDDR, we can avoid
the gpio_set_direction cross call.


>> If not, I really would just not use all
>> that "find pinctrl config" hackery... e.g. add a new flag,
>> USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.
>>
>> This would also align much better with the other i.MX devices, where
>> pinmuxing and GPIO is completely orthogonal.
>>
> 
> Actually this patch came only because to make the exist code
> not break MX7ULP.
> 
> Actually I'm wondering why we need implement 
> imx_pmx_gpio_request_enable function?
> 
> Per my understanding, IMX binding already set GPIO mux by
> Parsing MUX mode from device tree, so why need gpio_request_enable
> which looks like is duplicated.
> 
> Can you help explain it?

I suggest to read this clarification email wrt to pinctrl/gpio from
Linus Walleij:
https://lkml.org/lkml/2016/10/10/87

To summarize: We have different semantics in Vybrid: The GPIO subsystem
automatically mux the GPIO for you. So in Vybrid, you do not have to mux
a GPIO (but a valid entry in your device tree is needed, but does not
assigned to any node).

Is what the driver is doing for Vybrid wrong? It is different from i.MX,
but afaik, it is not really wrong... Its a valid implementation
according to the currently defined semantics...  Due to the *need* to
touch pinctrl for direction, I had to implement cross calls anyway, so I
thought I might as well go full mile and also mux the GPIO on request...

So the question is, what semantic do we want for i.MX 7ULP? Since it is
a i.MX device, we probably want the same semantics as i.MX 6/7 is
already using for the sake of consistency. So in this case the
gpio_request_enable/disable callbacks are not needed...

This is how I hope we can do the implementation for i.MX 7ULP:
- Do not use gpio_request_enable/disable
- Do not use gpio_set_direction either
- Users using a GPIO should enable output driver in IOMUXC (just use a
pin configuration where output driver is enabled)
- The GPIO driver only enables/disables the output driver using its
GPIO_PDDR register depending on GPIO direction

--
Stefan

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-17 00:18, A.S. Dong wrote:
>> -----Original Message-----
>> From: Stefan Agner [mailto:stefan at agner.ch]
>> Sent: Tuesday, May 16, 2017 1:27 AM
>> To: A.S. Dong
>> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
>> kernel at pengutronix.de; Alexandre Courbot
>> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
>>
>> On 2017-05-14 23:48, Dong Aisheng wrote:
>> > Do not assume MUX 0 is GPIO function in core driver as a different SoC
>> > may have different value. e.g. MX7ULP Mux 1 is GPIO.
>> >
>> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > Cc: Shawn Guo <shawnguo@kernel.org>
>> > Cc: Stefan Agner <stefan@agner.ch>
>> > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > Cc: Bai Ping <ping.bai@nxp.com>
>> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > ---
>> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > index 0d6aaca..ed8ea32 100644
>> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > pinctrl_dev *pctldev,
>> >  			continue;
>> >  		for (pin = 0; pin < grp->num_pins; pin++) {
>> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
>> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
>> > +			if (imx_pin->pin == offset)
>> >  				goto mux_pin;
>>
>> The reason I added that check was to make sure we pick a mux option which
>> is GPIO... With this change, any pinmux might be picked...
>>
> 
> First of all, this seems to be wrong to me that GPIO mux mode is SoC
> Dependant and should not be put in pinctrl-imx core driver.

Hm, agree, we should consider to move
imx_pmx_gpio_request_enable/disable_free and imx_pmx_gpio_set_direction
into pinctrl-vf610.c

> 
> Secondly, I think we may be over worried and it's not quite necessary
> As we did not do the sanity check for both raw config and mux data read
> From Device tree, why only do it for GPIO?
> 
> We may trust the data in device tree.

In Vybrid, there is no need to explicitly assign a pinmux to use a pin
as GPIO. So the pinmux could be anything... The implemented semantics
for Vyrbid is really different than i.MX, see below.

> 
>> >  		}
>> >  	}
>> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > pinctrl_dev *pctldev,
>> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> >  	reg &= ~info->mux_mask;
>> >  	reg |= imx_pin->config;
>> > +	reg |= imx_pin->mux_mode << info->mux_shift;
>>
>> ... and muxed...
>>
>> Not sure if we want that.
>>
>> I had to control GPIO output/input through pinctrl since Vybrid does not
>> allow to control that from the GPIO block.
>>
>> However, according to your GPIO patchset, the i.MX 7ULP has a new register
>> GPIO_PDDR to control output from the GPIO block. Is controlling the output
>> driver from IOMUXC still required?
> 
> Yes, it's still required.
> 

That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
enable the output driver to drive the pin, but can I disable output just
using GPIO_PDDR?

Maybe we have a miss understanding here:

Lets assume we want to switch a GPIO between output and input:

echo "output" > /sys/class/gpio/gpioN/direction
..
echo "input" > /sys/class/gpio/gpioN/direction

Do I need to disable the output driver in the IOMUXC or can we just
disable GPIO_PDDR and use the pin as input?

If we can disable the output driver just using GPIO_PDDR, we can avoid
the gpio_set_direction cross call.


>> If not, I really would just not use all
>> that "find pinctrl config" hackery... e.g. add a new flag,
>> USE_IOMUXC_FOR_GPIO_OUTPUT, and set that only for Vybrid.
>>
>> This would also align much better with the other i.MX devices, where
>> pinmuxing and GPIO is completely orthogonal.
>>
> 
> Actually this patch came only because to make the exist code
> not break MX7ULP.
> 
> Actually I'm wondering why we need implement 
> imx_pmx_gpio_request_enable function?
> 
> Per my understanding, IMX binding already set GPIO mux by
> Parsing MUX mode from device tree, so why need gpio_request_enable
> which looks like is duplicated.
> 
> Can you help explain it?

I suggest to read this clarification email wrt to pinctrl/gpio from
Linus Walleij:
https://lkml.org/lkml/2016/10/10/87

To summarize: We have different semantics in Vybrid: The GPIO subsystem
automatically mux the GPIO for you. So in Vybrid, you do not have to mux
a GPIO (but a valid entry in your device tree is needed, but does not
assigned to any node).

Is what the driver is doing for Vybrid wrong? It is different from i.MX,
but afaik, it is not really wrong... Its a valid implementation
according to the currently defined semantics...  Due to the *need* to
touch pinctrl for direction, I had to implement cross calls anyway, so I
thought I might as well go full mile and also mux the GPIO on request...

So the question is, what semantic do we want for i.MX 7ULP? Since it is
a i.MX device, we probably want the same semantics as i.MX 6/7 is
already using for the sake of consistency. So in this case the
gpio_request_enable/disable callbacks are not needed...

This is how I hope we can do the implementation for i.MX 7ULP:
- Do not use gpio_request_enable/disable
- Do not use gpio_set_direction either
- Users using a GPIO should enable output driver in IOMUXC (just use a
pin configuration where output driver is enabled)
- The GPIO driver only enables/disables the output driver using its
GPIO_PDDR register depending on GPIO direction

--
Stefan

RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Thursday, May 18, 2017 2:16 AM
> To: A.S. Dong
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-17 00:18, A.S. Dong wrote:
> >> -----Original Message-----
> >> From: Stefan Agner [mailto:stefan@agner.ch]
> >> Sent: Tuesday, May 16, 2017 1:27 AM
> >> To: A.S. Dong
> >> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> >> kernel@pengutronix.de; Alexandre Courbot
> >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> >> gpio
> >>
> >> On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > Do not assume MUX 0 is GPIO function in core driver as a different
> >> > SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >> >
> >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > Cc: Bai Ping <ping.bai@nxp.com>
> >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > ---
> >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > index 0d6aaca..ed8ea32 100644
> >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  			continue;
> >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> >> > +			if (imx_pin->pin == offset)
> >> >  				goto mux_pin;
> >>
> >> The reason I added that check was to make sure we pick a mux option
> >> which is GPIO... With this change, any pinmux might be picked...
> >>
> >
> > First of all, this seems to be wrong to me that GPIO mux mode is SoC
> > Dependant and should not be put in pinctrl-imx core driver.
> 
> Hm, agree, we should consider to move
> imx_pmx_gpio_request_enable/disable_free and imx_pmx_gpio_set_direction
> into pinctrl-vf610.c
> 

IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
dynamically change GPIO from output to input.

> >
> > Secondly, I think we may be over worried and it's not quite necessary
> > As we did not do the sanity check for both raw config and mux data
> > read From Device tree, why only do it for GPIO?
> >
> > We may trust the data in device tree.
> 
> In Vybrid, there is no need to explicitly assign a pinmux to use a pin as
> GPIO. So the pinmux could be anything... The implemented semantics for
> Vyrbid is really different than i.MX, see below.
> 

Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
e.g.
arch/arm/boot/dts/vf-colibri.dtsi
pinctrl_esdhc1: esdhc1grp {
        fsl,pins = <
                VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
                VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
                VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
                VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
                VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
                VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
                VF610_PAD_PTB20__GPIO_42        0x219d
        >;
};  

> >
> >> >  		}
> >> >  	}
> >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> >  	reg &= ~info->mux_mask;
> >> >  	reg |= imx_pin->config;
> >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> >>
> >> ... and muxed...
> >>
> >> Not sure if we want that.
> >>
> >> I had to control GPIO output/input through pinctrl since Vybrid does
> >> not allow to control that from the GPIO block.
> >>
> >> However, according to your GPIO patchset, the i.MX 7ULP has a new
> >> register GPIO_PDDR to control output from the GPIO block. Is
> >> controlling the output driver from IOMUXC still required?
> >
> > Yes, it's still required.
> >
> 
> That sounds weird, what is the GPIO_PDDR for then? Sure I  need to enable
> the output driver to drive the pin, but can I disable output just using
> GPIO_PDDR?

No, to fully disable a output, you must disable OBE as well.

> 
> Maybe we have a miss understanding here:
> 
> Lets assume we want to switch a GPIO between output and input:
> 
> echo "output" > /sys/class/gpio/gpioN/direction ..
> echo "input" > /sys/class/gpio/gpioN/direction
> 
> Do I need to disable the output driver in the IOMUXC or can we just
> disable GPIO_PDDR and use the pin as input?
> 

OBE should also be disabled. Otherwise the input may not function well.

> If we can disable the output driver just using GPIO_PDDR, we can avoid the
> gpio_set_direction cross call.
> 
> 
> >> If not, I really would just not use all that "find pinctrl config"
> >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
> >> that only for Vybrid.
> >>
> >> This would also align much better with the other i.MX devices, where
> >> pinmuxing and GPIO is completely orthogonal.
> >>
> >
> > Actually this patch came only because to make the exist code not break
> > MX7ULP.
> >
> > Actually I'm wondering why we need implement
> > imx_pmx_gpio_request_enable function?
> >
> > Per my understanding, IMX binding already set GPIO mux by Parsing MUX
> > mode from device tree, so why need gpio_request_enable which looks
> > like is duplicated.
> >
> > Can you help explain it?
> 
> I suggest to read this clarification email wrt to pinctrl/gpio from Linus
> Walleij:
> https://lkml.org/lkml/2016/10/10/87
> 
> To summarize: We have different semantics in Vybrid: The GPIO subsystem
> automatically mux the GPIO for you. So in Vybrid, you do not have to mux a
> GPIO (but a valid entry in your device tree is needed, but does not
> assigned to any node).

Okay, Clearer now.

But I do see the users of GPIO pads in Vybrid dts.
Above is an example which make me confuse at first. 

> 
> Is what the driver is doing for Vybrid wrong? It is different from i.MX,
> but afaik, it is not really wrong... Its a valid implementation according
> to the currently defined semantics...  Due to the *need* to touch pinctrl
> for direction, I had to implement cross calls anyway, so I thought I might
> as well go full mile and also mux the GPIO on request...
> 

It's not strickly wrong.
Just a bit confuse that gpio_request_enable seems not quite necessary
As we actually already and must define GPIO mux in device tree according
To standard IMX binding format.
e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
That means pinctrl already does the GPIO mux when enable sd function.

> So the question is, what semantic do we want for i.MX 7ULP? Since it is a
> i.MX device, we probably want the same semantics as i.MX 6/7 is already
> using for the sake of consistency. So in this case the
> gpio_request_enable/disable callbacks are not needed...
> 
> This is how I hope we can do the implementation for i.MX 7ULP:
> - Do not use gpio_request_enable/disable

Yes, we do want that.

> - Do not use gpio_set_direction either

Not, ULP needs it to support GPIO direction switch.

> - Users using a GPIO should enable output driver in IOMUXC (just use a pin
> configuration where output driver is enabled)

Users still need configure OBE/IBE in devicetree for statically assignment.

> - The GPIO driver only enables/disables the output driver using its
> GPIO_PDDR register depending on GPIO direction

No, same reason as the second question.


So, finnaly, I think the solution may be:
1. If Vybrid does not use gpio_request_enable/disable, we can simply
remove it. Both driver keeps using pinctrl gpio_set_direction.

Or.

2. Make gpio_request_enable/disable and gpio_set_direction
As pinctrl-imx core driver callbacks. And only assign gpio_set_direction
For IMX7ULP platform driver while assign both for Vybrid.

Which one would you prefer?

Regards
Dong Aisheng

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Thursday, May 18, 2017 2:16 AM
> To: A.S. Dong
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-17 00:18, A.S. Dong wrote:
> >> -----Original Message-----
> >> From: Stefan Agner [mailto:stefan at agner.ch]
> >> Sent: Tuesday, May 16, 2017 1:27 AM
> >> To: A.S. Dong
> >> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> >> kernel at pengutronix.de; Alexandre Courbot
> >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> >> gpio
> >>
> >> On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > Do not assume MUX 0 is GPIO function in core driver as a different
> >> > SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> >> >
> >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > Cc: Bai Ping <ping.bai@nxp.com>
> >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > ---
> >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > index 0d6aaca..ed8ea32 100644
> >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  			continue;
> >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> >> > +			if (imx_pin->pin == offset)
> >> >  				goto mux_pin;
> >>
> >> The reason I added that check was to make sure we pick a mux option
> >> which is GPIO... With this change, any pinmux might be picked...
> >>
> >
> > First of all, this seems to be wrong to me that GPIO mux mode is SoC
> > Dependant and should not be put in pinctrl-imx core driver.
> 
> Hm, agree, we should consider to move
> imx_pmx_gpio_request_enable/disable_free and imx_pmx_gpio_set_direction
> into pinctrl-vf610.c
> 

IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
dynamically change GPIO from output to input.

> >
> > Secondly, I think we may be over worried and it's not quite necessary
> > As we did not do the sanity check for both raw config and mux data
> > read From Device tree, why only do it for GPIO?
> >
> > We may trust the data in device tree.
> 
> In Vybrid, there is no need to explicitly assign a pinmux to use a pin as
> GPIO. So the pinmux could be anything... The implemented semantics for
> Vyrbid is really different than i.MX, see below.
> 

Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
e.g.
arch/arm/boot/dts/vf-colibri.dtsi
pinctrl_esdhc1: esdhc1grp {
        fsl,pins = <
                VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
                VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
                VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
                VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
                VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
                VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
                VF610_PAD_PTB20__GPIO_42        0x219d
        >;
};  

> >
> >> >  		}
> >> >  	}
> >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> >> > pinctrl_dev *pctldev,
> >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> >  	reg &= ~info->mux_mask;
> >> >  	reg |= imx_pin->config;
> >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> >>
> >> ... and muxed...
> >>
> >> Not sure if we want that.
> >>
> >> I had to control GPIO output/input through pinctrl since Vybrid does
> >> not allow to control that from the GPIO block.
> >>
> >> However, according to your GPIO patchset, the i.MX 7ULP has a new
> >> register GPIO_PDDR to control output from the GPIO block. Is
> >> controlling the output driver from IOMUXC still required?
> >
> > Yes, it's still required.
> >
> 
> That sounds weird, what is the GPIO_PDDR for then? Sure I  need to enable
> the output driver to drive the pin, but can I disable output just using
> GPIO_PDDR?

No, to fully disable a output, you must disable OBE as well.

> 
> Maybe we have a miss understanding here:
> 
> Lets assume we want to switch a GPIO between output and input:
> 
> echo "output" > /sys/class/gpio/gpioN/direction ..
> echo "input" > /sys/class/gpio/gpioN/direction
> 
> Do I need to disable the output driver in the IOMUXC or can we just
> disable GPIO_PDDR and use the pin as input?
> 

OBE should also be disabled. Otherwise the input may not function well.

> If we can disable the output driver just using GPIO_PDDR, we can avoid the
> gpio_set_direction cross call.
> 
> 
> >> If not, I really would just not use all that "find pinctrl config"
> >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
> >> that only for Vybrid.
> >>
> >> This would also align much better with the other i.MX devices, where
> >> pinmuxing and GPIO is completely orthogonal.
> >>
> >
> > Actually this patch came only because to make the exist code not break
> > MX7ULP.
> >
> > Actually I'm wondering why we need implement
> > imx_pmx_gpio_request_enable function?
> >
> > Per my understanding, IMX binding already set GPIO mux by Parsing MUX
> > mode from device tree, so why need gpio_request_enable which looks
> > like is duplicated.
> >
> > Can you help explain it?
> 
> I suggest to read this clarification email wrt to pinctrl/gpio from Linus
> Walleij:
> https://lkml.org/lkml/2016/10/10/87
> 
> To summarize: We have different semantics in Vybrid: The GPIO subsystem
> automatically mux the GPIO for you. So in Vybrid, you do not have to mux a
> GPIO (but a valid entry in your device tree is needed, but does not
> assigned to any node).

Okay, Clearer now.

But I do see the users of GPIO pads in Vybrid dts.
Above is an example which make me confuse at first. 

> 
> Is what the driver is doing for Vybrid wrong? It is different from i.MX,
> but afaik, it is not really wrong... Its a valid implementation according
> to the currently defined semantics...  Due to the *need* to touch pinctrl
> for direction, I had to implement cross calls anyway, so I thought I might
> as well go full mile and also mux the GPIO on request...
> 

It's not strickly wrong.
Just a bit confuse that gpio_request_enable seems not quite necessary
As we actually already and must define GPIO mux in device tree according
To standard IMX binding format.
e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
That means pinctrl already does the GPIO mux when enable sd function.

> So the question is, what semantic do we want for i.MX 7ULP? Since it is a
> i.MX device, we probably want the same semantics as i.MX 6/7 is already
> using for the sake of consistency. So in this case the
> gpio_request_enable/disable callbacks are not needed...
> 
> This is how I hope we can do the implementation for i.MX 7ULP:
> - Do not use gpio_request_enable/disable

Yes, we do want that.

> - Do not use gpio_set_direction either

Not, ULP needs it to support GPIO direction switch.

> - Users using a GPIO should enable output driver in IOMUXC (just use a pin
> configuration where output driver is enabled)

Users still need configure OBE/IBE in devicetree for statically assignment.

> - The GPIO driver only enables/disables the output driver using its
> GPIO_PDDR register depending on GPIO direction

No, same reason as the second question.


So, finnaly, I think the solution may be:
1. If Vybrid does not use gpio_request_enable/disable, we can simply
remove it. Both driver keeps using pinctrl gpio_set_direction.

Or.

2. Make gpio_request_enable/disable and gpio_set_direction
As pinctrl-imx core driver callbacks. And only assign gpio_set_direction
For IMX7ULP platform driver while assign both for Vybrid.

Which one would you prefer?

Regards
Dong Aisheng

RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: A.S. Dong
> Sent: Thursday, May 18, 2017 3:00 PM
> To: 'Stefan Agner'
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan@agner.ch]
> > Sent: Thursday, May 18, 2017 2:16 AM
> > To: A.S. Dong
> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> > kernel@pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> > gpio
> >
> > On 2017-05-17 00:18, A.S. Dong wrote:
> > >> -----Original Message-----
> > >> From: Stefan Agner [mailto:stefan@agner.ch]
> > >> Sent: Tuesday, May 16, 2017 1:27 AM
> > >> To: A.S. Dong
> > >> Cc: linux-gpio@vger.kernel.org;
> > >> linux-arm-kernel@lists.infradead.org;
> > >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
> > >> Duan; kernel@pengutronix.de; Alexandre Courbot
> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
> > >> is gpio
> > >>
> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
> > >> > Do not assume MUX 0 is GPIO function in core driver as a
> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> > >> >
> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
> > >> > Cc: Stefan Agner <stefan@agner.ch>
> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > >> > Cc: Bai Ping <ping.bai@nxp.com>
> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >> > ---
> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > index 0d6aaca..ed8ea32 100644
> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> > >> > pinctrl_dev *pctldev,
> > >> >  			continue;
> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> > >> > +			if (imx_pin->pin == offset)
> > >> >  				goto mux_pin;
> > >>
> > >> The reason I added that check was to make sure we pick a mux option
> > >> which is GPIO... With this change, any pinmux might be picked...
> > >>
> > >
> > > First of all, this seems to be wrong to me that GPIO mux mode is SoC
> > > Dependant and should not be put in pinctrl-imx core driver.
> >
> > Hm, agree, we should consider to move
> > imx_pmx_gpio_request_enable/disable_free and
> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
> >
> 
> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
> dynamically change GPIO from output to input.
> 
> > >
> > > Secondly, I think we may be over worried and it's not quite
> > > necessary As we did not do the sanity check for both raw config and
> > > mux data read From Device tree, why only do it for GPIO?
> > >
> > > We may trust the data in device tree.
> >
> > In Vybrid, there is no need to explicitly assign a pinmux to use a pin
> > as GPIO. So the pinmux could be anything... The implemented semantics
> > for Vyrbid is really different than i.MX, see below.
> >
> 
> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
> e.g.
> arch/arm/boot/dts/vf-colibri.dtsi
> pinctrl_esdhc1: esdhc1grp {
>         fsl,pins = <
>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
>                 VF610_PAD_PTB20__GPIO_42        0x219d
>         >;
> };
> 
> > >
> > >> >  		}
> > >> >  	}
> > >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> > >> > pinctrl_dev *pctldev,
> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >> >  	reg &= ~info->mux_mask;
> > >> >  	reg |= imx_pin->config;
> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> > >>
> > >> ... and muxed...
> > >>
> > >> Not sure if we want that.
> > >>
> > >> I had to control GPIO output/input through pinctrl since Vybrid
> > >> does not allow to control that from the GPIO block.
> > >>
> > >> However, according to your GPIO patchset, the i.MX 7ULP has a new
> > >> register GPIO_PDDR to control output from the GPIO block. Is
> > >> controlling the output driver from IOMUXC still required?
> > >
> > > Yes, it's still required.
> > >
> >
> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
> > enable the output driver to drive the pin, but can I disable output
> > just using GPIO_PDDR?
> 
> No, to fully disable a output, you must disable OBE as well.
> 
> >
> > Maybe we have a miss understanding here:
> >
> > Lets assume we want to switch a GPIO between output and input:
> >
> > echo "output" > /sys/class/gpio/gpioN/direction ..
> > echo "input" > /sys/class/gpio/gpioN/direction
> >
> > Do I need to disable the output driver in the IOMUXC or can we just
> > disable GPIO_PDDR and use the pin as input?
> >
> 
> OBE should also be disabled. Otherwise the input may not function well.
> 
> > If we can disable the output driver just using GPIO_PDDR, we can avoid
> > the gpio_set_direction cross call.
> >
> >
> > >> If not, I really would just not use all that "find pinctrl config"
> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
> > >> that only for Vybrid.
> > >>
> > >> This would also align much better with the other i.MX devices,
> > >> where pinmuxing and GPIO is completely orthogonal.
> > >>
> > >
> > > Actually this patch came only because to make the exist code not
> > > break MX7ULP.
> > >
> > > Actually I'm wondering why we need implement
> > > imx_pmx_gpio_request_enable function?
> > >
> > > Per my understanding, IMX binding already set GPIO mux by Parsing
> > > MUX mode from device tree, so why need gpio_request_enable which
> > > looks like is duplicated.
> > >
> > > Can you help explain it?
> >
> > I suggest to read this clarification email wrt to pinctrl/gpio from
> > Linus
> > Walleij:
> > https://lkml.org/lkml/2016/10/10/87
> >
> > To summarize: We have different semantics in Vybrid: The GPIO
> > subsystem automatically mux the GPIO for you. So in Vybrid, you do not
> > have to mux a GPIO (but a valid entry in your device tree is needed,
> > but does not assigned to any node).
> 
> Okay, Clearer now.
> 
> But I do see the users of GPIO pads in Vybrid dts.
> Above is an example which make me confuse at first.
> 
> >
> > Is what the driver is doing for Vybrid wrong? It is different from
> > i.MX, but afaik, it is not really wrong... Its a valid implementation
> > according to the currently defined semantics...  Due to the *need* to
> > touch pinctrl for direction, I had to implement cross calls anyway, so
> > I thought I might as well go full mile and also mux the GPIO on
> request...
> >
> 
> It's not strickly wrong.
> Just a bit confuse that gpio_request_enable seems not quite necessary As
> we actually already and must define GPIO mux in device tree according To
> standard IMX binding format.
> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
> That means pinctrl already does the GPIO mux when enable sd function.
> 
> > So the question is, what semantic do we want for i.MX 7ULP? Since it
> > is a i.MX device, we probably want the same semantics as i.MX 6/7 is
> > already using for the sake of consistency. So in this case the
> > gpio_request_enable/disable callbacks are not needed...
> >
> > This is how I hope we can do the implementation for i.MX 7ULP:
> > - Do not use gpio_request_enable/disable
> 
> Yes, we do want that.
> 
> > - Do not use gpio_set_direction either
> 
> Not, ULP needs it to support GPIO direction switch.
> 
> > - Users using a GPIO should enable output driver in IOMUXC (just use a
> > pin configuration where output driver is enabled)
> 
> Users still need configure OBE/IBE in devicetree for statically assignment.
> 
> > - The GPIO driver only enables/disables the output driver using its
> > GPIO_PDDR register depending on GPIO direction
> 
> No, same reason as the second question.
> 
> 
> So, finnaly, I think the solution may be:
> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
> remove it. Both driver keeps using pinctrl gpio_set_direction.
> 
> Or.
> 
> 2. Make gpio_request_enable/disable and gpio_set_direction As pinctrl-imx
> core driver callbacks. And only assign gpio_set_direction For IMX7ULP
> platform driver while assign both for Vybrid.
> 
> Which one would you prefer?
> 

Any answer about it?

Regards
Dong Aisheng

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

Hi Stefan,

> -----Original Message-----
> From: A.S. Dong
> Sent: Thursday, May 18, 2017 3:00 PM
> To: 'Stefan Agner'
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> > -----Original Message-----
> > From: Stefan Agner [mailto:stefan at agner.ch]
> > Sent: Thursday, May 18, 2017 2:16 AM
> > To: A.S. Dong
> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> > kernel at pengutronix.de; Alexandre Courbot
> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> > gpio
> >
> > On 2017-05-17 00:18, A.S. Dong wrote:
> > >> -----Original Message-----
> > >> From: Stefan Agner [mailto:stefan at agner.ch]
> > >> Sent: Tuesday, May 16, 2017 1:27 AM
> > >> To: A.S. Dong
> > >> Cc: linux-gpio at vger.kernel.org;
> > >> linux-arm-kernel at lists.infradead.org;
> > >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
> > >> Duan; kernel at pengutronix.de; Alexandre Courbot
> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
> > >> is gpio
> > >>
> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
> > >> > Do not assume MUX 0 is GPIO function in core driver as a
> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
> > >> >
> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
> > >> > Cc: Stefan Agner <stefan@agner.ch>
> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> > >> > Cc: Bai Ping <ping.bai@nxp.com>
> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > >> > ---
> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > index 0d6aaca..ed8ea32 100644
> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> > >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
> > >> > pinctrl_dev *pctldev,
> > >> >  			continue;
> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> > >> > +			if (imx_pin->pin == offset)
> > >> >  				goto mux_pin;
> > >>
> > >> The reason I added that check was to make sure we pick a mux option
> > >> which is GPIO... With this change, any pinmux might be picked...
> > >>
> > >
> > > First of all, this seems to be wrong to me that GPIO mux mode is SoC
> > > Dependant and should not be put in pinctrl-imx core driver.
> >
> > Hm, agree, we should consider to move
> > imx_pmx_gpio_request_enable/disable_free and
> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
> >
> 
> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
> dynamically change GPIO from output to input.
> 
> > >
> > > Secondly, I think we may be over worried and it's not quite
> > > necessary As we did not do the sanity check for both raw config and
> > > mux data read From Device tree, why only do it for GPIO?
> > >
> > > We may trust the data in device tree.
> >
> > In Vybrid, there is no need to explicitly assign a pinmux to use a pin
> > as GPIO. So the pinmux could be anything... The implemented semantics
> > for Vyrbid is really different than i.MX, see below.
> >
> 
> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
> e.g.
> arch/arm/boot/dts/vf-colibri.dtsi
> pinctrl_esdhc1: esdhc1grp {
>         fsl,pins = <
>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
>                 VF610_PAD_PTB20__GPIO_42        0x219d
>         >;
> };
> 
> > >
> > >> >  		}
> > >> >  	}
> > >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
> > >> > pinctrl_dev *pctldev,
> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> > >> >  	reg &= ~info->mux_mask;
> > >> >  	reg |= imx_pin->config;
> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> > >>
> > >> ... and muxed...
> > >>
> > >> Not sure if we want that.
> > >>
> > >> I had to control GPIO output/input through pinctrl since Vybrid
> > >> does not allow to control that from the GPIO block.
> > >>
> > >> However, according to your GPIO patchset, the i.MX 7ULP has a new
> > >> register GPIO_PDDR to control output from the GPIO block. Is
> > >> controlling the output driver from IOMUXC still required?
> > >
> > > Yes, it's still required.
> > >
> >
> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
> > enable the output driver to drive the pin, but can I disable output
> > just using GPIO_PDDR?
> 
> No, to fully disable a output, you must disable OBE as well.
> 
> >
> > Maybe we have a miss understanding here:
> >
> > Lets assume we want to switch a GPIO between output and input:
> >
> > echo "output" > /sys/class/gpio/gpioN/direction ..
> > echo "input" > /sys/class/gpio/gpioN/direction
> >
> > Do I need to disable the output driver in the IOMUXC or can we just
> > disable GPIO_PDDR and use the pin as input?
> >
> 
> OBE should also be disabled. Otherwise the input may not function well.
> 
> > If we can disable the output driver just using GPIO_PDDR, we can avoid
> > the gpio_set_direction cross call.
> >
> >
> > >> If not, I really would just not use all that "find pinctrl config"
> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
> > >> that only for Vybrid.
> > >>
> > >> This would also align much better with the other i.MX devices,
> > >> where pinmuxing and GPIO is completely orthogonal.
> > >>
> > >
> > > Actually this patch came only because to make the exist code not
> > > break MX7ULP.
> > >
> > > Actually I'm wondering why we need implement
> > > imx_pmx_gpio_request_enable function?
> > >
> > > Per my understanding, IMX binding already set GPIO mux by Parsing
> > > MUX mode from device tree, so why need gpio_request_enable which
> > > looks like is duplicated.
> > >
> > > Can you help explain it?
> >
> > I suggest to read this clarification email wrt to pinctrl/gpio from
> > Linus
> > Walleij:
> > https://lkml.org/lkml/2016/10/10/87
> >
> > To summarize: We have different semantics in Vybrid: The GPIO
> > subsystem automatically mux the GPIO for you. So in Vybrid, you do not
> > have to mux a GPIO (but a valid entry in your device tree is needed,
> > but does not assigned to any node).
> 
> Okay, Clearer now.
> 
> But I do see the users of GPIO pads in Vybrid dts.
> Above is an example which make me confuse at first.
> 
> >
> > Is what the driver is doing for Vybrid wrong? It is different from
> > i.MX, but afaik, it is not really wrong... Its a valid implementation
> > according to the currently defined semantics...  Due to the *need* to
> > touch pinctrl for direction, I had to implement cross calls anyway, so
> > I thought I might as well go full mile and also mux the GPIO on
> request...
> >
> 
> It's not strickly wrong.
> Just a bit confuse that gpio_request_enable seems not quite necessary As
> we actually already and must define GPIO mux in device tree according To
> standard IMX binding format.
> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
> That means pinctrl already does the GPIO mux when enable sd function.
> 
> > So the question is, what semantic do we want for i.MX 7ULP? Since it
> > is a i.MX device, we probably want the same semantics as i.MX 6/7 is
> > already using for the sake of consistency. So in this case the
> > gpio_request_enable/disable callbacks are not needed...
> >
> > This is how I hope we can do the implementation for i.MX 7ULP:
> > - Do not use gpio_request_enable/disable
> 
> Yes, we do want that.
> 
> > - Do not use gpio_set_direction either
> 
> Not, ULP needs it to support GPIO direction switch.
> 
> > - Users using a GPIO should enable output driver in IOMUXC (just use a
> > pin configuration where output driver is enabled)
> 
> Users still need configure OBE/IBE in devicetree for statically assignment.
> 
> > - The GPIO driver only enables/disables the output driver using its
> > GPIO_PDDR register depending on GPIO direction
> 
> No, same reason as the second question.
> 
> 
> So, finnaly, I think the solution may be:
> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
> remove it. Both driver keeps using pinctrl gpio_set_direction.
> 
> Or.
> 
> 2. Make gpio_request_enable/disable and gpio_set_direction As pinctrl-imx
> core driver callbacks. And only assign gpio_set_direction For IMX7ULP
> platform driver while assign both for Vybrid.
> 
> Which one would you prefer?
> 

Any answer about it?

Regards
Dong Aisheng

Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-23 03:23, A.S. Dong wrote:
> Hi Stefan,
> 
>> -----Original Message-----
>> From: A.S. Dong
>> Sent: Thursday, May 18, 2017 3:00 PM
>> To: 'Stefan Agner'
>> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
>> kernel@pengutronix.de; Alexandre Courbot
>> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
>>
>> > -----Original Message-----
>> > From: Stefan Agner [mailto:stefan@agner.ch]
>> > Sent: Thursday, May 18, 2017 2:16 AM
>> > To: A.S. Dong
>> > Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
>> > kernel@pengutronix.de; Alexandre Courbot
>> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
>> > gpio
>> >
>> > On 2017-05-17 00:18, A.S. Dong wrote:
>> > >> -----Original Message-----
>> > >> From: Stefan Agner [mailto:stefan@agner.ch]
>> > >> Sent: Tuesday, May 16, 2017 1:27 AM
>> > >> To: A.S. Dong
>> > >> Cc: linux-gpio@vger.kernel.org;
>> > >> linux-arm-kernel@lists.infradead.org;
>> > >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
>> > >> Duan; kernel@pengutronix.de; Alexandre Courbot
>> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
>> > >> is gpio
>> > >>
>> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
>> > >> > Do not assume MUX 0 is GPIO function in core driver as a
>> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
>> > >> >
>> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
>> > >> > Cc: Stefan Agner <stefan@agner.ch>
>> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > >> > Cc: Bai Ping <ping.bai@nxp.com>
>> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > >> > ---
>> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > index 0d6aaca..ed8ea32 100644
>> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > >> > pinctrl_dev *pctldev,
>> > >> >  			continue;
>> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
>> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
>> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
>> > >> > +			if (imx_pin->pin == offset)
>> > >> >  				goto mux_pin;
>> > >>
>> > >> The reason I added that check was to make sure we pick a mux option
>> > >> which is GPIO... With this change, any pinmux might be picked...
>> > >>
>> > >
>> > > First of all, this seems to be wrong to me that GPIO mux mode is SoC
>> > > Dependant and should not be put in pinctrl-imx core driver.
>> >
>> > Hm, agree, we should consider to move
>> > imx_pmx_gpio_request_enable/disable_free and
>> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
>> >
>>
>> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
>> dynamically change GPIO from output to input.
>>
>> > >
>> > > Secondly, I think we may be over worried and it's not quite
>> > > necessary As we did not do the sanity check for both raw config and
>> > > mux data read From Device tree, why only do it for GPIO?
>> > >
>> > > We may trust the data in device tree.
>> >
>> > In Vybrid, there is no need to explicitly assign a pinmux to use a pin
>> > as GPIO. So the pinmux could be anything... The implemented semantics
>> > for Vyrbid is really different than i.MX, see below.
>> >
>>
>> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
>> e.g.
>> arch/arm/boot/dts/vf-colibri.dtsi
>> pinctrl_esdhc1: esdhc1grp {
>>         fsl,pins = <
>>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
>>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
>>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
>>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
>>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
>>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
>>                 VF610_PAD_PTB20__GPIO_42        0x219d
>>         >;
>> };
>>
>> > >
>> > >> >  		}
>> > >> >  	}
>> > >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > >> > pinctrl_dev *pctldev,
>> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> > >> >  	reg &= ~info->mux_mask;
>> > >> >  	reg |= imx_pin->config;
>> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
>> > >>
>> > >> ... and muxed...
>> > >>
>> > >> Not sure if we want that.
>> > >>
>> > >> I had to control GPIO output/input through pinctrl since Vybrid
>> > >> does not allow to control that from the GPIO block.
>> > >>
>> > >> However, according to your GPIO patchset, the i.MX 7ULP has a new
>> > >> register GPIO_PDDR to control output from the GPIO block. Is
>> > >> controlling the output driver from IOMUXC still required?
>> > >
>> > > Yes, it's still required.
>> > >
>> >
>> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
>> > enable the output driver to drive the pin, but can I disable output
>> > just using GPIO_PDDR?
>>
>> No, to fully disable a output, you must disable OBE as well.
>>
>> >
>> > Maybe we have a miss understanding here:
>> >
>> > Lets assume we want to switch a GPIO between output and input:
>> >
>> > echo "output" > /sys/class/gpio/gpioN/direction ..
>> > echo "input" > /sys/class/gpio/gpioN/direction
>> >
>> > Do I need to disable the output driver in the IOMUXC or can we just
>> > disable GPIO_PDDR and use the pin as input?
>> >
>>
>> OBE should also be disabled. Otherwise the input may not function well.
>>
>> > If we can disable the output driver just using GPIO_PDDR, we can avoid
>> > the gpio_set_direction cross call.
>> >
>> >
>> > >> If not, I really would just not use all that "find pinctrl config"
>> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
>> > >> that only for Vybrid.
>> > >>
>> > >> This would also align much better with the other i.MX devices,
>> > >> where pinmuxing and GPIO is completely orthogonal.
>> > >>
>> > >
>> > > Actually this patch came only because to make the exist code not
>> > > break MX7ULP.
>> > >
>> > > Actually I'm wondering why we need implement
>> > > imx_pmx_gpio_request_enable function?
>> > >
>> > > Per my understanding, IMX binding already set GPIO mux by Parsing
>> > > MUX mode from device tree, so why need gpio_request_enable which
>> > > looks like is duplicated.
>> > >
>> > > Can you help explain it?
>> >
>> > I suggest to read this clarification email wrt to pinctrl/gpio from
>> > Linus
>> > Walleij:
>> > https://lkml.org/lkml/2016/10/10/87
>> >
>> > To summarize: We have different semantics in Vybrid: The GPIO
>> > subsystem automatically mux the GPIO for you. So in Vybrid, you do not
>> > have to mux a GPIO (but a valid entry in your device tree is needed,
>> > but does not assigned to any node).
>>
>> Okay, Clearer now.
>>
>> But I do see the users of GPIO pads in Vybrid dts.
>> Above is an example which make me confuse at first.
>>
>> >
>> > Is what the driver is doing for Vybrid wrong? It is different from
>> > i.MX, but afaik, it is not really wrong... Its a valid implementation
>> > according to the currently defined semantics...  Due to the *need* to
>> > touch pinctrl for direction, I had to implement cross calls anyway, so
>> > I thought I might as well go full mile and also mux the GPIO on
>> request...
>> >
>>
>> It's not strickly wrong.
>> Just a bit confuse that gpio_request_enable seems not quite necessary As
>> we actually already and must define GPIO mux in device tree according To
>> standard IMX binding format.
>> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
>> That means pinctrl already does the GPIO mux when enable sd function.
>>
>> > So the question is, what semantic do we want for i.MX 7ULP? Since it
>> > is a i.MX device, we probably want the same semantics as i.MX 6/7 is
>> > already using for the sake of consistency. So in this case the
>> > gpio_request_enable/disable callbacks are not needed...
>> >
>> > This is how I hope we can do the implementation for i.MX 7ULP:
>> > - Do not use gpio_request_enable/disable
>>
>> Yes, we do want that.
>>
>> > - Do not use gpio_set_direction either
>>
>> Not, ULP needs it to support GPIO direction switch.
>>
>> > - Users using a GPIO should enable output driver in IOMUXC (just use a
>> > pin configuration where output driver is enabled)
>>
>> Users still need configure OBE/IBE in devicetree for statically assignment.
>>
>> > - The GPIO driver only enables/disables the output driver using its
>> > GPIO_PDDR register depending on GPIO direction
>>
>> No, same reason as the second question.
>>
>>
>> So, finnaly, I think the solution may be:
>> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
>> remove it. Both driver keeps using pinctrl gpio_set_direction.
>>
>> Or.
>>
>> 2. Make gpio_request_enable/disable and gpio_set_direction As pinctrl-imx
>> core driver callbacks. And only assign gpio_set_direction For IMX7ULP
>> platform driver while assign both for Vybrid.
>>
>> Which one would you prefer?

1 would mean a semantic change.

For all GPIO I checked in upstream device trees we assign a pinctrl to
the same node, so in all cases gpio_request_enable/disable is really
unnecessary.

And since the current implementation has adversarial effects for I2C
recovery (https://patchwork.kernel.org/patch/9351401/), we use the
orthogonal semantic in the closely related i.MX SoCs and it even
simplifies the driver, I am ok to change the semantic.

Can you prepare such a patchset so that we can further test that on
Vybrid?

--
Stefan

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[Stefan Agner]

On 2017-05-23 03:23, A.S. Dong wrote:
> Hi Stefan,
> 
>> -----Original Message-----
>> From: A.S. Dong
>> Sent: Thursday, May 18, 2017 3:00 PM
>> To: 'Stefan Agner'
>> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
>> kernel at pengutronix.de; Alexandre Courbot
>> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
>>
>> > -----Original Message-----
>> > From: Stefan Agner [mailto:stefan at agner.ch]
>> > Sent: Thursday, May 18, 2017 2:16 AM
>> > To: A.S. Dong
>> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
>> > kernel at pengutronix.de; Alexandre Courbot
>> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
>> > gpio
>> >
>> > On 2017-05-17 00:18, A.S. Dong wrote:
>> > >> -----Original Message-----
>> > >> From: Stefan Agner [mailto:stefan at agner.ch]
>> > >> Sent: Tuesday, May 16, 2017 1:27 AM
>> > >> To: A.S. Dong
>> > >> Cc: linux-gpio at vger.kernel.org;
>> > >> linux-arm-kernel at lists.infradead.org;
>> > >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
>> > >> Duan; kernel at pengutronix.de; Alexandre Courbot
>> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
>> > >> is gpio
>> > >>
>> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
>> > >> > Do not assume MUX 0 is GPIO function in core driver as a
>> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is GPIO.
>> > >> >
>> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
>> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
>> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
>> > >> > Cc: Stefan Agner <stefan@agner.ch>
>> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
>> > >> > Cc: Bai Ping <ping.bai@nxp.com>
>> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
>> > >> > ---
>> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
>> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > index 0d6aaca..ed8ea32 100644
>> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
>> > >> > @@ -281,7 +281,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > >> > pinctrl_dev *pctldev,
>> > >> >  			continue;
>> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
>> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
>> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
>> > >> > +			if (imx_pin->pin == offset)
>> > >> >  				goto mux_pin;
>> > >>
>> > >> The reason I added that check was to make sure we pick a mux option
>> > >> which is GPIO... With this change, any pinmux might be picked...
>> > >>
>> > >
>> > > First of all, this seems to be wrong to me that GPIO mux mode is SoC
>> > > Dependant and should not be put in pinctrl-imx core driver.
>> >
>> > Hm, agree, we should consider to move
>> > imx_pmx_gpio_request_enable/disable_free and
>> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
>> >
>>
>> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
>> dynamically change GPIO from output to input.
>>
>> > >
>> > > Secondly, I think we may be over worried and it's not quite
>> > > necessary As we did not do the sanity check for both raw config and
>> > > mux data read From Device tree, why only do it for GPIO?
>> > >
>> > > We may trust the data in device tree.
>> >
>> > In Vybrid, there is no need to explicitly assign a pinmux to use a pin
>> > as GPIO. So the pinmux could be anything... The implemented semantics
>> > for Vyrbid is really different than i.MX, see below.
>> >
>>
>> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
>> e.g.
>> arch/arm/boot/dts/vf-colibri.dtsi
>> pinctrl_esdhc1: esdhc1grp {
>>         fsl,pins = <
>>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
>>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
>>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
>>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
>>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
>>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
>>                 VF610_PAD_PTB20__GPIO_42        0x219d
>>         >;
>> };
>>
>> > >
>> > >> >  		}
>> > >> >  	}
>> > >> > @@ -292,6 +292,7 @@ static int imx_pmx_gpio_request_enable(struct
>> > >> > pinctrl_dev *pctldev,
>> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
>> > >> >  	reg &= ~info->mux_mask;
>> > >> >  	reg |= imx_pin->config;
>> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
>> > >>
>> > >> ... and muxed...
>> > >>
>> > >> Not sure if we want that.
>> > >>
>> > >> I had to control GPIO output/input through pinctrl since Vybrid
>> > >> does not allow to control that from the GPIO block.
>> > >>
>> > >> However, according to your GPIO patchset, the i.MX 7ULP has a new
>> > >> register GPIO_PDDR to control output from the GPIO block. Is
>> > >> controlling the output driver from IOMUXC still required?
>> > >
>> > > Yes, it's still required.
>> > >
>> >
>> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
>> > enable the output driver to drive the pin, but can I disable output
>> > just using GPIO_PDDR?
>>
>> No, to fully disable a output, you must disable OBE as well.
>>
>> >
>> > Maybe we have a miss understanding here:
>> >
>> > Lets assume we want to switch a GPIO between output and input:
>> >
>> > echo "output" > /sys/class/gpio/gpioN/direction ..
>> > echo "input" > /sys/class/gpio/gpioN/direction
>> >
>> > Do I need to disable the output driver in the IOMUXC or can we just
>> > disable GPIO_PDDR and use the pin as input?
>> >
>>
>> OBE should also be disabled. Otherwise the input may not function well.
>>
>> > If we can disable the output driver just using GPIO_PDDR, we can avoid
>> > the gpio_set_direction cross call.
>> >
>> >
>> > >> If not, I really would just not use all that "find pinctrl config"
>> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and set
>> > >> that only for Vybrid.
>> > >>
>> > >> This would also align much better with the other i.MX devices,
>> > >> where pinmuxing and GPIO is completely orthogonal.
>> > >>
>> > >
>> > > Actually this patch came only because to make the exist code not
>> > > break MX7ULP.
>> > >
>> > > Actually I'm wondering why we need implement
>> > > imx_pmx_gpio_request_enable function?
>> > >
>> > > Per my understanding, IMX binding already set GPIO mux by Parsing
>> > > MUX mode from device tree, so why need gpio_request_enable which
>> > > looks like is duplicated.
>> > >
>> > > Can you help explain it?
>> >
>> > I suggest to read this clarification email wrt to pinctrl/gpio from
>> > Linus
>> > Walleij:
>> > https://lkml.org/lkml/2016/10/10/87
>> >
>> > To summarize: We have different semantics in Vybrid: The GPIO
>> > subsystem automatically mux the GPIO for you. So in Vybrid, you do not
>> > have to mux a GPIO (but a valid entry in your device tree is needed,
>> > but does not assigned to any node).
>>
>> Okay, Clearer now.
>>
>> But I do see the users of GPIO pads in Vybrid dts.
>> Above is an example which make me confuse at first.
>>
>> >
>> > Is what the driver is doing for Vybrid wrong? It is different from
>> > i.MX, but afaik, it is not really wrong... Its a valid implementation
>> > according to the currently defined semantics...  Due to the *need* to
>> > touch pinctrl for direction, I had to implement cross calls anyway, so
>> > I thought I might as well go full mile and also mux the GPIO on
>> request...
>> >
>>
>> It's not strickly wrong.
>> Just a bit confuse that gpio_request_enable seems not quite necessary As
>> we actually already and must define GPIO mux in device tree according To
>> standard IMX binding format.
>> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
>> That means pinctrl already does the GPIO mux when enable sd function.
>>
>> > So the question is, what semantic do we want for i.MX 7ULP? Since it
>> > is a i.MX device, we probably want the same semantics as i.MX 6/7 is
>> > already using for the sake of consistency. So in this case the
>> > gpio_request_enable/disable callbacks are not needed...
>> >
>> > This is how I hope we can do the implementation for i.MX 7ULP:
>> > - Do not use gpio_request_enable/disable
>>
>> Yes, we do want that.
>>
>> > - Do not use gpio_set_direction either
>>
>> Not, ULP needs it to support GPIO direction switch.
>>
>> > - Users using a GPIO should enable output driver in IOMUXC (just use a
>> > pin configuration where output driver is enabled)
>>
>> Users still need configure OBE/IBE in devicetree for statically assignment.
>>
>> > - The GPIO driver only enables/disables the output driver using its
>> > GPIO_PDDR register depending on GPIO direction
>>
>> No, same reason as the second question.
>>
>>
>> So, finnaly, I think the solution may be:
>> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
>> remove it. Both driver keeps using pinctrl gpio_set_direction.
>>
>> Or.
>>
>> 2. Make gpio_request_enable/disable and gpio_set_direction As pinctrl-imx
>> core driver callbacks. And only assign gpio_set_direction For IMX7ULP
>> platform driver while assign both for Vybrid.
>>
>> Which one would you prefer?

1 would mean a semantic change.

For all GPIO I checked in upstream device trees we assign a pinctrl to
the same node, so in all cases gpio_request_enable/disable is really
unnecessary.

And since the current implementation has adversarial effects for I2C
recovery (https://patchwork.kernel.org/patch/9351401/), we use the
orthogonal semantic in the closely related i.MX SoCs and it even
simplifies the driver, I am ok to change the semantic.

Can you prepare such a patchset so that we can further test that on
Vybrid?

--
Stefan

RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Wednesday, May 24, 2017 3:55 AM
> To: A.S. Dong
> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> kernel@pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-23 03:23, A.S. Dong wrote:
> > Hi Stefan,
> >
> >> -----Original Message-----
> >> From: A.S. Dong
> >> Sent: Thursday, May 18, 2017 3:00 PM
> >> To: 'Stefan Agner'
> >> Cc: linux-gpio@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy Duan;
> >> kernel@pengutronix.de; Alexandre Courbot
> >> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> >> gpio
> >>
> >> > -----Original Message-----
> >> > From: Stefan Agner [mailto:stefan@agner.ch]
> >> > Sent: Thursday, May 18, 2017 2:16 AM
> >> > To: A.S. Dong
> >> > Cc: linux-gpio@vger.kernel.org;
> >> > linux-arm-kernel@lists.infradead.org;
> >> > linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
> >> > Duan; kernel@pengutronix.de; Alexandre Courbot
> >> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
> >> > is gpio
> >> >
> >> > On 2017-05-17 00:18, A.S. Dong wrote:
> >> > >> -----Original Message-----
> >> > >> From: Stefan Agner [mailto:stefan@agner.ch]
> >> > >> Sent: Tuesday, May 16, 2017 1:27 AM
> >> > >> To: A.S. Dong
> >> > >> Cc: linux-gpio@vger.kernel.org;
> >> > >> linux-arm-kernel@lists.infradead.org;
> >> > >> linus.walleij@linaro.org; shawnguo@kernel.org; Jacky Bai; Andy
> >> > >> Duan; kernel@pengutronix.de; Alexandre Courbot
> >> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux
> >> > >> 0 is gpio
> >> > >>
> >> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > >> > Do not assume MUX 0 is GPIO function in core driver as a
> >> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is
> GPIO.
> >> > >> >
> >> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > >> > Cc: Bai Ping <ping.bai@nxp.com>
> >> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > >> > ---
> >> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >> >
> >> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > index 0d6aaca..ed8ea32 100644
> >> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > @@ -281,7 +281,7 @@ static int
> >> > >> > imx_pmx_gpio_request_enable(struct
> >> > >> > pinctrl_dev *pctldev,
> >> > >> >  			continue;
> >> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> >> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> >> > >> > +			if (imx_pin->pin == offset)
> >> > >> >  				goto mux_pin;
> >> > >>
> >> > >> The reason I added that check was to make sure we pick a mux
> >> > >> option which is GPIO... With this change, any pinmux might be
> picked...
> >> > >>
> >> > >
> >> > > First of all, this seems to be wrong to me that GPIO mux mode is
> >> > > SoC Dependant and should not be put in pinctrl-imx core driver.
> >> >
> >> > Hm, agree, we should consider to move
> >> > imx_pmx_gpio_request_enable/disable_free and
> >> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
> >> >
> >>
> >> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
> >> dynamically change GPIO from output to input.
> >>
> >> > >
> >> > > Secondly, I think we may be over worried and it's not quite
> >> > > necessary As we did not do the sanity check for both raw config
> >> > > and mux data read From Device tree, why only do it for GPIO?
> >> > >
> >> > > We may trust the data in device tree.
> >> >
> >> > In Vybrid, there is no need to explicitly assign a pinmux to use a
> >> > pin as GPIO. So the pinmux could be anything... The implemented
> >> > semantics for Vyrbid is really different than i.MX, see below.
> >> >
> >>
> >> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
> >> e.g.
> >> arch/arm/boot/dts/vf-colibri.dtsi
> >> pinctrl_esdhc1: esdhc1grp {
> >>         fsl,pins = <
> >>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
> >>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
> >>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
> >>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
> >>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
> >>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
> >>                 VF610_PAD_PTB20__GPIO_42        0x219d
> >>         >;
> >> };
> >>
> >> > >
> >> > >> >  		}
> >> > >> >  	}
> >> > >> > @@ -292,6 +292,7 @@ static int
> >> > >> > imx_pmx_gpio_request_enable(struct
> >> > >> > pinctrl_dev *pctldev,
> >> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> > >> >  	reg &= ~info->mux_mask;
> >> > >> >  	reg |= imx_pin->config;
> >> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> >> > >>
> >> > >> ... and muxed...
> >> > >>
> >> > >> Not sure if we want that.
> >> > >>
> >> > >> I had to control GPIO output/input through pinctrl since Vybrid
> >> > >> does not allow to control that from the GPIO block.
> >> > >>
> >> > >> However, according to your GPIO patchset, the i.MX 7ULP has a
> >> > >> new register GPIO_PDDR to control output from the GPIO block. Is
> >> > >> controlling the output driver from IOMUXC still required?
> >> > >
> >> > > Yes, it's still required.
> >> > >
> >> >
> >> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
> >> > enable the output driver to drive the pin, but can I disable output
> >> > just using GPIO_PDDR?
> >>
> >> No, to fully disable a output, you must disable OBE as well.
> >>
> >> >
> >> > Maybe we have a miss understanding here:
> >> >
> >> > Lets assume we want to switch a GPIO between output and input:
> >> >
> >> > echo "output" > /sys/class/gpio/gpioN/direction ..
> >> > echo "input" > /sys/class/gpio/gpioN/direction
> >> >
> >> > Do I need to disable the output driver in the IOMUXC or can we just
> >> > disable GPIO_PDDR and use the pin as input?
> >> >
> >>
> >> OBE should also be disabled. Otherwise the input may not function well.
> >>
> >> > If we can disable the output driver just using GPIO_PDDR, we can
> >> > avoid the gpio_set_direction cross call.
> >> >
> >> >
> >> > >> If not, I really would just not use all that "find pinctrl config"
> >> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and
> >> > >> set that only for Vybrid.
> >> > >>
> >> > >> This would also align much better with the other i.MX devices,
> >> > >> where pinmuxing and GPIO is completely orthogonal.
> >> > >>
> >> > >
> >> > > Actually this patch came only because to make the exist code not
> >> > > break MX7ULP.
> >> > >
> >> > > Actually I'm wondering why we need implement
> >> > > imx_pmx_gpio_request_enable function?
> >> > >
> >> > > Per my understanding, IMX binding already set GPIO mux by Parsing
> >> > > MUX mode from device tree, so why need gpio_request_enable which
> >> > > looks like is duplicated.
> >> > >
> >> > > Can you help explain it?
> >> >
> >> > I suggest to read this clarification email wrt to pinctrl/gpio from
> >> > Linus
> >> > Walleij:
> >> > https://lkml.org/lkml/2016/10/10/87
> >> >
> >> > To summarize: We have different semantics in Vybrid: The GPIO
> >> > subsystem automatically mux the GPIO for you. So in Vybrid, you do
> >> > not have to mux a GPIO (but a valid entry in your device tree is
> >> > needed, but does not assigned to any node).
> >>
> >> Okay, Clearer now.
> >>
> >> But I do see the users of GPIO pads in Vybrid dts.
> >> Above is an example which make me confuse at first.
> >>
> >> >
> >> > Is what the driver is doing for Vybrid wrong? It is different from
> >> > i.MX, but afaik, it is not really wrong... Its a valid
> >> > implementation according to the currently defined semantics...  Due
> >> > to the *need* to touch pinctrl for direction, I had to implement
> >> > cross calls anyway, so I thought I might as well go full mile and
> >> > also mux the GPIO on
> >> request...
> >> >
> >>
> >> It's not strickly wrong.
> >> Just a bit confuse that gpio_request_enable seems not quite necessary
> >> As we actually already and must define GPIO mux in device tree
> >> according To standard IMX binding format.
> >> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
> >> That means pinctrl already does the GPIO mux when enable sd function.
> >>
> >> > So the question is, what semantic do we want for i.MX 7ULP? Since
> >> > it is a i.MX device, we probably want the same semantics as i.MX
> >> > 6/7 is already using for the sake of consistency. So in this case
> >> > the gpio_request_enable/disable callbacks are not needed...
> >> >
> >> > This is how I hope we can do the implementation for i.MX 7ULP:
> >> > - Do not use gpio_request_enable/disable
> >>
> >> Yes, we do want that.
> >>
> >> > - Do not use gpio_set_direction either
> >>
> >> Not, ULP needs it to support GPIO direction switch.
> >>
> >> > - Users using a GPIO should enable output driver in IOMUXC (just
> >> > use a pin configuration where output driver is enabled)
> >>
> >> Users still need configure OBE/IBE in devicetree for statically
> assignment.
> >>
> >> > - The GPIO driver only enables/disables the output driver using its
> >> > GPIO_PDDR register depending on GPIO direction
> >>
> >> No, same reason as the second question.
> >>
> >>
> >> So, finnaly, I think the solution may be:
> >> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
> >> remove it. Both driver keeps using pinctrl gpio_set_direction.
> >>
> >> Or.
> >>
> >> 2. Make gpio_request_enable/disable and gpio_set_direction As
> >> pinctrl-imx core driver callbacks. And only assign gpio_set_direction
> >> For IMX7ULP platform driver while assign both for Vybrid.
> >>
> >> Which one would you prefer?
> 
> 1 would mean a semantic change.
> 
> For all GPIO I checked in upstream device trees we assign a pinctrl to the
> same node, so in all cases gpio_request_enable/disable is really
> unnecessary.
> 
> And since the current implementation has adversarial effects for I2C
> recovery (https://patchwork.kernel.org/patch/9351401/), we use the
> orthogonal semantic in the closely related i.MX SoCs and it even
> simplifies the driver, I am ok to change the semantic.
> 

I checked the I2C recovery issue you mentioned above, it looks
Changing the semantic as I proposed may fix it as well.

> Can you prepare such a patchset so that we can further test that on Vybrid?
> 

I will prepare it.

Thanks

Regards
Dong Aisheng

[PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio

[A.S. Dong]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: Wednesday, May 24, 2017 3:55 AM
> To: A.S. Dong
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> kernel at pengutronix.de; Alexandre Courbot
> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is gpio
> 
> On 2017-05-23 03:23, A.S. Dong wrote:
> > Hi Stefan,
> >
> >> -----Original Message-----
> >> From: A.S. Dong
> >> Sent: Thursday, May 18, 2017 3:00 PM
> >> To: 'Stefan Agner'
> >> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy Duan;
> >> kernel at pengutronix.de; Alexandre Courbot
> >> Subject: RE: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0 is
> >> gpio
> >>
> >> > -----Original Message-----
> >> > From: Stefan Agner [mailto:stefan at agner.ch]
> >> > Sent: Thursday, May 18, 2017 2:16 AM
> >> > To: A.S. Dong
> >> > Cc: linux-gpio at vger.kernel.org;
> >> > linux-arm-kernel at lists.infradead.org;
> >> > linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
> >> > Duan; kernel at pengutronix.de; Alexandre Courbot
> >> > Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux 0
> >> > is gpio
> >> >
> >> > On 2017-05-17 00:18, A.S. Dong wrote:
> >> > >> -----Original Message-----
> >> > >> From: Stefan Agner [mailto:stefan at agner.ch]
> >> > >> Sent: Tuesday, May 16, 2017 1:27 AM
> >> > >> To: A.S. Dong
> >> > >> Cc: linux-gpio at vger.kernel.org;
> >> > >> linux-arm-kernel at lists.infradead.org;
> >> > >> linus.walleij at linaro.org; shawnguo at kernel.org; Jacky Bai; Andy
> >> > >> Duan; kernel at pengutronix.de; Alexandre Courbot
> >> > >> Subject: Re: [PATCH 2/2] pinctrl: pinctrl-imx: do not assume mux
> >> > >> 0 is gpio
> >> > >>
> >> > >> On 2017-05-14 23:48, Dong Aisheng wrote:
> >> > >> > Do not assume MUX 0 is GPIO function in core driver as a
> >> > >> > different SoC may have different value. e.g. MX7ULP Mux 1 is
> GPIO.
> >> > >> >
> >> > >> > Cc: Linus Walleij <linus.walleij@linaro.org>
> >> > >> > Cc: Alexandre Courbot <gnurou@gmail.com>
> >> > >> > Cc: Shawn Guo <shawnguo@kernel.org>
> >> > >> > Cc: Stefan Agner <stefan@agner.ch>
> >> > >> > Cc: Fugang Duan <fugang.duan@nxp.com>
> >> > >> > Cc: Bai Ping <ping.bai@nxp.com>
> >> > >> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> >> > >> > ---
> >> > >> >  drivers/pinctrl/freescale/pinctrl-imx.c | 3 ++-
> >> > >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > >> >
> >> > >> > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > index 0d6aaca..ed8ea32 100644
> >> > >> > --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> >> > >> > @@ -281,7 +281,7 @@ static int
> >> > >> > imx_pmx_gpio_request_enable(struct
> >> > >> > pinctrl_dev *pctldev,
> >> > >> >  			continue;
> >> > >> >  		for (pin = 0; pin < grp->num_pins; pin++) {
> >> > >> >  			imx_pin = &((struct imx_pin *)(grp->data))[pin];
> >> > >> > -			if (imx_pin->pin == offset && !imx_pin->mux_mode)
> >> > >> > +			if (imx_pin->pin == offset)
> >> > >> >  				goto mux_pin;
> >> > >>
> >> > >> The reason I added that check was to make sure we pick a mux
> >> > >> option which is GPIO... With this change, any pinmux might be
> picked...
> >> > >>
> >> > >
> >> > > First of all, this seems to be wrong to me that GPIO mux mode is
> >> > > SoC Dependant and should not be put in pinctrl-imx core driver.
> >> >
> >> > Hm, agree, we should consider to move
> >> > imx_pmx_gpio_request_enable/disable_free and
> >> > imx_pmx_gpio_set_direction into pinctrl-vf610.c
> >> >
> >>
> >> IMX7ULP may want to use imx_pmx_gpio_set_direction as well to support
> >> dynamically change GPIO from output to input.
> >>
> >> > >
> >> > > Secondly, I think we may be over worried and it's not quite
> >> > > necessary As we did not do the sanity check for both raw config
> >> > > and mux data read From Device tree, why only do it for GPIO?
> >> > >
> >> > > We may trust the data in device tree.
> >> >
> >> > In Vybrid, there is no need to explicitly assign a pinmux to use a
> >> > pin as GPIO. So the pinmux could be anything... The implemented
> >> > semantics for Vyrbid is really different than i.MX, see below.
> >> >
> >>
> >> Strange, I do see Vybrid assigning pinmux to GPIO in device tree.
> >> e.g.
> >> arch/arm/boot/dts/vf-colibri.dtsi
> >> pinctrl_esdhc1: esdhc1grp {
> >>         fsl,pins = <
> >>                 VF610_PAD_PTA24__ESDHC1_CLK     0x31ef
> >>                 VF610_PAD_PTA25__ESDHC1_CMD     0x31ef
> >>                 VF610_PAD_PTA26__ESDHC1_DAT0    0x31ef
> >>                 VF610_PAD_PTA27__ESDHC1_DAT1    0x31ef
> >>                 VF610_PAD_PTA28__ESDHC1_DATA2   0x31ef
> >>                 VF610_PAD_PTA29__ESDHC1_DAT3    0x31ef
> >>                 VF610_PAD_PTB20__GPIO_42        0x219d
> >>         >;
> >> };
> >>
> >> > >
> >> > >> >  		}
> >> > >> >  	}
> >> > >> > @@ -292,6 +292,7 @@ static int
> >> > >> > imx_pmx_gpio_request_enable(struct
> >> > >> > pinctrl_dev *pctldev,
> >> > >> >  	reg = readl(ipctl->base + pin_reg->mux_reg);
> >> > >> >  	reg &= ~info->mux_mask;
> >> > >> >  	reg |= imx_pin->config;
> >> > >> > +	reg |= imx_pin->mux_mode << info->mux_shift;
> >> > >>
> >> > >> ... and muxed...
> >> > >>
> >> > >> Not sure if we want that.
> >> > >>
> >> > >> I had to control GPIO output/input through pinctrl since Vybrid
> >> > >> does not allow to control that from the GPIO block.
> >> > >>
> >> > >> However, according to your GPIO patchset, the i.MX 7ULP has a
> >> > >> new register GPIO_PDDR to control output from the GPIO block. Is
> >> > >> controlling the output driver from IOMUXC still required?
> >> > >
> >> > > Yes, it's still required.
> >> > >
> >> >
> >> > That sounds weird, what is the GPIO_PDDR for then? Sure I  need to
> >> > enable the output driver to drive the pin, but can I disable output
> >> > just using GPIO_PDDR?
> >>
> >> No, to fully disable a output, you must disable OBE as well.
> >>
> >> >
> >> > Maybe we have a miss understanding here:
> >> >
> >> > Lets assume we want to switch a GPIO between output and input:
> >> >
> >> > echo "output" > /sys/class/gpio/gpioN/direction ..
> >> > echo "input" > /sys/class/gpio/gpioN/direction
> >> >
> >> > Do I need to disable the output driver in the IOMUXC or can we just
> >> > disable GPIO_PDDR and use the pin as input?
> >> >
> >>
> >> OBE should also be disabled. Otherwise the input may not function well.
> >>
> >> > If we can disable the output driver just using GPIO_PDDR, we can
> >> > avoid the gpio_set_direction cross call.
> >> >
> >> >
> >> > >> If not, I really would just not use all that "find pinctrl config"
> >> > >> hackery... e.g. add a new flag, USE_IOMUXC_FOR_GPIO_OUTPUT, and
> >> > >> set that only for Vybrid.
> >> > >>
> >> > >> This would also align much better with the other i.MX devices,
> >> > >> where pinmuxing and GPIO is completely orthogonal.
> >> > >>
> >> > >
> >> > > Actually this patch came only because to make the exist code not
> >> > > break MX7ULP.
> >> > >
> >> > > Actually I'm wondering why we need implement
> >> > > imx_pmx_gpio_request_enable function?
> >> > >
> >> > > Per my understanding, IMX binding already set GPIO mux by Parsing
> >> > > MUX mode from device tree, so why need gpio_request_enable which
> >> > > looks like is duplicated.
> >> > >
> >> > > Can you help explain it?
> >> >
> >> > I suggest to read this clarification email wrt to pinctrl/gpio from
> >> > Linus
> >> > Walleij:
> >> > https://lkml.org/lkml/2016/10/10/87
> >> >
> >> > To summarize: We have different semantics in Vybrid: The GPIO
> >> > subsystem automatically mux the GPIO for you. So in Vybrid, you do
> >> > not have to mux a GPIO (but a valid entry in your device tree is
> >> > needed, but does not assigned to any node).
> >>
> >> Okay, Clearer now.
> >>
> >> But I do see the users of GPIO pads in Vybrid dts.
> >> Above is an example which make me confuse at first.
> >>
> >> >
> >> > Is what the driver is doing for Vybrid wrong? It is different from
> >> > i.MX, but afaik, it is not really wrong... Its a valid
> >> > implementation according to the currently defined semantics...  Due
> >> > to the *need* to touch pinctrl for direction, I had to implement
> >> > cross calls anyway, so I thought I might as well go full mile and
> >> > also mux the GPIO on
> >> request...
> >> >
> >>
> >> It's not strickly wrong.
> >> Just a bit confuse that gpio_request_enable seems not quite necessary
> >> As we actually already and must define GPIO mux in device tree
> >> according To standard IMX binding format.
> >> e.g. VF610_PAD_PTB20__GPIO_42 in above sd pad group.
> >> That means pinctrl already does the GPIO mux when enable sd function.
> >>
> >> > So the question is, what semantic do we want for i.MX 7ULP? Since
> >> > it is a i.MX device, we probably want the same semantics as i.MX
> >> > 6/7 is already using for the sake of consistency. So in this case
> >> > the gpio_request_enable/disable callbacks are not needed...
> >> >
> >> > This is how I hope we can do the implementation for i.MX 7ULP:
> >> > - Do not use gpio_request_enable/disable
> >>
> >> Yes, we do want that.
> >>
> >> > - Do not use gpio_set_direction either
> >>
> >> Not, ULP needs it to support GPIO direction switch.
> >>
> >> > - Users using a GPIO should enable output driver in IOMUXC (just
> >> > use a pin configuration where output driver is enabled)
> >>
> >> Users still need configure OBE/IBE in devicetree for statically
> assignment.
> >>
> >> > - The GPIO driver only enables/disables the output driver using its
> >> > GPIO_PDDR register depending on GPIO direction
> >>
> >> No, same reason as the second question.
> >>
> >>
> >> So, finnaly, I think the solution may be:
> >> 1. If Vybrid does not use gpio_request_enable/disable, we can simply
> >> remove it. Both driver keeps using pinctrl gpio_set_direction.
> >>
> >> Or.
> >>
> >> 2. Make gpio_request_enable/disable and gpio_set_direction As
> >> pinctrl-imx core driver callbacks. And only assign gpio_set_direction
> >> For IMX7ULP platform driver while assign both for Vybrid.
> >>
> >> Which one would you prefer?
> 
> 1 would mean a semantic change.
> 
> For all GPIO I checked in upstream device trees we assign a pinctrl to the
> same node, so in all cases gpio_request_enable/disable is really
> unnecessary.
> 
> And since the current implementation has adversarial effects for I2C
> recovery (https://patchwork.kernel.org/patch/9351401/), we use the
> orthogonal semantic in the closely related i.MX SoCs and it even
> simplifies the driver, I am ok to change the semantic.
> 

I checked the I2C recovery issue you mentioned above, it looks
Changing the semantic as I proposed may fix it as well.

> Can you prepare such a patchset so that we can further test that on Vybrid?
> 

I will prepare it.

Thanks

Regards
Dong Aisheng