Add an atomic_xfer method to the driver so that it behaves correctly
when controlling a PMIC that is responsible for device shutdown.
The atomic_xfer method added is similar to the one from the i2c-mv64xxx
driver. When running an atomic_xfer a bool flag in the driver data is
set, the interrupt is not unmasked on transfer start, and the IRQ
handler is manually invoked while waiting for pending transfers to
complete.
Signed-off-by: Sean Nyekjaer <[email protected]>
---
Tested on a STM32MP1 with:
https://lore.kernel.org/all/[email protected]/
Is it okay to keep the DMA transfer in atomic?
I'm annoyed by the return 1 in stm32f7_i2c_wait_polling() is there any
good idea to fix that?
drivers/i2c/busses/i2c-stm32f7.c | 73 ++++++++++++++++++++++++++------
1 file changed, 60 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index d1c59d83a65b..b63e8a7eb1aa 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -357,6 +357,7 @@ struct stm32f7_i2c_dev {
u32 dnf_dt;
u32 dnf;
struct stm32f7_i2c_alert *alert;
+ bool atomic;
};
/*
@@ -905,13 +906,18 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
cr2 |= STM32F7_I2C_CR2_NBYTES(f7_msg->count);
}
- /* Enable NACK, STOP, error and transfer complete interrupts */
- cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
- STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
+ if (!i2c_dev->atomic) {
+ /* Enable NACK, STOP, error and transfer complete interrupts */
+ cr1 |= STM32F7_I2C_CR1_ERRIE | STM32F7_I2C_CR1_TCIE |
+ STM32F7_I2C_CR1_STOPIE | STM32F7_I2C_CR1_NACKIE;
- /* Clear DMA req and TX/RX interrupt */
- cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
- STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
+ /* Clear DMA req and TX/RX interrupt */
+ cr1 &= ~(STM32F7_I2C_CR1_RXIE | STM32F7_I2C_CR1_TXIE |
+ STM32F7_I2C_CR1_RXDMAEN | STM32F7_I2C_CR1_TXDMAEN);
+ } else {
+ /* Disable interrupts */
+ cr1 &= ~STM32F7_I2C_ALL_IRQ_MASK;
+ }
/* Configure DMA or enable RX/TX interrupt */
i2c_dev->use_dma = false;
@@ -928,10 +934,12 @@ static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
}
if (!i2c_dev->use_dma) {
- if (msg->flags & I2C_M_RD)
- cr1 |= STM32F7_I2C_CR1_RXIE;
- else
- cr1 |= STM32F7_I2C_CR1_TXIE;
+ if (!i2c_dev->atomic) {
+ if (msg->flags & I2C_M_RD)
+ cr1 |= STM32F7_I2C_CR1_RXIE;
+ else
+ cr1 |= STM32F7_I2C_CR1_TXIE;
+ }
} else {
if (msg->flags & I2C_M_RD)
cr1 |= STM32F7_I2C_CR1_RXDMAEN;
@@ -1670,7 +1678,22 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data)
return IRQ_HANDLED;
}
-static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
+static int stm32f7_i2c_wait_polling(struct stm32f7_i2c_dev *i2c_dev)
+{
+ ktime_t timeout = ktime_add_ms(ktime_get(), i2c_dev->adap.timeout);
+
+ while (ktime_compare(ktime_get(), timeout) < 0) {
+ udelay(5);
+ stm32f7_i2c_isr_event(0, i2c_dev);
+
+ if (try_wait_for_completion(&i2c_dev->complete))
+ return 1;
+ }
+
+ return 0;
+}
+
+static int stm32f7_i2c_xfer_core(struct i2c_adapter *i2c_adap,
struct i2c_msg msgs[], int num)
{
struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
@@ -1694,8 +1717,13 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
stm32f7_i2c_xfer_msg(i2c_dev, msgs);
- time_left = wait_for_completion_timeout(&i2c_dev->complete,
- i2c_dev->adap.timeout);
+ if (!i2c_dev->atomic) {
+ time_left = wait_for_completion_timeout(&i2c_dev->complete,
+ i2c_dev->adap.timeout);
+ } else {
+ time_left = stm32f7_i2c_wait_polling(i2c_dev);
+ }
+
ret = f7_msg->result;
if (ret) {
if (i2c_dev->use_dma)
@@ -1727,6 +1755,24 @@ static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
return (ret < 0) ? ret : num;
}
+static int stm32f7_i2c_xfer(struct i2c_adapter *i2c_adap,
+ struct i2c_msg msgs[], int num)
+{
+ struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+ i2c_dev->atomic = 0;
+ return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
+static int stm32f7_i2c_xfer_atomic(struct i2c_adapter *i2c_adap,
+ struct i2c_msg msgs[], int num)
+{
+ struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
+
+ i2c_dev->atomic = 1;
+ return stm32f7_i2c_xfer_core(i2c_adap, msgs, num);
+}
+
static int stm32f7_i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
unsigned short flags, char read_write,
u8 command, int size,
@@ -2095,6 +2141,7 @@ static u32 stm32f7_i2c_func(struct i2c_adapter *adap)
static const struct i2c_algorithm stm32f7_i2c_algo = {
.master_xfer = stm32f7_i2c_xfer,
+ .master_xfer_atomic = stm32f7_i2c_xfer_atomic,
.smbus_xfer = stm32f7_i2c_smbus_xfer,
.functionality = stm32f7_i2c_func,
.reg_slave = stm32f7_i2c_reg_slave,
--
2.40.0
On Tue, May 09, 2023 at 03:21:59PM +0200, Sean Nyekjaer wrote:
> Add an atomic_xfer method to the driver so that it behaves correctly
> when controlling a PMIC that is responsible for device shutdown.
>
> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
> driver. When running an atomic_xfer a bool flag in the driver data is
> set, the interrupt is not unmasked on transfer start, and the IRQ
> handler is manually invoked while waiting for pending transfers to
> complete.
>
> Signed-off-by: Sean Nyekjaer <[email protected]>
Pierre-Yves, Alain, any further comments to this patch?
> Is it okay to keep the DMA transfer in atomic?
Will DMA actually run in atomic mode?
> On 23 Jun 2023, at 12.33, Wolfram Sang <[email protected]> wrote:
>
> On Tue, May 09, 2023 at 03:21:59PM +0200, Sean Nyekjaer wrote:
>> Add an atomic_xfer method to the driver so that it behaves correctly
>> when controlling a PMIC that is responsible for device shutdown.
>>
>> The atomic_xfer method added is similar to the one from the i2c-mv64xxx
>> driver. When running an atomic_xfer a bool flag in the driver data is
>> set, the interrupt is not unmasked on transfer start, and the IRQ
>> handler is manually invoked while waiting for pending transfers to
>> complete.
>>
>> Signed-off-by: Sean Nyekjaer <[email protected]>
>
> Pierre-Yves, Alain, any further comments to this patch?
>
>> Is it okay to keep the DMA transfer in atomic?
>
> Will DMA actually run in atomic mode?
>
Hi Wolfram,
Atomic is mainly(only) used for writing a single register in the PMIC for the stpmic. Guess that will not trigger any DMA use.
But let’s wait for other comments…
/Sean
> > Will DMA actually run in atomic mode?
> Atomic is mainly(only) used for writing a single register in the PMIC
> for the stpmic.
And this most probably during shutdown...
> Guess that will not trigger any DMA use.
... so I'd be very surprised if DMA is operational that late. I think we
can rule that out independent of I2C messages to be trasnferred.
> On 3 Jul 2023, at 11.50, Wolfram Sang <[email protected]> wrote:
>
>
>>> Will DMA actually run in atomic mode?
>
>> Atomic is mainly(only) used for writing a single register in the PMIC
>> for the stpmic.
>
> And this most probably during shutdown...
>
>> Guess that will not trigger any DMA use.
>
> ... so I'd be very surprised if DMA is operational that late. I think we
> can rule that out independent of I2C messages to be trasnferred.
>
Yes, I’ll submit a V2 without the DMA functionality.
/Sean