| Message ID | 20230107211814.1179438-6-lars@metafoo.de |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: cadence: Small cleanups | expand |
On 1/7/23 22:18, Lars-Peter Clausen wrote: > > In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C > controller is written and read back multiple times. The register value does > not change on its own. So it is possible to remember the just written value > instead of reading it back from the hardware. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/i2c/busses/i2c-cadence.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > index bec50bfe7aad..93c6d0822468 100644 > --- a/drivers/i2c/busses/i2c-cadence.c > +++ b/drivers/i2c/busses/i2c-cadence.c > @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) > > /* Determine hold_clear based on number of bytes to receive and hold flag */ > if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) { > - if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) { > + if (ctrl_reg & CDNS_I2C_CR_HOLD) { > hold_clear = true; > if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT) > irq_save = true; > @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) > addr &= CDNS_I2C_ADDR_MASK; > > if (hold_clear) { > - ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & ~CDNS_I2C_CR_HOLD; > + ctrl_reg &= ~CDNS_I2C_CR_HOLD; > /* > * 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.30.2 > Logically this is fine but that additional read on CR register ensures that IP receive previous writes. The code itself is related to bug on Zynq SoC and that two additional readbacks can actually do something. I think this should be properly tested on zynq to ensure that it doesn't break anything. Shubhrajyoti: Can you please make sure that it is tested on Zynq? Thanks, Michal
On 1/16/23 06:58, Michal Simek wrote: > > > On 1/7/23 22:18, Lars-Peter Clausen wrote: >> >> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C >> controller is written and read back multiple times. The register >> value does >> not change on its own. So it is possible to remember the just written >> value >> instead of reading it back from the hardware. >> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >> --- >> drivers/i2c/busses/i2c-cadence.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-cadence.c >> b/drivers/i2c/busses/i2c-cadence.c >> index bec50bfe7aad..93c6d0822468 100644 >> --- a/drivers/i2c/busses/i2c-cadence.c >> +++ b/drivers/i2c/busses/i2c-cadence.c >> @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) >> >> /* Determine hold_clear based on number of bytes to receive >> and hold flag */ >> if (!id->bus_hold_flag && id->recv_count <= >> CDNS_I2C_FIFO_DEPTH) { >> - if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & >> CDNS_I2C_CR_HOLD) { >> + if (ctrl_reg & CDNS_I2C_CR_HOLD) { >> hold_clear = true; >> if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT) >> irq_save = true; >> @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) >> addr &= CDNS_I2C_ADDR_MASK; >> >> if (hold_clear) { >> - ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & >> ~CDNS_I2C_CR_HOLD; >> + ctrl_reg &= ~CDNS_I2C_CR_HOLD; >> /* >> * 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.30.2 >> > > Logically this is fine but that additional read on CR register ensures > that IP receive previous writes. The code itself is related to bug on > Zynq SoC and that two additional readbacks can actually do something. > > I think this should be properly tested on zynq to ensure that it > doesn't break anything. > > Shubhrajyoti: Can you please make sure that it is tested on Zynq? Maybe it is better to drop the patch then if it is used to enforce ordering in the hardware. But I guess we should add a comment to explain this.
On 1/16/23 18:14, Lars-Peter Clausen wrote: > On 1/16/23 06:58, Michal Simek wrote: >> >> >> On 1/7/23 22:18, Lars-Peter Clausen wrote: >>> >>> In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C >>> controller is written and read back multiple times. The register value does >>> not change on its own. So it is possible to remember the just written value >>> instead of reading it back from the hardware. >>> >>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >>> --- >>> drivers/i2c/busses/i2c-cadence.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c >>> index bec50bfe7aad..93c6d0822468 100644 >>> --- a/drivers/i2c/busses/i2c-cadence.c >>> +++ b/drivers/i2c/busses/i2c-cadence.c >>> @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) >>> >>> /* Determine hold_clear based on number of bytes to receive and hold >>> flag */ >>> if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) { >>> - if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) { >>> + if (ctrl_reg & CDNS_I2C_CR_HOLD) { >>> hold_clear = true; >>> if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT) >>> irq_save = true; >>> @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) >>> addr &= CDNS_I2C_ADDR_MASK; >>> >>> if (hold_clear) { >>> - ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & >>> ~CDNS_I2C_CR_HOLD; >>> + ctrl_reg &= ~CDNS_I2C_CR_HOLD; >>> /* >>> * 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.30.2 >>> >> >> Logically this is fine but that additional read on CR register ensures that IP >> receive previous writes. The code itself is related to bug on Zynq SoC and >> that two additional readbacks can actually do something. >> >> I think this should be properly tested on zynq to ensure that it doesn't break >> anything. >> >> Shubhrajyoti: Can you please make sure that it is tested on Zynq? > Maybe it is better to drop the patch then if it is used to enforce ordering in > the hardware. But I guess we should add a comment to explain this. Mani has tested it and he can't see any issue that's why I am fine both ways. Thanks, Michal
> Mani has tested it and he can't see any issue that's why I am fine both ways.
I read this as an ack.
On Sat, Jan 07, 2023 at 01:18:14PM -0800, Lars-Peter Clausen wrote: > In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C > controller is written and read back multiple times. The register value does > not change on its own. So it is possible to remember the just written value > instead of reading it back from the hardware. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index bec50bfe7aad..93c6d0822468 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -613,7 +613,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) /* Determine hold_clear based on number of bytes to receive and hold flag */ if (!id->bus_hold_flag && id->recv_count <= CDNS_I2C_FIFO_DEPTH) { - if (cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & CDNS_I2C_CR_HOLD) { + if (ctrl_reg & CDNS_I2C_CR_HOLD) { hold_clear = true; if (id->quirks & CDNS_I2C_BROKEN_HOLD_BIT) irq_save = true; @@ -624,7 +624,7 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id) addr &= CDNS_I2C_ADDR_MASK; if (hold_clear) { - ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET) & ~CDNS_I2C_CR_HOLD; + ctrl_reg &= ~CDNS_I2C_CR_HOLD; /* * 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
In the `cdns_i2c_mrecv()` function the CTRL register of the Cadence I2C controller is written and read back multiple times. The register value does not change on its own. So it is possible to remember the just written value instead of reading it back from the hardware. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/i2c/busses/i2c-cadence.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)