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

[PATCH] 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>
---
 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 45df190c2f94..214839f51048 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]);
 }

+/**
+ * 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 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);
+			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);
+		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);
+	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);
+	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
+	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);
+	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
+	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);
+	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] staging: nvec: make i2c controller register writes robust

[Greg KH]

On Sun, Apr 21, 2024 at 12:46:42PM +0200, 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>
> ---
>  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 45df190c2f94..214839f51048 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]);
>  }
> 
> +/**
> + * 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 i2c_writel(u32 val, void *reg)
> +{
> +	writel_relaxed(val, reg);
> +
> +	/* read back register to make sure that register writes completed */
> +	readl_relaxed(reg);

Do you need to compare the value to make sure it happened properly?

And how is this an i2c write?  Normally that implies this is using the
i2c core functions, this name is going to get confusing very quickly...

thanks,

greg k-h

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

[Marc Dietrich]

Hi Greg,

On Sun, 21 Apr 2024, Greg KH wrote:

> On Sun, Apr 21, 2024 at 12:46:42PM +0200, 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>
>> ---
>> ...
>> + * A write to an I2C controller register needs to be read back to make sure
>> + * that the value has arrived.
>> + */
>> +static void i2c_writel(u32 val, void *reg)
>> +{
>> +	writel_relaxed(val, reg);
>> +
>> +	/* read back register to make sure that register writes completed */
>> +	readl_relaxed(reg);
>
> Do you need to compare the value to make sure it happened properly?
>
> And how is this an i2c write?  Normally that implies this is using the
> i2c core functions, this name is going to get confusing very quickly...

I just used the same name as in i2c-tegra, which may be helpful when the 
drivers will be merged in the future.
The reason why a readback is needed is descibed in commit ec7aaca2f6. As 
far as I understand (it is not mentioned in the TRM AFAIK), writes are 
somehow buffered and a readback guaranties that the write really reached 
the controller. The alternative was just to wait some time...
From the commit message it seems that this only applies to the interrupt 
status register, but it does not harm the do this also for the data 
registers just like the host driver does.

Best regards,

Marc

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

[Ben Dooks]

On 21/04/2024 11:46, 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>
> ---
>   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 45df190c2f94..214839f51048 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]);
>   }
> 
> +/**
> + * 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 i2c_writel(u32 val, void *reg)
> +{
> +	writel_relaxed(val, reg);
> +
> +	/* read back register to make sure that register writes completed */
> +	readl_relaxed(reg);
> +}

I thought the default behaviour of writel() should be to force writes
out of any CPU buffers. Are there any bus isuses here causing the code
to be necessary (and if so, why is there another buffer breaking the
writel behaviour?)

>   /**
>    * 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);
> +			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);
> +		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);
> +	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);
> +	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
> +	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);
> +	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
> +	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);
> +	i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
>   	clk_disable_unprepare(nvec->i2c_clk);
>   }
>   #endif
> --
> 2.43.0
> 
> 
> 

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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

[Marc Dietrich]

Hi Ben,

On Mon, 22 Apr 2024, Ben Dooks wrote:

> On 21/04/2024 11:46, 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>
>> ---
>>   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 45df190c2f94..214839f51048 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]);
>>   }
>>
>> +/**
>> + * 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 i2c_writel(u32 val, void *reg)
>> +{
>> +	writel_relaxed(val, reg);
>> +
>> +	/* read back register to make sure that register writes completed */
>> +	readl_relaxed(reg);
>> +}
>
> I thought the default behaviour of writel() should be to force writes
> out of any CPU buffers. Are there any bus isuses here causing the code
> to be necessary (and if so, why is there another buffer breaking the
> writel behaviour?)

if fear that's a question only NVIDIA can answer.

Marc