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
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 <[email protected]>
---
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
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 <[email protected]>
This patch may conflict with another one removing the explicit set to
false.
> ---
>
> Changes in v2:
> - use CLEAR command only if SDA is down; update patch subject to
> reflect this;
> - CLEAR command is no longer used for sama5d2, only sam9x60;
>
> drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++
> drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++-----
> drivers/i2c/busses/i2c-at91.h | 7 +++++-
> 3 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c
> index e13af4874976..128eafc05adb 100644
> --- a/drivers/i2c/busses/i2c-at91-core.c
> +++ b/drivers/i2c/busses/i2c-at91-core.c
> @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9261_config = {
> @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9260_config = {
> @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9g20_config = {
> @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9g10_config = {
> @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static const struct platform_device_id at91_twi_devtypes[] = {
> @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = {
> .has_dig_filtr = false,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata sama5d4_config = {
> @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = {
> .has_dig_filtr = true,
> .has_adv_dig_filtr = false,
> .has_ana_filtr = false,
> + .has_clear_cmd = false,
> };
>
> static struct at91_twi_pdata sama5d2_config = {
> @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = {
> .has_dig_filtr = true,
> .has_adv_dig_filtr = true,
> .has_ana_filtr = true,
> + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */
In this case it's probably worth keeping it.
Ludovic
> };
>
> static struct at91_twi_pdata sam9x60_config = {
> @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = {
> .has_dig_filtr = true,
> .has_adv_dig_filtr = true,
> .has_ana_filtr = true,
> + .has_clear_cmd = true,
> };
>
> static const struct of_device_id atmel_twi_dt_ids[] = {
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index c55e8ff35201..8c4f20317847 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> unsigned long time_left;
> bool has_unre_flag = dev->pdata->has_unre_flag;
> bool has_alt_cmd = dev->pdata->has_alt_cmd;
> - struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> + bool has_clear_cmd = dev->pdata->has_clear_cmd;
>
> /*
> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
> @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
> }
>
> - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) {
> - dev_dbg(dev->dev,
> - "SDA is down; clear bus using gpio\n");
> - i2c_recover_bus(&dev->adapter);
> + /*
> + * some faulty I2C slave devices might hold SDA down;
> + * we can send a bus clear command, hoping that the pins will be
> + * released
> + */
> + if (has_clear_cmd) {
> + if (!(dev->transfer_status & AT91_TWI_SDA)) {
> + dev_dbg(dev->dev,
> + "SDA is down; sending bus clear command\n");
> + if (dev->use_alt_cmd) {
> + unsigned int acr;
> +
> + acr = at91_twi_read(dev, AT91_TWI_ACR);
> + acr &= ~AT91_TWI_ACR_DATAL_MASK;
> + at91_twi_write(dev, AT91_TWI_ACR, acr);
> + }
> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);
> + }
> + } else {
> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> +
> + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) {
> + dev_dbg(dev->dev,
> + "SDA is down; clear bus using gpio\n");
> + i2c_recover_bus(&dev->adapter);
> + }
> }
>
> return ret;
> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
> index eb335b71e775..80f77d523346 100644
> --- a/drivers/i2c/busses/i2c-at91.h
> +++ b/drivers/i2c/busses/i2c-at91.h
> @@ -36,6 +36,7 @@
> #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
> #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
> #define AT91_TWI_SWRST BIT(7) /* Software Reset */
> +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */
> #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */
> #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */
> #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
> @@ -69,6 +70,8 @@
> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */
> #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
> +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */
> +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */
>
> #define AT91_TWI_INT_MASK \
> (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \
> @@ -81,7 +84,8 @@
> #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
>
> #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */
> -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
> +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0)
> +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK)
> #define AT91_TWI_ACR_DIR BIT(8)
>
> #define AT91_TWI_FILTR 0x0044
> @@ -118,6 +122,7 @@ struct at91_twi_pdata {
> bool has_dig_filtr;
> bool has_adv_dig_filtr;
> bool has_ana_filtr;
> + bool has_clear_cmd;
> struct at_dma_slave dma_slave;
> };
>
> --
> 2.20.1
On Fri, Jan 03, 2020 at 09:49:07AM +0000, [email protected] 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 <[email protected]>
> ---
>
> 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
> [email protected]
> 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
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 <[email protected]>
>
> This patch may conflict with another one removing the explicit set to
> false.
Yes, Eugen's patches got accepted in the meantime. I will rebase and
adapt to the new style.
>
>> ---
>>
>> Changes in v2:
>> - use CLEAR command only if SDA is down; update patch subject to
>> reflect this;
>> - CLEAR command is no longer used for sama5d2, only sam9x60;
>>
>> drivers/i2c/busses/i2c-at91-core.c | 9 ++++++++
>> drivers/i2c/busses/i2c-at91-master.c | 32 +++++++++++++++++++++++-----
>> drivers/i2c/busses/i2c-at91.h | 7 +++++-
>> 3 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c
>> index e13af4874976..128eafc05adb 100644
>> --- a/drivers/i2c/busses/i2c-at91-core.c
>> +++ b/drivers/i2c/busses/i2c-at91-core.c
>> @@ -71,6 +71,7 @@ static struct at91_twi_pdata at91rm9200_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata at91sam9261_config = {
>> @@ -82,6 +83,7 @@ static struct at91_twi_pdata at91sam9261_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata at91sam9260_config = {
>> @@ -93,6 +95,7 @@ static struct at91_twi_pdata at91sam9260_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata at91sam9g20_config = {
>> @@ -104,6 +107,7 @@ static struct at91_twi_pdata at91sam9g20_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata at91sam9g10_config = {
>> @@ -115,6 +119,7 @@ static struct at91_twi_pdata at91sam9g10_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static const struct platform_device_id at91_twi_devtypes[] = {
>> @@ -148,6 +153,7 @@ static struct at91_twi_pdata at91sam9x5_config = {
>> .has_dig_filtr = false,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata sama5d4_config = {
>> @@ -159,6 +165,7 @@ static struct at91_twi_pdata sama5d4_config = {
>> .has_dig_filtr = true,
>> .has_adv_dig_filtr = false,
>> .has_ana_filtr = false,
>> + .has_clear_cmd = false,
>> };
>>
>> static struct at91_twi_pdata sama5d2_config = {
>> @@ -170,6 +177,7 @@ static struct at91_twi_pdata sama5d2_config = {
>> .has_dig_filtr = true,
>> .has_adv_dig_filtr = true,
>> .has_ana_filtr = true,
>> + .has_clear_cmd = false, /* due to errata, CLEAR cmd is not working */
>
> In this case it's probably worth keeping it.
Yes, I also think it's useful to let this one.
Thanks and best regards,
Codrin
>
> Ludovic
>
>> };
>>
>> static struct at91_twi_pdata sam9x60_config = {
>> @@ -181,6 +189,7 @@ static struct at91_twi_pdata sam9x60_config = {
>> .has_dig_filtr = true,
>> .has_adv_dig_filtr = true,
>> .has_ana_filtr = true,
>> + .has_clear_cmd = true,
>> };
>>
>> static const struct of_device_id atmel_twi_dt_ids[] = {
>> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
>> index c55e8ff35201..8c4f20317847 100644
>> --- a/drivers/i2c/busses/i2c-at91-master.c
>> +++ b/drivers/i2c/busses/i2c-at91-master.c
>> @@ -480,7 +480,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
>> unsigned long time_left;
>> bool has_unre_flag = dev->pdata->has_unre_flag;
>> bool has_alt_cmd = dev->pdata->has_alt_cmd;
>> - struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>> + bool has_clear_cmd = dev->pdata->has_clear_cmd;
>>
>> /*
>> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
>> @@ -641,10 +641,32 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
>> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
>> }
>>
>> - if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) {
>> - dev_dbg(dev->dev,
>> - "SDA is down; clear bus using gpio\n");
>> - i2c_recover_bus(&dev->adapter);
>> + /*
>> + * some faulty I2C slave devices might hold SDA down;
>> + * we can send a bus clear command, hoping that the pins will be
>> + * released
>> + */
>> + if (has_clear_cmd) {
>> + if (!(dev->transfer_status & AT91_TWI_SDA)) {
>> + dev_dbg(dev->dev,
>> + "SDA is down; sending bus clear command\n");
>> + if (dev->use_alt_cmd) {
>> + unsigned int acr;
>> +
>> + acr = at91_twi_read(dev, AT91_TWI_ACR);
>> + acr &= ~AT91_TWI_ACR_DATAL_MASK;
>> + at91_twi_write(dev, AT91_TWI_ACR, acr);
>> + }
>> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);
>> + }
>> + } else {
>> + struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>> +
>> + if (rinfo->get_sda && !(rinfo->get_sda(&dev->adapter))) {
>> + dev_dbg(dev->dev,
>> + "SDA is down; clear bus using gpio\n");
>> + i2c_recover_bus(&dev->adapter);
>> + }
>> }
>>
>> return ret;
>> diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
>> index eb335b71e775..80f77d523346 100644
>> --- a/drivers/i2c/busses/i2c-at91.h
>> +++ b/drivers/i2c/busses/i2c-at91.h
>> @@ -36,6 +36,7 @@
>> #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
>> #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
>> #define AT91_TWI_SWRST BIT(7) /* Software Reset */
>> +#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */
>> #define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */
>> #define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */
>> #define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
>> @@ -69,6 +70,8 @@
>> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
>> #define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */
>> #define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
>> +#define AT91_TWI_SCL BIT(24) /* TWI SCL status */
>> +#define AT91_TWI_SDA BIT(25) /* TWI SDA status */
>>
>> #define AT91_TWI_INT_MASK \
>> (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \
>> @@ -81,7 +84,8 @@
>> #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
>>
>> #define AT91_TWI_ACR 0x0040 /* Alternative Command Register */
>> -#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
>> +#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0)
>> +#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK)
>> #define AT91_TWI_ACR_DIR BIT(8)
>>
>> #define AT91_TWI_FILTR 0x0044
>> @@ -118,6 +122,7 @@ struct at91_twi_pdata {
>> bool has_dig_filtr;
>> bool has_adv_dig_filtr;
>> bool has_ana_filtr;
>> + bool has_clear_cmd;
>> struct at_dma_slave dma_slave;
>> };
>>
>> --
>> 2.20.1
On 09.01.2020 13:05, Russell King - ARM Linux admin wrote:
> On Fri, Jan 03, 2020 at 09:49:07AM +0000, [email protected] 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 <[email protected]>
>> ---
>>
>> 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
>> [email protected]
>> 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
>