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

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

[Sowjanya Komatineni]

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);
 
 		buf += words_to_transfer * BYTES_PER_FIFO_WORD;
 	}
-- 
2.7.4

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

[Thierry Reding]

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

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

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

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

[Sowjanya Komatineni]

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

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

[Sowjanya Komatineni]

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
>

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

[Dmitry Osipenko]

20.10.2020 19:37, Sowjanya Komatineni пишет:
> 
> 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.

Please explain how you checked that 8 slots are available, provide
example code.

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

[Dmitry Osipenko]

11.01.2021 14:50, Dmitry Osipenko пишет:
> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>
>> 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.
> 
> Please explain how you checked that 8 slots are available, provide
> example code.
> 

Have you checked the FIFO overflow interrupt?

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

[Sowjanya Komatineni]

On 1/11/21 4:09 AM, Dmitry Osipenko wrote:
> 11.01.2021 14:50, Dmitry Osipenko пишет:
>> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>> 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.
>> Please explain how you checked that 8 slots are available, provide
>> example code.
>>
> Have you checked the FIFO overflow interrupt?

This is seen with VI I2C (which is under host1x) as we use PIO mode 
always even for large transfers.
HW wise VI I2C is similar to other I2C and FIFO depth is also 8 words.

tegra_i2c_fill_tx_fifo() reads I2C_FIFO_STATUS register field 
TX_FIFO_EMPTY_CNT which tells empty slots available in TX_FIFO that can 
be filled.
Added debug message to print empty count and, during beginning of 
transfer when it executes tegra_i2c_fill_tx_fifo(), empty slots are 8


Using writesl() for filling TX_FIFO causing silent hang immediate on any 
i2c register access after filling FIFO with 8 words and some times with 
6 words as well.

So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows 
when this silent hang happens.

Tried to read thru back-door (JTAG path) but could not connect to JTAG 
either. Looks like Tegra chip is in some weird state.

But using writel() followed by i2c_readl helps. Not sure if any thing 
related to register access delay or some other issue.

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

[Dmitry Osipenko]

11.01.2021 20:38, Sowjanya Komatineni пишет:
> 
> On 1/11/21 4:09 AM, Dmitry Osipenko wrote:
>> 11.01.2021 14:50, Dmitry Osipenko пишет:
>>> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>>> 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.
>>> Please explain how you checked that 8 slots are available, provide
>>> example code.
>>>
>> Have you checked the FIFO overflow interrupt?
> 
> This is seen with VI I2C (which is under host1x) as we use PIO mode
> always even for large transfers.
> HW wise VI I2C is similar to other I2C and FIFO depth is also 8 words.
> 
> tegra_i2c_fill_tx_fifo() reads I2C_FIFO_STATUS register field
> TX_FIFO_EMPTY_CNT which tells empty slots available in TX_FIFO that can
> be filled.
> Added debug message to print empty count and, during beginning of
> transfer when it executes tegra_i2c_fill_tx_fifo(), empty slots are 8
> 
> 
> Using writesl() for filling TX_FIFO causing silent hang immediate on any
> i2c register access after filling FIFO with 8 words and some times with
> 6 words as well.
> 
> So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows
> when this silent hang happens.
> 
> Tried to read thru back-door (JTAG path) but could not connect to JTAG
> either. Looks like Tegra chip is in some weird state.
> 
> But using writel() followed by i2c_readl helps. Not sure if any thing
> related to register access delay or some other issue.
> 
> 

Does downstream kernel have this problem?

If there is really no good alternative right now, then perhaps should be
a bit cleaner to add i2c_writesl_vi(), which should contain explanatory
comment telling why it's needed and then it should be used only for the
VI I2C controller.

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

[Sowjanya Komatineni]

On 1/11/21 11:29 AM, Dmitry Osipenko wrote:
> 11.01.2021 20:38, Sowjanya Komatineni пишет:
>> On 1/11/21 4:09 AM, Dmitry Osipenko wrote:
>>> 11.01.2021 14:50, Dmitry Osipenko пишет:
>>>> 20.10.2020 19:37, Sowjanya Komatineni пишет:
>>>>> 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.
>>>> Please explain how you checked that 8 slots are available, provide
>>>> example code.
>>>>
>>> Have you checked the FIFO overflow interrupt?
>> This is seen with VI I2C (which is under host1x) as we use PIO mode
>> always even for large transfers.
>> HW wise VI I2C is similar to other I2C and FIFO depth is also 8 words.
>>
>> tegra_i2c_fill_tx_fifo() reads I2C_FIFO_STATUS register field
>> TX_FIFO_EMPTY_CNT which tells empty slots available in TX_FIFO that can
>> be filled.
>> Added debug message to print empty count and, during beginning of
>> transfer when it executes tegra_i2c_fill_tx_fifo(), empty slots are 8
>>
>>
>> Using writesl() for filling TX_FIFO causing silent hang immediate on any
>> i2c register access after filling FIFO with 8 words and some times with
>> 6 words as well.
>>
>> So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows
>> when this silent hang happens.
>>
>> Tried to read thru back-door (JTAG path) but could not connect to JTAG
>> either. Looks like Tegra chip is in some weird state.
>>
>> But using writel() followed by i2c_readl helps. Not sure if any thing
>> related to register access delay or some other issue.
>>
>>
> Does downstream kernel have this problem?
>
> If there is really no good alternative right now, then perhaps should be
> a bit cleaner to add i2c_writesl_vi(), which should contain explanatory
> comment telling why it's needed and then it should be used only for the
> VI I2C controller.

Yes downstream also has same issue when using writesl() and downstream 
vi i2c driver uses writel() followed by i2c_readl()

OK. Will create separate i2c_writesl_vi() to use with vi i2c and will 
add comment in the code.

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

[David Laight]

From: Sowjanya Komatineni
> Sent: 11 January 2021 17:38
...
> Using writesl() for filling TX_FIFO causing silent hang immediate on any
> i2c register access after filling FIFO with 8 words and some times with
> 6 words as well.
> 
> So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows
> when this silent hang happens.
> 
> Tried to read thru back-door (JTAG path) but could not connect to JTAG
> either. Looks like Tegra chip is in some weird state.
> 
> But using writel() followed by i2c_readl helps. Not sure if any thing
> related to register access delay or some other issue.

How much does the i2c_read() slow down the transfer?
If the device is PCIe it is probably significant.

If the underlying problem is that the Tegra chip can't handle
back to back writes to the tx fifo maybe there are other solutions!
1) Send it back and ask for a working chip :-)
2) Maybe an interleaved write will slow things down enough?

It may be worth testing back to back writes to other registers
to see if it is a problem that is specific to the tx fifo.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Sowjanya Komatineni]

On 1/12/21 1:32 AM, David Laight wrote:
> From: Sowjanya Komatineni
>> Sent: 11 January 2021 17:38
> ...
>> Using writesl() for filling TX_FIFO causing silent hang immediate on any
>> i2c register access after filling FIFO with 8 words and some times with
>> 6 words as well.
>>
>> So couldn't INTERRUPT_STATUS registers to check for TX FIFO Overflows
>> when this silent hang happens.
>>
>> Tried to read thru back-door (JTAG path) but could not connect to JTAG
>> either. Looks like Tegra chip is in some weird state.
>>
>> But using writel() followed by i2c_readl helps. Not sure if any thing
>> related to register access delay or some other issue.
> How much does the i2c_read() slow down the transfer?
> If the device is PCIe it is probably significant.
>
> If the underlying problem is that the Tegra chip can't handle
> back to back writes to the tx fifo maybe there are other solutions!
> 1) Send it back and ask for a working chip :-)
> 2) Maybe an interleaved write will slow things down enough?
>
> It may be worth testing back to back writes to other registers
> to see if it is a problem that is specific to the tx fifo.
>
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

This is a known hardware bug with VI I2C controller which is under 
host1x where immediate multiple writes to TX FIFO register gets stuck 
and reading from a register allows them to be flushed out.

VI I2C is dedicated for camera sensors or HDMI2CSI bridge.

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

[Thierry Reding]

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

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(-)

Hi Wolfram,

after discussing a bit more with Sowjanya, I don't think we're going to
have a better solution than this. It should be fairly harmless to have
this additional flushing read of the interrupt status register because
the reads don't have any side-effects (the register is write-1-to-clear)
and these write loops don't happen very often (or when they do we tend
to use DMA anyway), so:

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

I did notice that for some reason Sowjanya hadn't listed you as a
recipient, so perhaps you don't have this anywhere in your inbox. I've
quoted the patch fully for reference below and the patchwork link for
this is:

	https://patchwork.ozlabs.org/project/linux-i2c/patch/1603166634-13639-1-git-send-email-skomatineni@nvidia.com/

If you'd prefer to have this in your inbox for proper review, please
let us know so that Sowjanya can resend this.

Thanks,
Thierry

> 
> 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;
>  	}
> -- 
> 2.7.4
> 

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