[PATCH v5 0/2] pca954x: Add DT bindings and driver changes for reset after timeout

[PATCH v5 0/2] pca954x: Add DT bindings and driver changes for reset after timeout

[Wojciech Siudy]

The pca954x mux might not respond under certain cicumstances, like
device behindit holding SDA after recovery loop or some internal issue
in mux itself. Those situations are indicated by ETIMEDOUT returned
from I2C transaction attempting selecting or deselecting the channel.
According to device documentation the reset pulse restores I2C
subsystem of the mux and deselects the channel.

Since the mux switches using transistor switches, the failure of line
behind mux that is currently conneted prevents sending commands to mux
itself, so external reset signal is required. 

The following series of patches implements the reset functionality if
it was selected in devicetree. This cannot be default behaviour,
beceuse the reset line might not be dedivated on some boards and such
reset pulse would make other chips need reinitialisation.

---
Changelog:
v2:
  * Removed mail header from the commit log
  * Decreased reset pulse hold time from 10 to 1 ms
v3:
  * Make this functionality enabled by appropriate property in
    devicetree
v4:
  * Fix missing condition check from devicetree
v5:
  * Declare dependency of a new property
---
Wojciech Siudy (2):
  dt-bindings: i2c: pca954x: Add timeout reset property
  pca954x: Reset if channel select fails

 .../bindings/i2c/i2c-mux-pca954x.yaml         | 11 ++++
 drivers/i2c/muxes/i2c-mux-pca954x.c           | 51 +++++++++++++++----
 2 files changed, 51 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Wojciech Siudy]

For cases when the mux shares reset line with other chips we cannot
use it always when channel selection or deselection times out, because
it could break them without proper init/probe. The property is
necessary, because reset lines are board-specific.

Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>

---
Changelog:
v5:
  * Declare dependency of a new property
---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
index 9aa0585200c9..37882a5a8c87 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
@@ -63,6 +63,11 @@ properties:
       necessary for example, if there are several multiplexers on the bus and
       the devices behind them use same I2C addresses.
 
+  i2c-mux-timeout-reset:
+    type: boolean
+    description: Sends reset pulse if channel selection or deselection times
+      out. Do not use if other chips share the same reset line.
+
   idle-state:
     description: if present, overrides i2c-mux-idle-disconnect
     $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state
@@ -88,6 +93,9 @@ properties:
       register activates a channel to detect a stuck high fault. On fault the
       channel is isolated from the upstream bus.
 
+dependencies:
+  i2c-mux-timeout-reset: [ reset-gpios ]
+
 required:
   - compatible
   - reg
@@ -146,6 +154,9 @@ examples:
             interrupt-parent = <&ipic>;
             interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
             interrupt-controller;
+            reset-gpios = <&gpio1 27 1>;
+            i2c-mux-idle-disconnect;
+            i2c-mux-timeout-reset;
             #interrupt-cells = <2>;
 
             i2c@2 {
-- 
2.34.1

Re: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Rob Herring]

On Fri, Oct 18, 2024 at 12:03:37PM +0200, Wojciech Siudy wrote:
> For cases when the mux shares reset line with other chips we cannot
> use it always when channel selection or deselection times out, because
> it could break them without proper init/probe. The property is
> necessary, because reset lines are board-specific.
> 
> Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>
> 
> ---
> Changelog:
> v5:
>   * Declare dependency of a new property
> ---
>  .../devicetree/bindings/i2c/i2c-mux-pca954x.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> index 9aa0585200c9..37882a5a8c87 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
> @@ -63,6 +63,11 @@ properties:
>        necessary for example, if there are several multiplexers on the bus and
>        the devices behind them use same I2C addresses.
>  
> +  i2c-mux-timeout-reset:
> +    type: boolean
> +    description: Sends reset pulse if channel selection or deselection times
> +      out. Do not use if other chips share the same reset line.

If you have a reset GPIO for the mux, then why wouldn't just use it on 
timeout? What happens if you timeout and don't have this property? Just 
give up? 

Does the timeout time need to be configurable?

> +
>    idle-state:
>      description: if present, overrides i2c-mux-idle-disconnect
>      $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state
> @@ -88,6 +93,9 @@ properties:
>        register activates a channel to detect a stuck high fault. On fault the
>        channel is isolated from the upstream bus.
>  
> +dependencies:
> +  i2c-mux-timeout-reset: [ reset-gpios ]
> +
>  required:
>    - compatible
>    - reg
> @@ -146,6 +154,9 @@ examples:
>              interrupt-parent = <&ipic>;
>              interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
>              interrupt-controller;
> +            reset-gpios = <&gpio1 27 1>;
> +            i2c-mux-idle-disconnect;
> +            i2c-mux-timeout-reset;
>              #interrupt-cells = <2>;
>  
>              i2c@2 {
> -- 
> 2.34.1
> 

Re: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Wojciech Siudy (Nokia)]

Hi!

> If you have a reset GPIO for the mux, then why wouldn't just use it
> on timeout?

Because we cannot do that on every board. The reset GPIO is provided to
ensure, that we have reset signal de-asserted during initialisation.
You might have connected other devices to the same reset line so this
must be a configurable option.

> What happens if you timeout and don't have this property? Just 
give up?

This would be the case just like before introducting this change. Some
aplications might do other attempt, the bus driver can try recovery.
Unfortunately common reset line for multiple chips is not a rare
situation.

> Does the timeout time need to be configurable?

The timeout we are talking about is error code returned from function
pca954x_reg_write(), which calls smbus xfer that is out of this driver
scope so there is nothing we can configure.

Regards,
Wojciech

Re: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Krzysztof Kozlowski]

On Sat, Oct 19, 2024 at 11:09:43AM +0000, Wojciech Siudy (Nokia) wrote:
> Hi!
> 
> > If you have a reset GPIO for the mux, then why wouldn't just use it
> > on timeout?
> 
> Because we cannot do that on every board. The reset GPIO is provided to
> ensure, that we have reset signal de-asserted during initialisation.
> You might have connected other devices to the same reset line so this
> must be a configurable option.
> 
> > What happens if you timeout and don't have this property? Just 
> give up?
> 
> This would be the case just like before introducting this change. Some
> aplications might do other attempt, the bus driver can try recovery.
> Unfortunately common reset line for multiple chips is not a rare
> situation.

And Linux handles it well now. This is reset of the I2C devices
(children), not the controller, right? If so, then:
1. It's not a property of the controller,
2. Linux already handles it - switch to reset controller framework.

Best regards,
Krzysztof

ODP: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Wojciech Siudy (Nokia)]

Hello,

> This is reset of the I2C devices (children), not the controller, right?
Right, the mux is child device.

>  switch to reset controller framework
Please note that I left the function pca954x_reset_deassert() unchanged,
just moved it up and implemented two corresponding ones.
Do you mean that we should get rid of gpiod_set_value_cansleep(),
because the reset controller will handle it? I can agree but this is topic
for another submission since there we change when the reset is done,
not the way we achieve that.

Regards,
Wojciech

Re: ODP: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Krzysztof Kozlowski]

On 24/10/2024 09:05, Wojciech Siudy (Nokia) wrote:
> Hello,
> 
>> This is reset of the I2C devices (children), not the controller, right?
> Right, the mux is child device.

Not sure to which part I replied to. I have DT 200 emails in inbox per
day, I respond to less but still more than I can track. Keep some context.

I suspect you are adding reset for some other device, not for the one here.

> 
>>  switch to reset controller framework
> Please note that I left the function pca954x_reset_deassert() unchanged,
> just moved it up and implemented two corresponding ones.
> Do you mean that we should get rid of gpiod_set_value_cansleep(),
> because the reset controller will handle it? I can agree but this is topic
> for another submission since there we change when the reset is done,
> not the way we achieve that.

No, I meant I suspect you place property in inappropriate binding to
avoid shared GPIO problem.

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Wojciech Siudy (Nokia)]

> I suspect you are adding reset for some other device, not for the one here.
No, I reset pac954x chip in pca954x driver.

>  I suspect you place property in inappropriate binding
Many boards provide reset-gpio for this very device, but it was
used only to deassert the reset during initialization, so shared
gpio line was not a problem until now. New binding is to
reset later in runtime if necessary only if explicitly specified.

Best regards,
Wojciech

Re: [PATCH v5 1/2] dt-bindings: i2c: pca954x: Add timeout reset property

[Krzysztof Kozlowski]

On 24/10/2024 11:18, Wojciech Siudy (Nokia) wrote:
> No, I reset pac954x chip in pca954x driver.

How useful is such context? That's the last reply here, I am not going
to waste more time on such style.

All my earlier comments stay valid. You have shared reset and you want
some hacks to avoid that problem. I gave you the solution.

Best regards,
Krzysztof

[PATCH v5 2/2] pca954x: Reset if channel select fails

[Wojciech Siudy]

If the channel selection or deselection times out, it indicates
a failure in the mux's I2C subsystem. Without sending a reset pulse,
a power-on-reset of the entire device would be required to restore
communication.

The datasheet specifies a minimum hold time of 4 ns for the reset
pulse, but due to the path's capacitance and themux having its own
clock, it is recommended to extend this to approximately 1 us.

This option can be enabled using the i2c-mux-timeout-reset property
in the device tree and should only be used if the reset line is not
shared with other devices.

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 ms
v3:
  * Make this functionality enabled by appropriate property in
    devicetree
v4:
  * Fix missing condition check from devicetree
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 51 ++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 6f84018258c4..316048b0011d 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -110,6 +110,7 @@ struct pca954x {
 	u8 last_chan;		/* last register value */
 	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
 	s32 idle_state;
+	u8 timeout_reset;
 
 	struct i2c_client *client;
 
@@ -316,6 +317,30 @@ 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 void pca954x_reset_mux(struct pca954x *data)
+{
+	dev_warn(&data->client->dev, "resetting the device\n");
+	pca954x_reset_assert(data);
+	udelay(1);
+	pca954x_reset_deassert(data);
+}
+
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
@@ -329,6 +354,9 @@ 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) &&
+	    data->timeout_reset)
+		pca954x_reset_mux(data);
 
 	return ret;
 }
@@ -338,6 +366,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 	s32 idle_state;
+	int ret = 0;
 
 	idle_state = READ_ONCE(data->idle_state);
 	if (idle_state >= 0)
@@ -347,13 +376,16 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	if (idle_state == MUX_IDLE_DISCONNECT) {
 		/* Deselect active channel */
 		data->last_chan = 0;
-		return pca954x_reg_write(muxc->parent, client,
-					 data->last_chan);
+		ret = pca954x_reg_write(muxc->parent, client, data->last_chan);
+		if (ret == -ETIMEDOUT &&
+		    (data->reset_cont || data->reset_gpio) &&
+		    data->timeout_reset)
+			pca954x_reset_mux(data);
 	}
 
 	/* otherwise leave as-is */
 
-	return 0;
+	return ret;
 }
 
 static ssize_t idle_state_show(struct device *dev,
@@ -543,14 +575,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
  */
@@ -625,6 +649,11 @@ static int pca954x_probe(struct i2c_client *client)
 			data->idle_state = MUX_IDLE_DISCONNECT;
 	}
 
+	if (device_property_read_bool(dev, "i2c-mux-timeout-reset"))
+		data->timeout_reset = 1;
+	else
+		data->timeout_reset = 0;
+
 	/*
 	 * Write the mux register at addr to verify
 	 * that the mux is in fact present. This also
-- 
2.34.1

Re: [PATCH v5 2/2] pca954x: Reset if channel select fails

[Andi Shyti]

Hi Wojciech,

> +	if (device_property_read_bool(dev, "i2c-mux-timeout-reset"))
> +		data->timeout_reset = 1;
> +	else
> +		data->timeout_reset = 0;
> +

For as much as I would like this patch to be in, I can't take it
if we don't sort it out in the binding.

I agree with the suggestion that Krzysztof has provided there and
I'm aligned with his comments.

Thanks,
Andi