| Message ID | 20231207165027.2628302-2-enachman@marvell.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock | expand |
Hi Elad, kernel test robot noticed the following build warnings: [auto build test WARNING on wsa/i2c/for-next] [also build test WARNING on linus/master v6.7-rc4 next-20231207] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Elad-Nachman/i2c-busses-i2c-mv64xxx-fix-arb-loss-i2c-lock/20231208-005406 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next patch link: https://lore.kernel.org/r/20231207165027.2628302-2-enachman%40marvell.com patch subject: [PATCH v2 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock config: i386-buildonly-randconfig-004-20231208 (https://download.01.org/0day-ci/archive/20231208/202312081143.38GWJJ2J-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231208/202312081143.38GWJJ2J-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312081143.38GWJJ2J-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/i2c/busses/i2c-mv64xxx.c:440:9: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] if (!ret) { ^~~ drivers/i2c/busses/i2c-mv64xxx.c:372:12: note: initialize the variable 'ret' to silence this warning int i, ret; ^ = 0 1 warning generated. vim +/ret +440 drivers/i2c/busses/i2c-mv64xxx.c 367 368 static void 369 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) 370 { 371 struct pinctrl *pc; 372 int i, ret; 373 374 switch(drv_data->action) { 375 case MV64XXX_I2C_ACTION_SEND_RESTART: 376 /* We should only get here if we have further messages */ 377 BUG_ON(drv_data->num_msgs == 0); 378 379 drv_data->msgs++; 380 drv_data->num_msgs--; 381 mv64xxx_i2c_send_start(drv_data); 382 383 if (drv_data->errata_delay) 384 udelay(5); 385 386 /* 387 * We're never at the start of the message here, and by this 388 * time it's already too late to do any protocol mangling. 389 * Thankfully, do not advertise support for that feature. 390 */ 391 drv_data->send_stop = drv_data->num_msgs == 1; 392 break; 393 394 case MV64XXX_I2C_ACTION_CONTINUE: 395 writel(drv_data->cntl_bits, 396 drv_data->reg_base + drv_data->reg_offsets.control); 397 break; 398 399 case MV64XXX_I2C_ACTION_SEND_ADDR_1: 400 writel(drv_data->addr1, 401 drv_data->reg_base + drv_data->reg_offsets.data); 402 writel(drv_data->cntl_bits, 403 drv_data->reg_base + drv_data->reg_offsets.control); 404 break; 405 406 case MV64XXX_I2C_ACTION_SEND_ADDR_2: 407 writel(drv_data->addr2, 408 drv_data->reg_base + drv_data->reg_offsets.data); 409 writel(drv_data->cntl_bits, 410 drv_data->reg_base + drv_data->reg_offsets.control); 411 break; 412 413 case MV64XXX_I2C_ACTION_SEND_DATA: 414 writel(drv_data->msg->buf[drv_data->byte_posn++], 415 drv_data->reg_base + drv_data->reg_offsets.data); 416 writel(drv_data->cntl_bits, 417 drv_data->reg_base + drv_data->reg_offsets.control); 418 break; 419 420 case MV64XXX_I2C_ACTION_RCV_DATA: 421 drv_data->msg->buf[drv_data->byte_posn++] = 422 readl(drv_data->reg_base + drv_data->reg_offsets.data); 423 writel(drv_data->cntl_bits, 424 drv_data->reg_base + drv_data->reg_offsets.control); 425 break; 426 427 case MV64XXX_I2C_ACTION_UNLOCK_BUS: 428 if (!drv_data->soft_reset) 429 break; 430 431 pc = devm_pinctrl_get(drv_data->adapter.dev.parent); 432 if (IS_ERR(pc)) { 433 dev_err(&drv_data->adapter.dev, 434 "failed to get pinctrl for bus unlock!\n"); 435 break; 436 } 437 438 /* Change i2c MPPs state to act as GPIOs: */ 439 if (pinctrl_select_state(pc, drv_data->i2c_gpio_state) >= 0) { > 440 if (!ret) { 441 /* 442 * Toggle i2c scl (serial clock) 10 times. 443 * 10 clocks are enough to transfer a full 444 * byte plus extra as seen from tests with 445 * Ubiquity SFP module causing the issue. 446 * This allows the slave that occupies 447 * the bus to transmit its remaining data, 448 * so it can release the i2c bus: 449 */ 450 for (i = 0; i < 10; i++) { 451 gpio_set_value(drv_data->scl_gpio, 1); 452 udelay(100); 453 gpio_set_value(drv_data->scl_gpio, 0); 454 }; 455 } 456 457 /* restore i2c pin state to MPPs: */ 458 pinctrl_select_state(pc, drv_data->i2c_mpp_state); 459 } 460 461 /* 462 * Trigger controller soft reset 463 * This register is write only, with none of the bits defined. 464 * So any value will do. 465 * 0x1 just seems more intuitive than 0x0 ... 466 */ 467 writel(0x1, drv_data->reg_base + drv_data->reg_offsets.soft_reset); 468 /* wait for i2c controller to complete reset: */ 469 udelay(100); 470 /* 471 * need to proceed to the data stop condition generation clause. 472 * This is needed after clock toggling to put the i2c slave 473 * in the correct state. 474 */ 475 fallthrough; 476 477 case MV64XXX_I2C_ACTION_RCV_DATA_STOP: 478 drv_data->msg->buf[drv_data->byte_posn++] = 479 readl(drv_data->reg_base + drv_data->reg_offsets.data); 480 if (!drv_data->atomic) 481 drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; 482 writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, 483 drv_data->reg_base + drv_data->reg_offsets.control); 484 drv_data->block = 0; 485 if (drv_data->errata_delay) 486 udelay(5); 487 488 wake_up(&drv_data->waitq); 489 break; 490 491 case MV64XXX_I2C_ACTION_INVALID: 492 default: 493 dev_err(&drv_data->adapter.dev, 494 "mv64xxx_i2c_do_action: Invalid action: %d\n", 495 drv_data->action); 496 drv_data->rc = -EIO; 497 fallthrough; 498 case MV64XXX_I2C_ACTION_SEND_STOP: 499 if (!drv_data->atomic) 500 drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; 501 writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, 502 drv_data->reg_base + drv_data->reg_offsets.control); 503 drv_data->block = 0; 504 wake_up(&drv_data->waitq); 505 break; 506 } 507 } 508
Hi Elad,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elad-Nachman/i2c-busses-i2c-mv64xxx-fix-arb-loss-i2c-lock/20231208-005406
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link: https://lore.kernel.org/r/20231207165027.2628302-2-enachman%40marvell.com
patch subject: [PATCH v2 1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock
config: csky-randconfig-r081-20231208 (https://download.01.org/0day-ci/archive/20231208/202312081920.oIwWca3H-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231208/202312081920.oIwWca3H-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202312081920.oIwWca3H-lkp@intel.com/
smatch warnings:
drivers/i2c/busses/i2c-mv64xxx.c:440 mv64xxx_i2c_do_action() error: uninitialized symbol 'ret'.
vim +/ret +440 drivers/i2c/busses/i2c-mv64xxx.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 368 static void
^1da177e4c3f41 Linus Torvalds 2005-04-16 369 mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
^1da177e4c3f41 Linus Torvalds 2005-04-16 370 {
8ea9d334890b12 Elad Nachman 2023-12-07 371 struct pinctrl *pc;
8ea9d334890b12 Elad Nachman 2023-12-07 372 int i, ret;
8ea9d334890b12 Elad Nachman 2023-12-07 373
^1da177e4c3f41 Linus Torvalds 2005-04-16 374 switch(drv_data->action) {
eda6bee6c7e67b Rodolfo Giometti 2010-11-26 375 case MV64XXX_I2C_ACTION_SEND_RESTART:
4243fa0bad551b Russell King 2013-05-16 376 /* We should only get here if we have further messages */
4243fa0bad551b Russell King 2013-05-16 377 BUG_ON(drv_data->num_msgs == 0);
4243fa0bad551b Russell King 2013-05-16 378
930ab3d403ae43 Gregory CLEMENT 2013-08-22 379 drv_data->msgs++;
930ab3d403ae43 Gregory CLEMENT 2013-08-22 380 drv_data->num_msgs--;
4c5b38e881b1a9 Wolfram Sang 2014-02-13 381 mv64xxx_i2c_send_start(drv_data);
4243fa0bad551b Russell King 2013-05-16 382
c1d15b68aab86f Gregory CLEMENT 2013-08-22 383 if (drv_data->errata_delay)
c1d15b68aab86f Gregory CLEMENT 2013-08-22 384 udelay(5);
c1d15b68aab86f Gregory CLEMENT 2013-08-22 385
4243fa0bad551b Russell King 2013-05-16 386 /*
4243fa0bad551b Russell King 2013-05-16 387 * We're never at the start of the message here, and by this
4243fa0bad551b Russell King 2013-05-16 388 * time it's already too late to do any protocol mangling.
4243fa0bad551b Russell King 2013-05-16 389 * Thankfully, do not advertise support for that feature.
4243fa0bad551b Russell King 2013-05-16 390 */
4243fa0bad551b Russell King 2013-05-16 391 drv_data->send_stop = drv_data->num_msgs == 1;
eda6bee6c7e67b Rodolfo Giometti 2010-11-26 392 break;
eda6bee6c7e67b Rodolfo Giometti 2010-11-26 393
^1da177e4c3f41 Linus Torvalds 2005-04-16 394 case MV64XXX_I2C_ACTION_CONTINUE:
^1da177e4c3f41 Linus Torvalds 2005-04-16 395 writel(drv_data->cntl_bits,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 396 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 397 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 398
^1da177e4c3f41 Linus Torvalds 2005-04-16 399 case MV64XXX_I2C_ACTION_SEND_ADDR_1:
^1da177e4c3f41 Linus Torvalds 2005-04-16 400 writel(drv_data->addr1,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 401 drv_data->reg_base + drv_data->reg_offsets.data);
^1da177e4c3f41 Linus Torvalds 2005-04-16 402 writel(drv_data->cntl_bits,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 403 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 404 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 405
^1da177e4c3f41 Linus Torvalds 2005-04-16 406 case MV64XXX_I2C_ACTION_SEND_ADDR_2:
^1da177e4c3f41 Linus Torvalds 2005-04-16 407 writel(drv_data->addr2,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 408 drv_data->reg_base + drv_data->reg_offsets.data);
^1da177e4c3f41 Linus Torvalds 2005-04-16 409 writel(drv_data->cntl_bits,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 410 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 411 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 412
^1da177e4c3f41 Linus Torvalds 2005-04-16 413 case MV64XXX_I2C_ACTION_SEND_DATA:
^1da177e4c3f41 Linus Torvalds 2005-04-16 414 writel(drv_data->msg->buf[drv_data->byte_posn++],
004e8ed7cc67f4 Maxime Ripard 2013-06-12 415 drv_data->reg_base + drv_data->reg_offsets.data);
^1da177e4c3f41 Linus Torvalds 2005-04-16 416 writel(drv_data->cntl_bits,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 417 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 418 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 419
^1da177e4c3f41 Linus Torvalds 2005-04-16 420 case MV64XXX_I2C_ACTION_RCV_DATA:
^1da177e4c3f41 Linus Torvalds 2005-04-16 421 drv_data->msg->buf[drv_data->byte_posn++] =
004e8ed7cc67f4 Maxime Ripard 2013-06-12 422 readl(drv_data->reg_base + drv_data->reg_offsets.data);
^1da177e4c3f41 Linus Torvalds 2005-04-16 423 writel(drv_data->cntl_bits,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 424 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 425 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 426
8ea9d334890b12 Elad Nachman 2023-12-07 427 case MV64XXX_I2C_ACTION_UNLOCK_BUS:
8ea9d334890b12 Elad Nachman 2023-12-07 428 if (!drv_data->soft_reset)
8ea9d334890b12 Elad Nachman 2023-12-07 429 break;
8ea9d334890b12 Elad Nachman 2023-12-07 430
8ea9d334890b12 Elad Nachman 2023-12-07 431 pc = devm_pinctrl_get(drv_data->adapter.dev.parent);
8ea9d334890b12 Elad Nachman 2023-12-07 432 if (IS_ERR(pc)) {
8ea9d334890b12 Elad Nachman 2023-12-07 433 dev_err(&drv_data->adapter.dev,
8ea9d334890b12 Elad Nachman 2023-12-07 434 "failed to get pinctrl for bus unlock!\n");
8ea9d334890b12 Elad Nachman 2023-12-07 435 break;
8ea9d334890b12 Elad Nachman 2023-12-07 436 }
8ea9d334890b12 Elad Nachman 2023-12-07 437
8ea9d334890b12 Elad Nachman 2023-12-07 438 /* Change i2c MPPs state to act as GPIOs: */
8ea9d334890b12 Elad Nachman 2023-12-07 439 if (pinctrl_select_state(pc, drv_data->i2c_gpio_state) >= 0) {
8ea9d334890b12 Elad Nachman 2023-12-07 @440 if (!ret) {
^^^^
"ret" isn't ever initialized.
8ea9d334890b12 Elad Nachman 2023-12-07 441 /*
8ea9d334890b12 Elad Nachman 2023-12-07 442 * Toggle i2c scl (serial clock) 10 times.
8ea9d334890b12 Elad Nachman 2023-12-07 443 * 10 clocks are enough to transfer a full
8ea9d334890b12 Elad Nachman 2023-12-07 444 * byte plus extra as seen from tests with
8ea9d334890b12 Elad Nachman 2023-12-07 445 * Ubiquity SFP module causing the issue.
8ea9d334890b12 Elad Nachman 2023-12-07 446 * This allows the slave that occupies
8ea9d334890b12 Elad Nachman 2023-12-07 447 * the bus to transmit its remaining data,
8ea9d334890b12 Elad Nachman 2023-12-07 448 * so it can release the i2c bus:
8ea9d334890b12 Elad Nachman 2023-12-07 449 */
8ea9d334890b12 Elad Nachman 2023-12-07 450 for (i = 0; i < 10; i++) {
8ea9d334890b12 Elad Nachman 2023-12-07 451 gpio_set_value(drv_data->scl_gpio, 1);
8ea9d334890b12 Elad Nachman 2023-12-07 452 udelay(100);
8ea9d334890b12 Elad Nachman 2023-12-07 453 gpio_set_value(drv_data->scl_gpio, 0);
8ea9d334890b12 Elad Nachman 2023-12-07 454 };
8ea9d334890b12 Elad Nachman 2023-12-07 455 }
8ea9d334890b12 Elad Nachman 2023-12-07 456
8ea9d334890b12 Elad Nachman 2023-12-07 457 /* restore i2c pin state to MPPs: */
8ea9d334890b12 Elad Nachman 2023-12-07 458 pinctrl_select_state(pc, drv_data->i2c_mpp_state);
8ea9d334890b12 Elad Nachman 2023-12-07 459 }
8ea9d334890b12 Elad Nachman 2023-12-07 460
8ea9d334890b12 Elad Nachman 2023-12-07 461 /*
8ea9d334890b12 Elad Nachman 2023-12-07 462 * Trigger controller soft reset
8ea9d334890b12 Elad Nachman 2023-12-07 463 * This register is write only, with none of the bits defined.
8ea9d334890b12 Elad Nachman 2023-12-07 464 * So any value will do.
8ea9d334890b12 Elad Nachman 2023-12-07 465 * 0x1 just seems more intuitive than 0x0 ...
8ea9d334890b12 Elad Nachman 2023-12-07 466 */
8ea9d334890b12 Elad Nachman 2023-12-07 467 writel(0x1, drv_data->reg_base + drv_data->reg_offsets.soft_reset);
8ea9d334890b12 Elad Nachman 2023-12-07 468 /* wait for i2c controller to complete reset: */
8ea9d334890b12 Elad Nachman 2023-12-07 469 udelay(100);
8ea9d334890b12 Elad Nachman 2023-12-07 470 /*
8ea9d334890b12 Elad Nachman 2023-12-07 471 * need to proceed to the data stop condition generation clause.
8ea9d334890b12 Elad Nachman 2023-12-07 472 * This is needed after clock toggling to put the i2c slave
8ea9d334890b12 Elad Nachman 2023-12-07 473 * in the correct state.
8ea9d334890b12 Elad Nachman 2023-12-07 474 */
8ea9d334890b12 Elad Nachman 2023-12-07 475 fallthrough;
8ea9d334890b12 Elad Nachman 2023-12-07 476
^1da177e4c3f41 Linus Torvalds 2005-04-16 477 case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
^1da177e4c3f41 Linus Torvalds 2005-04-16 478 drv_data->msg->buf[drv_data->byte_posn++] =
004e8ed7cc67f4 Maxime Ripard 2013-06-12 479 readl(drv_data->reg_base + drv_data->reg_offsets.data);
544a8d75f3d6e6 Chris Morgan 2022-03-30 480 if (!drv_data->atomic)
^1da177e4c3f41 Linus Torvalds 2005-04-16 481 drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
^1da177e4c3f41 Linus Torvalds 2005-04-16 482 writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 483 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 484 drv_data->block = 0;
c1d15b68aab86f Gregory CLEMENT 2013-08-22 485 if (drv_data->errata_delay)
c1d15b68aab86f Gregory CLEMENT 2013-08-22 486 udelay(5);
c1d15b68aab86f Gregory CLEMENT 2013-08-22 487
d295a86eab200b Russell King 2013-05-16 488 wake_up(&drv_data->waitq);
^1da177e4c3f41 Linus Torvalds 2005-04-16 489 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 490
^1da177e4c3f41 Linus Torvalds 2005-04-16 491 case MV64XXX_I2C_ACTION_INVALID:
^1da177e4c3f41 Linus Torvalds 2005-04-16 492 default:
^1da177e4c3f41 Linus Torvalds 2005-04-16 493 dev_err(&drv_data->adapter.dev,
^1da177e4c3f41 Linus Torvalds 2005-04-16 494 "mv64xxx_i2c_do_action: Invalid action: %d\n",
^1da177e4c3f41 Linus Torvalds 2005-04-16 495 drv_data->action);
^1da177e4c3f41 Linus Torvalds 2005-04-16 496 drv_data->rc = -EIO;
4db7e1786db505 Gustavo A. R. Silva 2020-07-21 497 fallthrough;
^1da177e4c3f41 Linus Torvalds 2005-04-16 498 case MV64XXX_I2C_ACTION_SEND_STOP:
544a8d75f3d6e6 Chris Morgan 2022-03-30 499 if (!drv_data->atomic)
^1da177e4c3f41 Linus Torvalds 2005-04-16 500 drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
^1da177e4c3f41 Linus Torvalds 2005-04-16 501 writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
004e8ed7cc67f4 Maxime Ripard 2013-06-12 502 drv_data->reg_base + drv_data->reg_offsets.control);
^1da177e4c3f41 Linus Torvalds 2005-04-16 503 drv_data->block = 0;
d295a86eab200b Russell King 2013-05-16 504 wake_up(&drv_data->waitq);
^1da177e4c3f41 Linus Torvalds 2005-04-16 505 break;
00d8689b85a7bb Thomas Petazzoni 2014-12-11 506 }
00d8689b85a7bb Thomas Petazzoni 2014-12-11 507 }
Hi Elad, On Thu, Dec 07, 2023 at 06:50:27PM +0200, Elad Nachman wrote: > From: Elad Nachman <enachman@marvell.com> > > Some i2c slaves, mainly SFPs, might cause the bus to lose arbitration > while slave is in the middle of responding. > This means that the i2c slave has not finished the transmission, but > the master has already finished toggling the clock, probably due to > the slave missing some of the master's clocks. > This was seen with Ubiquity SFP module. > This is typically caused by slaves which do not adhere completely > to the i2c standard. > > The solution is to change the I2C mode from mpps to gpios, and toggle > the i2c_scl gpio to emulate bus clock toggling, so slave will finish > its transmission, driven by the manual clock toggling, and will release > the i2c bus. Thanks for the explanation. [...] > @@ -409,6 +424,56 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > drv_data->reg_base + drv_data->reg_offsets.control); > break; > > + case MV64XXX_I2C_ACTION_UNLOCK_BUS: > + if (!drv_data->soft_reset) > + break; > + > + pc = devm_pinctrl_get(drv_data->adapter.dev.parent); Why don't you get this in the probe? Besides where are you "releasing" it? > + if (IS_ERR(pc)) { > + dev_err(&drv_data->adapter.dev, > + "failed to get pinctrl for bus unlock!\n"); > + break; > + } > + > + /* Change i2c MPPs state to act as GPIOs: */ > + if (pinctrl_select_state(pc, drv_data->i2c_gpio_state) >= 0) { > + if (!ret) { yeah... this was already already pointed out, I think you can remove it. > + /* > + * Toggle i2c scl (serial clock) 10 times. > + * 10 clocks are enough to transfer a full > + * byte plus extra as seen from tests with > + * Ubiquity SFP module causing the issue. > + * This allows the slave that occupies > + * the bus to transmit its remaining data, > + * so it can release the i2c bus: > + */ > + for (i = 0; i < 10; i++) { > + gpio_set_value(drv_data->scl_gpio, 1); > + udelay(100); why do you want to use the atomic sleeps? Isn't uslee_range() good for this case? > + gpio_set_value(drv_data->scl_gpio, 0); > + }; > + } > + > + /* restore i2c pin state to MPPs: */ > + pinctrl_select_state(pc, drv_data->i2c_mpp_state); > + } > + > + /* > + * Trigger controller soft reset > + * This register is write only, with none of the bits defined. > + * So any value will do. > + * 0x1 just seems more intuitive than 0x0 ... > + */ Thanks! > + writel(0x1, drv_data->reg_base + drv_data->reg_offsets.soft_reset); > + /* wait for i2c controller to complete reset: */ > + udelay(100); > + /* > + * need to proceed to the data stop condition generation clause. > + * This is needed after clock toggling to put the i2c slave > + * in the correct state. > + */ Thanks for the comment! /need/Need/ for consistency. Thanks, Andi
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index dc160cbc3155..954b94334772 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -26,6 +26,7 @@ #include <linux/clk.h> #include <linux/err.h> #include <linux/delay.h> +#include <linux/of_gpio.h> #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) @@ -104,6 +105,7 @@ enum { MV64XXX_I2C_ACTION_RCV_DATA, MV64XXX_I2C_ACTION_RCV_DATA_STOP, MV64XXX_I2C_ACTION_SEND_STOP, + MV64XXX_I2C_ACTION_UNLOCK_BUS }; struct mv64xxx_i2c_regs { @@ -150,6 +152,11 @@ struct mv64xxx_i2c_data { bool clk_n_base_0; struct i2c_bus_recovery_info rinfo; bool atomic; + /* I2C mpp states & gpios needed for arbitration lost recovery */ + int scl_gpio, sda_gpio; + bool soft_reset; + struct pinctrl_state *i2c_mpp_state; + struct pinctrl_state *i2c_gpio_state; }; static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { @@ -318,6 +325,11 @@ mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status) drv_data->state = MV64XXX_I2C_STATE_IDLE; break; + case MV64XXX_I2C_STATUS_MAST_LOST_ARB: /* 0x38 */ + drv_data->action = MV64XXX_I2C_ACTION_UNLOCK_BUS; + drv_data->state = MV64XXX_I2C_STATE_IDLE; + break; + case MV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK: /* 0x20 */ case MV64XXX_I2C_STATUS_MAST_WR_NO_ACK: /* 30 */ case MV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK: /* 48 */ @@ -356,6 +368,9 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) static void mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) { + struct pinctrl *pc; + int i, ret; + switch(drv_data->action) { case MV64XXX_I2C_ACTION_SEND_RESTART: /* We should only get here if we have further messages */ @@ -409,6 +424,56 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) drv_data->reg_base + drv_data->reg_offsets.control); break; + case MV64XXX_I2C_ACTION_UNLOCK_BUS: + if (!drv_data->soft_reset) + break; + + pc = devm_pinctrl_get(drv_data->adapter.dev.parent); + if (IS_ERR(pc)) { + dev_err(&drv_data->adapter.dev, + "failed to get pinctrl for bus unlock!\n"); + break; + } + + /* Change i2c MPPs state to act as GPIOs: */ + if (pinctrl_select_state(pc, drv_data->i2c_gpio_state) >= 0) { + if (!ret) { + /* + * Toggle i2c scl (serial clock) 10 times. + * 10 clocks are enough to transfer a full + * byte plus extra as seen from tests with + * Ubiquity SFP module causing the issue. + * This allows the slave that occupies + * the bus to transmit its remaining data, + * so it can release the i2c bus: + */ + for (i = 0; i < 10; i++) { + gpio_set_value(drv_data->scl_gpio, 1); + udelay(100); + gpio_set_value(drv_data->scl_gpio, 0); + }; + } + + /* restore i2c pin state to MPPs: */ + pinctrl_select_state(pc, drv_data->i2c_mpp_state); + } + + /* + * Trigger controller soft reset + * This register is write only, with none of the bits defined. + * So any value will do. + * 0x1 just seems more intuitive than 0x0 ... + */ + writel(0x1, drv_data->reg_base + drv_data->reg_offsets.soft_reset); + /* wait for i2c controller to complete reset: */ + udelay(100); + /* + * need to proceed to the data stop condition generation clause. + * This is needed after clock toggling to put the i2c slave + * in the correct state. + */ + fallthrough; + case MV64XXX_I2C_ACTION_RCV_DATA_STOP: drv_data->msg->buf[drv_data->byte_posn++] = readl(drv_data->reg_base + drv_data->reg_offsets.data); @@ -985,6 +1050,7 @@ mv64xxx_i2c_probe(struct platform_device *pd) { struct mv64xxx_i2c_data *drv_data; struct mv64xxx_i2c_pdata *pdata = dev_get_platdata(&pd->dev); + struct pinctrl *pc; int rc; if ((!pdata && !pd->dev.of_node)) @@ -1040,6 +1106,46 @@ mv64xxx_i2c_probe(struct platform_device *pd) if (rc == -EPROBE_DEFER) return rc; + /* + * Start with arbitration lost soft reset enabled as to false. + * Try to locate the necessary items in the device tree to + * make this feature work. + * Only after we verify that the device tree contains all of + * the needed information and that it is sound and usable, + * then we enable this flag. + * This information should be defined, but the driver maintains + * backward compatibility with old dts files, so it will not fail + * the probe in case these are missing. + */ + drv_data->soft_reset = false; + pc = devm_pinctrl_get(&pd->dev); + if (!IS_ERR(pc)) { + drv_data->i2c_mpp_state = + pinctrl_lookup_state(pc, "default"); + drv_data->i2c_gpio_state = + pinctrl_lookup_state(pc, "gpio"); + drv_data->scl_gpio = + of_get_named_gpio(pd->dev.of_node, "scl-gpios", 0); + drv_data->sda_gpio = + of_get_named_gpio(pd->dev.of_node, "sda-gpios", 0); + + if (!IS_ERR(drv_data->i2c_gpio_state) && + !IS_ERR(drv_data->i2c_mpp_state) && + gpio_is_valid(drv_data->scl_gpio) && + gpio_is_valid(drv_data->sda_gpio)) { + rc = devm_gpio_request_one(&pd->dev, drv_data->scl_gpio, + GPIOF_DIR_OUT, NULL); + rc |= devm_gpio_request_one(&pd->dev, drv_data->sda_gpio, + GPIOF_DIR_OUT, NULL); + if (!rc) + drv_data->soft_reset = true; + } + } + + if (!drv_data->soft_reset) + dev_info(&pd->dev, + "mv64xxx: missing arbitration-lost recovery definitions in dts file\n"); + drv_data->adapter.dev.parent = &pd->dev; drv_data->adapter.algo = &mv64xxx_i2c_algo; drv_data->adapter.owner = THIS_MODULE; @@ -1088,6 +1194,13 @@ mv64xxx_i2c_remove(struct platform_device *pd) { struct mv64xxx_i2c_data *drv_data = platform_get_drvdata(pd); + if (drv_data->soft_reset) { + devm_gpiod_put(drv_data->adapter.dev.parent, + gpio_to_desc(drv_data->scl_gpio)); + devm_gpiod_put(drv_data->adapter.dev.parent, + gpio_to_desc(drv_data->sda_gpio)); + } + i2c_del_adapter(&drv_data->adapter); free_irq(drv_data->irq, drv_data); pm_runtime_disable(&pd->dev);