| Message ID | 20240215-mbly-i2c-v1-8-19a336e91dca@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > The FIFO flush function uses a jiffies amount to detect timeouts as the > flushing is async. Replace with ktime to get more accurate precision > and support short timeouts. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Excellent patch. Thanks. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hello, On Mon Feb 19, 2024 at 3:21 PM CET, Linus Walleij wrote: > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > The FIFO flush function uses a jiffies amount to detect timeouts as the > > flushing is async. Replace with ktime to get more accurate precision > > and support short timeouts. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Excellent patch. Thanks. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Somewhat related to this patch: while writing it, I noticed the total timeout of flush_i2c_fifo() is 10 times the timeout. Without this series, this means 10*200ms of busywaiting! If you have an opinion on a more sensible option for this I could add a patch to my V2. I don't know enough to pick a sensible value. I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a multiple of the transfer timeout. Does it make sense that those two timeouts are correlated? Big thanks for your review, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Feb 19, 2024 at 3:38 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Somewhat related to this patch: while writing it, I noticed the total > timeout of flush_i2c_fifo() is 10 times the timeout. Without this > series, this means 10*200ms of busywaiting! > > If you have an opinion on a more sensible option for this I could add a > patch to my V2. I don't know enough to pick a sensible value. > > I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a > multiple of the transfer timeout. Does it make sense that those two > timeouts are correlated? I have a *vague* memory of the timeouts for flushing needing to be longer but I might be mistaken. This is probably a Srinidhi or even Sachin question... Sadly I don't have their current mail addresses. Yours, Linus Walleij
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index e68b8e0d7919..afd54999bbbb 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -219,8 +219,8 @@ static inline void i2c_clr_bit(void __iomem *reg, u32 mask) static int flush_i2c_fifo(struct nmk_i2c_dev *priv) { #define LOOP_ATTEMPTS 10 + ktime_t timeout; int i; - unsigned long timeout; /* * flush the transmit and receive FIFO. The flushing @@ -232,9 +232,9 @@ static int flush_i2c_fifo(struct nmk_i2c_dev *priv) writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR); for (i = 0; i < LOOP_ATTEMPTS; i++) { - timeout = jiffies + priv->adap.timeout; + timeout = ktime_add_us(ktime_get(), priv->timeout_usecs); - while (!time_after(jiffies, timeout)) { + while (ktime_after(timeout, ktime_get())) { if ((readl(priv->virtbase + I2C_CR) & (I2C_CR_FTX | I2C_CR_FRX)) == 0) return 0;
The FIFO flush function uses a jiffies amount to detect timeouts as the flushing is async. Replace with ktime to get more accurate precision and support short timeouts. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/i2c/busses/i2c-nomadik.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)