* [PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down
@ 2019-09-25 12:59 Codrin Ciubotariu
2019-09-25 15:26 ` kbuild test robot
0 siblings, 1 reply; 2+ messages in thread
From: Codrin Ciubotariu @ 2019-09-25 12:59 UTC (permalink / raw)
To: linux-i2c, linux-arm-kernel, linux-kernel
Cc: ludovic.desroches, nicolas.ferre, alexandre.belloni, wsa,
Claudiu.Beznea, Codrin Ciubotariu
After a transfer timeout, some faulty I2C slave devices might hold down
the SCL or the SDA pins. We can generate a bus clear command, hoping that
the slave might release the pins.
Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
Changes in v2:
- added '.has_clear_cmd' struct member to specify which IPs support the
clear command; for now, only SAMA5D2 supports it;
- added Ludovic's V1 ack since there were no major changes;
drivers/i2c/busses/i2c-at91-core.c | 8 ++++++++
drivers/i2c/busses/i2c-at91-master.c | 21 +++++++++++++++++++++
drivers/i2c/busses/i2c-at91.h | 7 ++++++-
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c
index 435c7d7377a3..cb07489e698f 100644
--- a/drivers/i2c/busses/i2c-at91-core.c
+++ b/drivers/i2c/busses/i2c-at91-core.c
@@ -68,6 +68,7 @@ static struct at91_twi_pdata at91rm9200_config = {
.has_unre_flag = true,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata at91sam9261_config = {
@@ -76,6 +77,7 @@ static struct at91_twi_pdata at91sam9261_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata at91sam9260_config = {
@@ -84,6 +86,7 @@ static struct at91_twi_pdata at91sam9260_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata at91sam9g20_config = {
@@ -92,6 +95,7 @@ static struct at91_twi_pdata at91sam9g20_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata at91sam9g10_config = {
@@ -100,6 +104,7 @@ static struct at91_twi_pdata at91sam9g10_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static const struct platform_device_id at91_twi_devtypes[] = {
@@ -130,6 +135,7 @@ static struct at91_twi_pdata at91sam9x5_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = false,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata sama5d4_config = {
@@ -138,6 +144,7 @@ static struct at91_twi_pdata sama5d4_config = {
.has_unre_flag = false,
.has_alt_cmd = false,
.has_hold_field = true,
+ .has_clear_cmd = false,
};
static struct at91_twi_pdata sama5d2_config = {
@@ -146,6 +153,7 @@ static struct at91_twi_pdata sama5d2_config = {
.has_unre_flag = true,
.has_alt_cmd = true,
.has_hold_field = true,
+ .has_clear_cmd = true,
};
static const struct of_device_id atmel_twi_dt_ids[] = {
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index a3fcc35ffd3b..8082dff77724 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -440,6 +440,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
unsigned long time_left;
bool has_unre_flag = dev->pdata->has_unre_flag;
bool has_alt_cmd = dev->pdata->has_alt_cmd;
+ bool has_clear_cmd = dev->pdata->has_clear_cmd;
/*
* WARNING: the TXCOMP bit in the Status Register is NOT a clear on
@@ -599,6 +600,26 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
at91_twi_write(dev, AT91_TWI_CR,
AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
}
+
+ /*
+ * After timeout, some faulty I2C slave devices might hold SCL/SDA down;
+ * we can send a bus clear command, hoping that the pins will be
+ * released
+ */
+ if (has_clear_cmd && !(dev->transfer_status & AT91_TWI_SDA) ||
+ !(dev->transfer_status & AT91_TWI_SCL)) {
+ dev_dbg(dev->dev,
+ "SDA/SCL are down; sending bus clear command\n");
+ if (dev->use_alt_cmd) {
+ unsigned int acr;
+
+ acr = at91_twi_read(dev, AT91_TWI_ACR);
+ acr &= ~AT91_TWI_ACR_DATAL_MASK;
+ at91_twi_write(dev, AT91_TWI_ACR, acr);
+ }
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);
+ }
+
return ret;
}
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 499b506f6128..0827c28a84db 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -36,6 +36,7 @@
#define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
#define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
#define AT91_TWI_SWRST BIT(7) /* Software Reset */
+#define AT91_TWI_CLEAR BIT(15) /* Bus clear command */
#define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */
#define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */
#define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
@@ -69,6 +70,8 @@
#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
#define AT91_TWI_EOSACC BIT(11) /* End Of Slave Access */
#define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
+#define AT91_TWI_SCL BIT(24) /* TWI SCL status */
+#define AT91_TWI_SDA BIT(25) /* TWI SDA status */
#define AT91_TWI_INT_MASK \
(AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK \
@@ -81,7 +84,8 @@
#define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
#define AT91_TWI_ACR 0x0040 /* Alternative Command Register */
-#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
+#define AT91_TWI_ACR_DATAL_MASK GENMASK(15, 0)
+#define AT91_TWI_ACR_DATAL(len) ((len) & AT91_TWI_ACR_DATAL_MASK)
#define AT91_TWI_ACR_DIR BIT(8)
#define AT91_TWI_FMR 0x0050 /* FIFO Mode Register */
@@ -108,6 +112,7 @@ struct at91_twi_pdata {
bool has_unre_flag;
bool has_alt_cmd;
bool has_hold_field;
+ bool has_clear_cmd;
struct at_dma_slave dma_slave;
};
--
2.20.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down
2019-09-25 12:59 [PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down Codrin Ciubotariu
@ 2019-09-25 15:26 ` kbuild test robot
0 siblings, 0 replies; 2+ messages in thread
From: kbuild test robot @ 2019-09-25 15:26 UTC (permalink / raw)
To: Codrin Ciubotariu
Cc: kbuild-all, linux-i2c, linux-arm-kernel, linux-kernel,
ludovic.desroches, nicolas.ferre, alexandre.belloni, wsa,
Claudiu.Beznea, Codrin Ciubotariu
[-- Attachment #1: Type: text/plain, Size: 9921 bytes --]
Hi Codrin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Codrin-Ciubotariu/i2c-at91-Send-bus-clear-command-if-SCL-or-SDA-is-down/20190925-215623
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/i2c/busses/i2c-at91-master.c: In function 'at91_do_twi_transfer':
>> drivers/i2c/busses/i2c-at91-master.c:609:20: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
if (has_clear_cmd && !(dev->transfer_status & AT91_TWI_SDA) ||
~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +609 drivers/i2c/busses/i2c-at91-master.c
436
437 static int at91_do_twi_transfer(struct at91_twi_dev *dev)
438 {
439 int ret;
440 unsigned long time_left;
441 bool has_unre_flag = dev->pdata->has_unre_flag;
442 bool has_alt_cmd = dev->pdata->has_alt_cmd;
443 bool has_clear_cmd = dev->pdata->has_clear_cmd;
444
445 /*
446 * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
447 * read flag but shows the state of the transmission at the time the
448 * Status Register is read. According to the programmer datasheet,
449 * TXCOMP is set when both holding register and internal shifter are
450 * empty and STOP condition has been sent.
451 * Consequently, we should enable NACK interrupt rather than TXCOMP to
452 * detect transmission failure.
453 * Indeed let's take the case of an i2c write command using DMA.
454 * Whenever the slave doesn't acknowledge a byte, the LOCK, NACK and
455 * TXCOMP bits are set together into the Status Register.
456 * LOCK is a clear on write bit, which is set to prevent the DMA
457 * controller from sending new data on the i2c bus after a NACK
458 * condition has happened. Once locked, this i2c peripheral stops
459 * triggering the DMA controller for new data but it is more than
460 * likely that a new DMA transaction is already in progress, writing
461 * into the Transmit Holding Register. Since the peripheral is locked,
462 * these new data won't be sent to the i2c bus but they will remain
463 * into the Transmit Holding Register, so TXCOMP bit is cleared.
464 * Then when the interrupt handler is called, the Status Register is
465 * read: the TXCOMP bit is clear but NACK bit is still set. The driver
466 * manage the error properly, without waiting for timeout.
467 * This case can be reproduced easyly when writing into an at24 eeprom.
468 *
469 * Besides, the TXCOMP bit is already set before the i2c transaction
470 * has been started. For read transactions, this bit is cleared when
471 * writing the START bit into the Control Register. So the
472 * corresponding interrupt can safely be enabled just after.
473 * However for write transactions managed by the CPU, we first write
474 * into THR, so TXCOMP is cleared. Then we can safely enable TXCOMP
475 * interrupt. If TXCOMP interrupt were enabled before writing into THR,
476 * the interrupt handler would be called immediately and the i2c command
477 * would be reported as completed.
478 * Also when a write transaction is managed by the DMA controller,
479 * enabling the TXCOMP interrupt in this function may lead to a race
480 * condition since we don't know whether the TXCOMP interrupt is enabled
481 * before or after the DMA has started to write into THR. So the TXCOMP
482 * interrupt is enabled later by at91_twi_write_data_dma_callback().
483 * Immediately after in that DMA callback, if the alternative command
484 * mode is not used, we still need to send the STOP condition manually
485 * writing the corresponding bit into the Control Register.
486 */
487
488 dev_dbg(dev->dev, "transfer: %s %zu bytes.\n",
489 (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
490
491 reinit_completion(&dev->cmd_complete);
492 dev->transfer_status = 0;
493
494 /* Clear pending interrupts, such as NACK. */
495 at91_twi_read(dev, AT91_TWI_SR);
496
497 if (dev->fifo_size) {
498 unsigned fifo_mr = at91_twi_read(dev, AT91_TWI_FMR);
499
500 /* Reset FIFO mode register */
501 fifo_mr &= ~(AT91_TWI_FMR_TXRDYM_MASK |
502 AT91_TWI_FMR_RXRDYM_MASK);
503 fifo_mr |= AT91_TWI_FMR_TXRDYM(AT91_TWI_ONE_DATA);
504 fifo_mr |= AT91_TWI_FMR_RXRDYM(AT91_TWI_ONE_DATA);
505 at91_twi_write(dev, AT91_TWI_FMR, fifo_mr);
506
507 /* Flush FIFOs */
508 at91_twi_write(dev, AT91_TWI_CR,
509 AT91_TWI_THRCLR | AT91_TWI_RHRCLR);
510 }
511
512 if (!dev->buf_len) {
513 at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_QUICK);
514 at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
515 } else if (dev->msg->flags & I2C_M_RD) {
516 unsigned start_flags = AT91_TWI_START;
517
518 /* if only one byte is to be read, immediately stop transfer */
519 if (!dev->use_alt_cmd && dev->buf_len <= 1 &&
520 !(dev->msg->flags & I2C_M_RECV_LEN))
521 start_flags |= AT91_TWI_STOP;
522 at91_twi_write(dev, AT91_TWI_CR, start_flags);
523 /*
524 * When using dma without alternative command mode, the last
525 * byte has to be read manually in order to not send the stop
526 * command too late and then to receive extra data.
527 * In practice, there are some issues if you use the dma to
528 * read n-1 bytes because of latency.
529 * Reading n-2 bytes with dma and the two last ones manually
530 * seems to be the best solution.
531 */
532 if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
533 at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
534 at91_twi_read_data_dma(dev);
535 } else {
536 at91_twi_write(dev, AT91_TWI_IER,
537 AT91_TWI_TXCOMP |
538 AT91_TWI_NACK |
539 AT91_TWI_RXRDY);
540 }
541 } else {
542 if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
543 at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
544 at91_twi_write_data_dma(dev);
545 } else {
546 at91_twi_write_next_byte(dev);
547 at91_twi_write(dev, AT91_TWI_IER,
548 AT91_TWI_TXCOMP | AT91_TWI_NACK |
549 (dev->buf_len ? AT91_TWI_TXRDY : 0));
550 }
551 }
552
553 time_left = wait_for_completion_timeout(&dev->cmd_complete,
554 dev->adapter.timeout);
555 if (time_left == 0) {
556 dev->transfer_status |= at91_twi_read(dev, AT91_TWI_SR);
557 dev_err(dev->dev, "controller timed out\n");
558 at91_init_twi_bus(dev);
559 ret = -ETIMEDOUT;
560 goto error;
561 }
562 if (dev->transfer_status & AT91_TWI_NACK) {
563 dev_dbg(dev->dev, "received nack\n");
564 ret = -EREMOTEIO;
565 goto error;
566 }
567 if (dev->transfer_status & AT91_TWI_OVRE) {
568 dev_err(dev->dev, "overrun while reading\n");
569 ret = -EIO;
570 goto error;
571 }
572 if (has_unre_flag && dev->transfer_status & AT91_TWI_UNRE) {
573 dev_err(dev->dev, "underrun while writing\n");
574 ret = -EIO;
575 goto error;
576 }
577 if ((has_alt_cmd || dev->fifo_size) &&
578 (dev->transfer_status & AT91_TWI_LOCK)) {
579 dev_err(dev->dev, "tx locked\n");
580 ret = -EIO;
581 goto error;
582 }
583 if (dev->recv_len_abort) {
584 dev_err(dev->dev, "invalid smbus block length recvd\n");
585 ret = -EPROTO;
586 goto error;
587 }
588
589 dev_dbg(dev->dev, "transfer complete\n");
590
591 return 0;
592
593 error:
594 /* first stop DMA transfer if still in progress */
595 at91_twi_dma_cleanup(dev);
596 /* then flush THR/FIFO and unlock TX if locked */
597 if ((has_alt_cmd || dev->fifo_size) &&
598 (dev->transfer_status & AT91_TWI_LOCK)) {
599 dev_dbg(dev->dev, "unlock tx\n");
600 at91_twi_write(dev, AT91_TWI_CR,
601 AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
602 }
603
604 /*
605 * After timeout, some faulty I2C slave devices might hold SCL/SDA down;
606 * we can send a bus clear command, hoping that the pins will be
607 * released
608 */
> 609 if (has_clear_cmd && !(dev->transfer_status & AT91_TWI_SDA) ||
610 !(dev->transfer_status & AT91_TWI_SCL)) {
611 dev_dbg(dev->dev,
612 "SDA/SCL are down; sending bus clear command\n");
613 if (dev->use_alt_cmd) {
614 unsigned int acr;
615
616 acr = at91_twi_read(dev, AT91_TWI_ACR);
617 acr &= ~AT91_TWI_ACR_DATAL_MASK;
618 at91_twi_write(dev, AT91_TWI_ACR, acr);
619 }
620 at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);
621 }
622
623 return ret;
624 }
625
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71388 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-09-25 15:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 12:59 [PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down Codrin Ciubotariu
2019-09-25 15:26 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).