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

[Andi Shyti]

Hi Elad,

On Thu, Dec 07, 2023 at 06:50:27PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@xxxxxxxxxxx>
> 
> 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