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@xxxxxxxxxxxxxxxxxxxx>
> Sent: Monday, October 21, 2024 7:49 PM
> To: Tommy Huang <tommy_huang@xxxxxxxxxxxxxx>;
> brendanhiggins@xxxxxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; joel@xxxxxxxxx;
> andi.shyti@xxxxxxxxxx
> Cc: BMC-SW <BMC-SW@xxxxxxxxxxxxxx>; linux-aspeed@xxxxxxxxxxxxxxxx;
> openbmc@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx>
> > ---
> > 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
