| Message ID | 20200115115422.17097-1-codrin.ciubotariu@microchip.com |
|---|---|
| Headers | show |
| Series | i2c bus recovery for Microchip SoCs | expand |
On Wed, Jan 15, 2020 at 01:54:18PM +0200, Codrin Ciubotariu 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> > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > > Changes in v3: > - removed unnecessary condition from info print; > - removed unneded declarations; > > 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 | 78 ++++++++++++++++++++++++++++ > drivers/i2c/busses/i2c-at91.h | 4 ++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c > index 7a862e00b475..0aba51a7df32 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,70 @@ 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, sda for recovery\n"); > + > + 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 +912,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..f57a6cab96b4 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; > -- > 2.20.1 >
On Wed, Jan 15, 2020 at 01:54:19PM +0200, Codrin Ciubotariu wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > After a transfer timeout, some faulty I2C slave devices might hold down > the SDA pin. We can generate a bus clear command, hoping that the slave > might release the pins. > If the CLEAR command is not supported, we will use gpio recovery, if > available, to reset the bus. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> I still have issue to apply it even if the context seems correct. Sometimes git am is picky... so Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > > Changes in v3: > - rebased on top of i2c/for-next; > - remove unnecessary initializations with false; > - replaced SCL with SDA in title and commit message; > - updated commit message; > > 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 | 2 ++ > drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++----- > drivers/i2c/busses/i2c-at91.h | 7 +++++- > 3 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c > index 3da1a8acecb5..e14edd236108 100644 > --- a/drivers/i2c/busses/i2c-at91-core.c > +++ b/drivers/i2c/busses/i2c-at91-core.c > @@ -131,6 +131,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 = { > @@ -142,6 +143,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 0aba51a7df32..bcc05a8fe826 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 f57a6cab96b4..7e7b4955ca7f 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 Wed, Jan 15, 2020 at 01:54:20PM +0200, Codrin Ciubotariu 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> > [codrin.ciubotariu@microchip.com: removed gpio pull-ups] > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > > Changes in v3: > - removed gpio pull-ups; > > Changes in v2: > - none; > > arch/arm/boot/dts/sama5d3.dtsi | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi > index f770aace0efd..1cea2137decf 100644 > --- a/arch/arm/boot/dts/sama5d3.dtsi > +++ b/arch/arm/boot/dts/sama5d3.dtsi > @@ -159,8 +159,11 @@ > dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(7)>, > <&dma0 2 AT91_DMA_CFG_PER_ID(8)>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c0>; > + pinctrl-1 = <&pinctrl_i2c0_gpio>; > + sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&twi0_clk>; > @@ -174,8 +177,11 @@ > dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(9)>, > <&dma0 2 AT91_DMA_CFG_PER_ID(10)>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c1>; > + pinctrl-1 = <&pinctrl_i2c1_gpio>; > + sda-gpios = <&pioC 26 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioC 27 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&twi1_clk>; > @@ -357,8 +363,11 @@ > dmas = <&dma1 2 AT91_DMA_CFG_PER_ID(11)>, > <&dma1 2 AT91_DMA_CFG_PER_ID(12)>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c2>; > + pinctrl-1 = <&pinctrl_i2c2_gpio>; > + sda-gpios = <&pioA 18 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA 19 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&twi2_clk>; > @@ -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_NONE > + AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > i2c1 { > @@ -647,6 +662,12 @@ > <AT91_PIOC 26 AT91_PERIPH_B AT91_PINCTRL_NONE /* PC26 periph B TWD1 pin, conflicts with SPI1_NPCS1, ISI_D11 */ > AT91_PIOC 27 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* PC27 periph B TWCK1 pin, conflicts with SPI1_NPCS2, ISI_D10 */ > }; > + > + pinctrl_i2c1_gpio: i2c1-gpio { > + atmel,pins = > + <AT91_PIOC 26 AT91_PERIPH_GPIO AT91_PINCTRL_NONE > + AT91_PIOC 27 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > i2c2 { > @@ -655,6 +676,12 @@ > <AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_NONE /* TWD2 pin, conflicts with LCDDAT18, ISI_D2 */ > AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_NONE>; /* TWCK2 pin, conflicts with LCDDAT19, ISI_D3 */ > }; > + > + pinctrl_i2c2_gpio: i2c2-gpio { > + atmel,pins = > + <AT91_PIOA 18 AT91_PERIPH_GPIO AT91_PINCTRL_NONE > + AT91_PIOA 19 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > isi { > -- > 2.20.1 >
On Wed, Jan 15, 2020 at 01:54:21PM +0200, Codrin Ciubotariu wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > [codrin.ciubotariu@microchip.com: removed gpio pull-ups] > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > > Changes in v3: > - removed gpio pull-ups; > > Changes in v2: > - none; > > arch/arm/boot/dts/sama5d4.dtsi | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi > index 6ab27a7b388d..26ce868096c2 100644 > --- a/arch/arm/boot/dts/sama5d4.dtsi > +++ b/arch/arm/boot/dts/sama5d4.dtsi > @@ -458,8 +458,11 @@ > (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) > | AT91_XDMAC_DT_PERID(3))>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c0>; > + pinctrl-1 = <&pinctrl_i2c0_gpio>; > + sda-gpios = <&pioA 30 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA 31 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&pmc PMC_TYPE_PERIPHERAL 32>; > @@ -477,8 +480,11 @@ > (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) > | AT91_XDMAC_DT_PERID(5))>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c1>; > + pinctrl-1 = <&pinctrl_i2c1_gpio>; > + sda-gpios = <&pioE 29 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioE 30 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&pmc PMC_TYPE_PERIPHERAL 33>; > @@ -519,8 +525,11 @@ > (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) > | AT91_XDMAC_DT_PERID(7))>; > dma-names = "tx", "rx"; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c2>; > + pinctrl-1 = <&pinctrl_i2c2_gpio>; > + sda-gpios = <&pioB 29 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioB 30 GPIO_ACTIVE_HIGH>; > #address-cells = <1>; > #size-cells = <0>; > clocks = <&pmc PMC_TYPE_PERIPHERAL 34>; > @@ -1122,6 +1131,12 @@ > <AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_NONE > AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_NONE>; > }; > + > + pinctrl_i2c0_gpio: i2c0-gpio { > + atmel,pins = > + <AT91_PIOA 30 AT91_PERIPH_GPIO AT91_PINCTRL_NONE > + AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > i2c1 { > @@ -1130,6 +1145,12 @@ > <AT91_PIOE 29 AT91_PERIPH_C AT91_PINCTRL_NONE /* TWD1, conflicts with UART0 RX and DIBP */ > AT91_PIOE 30 AT91_PERIPH_C AT91_PINCTRL_NONE>; /* TWCK1, conflicts with UART0 TX and DIBN */ > }; > + > + pinctrl_i2c1_gpio: i2c1-gpio { > + atmel,pins = > + <AT91_PIOE 29 AT91_PERIPH_GPIO AT91_PINCTRL_NONE > + AT91_PIOE 30 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > i2c2 { > @@ -1138,6 +1159,12 @@ > <AT91_PIOB 29 AT91_PERIPH_A AT91_PINCTRL_NONE /* TWD2, conflicts with RD0 and PWML1 */ > AT91_PIOB 30 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* TWCK2, conflicts with RF0 */ > }; > + > + pinctrl_i2c2_gpio: i2c2-gpio { > + atmel,pins = > + <AT91_PIOB 29 AT91_PERIPH_GPIO AT91_PINCTRL_NONE > + AT91_PIOB 30 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>; > + }; > }; > > isi { > -- > 2.20.1 >
On Wed, Jan 15, 2020 at 01:54:22PM +0200, Codrin Ciubotariu 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> > [codrin.ciubotariu@microchip.com: removed gpio pull-ups] > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > > Changes in v3: > - removed gpio pull-ups; > > Changes in v2: > - new patch; > > arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 33 +++++++++++++++++++-- > arch/arm/boot/dts/at91-sama5d2_xplained.dts | 33 +++++++++++++++++++-- > 2 files changed, 60 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts b/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts > index ba7f3e646c26..1c24ac8019ba 100644 > --- a/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts > +++ b/arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts > @@ -180,8 +180,11 @@ > > i2c0: i2c@f8028000 { > dmas = <0>, <0>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c0_default>; > + pinctrl-1 = <&pinctrl_i2c0_gpio>; > + sda-gpios = <&pioA PIN_PD21 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PD22 GPIO_ACTIVE_HIGH>; > status = "okay"; > }; > > @@ -198,8 +201,11 @@ > #address-cells = <1>; > #size-cells = <0>; > clocks = <&pmc PMC_TYPE_PERIPHERAL 19>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_flx0_default>; > + pinctrl-1 = <&pinctrl_flx0_gpio>; > + sda-gpios = <&pioA PIN_PB28 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PB29 GPIO_ACTIVE_HIGH>; > atmel,fifo-size = <16>; > status = "okay"; > }; > @@ -226,8 +232,11 @@ > > i2c1: i2c@fc028000 { > dmas = <0>, <0>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c1_default>; > + pinctrl-1 = <&pinctrl_i2c1_gpio>; > + sda-gpios = <&pioA PIN_PC6 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PC7 GPIO_ACTIVE_HIGH>; > status = "okay"; > > at24@50 { > @@ -244,18 +253,36 @@ > bias-disable; > }; > > + pinctrl_flx0_gpio: flx0_gpio { > + pinmux = <PIN_PB28__GPIO>, > + <PIN_PB29__GPIO>; > + bias-disable; > + }; > + > pinctrl_i2c0_default: i2c0_default { > pinmux = <PIN_PD21__TWD0>, > <PIN_PD22__TWCK0>; > bias-disable; > }; > > + pinctrl_i2c0_gpio: i2c0_gpio { > + pinmux = <PIN_PD21__GPIO>, > + <PIN_PD22__GPIO>; > + bias-disable; > + }; > + > pinctrl_i2c1_default: i2c1_default { > pinmux = <PIN_PC6__TWD1>, > <PIN_PC7__TWCK1>; > bias-disable; > }; > > + pinctrl_i2c1_gpio: i2c1_gpio { > + pinmux = <PIN_PC6__GPIO>, > + <PIN_PC7__GPIO>; > + bias-disable; > + }; > + > pinctrl_key_gpio_default: key_gpio_default { > pinmux = <PIN_PA10__GPIO>; > bias-pull-up; > diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts > index 9d0a7fbea725..055ee53e4773 100644 > --- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts > +++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts > @@ -129,8 +129,11 @@ > > i2c0: i2c@f8028000 { > dmas = <0>, <0>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c0_default>; > + pinctrl-1 = <&pinctrl_i2c0_gpio>; > + sda-gpios = <&pioA PIN_PD21 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PD22 GPIO_ACTIVE_HIGH>; > i2c-sda-hold-time-ns = <350>; > status = "okay"; > > @@ -331,8 +334,11 @@ > #address-cells = <1>; > #size-cells = <0>; > clocks = <&pmc PMC_TYPE_PERIPHERAL 23>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_flx4_default>; > + pinctrl-1 = <&pinctrl_flx4_gpio>; > + sda-gpios = <&pioA PIN_PD12 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PD13 GPIO_ACTIVE_HIGH>; > atmel,fifo-size = <16>; > i2c-analog-filter; > i2c-digital-filter; > @@ -343,11 +349,14 @@ > > i2c1: i2c@fc028000 { > dmas = <0>, <0>; > - pinctrl-names = "default"; > + pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c1_default>; > i2c-analog-filter; > i2c-digital-filter; > i2c-digital-filter-width-ns = <35>; > + pinctrl-1 = <&pinctrl_i2c1_gpio>; > + sda-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_HIGH>; > + scl-gpios = <&pioA PIN_PD5 GPIO_ACTIVE_HIGH>; > status = "okay"; > > at24@54 { > @@ -441,18 +450,36 @@ > bias-disable; > }; > > + pinctrl_flx4_gpio: flx4_gpio { > + pinmux = <PIN_PD12__GPIO>, > + <PIN_PD13__GPIO>; > + bias-disable; > + }; > + > pinctrl_i2c0_default: i2c0_default { > pinmux = <PIN_PD21__TWD0>, > <PIN_PD22__TWCK0>; > bias-disable; > }; > > + pinctrl_i2c0_gpio: i2c0_gpio { > + pinmux = <PIN_PD21__GPIO>, > + <PIN_PD22__GPIO>; > + bias-disable; > + }; > + > pinctrl_i2c1_default: i2c1_default { > pinmux = <PIN_PD4__TWD1>, > <PIN_PD5__TWCK1>; > bias-disable; > }; > > + pinctrl_i2c1_gpio: i2c1_gpio { > + pinmux = <PIN_PD4__GPIO>, > + <PIN_PD5__GPIO>; > + bias-disable; > + }; > + > pinctrl_i2s0_default: i2s0_default { > pinmux = <PIN_PC1__I2SC0_CK>, > <PIN_PC2__I2SC0_MCK>, > -- > 2.20.1 >
On Wed, Jan 15, 2020 at 01:54:18PM +0200, Codrin Ciubotariu 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> > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Applied to for-next, thanks! The implementation is very similar to i2c-imx. Maybe we should move this mechanism into the core somewhen...
On Wed, Jan 15, 2020 at 01:54:19PM +0200, Codrin Ciubotariu wrote: > After a transfer timeout, some faulty I2C slave devices might hold down > the SDA pin. We can generate a bus clear command, hoping that the slave > might release the pins. > If the CLEAR command is not supported, we will use gpio recovery, if > available, to reset the bus. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> One thing to improve: > + /* > + * 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); > + } The inner if-block should be a seperate function, then you could do in probe: if (has_clear_cmd) rinfo->recover_bus = <the above function>; else rinfo->recover_bus = i2c_generic_scl_recovery; Then, i2c_recover_bus() will always do the right thing. More readable and better maintainable IMO. If this is not possible (maybe I overlooked some logic), then maybe this will work: rinfo->recover_bus = <your custom function>; and put the if (has_clear_cmd) block there.