| Message ID | 20200103094821.13185-1-codrin.ciubotariu@microchip.com |
|---|---|
| Headers | show |
| Series | i2c bus recovery for Microchip SoCs | expand |
On 2020-01-03 10:49, Codrin.Ciubotariu@microchip.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add the i2c gpio pinctrls to support the i2c bus recovery > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > --- > > Changes in v2: > - none; > > arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > *snip* > @@ -639,6 +648,12 @@ > <AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE /* PA30 periph A TWD0 pin, conflicts with URXD1, ISI_VSYNC */ > AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* PA31 periph A TWCK0 pin, conflicts with UTXD1, ISI_HSYNC */ > }; > + > + pinctrl_i2c0_gpio: i2c0-gpio { > + atmel,pins = > + <AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP > + AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>; > + }; I'm curious, but why are pull-ups suddenly needed just because the pins are used for GPIO recovery? Why are pull-ups not needed when the pins are used by the I2C peripheral device(s)? Given figure 27-2 "I/O Line Control Logic" in my SAMA5D3 datasheet, I see no difference as to how and why the pull-ups are applied depending on what the current function of the pin is. So, if the I2C bus works w/o pulls, bus recovery using GPIO must also work w/o pulls. I.e. the device tree requires you to have external pull-ups on the I2C bus anyway, so why bother with internal pull-ups for the recovery case? Changing pull-up settings just for recovery feels like something that will inevitably create hard to debug surprises at the least opportune time... Or am I missing something? (I'm focusing on SAMA5D3 since that is what I happen to work with, but the same question appears to apply for SAMA5D2 and SAMA5D4...) Cheers, Peter
On 05.01.2020 00:39, Peter Rosin wrote: > On 2020-01-03 10:49, Codrin.Ciubotariu@microchip.com wrote: >> From: Kamel Bouhara <kamel.bouhara@bootlin.com> >> >> Add the i2c gpio pinctrls to support the i2c bus recovery >> >> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> >> --- >> >> Changes in v2: >> - none; >> >> arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++--- >> 1 file changed, 30 insertions(+), 3 deletions(-) >> > > *snip* > >> @@ -639,6 +648,12 @@ >> <AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE /* PA30 periph A TWD0 pin, conflicts with URXD1, ISI_VSYNC */ >> AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* PA31 periph A TWCK0 pin, conflicts with UTXD1, ISI_HSYNC */ >> }; >> + >> + pinctrl_i2c0_gpio: i2c0-gpio { >> + atmel,pins = >> + <AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP >> + AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>; >> + }; > > I'm curious, but why are pull-ups suddenly needed just because the pins are > used for GPIO recovery? Why are pull-ups not needed when the pins are used > by the I2C peripheral device(s)? > > Given figure 27-2 "I/O Line Control Logic" in my SAMA5D3 datasheet, I see > no difference as to how and why the pull-ups are applied depending on what > the current function of the pin is. So, if the I2C bus works w/o pulls, bus > recovery using GPIO must also work w/o pulls. > > I.e. the device tree requires you to have external pull-ups on the I2C bus > anyway, so why bother with internal pull-ups for the recovery case? > > Changing pull-up settings just for recovery feels like something that will > inevitably create hard to debug surprises at the least opportune time... > > Or am I missing something? > > (I'm focusing on SAMA5D3 since that is what I happen to work with, > but the same question appears to apply for SAMA5D2 and SAMA5D4...) I don't think we need the pull-ups. I will remove them in v3. Thanks and best regards, Codrin
On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin Ciubotariu - M19940 wrote: > After a transfer timeout, some faulty I2C slave devices might hold down > the SCL pin. We can generate a bus clear command, hoping that the slave > might release the pins. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> This patch may conflict with another one removing the explicit set to false. > --- > > Changes in v2: > - use CLEAR command only if SDA is down; update patch subject to > reflect this; > - CLEAR command is no longer used for sama5d2, only sam9x60; > > drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++ > drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++----- > drivers/i2c/busses/i2c-at91.h | 7 +++++- > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c > index e13af4874976..128eafc05adb 100644 > --- a/drivers/i2c/busses/i2c-at91-core.c > +++ b/drivers/i2c/busses/i2c-at91-core.c > @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9261_config = { > @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9260_config = { > @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9g20_config = { > @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9g10_config = { > @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static const struct platform_device_id at91_twi_devtypes[] = { > @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata sama5d4_config = { > @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata sama5d2_config = { > @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = true, > .has_ana_filtr = true, > + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */ In this case it's probably worth keeping it. Ludovic > }; > > static struct at91_twi_pdata sam9x60_config = { > @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = true, > .has_ana_filtr = true, > + .has_clear_cmd = true, > }; > > static const struct of_device_id atmel_twi_dt_ids[] = { > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index c55e8ff35201..8c4f20317847 100644 > --- a/drivers/i2c/busses/i2c-at91-master.c > +++ b/drivers/i2c/busses/i2c-at91-master.c > @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > unsigned long time_left; > bool has_unre_flag = dev->pdata->has_unre_flag; > bool has_alt_cmd = dev->pdata->has_alt_cmd; > - struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > + bool has_clear_cmd = dev->pdata->has_clear_cmd; > > /* > * WARNING: the TXCOMP bit in the Status Register is NOT a clear on > @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); > } > > - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { > - dev_dbg(dev->dev, > - "SDA is down; clear bus using gpio\n"); > - i2c_recover_bus(&dev->adapter); > + /* > + * some faulty I2C slave devices might hold SDA down; > + * we can send a bus clear command, hoping that the pins will be > + * released > + */ > + if (has_clear_cmd) { > + if (!(dev->transfer_status & AT91_TWI_SDA)) { > + dev_dbg(dev->dev, > + "SDA is down; sending bus clear command\n"); > + if (dev->use_alt_cmd) { > + unsigned int acr; > + > + acr = at91_twi_read(dev, AT91_TWI_ACR); > + acr &= ~AT91_TWI_ACR_DATAL_MASK; > + at91_twi_write(dev, AT91_TWI_ACR, acr); > + } > + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR); > + } > + } else { > + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > + > + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { > + dev_dbg(dev->dev, > + "SDA is down; clear bus using gpio\n"); > + i2c_recover_bus(&dev->adapter); > + } > } > > return ret; > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h > index eb335b71e775..80f77d523346 100644 > --- a/drivers/i2c/busses/i2c-at91.h > +++ b/drivers/i2c/busses/i2c-at91.h > @@ -36,6 +36,7 @@ > #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */ > #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */ > #define AT91_TWI_SWRST BIT(7) /* Software Reset */ > +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */ > #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */ > #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */ > #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */ > @@ -69,6 +70,8 @@ > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */ > #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */ > #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */ > +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */ > +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */ > > #define AT91_TWI_INT_MASK \ > (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \ > @@ -81,7 +84,8 @@ > #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */ > > #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */ > -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff) > +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0) > +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK) > #define AT91_TWI_ACR_DIR BIT(8) > > #define AT91_TWI_FILTR 0x0044 > @@ -118,6 +122,7 @@ struct at91_twi_pdata { > bool has_dig_filtr; > bool has_adv_dig_filtr; > bool has_ana_filtr; > + bool has_clear_cmd; > struct at_dma_slave dma_slave; > }; > > -- > 2.20.1
On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin Ciubotariu - M19940 wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > 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> > [codrin.ciubotariu@microchip.com: release gpios on error; move i2c_recover_bus] > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > Changes in v2: > - called i2c_recover_bus() after an error occurs, if SDA is down; > - release gpios if recovery information is incomplete; > > drivers/i2c/busses/i2c-at91-master.c | 79 ++++++++++++++++++++++++++++ > drivers/i2c/busses/i2c-at91.h | 8 +++ > 2 files changed, 87 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index 7a862e00b475..c55e8ff35201 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> > @@ -478,6 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > unsigned long time_left; > bool has_unre_flag = dev->pdata->has_unre_flag; > bool has_alt_cmd = dev->pdata->has_alt_cmd; > + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > > /* > * WARNING: the TXCOMP bit in the Status Register is NOT a clear on > @@ -637,6 +640,13 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > at91_twi_write(dev, AT91_TWI_CR, > AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); > } > + > + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { > + dev_dbg(dev->dev, > + "SDA is down; clear bus using gpio\n"); > + i2c_recover_bus(&dev->adapter); > + } > + > return ret; > } > > @@ -806,6 +816,71 @@ 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"); > + if (!IS_ERR(rinfo->sda_gpiod)) { > + gpiod_put(rinfo->sda_gpiod); > + rinfo->sda_gpiod = NULL; > + } > + if (!IS_ERR(rinfo->scl_gpiod)) { > + gpiod_put(rinfo->scl_gpiod); > + rinfo->scl_gpiod = NULL; > + } > + return -EINVAL; > + } > + > + dev_info(&pdev->dev, "using scl%s for recovery\n", > + rinfo->sda_gpiod ? ",sda" : ""); Is it possible to have scl without sda? From the condition before, if there is an error for sda or scl, an error is returned. > + > + 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) > { > @@ -838,6 +913,10 @@ int at91_twi_probe_master(struct platform_device *pdev, > "i2c-analog-filter"); > 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 977a67bc0f88..eb335b71e775 100644 > --- a/drivers/i2c/busses/i2c-at91.h > +++ b/drivers/i2c/busses/i2c-at91.h > @@ -151,6 +151,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; > @@ -171,6 +175,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); > Wrong names here: > +void at91_twi_prepare_recovery(struct i2c_adapter *adap); at91_prepare_twi_recovery > +void at91_twi_unprepare_recovery(struct i2c_adapter *adap); at91_unprepare_twi_recovery > +void at91_twi_init_recovery_info(struct at91_twi_dev *dev); at91_init_twi_recovery_info Are these declarations still needed? Ludovic > + > #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.20.1
On 09.01.2020 09:48, Ludovic Desroches wrote: > On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin Ciubotariu - M19940 wrote: >> From: Kamel Bouhara <kamel.bouhara@bootlin.com> >> >> 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> >> [codrin.ciubotariu@microchip.com: release gpios on error; move i2c_recover_bus] >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> >> Changes in v2: >> - called i2c_recover_bus() after an error occurs, if SDA is down; >> - release gpios if recovery information is incomplete; >> >> drivers/i2c/busses/i2c-at91-master.c | 79 ++++++++++++++++++++++++++++ >> drivers/i2c/busses/i2c-at91.h | 8 +++ >> 2 files changed, 87 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c >> index 7a862e00b475..c55e8ff35201 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> >> @@ -478,6 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> unsigned long time_left; >> bool has_unre_flag = dev->pdata->has_unre_flag; >> bool has_alt_cmd = dev->pdata->has_alt_cmd; >> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; >> >> /* >> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on >> @@ -637,6 +640,13 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> at91_twi_write(dev, AT91_TWI_CR, >> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); >> } >> + >> + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { >> + dev_dbg(dev->dev, >> + "SDA is down; clear bus using gpio\n"); >> + i2c_recover_bus(&dev->adapter); >> + } >> + >> return ret; >> } >> >> @@ -806,6 +816,71 @@ 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"); >> + if (!IS_ERR(rinfo->sda_gpiod)) { >> + gpiod_put(rinfo->sda_gpiod); >> + rinfo->sda_gpiod = NULL; >> + } >> + if (!IS_ERR(rinfo->scl_gpiod)) { >> + gpiod_put(rinfo->scl_gpiod); >> + rinfo->scl_gpiod = NULL; >> + } >> + return -EINVAL; >> + } >> + >> + dev_info(&pdev->dev, "using scl%s for recovery\n", >> + rinfo->sda_gpiod ? ",sda" : ""); > > Is it possible to have scl without sda? From the condition before, if > there is an error for sda or scl, an error is returned. No, it's not. We need both sda and scl gpios. I will fix it. > >> + >> + 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) >> { >> @@ -838,6 +913,10 @@ int at91_twi_probe_master(struct platform_device *pdev, >> "i2c-analog-filter"); >> 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 977a67bc0f88..eb335b71e775 100644 >> --- a/drivers/i2c/busses/i2c-at91.h >> +++ b/drivers/i2c/busses/i2c-at91.h >> @@ -151,6 +151,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; >> @@ -171,6 +175,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); >> > > Wrong names here: > >> +void at91_twi_prepare_recovery(struct i2c_adapter *adap); > at91_prepare_twi_recovery > >> +void at91_twi_unprepare_recovery(struct i2c_adapter *adap); > at91_unprepare_twi_recovery > >> +void at91_twi_init_recovery_info(struct at91_twi_dev *dev); > at91_init_twi_recovery_info > > Are these declarations still needed? No, there is not. They are used only in the master mode of the driver. I will remove them. Thank you for your review Ludovic. Best regards, Codrin
On 09.01.2020 09:47, Ludovic Desroches wrote: > On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin Ciubotariu - M19940 wrote: >> After a transfer timeout, some faulty I2C slave devices might hold down >> the SCL pin. We can generate a bus clear command, hoping that the slave >> might release the pins. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > > This patch may conflict with another one removing the explicit set to > false. Yes, Eugen's patches got accepted in the meantime. I will rebase and adapt to the new style. > >> --- >> >> Changes in v2: >> - use CLEAR command only if SDA is down; update patch subject to >> reflect this; >> - CLEAR command is no longer used for sama5d2, only sam9x60; >> >> drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++ >> drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++----- >> drivers/i2c/busses/i2c-at91.h | 7 +++++- >> 3 files changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c >> index e13af4874976..128eafc05adb 100644 >> --- a/drivers/i2c/busses/i2c-at91-core.c >> +++ b/drivers/i2c/busses/i2c-at91-core.c >> @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9261_config = { >> @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9260_config = { >> @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9g20_config = { >> @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9g10_config = { >> @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static const struct platform_device_id at91_twi_devtypes[] = { >> @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata sama5d4_config = { >> @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata sama5d2_config = { >> @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = true, >> .has_ana_filtr = true, >> + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */ > > In this case it's probably worth keeping it. Yes, I also think it's useful to let this one. Thanks and best regards, Codrin > > Ludovic > >> }; >> >> static struct at91_twi_pdata sam9x60_config = { >> @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = true, >> .has_ana_filtr = true, >> + .has_clear_cmd = true, >> }; >> >> static const struct of_device_id atmel_twi_dt_ids[] = { >> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c >> index c55e8ff35201..8c4f20317847 100644 >> --- a/drivers/i2c/busses/i2c-at91-master.c >> +++ b/drivers/i2c/busses/i2c-at91-master.c >> @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> unsigned long time_left; >> bool has_unre_flag = dev->pdata->has_unre_flag; >> bool has_alt_cmd = dev->pdata->has_alt_cmd; >> - struct i2c_bus_recovery_info *rinfo = &dev->rinfo; >> + bool has_clear_cmd = dev->pdata->has_clear_cmd; >> >> /* >> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on >> @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); >> } >> >> - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { >> - dev_dbg(dev->dev, >> - "SDA is down; clear bus using gpio\n"); >> - i2c_recover_bus(&dev->adapter); >> + /* >> + * some faulty I2C slave devices might hold SDA down; >> + * we can send a bus clear command, hoping that the pins will be >> + * released >> + */ >> + if (has_clear_cmd) { >> + if (!(dev->transfer_status & AT91_TWI_SDA)) { >> + dev_dbg(dev->dev, >> + "SDA is down; sending bus clear command\n"); >> + if (dev->use_alt_cmd) { >> + unsigned int acr; >> + >> + acr = at91_twi_read(dev, AT91_TWI_ACR); >> + acr &= ~AT91_TWI_ACR_DATAL_MASK; >> + at91_twi_write(dev, AT91_TWI_ACR, acr); >> + } >> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR); >> + } >> + } else { >> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; >> + >> + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { >> + dev_dbg(dev->dev, >> + "SDA is down; clear bus using gpio\n"); >> + i2c_recover_bus(&dev->adapter); >> + } >> } >> >> return ret; >> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h >> index eb335b71e775..80f77d523346 100644 >> --- a/drivers/i2c/busses/i2c-at91.h >> +++ b/drivers/i2c/busses/i2c-at91.h >> @@ -36,6 +36,7 @@ >> #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */ >> #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */ >> #define AT91_TWI_SWRST BIT(7) /* Software Reset */ >> +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */ >> #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */ >> #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */ >> #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */ >> @@ -69,6 +70,8 @@ >> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */ >> #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */ >> #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */ >> +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */ >> +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */ >> >> #define AT91_TWI_INT_MASK \ >> (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \ >> @@ -81,7 +84,8 @@ >> #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */ >> >> #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */ >> -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff) >> +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0) >> +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK) >> #define AT91_TWI_ACR_DIR BIT(8) >> >> #define AT91_TWI_FILTR 0x0044 >> @@ -118,6 +122,7 @@ struct at91_twi_pdata { >> bool has_dig_filtr; >> bool has_adv_dig_filtr; >> bool has_ana_filtr; >> + bool has_clear_cmd; >> struct at_dma_slave dma_slave; >> }; >> >> -- >> 2.20.1
On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin.Ciubotariu@microchip.com wrote: > After a transfer timeout, some faulty I2C slave devices might hold down > the SCL pin. We can generate a bus clear command, hoping that the slave > might release the pins. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- > > Changes in v2: > - use CLEAR command only if SDA is down; update patch subject to > reflect this; Given this change, shouldn't the commit message above be updated? > - CLEAR command is no longer used for sama5d2, only sam9x60; > > drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++ > drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++----- > drivers/i2c/busses/i2c-at91.h | 7 +++++- > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c > index e13af4874976..128eafc05adb 100644 > --- a/drivers/i2c/busses/i2c-at91-core.c > +++ b/drivers/i2c/busses/i2c-at91-core.c > @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9261_config = { > @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9260_config = { > @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9g20_config = { > @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata at91sam9g10_config = { > @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static const struct platform_device_id at91_twi_devtypes[] = { > @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = { > .has_dig_filtr = false, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata sama5d4_config = { > @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = false, > .has_ana_filtr = false, > + .has_clear_cmd = false, > }; > > static struct at91_twi_pdata sama5d2_config = { > @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = true, > .has_ana_filtr = true, > + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */ > }; > > static struct at91_twi_pdata sam9x60_config = { > @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = { > .has_dig_filtr = true, > .has_adv_dig_filtr = true, > .has_ana_filtr = true, > + .has_clear_cmd = true, > }; > > static const struct of_device_id atmel_twi_dt_ids[] = { > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index c55e8ff35201..8c4f20317847 100644 > --- a/drivers/i2c/busses/i2c-at91-master.c > +++ b/drivers/i2c/busses/i2c-at91-master.c > @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > unsigned long time_left; > bool has_unre_flag = dev->pdata->has_unre_flag; > bool has_alt_cmd = dev->pdata->has_alt_cmd; > - struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > + bool has_clear_cmd = dev->pdata->has_clear_cmd; > > /* > * WARNING: the TXCOMP bit in the Status Register is NOT a clear on > @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); > } > > - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { > - dev_dbg(dev->dev, > - "SDA is down; clear bus using gpio\n"); > - i2c_recover_bus(&dev->adapter); > + /* > + * some faulty I2C slave devices might hold SDA down; > + * we can send a bus clear command, hoping that the pins will be > + * released > + */ > + if (has_clear_cmd) { > + if (!(dev->transfer_status & AT91_TWI_SDA)) { > + dev_dbg(dev->dev, > + "SDA is down; sending bus clear command\n"); > + if (dev->use_alt_cmd) { > + unsigned int acr; > + > + acr = at91_twi_read(dev, AT91_TWI_ACR); > + acr &= ~AT91_TWI_ACR_DATAL_MASK; > + at91_twi_write(dev, AT91_TWI_ACR, acr); > + } > + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR); > + } > + } else { > + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; > + > + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { > + dev_dbg(dev->dev, > + "SDA is down; clear bus using gpio\n"); > + i2c_recover_bus(&dev->adapter); > + } > } > > return ret; > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h > index eb335b71e775..80f77d523346 100644 > --- a/drivers/i2c/busses/i2c-at91.h > +++ b/drivers/i2c/busses/i2c-at91.h > @@ -36,6 +36,7 @@ > #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */ > #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */ > #define AT91_TWI_SWRST BIT(7) /* Software Reset */ > +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */ > #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */ > #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */ > #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */ > @@ -69,6 +70,8 @@ > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */ > #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */ > #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */ > +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */ > +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */ > > #define AT91_TWI_INT_MASK \ > (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \ > @@ -81,7 +84,8 @@ > #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */ > > #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */ > -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff) > +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0) > +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK) > #define AT91_TWI_ACR_DIR BIT(8) > > #define AT91_TWI_FILTR 0x0044 > @@ -118,6 +122,7 @@ struct at91_twi_pdata { > bool has_dig_filtr; > bool has_adv_dig_filtr; > bool has_ana_filtr; > + bool has_clear_cmd; > struct at_dma_slave dma_slave; > }; > > -- > 2.20.1 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 09.01.2020 13:05, Russell King - ARM Linux admin wrote: > On Fri, Jan 03, 2020 at 09:49:07AM +0000, Codrin.Ciubotariu@microchip.com wrote: >> After a transfer timeout, some faulty I2C slave devices might hold down >> the SCL pin. We can generate a bus clear command, hoping that the slave >> might release the pins. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> >> Changes in v2: >> - use CLEAR command only if SDA is down; update patch subject to >> reflect this; > > Given this change, shouldn't the commit message above be updated? It looks like both the subject and the commit message need to be fixed... Thank you for your review. Best regards, Codrin > >> - CLEAR command is no longer used for sama5d2, only sam9x60; >> >> drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++ >> drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++----- >> drivers/i2c/busses/i2c-at91.h | 7 +++++- >> 3 files changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c >> index e13af4874976..128eafc05adb 100644 >> --- a/drivers/i2c/busses/i2c-at91-core.c >> +++ b/drivers/i2c/busses/i2c-at91-core.c >> @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9261_config = { >> @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9260_config = { >> @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9g20_config = { >> @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata at91sam9g10_config = { >> @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static const struct platform_device_id at91_twi_devtypes[] = { >> @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = { >> .has_dig_filtr = false, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata sama5d4_config = { >> @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = false, >> .has_ana_filtr = false, >> + .has_clear_cmd = false, >> }; >> >> static struct at91_twi_pdata sama5d2_config = { >> @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = true, >> .has_ana_filtr = true, >> + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */ >> }; >> >> static struct at91_twi_pdata sam9x60_config = { >> @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = { >> .has_dig_filtr = true, >> .has_adv_dig_filtr = true, >> .has_ana_filtr = true, >> + .has_clear_cmd = true, >> }; >> >> static const struct of_device_id atmel_twi_dt_ids[] = { >> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c >> index c55e8ff35201..8c4f20317847 100644 >> --- a/drivers/i2c/busses/i2c-at91-master.c >> +++ b/drivers/i2c/busses/i2c-at91-master.c >> @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> unsigned long time_left; >> bool has_unre_flag = dev->pdata->has_unre_flag; >> bool has_alt_cmd = dev->pdata->has_alt_cmd; >> - struct i2c_bus_recovery_info *rinfo = &dev->rinfo; >> + bool has_clear_cmd = dev->pdata->has_clear_cmd; >> >> /* >> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on >> @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) >> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); >> } >> >> - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { >> - dev_dbg(dev->dev, >> - "SDA is down; clear bus using gpio\n"); >> - i2c_recover_bus(&dev->adapter); >> + /* >> + * some faulty I2C slave devices might hold SDA down; >> + * we can send a bus clear command, hoping that the pins will be >> + * released >> + */ >> + if (has_clear_cmd) { >> + if (!(dev->transfer_status & AT91_TWI_SDA)) { >> + dev_dbg(dev->dev, >> + "SDA is down; sending bus clear command\n"); >> + if (dev->use_alt_cmd) { >> + unsigned int acr; >> + >> + acr = at91_twi_read(dev, AT91_TWI_ACR); >> + acr &= ~AT91_TWI_ACR_DATAL_MASK; >> + at91_twi_write(dev, AT91_TWI_ACR, acr); >> + } >> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR); >> + } >> + } else { >> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo; >> + >> + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) { >> + dev_dbg(dev->dev, >> + "SDA is down; clear bus using gpio\n"); >> + i2c_recover_bus(&dev->adapter); >> + } >> } >> >> return ret; >> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h >> index eb335b71e775..80f77d523346 100644 >> --- a/drivers/i2c/busses/i2c-at91.h >> +++ b/drivers/i2c/busses/i2c-at91.h >> @@ -36,6 +36,7 @@ >> #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */ >> #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */ >> #define AT91_TWI_SWRST BIT(7) /* Software Reset */ >> +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */ >> #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */ >> #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */ >> #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */ >> @@ -69,6 +70,8 @@ >> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */ >> #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */ >> #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */ >> +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */ >> +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */ >> >> #define AT91_TWI_INT_MASK \ >> (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \ >> @@ -81,7 +84,8 @@ >> #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */ >> >> #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */ >> -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff) >> +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0) >> +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK) >> #define AT91_TWI_ACR_DIR BIT(8) >> >> #define AT91_TWI_FILTR 0x0044 >> @@ -118,6 +122,7 @@ struct at91_twi_pdata { >> bool has_dig_filtr; >> bool has_adv_dig_filtr; >> bool has_ana_filtr; >> + bool has_clear_cmd; >> struct at_dma_slave dma_slave; >> }; >> >> -- >> 2.20.1 >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up >