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

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

[Tommy Huang]

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);
 
 		/*
-- 
2.25.1

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

[Andrew Jeffery]

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

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

[Tommy Huang]

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

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

[Andi Shyti]

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

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

[Tommy Huang]

> -----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