[RFC PATCH] i2c: mvtwsi: reinitialize controller to clear bus errors

[Heiko Schocher]

Hello Sam,

On 10.06.23 08:15, Sam Edwards wrote:
> Hi I²C maintainers,
> 
> My target has the following devices sharing one bus:
> - 24C02 EEPROM
> - Realtek 8370 Ethernet switch
> - Allwinner T113-s3 (running U-Boot, interfacing via MVTWSI)
> 
> The RTL8370 is configured in "EEPROM autoload" mode, so on reset
> it will load the full contents of the EEPROM. During this sequence,
> it does an odd move where it sends a re-start, stop, pulses scl low,
> and then a fresh start.
> 
> Something about this sequence (I'm betting the scl pulse after stop)
> upsets the MVTWSI controller, causing it to retreat into state 0x00,
> which the documentation for my chip names "bus error." I'd guess this
> is a feature for slave operation: in slave mode, the controller FSM
> is completely at the mercy of the bus, and so a misbehaving/glitching
> bus can result in essentially a random walk through the states. Rather
> than allow that (and risk a potentially dangerous accidental write),
> the controller goes to a failsafe "bus error" state and remains there,
> effectively shutting down the whole controller.
> 
> However, in master mode (which U-Boot uses exclusively), this feature
> is a nuisance. We do not care what another master was doing on the bus
> previously, as long as it is in the proper idle state when we want to
> use it. We also don't care if the bus error was our fault in a previous
> transaction, since the error would have been reported at that time. I
> reckon that it makes sense to check for this "bus error" state at the
> beginning of each new read/write and clear it if detected.
> 
> Unfortunately, I couldn't find any way to coax the FSM out of the error
> state just by poking at the control register. It's possible I didn't
> look hard enough (I'm willing to try other things), but I'm otherwise
> left with only the obvious way out: a reset. Since that also resets the
> baud and address registers, I have to save and restore those too.
> 
> Attached here is my RFC patch (which DOES resolve my problem), for
> feedback and suggestions on what I might try differently, as I'm not
> sure whether or not I like this approach:
> - I would be happier if the code did a fresh init instead of saving
>   and restoring register state, but this driver is plumbed in a way
>   that the config struct isn't readily accessible at the low level.
> - I don't really like having to duplicate the state check in the read
>   and write functions; is there anything I can do that's more DRY?
> - Avoiding a reset would be nice, ideally avoiding the "bus error"
>   state altogether by disabling the error detection somehow.
> 
> Thoughts?

I have not the deep knowledge of this specific i2c driver, but may
also an option is to set

int (*deblock)(struct udevice *bus);

in

static const struct dm_i2c_ops mvtwsi_i2c_ops = {

for this driver and do there the stuff needed to deblock the bus,
as you have done in twsi_i2c_reinit() in your patch?

See as an example:

drivers/i2c/ast_i2c.c

Currently this deblock function is only used from i2c core in
i2c command, but may it makes sense to add in dm_i2c_xfer function in

drivers/i2c/i2c-uclass.c

a call to i2c_deblock() in case bus is blocked?

bye,
Heiko
> Cheers,
> Sam
> ---
>  drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> index d088ea75b9..38a3bdade0 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);
> 

-- 
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 at denx.de