| Message ID | 20241018034919.974025-1-tommy_huang@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: aspeed: Consider i2c reset for muti-master case | expand |
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
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
Hi Tommy, On Tue, Oct 22, 2024 at 02:42:08AM +0000, Tommy Huang wrote: > 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. Please, avoid top posting, I don't understand which part of the original message you are trying to comment on. Second thing, please, before sending a patch, always always always make sure that checkpatch.pl reports '0' errors and '0' warnings, except for few sporadic cases. Andi
> -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, October 24, 2024 9:23 PM > To: Tommy Huang <tommy_huang@aspeedtech.com> > Cc: Andrew Jeffery <andrew@codeconstruct.com.au>; > brendanhiggins@google.com; benh@kernel.crashing.org; joel@jms.id.au; > 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 Tue, Oct 22, 2024 at 02:42:08AM +0000, Tommy Huang wrote: > > 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. > > Please, avoid top posting, I don't understand which part of the original > message you are trying to comment on. Got it. > Second thing, please, before sending a patch, always always always make sure > that checkpatch.pl reports '0' errors and '0' > warnings, except for few sporadic cases. Sure. Thanks for your suggestion. > > Andi
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); + } else aspeed_i2c_reset(bus); /*
In the original code, the device reset would not be triggered when the driver is set to multi-master and bus is free. It needs to be considered with multi-master condition. 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(-)