[v2,1/1] i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock

Message ID 20231207165027.2628302-2-enachman@marvell.com
State New
Headers
Series i2c: busses: i2c-mv64xxx: fix arb-loss i2c lock |

Commit Message

Elad Nachman Dec. 7, 2023, 4:50 p.m. UTC
  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.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 113 +++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
  

Comments

kernel test robot Dec. 8, 2023, 4:15 a.m. UTC | #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/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
  
Dan Carpenter Dec. 9, 2023, 6:19 a.m. UTC | #2
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  }
  
Andi Shyti Dec. 9, 2023, 12:59 p.m. UTC | #3
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
  

Patch

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);