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: h...@denx.de
