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(-)
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);
/*
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH] i2c: aspeed: Consider i2c reset for muti-master case Link: https://lore.kernel.org/stable/20241018034919.974025-1-tommy_huang%40aspeedt...
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
linux-stable-mirror@lists.linaro.org
-
Andi Shyti -
Andrew Jeffery -
kernel test robot -
Tommy Huang