| Message ID | DB6PR07MB35090E8350CC105E00E0462C9D812@DB6PR07MB3509.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pca954x: Reset if channel select fails | expand |
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 >
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
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 */