[U-Boot] [PATCH u-boot-marvell v2.1 15/15] i2c: mvtwsi: fix reading status register after interrupt

[Stefan Roese]

On 01.05.19 23:09, Marek Behún wrote:
> The twsi_wait function reads the control register for interrupt flag,
> and if interrupt flag is present, it immediately reads status register.
> 
> On our device this sometimes causes bad value being read from status
> register, as if the value was not yet updated.
> 
> My theory is that the controller does approximately this:
>    1. sets interrupt flag in control register,
>    2. sets the value of status register,
>    3. causes an interrupt
> 
> In U-Boot we do not use interrupts, so I think that it is possible that
> sometimes the status register in the twsi_wait function is read between
> points 1 and 2.
> 
> The bug does not appear if I add a small delay before reading status
> register.
> 
> Wait 100ns (which in U-Boot currently means 1 us, because ndelay(i)
> function calls udelay(DIV_ROUND_UP(i, 1000))) before reading the status
> register.
> 
> Signed-off-by: Marek Behún <marek.behun at nic.cz>
> Acked-by: Heiko Schocher <hs at denx.de>
> Cc: Mario Six <mario.six at gdsys.cc>
> Cc: Stefan Roese <sr at denx.de>
> Cc: Baruch Siach <baruch at tkos.co.il>
> ---
>   drivers/i2c/mvtwsi.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> index 74ac0a4aa7..0a2dafcec6 100644
> --- a/drivers/i2c/mvtwsi.c
> +++ b/drivers/i2c/mvtwsi.c
> @@ -271,6 +271,17 @@ static int twsi_wait(struct mvtwsi_registers *twsi, int expected_status,
>   	do {
>   		control = readl(&twsi->control);
>   		if (control & MVTWSI_CONTROL_IFLG) {
> +			/*
> +			 * On Armada 38x it seems that the controller works as
> +			 * if it first set the MVTWSI_CONTROL_IFLAG in the
> +			 * control register and only after that it changed the
> +			 * status register.
> +			 * This sometimes caused weird bugs which only appeared
> +			 * on selected I2C speeds and even then only sometimes.
> +			 * We therefore add here a simple ndealy(100), which
> +			 * seems to fix this weird bug.
> +			 */
> +			ndelay(100);
>   			status = readl(&twsi->status);
>   			if (status == expected_status)
>   				return 0;
> 

This still looks a bit strange. Looking at the Linux driver, there
is no delay after reading the control register. But its using
interrupts and therefore an implicit delay is added before this
interrupt service routine is called (instead of the busy loop here
in the U-Boot driver).

So since I don't have a better idea:

Reviewed-by: Stefan Roese <sr at denx.de>

BTW: Whats the value of "tick" in twsi_wait() in your case?

Thanks,
Stefan