| Message ID | 20191002144658.7718-3-kamel.bouhara@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c bus recovery for Microchip SoCs. | expand |
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); > + } > +
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
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 >
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 > >
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.
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
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?
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
> 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.
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.
Hi Phil, yes, this thread is old but a similar issue came up again... On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid 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. I wonder what you ended up with? You are right, a single poll is not enough. It only might be if one applies the new "single-master" binding for a given bus. If that is not present, my best idea so far is to poll SDA for the time defined in adapter->timeout and if it is all low, then initiate a recovery. All the best, Wolfram
On 25/08/2020 21:28, Wolfram Sang wrote: > Hi Phil, > > yes, this thread is old but a similar issue came up again... > > On Fri, Oct 25, 2019 at 09:14:00AM +0800, Phil Reid 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. > > I wonder what you ended up with? You are right, a single poll is not > enough. It only might be if one applies the new "single-master" binding > for a given bus. If that is not present, my best idea so far is to poll > SDA for the time defined in adapter->timeout and if it is all low, then > initiate a recovery. > On my todo list still. Our system eventually recovers at the moment and the multi-master bus doesn't contain anything that's time critical to our systems operation.
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,
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(+)