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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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.