Messages in this thread | | | Subject | Re: [PATCH v1] i2c: tegra: Fix i2c_writesl() to use writel() instead of writesl() | From | Sowjanya Komatineni <> | Date | Mon, 11 Jan 2021 11:33:23 -0800 |
| |
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.
| |