[PATCH/RFT 0/6] i2c: recovery: fix GPIO usage for recovery

[PATCH/RFT 0/6] i2c: recovery: fix GPIO usage for recovery

[Wolfram Sang]

I have sent the last patch of this series before, but then I realized I need to
convert all users of GPIO recovery before. I needed to make sure they all set
the SDA GPIO to output, this is what patches 3-5 are doing. Which is also good
for them because then they can send STOP at apropriate places when doing
recovery.

Then, I noticed that two drivers were not using the open drain mode for SCL
which seems like a bug to me. So, patches 1+2 address that. I'd think those two
are stable material.

Due to no hardware, I could only build test these patches. I'd be really
looking forward to comments or tests of these patches.

A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/recovery-sda-output

Thanks,

   Wolfram


Wolfram Sang (6):
  i2c: designware: use open drain for recovery GPIO
  i2c: imx: use open drain for recovery GPIO
  i2c: designware: set SDA as output for recovery
  i2c: davinci: set SDA as output for recovery
  i2c: imx: set SDA as output for recovery
  i2c: recovery: remove bogus check if SDA GPIO is set to output

 drivers/i2c/busses/i2c-davinci.c           | 3 ++-
 drivers/i2c/busses/i2c-designware-master.c | 4 ++--
 drivers/i2c/busses/i2c-imx.c               | 4 ++--
 drivers/i2c/i2c-core-base.c                | 4 +---
 4 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Wolfram Sang]

I2C is open drain, so set up the GPIO accordingly.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index fc7c255c80af..a546db80f53e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -665,7 +665,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
 	struct gpio_desc *gpio;
 	int r;
 
-	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
+	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
 	if (IS_ERR(gpio)) {
 		r = PTR_ERR(gpio);
 		if (r == -ENOENT || r == -ENOSYS)
-- 
2.11.0

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 14/07/2018 05:09, Wolfram Sang wrote:
> I2C is open drain, so set up the GPIO accordingly.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   drivers/i2c/busses/i2c-designware-master.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index fc7c255c80af..a546db80f53e 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -665,7 +665,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
>   	struct gpio_desc *gpio;
>   	int r;
>   
> -	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
> +	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
>   	if (IS_ERR(gpio)) {
>   		r = PTR_ERR(gpio);
>   		if (r == -ENOENT || r == -ENOSYS)
> 
G'day Wolfram,

This was intentional. The gpio we use to drive the i2c line is implemented in an
FPGA and signals a buffer attached to the GPIO to drive scl OPEN drain. The GPIO is output
only.
The gpio setup can still specify the the GPIO be allocated OPEN drain if someone wishes
to use a "smarter" gpio.

So while the scl is open drain, there may be hardware in between that isn't.
What would the correct way be to deal with that now?


-- 
Regards
Phil

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

Hi Phil,

> > -	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
> > +	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> >   	if (IS_ERR(gpio)) {
> >   		r = PTR_ERR(gpio);
> >   		if (r == -ENOENT || r == -ENOSYS)
> > 
>
> This was intentional. The gpio we use to drive the i2c line is implemented in an
> FPGA and signals a buffer attached to the GPIO to drive scl OPEN drain. The GPIO is output
> only.

So, it is not possible to read SCL status then? Hmm, currently a working
get_scl is required...

> The gpio setup can still specify the the GPIO be allocated OPEN drain if someone wishes
> to use a "smarter" gpio.
> 
> So while the scl is open drain, there may be hardware in between that isn't.
> What would the correct way be to deal with that now?

Well, I don't know much about this IP core and how/where it is used. I
just wonder what happens if another user comes along using an
open-drain GPIO. Is that possible?

I assume it is the same with SDA? Non open-drain? Output only?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 17/07/2018 17:09, Wolfram Sang wrote:
> Hi Phil,
> 
>>> -	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH);
>>> +	gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
>>>    	if (IS_ERR(gpio)) {
>>>    		r = PTR_ERR(gpio);
>>>    		if (r == -ENOENT || r == -ENOSYS)
>>>
>>
>> This was intentional. The gpio we use to drive the i2c line is implemented in an
>> FPGA and signals a buffer attached to the GPIO to drive scl OPEN drain. The GPIO is output
>> only.
> 
> So, it is not possible to read SCL status then? Hmm, currently a working
> get_scl is required...
> 
>> The gpio setup can still specify the the GPIO be allocated OPEN drain if someone wishes
>> to use a "smarter" gpio.
>>
>> So while the scl is open drain, there may be hardware in between that isn't.
>> What would the correct way be to deal with that now?
> 
> Well, I don't know much about this IP core and how/where it is used. I
> just wonder what happens if another user comes along using an
> open-drain GPIO. Is that possible?
> 
> I assume it is the same with SDA? Non open-drain? Output only?
> 

Just had a closer look at how it's setup here.
Maybe the following helps.

The designware core is routed thru the fpga fabric.
Which provides and SCL SDA output enable pin.

Recovery gpio are provided by a FPGA gpio IO core.
This core has a fixed output and fixed input.

Here's the relevant bit on how it's all combined.
PWR_SDA_a / PWR_SCL_a are the signals to the outside world.
All the other signals are internal

   I2c0_Dat_s <= PWR_SDA_a;
   I2c0_Clk_s <= PWR_SCL_a;
   PWR_SDA_a <= '0' when  (I2c0_Dat_Oe_s = '1') else 'Z';

This bit of logic combines the i2c core and gpios.
   PWR_SCL_a <= '0' when ((I2c0_Clk_Oe_s = '1') or (PWR_SCL_rec_s = '0')) else 'Z';

     , pio_io_in_port(1)                   => PWR_SCL_a
     , pio_io_in_port(2)                   => PWR_SDA_a

     , pio_io_out_port(1)                  => PWR_SCL_rec_s

pio_io_out_port port is the fixed config for output
pio_io_in_port is the fixed config for input

The gpio input / outputs exist in the same ip core.

PWR_SCL_rec_s is the recovery clock gpio signal. It needs to be driven high / low.
There's no concept of HiZ internally in the FPGA.

If there was some kinda of OpenDrain gpio driver that modelled a FET driven by a push pull GPIO I guess
it could be made to work.

-- 
Regards
Phil Reid

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

Hi Phil,

> > So, it is not possible to read SCL status then? Hmm, currently a working
> > get_scl is required...
...
> > Well, I don't know much about this IP core and how/where it is used. I
> > just wonder what happens if another user comes along using an
> > open-drain GPIO. Is that possible?
> > 
> > I assume it is the same with SDA? Non open-drain? Output only?
> > 
> 
> Just had a closer look at how it's setup here.
> Maybe the following helps.

Thanks for the detailed explanation. I am just afraid it is a litle too
detailed for me. I am not sure if I can read it correctly:

When you read the SCL/SDA GPIO, does it return the true state of the
SCL/SDA line or does it just reflect the value it was set to output?

> There's no concept of HiZ internally in the FPGA.

Which probably means SDA is to be treated the same as SCL -> push/pull.

> If there was some kinda of OpenDrain gpio driver that modelled a FET
> driven by a push pull GPIO I guess it could be made to work.

Still, that sounds quite unlikely to me, so we can for now assume that
all designware users will have push/pull?

Disclaimer: I have zero experience with this core, I don't know how hard
it is to modify or which versions are out there.

Thanks for your help,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFT 1/6] i2c: designware: use open drain for recovery GPIO

[Phil Reid]

On 24/07/2018 15:26, Wolfram Sang wrote:
> Hi Phil,
> 
>>> So, it is not possible to read SCL status then? Hmm, currently a working
>>> get_scl is required...
> ...
>>> Well, I don't know much about this IP core and how/where it is used. I
>>> just wonder what happens if another user comes along using an
>>> open-drain GPIO. Is that possible?
>>>
>>> I assume it is the same with SDA? Non open-drain? Output only?
>>>
>>
>> Just had a closer look at how it's setup here.
>> Maybe the following helps.
> 
> Thanks for the detailed explanation. I am just afraid it is a litle too
> detailed for me. I am not sure if I can read it correctly:
> 
> When you read the SCL/SDA GPIO, does it return the true state of the
> SCL/SDA line or does it just reflect the value it was set to output?
Yes it returns the true state of the output pin.
I admit it's a bit odd from the classic GPIO point of view.

> 
>> There's no concept of HiZ internally in the FPGA.
> 
> Which probably means SDA is to be treated the same as SCL -> push/pull.
Yes. They're both driven push/pull, but the try state of the line is available.

> 
>> If there was some kinda of OpenDrain gpio driver that modelled a FET
>> driven by a push pull GPIO I guess it could be made to work.
> 
> Still, that sounds quite unlikely to me, so we can for now assume that
> all designware users will have push/pull?
I know of one other doing the same thing with the core in the Altera SocFPGA platform.
As they put me on to this solution for doing the recovery when the i2c was routed thru the SOC's fpga.

In other hard configurations they may have a 'proper' GPIO available that needs to be OpenDrain.



> 
> Disclaimer: I have zero experience with this core, I don't know how hard
> it is to modify or which versions are out there.
> 


-- 
Regards
Phil Reid

[PATCH/RFT 2/6] i2c: imx: use open drain for recovery GPIO

[Wolfram Sang]

I2C is open drain, so set up the GPIO accordingly.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0207e194f84b..3e23ee26c55c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1010,7 +1010,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
 			"gpio");
 	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
-	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH);
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
 
 	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
 	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
-- 
2.11.0

Re: [PATCH/RFT 2/6] i2c: imx: use open drain for recovery GPIO

[Lucas Stach]

Am Freitag, den 13.07.2018, 23:09 +0200 schrieb Wolfram Sang:
> I2C is open drain, so set up the GPIO accordingly.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I don't think this matters on any of the i.MX platforms, as the GPIO
pin configuration (including open-drain) is only taken from the DT
pinctrl, with the GPIO driver not being able to change any of those. On
 the flipside this results in a near zero probability of regressions.
;)

As this is obviously right even if it doesn't have any effect on
current software:

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/i2c/busses/i2c-imx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 0207e194f84b..3e23ee26c55c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1010,7 +1010,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
> >  	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> >  			"gpio");
> >  	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> > -	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH);
> > +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
>  
> >  	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> >  	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {

Re: [PATCH/RFT 2/6] i2c: imx: use open drain for recovery GPIO

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi Lucas,

> I don't think this matters on any of the i.MX platforms, as the GPIO
> pin configuration (including open-drain) is only taken from the DT
> pinctrl, with the GPIO driver not being able to change any of those. On
>  the flipside this results in a near zero probability of regressions.
> ;)

I see. Thanks for the heads up.

> As this is obviously right even if it doesn't have any effect on
> current software:
> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Thanks. Yeah, let's fix it. It might serve as an example for other
people.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH/RFT 2/6] i2c: imx: use open drain for recovery GPIO

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 288 bytes --]

On Fri, Jul 13, 2018 at 11:09:14PM +0200, Wolfram Sang wrote:
> I2C is open drain, so set up the GPIO accordingly.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Updated the commit message with the pinmux info from Lucas and applied
to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH/RFT 3/6] i2c: designware: set SDA as output for recovery

[Wolfram Sang]

This allows for sending STOP when generating pulses which is much more
robust because it will bring clients into a default state.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-designware-master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index a546db80f53e..25981efc844e 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -674,7 +674,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
 	}
 	rinfo->scl_gpiod = gpio;
 
-	gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_IN);
+	gpio = devm_gpiod_get_optional(dev->dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 	rinfo->sda_gpiod = gpio;
-- 
2.11.0

[PATCH/RFT 4/6] i2c: davinci: set SDA as output for recovery

[Wolfram Sang]

This allows for sending STOP when generating pulses which is much more
robust because it will bring clients into a default state.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-davinci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index d945a2654c2f..478acf0870aa 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -874,7 +874,8 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 			r = PTR_ERR(rinfo->scl_gpiod);
 			goto err_unuse_clocks;
 		}
-		rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+		rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda",
+						  GPIOD_OUT_HIGH_OPEN_DRAIN);
 		if (IS_ERR(rinfo->sda_gpiod)) {
 			r = PTR_ERR(rinfo->sda_gpiod);
 			goto err_unuse_clocks;
-- 
2.11.0

[PATCH/RFT 5/6] i2c: imx: set SDA as output for recovery

[Wolfram Sang]

This allows for sending STOP when generating pulses which is much more
robust because it will bring clients into a default state.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 3e23ee26c55c..a3881e270106 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1009,7 +1009,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 			PINCTRL_STATE_DEFAULT);
 	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
 			"gpio");
-	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
 	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
 
 	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
-- 
2.11.0

[PATCH/RFT 6/6] i2c: recovery: remove bogus check if SDA GPIO is set to output

[Wolfram Sang]

This check did not work as intended. I2C is open drain, so this function
will likely always have presented the GPIO as input because
gpiod_get_direction doesn't know about open drain states. Remove this
check for now. We can add it again once we know how to get more precise
information about the GPIO.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 301285c54603..7c5f012f561c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -261,9 +261,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 		bri->set_scl = set_scl_gpio_value;
 		if (bri->sda_gpiod) {
 			bri->get_sda = get_sda_gpio_value;
-			/* FIXME: add proper flag instead of '0' once available */
-			if (gpiod_get_direction(bri->sda_gpiod) == 0)
-				bri->set_sda = set_sda_gpio_value;
+			bri->set_sda = set_sda_gpio_value;
 		}
 		return;
 	}
-- 
2.11.0

Re: [PATCH/RFT 6/6] i2c: recovery: remove bogus check if SDA GPIO is set to output

[Ulrich Hecht]

On Fri, Jul 13, 2018 at 11:09 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> This check did not work as intended. I2C is open drain, so this function
> will likely always have presented the GPIO as input because
> gpiod_get_direction doesn't know about open drain states. Remove this
> check for now. We can add it again once we know how to get more precise
> information about the GPIO.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 301285c54603..7c5f012f561c 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -261,9 +261,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>                 bri->set_scl = set_scl_gpio_value;
>                 if (bri->sda_gpiod) {
>                         bri->get_sda = get_sda_gpio_value;
> -                       /* FIXME: add proper flag instead of '0' once available */
> -                       if (gpiod_get_direction(bri->sda_gpiod) == 0)
> -                               bri->set_sda = set_sda_gpio_value;
> +                       bri->set_sda = set_sda_gpio_value;
>                 }
>                 return;
>         }
> --
> 2.11.0
>

Reviewed-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

CU
Uli