RE: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case

[Tommy Huang <tommy_huang@aspeedtech.com>]

Hi Andrew,

	Thanks for your comments.
	I want to fix the situation when our controller is set as target mode and reading / writing by other i2c host.
	However, this host is stopped by any other reason (DC on/off..etc).
	It will cause the controller is stuck in this situation.
	But I find it might not have clear hints to identify this situation is normal or abnormal.
	So, this patch should not be applied into mainstream.

	BR,

	By Tommy

> -----Original Message-----
> From: Andrew Jeffery <andrew@codeconstruct.com.au>
> Sent: Monday, October 21, 2024 7:49 PM
> To: Tommy Huang <tommy_huang@aspeedtech.com>;
> brendanhiggins@google.com; benh@kernel.crashing.org; joel@jms.id.au;
> andi.shyti@kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>; linux-aspeed@lists.ozlabs.org;
> openbmc@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; linux-i2c@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case
> 
> Hi Tommy,
> 
> On Fri, 2024-10-18 at 11:49 +0800, Tommy Huang wrote:
> > In the original code, the device reset would not be triggered when the
> > driver is set to multi-master and bus is free.
> 
> That's not how I read the existing code. As it stands, if it's multi- master and
> busy we do the recovery, however, if it's multi-master and free, or busy but not
> multi-master, or free and not multi-master, then we do the reset.
> 

> > It needs to be considered with multi-master condition.
> 
> Is there a specific circumstance you've found that's problematic? Can you
> provide some more details about that scenario?
> 
> >
> > Fixes: <f327c686d3ba> ("i2c: aspeed: Reset the i2c controller when
> > timeout occurs")
> >
> > Signed-off-by: Tommy Huang <tommy_huang@aspeedtech.com>
> > ---
> >  drivers/i2c/busses/i2c-aspeed.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-aspeed.c
> > b/drivers/i2c/busses/i2c-aspeed.c index cc5a26637fd5..7639ae3ace67
> > 100644
> > --- a/drivers/i2c/busses/i2c-aspeed.c
> > +++ b/drivers/i2c/busses/i2c-aspeed.c
> > @@ -716,14 +716,15 @@ static int aspeed_i2c_master_xfer(struct
> i2c_adapter *adap,
> >  	if (time_left == 0) {
> >  		/*
> >  		 * In a multi-master setup, if a timeout occurs, attempt
> > -		 * recovery. But if the bus is idle, we still need to reset the
> > -		 * i2c controller to clear the remaining interrupts.
> > +		 * recovery device. But if the bus is idle,
> > +		 * we still need to reset the i2c controller to clear
> > +		 * the remaining interrupts or reset device abnormal condition.
> >  		 */
> > -		if (bus->multi_master &&
> > -		    (readl(bus->base + ASPEED_I2C_CMD_REG) &
> > -		     ASPEED_I2CD_BUS_BUSY_STS))
> > -			aspeed_i2c_recover_bus(bus);
> > -		else
> > +		if ((readl(bus->base + ASPEED_I2C_CMD_REG) &
> > +			ASPEED_I2CD_BUS_BUSY_STS)){
> > +			if (bus->multi_master)
> > +				aspeed_i2c_recover_bus(bus);
> 
> The change doesn't seem match the commit message. In this case you've
> punched a hole - if the bus is busy but _not_ multi-master, we neither do the
> reset _nor_ the recovery.
> 
> Which is what you intended? The implementation? Or the prose description?
> 
> Now, back to the implementation, punching this hole seems reasonable on the
> surface, but I guess we need to keep in mind that time_left has also expired...
> 
> > +		} else
> >  			aspeed_i2c_reset(bus);
> >
> >  		/*
> 
> Andrew