diff mbox series

[v2] pca954x: Reset if channel select fails

Message ID DB6PR07MB35090E8350CC105E00E0462C9D812@DB6PR07MB3509.eurprd07.prod.outlook.com
State Changes Requested
Delegated to: Andi Shyti
Headers show
Series [v2] pca954x: Reset if channel select fails | expand

Commit Message

Wojciech Siudy (Nokia) Aug. 17, 2024, 3:06 p.m. UTC 
From: Wojciech Siudy <wojciech.siudy@nokia.com>

Channel selection that has timed out is a symptom of mux'es I2C
subsystem failure. Without sending reset pulse the power-on-reset
of entire device would be needed to restore the communication.

The datasheet mentions 4 ns as a minimal hold time of reset pulse,
but due to paths capacity and mux having its own clock it is better
to have it about 1 us.

Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>
---
Changelog:
v2:
  * Removed mail header from the commit log
  * Decreased reset pulse hold time from 10 to 1 us
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 30 +++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

Comments

Peter Rosin Aug. 19, 2024, 7:39 a.m. UTC | #1 
Hi!

2024-08-17 at 17:06, Wojciech Siudy (Nokia) wrote:
> [You don't often get email from wojciech.siudy@nokia.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Wojciech Siudy <wojciech.siudy@nokia.com>
> 
> Channel selection that has timed out is a symptom of mux'es I2C
> subsystem failure. Without sending reset pulse the power-on-reset
> of entire device would be needed to restore the communication.
> 
> The datasheet mentions 4 ns as a minimal hold time of reset pulse,
> but due to paths capacity and mux having its own clock it is better
> to have it about 1 us.
> 
> Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>
> ---
> Changelog:
> v2:
>   * Removed mail header from the commit log
>   * Decreased reset pulse hold time from 10 to 1 us
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 30 +++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 6f8401825..e09d4d107 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -316,6 +316,22 @@ static u8 pca954x_regval(struct pca954x *data, u8 chan)
>                 return 1 << chan;
>  }
> 
> +static void pca954x_reset_deassert(struct pca954x *data)
> +{
> +       if (data->reset_cont)
> +               reset_control_deassert(data->reset_cont);
> +       else
> +               gpiod_set_value_cansleep(data->reset_gpio, 0);
> +}
> +
> +static void pca954x_reset_assert(struct pca954x *data)
> +{
> +       if (data->reset_cont)
> +               reset_control_assert(data->reset_cont);
> +       else
> +               gpiod_set_value_cansleep(data->reset_gpio, 1);
> +}
> +
>  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  {
>         struct pca954x *data = i2c_mux_priv(muxc);
> @@ -329,6 +345,12 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>                 ret = pca954x_reg_write(muxc->parent, client, regval);
>                 data->last_chan = ret < 0 ? 0 : regval;
>         }
> +       if (ret == -ETIMEDOUT && (data->reset_cont || data->reset_gpio)) {
> +               dev_warn(&client->dev, "channel select failed, resetting...\n");
> +               pca954x_reset_assert(data);
> +               udelay(1);
> +               pca954x_reset_deassert(data);
> +       }

For the case where you have a dedicated pin (i.e., !data->reset_cont) this
might be an ok thing to do? But when you have more things (likely sibling
pca954x chips?) connected to the same reset line an assert of the reset line
destroys the assumed state of the other drivers/chips.

During probe, the reset is followed by some init for max7357 chips. This is
completely ignored and a timeout/reset probably breaks those chips badly.

Also, if this timeout needs to be handled, it is likely needed if deselect
times out too. Depending on surrounding hardware, it might be really
important to successfully put the mux in the correct idle state when it is
not used.

So, I feel that this needs more work.

Cheers,
Peter

> 
>         return ret;
>  }
> @@ -543,14 +565,6 @@ static int pca954x_get_reset(struct device *dev, struct pca954x *data)
>         return 0;
>  }
> 
> -static void pca954x_reset_deassert(struct pca954x *data)
> -{
> -       if (data->reset_cont)
> -               reset_control_deassert(data->reset_cont);
> -       else
> -               gpiod_set_value_cansleep(data->reset_gpio, 0);
> -}
> -
>  /*
>   * I2C init/probing/exit functions
>   */
> --
> 2.34.1
>
Wojciech Siudy (Nokia) Aug. 21, 2024, 7:42 a.m. UTC | #2 
Hi Peter!

> But when you have more things (likely sibling pca954x chips?)
> connected to the same reset line
This is very good point, thank you for that. Let's make this procedure
enabled by appropriate property in device tree.

> Also, if this timeout needs to be handled, it is likely needed if deselect
> times out too
Timeout of deselect is the symptom of hang mux as well, so It is even better
to reset it inside this function as well, because if we have
i2c-mux-idle-disconnect set we will be able to recover without failing any
transaction on the bus.

Regards,
Wojciech
Andi Shyti Sept. 9, 2024, 9:49 p.m. UTC | #3 
Hi Wojciech,

On Wed, Aug 21, 2024 at 07:42:33AM GMT, Wojciech Siudy (Nokia) wrote:
> Hi Peter!
> 
> > But when you have more things (likely sibling pca954x chips?)
> > connected to the same reset line
> This is very good point, thank you for that. Let's make this procedure
> enabled by appropriate property in device tree.
> 
> > Also, if this timeout needs to be handled, it is likely needed if deselect
> > times out too
> Timeout of deselect is the symptom of hang mux as well, so It is even better
> to reset it inside this function as well, because if we have
> i2c-mux-idle-disconnect set we will be able to recover without failing any
> transaction on the bus.

Are you working on a new version here?

Andi
Wojciech Siudy (Nokia) Sept. 11, 2024, 5:25 a.m. UTC | #4 
Hi Andi,
new version enabling this functionality via device tree property nearly done, but not sent because I didn't have time yet to make proper testing. Will do this week.

Regards,
Wojciech
diff mbox series

Patch

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 6f8401825..e09d4d107 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -316,6 +316,22 @@  static u8 pca954x_regval(struct pca954x *data, u8 chan)
 		return 1 << chan;
 }
 
+static void pca954x_reset_deassert(struct pca954x *data)
+{
+	if (data->reset_cont)
+		reset_control_deassert(data->reset_cont);
+	else
+		gpiod_set_value_cansleep(data->reset_gpio, 0);
+}
+
+static void pca954x_reset_assert(struct pca954x *data)
+{
+	if (data->reset_cont)
+		reset_control_assert(data->reset_cont);
+	else
+		gpiod_set_value_cansleep(data->reset_gpio, 1);
+}
+
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
@@ -329,6 +345,12 @@  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 		ret = pca954x_reg_write(muxc->parent, client, regval);
 		data->last_chan = ret < 0 ? 0 : regval;
 	}
+	if (ret == -ETIMEDOUT && (data->reset_cont || data->reset_gpio)) {
+		dev_warn(&client->dev, "channel select failed, resetting...\n");
+		pca954x_reset_assert(data);
+		udelay(1);
+		pca954x_reset_deassert(data);
+	}
 
 	return ret;
 }
@@ -543,14 +565,6 @@  static int pca954x_get_reset(struct device *dev, struct pca954x *data)
 	return 0;
 }
 
-static void pca954x_reset_deassert(struct pca954x *data)
-{
-	if (data->reset_cont)
-		reset_control_deassert(data->reset_cont);
-	else
-		gpiod_set_value_cansleep(data->reset_gpio, 0);
-}
-
 /*
  * I2C init/probing/exit functions
  */