2020-01-15 11:56:00

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 0/6] i2c bus recovery for Microchip SoCs

This patch series introduce the i2c bus recovery mechanism
for the Microchip SoCs. Some SoCs have hardware support for
recovery, while for those who don't the i2c-gpio bus recovery
mechanism is used. Updated the corresponding dts to add i2c
gpio pinctrl. The bus recovery is configured for the sama5d2/3/4
xplained and sama5d27 som1 EK boards in dts.

Changes in v3:
- addressed list comments:
- removed pull-ups from gpios;
- removed unused headers from i2c-at91.h;
- fixed commit message and subject on patch 3/6;
- added received tags;
- rebased on top of i2c/for-next;

Changes in v2:
- integrated the HW CLEAR command patch;
- call i2c_recover_bus() after an error occurs, if SDA is down;
- added i2c gpio pinctrl in sama5d2 xplained and sama5d27 som1 EK
boards;

Codrin Ciubotariu (1):
i2c: at91: Send bus clear command if SDA is down

Kamel Bouhara (5):
dt-bindings: i2c: at91: document optional bus recovery properties
i2c: at91: implement i2c bus recovery
ARM: at91/dt: sama5d3: add i2c gpio pinctrl
ARM: at91/dt: sama5d4: add i2c gpio pinctrl
ARM: at91/dt: sama5d2: add i2c gpio pinctrl

.../devicetree/bindings/i2c/i2c-at91.txt | 10 ++
arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 33 +++++-
arch/arm/boot/dts/at91-sama5d2_xplained.dts | 33 +++++-
arch/arm/boot/dts/sama5d3.dtsi | 33 +++++-
arch/arm/boot/dts/sama5d4.dtsi | 33 +++++-
drivers/i2c/busses/i2c-at91-core.c | 2 +
drivers/i2c/busses/i2c-at91-master.c | 100 ++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 11 +-
8 files changed, 242 insertions(+), 13 deletions(-)

--
2.20.1


2020-01-15 11:56:01

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 2/6] i2c: at91: implement i2c bus recovery

From: Kamel Bouhara <[email protected]>

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 <[email protected]>
Signed-off-by: Codrin Ciubotariu <[email protected]>
---

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

2020-01-15 11:56:08

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 1/6] dt-bindings: i2c: at91: document optional bus recovery properties

From: Kamel Bouhara <[email protected]>

The at91 I2C controller can support bus recovery by re-assigning SCL
and SDA to gpios. Add the optional pinctrl and gpio properties to do
so.

Signed-off-by: Kamel Bouhara <[email protected]>
[[email protected]: rebased]
Signed-off-by: Codrin Ciubotariu <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---

Changes in v3:
- rebased;
- added Reviewed-by tag from Rob;

Changes in v2:
- none;

Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index d4bad86107b8..96c914e048f5 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -28,8 +28,13 @@ Optional properties:
"atmel,sama5d4-i2c",
"atmel,sama5d2-i2c",
"microchip,sam9x60-i2c".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+ bus recovery, call it "gpio" state
- Child nodes conforming to i2c bus binding

+
Examples :

i2c0: i2c@fff84000 {
@@ -64,6 +69,11 @@ i2c0: i2c@f8034600 {
clocks = <&flx0>;
atmel,fifo-size = <16>;
i2c-sda-hold-time-ns = <336>;
+ 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>;

wm8731: wm8731@1a {
compatible = "wm8731";
--
2.20.1

2020-01-15 11:56:16

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 4/6] ARM: at91/dt: sama5d3: add i2c gpio pinctrl

From: Kamel Bouhara <[email protected]>

Add the i2c gpio pinctrls to support the i2c bus recovery

Signed-off-by: Kamel Bouhara <[email protected]>
[[email protected]: removed gpio pull-ups]
Signed-off-by: Codrin Ciubotariu <[email protected]>
---

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

2020-01-15 11:56:20

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl

From: Kamel Bouhara <[email protected]>

Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled

Signed-off-by: Kamel Bouhara <[email protected]>
[[email protected]: removed gpio pull-ups]
Signed-off-by: Codrin Ciubotariu <[email protected]>
---

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

2020-01-15 11:56:33

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 6/6] ARM: at91/dt: sama5d2: add i2c gpio pinctrl

From: Kamel Bouhara <[email protected]>

Add the i2c gpio pinctrls to support the i2c bus recovery

Signed-off-by: Kamel Bouhara <[email protected]>
[[email protected]: removed gpio pull-ups]
Signed-off-by: Codrin Ciubotariu <[email protected]>
---

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

2020-01-15 11:58:01

by Codrin Ciubotariu

[permalink] [raw]
Subject: [PATCH v3 3/6] i2c: at91: Send bus clear command if SDA is down

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 <[email protected]>
---

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

2020-01-27 08:55:23

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 2/6] i2c: at91: implement i2c bus recovery

On Wed, Jan 15, 2020 at 01:54:18PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> 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
>

2020-01-27 08:58:31

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl

On Wed, Jan 15, 2020 at 01:54:21PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> Add the i2c gpio pinctrls so the i2c bus recovery option can be enabled
>
> Signed-off-by: Kamel Bouhara <[email protected]>
> [[email protected]: removed gpio pull-ups]
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> 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
>

2020-01-27 08:58:46

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 4/6] ARM: at91/dt: sama5d3: add i2c gpio pinctrl

On Wed, Jan 15, 2020 at 01:54:20PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> Add the i2c gpio pinctrls to support the i2c bus recovery
>
> Signed-off-by: Kamel Bouhara <[email protected]>
> [[email protected]: removed gpio pull-ups]
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> 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
>

2020-01-27 08:59:59

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 6/6] ARM: at91/dt: sama5d2: add i2c gpio pinctrl

On Wed, Jan 15, 2020 at 01:54:22PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> Add the i2c gpio pinctrls to support the i2c bus recovery
>
> Signed-off-by: Kamel Bouhara <[email protected]>
> [[email protected]: removed gpio pull-ups]
> Signed-off-by: Codrin Ciubotariu <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> 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
>

2020-01-27 09:14:18

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] dt-bindings: i2c: at91: document optional bus recovery properties

On Wed, Jan 15, 2020 at 01:54:17PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> The at91 I2C controller can support bus recovery by re-assigning SCL
> and SDA to gpios. Add the optional pinctrl and gpio properties to do
> so.
>
> Signed-off-by: Kamel Bouhara <[email protected]>
> [[email protected]: rebased]
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> Changes in v3:
> - rebased;
> - added Reviewed-by tag from Rob;
>
> Changes in v2:
> - none;
>
> Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> index d4bad86107b8..96c914e048f5 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> @@ -28,8 +28,13 @@ Optional properties:
> "atmel,sama5d4-i2c",
> "atmel,sama5d2-i2c",
> "microchip,sam9x60-i2c".
> +- scl-gpios: specify the gpio related to SCL pin
> +- sda-gpios: specify the gpio related to SDA pin
> +- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
> + bus recovery, call it "gpio" state
> - Child nodes conforming to i2c bus binding
>
> +
> Examples :
>
> i2c0: i2c@fff84000 {
> @@ -64,6 +69,11 @@ i2c0: i2c@f8034600 {
> clocks = <&flx0>;
> atmel,fifo-size = <16>;
> i2c-sda-hold-time-ns = <336>;
> + 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>;
>
> wm8731: wm8731@1a {
> compatible = "wm8731";
> --
> 2.20.1
>

2020-01-27 09:19:17

by Ludovic Desroches

[permalink] [raw]
Subject: Re: [PATCH v3 3/6] i2c: at91: Send bus clear command if SDA is down

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 <[email protected]>

I still have issue to apply it even if the context seems correct.
Sometimes git am is picky... so

Acked-by: Ludovic Desroches <[email protected]>

> ---
>
> 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
>

2020-02-22 11:33:23

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v3 1/6] dt-bindings: i2c: at91: document optional bus recovery properties

On Wed, Jan 15, 2020 at 01:54:17PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> The at91 I2C controller can support bus recovery by re-assigning SCL
> and SDA to gpios. Add the optional pinctrl and gpio properties to do
> so.
>
> Signed-off-by: Kamel Bouhara <[email protected]>
> [[email protected]: rebased]
> Signed-off-by: Codrin Ciubotariu <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>

Applied to for-next, thanks!


Attachments:
(No filename) (534.00 B)
signature.asc (849.00 B)
Download all attachments

2020-02-22 11:35:11

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v3 2/6] i2c: at91: implement i2c bus recovery

On Wed, Jan 15, 2020 at 01:54:18PM +0200, Codrin Ciubotariu wrote:
> From: Kamel Bouhara <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Codrin Ciubotariu <[email protected]>

Applied to for-next, thanks!

The implementation is very similar to i2c-imx. Maybe we should move this
mechanism into the core somewhen...


Attachments:
(No filename) (580.00 B)
signature.asc (849.00 B)
Download all attachments

2020-02-22 11:44:57

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v3 3/6] i2c: at91: Send bus clear command if SDA is down

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 <[email protected]>

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.


Attachments:
(No filename) (1.51 kB)
signature.asc (849.00 B)
Download all attachments