[PATCH V2] staging: nvec: make i2c controller register writes robust

[PATCH V2] staging: nvec: make i2c controller register writes robust

[Marc Dietrich]

The i2c controller needs to read back the data written to its registers.
This way we can avoid the long delay in the interrupt handler.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
V2: rename i2c_writel to tegra_i2c_writel
 drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index e5ca78e57384..d67d2f3338b2 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -570,6 +570,22 @@ static void nvec_tx_set(struct nvec_chip *nvec)
 		(uint)nvec->tx->size, nvec->tx->data[1]);
 }

+/**
+ * tegra_i2c_writel - safely write to an I2C client controller register
+ * @val: value to be written
+ * @reg: register to write to
+ *
+ * A write to an I2C controller register needs to be read back to make sure
+ * that the value has arrived.
+ */
+static void tegra_i2c_writel(u32 val, void *reg)
+{
+	writel_relaxed(val, reg);
+
+	/* read back register to make sure that register writes completed */
+	readl_relaxed(reg);
+}
+
 /**
  * nvec_interrupt - Interrupt handler
  * @irq: The IRQ
@@ -604,7 +620,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 	if ((status & RNW) == 0) {
 		received = readl(nvec->base + I2C_SL_RCVD);
 		if (status & RCVD)
-			writel(0, nvec->base + I2C_SL_RCVD);
+			tegra_i2c_writel(0, nvec->base + I2C_SL_RCVD);
 	}

 	if (status == (I2C_SL_IRQ | RCVD))
@@ -696,7 +712,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)

 	/* Send data if requested, but not on end of transmission */
 	if ((status & (RNW | END_TRANS)) == RNW)
-		writel(to_send, nvec->base + I2C_SL_RCVD);
+		tegra_i2c_writel(to_send, nvec->base + I2C_SL_RCVD);

 	/* If we have send the first byte */
 	if (status == (I2C_SL_IRQ | RNW | RCVD))
@@ -713,15 +729,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 		status & RCVD ? " RCVD" : "",
 		status & RNW ? " RNW" : "");

-	/*
-	 * TODO: replace the udelay with a read back after each writel above
-	 * in order to work around a hardware issue, see i2c-tegra.c
-	 *
-	 * Unfortunately, this change causes an initialisation issue with the
-	 * touchpad, which needs to be fixed first.
-	 */
-	udelay(100);
-
 	return IRQ_HANDLED;
 }

@@ -737,15 +744,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)

 	val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
 	    (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
-	writel(val, nvec->base + I2C_CNFG);
+	tegra_i2c_writel(val, nvec->base + I2C_CNFG);

 	clk_set_rate(nvec->i2c_clk, 8 * 80000);

-	writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
-	writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
+	tegra_i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
+	tegra_i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);

-	writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
-	writel(0, nvec->base + I2C_SL_ADDR2);
+	tegra_i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
+	tegra_i2c_writel(0, nvec->base + I2C_SL_ADDR2);

 	enable_irq(nvec->irq);
 }
@@ -754,7 +761,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
 static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
 {
 	disable_irq(nvec->irq);
-	writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
+	tegra_i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
 	clk_disable_unprepare(nvec->i2c_clk);
 }
 #endif
--
2.43.0

Re: [PATCH V2] staging: nvec: make i2c controller register writes robust

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Sun May 26, 2024 at 9:39 PM CEST, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.
>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
> V2: rename i2c_writel to tegra_i2c_writel
>  drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 17 deletions(-)

Hi Marc,

I've been trying to find out why we need to do these register read backs
and so far I haven't found anything tangible. The only thing I was able
to find that sounds like it could be remotely related to this is a
mention of the interface clock being fixed at 72 MHz. So I'm wondering
if you could perhaps verify in your setup what the I2C module clock is
for the NVEC controller (any dump of the clk_summary debugfs file after
boot would do).

Since I'm not sure we'll get to the bottom of this, this looks clean and
is certainly an improvement over the udelay(100), so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V2] staging: nvec: make i2c controller register writes robust

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

On Thu May 30, 2024 at 3:18 PM CEST, Thierry Reding wrote:
> On Sun May 26, 2024 at 9:39 PM CEST, Marc Dietrich wrote:
> > The i2c controller needs to read back the data written to its registers.
> > This way we can avoid the long delay in the interrupt handler.
> >
> > Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> > ---
> > V2: rename i2c_writel to tegra_i2c_writel
> >  drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
>
> Hi Marc,
>
> I've been trying to find out why we need to do these register read backs
> and so far I haven't found anything tangible. The only thing I was able
> to find that sounds like it could be remotely related to this is a
> mention of the interface clock being fixed at 72 MHz. So I'm wondering
> if you could perhaps verify in your setup what the I2C module clock is
> for the NVEC controller (any dump of the clk_summary debugfs file after
> boot would do).
>
> Since I'm not sure we'll get to the bottom of this, this looks clean and
> is certainly an improvement over the udelay(100), so:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>

So, after digging through some more old archives I think I now have a
better understanding of this. Looking through the Tegra I2C driver git
log I see that the read-back was added in commit ec7aaca2f64f ("i2c:
tegra: make sure register writes completes"), which mentions the
PortalPlayer System Bus (PPSB). That's a custom bus (similar to APB)
that can be found in Tegra devices from Tegra20 to Tegra210. The first
chip where this no longer seems to be present is Tegra186.

Now, as Laxman said in the description of the above commit, this bus
seems to queue writes, and the read-backs are needed to flush the write
queue. Based on that it should be possible to narrow down the scope of
this patch and only do the read-back in a couple of strategic places.

Again, not sure if it's really worth it because it could be quite tricky
to understand where exactly they are needed and people may miss this
when shuffling code around or adding new code, so doing it for all
writes seems like the safer option.

Anyway, scratch that idea about the clock domain. It might be worth
adding some of the above background information to the commit message to
clarify why this is needed.

My earlier Reviewed-by stands regardless.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]