This serie contains 3 fixes for I²C error handling cases.
It also includes a patch to get rid of the deprecated
dmaengine_terminate_all calls.
Alain Volmat (4):
i2c: stm32f7: flush TX FIFO upon transfer errors
i2c: stm32f7: recover the bus on access timeout
i2c: stm32f7: stop dma transfer in case of NACK
i2c: stm32f7: use proper DMAENGINE API for termination
drivers/i2c/busses/i2c-stm32f7.c | 45 +++++++++++++++++++++++++++-----
1 file changed, 38 insertions(+), 7 deletions(-)
--
2.25.1
dmaengine_terminate_all() is deprecated in favor of explicitly saying if
it should be sync or async. Here, we use dmaengine_terminate_sync in
i2c_xfer and i2c_smbus_xfer handlers and rely on
dmaengine_terminate_async within interrupt handlers
(transmission error cases).
dmaengine_synchronize is added within i2c_xfer and i2c_smbus_xfer handler
to finalize terminate started in interrupt handlers.
Signed-off-by: Alain Volmat <[email protected]>
---
drivers/i2c/busses/i2c-stm32f7.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 50d5ae81d227..66145d2b9b55 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -1521,7 +1521,7 @@ static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);
if (i2c_dev->use_dma) {
stm32f7_i2c_disable_dma_req(i2c_dev);
- dmaengine_terminate_all(dma->chan_using);
+ dmaengine_terminate_async(dma->chan_using);
}
f7_msg->result = -ENXIO;
}
@@ -1588,7 +1588,7 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
if (!ret) {
dev_dbg(i2c_dev->dev, "<%s>: Timed out\n", __func__);
stm32f7_i2c_disable_dma_req(i2c_dev);
- dmaengine_terminate_all(dma->chan_using);
+ dmaengine_terminate_async(dma->chan_using);
f7_msg->result = -ETIMEDOUT;
}
@@ -1665,7 +1665,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
/* Disable dma */
if (i2c_dev->use_dma) {
stm32f7_i2c_disable_dma_req(i2c_dev);
- dmaengine_terminate_all(dma->chan_using);
+ dmaengine_terminate_async(dma->chan_using);
}
i2c_dev->master_mode = false;
@@ -1702,6 +1702,9 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
i2c_dev->adap.timeout);
ret = f7_msg->result;
if (ret) {
+ if (i2c_dev->use_dma)
+ dmaengine_synchronize(dma->chan_using);
+
/*
* It is possible that some unsent data have already been
* written into TXDR. To avoid sending old data in a
@@ -1716,7 +1719,7 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n",
i2c_dev->msg->addr);
if (i2c_dev->use_dma)
- dmaengine_terminate_all(dma->chan_using);
+ dmaengine_terminate_sync(dma->chan_using);
stm32f7_i2c_wait_free_bus(i2c_dev);
ret = -ETIMEDOUT;
}
@@ -1761,6 +1764,9 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
i2c_dev->adap.timeout);
ret = f7_msg->result;
if (ret) {
+ if (i2c_dev->use_dma)
+ dmaengine_synchronize(dma->chan_using);
+
/*
* It is possible that some unsent data have already been
* written into TXDR. To avoid sending old data in a
@@ -1774,7 +1780,7 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
if (!timeout) {
dev_dbg(dev, "Access to slave 0x%x timed out\n", f7_msg->addr);
if (i2c_dev->use_dma)
- dmaengine_terminate_all(dma->chan_using);
+ dmaengine_terminate_sync(dma->chan_using);
stm32f7_i2c_wait_free_bus(i2c_dev);
ret = -ETIMEDOUT;
goto pm_free;
--
2.25.1
When getting an access timeout, ensure that the bus is in a proper
state prior to returning the error.
Fixes: aeb068c57214 ("i2c: i2c-stm32f7: add driver")
Signed-off-by: Alain Volmat <[email protected]>
---
drivers/i2c/busses/i2c-stm32f7.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index ed977b6f7ab6..ad3459a3bc5e 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -1712,6 +1712,7 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
i2c_dev->msg->addr);
if (i2c_dev->use_dma)
dmaengine_terminate_all(dma->chan_using);
+ stm32f7_i2c_wait_free_bus(i2c_dev);
ret = -ETIMEDOUT;
}
@@ -1769,6 +1770,7 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
dev_dbg(dev, "Access to slave 0x%x timed out\n", f7_msg->addr);
if (i2c_dev->use_dma)
dmaengine_terminate_all(dma->chan_using);
+ stm32f7_i2c_wait_free_bus(i2c_dev);
ret = -ETIMEDOUT;
goto pm_free;
}
--
2.25.1
Hi Alain
Look good to me
Reviewed-by: Pierre-Yves MORDRET <[email protected]>
Regards
On 9/20/21 5:21 PM, Alain Volmat wrote:
> When getting an access timeout, ensure that the bus is in a proper
> state prior to returning the error.
>
> Fixes: aeb068c57214 ("i2c: i2c-stm32f7: add driver")
>
> Signed-off-by: Alain Volmat <[email protected]>
> ---
> drivers/i2c/busses/i2c-stm32f7.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index ed977b6f7ab6..ad3459a3bc5e 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -1712,6 +1712,7 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> i2c_dev->msg->addr);
> if (i2c_dev->use_dma)
> dmaengine_terminate_all(dma->chan_using);
> + stm32f7_i2c_wait_free_bus(i2c_dev);
> ret = -ETIMEDOUT;
> }
>
> @@ -1769,6 +1770,7 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> dev_dbg(dev, "Access to slave 0x%x timed out\n", f7_msg->addr);
> if (i2c_dev->use_dma)
> dmaengine_terminate_all(dma->chan_using);
> + stm32f7_i2c_wait_free_bus(i2c_dev);
> ret = -ETIMEDOUT;
> goto pm_free;
> }
>
--
--
~ Py MORDRET
--
Hi Alain
Look good to me
Reviewed-by: Pierre-Yves MORDRET <[email protected]>
Regards
On 9/20/21 5:21 PM, Alain Volmat wrote:
> dmaengine_terminate_all() is deprecated in favor of explicitly saying if
> it should be sync or async. Here, we use dmaengine_terminate_sync in
> i2c_xfer and i2c_smbus_xfer handlers and rely on
> dmaengine_terminate_async within interrupt handlers
> (transmission error cases).
> dmaengine_synchronize is added within i2c_xfer and i2c_smbus_xfer handler
> to finalize terminate started in interrupt handlers.
>
> Signed-off-by: Alain Volmat <[email protected]>
> ---
> drivers/i2c/busses/i2c-stm32f7.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 50d5ae81d227..66145d2b9b55 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -1521,7 +1521,7 @@ static irqreturn_t stm32f7_i2c_isr_event(int irq, void *data)
> writel_relaxed(STM32F7_I2C_ICR_NACKCF, base + STM32F7_I2C_ICR);
> if (i2c_dev->use_dma) {
> stm32f7_i2c_disable_dma_req(i2c_dev);
> - dmaengine_terminate_all(dma->chan_using);
> + dmaengine_terminate_async(dma->chan_using);
> }
> f7_msg->result = -ENXIO;
> }
> @@ -1588,7 +1588,7 @@ static irqreturn_t stm32f7_i2c_isr_event_thread(int irq, void *data)
> if (!ret) {
> dev_dbg(i2c_dev->dev, "<%s>: Timed out\n", __func__);
> stm32f7_i2c_disable_dma_req(i2c_dev);
> - dmaengine_terminate_all(dma->chan_using);
> + dmaengine_terminate_async(dma->chan_using);
> f7_msg->result = -ETIMEDOUT;
> }
>
> @@ -1665,7 +1665,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
> /* Disable dma */
> if (i2c_dev->use_dma) {
> stm32f7_i2c_disable_dma_req(i2c_dev);
> - dmaengine_terminate_all(dma->chan_using);
> + dmaengine_terminate_async(dma->chan_using);
> }
>
> i2c_dev->master_mode = false;
> @@ -1702,6 +1702,9 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> i2c_dev->adap.timeout);
> ret = f7_msg->result;
> if (ret) {
> + if (i2c_dev->use_dma)
> + dmaengine_synchronize(dma->chan_using);
> +
> /*
> * It is possible that some unsent data have already been
> * written into TXDR. To avoid sending old data in a
> @@ -1716,7 +1719,7 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
> dev_dbg(i2c_dev->dev, "Access to slave 0x%x timed out\n",
> i2c_dev->msg->addr);
> if (i2c_dev->use_dma)
> - dmaengine_terminate_all(dma->chan_using);
> + dmaengine_terminate_sync(dma->chan_using);
> stm32f7_i2c_wait_free_bus(i2c_dev);
> ret = -ETIMEDOUT;
> }
> @@ -1761,6 +1764,9 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> i2c_dev->adap.timeout);
> ret = f7_msg->result;
> if (ret) {
> + if (i2c_dev->use_dma)
> + dmaengine_synchronize(dma->chan_using);
> +
> /*
> * It is possible that some unsent data have already been
> * written into TXDR. To avoid sending old data in a
> @@ -1774,7 +1780,7 @@ static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> if (!timeout) {
> dev_dbg(dev, "Access to slave 0x%x timed out\n", f7_msg->addr);
> if (i2c_dev->use_dma)
> - dmaengine_terminate_all(dma->chan_using);
> + dmaengine_terminate_sync(dma->chan_using);
> stm32f7_i2c_wait_free_bus(i2c_dev);
> ret = -ETIMEDOUT;
> goto pm_free;
>
--
--
~ Py MORDRET
--
Hi Wolfram
On Mon, Nov 29, 2021 at 01:17:21PM +0100, Wolfram Sang wrote:
>
> > + stm32f7_i2c_wait_free_bus(i2c_dev);
>
> This does only a controller reset, not a bus recovery with 9 toggling
> pulses, or?
indeed. I might better rework this and at the same time introduce the
bus recovery mechanism via the bus recovery callback in this driver.
Please don't merge this patch and I will rework that.
Hi Alain,
> > > + stm32f7_i2c_wait_free_bus(i2c_dev);
> >
> > This does only a controller reset, not a bus recovery with 9 toggling
> > pulses, or?
>
> indeed. I might better rework this and at the same time introduce the
> bus recovery mechanism via the bus recovery callback in this driver.
> Please don't merge this patch and I will rework that.
Wait a sec. Resetting a controller at the end of a failed transfer might
make sense if the controller is otherwise in an confused state.
Full bus recovery (9 pulses) should be done at the beginning of a
transfer when SDA is low, though.
So, I'd actually suggest to apply this patch and add full bus recovery
based on SDA low at the beginning of a transfer seperately.
What do you think?
All the best,
Wolfram
> + stm32f7_i2c_wait_free_bus(i2c_dev);
This does only a controller reset, not a bus recovery with 9 toggling
pulses, or?
Hi Alain,
On Mon, Nov 29, 2021 at 01:52:30PM +0100, Wolfram Sang wrote:
> Hi Alain,
>
> > > > + stm32f7_i2c_wait_free_bus(i2c_dev);
> > >
> > > This does only a controller reset, not a bus recovery with 9 toggling
> > > pulses, or?
> >
> > indeed. I might better rework this and at the same time introduce the
> > bus recovery mechanism via the bus recovery callback in this driver.
> > Please don't merge this patch and I will rework that.
>
> Wait a sec. Resetting a controller at the end of a failed transfer might
> make sense if the controller is otherwise in an confused state.
>
> Full bus recovery (9 pulses) should be done at the beginning of a
> transfer when SDA is low, though.
>
> So, I'd actually suggest to apply this patch and add full bus recovery
> based on SDA low at the beginning of a transfer seperately.
>
> What doo you think?
I just checked again. Indeed, this patch is here to handle cases when
communication went bad with a device leading to controller being left in
a confused state. This is done to put it back in a working state.
I agree with you on the fact to decouple this with the 9 pulses bus
recovery and first apply this one first.
Thanks.
Alain
>
> All the best,
>
> Wolfram
>
> I agree with you on the fact to decouple this with the 9 pulses bus
> recovery and first apply this one first.
Good. I noticed something in your driver on the way. Will send an RFC in
some minutes.
On Mon, Sep 20, 2021 at 05:21:30PM +0200, Alain Volmat wrote:
> When getting an access timeout, ensure that the bus is in a proper
> state prior to returning the error.
>
> Fixes: aeb068c57214 ("i2c: i2c-stm32f7: add driver")
>
> Signed-off-by: Alain Volmat <[email protected]>
Applied to for-current, thanks!
On Mon, Sep 20, 2021 at 05:21:32PM +0200, Alain Volmat wrote:
> dmaengine_terminate_all() is deprecated in favor of explicitly saying if
> it should be sync or async. Here, we use dmaengine_terminate_sync in
> i2c_xfer and i2c_smbus_xfer handlers and rely on
> dmaengine_terminate_async within interrupt handlers
> (transmission error cases).
> dmaengine_synchronize is added within i2c_xfer and i2c_smbus_xfer handler
> to finalize terminate started in interrupt handlers.
>
> Signed-off-by: Alain Volmat <[email protected]>
Applied to for-current, thanks!