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

[Boddu, Sai Pavan]

Hi Andi,

>-----Original Message-----
>From: Andi Shyti <andi.shyti@xxxxxxxxxx>
>Sent: Wednesday, January 17, 2024 6:50 PM
>To: Boddu, Sai Pavan <sai.pavan.boddu@xxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; Lars-
>Peter Clausen <lars@xxxxxxxxxx>; Wolfram Sang <wsa@xxxxxxxxxx>
>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@xxxxxxx>
>> ---
>>  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
>>