| Message ID | 20200908224006.25636-2-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
| Context | Check | Description |
|---|---|---|
| tagr/GVS-1122491 | fail | None |
| tagr/GVS-1122491 | pending | None |
| tagr/GVS | pending | 1122533 |
| tagr/GVS | success | 1122533 |
| tagr/GVS | pending | 1124251 |
| tagr/GVS | success | 1124251 |
On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko wrote: > The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies > are not updating if interrupts are disabled. Let's switch to use iopoll > API helpers for register-polling. The iopoll API provides helpers for both > atomic and non-atomic cases. > > Note that this patch doesn't fix any known problem because normally FIFO > is flushed at the time of starting a new transfer. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 00d3e4d7a01e..ab88cdd70376 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) > > static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > { > - unsigned long timeout = jiffies + HZ; > - unsigned int offset; > - u32 mask, val; > + u32 mask, val, offset, reg_offset; Is there are reason why we need reg_offset? Seems to me like we could simplify this, see below. > + void __iomem *addr; > + int err; > > if (i2c_dev->hw->has_mst_fifo) { > mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | > @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > val |= mask; > i2c_writel(i2c_dev, val, offset); > > - while (i2c_readl(i2c_dev, offset) & mask) { > - if (time_after(jiffies, timeout)) { > - dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n"); > - return -ETIMEDOUT; > - } > - usleep_range(1000, 2000); > + reg_offset = tegra_i2c_reg_addr(i2c_dev, offset); > + addr = i2c_dev->base + reg_offset; Why not just: offset = tegra_i2c_reg_offset(i2c_dev, offset); addr = i2c_dev->base + offset; or even just: addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset); ? That makes the patch much smaller because you don't have to rewrite the whole variable declaration block and just add the "addr" and "err" variables while removing "timeout". Thierry
17.09.2020 14:10, Thierry Reding пишет: > On Wed, Sep 09, 2020 at 01:39:33AM +0300, Dmitry Osipenko wrote: >> The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies >> are not updating if interrupts are disabled. Let's switch to use iopoll >> API helpers for register-polling. The iopoll API provides helpers for both >> atomic and non-atomic cases. >> >> Note that this patch doesn't fix any known problem because normally FIFO >> is flushed at the time of starting a new transfer. >> >> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++--------- >> 1 file changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >> index 00d3e4d7a01e..ab88cdd70376 100644 >> --- a/drivers/i2c/busses/i2c-tegra.c >> +++ b/drivers/i2c/busses/i2c-tegra.c >> @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) >> >> static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) >> { >> - unsigned long timeout = jiffies + HZ; >> - unsigned int offset; >> - u32 mask, val; >> + u32 mask, val, offset, reg_offset; > > Is there are reason why we need reg_offset? Seems to me like we could > simplify this, see below. The reason is that this patch addresses potential problem, hence it's not a cleanup change and it should be kept minimal. >> + void __iomem *addr; >> + int err; >> >> if (i2c_dev->hw->has_mst_fifo) { >> mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | >> @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) >> val |= mask; >> i2c_writel(i2c_dev, val, offset); >> >> - while (i2c_readl(i2c_dev, offset) & mask) { >> - if (time_after(jiffies, timeout)) { >> - dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n"); >> - return -ETIMEDOUT; >> - } >> - usleep_range(1000, 2000); >> + reg_offset = tegra_i2c_reg_addr(i2c_dev, offset); >> + addr = i2c_dev->base + reg_offset; > > Why not just: > > offset = tegra_i2c_reg_offset(i2c_dev, offset); > addr = i2c_dev->base + offset; > > or even just: > > addr = i2c_dev->base + tegra_i2c_reg_offset(i2c_dev, offset); > > ? That makes the patch much smaller because you don't have to rewrite > the whole variable declaration block and just add the "addr" and "err" > variables while removing "timeout". There is a further "cleanup" patch named "Factor out register polling into separate function" that already implements yours suggestion.
On Wed, 09 Sep 2020 01:39:33 +0300, Dmitry Osipenko wrote: > The tegra_i2c_flush_fifos() shouldn't sleep in atomic transfer and jiffies > are not updating if interrupts are disabled. Let's switch to use iopoll > API helpers for register-polling. The iopoll API provides helpers for both > atomic and non-atomic cases. > > Note that this patch doesn't fix any known problem because normally FIFO > is flushed at the time of starting a new transfer. > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/i2c/busses/i2c-tegra.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) Tested-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 00d3e4d7a01e..ab88cdd70376 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -470,9 +470,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) { - unsigned long timeout = jiffies + HZ; - unsigned int offset; - u32 mask, val; + u32 mask, val, offset, reg_offset; + void __iomem *addr; + int err; if (i2c_dev->hw->has_mst_fifo) { mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | @@ -488,12 +488,19 @@ static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) val |= mask; i2c_writel(i2c_dev, val, offset); - while (i2c_readl(i2c_dev, offset) & mask) { - if (time_after(jiffies, timeout)) { - dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n"); - return -ETIMEDOUT; - } - usleep_range(1000, 2000); + reg_offset = tegra_i2c_reg_addr(i2c_dev, offset); + addr = i2c_dev->base + reg_offset; + + if (i2c_dev->is_curr_atomic_xfer) + err = readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), + 1000, 1000000); + else + err = readl_relaxed_poll_timeout(addr, val, !(val & mask), + 1000, 1000000); + + if (err) { + dev_err(i2c_dev->dev, "failed to flush FIFO\n"); + return err; } return 0; }