On Sun, May 21, 2023 at 02:19:40PM +0200, Marek Behún wrote: > There seems to be a bug within the mv64xxx I2C controller, wherein the > status register may not necessarily contain valid value immediately > after the IFLG flag is set in the control register. > > My theory is that the controller: > - first sets the IFLG in control register > - then updates the status register > - then raises an interrupt > > This may sometime cause weird bugs when in atomic mode, since in this > mode we do not wait for an interrupt, but instead we poll the control > register for IFLG and read status register immediately after. > > I encountered -ENXIO from mv64xxx_i2c_fsm() due to this issue when using > this driver in atomic mode. > > Note that I've only seen this issue on Armada 385, I don't know whether > other SOCs with this controller are also affected. Also note that this > fix has been in U-Boot for over 4 years [1] without anybody complaining, > so it should not cause regressions. I've never seen this bug before, but don't suspect it should cause any issues for me. Thank you for finding/fixing this. > > [1] https://source.denx.de/u-boot/u-boot/-/commit/d50e29662f78 > > Fixes: 544a8d75f3d6 ("i2c: mv64xxx: Add atomic_xfer method to driver") > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx> > --- > drivers/i2c/busses/i2c-mv64xxx.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index 047dfef7a657..878c076ebdc6 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -520,6 +520,17 @@ mv64xxx_i2c_intr(int irq, void *dev_id) > > while (readl(drv_data->reg_base + drv_data->reg_offsets.control) & > MV64XXX_I2C_REG_CONTROL_IFLG) { > + /* > + * It seems that sometime the controller updates the status > + * register only after it asserts IFLG in control register. > + * This may result in weird bugs when in atomic mode. A delay > + * of 100 ns before reading the status register solves this > + * issue. This bug does not seem to appear when using > + * interrupts. > + */ > + if (drv_data->atomic) > + ndelay(100); > + > status = readl(drv_data->reg_base + drv_data->reg_offsets.status); > mv64xxx_i2c_fsm(drv_data, status); > mv64xxx_i2c_do_action(drv_data); > -- > 2.39.3 >
