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

[Heiko Schocher]

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