Skip to content

Commit 1777cbd

Browse files
rh-jkangasrh-jkangas
committed
i2c: imx: prevent rescheduling in non dma mode
JIRA: https://issues.redhat.com/browse/RHEL-116101 commit 5f5c2d4 Author: Stefan Eichenberger <stefan.eichenberger@toradex.com> Date: Mon Oct 14 15:15:14 2024 +0200 i2c: imx: prevent rescheduling in non dma mode We are experiencing a problem with the i.MX I2C controller when communicating with SMBus devices. We are seeing devices time-out because the time between sending/receiving two bytes is too long, and the SMBus device returns to the idle state. This happens because the i.MX I2C controller sends and receives byte by byte. When a byte is sent or received, we get an interrupt and can send or receive the next byte. The current implementation sends a byte and then waits for an event generated by the interrupt subroutine. After the event is received, the next byte is sent and we wait again. This waiting allows the scheduler to reschedule other tasks, with the disadvantage that we may not send the next byte for a long time because the send task is not immediately scheduled. For example, if the rescheduling takes more than 25ms, this can cause SMBus devices to timeout and communication to fail. This patch changes the behavior so that we do not reschedule the send/receive task, but instead send or receive the next byte in the interrupt subroutine. This prevents rescheduling and drastically reduces the time between sending/receiving bytes. The cost in the interrupt subroutine is relatively small, we check what state we are in and then send/receive the next byte. Before we had to call wake_up, which is even less expensive. However, we also had to do some scheduling, which increased the overall cost compared to the new solution. The wake_up function to wake up the send/receive task is now only called when an error occurs or when the transfer is complete. Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Signed-off-by: Andi Shyti <andi.shyti@kernel.org> Signed-off-by: Jared Kangas <jkangas@redhat.com>
1 parent c10cfa9 commit 1777cbd

File tree

1 file changed

+249
-23
lines changed

1 file changed

+249
-23
lines changed

‎drivers/i2c/busses/i2c-imx.c‎

Lines changed: 249 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,17 @@ struct imx_i2c_dma {
197197
enum dma_data_direction dma_data_dir;
198198
};
199199

200+
enum imx_i2c_state {
201+
IMX_I2C_STATE_DONE,
202+
IMX_I2C_STATE_FAILED,
203+
IMX_I2C_STATE_WRITE,
204+
IMX_I2C_STATE_DMA,
205+
IMX_I2C_STATE_READ,
206+
IMX_I2C_STATE_READ_CONTINUE,
207+
IMX_I2C_STATE_READ_BLOCK_DATA,
208+
IMX_I2C_STATE_READ_BLOCK_DATA_LEN,
209+
};
210+
200211
struct imx_i2c_struct {
201212
struct i2c_adapter adapter;
202213
struct clk *clk;
@@ -216,6 +227,12 @@ struct imx_i2c_struct {
216227
struct i2c_client *slave;
217228
enum i2c_slave_event last_slave_event;
218229

230+
struct i2c_msg *msg;
231+
unsigned int msg_buf_idx;
232+
int isr_result;
233+
bool is_lastmsg;
234+
enum imx_i2c_state state;
235+
219236
bool multi_master;
220237

221238
/* For checking slave events. */
@@ -908,11 +925,156 @@ static int i2c_imx_unreg_slave(struct i2c_client *client)
908925
return ret;
909926
}
910927

928+
static inline int i2c_imx_isr_acked(struct imx_i2c_struct *i2c_imx)
929+
{
930+
i2c_imx->isr_result = 0;
931+
932+
if (imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR) & I2SR_RXAK) {
933+
i2c_imx->state = IMX_I2C_STATE_FAILED;
934+
i2c_imx->isr_result = -ENXIO;
935+
wake_up(&i2c_imx->queue);
936+
}
937+
938+
return i2c_imx->isr_result;
939+
}
940+
941+
static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
942+
{
943+
int result;
944+
945+
result = i2c_imx_isr_acked(i2c_imx);
946+
if (result)
947+
return result;
948+
949+
if (i2c_imx->msg->len == i2c_imx->msg_buf_idx)
950+
return 0;
951+
952+
imx_i2c_write_reg(i2c_imx->msg->buf[i2c_imx->msg_buf_idx++], i2c_imx, IMX_I2C_I2DR);
953+
954+
return 1;
955+
}
956+
957+
static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
958+
{
959+
int result;
960+
unsigned int temp;
961+
962+
result = i2c_imx_isr_acked(i2c_imx);
963+
if (result)
964+
return result;
965+
966+
/* setup bus to read data */
967+
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
968+
temp &= ~I2CR_MTX;
969+
if (i2c_imx->msg->len - 1)
970+
temp &= ~I2CR_TXAK;
971+
972+
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
973+
imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
974+
975+
return 0;
976+
}
977+
978+
static inline void i2c_imx_isr_read_continue(struct imx_i2c_struct *i2c_imx)
979+
{
980+
unsigned int temp;
981+
982+
if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
983+
if (i2c_imx->is_lastmsg) {
984+
/*
985+
* It must generate STOP before read I2DR to prevent
986+
* controller from generating another clock cycle
987+
*/
988+
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
989+
if (!(temp & I2CR_MSTA))
990+
i2c_imx->stopped = 1;
991+
temp &= ~(I2CR_MSTA | I2CR_MTX);
992+
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
993+
} else {
994+
/*
995+
* For i2c master receiver repeat restart operation like:
996+
* read -> repeat MSTA -> read/write
997+
* The controller must set MTX before read the last byte in
998+
* the first read operation, otherwise the first read cost
999+
* one extra clock cycle.
1000+
*/
1001+
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
1002+
temp |= I2CR_MTX;
1003+
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
1004+
}
1005+
} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
1006+
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
1007+
temp |= I2CR_TXAK;
1008+
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
1009+
}
1010+
1011+
i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
1012+
}
1013+
1014+
static inline void i2c_imx_isr_read_block_data_len(struct imx_i2c_struct *i2c_imx)
1015+
{
1016+
u8 len = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
1017+
1018+
if (len == 0 || len > I2C_SMBUS_BLOCK_MAX) {
1019+
i2c_imx->isr_result = -EPROTO;
1020+
i2c_imx->state = IMX_I2C_STATE_FAILED;
1021+
wake_up(&i2c_imx->queue);
1022+
}
1023+
i2c_imx->msg->len += len;
1024+
}
1025+
9111026
static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned int status)
9121027
{
913-
/* save status register */
914-
i2c_imx->i2csr = status;
915-
wake_up(&i2c_imx->queue);
1028+
/*
1029+
* This state machine handles I2C reception and transmission in non-DMA
1030+
* mode. We must process all the data in the ISR to reduce the delay
1031+
* between two consecutive messages. If the data is not processed in
1032+
* the ISR, SMBus devices may timeout, leading to a bus error.
1033+
*/
1034+
switch (i2c_imx->state) {
1035+
case IMX_I2C_STATE_DMA:
1036+
i2c_imx->i2csr = status;
1037+
wake_up(&i2c_imx->queue);
1038+
break;
1039+
1040+
case IMX_I2C_STATE_READ:
1041+
if (i2c_imx_isr_read(i2c_imx))
1042+
break;
1043+
i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
1044+
break;
1045+
1046+
case IMX_I2C_STATE_READ_CONTINUE:
1047+
i2c_imx_isr_read_continue(i2c_imx);
1048+
if (i2c_imx->msg_buf_idx == i2c_imx->msg->len) {
1049+
i2c_imx->state = IMX_I2C_STATE_DONE;
1050+
wake_up(&i2c_imx->queue);
1051+
}
1052+
break;
1053+
1054+
case IMX_I2C_STATE_READ_BLOCK_DATA:
1055+
if (i2c_imx_isr_read(i2c_imx))
1056+
break;
1057+
i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA_LEN;
1058+
break;
1059+
1060+
case IMX_I2C_STATE_READ_BLOCK_DATA_LEN:
1061+
i2c_imx_isr_read_block_data_len(i2c_imx);
1062+
i2c_imx->state = IMX_I2C_STATE_READ_CONTINUE;
1063+
break;
1064+
1065+
case IMX_I2C_STATE_WRITE:
1066+
if (i2c_imx_isr_write(i2c_imx))
1067+
break;
1068+
i2c_imx->state = IMX_I2C_STATE_DONE;
1069+
wake_up(&i2c_imx->queue);
1070+
break;
1071+
1072+
default:
1073+
i2c_imx->i2csr = status;
1074+
i2c_imx->state = IMX_I2C_STATE_FAILED;
1075+
i2c_imx->isr_result = -EINVAL;
1076+
wake_up(&i2c_imx->queue);
1077+
}
9161078

9171079
return IRQ_HANDLED;
9181080
}
@@ -959,6 +1121,8 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
9591121
struct imx_i2c_dma *dma = i2c_imx->dma;
9601122
struct device *dev = &i2c_imx->adapter.dev;
9611123

1124+
i2c_imx->state = IMX_I2C_STATE_DMA;
1125+
9621126
dma->chan_using = dma->chan_tx;
9631127
dma->dma_transfer_dir = DMA_MEM_TO_DEV;
9641128
dma->dma_data_dir = DMA_TO_DEVICE;
@@ -1012,15 +1176,14 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
10121176
}
10131177

10141178
static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
1015-
struct i2c_msg *msgs, bool atomic,
1016-
bool use_dma)
1179+
struct i2c_msg *msgs, bool use_dma)
10171180
{
10181181
int result;
10191182
unsigned int temp = 0;
10201183

10211184
/* write slave address */
10221185
imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
1023-
result = i2c_imx_trx_complete(i2c_imx, atomic);
1186+
result = i2c_imx_trx_complete(i2c_imx, !use_dma);
10241187
if (result)
10251188
return result;
10261189
result = i2c_imx_acked(i2c_imx);
@@ -1058,7 +1221,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
10581221
struct imx_i2c_dma *dma = i2c_imx->dma;
10591222
struct device *dev = &i2c_imx->adapter.dev;
10601223

1061-
result = i2c_imx_prepare_read(i2c_imx, msgs, false, true);
1224+
i2c_imx->state = IMX_I2C_STATE_DMA;
1225+
1226+
result = i2c_imx_prepare_read(i2c_imx, msgs, true);
10621227
if (result)
10631228
return result;
10641229

@@ -1139,8 +1304,8 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
11391304
return 0;
11401305
}
11411306

1142-
static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
1143-
bool atomic)
1307+
static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
1308+
struct i2c_msg *msgs)
11441309
{
11451310
int i, result;
11461311

@@ -1149,7 +1314,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
11491314

11501315
/* write slave address */
11511316
imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
1152-
result = i2c_imx_trx_complete(i2c_imx, atomic);
1317+
result = i2c_imx_trx_complete(i2c_imx, true);
11531318
if (result)
11541319
return result;
11551320
result = i2c_imx_acked(i2c_imx);
@@ -1163,7 +1328,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
11631328
"<%s> write byte: B%d=0x%X\n",
11641329
__func__, i, msgs->buf[i]);
11651330
imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
1166-
result = i2c_imx_trx_complete(i2c_imx, atomic);
1331+
result = i2c_imx_trx_complete(i2c_imx, true);
11671332
if (result)
11681333
return result;
11691334
result = i2c_imx_acked(i2c_imx);
@@ -1173,19 +1338,44 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
11731338
return 0;
11741339
}
11751340

1176-
static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
1341+
static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
11771342
{
1178-
return i2c_imx_write(i2c_imx, msgs, true);
1343+
dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
1344+
__func__, i2c_8bit_addr_from_msg(msgs));
1345+
1346+
i2c_imx->state = IMX_I2C_STATE_WRITE;
1347+
i2c_imx->msg = msgs;
1348+
i2c_imx->msg_buf_idx = 0;
1349+
1350+
/*
1351+
* By writing the device address we start the state machine in the ISR.
1352+
* The ISR will report when it is done or when it fails.
1353+
*/
1354+
imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
1355+
wait_event_timeout(i2c_imx->queue,
1356+
i2c_imx->state == IMX_I2C_STATE_DONE ||
1357+
i2c_imx->state == IMX_I2C_STATE_FAILED,
1358+
(msgs->len + 1) * HZ / 10);
1359+
if (i2c_imx->state == IMX_I2C_STATE_FAILED) {
1360+
dev_dbg(&i2c_imx->adapter.dev, "<%s> write failed with %d\n",
1361+
__func__, i2c_imx->isr_result);
1362+
return i2c_imx->isr_result;
1363+
}
1364+
if (i2c_imx->state != IMX_I2C_STATE_DONE) {
1365+
dev_err(&i2c_imx->adapter.dev, "<%s> write timedout\n", __func__);
1366+
return -ETIMEDOUT;
1367+
}
1368+
return 0;
11791369
}
11801370

1181-
static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
1182-
bool is_lastmsg, bool atomic)
1371+
static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx,
1372+
struct i2c_msg *msgs, bool is_lastmsg)
11831373
{
11841374
int i, result;
11851375
unsigned int temp;
11861376
int block_data = msgs->flags & I2C_M_RECV_LEN;
11871377

1188-
result = i2c_imx_prepare_read(i2c_imx, msgs, atomic, false);
1378+
result = i2c_imx_prepare_read(i2c_imx, msgs, false);
11891379
if (result)
11901380
return result;
11911381

@@ -1195,7 +1385,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
11951385
for (i = 0; i < msgs->len; i++) {
11961386
u8 len = 0;
11971387

1198-
result = i2c_imx_trx_complete(i2c_imx, atomic);
1388+
result = i2c_imx_trx_complete(i2c_imx, true);
11991389
if (result)
12001390
return result;
12011391
/*
@@ -1226,7 +1416,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
12261416
temp &= ~(I2CR_MSTA | I2CR_MTX);
12271417
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
12281418
if (!i2c_imx->stopped)
1229-
i2c_imx_bus_busy(i2c_imx, 0, atomic);
1419+
i2c_imx_bus_busy(i2c_imx, 0, true);
12301420
} else {
12311421
/*
12321422
* For i2c master receiver repeat restart operation like:
@@ -1257,10 +1447,46 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
12571447
return 0;
12581448
}
12591449

1260-
static int i2c_imx_atomic_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
1261-
bool is_lastmsg)
1450+
static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
1451+
bool is_lastmsg)
12621452
{
1263-
return i2c_imx_read(i2c_imx, msgs, is_lastmsg, true);
1453+
int block_data = msgs->flags & I2C_M_RECV_LEN;
1454+
1455+
dev_dbg(&i2c_imx->adapter.dev,
1456+
"<%s> write slave address: addr=0x%x\n",
1457+
__func__, i2c_8bit_addr_from_msg(msgs));
1458+
1459+
i2c_imx->is_lastmsg = is_lastmsg;
1460+
1461+
if (block_data)
1462+
i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
1463+
else
1464+
i2c_imx->state = IMX_I2C_STATE_READ;
1465+
i2c_imx->msg = msgs;
1466+
i2c_imx->msg_buf_idx = 0;
1467+
1468+
/*
1469+
* By writing the device address we start the state machine in the ISR.
1470+
* The ISR will report when it is done or when it fails.
1471+
*/
1472+
imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
1473+
wait_event_timeout(i2c_imx->queue,
1474+
i2c_imx->state == IMX_I2C_STATE_DONE ||
1475+
i2c_imx->state == IMX_I2C_STATE_FAILED,
1476+
(msgs->len + 1) * HZ / 10);
1477+
if (i2c_imx->state == IMX_I2C_STATE_FAILED) {
1478+
dev_dbg(&i2c_imx->adapter.dev, "<%s> read failed with %d\n",
1479+
__func__, i2c_imx->isr_result);
1480+
return i2c_imx->isr_result;
1481+
}
1482+
if (i2c_imx->state != IMX_I2C_STATE_DONE) {
1483+
dev_err(&i2c_imx->adapter.dev, "<%s> read timedout\n", __func__);
1484+
return -ETIMEDOUT;
1485+
}
1486+
if (!i2c_imx->stopped)
1487+
return i2c_imx_bus_busy(i2c_imx, 0, false);
1488+
1489+
return 0;
12641490
}
12651491

12661492
static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
@@ -1334,14 +1560,14 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
13341560
else if (use_dma && !block_data)
13351561
result = i2c_imx_dma_read(i2c_imx, &msgs[i], is_lastmsg);
13361562
else
1337-
result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg, false);
1563+
result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
13381564
} else {
13391565
if (atomic)
13401566
result = i2c_imx_atomic_write(i2c_imx, &msgs[i]);
13411567
else if (use_dma)
13421568
result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
13431569
else
1344-
result = i2c_imx_write(i2c_imx, &msgs[i], false);
1570+
result = i2c_imx_write(i2c_imx, &msgs[i]);
13451571
}
13461572
if (result)
13471573
goto fail0;

0 commit comments

Comments
 (0)