diff mbox series

[v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl()

Message ID 1603166634-13639-1-git-send-email-skomatineni@nvidia.com
State New
Headers show
Series [v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl() | expand

Commit Message

Sowjanya Komatineni Oct. 20, 2020, 4:03 a.m. UTC
VI I2C don't have DMA support and uses PIO mode all the time.

Current driver uses writesl() to fill TX FIFO based on available
empty slots and with this seeing strange silent hang during any I2C
register access after filling TX FIFO with 8 words.

Using writel() followed by i2c_readl() in a loop to write all words
to TX FIFO instead of using writesl() helps for large transfers in
PIO mode.

So, this patch updates i2c_writesl() API to use writel() in a loop
instead of writesl().

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Thierry Reding Oct. 20, 2020, 7:48 a.m. UTC | #1
On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
> VI I2C don't have DMA support and uses PIO mode all the time.
> 
> Current driver uses writesl() to fill TX FIFO based on available
> empty slots and with this seeing strange silent hang during any I2C
> register access after filling TX FIFO with 8 words.
> 
> Using writel() followed by i2c_readl() in a loop to write all words
> to TX FIFO instead of using writesl() helps for large transfers in
> PIO mode.
> 
> So, this patch updates i2c_writesl() API to use writel() in a loop
> instead of writesl().
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 6f08c0c..274bf3a 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned int reg)
>  	return readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>  }
>  
> -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>  			unsigned int reg, unsigned int len)
>  {
> -	writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> +	while (len--) {
> +		writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +		i2c_readl(i2c_dev, I2C_INT_STATUS);
> +	}
>  }
>  
>  static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>  		i2c_dev->msg_buf_remaining = buf_remaining;
>  		i2c_dev->msg_buf = buf + words_to_transfer * BYTES_PER_FIFO_WORD;
>  
> -		i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
> +		i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO, words_to_transfer);

I've thought a bit more about this and I wonder if we're simply reading
out the wrong value for tx_fifo_avail and therefore end up overflowing
the TX FIFO. Have you checked what the value is for tx_fifo_avail when
this silent hang occurs? Given that this is specific to the VI I2C I'm
wondering if this is perhaps a hardware bug where we read the wrong TX
FIFO available count.

Thierry
Sowjanya Komatineni Oct. 20, 2020, 4:37 p.m. UTC | #2
On 10/20/20 12:48 AM, Thierry Reding wrote:
> On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
>> VI I2C don't have DMA support and uses PIO mode all the time.
>>
>> Current driver uses writesl() to fill TX FIFO based on available
>> empty slots and with this seeing strange silent hang during any I2C
>> register access after filling TX FIFO with 8 words.
>>
>> Using writel() followed by i2c_readl() in a loop to write all words
>> to TX FIFO instead of using writesl() helps for large transfers in
>> PIO mode.
>>
>> So, this patch updates i2c_writesl() API to use writel() in a loop
>> instead of writesl().
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   drivers/i2c/busses/i2c-tegra.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 6f08c0c..274bf3a 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned int reg)
>>   	return readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>>   }
>>   
>> -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>>   			unsigned int reg, unsigned int len)
>>   {
>> -	writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
>> +	while (len--) {
>> +		writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
>> +		i2c_readl(i2c_dev, I2C_INT_STATUS);
>> +	}
>>   }
>>   
>>   static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>>   		i2c_dev->msg_buf_remaining = buf_remaining;
>>   		i2c_dev->msg_buf = buf + words_to_transfer * BYTES_PER_FIFO_WORD;
>>   
>> -		i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
>> +		i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO, words_to_transfer);
> I've thought a bit more about this and I wonder if we're simply reading
> out the wrong value for tx_fifo_avail and therefore end up overflowing
> the TX FIFO. Have you checked what the value is for tx_fifo_avail when
> this silent hang occurs? Given that this is specific to the VI I2C I'm
> wondering if this is perhaps a hardware bug where we read the wrong TX
> FIFO available count.
>
> Thierry

Yes FIFO status shows all 8 slots available.

Also, HW wise VI I2C is similar to other I2C and FIFO depth is also 8 
words. Confirmed from HW designers as well.

Using writesl() causes silent hang after filling some words in FIFO and 
most of time after filling 8 words and sometime after filling it with 
around 6 words.

I am not sure if this issue is specific to VI I2C alone as other I2C 
mostly use DMA for more than 8 words and I am not sure if we ever used 
other I2C in PIO mode for ~8 words transfer for slave devices we have on 
the platform.


Thanks

Sowjanya
Sowjanya Komatineni Oct. 20, 2020, 4:39 p.m. UTC | #3
On 10/20/20 9:37 AM, Sowjanya Komatineni wrote:
>
> On 10/20/20 12:48 AM, Thierry Reding wrote:
>> On Mon, Oct 19, 2020 at 09:03:54PM -0700, Sowjanya Komatineni wrote:
>>> VI I2C don't have DMA support and uses PIO mode all the time.
>>>
>>> Current driver uses writesl() to fill TX FIFO based on available
>>> empty slots and with this seeing strange silent hang during any I2C
>>> register access after filling TX FIFO with 8 words.
>>>
>>> Using writel() followed by i2c_readl() in a loop to write all words
>>> to TX FIFO instead of using writesl() helps for large transfers in
>>> PIO mode.
>>>
>>> So, this patch updates i2c_writesl() API to use writel() in a loop
>>> instead of writesl().
>>>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>>   drivers/i2c/busses/i2c-tegra.c | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c 
>>> b/drivers/i2c/busses/i2c-tegra.c
>>> index 6f08c0c..274bf3a 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -333,10 +333,13 @@ static u32 i2c_readl(struct tegra_i2c_dev 
>>> *i2c_dev, unsigned int reg)
>>>       return readl_relaxed(i2c_dev->base + 
>>> tegra_i2c_reg_addr(i2c_dev, reg));
>>>   }
>>>   -static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
>>>               unsigned int reg, unsigned int len)
>>>   {
>>> -    writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, 
>>> len);
>>> +    while (len--) {
>>> +        writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, 
>>> reg));
>>> +        i2c_readl(i2c_dev, I2C_INT_STATUS);
>>> +    }
>>>   }
>>>     static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
>>> @@ -811,7 +814,7 @@ static int tegra_i2c_fill_tx_fifo(struct 
>>> tegra_i2c_dev *i2c_dev)
>>>           i2c_dev->msg_buf_remaining = buf_remaining;
>>>           i2c_dev->msg_buf = buf + words_to_transfer * 
>>> BYTES_PER_FIFO_WORD;
>>>   -        i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
>>> +        i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO, 
>>> words_to_transfer);
>> I've thought a bit more about this and I wonder if we're simply reading
>> out the wrong value for tx_fifo_avail and therefore end up overflowing
>> the TX FIFO. Have you checked what the value is for tx_fifo_avail when
>> this silent hang occurs? Given that this is specific to the VI I2C I'm
>> wondering if this is perhaps a hardware bug where we read the wrong TX
>> FIFO available count.
>>
>> Thierry
>
> Yes FIFO status shows all 8 slots available.

To be clear, TX slots availability in FIFO status before filling shows 8 
words empty.

With writesl() when silent hang is seen, couldn't access any registers 
and with debug messages I see silent hang happens immediate during 
accessing any of i2c controller register after writesl() call

>
> Also, HW wise VI I2C is similar to other I2C and FIFO depth is also 8 
> words. Confirmed from HW designers as well.
>
> Using writesl() causes silent hang after filling some words in FIFO 
> and most of time after filling 8 words and sometime after filling it 
> with around 6 words.
>
> I am not sure if this issue is specific to VI I2C alone as other I2C 
> mostly use DMA for more than 8 words and I am not sure if we ever used 
> other I2C in PIO mode for ~8 words transfer for slave devices we have 
> on the platform.
>
>
> Thanks
>
> Sowjanya
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 6f08c0c..274bf3a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -333,10 +333,13 @@  static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned int reg)
 	return readl_relaxed(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
 }
 
-static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
+static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, u32 *data,
 			unsigned int reg, unsigned int len)
 {
-	writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
+	while (len--) {
+		writel(*data++, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+		i2c_readl(i2c_dev, I2C_INT_STATUS);
+	}
 }
 
 static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
@@ -811,7 +814,7 @@  static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
 		i2c_dev->msg_buf_remaining = buf_remaining;
 		i2c_dev->msg_buf = buf + words_to_transfer * BYTES_PER_FIFO_WORD;
 
-		i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
+		i2c_writesl(i2c_dev, (u32 *)buf, I2C_TX_FIFO, words_to_transfer);
 
 		buf += words_to_transfer * BYTES_PER_FIFO_WORD;
 	}