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 <l...@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
.config.gz
Description: application/gzip
