Re: [PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down
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 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
[PATCH v2] i2c: at91: Send bus clear command if SCL or SDA is down
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 Acked-by: Ludovic Desroches --- 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 @@ #defineAT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */ #defineAT91_TWI_QUICK BIT(6) /* SMBus quick command */ #defineAT91_TWI_SWRST BIT(7) /* Software Reset */ +#defineAT91_TWI_CLEAR