Hi Marco, On 2020-01-20 10:36, Marco Felsch wrote: > From: Stefan Lengfeld <contact@xxxxxxxxxxxxxxx> > > Rework the read and write code paths in the driver to support operation > in atomic contexts. To achieve this, the driver must not rely on IRQs > and not call schedule(), e.g. via a sleep routine, in these cases. > > With this patch the driver supports normal operation, DMA transfers and > now the polling mode or also called sleep-free or IRQ-less operation. It > makes the code not simpler or easier to read, but atomic I2C transfers > are needed on some hardware configurations, e.g. to trigger reboots on > an external PMIC chip. Thanks for picking this up! > > Signed-off-by: Stefan Lengfeld <contact@xxxxxxxxxxxxxxx> > [m.felsch@xxxxxxxxxxxxxx: integrate > https://patchwork.ozlabs.org/patch/1085943/ review feedback] > [m.felsch@xxxxxxxxxxxxxx: adapt commit message] > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx> > Acked-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> > --- > Hi, > > I picked Stefan Lengfeld RFC patch [1] and added Stefan Agner's review > feedback [1]. Checkpatch complains about a few 80 char violations. I > kept those to gain readability. > > Regards, > Marco > > [1] https://patchwork.ozlabs.org/patch/1085943/ > > Changes: > - general: adapt commit message > - general: fix some 80char line issues > - general: s/if(!atomic)/if(atomic)/ > - i2c_imx_trx_complete: use readb_poll_timeout_atomic() > - i2c_imx_trx_complete: adapt poll_timeout and add poll_timeout calc comment > - i2c_imx_start: simplify irq disable > - i2c_imx_xfer_common: don't allow bus recovery within atomic context > - i2c_imx_probe: drop pm_runtime_irq_safe usage and instead: > * i2c_imx_xfer_common: move rpm calls into i2c_imx_xfer > * i2c_imx_xfer_common: add clk_enable/disable for i2c_imx_xfer_atomic > --- > drivers/i2c/busses/i2c-imx.c | 146 +++++++++++++++++++++++++---------- > 1 file changed, 105 insertions(+), 41 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index a3b61336fe55..79d5b37fd8a1 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -34,6 +34,7 @@ > #include <linux/init.h> > #include <linux/interrupt.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > @@ -414,7 +415,7 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) > dma->chan_using = NULL; > } > > -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) > +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int > for_busy, bool atomic) > { > unsigned long orig_jiffies = jiffies; > unsigned int temp; > @@ -444,15 +445,37 @@ static int i2c_imx_bus_busy(struct > imx_i2c_struct *i2c_imx, int for_busy) > "<%s> I2C bus is busy\n", __func__); > return -ETIMEDOUT; > } > - schedule(); > + if (atomic) > + udelay(100); > + else > + schedule(); > } > > return 0; > } > > -static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx) > +static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool atomic) > { > - wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10); > + if (atomic) { > + void __iomem *addr = i2c_imx->base + (IMX_I2C_I2SR << > i2c_imx->hwdata->regshift); > + unsigned int regval; > + > + /* > + * The formula for the poll timeout is documented in the RM > + * Rev.5 on page 1878: > + * T_min = 10/F_scl > + * Set the value hard as it is done for the non-atomic use-case. > + * Use 10 kHz for the calculation since this is the minimum > + * allowed SMBus frequency. Also add an offset of 100us since it > + * turned out that the I2SR_IIF bit isn't set correctly within > + * the minimum timeout in polling mode. > + */ > + readb_poll_timeout_atomic(addr, regval, regval & I2SR_IIF, 5, 1000 + 100); > + i2c_imx->i2csr = regval; > + imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); > + } else { > + wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10); > + } > > if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) { > dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__); > @@ -530,7 +553,7 @@ static int i2c_imx_clk_notifier_call(struct > notifier_block *nb, > return NOTIFY_OK; > } > > -static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) > +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx, bool atomic) > { > unsigned int temp = 0; > int result; > @@ -543,23 +566,29 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) > imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR); > > /* Wait controller to be stable */ > - usleep_range(50, 150); > + if (atomic) > + udelay(50); > + else > + usleep_range(50, 150); > > /* Start I2C transaction */ > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > temp |= I2CR_MSTA; > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - result = i2c_imx_bus_busy(i2c_imx, 1); > + result = i2c_imx_bus_busy(i2c_imx, 1, atomic); > if (result) > return result; > > temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; > + if (atomic) > + temp &= ~I2CR_IIEN; /* Disable interrupt */ > + Wouldn't it make more sense if we reverse the logic here, e.g. only enable interrupt if (!atomic). Otherwise looks good to me! Reviewed-by: Stefan Agner <stefan@xxxxxxxx> -- Stefan > temp &= ~I2CR_DMAEN; > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > return result; > } > > -static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) > +static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool atomic) > { > unsigned int temp = 0; > > @@ -581,7 +610,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) > } > > if (!i2c_imx->stopped) > - i2c_imx_bus_busy(i2c_imx, 0); > + i2c_imx_bus_busy(i2c_imx, 0, atomic); > > /* Disable I2C controller */ > temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, > @@ -662,7 +691,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx, > /* The last data byte must be transferred by the CPU. */ > imx_i2c_write_reg(msgs->buf[msgs->len-1], > i2c_imx, IMX_I2C_I2DR); > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, false); > if (result) > return result; > > @@ -721,7 +750,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, > > msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); > /* read n byte data */ > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, false); > if (result) > return result; > > @@ -734,7 +763,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > temp &= ~(I2CR_MSTA | I2CR_MTX); > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - i2c_imx_bus_busy(i2c_imx, 0); > + i2c_imx_bus_busy(i2c_imx, 0, false); > } else { > /* > * For i2c master receiver repeat restart operation like: > @@ -752,7 +781,8 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, > return 0; > } > > -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) > +static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, > + bool atomic) > { > int i, result; > > @@ -761,7 +791,7 @@ static int i2c_imx_write(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs) > > /* write slave address */ > imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR); > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, atomic); > if (result) > return result; > result = i2c_imx_acked(i2c_imx); > @@ -775,7 +805,7 @@ static int i2c_imx_write(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs) > "<%s> write byte: B%d=0x%X\n", > __func__, i, msgs->buf[i]); > imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR); > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, atomic); > if (result) > return result; > result = i2c_imx_acked(i2c_imx); > @@ -785,7 +815,8 @@ static int i2c_imx_write(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs) > return 0; > } > > -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct > i2c_msg *msgs, bool is_lastmsg) > +static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, > + bool is_lastmsg, bool atomic) > { > int i, result; > unsigned int temp; > @@ -798,7 +829,7 @@ static int i2c_imx_read(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs, bo > > /* write slave address */ > imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR); > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, atomic); > if (result) > return result; > result = i2c_imx_acked(i2c_imx); > @@ -831,7 +862,7 @@ static int i2c_imx_read(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs, bo > for (i = 0; i < msgs->len; i++) { > u8 len = 0; > > - result = i2c_imx_trx_complete(i2c_imx); > + result = i2c_imx_trx_complete(i2c_imx, atomic); > if (result) > return result; > /* > @@ -859,7 +890,7 @@ static int i2c_imx_read(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs, bo > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > temp &= ~(I2CR_MSTA | I2CR_MTX); > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - i2c_imx_bus_busy(i2c_imx, 0); > + i2c_imx_bus_busy(i2c_imx, 0, atomic); > } else { > /* > * For i2c master receiver repeat restart operation like: > @@ -890,8 +921,8 @@ static int i2c_imx_read(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs, bo > return 0; > } > > -static int i2c_imx_xfer(struct i2c_adapter *adapter, > - struct i2c_msg *msgs, int num) > +static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > + struct i2c_msg *msgs, int num, bool atomic) > { > unsigned int i, temp; > int result; > @@ -900,16 +931,16 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); > > - result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > - if (result < 0) > - goto out; > - > /* Start I2C transfer */ > - result = i2c_imx_start(i2c_imx); > + result = i2c_imx_start(i2c_imx, atomic); > if (result) { > - if (i2c_imx->adapter.bus_recovery_info) { > + /* > + * Bus recovery uses gpiod_get_value_cansleep() which is not > + * allowed within atomic context. > + */ > + if (!atomic && i2c_imx->adapter.bus_recovery_info) { > i2c_recover_bus(&i2c_imx->adapter); > - result = i2c_imx_start(i2c_imx); > + result = i2c_imx_start(i2c_imx, atomic); > } > } > > @@ -927,7 +958,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > temp |= I2CR_RSTA; > imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - result = i2c_imx_bus_busy(i2c_imx, 1); > + result = i2c_imx_bus_busy(i2c_imx, 1, atomic); > if (result) > goto fail0; > } > @@ -951,13 +982,14 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > (temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0), > (temp & I2SR_RXAK ? 1 : 0)); > #endif > - if (msgs[i].flags & I2C_M_RD) > - result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg); > - else { > - if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD) > + if (msgs[i].flags & I2C_M_RD) { > + result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg, atomic); > + } else { > + if (!atomic && > + i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD) > result = i2c_imx_dma_write(i2c_imx, &msgs[i]); > else > - result = i2c_imx_write(i2c_imx, &msgs[i]); > + result = i2c_imx_write(i2c_imx, &msgs[i], atomic); > } > if (result) > goto fail0; > @@ -965,18 +997,49 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, > > fail0: > /* Stop I2C transfer */ > - i2c_imx_stop(i2c_imx); > - > - pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > - pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > + i2c_imx_stop(i2c_imx, atomic); > > -out: > dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__, > (result < 0) ? "error" : "success msg", > (result < 0) ? result : num); > return (result < 0) ? result : num; > } > > +static int i2c_imx_xfer(struct i2c_adapter *adapter, > + struct i2c_msg *msgs, int num) > +{ > + struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); > + int result; > + > + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > + if (result < 0) > + return result; > + > + result = i2c_imx_xfer_common(adapter, msgs, num, false); > + > + pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent); > + pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent); > + > + return result; > +} > + > +static int i2c_imx_xfer_atomic(struct i2c_adapter *adapter, > + struct i2c_msg *msgs, int num) > +{ > + struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); > + int result; > + > + result = clk_enable(i2c_imx->clk); > + if (result) > + return result; > + > + result = i2c_imx_xfer_common(adapter, msgs, num, true); > + > + clk_disable(i2c_imx->clk); > + > + return result; > +} > + > static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) > { > struct imx_i2c_struct *i2c_imx; > @@ -1049,8 +1112,9 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter) > } > > static const struct i2c_algorithm i2c_imx_algo = { > - .master_xfer = i2c_imx_xfer, > - .functionality = i2c_imx_func, > + .master_xfer = i2c_imx_xfer, > + .master_xfer_atomic = i2c_imx_xfer_atomic, > + .functionality = i2c_imx_func, > }; > > static int i2c_imx_probe(struct platform_device *pdev)
