devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] i2c bus recovery for Microchip SoCs
@ 2020-01-03  9:49 Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties Codrin.Ciubotariu
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh, Codrin.Ciubotariu

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 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 SCL 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            |   9 ++
 drivers/i2c/busses/i2c-at91-master.c          | 101 ++++++++++++++++++
 drivers/i2c/busses/i2c-at91.h                 |  15 ++-
 8 files changed, 254 insertions(+), 13 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  2020-01-03 22:29   ` Rob Herring
  2020-01-03  9:49 ` [PATCH v2 2/6] i2c: at91: implement i2c bus recovery Codrin.Ciubotariu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

From: Kamel Bouhara <kamel.bouhara@bootlin.com>

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 <kamel.bouhara@bootlin.com>
---

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 2210f4359c45..551ddabb566b 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -20,8 +20,13 @@ Optional properties:
   capable I2C controllers.
 - i2c-sda-hold-time-ns: TWD hold time, only available for "atmel,sama5d4-i2c"
   and "atmel,sama5d2-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 {
@@ -56,6 +61,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/6] i2c: at91: implement i2c bus recovery
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  2020-01-09  7:48   ` Ludovic Desroches
  2020-01-03  9:49 ` [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down Codrin.Ciubotariu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh, Codrin.Ciubotariu

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" : "");
+
+	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);
 
+void at91_twi_prepare_recovery(struct i2c_adapter *adap);
+void at91_twi_unprepare_recovery(struct i2c_adapter *adap);
+void at91_twi_init_recovery_info(struct at91_twi_dev *dev);
+
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 void at91_init_twi_bus_slave(struct at91_twi_dev *dev);
 int at91_twi_probe_slave(struct platform_device *pdev, u32 phy_addr,
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 2/6] i2c: at91: implement i2c bus recovery Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  2020-01-09  7:47   ` Ludovic Desroches
  2020-01-09 11:05   ` Russell King - ARM Linux admin
  2020-01-03  9:49 ` [PATCH v2 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl Codrin.Ciubotariu
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh, Codrin.Ciubotariu

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
                   ` (2 preceding siblings ...)
  2020-01-03  9:49 ` [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 4/6] ARM: at91/dt: sama5d3: " Codrin.Ciubotariu
  2020-01-03  9:49 ` [PATCH v2 6/6] ARM: at91/dt: sama5d2: " Codrin.Ciubotariu
  5 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

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

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..34351baab985 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_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				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_PULL_UP
+							 AT91_PIOE 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				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_PULL_UP
+							 AT91_PIOB 30 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/6] ARM: at91/dt: sama5d3: add i2c gpio pinctrl
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
                   ` (3 preceding siblings ...)
  2020-01-03  9:49 ` [PATCH v2 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  2020-01-04 22:39   ` Peter Rosin
  2020-01-03  9:49 ` [PATCH v2 6/6] ARM: at91/dt: sama5d2: " Codrin.Ciubotariu
  5 siblings, 1 reply; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

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

diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index f770aace0efd..faf8907d8d7d 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_PULL_UP
+							 AT91_PIOA 31 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				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_PULL_UP
+							 AT91_PIOC 27 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				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_PULL_UP
+							 AT91_PIOA 19 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
+					};
 				};
 
 				isi {
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/6] ARM: at91/dt: sama5d2: add i2c gpio pinctrl
  2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
                   ` (4 preceding siblings ...)
  2020-01-03  9:49 ` [PATCH v2 4/6] ARM: at91/dt: sama5d3: " Codrin.Ciubotariu
@ 2020-01-03  9:49 ` Codrin.Ciubotariu
  5 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-03  9:49 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

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:
 - new changes;

 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..b3cd651ecda0 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-pull-up;
+				};
+
 				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-pull-up;
+				};
+
 				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-pull-up;
+				};
+
 				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..500e49eeb8cc 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-pull-up;
+				};
+
 				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-pull-up;
+				};
+
 				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-pull-up;
+				};
+
 				pinctrl_i2s0_default: i2s0_default {
 					pinmux = <PIN_PC1__I2SC0_CK>,
 						 <PIN_PC2__I2SC0_MCK>,
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery  properties
  2020-01-03  9:49 ` [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties Codrin.Ciubotariu
@ 2020-01-03 22:29   ` Rob Herring
  2020-01-06 16:34     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2020-01-03 22:29 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

On Fri, 3 Jan 2020 09:49:06 +0000, <Codrin.Ciubotariu@microchip.com> wrote:
> 
> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> 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 <kamel.bouhara@bootlin.com>
> ---
> 
> Changes in v2:
>  - none
> 
>  Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/6] ARM: at91/dt: sama5d3: add i2c gpio pinctrl
  2020-01-03  9:49 ` [PATCH v2 4/6] ARM: at91/dt: sama5d3: " Codrin.Ciubotariu
@ 2020-01-04 22:39   ` Peter Rosin
  2020-01-06 16:58     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2020-01-04 22:39 UTC (permalink / raw)
  To: Codrin.Ciubotariu, linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties
  2020-01-03 22:29   ` Rob Herring
@ 2020-01-06 16:34     ` Codrin.Ciubotariu
  0 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-06 16:34 UTC (permalink / raw)
  To: robh
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches

On 04.01.2020 00:29, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, 3 Jan 2020 09:49:06 +0000, <Codrin.Ciubotariu@microchip.com> wrote:
>>
>> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
>>
>> 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 <kamel.bouhara@bootlin.com>
>> ---
>>
>> Changes in v2:
>>   - none
>>
>>   Documentation/devicetree/bindings/i2c/i2c-at91.txt | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
> 
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
> 
> If a tag was not added on purpose, please state why and what changed.
> 

Sorry Rob, I forgot to add your Acked-by. There is nothing changed in 
this version. How should I proceed, should I resend it with your tag?

Best regards,
Codrin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/6] ARM: at91/dt: sama5d3: add i2c gpio pinctrl
  2020-01-04 22:39   ` Peter Rosin
@ 2020-01-06 16:58     ` Codrin.Ciubotariu
  0 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-06 16:58 UTC (permalink / raw)
  To: peda, linux-i2c, devicetree, linux-arm-kernel, linux-kernel
  Cc: kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni,
	Ludovic.Desroches, robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down
  2020-01-03  9:49 ` [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down Codrin.Ciubotariu
@ 2020-01-09  7:47   ` Ludovic Desroches
  2020-01-09 10:58     ` Codrin.Ciubotariu
  2020-01-09 11:05   ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 17+ messages in thread
From: Ludovic Desroches @ 2020-01-09  7:47 UTC (permalink / raw)
  To: Codrin Ciubotariu - M19940
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas Ferre - M43238, alexandre.belloni,
	robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] i2c: at91: implement i2c bus recovery
  2020-01-03  9:49 ` [PATCH v2 2/6] i2c: at91: implement i2c bus recovery Codrin.Ciubotariu
@ 2020-01-09  7:48   ` Ludovic Desroches
  2020-01-09 10:54     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 17+ messages in thread
From: Ludovic Desroches @ 2020-01-09  7:48 UTC (permalink / raw)
  To: Codrin Ciubotariu - M19940
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas Ferre - M43238, alexandre.belloni,
	robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/6] i2c: at91: implement i2c bus recovery
  2020-01-09  7:48   ` Ludovic Desroches
@ 2020-01-09 10:54     ` Codrin.Ciubotariu
  0 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-09 10:54 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni, robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down
  2020-01-09  7:47   ` Ludovic Desroches
@ 2020-01-09 10:58     ` Codrin.Ciubotariu
  0 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-09 10:58 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-arm-kernel, linux-kernel,
	kamel.bouhara, wsa, Nicolas.Ferre, alexandre.belloni, robh

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down
  2020-01-03  9:49 ` [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down Codrin.Ciubotariu
  2020-01-09  7:47   ` Ludovic Desroches
@ 2020-01-09 11:05   ` Russell King - ARM Linux admin
  2020-01-09 13:29     ` Codrin.Ciubotariu
  1 sibling, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux admin @ 2020-01-09 11:05 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel, robh,
	alexandre.belloni, kamel.bouhara, wsa, Ludovic.Desroches

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
> 

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down
  2020-01-09 11:05   ` Russell King - ARM Linux admin
@ 2020-01-09 13:29     ` Codrin.Ciubotariu
  0 siblings, 0 replies; 17+ messages in thread
From: Codrin.Ciubotariu @ 2020-01-09 13:29 UTC (permalink / raw)
  To: linux
  Cc: linux-i2c, devicetree, linux-arm-kernel, linux-kernel, robh,
	alexandre.belloni, kamel.bouhara, wsa, Ludovic.Desroches

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
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-01-09 13:29 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-03  9:49 [PATCH v2 0/6] i2c bus recovery for Microchip SoCs Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 1/6] dt-bindings: i2c: at91: document optional bus recovery properties Codrin.Ciubotariu
2020-01-03 22:29   ` Rob Herring
2020-01-06 16:34     ` Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 2/6] i2c: at91: implement i2c bus recovery Codrin.Ciubotariu
2020-01-09  7:48   ` Ludovic Desroches
2020-01-09 10:54     ` Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 3/6] i2c: at91: Send bus clear command if SCL is down Codrin.Ciubotariu
2020-01-09  7:47   ` Ludovic Desroches
2020-01-09 10:58     ` Codrin.Ciubotariu
2020-01-09 11:05   ` Russell King - ARM Linux admin
2020-01-09 13:29     ` Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 5/6] ARM: at91/dt: sama5d4: add i2c gpio pinctrl Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 4/6] ARM: at91/dt: sama5d3: " Codrin.Ciubotariu
2020-01-04 22:39   ` Peter Rosin
2020-01-06 16:58     ` Codrin.Ciubotariu
2020-01-03  9:49 ` [PATCH v2 6/6] ARM: at91/dt: sama5d2: " Codrin.Ciubotariu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).