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

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?

Cheers,
Sam
---
 drivers/i2c/mvtwsi.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

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

Hey there Heiko,

On 6/12/23 06:35, Heiko Schocher wrote:
> 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?

I'm not sure that this is a good fit. The comment explaining deblock 
says this is for situations where "Resetting the I2C master does not 
help. The only way is to force the bus through a series of transitions 
to make sure that all slaves are done with the transaction."

Which reads to me like it's for situations where some slave is holding 
down SDA (making it impossible for us to send a start/stop) and several 
SCL pulses are required in order to shake it loose.

In this case, the *bus* is okay and the *master* is upset (ironically, I 
think, because the Realtek just did its own "deblock" equivalent), so a 
master reset is really all that's required here. We also know the 
specific state that it runs off to, so it's easy to detect this 
situation and resolve it.


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

I wouldn't object to such a change, but since deblock is an "active" 
operation that might have side effects, this probably needs a bigger 
discussion (beyond what I'm trying to do here) since other users might 
be unhappy about this happening without their explicit say-so.

Thanks for your feedback,
Sam

Hello Sam,

On 12.06.23 22:16, Sam Edwards wrote:
> Hey there Heiko,
> 
> On 6/12/23 06:35, Heiko Schocher wrote:
>> 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?
> 
> I'm not sure that this is a good fit. The comment explaining deblock says this is for situations
> where "Resetting the I2C master does not help. The only way is to force the bus through a series of
> transitions to make sure that all slaves are done with the transaction."
> 
> Which reads to me like it's for situations where some slave is holding down SDA (making it
> impossible for us to send a start/stop) and several SCL pulses are required in order to shake it loose.

Yes.

> In this case, the *bus* is okay and the *master* is upset (ironically, I think, because the Realtek
> just did its own "deblock" equivalent), so a master reset is really all that's required here. We
> also know the specific state that it runs off to, so it's easy to detect this situation and resolve it.

Hm, okay, than I misunderstood the subject line ;-)

Just looked into linux code, but have to less knowledge to give
you some good hints ... just looked in linux there is in

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_fsm()

        default:
                dev_err(&drv_data->adapter.dev,
                        "mv64xxx_i2c_fsm: Ctlr Error -- state: 0x%x, "
                        "status: 0x%x, addr: 0x%x, flags: 0x%x\n",
                         drv_data->state, status, drv_data->msg->addr,
                         drv_data->msg->flags);
                drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
                mv64xxx_i2c_hw_init(drv_data);
                i2c_recover_bus(&drv_data->adapter);
                drv_data->rc = -EAGAIN;
        }

(No idea how i2c_recover_bus() looks like for your case, as it comes from
core code...)

But it seems this is for status register reads, which have unknown values...
so his part should be called in case status == 0 too... linux driver does
also a hw_init in this case and calls deblock ...

So similiiar what you planned (beside no deblock) ...

May it makes sense to extend your approach (as it is so in linux too)
to do this for all "unknown status register values?

May its worth if you take some time, to look into linux driver?
(If you not already did!)

>> 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?
> 
> I wouldn't object to such a change, but since deblock is an "active" operation that might have side
> effects, this probably needs a bigger discussion (beyond what I'm trying to do here) since other
> users might be unhappy about this happening without their explicit say-so.

Yes, such a patch would have more impact, but may it makes sense,
as it would be good if U-Boot self heal a blocked i2c bus, but
yes, this needs extensive testing...

bye,
Heiko