[PATCH] i2c: cadence: Avoid fifo clear after start

[PATCH] i2c: cadence: Avoid fifo clear after start

[Sai Pavan Boddu]

Driver unintentionally programs ctrl reg to clear fifo which is
happening after start of transaction, this was not the case previously
as it was read-modified-write. This issue breaks i2c reads on QEMU as
i2c-read is done before guest starts programming ctrl register.

Fixes: ff0cf7bca6309 ("i2c: cadence: Remove unnecessary register reads")
Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
---
 drivers/i2c/busses/i2c-cadence.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index de3f58b60dce..6f7d753a8197 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 
 	if (hold_clear) {
 		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
+		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;
 		/*
 		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
 		 * register reaches '0'. This is an IP bug which causes transfer size
-- 
2.25.1

Re: [PATCH] i2c: cadence: Avoid fifo clear after start

[Michal Simek]

On 1/5/24 13:52, Sai Pavan Boddu wrote:
> Driver unintentionally programs ctrl reg to clear fifo which is
> happening after start of transaction, this was not the case previously
> as it was read-modified-write. This issue breaks i2c reads on QEMU as
> i2c-read is done before guest starts programming ctrl register.
> 
> Fixes: ff0cf7bca6309 ("i2c: cadence: Remove unnecessary register reads")
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
> ---
>   drivers/i2c/busses/i2c-cadence.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index de3f58b60dce..6f7d753a8197 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>   
>   	if (hold_clear) {
>   		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;
>   		/*
>   		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
>   		 * register reaches '0'. This is an IP bug which causes transfer size

Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH] i2c: cadence: Avoid fifo clear after start

[Andi Shyti]

Hi Sai,

sorry, but I'm not really understanding the issue here.
On Fri, Jan 05, 2024 at 06:22:58PM +0530, Sai Pavan Boddu wrote:
> Driver unintentionally programs ctrl reg to clear fifo which is
> happening after start of transaction

what does it mean "unintentionally"?

> this was not the case previously
> as it was read-modified-write. This issue breaks i2c reads on QEMU as
> i2c-read is done before guest starts programming ctrl register.

this log can be improved. How about something like

The driver unintentionally programs the control register to clear
the FIFO, which occurs after the start of the transaction.
Previously, this was not an issue as it involved
read-modify-write operations. However, this current issue
disrupts I2C reads on QEMU, as the I2C read is executed before
the guest starts programming the control register.

> Fixes: ff0cf7bca6309 ("i2c: cadence: Remove unnecessary register reads")
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
> ---
>  drivers/i2c/busses/i2c-cadence.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index de3f58b60dce..6f7d753a8197 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  
>  	if (hold_clear) {
>  		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;

I'm wondering whether the whole ctrl_reg should be reset after
the first write.

Andi

>  		/*
>  		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer size
>  		 * register reaches '0'. This is an IP bug which causes transfer size
> -- 
> 2.25.1
> 

RE: [PATCH] i2c: cadence: Avoid fifo clear after start

[Boddu, Sai Pavan]

Hi Andi,

>-----Original Message-----
>From: Andi Shyti <andi.shyti@kernel.org>
>Sent: Wednesday, January 17, 2024 6:50 PM
>To: Boddu, Sai Pavan <sai.pavan.boddu@amd.com>
>Cc: linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Lars-
>Peter Clausen <lars@metafoo.de>; Wolfram Sang <wsa@kernel.org>
>Subject: Re: [PATCH] i2c: cadence: Avoid fifo clear after start
>
>Hi Sai,
>
>sorry, but I'm not really understanding the issue here.
>On Fri, Jan 05, 2024 at 06:22:58PM +0530, Sai Pavan Boddu wrote:
>> Driver unintentionally programs ctrl reg to clear fifo which is
>> happening after start of transaction
>
>what does it mean "unintentionally"?
[Boddu, Sai Pavan] I mean, the previous patch which introduced the issue, was unintentional.
>
>> this was not the case previously
>> as it was read-modified-write. This issue breaks i2c reads on QEMU as
>> i2c-read is done before guest starts programming ctrl register.
>
>this log can be improved. How about something like
>
>The driver unintentionally programs the control register to clear the FIFO,
>which occurs after the start of the transaction.
>Previously, this was not an issue as it involved read-modify-write operations.
>However, this current issue disrupts I2C reads on QEMU, as the I2C read is
>executed before the guest starts programming the control register.
[Boddu, Sai Pavan] Looks good. I will mention as above.

>> Fixes: ff0cf7bca6309 ("i2c: cadence: Remove unnecessary register
>> reads")
>> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
>> ---
>>  drivers/i2c/busses/i2c-cadence.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c
>> b/drivers/i2c/busses/i2c-cadence.c
>> index de3f58b60dce..6f7d753a8197 100644
>> --- a/drivers/i2c/busses/i2c-cadence.c
>> +++ b/drivers/i2c/busses/i2c-cadence.c
>> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>
>>  	if (hold_clear) {
>>  		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
>> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;
>
>I'm wondering whether the whole ctrl_reg should be reset after the first write.
[Boddu, Sai Pavan] previous implementation of read-modify-write was good then ?

Regards,
Sai Pavan
>
>Andi
>
>>  		/*
>>  		 * In case of Xilinx Zynq SOC, clear the HOLD bit before transfer
>size
>>  		 * register reaches '0'. This is an IP bug which causes transfer
>> size
>> --
>> 2.25.1
>>

Re: [PATCH] i2c: cadence: Avoid fifo clear after start

[Andi Shyti]

Hi,

> >> b/drivers/i2c/busses/i2c-cadence.c
> >> index de3f58b60dce..6f7d753a8197 100644
> >> --- a/drivers/i2c/busses/i2c-cadence.c
> >> +++ b/drivers/i2c/busses/i2c-cadence.c
> >> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> >>
> >>  	if (hold_clear) {
> >>  		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
> >> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;
> >
> >I'm wondering whether the whole ctrl_reg should be reset after the first write.

> [Boddu, Sai Pavan] previous implementation of read-modify-write was good then ?

I don't know, I'm just asking... because rather than
read-modify-write, this is read-modify-write-modify-write :-)

I'm just wondering if after the first write ctrl_reg is still
holding a valid value.

Andi

RE: [PATCH] i2c: cadence: Avoid fifo clear after start

[Boddu, Sai Pavan]

Hi Andi,

Sorry, I did not close on this one.
Anyway I will re-spin fixing the commit message issues. More comments inline below.

>-----Original Message-----
>From: Andi Shyti <andi.shyti@kernel.org>
>Sent: Thursday, January 18, 2024 2:36 AM
>To: Boddu, Sai Pavan <sai.pavan.boddu@amd.com>
>Cc: linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Simek, Michal <michal.simek@amd.com>; Lars-
>Peter Clausen <lars@metafoo.de>; Wolfram Sang <wsa@kernel.org>
>Subject: Re: [PATCH] i2c: cadence: Avoid fifo clear after start
>
>Hi,
>
>> >> b/drivers/i2c/busses/i2c-cadence.c
>> >> index de3f58b60dce..6f7d753a8197 100644
>> >> --- a/drivers/i2c/busses/i2c-cadence.c
>> >> +++ b/drivers/i2c/busses/i2c-cadence.c
>> >> @@ -633,6 +633,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>> >>
>> >>  	if (hold_clear) {
>> >>  		ctrl_reg &= ~CDNS_I2C_CR_HOLD;
>> >> +		ctrl_reg &= ~CDNS_I2C_CR_CLR_FIFO;
>> >
>> >I'm wondering whether the whole ctrl_reg should be reset after the first
>write.
>
>> [Boddu, Sai Pavan] previous implementation of read-modify-write was good
>then ?
>
>I don't know, I'm just asking... because rather than read-modify-write, this is
>read-modify-write-modify-write :-)
>
>I'm just wondering if after the first write ctrl_reg is still holding a valid value.
[Boddu, Sai Pavan] Yes, all bits in ctrl_reg stay as configured except CLR_FIFO which is self-clearing.
None of the other bits would change state.

CLR_FIFO post start of transactions should not be allowed; this patch address the same.

Regards,
Sai Pavan 
>
>Andi

Re: [PATCH] i2c: cadence: Avoid fifo clear after start

[Andi Shyti]

Hi Sai Pavan,

> Sorry, I did not close on this one.
> Anyway I will re-spin fixing the commit message issues. More comments inline below.

Thanks! :-)

Andi