[2/4] i2c: at91: implement i2c bus recovery
diff mbox series

Message ID 20191002144658.7718-3-kamel.bouhara@bootlin.com
State Superseded
Headers show
Series
  • i2c bus recovery for Microchip SoCs.
Related show

Commit Message

Kamel Bouhara Oct. 2, 2019, 2:46 p.m. UTC
Implement i2c bus recovery when slaves devices might hold SDA low.
In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
until the slave release SDA.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h        |  8 ++++
 2 files changed, 71 insertions(+)

Comments

Claudiu Beznea Oct. 4, 2019, 9:35 a.m. UTC | #1
Hi Kamel,

On 02.10.2019 17:46, Kamel Bouhara wrote:
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {

You may use IS_ERR_OR_NULL() here.

> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
Uwe Kleine-König Oct. 4, 2019, 8:39 p.m. UTC | #2
On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
> Hi Kamel,
> 
> On 02.10.2019 17:46, Kamel Bouhara wrote:
> > +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> > +				       struct at91_twi_dev *dev)
> > +{
> > +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> > +
> > +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> 
> You may use IS_ERR_OR_NULL() here.

Can devm_pinctrl_get return NULL? From a quick look, it cannot.

rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
value semantics.

Best regards
Uwe
Claudiu Beznea Oct. 7, 2019, 10:17 a.m. UTC | #3
On 04.10.2019 23:39, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> On Fri, Oct 04, 2019 at 09:35:23AM +0000, Claudiu.Beznea@microchip.com wrote:
>> Hi Kamel,
>>
>> On 02.10.2019 17:46, Kamel Bouhara wrote:
>>> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
>>> +				       struct at91_twi_dev *dev)
>>> +{
>>> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>> +
>>> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
>>> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
>>
>> You may use IS_ERR_OR_NULL() here.
> 
> Can devm_pinctrl_get return NULL? From a quick look, it cannot.

Looking quickly though it, yes, it seems it can't.

> 
> rule of thumb: IS_ERR_OR_NULL is wrong as it is a sign of poor return
> value semantics.
> 
> Best regards
> Uwe
>
Ludovic Desroches Oct. 9, 2019, 1:55 p.m. UTC | #4
On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 

Hi Kamel,

Thanks for adding this new feature. As I see patches only for sama5d3 and
sama5d4, I assume it has not been tested with a sama5d2, isn't it?

I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
work if we add .strict = true to pinmux_ops which is something plan for the
future...

Are you able to test these points? It would be nice to be aware of
possible side effects.

Regards

Ludovic

> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 63 ++++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-at91.h        |  8 ++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index a3fcc35ffd3b..df5bb93f952d 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -18,11 +18,13 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/err.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/dma-atmel.h>
>  #include <linux/pm_runtime.h>
> @@ -768,6 +770,63 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
>  	return ret;
>  }
>  
> +static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +}
> +
> +static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
> +{
> +	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
> +
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +}
> +
> +static int at91_init_twi_recovery_info(struct platform_device *pdev,
> +				       struct at91_twi_dev *dev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> +	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
> +		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(dev->pinctrl);
> +	}
> +
> +	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
> +							 PINCTRL_STATE_DEFAULT);
> +	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
> +						      "gpio");
> +	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> +	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> +					  GPIOD_OUT_HIGH_OPEN_DRAIN);
> +	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +
> +	if (IS_ERR(rinfo->sda_gpiod) ||
> +		   IS_ERR(rinfo->scl_gpiod) ||
> +		   IS_ERR(dev->pinctrl_pins_default) ||
> +		   IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "recovery information incomplete\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_info(&pdev->dev, "using scl%s for recovery\n",
> +		 rinfo->sda_gpiod ? ",sda" : "");
> +
> +	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> +	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	dev->adapter.bus_recovery_info = rinfo;
> +
> +	return 0;
> +}
> +
>  int at91_twi_probe_master(struct platform_device *pdev,
>  			  u32 phy_addr, struct at91_twi_dev *dev)
>  {
> @@ -795,6 +854,10 @@ int at91_twi_probe_master(struct platform_device *pdev,
>  
>  	at91_calc_twi_clock(dev);
>  
> +	rc = at91_init_twi_recovery_info(pdev, dev);
> +	if (rc == -EPROBE_DEFER)
> +		return rc;
> +
>  	dev->adapter.algo = &at91_twi_algorithm;
>  	dev->adapter.quirks = &at91_twi_quirks;
>  
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index 499b506f6128..b89dab55e776 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -141,6 +141,10 @@ struct at91_twi_dev {
>  	u32 fifo_size;
>  	struct at91_twi_dma dma;
>  	bool slave_detected;
> +	struct i2c_bus_recovery_info rinfo;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_default;
> +	struct pinctrl_state *pinctrl_pins_gpio;
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  	unsigned smr;
>  	struct i2c_client *slave;
> @@ -158,6 +162,10 @@ void at91_init_twi_bus_master(struct at91_twi_dev *dev);
>  int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
>  			  struct at91_twi_dev *dev);
>  
> +void at91_twi_prepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
> +
>  #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
>  void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
>  int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
> -- 
> 2.23.0
> 
>
Alexandre Belloni Oct. 9, 2019, 2:01 p.m. UTC | #5
On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > External E-Mail
> > 
> > 
> > Implement i2c bus recovery when slaves devices might hold SDA low.
> > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > until the slave release SDA.
> > 
> 
> Hi Kamel,
> 
> Thanks for adding this new feature. As I see patches only for sama5d3 and
> sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> 

I there a point having it on sama5d2 as the controller already supports
this feature?

> I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> work if we add .strict = true to pinmux_ops which is something plan for the
> future...
> 

I don't see why it wouldn't work with strict as this is switching muxing
properly instead of using the pins for two functions at the same time.
Ludovic Desroches Oct. 10, 2019, 6:54 a.m. UTC | #6
On Wed, Oct 09, 2019 at 04:01:47PM +0200, Alexandre Belloni wrote:
> 
> On 09/10/2019 15:55:00+0200, Ludovic Desroches wrote:
> > On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> > > External E-Mail
> > > 
> > > 
> > > Implement i2c bus recovery when slaves devices might hold SDA low.
> > > In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> > > until the slave release SDA.
> > > 
> > 
> > Hi Kamel,
> > 
> > Thanks for adding this new feature. As I see patches only for sama5d3 and
> > sama5d4, I assume it has not been tested with a sama5d2, isn't it?
> > 
> 
> I there a point having it on sama5d2 as the controller already supports
> this feature?
> 

Right, I was focused on pinctrl and forget we have this feature
supported by the IP.

> > I doubt it works with a sama5d2 because of the pinctrl. I also wonder if it can
> > work if we add .strict = true to pinmux_ops which is something plan for the
> > future...
> > 
> 
> I don't see why it wouldn't work with strict as this is switching muxing
> properly instead of using the pins for two functions at the same time.
> 

Not sure devm_gpiod_get won't fail with strict.

Ludovic
Wolfram Sang Oct. 21, 2019, 8:20 p.m. UTC | #7
On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
> Implement i2c bus recovery when slaves devices might hold SDA low.
> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
> until the slave release SDA.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>

Setting up the bus_recovery looks OK. However, I don't see any call to
i2c_recover_bus(), so the bus_recovery is never used. Did you test this
and see an effect?

Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
clear command if SCL or SDA is down" into this series. The crucial thing
for both is when to apply the recovery (at the beginning of a
transfer!). The rest is "just" that some HW needs a bus_recovery_info
for pinctrl/GPIO handling (from this patch), while other HW needs a
bus_recovery_info with a custom recover_bus callback.

Opinions?
Codrin Ciubotariu Oct. 24, 2019, 2:08 p.m. UTC | #8
On 22.10.2019 10:59, Kamel Bouhara wrote:
> On 21/10/2019 22:20, Wolfram Sang wrote:
>> On Wed, Oct 02, 2019 at 04:46:56PM +0200, Kamel Bouhara wrote:
>>> Implement i2c bus recovery when slaves devices might hold SDA low.
>>> In this case re-assign SCL/SDA to gpios and issue 9 dummy clock pulses
>>> until the slave release SDA.
>>>
>>> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
>>
>> Setting up the bus_recovery looks OK. However, I don't see any call to
>> i2c_recover_bus(), so the bus_recovery is never used. Did you test this
>> and see an effect?
>>
> Indeed, I guess I mess it up while doing some git stuff, it should be 
> called from at91_do_twi_transfer() when the transfer times out...
> I actually tested it and verified the recovery is triggered by pulling 
> the SCL to the ground ...
> 
>> Also, I think we should merge this patch "[PATCH v3] i2c: at91: Send bus
>> clear command if SCL or SDA is down" into this series. The crucial thing
>> for both is when to apply the recovery (at the beginning of a
>> transfer!). The rest is "just" that some HW needs a bus_recovery_info
>> for pinctrl/GPIO handling (from this patch), while other HW needs a
>> bus_recovery_info with a custom recover_bus callback.
>>
>> Opinions?
>>
> I'm OK to merge the two series.

So at the beginning of a new transfer, we should check if SDA (or SCL?) 
is low and, if it's true, only then we should try recover the bus.

Kamel, let me know if I can help with anything.

Best regards,
Codrin
Wolfram Sang Oct. 24, 2019, 3:07 p.m. UTC | #9
> So at the beginning of a new transfer, we should check if SDA (or SCL?) 
> is low and, if it's true, only then we should try recover the bus.

Yes, this is the proper time to do it. Remember, I2C does not define a
timeout.
Phil Reid Oct. 25, 2019, 1:14 a.m. UTC | #10
On 24/10/2019 23:07, Wolfram Sang wrote:
> 
>> So at the beginning of a new transfer, we should check if SDA (or SCL?)
>> is low and, if it's true, only then we should try recover the bus.
> 
> Yes, this is the proper time to do it. Remember, I2C does not define a
> timeout.
> 

FYI: Just a single poll at the start of the transfer, for it being low, will cause problems with multi-master buses.
Bus recovery should be attempted after a timeout when trying to communicate, even thou i2c doesn't define a timeout.

I'm trying to fix the designware drivers handling of this at the moment.

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index a3fcc35ffd3b..df5bb93f952d 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -18,11 +18,13 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/dma-atmel.h>
 #include <linux/pm_runtime.h>
@@ -768,6 +770,63 @@  static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
 	return ret;
 }
 
+static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+}
+
+static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
+{
+	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+}
+
+static int at91_init_twi_recovery_info(struct platform_device *pdev,
+				       struct at91_twi_dev *dev)
+{
+	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
+
+	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
+		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(dev->pinctrl);
+	}
+
+	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
+							 PINCTRL_STATE_DEFAULT);
+	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
+						      "gpio");
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
+					  GPIOD_OUT_HIGH_OPEN_DRAIN);
+	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (IS_ERR(rinfo->sda_gpiod) ||
+		   IS_ERR(rinfo->scl_gpiod) ||
+		   IS_ERR(dev->pinctrl_pins_default) ||
+		   IS_ERR(dev->pinctrl_pins_gpio)) {
+		dev_info(&pdev->dev, "recovery information incomplete\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "using scl%s for recovery\n",
+		 rinfo->sda_gpiod ? ",sda" : "");
+
+	rinfo->prepare_recovery = at91_prepare_twi_recovery;
+	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	dev->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 int at91_twi_probe_master(struct platform_device *pdev,
 			  u32 phy_addr, struct at91_twi_dev *dev)
 {
@@ -795,6 +854,10 @@  int at91_twi_probe_master(struct platform_device *pdev,
 
 	at91_calc_twi_clock(dev);
 
+	rc = at91_init_twi_recovery_info(pdev, dev);
+	if (rc == -EPROBE_DEFER)
+		return rc;
+
 	dev->adapter.algo = &at91_twi_algorithm;
 	dev->adapter.quirks = &at91_twi_quirks;
 
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 499b506f6128..b89dab55e776 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -141,6 +141,10 @@  struct at91_twi_dev {
 	u32 fifo_size;
 	struct at91_twi_dma dma;
 	bool slave_detected;
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 	unsigned smr;
 	struct i2c_client *slave;
@@ -158,6 +162,10 @@  void at91_init_twi_bus_master(struct at91_twi_dev *dev);
 int at91_twi_probe_master(struct platform_device *pdev, u32 phy_addr,
 			  struct at91_twi_dev *dev);
 
+void at91_twi_prepare_recovery(struct i2c_adapter *adap);
+void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
+void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
+
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
 int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,