| Message ID | 20240105125258.2470397-1-sai.pavan.boddu@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: cadence: Avoid fifo clear after start | expand |
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
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 >
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 >>
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
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
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(+)