From: Elad Nachman <[email protected]>
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.
v2:
1) Explain more about cause of issue in commit message
2) Change variable name to something clearer
3) Leave space between comments
4) Remove redundant blank line
5) Add error message if pinctrl get failed
6) Move gpio request to probe function
7) Fix commenting style
8) Explain in comments why 10 togglings are required
9) Move from mdelay to udelay, reducing delay time
10) Explain in comments what is the value written
to the reset register.
11) Explain why fallthrough is required (generate stop condition)
12) Explain why in case of missing i2c arbitration loss details,
driver probe will not fail, in order to be backward compatible
with older dts files
Elad Nachman (1):
i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock
drivers/i2c/busses/i2c-mv64xxx.c | 113 +++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
--
2.25.1
From: Elad Nachman <[email protected]>
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.
Signed-off-by: Elad Nachman <[email protected]>
---
drivers/i2c/busses/i2c-mv64xxx.c | 113 +++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
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);
--
2.25.1
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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/[email protected]/config)
compiler: csky-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231208/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
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 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Elad,
On Thu, Dec 07, 2023 at 06:50:27PM +0200, Elad Nachman wrote:
> From: Elad Nachman <[email protected]>
>
> 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