[PATCH] i2c: mvtwsi: reset controller if stuck in "bus error" state

[PATCH] i2c: mvtwsi: reset controller if stuck in "bus error" state

[Sam Edwards]

The MVTWSI controller can act either as a master or slave device. When
acting as a master, the FSM is driven by the CPU. As a slave, the FSM is
driven by the bus directly. In what is (apparently) a safety mechanism,
if the bus transitions our FSM in any improper way, the FSM goes to a
"bus error" state (0x00). I could find no documented or experimental way
to get the FSM out of this state, except for a controller reset.

Since U-Boot only uses the MVTWSI controller as a bus master, this
feature only gets in the way: we do not care what happened on the bus
previously as long as the bus is ready for a new transaction. So, when
trying to start a new transaction, check for this state and reset the
controller if necessary.

Note that this should not be confused with the "deblocking" technique
(used by the `i2c reset` command), which involves pulsing SCL repeatedly
if SDA is found to be held low, in an attempt to force the bus back to
an idle state. This patch only resets the controller in case something
else had previously upset it, and (in principle) results in no
externally-observable change in behavior.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
index 93bbc6916e..14cdb0f663 100644
--- a/drivers/i2c/mvtwsi.c
+++ b/drivers/i2c/mvtwsi.c
@@ -142,6 +142,8 @@ enum mvtwsi_ctrl_register_fields {
  * code.
  */
 enum mvstwsi_status_values {
+	/* Protocol violation on bus; this is a terminal state */
+	MVTWSI_BUS_ERROR		= 0x00,
 	/* START condition transmitted */
 	MVTWSI_STATUS_START		= 0x08,
 	/* Repeated START condition transmitted */
@@ -525,6 +527,36 @@ static void __twsi_i2c_init(struct mvtwsi_registers *twsi, int speed,
 #endif
 }
 
+/*
+ * __twsi_i2c_reinit() - Reset and reinitialize the I2C controller.
+ *
+ * This function should be called to get the MVTWSI controller out of the
+ * "bus error" state. It saves and restores the baud and address registers.
+ *
+ * @twsi:	The MVTWSI register structure to use.
+ * @tick:	The duration of a clock cycle at the current I2C speed.
+ */
+static void __twsi_i2c_reinit(struct mvtwsi_registers *twsi, uint tick)
+{
+	uint baud;
+	uint slaveadd;
+
+	/* Save baud, address registers */
+	baud = readl(&twsi->baudrate);
+	slaveadd = readl(&twsi->slave_address);
+
+	/* Reset controller */
+	twsi_reset(twsi);
+
+	/* Restore baud, address registers */
+	writel(baud, &twsi->baudrate);
+	writel(slaveadd, &twsi->slave_address);
+	writel(0, &twsi->xtnd_slave_addr);
+
+	/* Assert STOP, but don't care for the result */
+	(void) twsi_stop(twsi, tick);
+}
+
 /*
  * i2c_begin() - Start a I2C transaction.
  *
@@ -621,6 +653,11 @@ static int __twsi_i2c_read(struct mvtwsi_registers *twsi, uchar chip,
 	int stop_status;
 	int expected_start = MVTWSI_STATUS_START;
 
+	/* Check for (and clear) a bus error from a previous failed transaction
+	 * or another master on the same bus */
+	if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
+		__twsi_i2c_reinit(twsi, tick);
+
 	if (alen > 0) {
 		/* Begin i2c write to send the address bytes */
 		status = i2c_begin(twsi, expected_start, (chip << 1), tick);
@@ -668,6 +705,11 @@ static int __twsi_i2c_write(struct mvtwsi_registers *twsi, uchar chip,
 {
 	int status, stop_status;
 
+	/* Check for (and clear) a bus error from a previous failed transaction
+	 * or another master on the same bus */
+	if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
+		__twsi_i2c_reinit(twsi, tick);
+
 	/* Begin i2c write to send first the address bytes, then the
 	 * data bytes */
 	status = i2c_begin(twsi, MVTWSI_STATUS_START, (chip << 1), tick);
-- 
2.41.0

Re: [PATCH] i2c: mvtwsi: reset controller if stuck in "bus error" state

[Stefan Roese]

On 7/26/23 00:13, Sam Edwards wrote:
> The MVTWSI controller can act either as a master or slave device. When
> acting as a master, the FSM is driven by the CPU. As a slave, the FSM is
> driven by the bus directly. In what is (apparently) a safety mechanism,
> if the bus transitions our FSM in any improper way, the FSM goes to a
> "bus error" state (0x00). I could find no documented or experimental way
> to get the FSM out of this state, except for a controller reset.
> 
> Since U-Boot only uses the MVTWSI controller as a bus master, this
> feature only gets in the way: we do not care what happened on the bus
> previously as long as the bus is ready for a new transaction. So, when
> trying to start a new transaction, check for this state and reset the
> controller if necessary.
> 
> Note that this should not be confused with the "deblocking" technique
> (used by the `i2c reset` command), which involves pulsing SCL repeatedly
> if SDA is found to be held low, in an attempt to force the bus back to
> an idle state. This patch only resets the controller in case something
> else had previously upset it, and (in principle) results in no
> externally-observable change in behavior.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> index 93bbc6916e..14cdb0f663 100644
> --- a/drivers/i2c/mvtwsi.c
> +++ b/drivers/i2c/mvtwsi.c
> @@ -142,6 +142,8 @@ enum mvtwsi_ctrl_register_fields {
>    * code.
>    */
>   enum mvstwsi_status_values {
> +	/* Protocol violation on bus; this is a terminal state */
> +	MVTWSI_BUS_ERROR		= 0x00,
>   	/* START condition transmitted */
>   	MVTWSI_STATUS_START		= 0x08,
>   	/* Repeated START condition transmitted */
> @@ -525,6 +527,36 @@ static void __twsi_i2c_init(struct mvtwsi_registers *twsi, int speed,
>   #endif
>   }
>   
> +/*
> + * __twsi_i2c_reinit() - Reset and reinitialize the I2C controller.
> + *
> + * This function should be called to get the MVTWSI controller out of the
> + * "bus error" state. It saves and restores the baud and address registers.
> + *
> + * @twsi:	The MVTWSI register structure to use.
> + * @tick:	The duration of a clock cycle at the current I2C speed.
> + */
> +static void __twsi_i2c_reinit(struct mvtwsi_registers *twsi, uint tick)
> +{
> +	uint baud;
> +	uint slaveadd;
> +
> +	/* Save baud, address registers */
> +	baud = readl(&twsi->baudrate);
> +	slaveadd = readl(&twsi->slave_address);
> +
> +	/* Reset controller */
> +	twsi_reset(twsi);
> +
> +	/* Restore baud, address registers */
> +	writel(baud, &twsi->baudrate);
> +	writel(slaveadd, &twsi->slave_address);
> +	writel(0, &twsi->xtnd_slave_addr);
> +
> +	/* Assert STOP, but don't care for the result */
> +	(void) twsi_stop(twsi, tick);
> +}
> +
>   /*
>    * i2c_begin() - Start a I2C transaction.
>    *
> @@ -621,6 +653,11 @@ static int __twsi_i2c_read(struct mvtwsi_registers *twsi, uchar chip,
>   	int stop_status;
>   	int expected_start = MVTWSI_STATUS_START;
>   
> +	/* Check for (and clear) a bus error from a previous failed transaction
> +	 * or another master on the same bus */
> +	if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
> +		__twsi_i2c_reinit(twsi, tick);
> +
>   	if (alen > 0) {
>   		/* Begin i2c write to send the address bytes */
>   		status = i2c_begin(twsi, expected_start, (chip << 1), tick);
> @@ -668,6 +705,11 @@ static int __twsi_i2c_write(struct mvtwsi_registers *twsi, uchar chip,
>   {
>   	int status, stop_status;
>   
> +	/* Check for (and clear) a bus error from a previous failed transaction
> +	 * or another master on the same bus */
> +	if (readl(&twsi->status) == MVTWSI_BUS_ERROR)
> +		__twsi_i2c_reinit(twsi, tick);
> +
>   	/* Begin i2c write to send first the address bytes, then the
>   	 * data bytes */
>   	status = i2c_begin(twsi, MVTWSI_STATUS_START, (chip << 1), tick);

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] i2c: mvtwsi: reset controller if stuck in "bus error" state

[Heiko Schocher]

Hello Sam,

On 26.07.23 00:13, Sam Edwards wrote:
> The MVTWSI controller can act either as a master or slave device. When
> acting as a master, the FSM is driven by the CPU. As a slave, the FSM is
> driven by the bus directly. In what is (apparently) a safety mechanism,
> if the bus transitions our FSM in any improper way, the FSM goes to a
> "bus error" state (0x00). I could find no documented or experimental way
> to get the FSM out of this state, except for a controller reset.
> 
> Since U-Boot only uses the MVTWSI controller as a bus master, this
> feature only gets in the way: we do not care what happened on the bus
> previously as long as the bus is ready for a new transaction. So, when
> trying to start a new transaction, check for this state and reset the
> controller if necessary.
> 
> Note that this should not be confused with the "deblocking" technique
> (used by the `i2c reset` command), which involves pulsing SCL repeatedly
> if SDA is found to be held low, in an attempt to force the bus back to
> an idle state. This patch only resets the controller in case something
> else had previously upset it, and (in principle) results in no
> externally-observable change in behavior.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de