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
