[PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Bastian Stender]

Signed-off-by: Bastian Stender <bst@pengutronix.de>
---
 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 34d91501342e..864ac91f8c1c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -36,6 +36,22 @@ Optional Properties:
     - first cell is the pin number
     - second cell is used to specify flags.
     See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
+    parent I2C adapter at these times:
+    + during setup of the multiplexer
+    + between setup of the multiplexer and the child bus I2C transaction
+    + between the child bus I2C transaction and releasing of the multiplexer
+    + during releasing of the multiplexer
+
+    However, I2C transactions to devices behind all I2C multiplexers connected
+    to the same parent adapter that this multiplexer is connected to are blocked
+    for the full duration of the complete multiplexed I2C transaction (i.e.
+    including the times covered by the above list).
+    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
+    This means that no unrelated I2C transactions are allowed on the parent I2C
+    adapter for the complete multiplexed I2C transaction.
+    The properties of mux-locked and parent-locked multiplexers are discussed
+    in more detail in Documentation/i2c/i2c-topology.
 
 Example:
 
-- 
2.17.0

[PATCH v2 2/3] i2c: Documentation: i2c-topology: i2c-mux-pca954x can be mux-locked via dt

[Bastian Stender]

Signed-off-by: Bastian Stender <bst@pengutronix.de>
---
 Documentation/i2c/i2c-topology | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
index f74d78b53d4d..165af7898e50 100644
--- a/Documentation/i2c/i2c-topology
+++ b/Documentation/i2c/i2c-topology
@@ -47,7 +47,8 @@ i2c-mux-gpmux             Normally parent-locked, mux-locked iff
 i2c-mux-ltc4306           Mux-locked
 i2c-mux-mlxcpld           Parent-locked
 i2c-mux-pca9541           Parent-locked
-i2c-mux-pca954x           Parent-locked
+i2c-mux-pca954x           Normally parent-locked, mux-locked iff
+                          specified in device-tree.
 i2c-mux-pinctrl           Normally parent-locked, mux-locked iff
                           all involved pinctrl devices are controlled
                           by the same i2c root adapter that they mux.
-- 
2.17.0

[PATCH v2 3/3] i2c: mux: pca954x: add option to use as mux-locked

[Bastian Stender]

Commit 6ef91fcca8a8 ("i2c: mux: relax locking of the top i2c adapter
during mux-locked muxing") introduced a new locking concept "mux-locked",
so that these muxes lock only the muxes on the parent adapter instead of
the whole i2c bus.

In case the dt property "mux-locked" is present make use of this
concept. This makes interaction between i2c devices behind the mux and
devices directly on the bus possible - at least in simple cases. It
might not work properly for complex i2c topologies.

If the dt property is not present use the parent-locked concept as
before.

Signed-off-by: Bastian Stender <bst@pengutronix.de>
---
Changes since implicit v1 ("i2c: mux: pca954x: use relaxed locking of
the top i2c adapter during mux-locked muxing"):

- fix unlocked SMBus access
- make mux-locked optional via dt
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 32 +++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 09bafd3e68fa..dfa9e8d350c7 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -219,6 +219,7 @@ MODULE_DEVICE_TABLE(of, pca954x_of_match);
 static int pca954x_reg_write(struct i2c_adapter *adap,
 			     struct i2c_client *client, u8 val)
 {
+	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 	int ret = -ENODEV;
 
 	if (adap->algo->master_xfer) {
@@ -230,16 +231,26 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
 		msg.len = 1;
 		buf[0] = val;
 		msg.buf = buf;
-		ret = __i2c_transfer(adap, &msg, 1);
+
+		if (muxc->mux_locked)
+			ret = i2c_transfer(adap, &msg, 1);
+		else
+			ret = __i2c_transfer(adap, &msg, 1);
 
 		if (ret >= 0 && ret != 1)
 			ret = -EREMOTEIO;
 	} else {
 		union i2c_smbus_data data;
-		ret = adap->algo->smbus_xfer(adap, client->addr,
+		if (muxc->mux_locked)
+			ret = i2c_smbus_xfer(adap, client->addr,
 					     client->flags,
 					     I2C_SMBUS_WRITE,
 					     val, I2C_SMBUS_BYTE, &data);
+		else
+			ret = adap->algo->smbus_xfer(adap, client->addr,
+						     client->flags,
+						     I2C_SMBUS_WRITE, val,
+						     I2C_SMBUS_BYTE, &data);
 	}
 
 	return ret;
@@ -371,7 +382,9 @@ static int pca954x_probe(struct i2c_client *client,
 	bool idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	int num, force, class;
+	bool mux_locked;
 	struct i2c_mux_core *muxc;
+	u32 mux_alloc_flags;
 	struct pca954x *data;
 	const struct of_device_id *match;
 	int ret;
@@ -379,9 +392,13 @@ static int pca954x_probe(struct i2c_client *client,
 	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
 		return -ENODEV;
 
+	mux_locked = of_property_read_bool(of_node, "mux-locked");
+
+	mux_alloc_flags = mux_locked ? I2C_MUX_LOCKED : 0;
 	muxc = i2c_mux_alloc(adap, &client->dev,
-			     PCA954X_MAX_NCHANS, sizeof(*data), 0,
-			     pca954x_select_chan, pca954x_deselect_mux);
+			     PCA954X_MAX_NCHANS, sizeof(*data),
+			     mux_alloc_flags, pca954x_select_chan,
+			     pca954x_deselect_mux);
 	if (!muxc)
 		return -ENOMEM;
 	data = i2c_mux_priv(muxc);
@@ -389,6 +406,8 @@ static int pca954x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
+	muxc->mux_locked = mux_locked;
+
 	/* Get the mux out of reset if a reset GPIO is specified. */
 	gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(gpio))
@@ -470,8 +489,9 @@ static int pca954x_probe(struct i2c_client *client,
 	}
 
 	dev_info(&client->dev,
-		 "registered %d multiplexed busses for I2C %s %s\n",
-		 num, data->chip->muxtype == pca954x_ismux
+		 "registered %d multiplexed busses for %s-locked I2C %s %s\n",
+		 num, muxc->mux_locked ? "mux" : "parent",
+		 data->chip->muxtype == pca954x_ismux
 				? "mux" : "switch", client->name);
 
 	return 0;
-- 
2.17.0

Re: [PATCH v2 3/3] i2c: mux: pca954x: add option to use as mux-locked

[Peter Rosin]

On 2018-05-04 15:04, Bastian Stender wrote:
> Commit 6ef91fcca8a8 ("i2c: mux: relax locking of the top i2c adapter
> during mux-locked muxing") introduced a new locking concept "mux-locked",
> so that these muxes lock only the muxes on the parent adapter instead of
> the whole i2c bus.
> 
> In case the dt property "mux-locked" is present make use of this
> concept. This makes interaction between i2c devices behind the mux and
> devices directly on the bus possible - at least in simple cases. It
> might not work properly for complex i2c topologies.
> 
> If the dt property is not present use the parent-locked concept as
> before.
> 
> Signed-off-by: Bastian Stender <bst@pengutronix.de>
> ---
> Changes since implicit v1 ("i2c: mux: pca954x: use relaxed locking of
> the top i2c adapter during mux-locked muxing"):
> 
> - fix unlocked SMBus access
> - make mux-locked optional via dt
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 32 +++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 09bafd3e68fa..dfa9e8d350c7 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -219,6 +219,7 @@ MODULE_DEVICE_TABLE(of, pca954x_of_match);
>  static int pca954x_reg_write(struct i2c_adapter *adap,
>  			     struct i2c_client *client, u8 val)
>  {
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>  	int ret = -ENODEV;
>  
>  	if (adap->algo->master_xfer) {
> @@ -230,16 +231,26 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
>  		msg.len = 1;
>  		buf[0] = val;
>  		msg.buf = buf;
> -		ret = __i2c_transfer(adap, &msg, 1);
> +
> +		if (muxc->mux_locked)
> +			ret = i2c_transfer(adap, &msg, 1);
> +		else
> +			ret = __i2c_transfer(adap, &msg, 1);
>  
>  		if (ret >= 0 && ret != 1)
>  			ret = -EREMOTEIO;
>  	} else {
>  		union i2c_smbus_data data;
> -		ret = adap->algo->smbus_xfer(adap, client->addr,
> +		if (muxc->mux_locked)
> +			ret = i2c_smbus_xfer(adap, client->addr,
>  					     client->flags,
>  					     I2C_SMBUS_WRITE,
>  					     val, I2C_SMBUS_BYTE, &data);
> +		else
> +			ret = adap->algo->smbus_xfer(adap, client->addr,
> +						     client->flags,
> +						     I2C_SMBUS_WRITE, val,
> +						     I2C_SMBUS_BYTE, &data);
>  	}
>  
>  	return ret;


Hmm, there is no need to handle the smbus/i2c_transfer differently when
mux-locked. Hence, I would prefer to "just" have three branches instead
of four. Also, the comment at the top of the function is now incorrect.
So, I suggest something like this (only compile-tested, I have no pca954x):

--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -214,33 +214,38 @@ static const struct of_device_id pca954x_of_match[] = {
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
 #endif
 
-/* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
-   for this as they will try to lock adapter a second time */
 static int pca954x_reg_write(struct i2c_adapter *adap,
 			     struct i2c_client *client, u8 val)
 {
-	int ret = -ENODEV;
-
-	if (adap->algo->master_xfer) {
-		struct i2c_msg msg;
-		char buf[1];
-
-		msg.addr = client->addr;
-		msg.flags = 0;
-		msg.len = 1;
-		buf[0] = val;
-		msg.buf = buf;
-		ret = __i2c_transfer(adap, &msg, 1);
-
-		if (ret >= 0 && ret != 1)
-			ret = -EREMOTEIO;
-	} else {
-		union i2c_smbus_data data;
-		ret = adap->algo->smbus_xfer(adap, client->addr,
-					     client->flags,
-					     I2C_SMBUS_WRITE,
-					     val, I2C_SMBUS_BYTE, &data);
-	}
+	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.len = 1,
+		.buf = &val,
+	};
+	int ret;
+
+	if (muxc->mux_locked)
+		return i2c_smbus_write_byte(client, val);
+
+	/*
+	 * Since we are not mux-locked, we can't use i2c_transfer() or
+	 * i2c_smbus_xfer() as they will try to lock the adapter a second
+	 * time.
+	 */
+	if (!adap->algo->master_xfer) {
+		union i2c_smbus_data data;
+
+		return adap->algo->smbus_xfer(adap, client->addr,
+					      client->flags,
+					      I2C_SMBUS_WRITE,
+					      val, I2C_SMBUS_BYTE, &data);
+	}
+
+	ret = __i2c_transfer(adap, &msg, 1);
+
+	if (ret >= 0 && ret != 1)
+		return -EREMOTEIO;
 
 	return ret;
 }

Otherwise, this patch LGTM. It could be squashed with 2/3 though.

Cheers,
Peter

> @@ -371,7 +382,9 @@ static int pca954x_probe(struct i2c_client *client,
>  	bool idle_disconnect_dt;
>  	struct gpio_desc *gpio;
>  	int num, force, class;
> +	bool mux_locked;
>  	struct i2c_mux_core *muxc;
> +	u32 mux_alloc_flags;
>  	struct pca954x *data;
>  	const struct of_device_id *match;
>  	int ret;
> @@ -379,9 +392,13 @@ static int pca954x_probe(struct i2c_client *client,
>  	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE))
>  		return -ENODEV;
>  
> +	mux_locked = of_property_read_bool(of_node, "mux-locked");
> +
> +	mux_alloc_flags = mux_locked ? I2C_MUX_LOCKED : 0;
>  	muxc = i2c_mux_alloc(adap, &client->dev,
> -			     PCA954X_MAX_NCHANS, sizeof(*data), 0,
> -			     pca954x_select_chan, pca954x_deselect_mux);
> +			     PCA954X_MAX_NCHANS, sizeof(*data),
> +			     mux_alloc_flags, pca954x_select_chan,
> +			     pca954x_deselect_mux);
>  	if (!muxc)
>  		return -ENOMEM;
>  	data = i2c_mux_priv(muxc);
> @@ -389,6 +406,8 @@ static int pca954x_probe(struct i2c_client *client,
>  	i2c_set_clientdata(client, muxc);
>  	data->client = client;
>  
> +	muxc->mux_locked = mux_locked;
> +
>  	/* Get the mux out of reset if a reset GPIO is specified. */
>  	gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(gpio))
> @@ -470,8 +489,9 @@ static int pca954x_probe(struct i2c_client *client,
>  	}
>  
>  	dev_info(&client->dev,
> -		 "registered %d multiplexed busses for I2C %s %s\n",
> -		 num, data->chip->muxtype == pca954x_ismux
> +		 "registered %d multiplexed busses for %s-locked I2C %s %s\n",
> +		 num, muxc->mux_locked ? "mux" : "parent",
> +		 data->chip->muxtype == pca954x_ismux
>  				? "mux" : "switch", client->name);
>  
>  	return 0;
> 

Re: [PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Bastian Stender]

On 05/04/2018 03:04 PM, Bastian Stender wrote:
> Signed-off-by: Bastian Stender <bst@pengutronix.de>
> ---
>   .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index 34d91501342e..864ac91f8c1c 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -36,6 +36,22 @@ Optional Properties:
>       - first cell is the pin number
>       - second cell is used to specify flags.
>       See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
> +    parent I2C adapter at these times:
> +    + during setup of the multiplexer
> +    + between setup of the multiplexer and the child bus I2C transaction
> +    + between the child bus I2C transaction and releasing of the multiplexer
> +    + during releasing of the multiplexer
> +
> +    However, I2C transactions to devices behind all I2C multiplexers connected
> +    to the same parent adapter that this multiplexer is connected to are blocked
> +    for the full duration of the complete multiplexed I2C transaction (i.e.
> +    including the times covered by the above list).
> +    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
> +    This means that no unrelated I2C transactions are allowed on the parent I2C
> +    adapter for the complete multiplexed I2C transaction.
> +    The properties of mux-locked and parent-locked multiplexers are discussed
> +    in more detail in Documentation/i2c/i2c-topology.

I am not sure about this. I think it will act like the gpmux driver here
so I copied it from
Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt. Is this correct?

Regards,
Bastian

-- 
Pengutronix e.K.
Industrial Linux Solutions
http://www.pengutronix.de/
Peiner Str. 6-8, 31137 Hildesheim, Germany
Amtsgericht Hildesheim, HRA 2686

Re: [PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Peter Rosin]

On 2018-05-04 15:10, Bastian Stender wrote:
> On 05/04/2018 03:04 PM, Bastian Stender wrote:
>> Signed-off-by: Bastian Stender <bst@pengutronix.de>
>> ---
>>   .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> index 34d91501342e..864ac91f8c1c 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>> @@ -36,6 +36,22 @@ Optional Properties:
>>       - first cell is the pin number
>>       - second cell is used to specify flags.
>>       See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> +  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
>> +    parent I2C adapter at these times:
>> +    + during setup of the multiplexer
>> +    + between setup of the multiplexer and the child bus I2C transaction
>> +    + between the child bus I2C transaction and releasing of the multiplexer
>> +    + during releasing of the multiplexer
>> +
>> +    However, I2C transactions to devices behind all I2C multiplexers connected
>> +    to the same parent adapter that this multiplexer is connected to are blocked
>> +    for the full duration of the complete multiplexed I2C transaction (i.e.
>> +    including the times covered by the above list).
>> +    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
>> +    This means that no unrelated I2C transactions are allowed on the parent I2C
>> +    adapter for the complete multiplexed I2C transaction.
>> +    The properties of mux-locked and parent-locked multiplexers are discussed
>> +    in more detail in Documentation/i2c/i2c-topology.
> 
> I am not sure about this. I think it will act like the gpmux driver here
> so I copied it from
> Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt. Is this correct?

I don't think it's wrong, but it might be a little bit too generic. The gpmux
binding cannot assume too much about the actual mux, but in this case we
know exactly how the mux operates. I.e. the initial 4-point list can drop
the points "during setup/releasing of the multiplexer" because those actions
happen as i2c transactions that are locking the parent adapter meaning that
nothing can disturb them.

However, it would be very good to know what the actual deadlock is that this
mux-locked is fixing. And a description of that deadlock would fit nicely in
the commit message. I understood it as if you could trigger it quite easily?
Any chance that you could make another attempt to pinpoint it with some
printk-debugging or something, given that lockdep wasn't helpful?

Cheers,
Peter

Re: [PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Bastian Stender]

Hi Peter,

On 05/07/2018 11:07 AM, Peter Rosin wrote:
> On 2018-05-04 15:10, Bastian Stender wrote:
>> On 05/04/2018 03:04 PM, Bastian Stender wrote:
>>> Signed-off-by: Bastian Stender <bst@pengutronix.de>
>>> ---
>>>    .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>> index 34d91501342e..864ac91f8c1c 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>> @@ -36,6 +36,22 @@ Optional Properties:
>>>        - first cell is the pin number
>>>        - second cell is used to specify flags.
>>>        See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>> +  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
>>> +    parent I2C adapter at these times:
>>> +    + during setup of the multiplexer
>>> +    + between setup of the multiplexer and the child bus I2C transaction
>>> +    + between the child bus I2C transaction and releasing of the multiplexer
>>> +    + during releasing of the multiplexer
>>> +
>>> +    However, I2C transactions to devices behind all I2C multiplexers connected
>>> +    to the same parent adapter that this multiplexer is connected to are blocked
>>> +    for the full duration of the complete multiplexed I2C transaction (i.e.
>>> +    including the times covered by the above list).
>>> +    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
>>> +    This means that no unrelated I2C transactions are allowed on the parent I2C
>>> +    adapter for the complete multiplexed I2C transaction.
>>> +    The properties of mux-locked and parent-locked multiplexers are discussed
>>> +    in more detail in Documentation/i2c/i2c-topology.
>>
>> I am not sure about this. I think it will act like the gpmux driver here
>> so I copied it from
>> Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt. Is this correct?
> 
> I don't think it's wrong, but it might be a little bit too generic. The gpmux
> binding cannot assume too much about the actual mux, but in this case we
> know exactly how the mux operates. I.e. the initial 4-point list can drop
> the points "during setup/releasing of the multiplexer" because those actions
> happen as i2c transactions that are locking the parent adapter meaning that
> nothing can disturb them.
> 
> However, it would be very good to know what the actual deadlock is that this
> mux-locked is fixing. And a description of that deadlock would fit nicely in
> the commit message. I understood it as if you could trigger it quite easily?
> Any chance that you could make another attempt to pinpoint it with some
> printk-debugging or something, given that lockdep wasn't helpful?

The root cause was now identified by a colleague of mine. The following
patch set was used along with the proposed patches:

   https://www.spinics.net/lists/linux-i2c/msg32324.html

As a result __i2c_transfer() ends up without enabled clocks. The problem
is solved in our case by switching to the mainline solution:

   https://www.spinics.net/lists/linux-i2c/msg33890.html

The question remains whether the proposed patch (along with the
suggested modifications) should be applied nonetheless?

Thanks for your explanations and patience.

Regards,
Bastian

-- 
Pengutronix e.K.
Industrial Linux Solutions
http://www.pengutronix.de/
Peiner Str. 6-8, 31137 Hildesheim, Germany
Amtsgericht Hildesheim, HRA 2686

Re: [PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Peter Rosin]

On 2018-05-14 14:26, Bastian Stender wrote:
> Hi Peter,
> 
> On 05/07/2018 11:07 AM, Peter Rosin wrote:
>> On 2018-05-04 15:10, Bastian Stender wrote:
>>> On 05/04/2018 03:04 PM, Bastian Stender wrote:
>>>> Signed-off-by: Bastian Stender <bst@pengutronix.de>
>>>> ---
>>>>    .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
>>>>    1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> index 34d91501342e..864ac91f8c1c 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> @@ -36,6 +36,22 @@ Optional Properties:
>>>>        - first cell is the pin number
>>>>        - second cell is used to specify flags.
>>>>        See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>>> +  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
>>>> +    parent I2C adapter at these times:
>>>> +    + during setup of the multiplexer
>>>> +    + between setup of the multiplexer and the child bus I2C transaction
>>>> +    + between the child bus I2C transaction and releasing of the multiplexer
>>>> +    + during releasing of the multiplexer
>>>> +
>>>> +    However, I2C transactions to devices behind all I2C multiplexers connected
>>>> +    to the same parent adapter that this multiplexer is connected to are blocked
>>>> +    for the full duration of the complete multiplexed I2C transaction (i.e.
>>>> +    including the times covered by the above list).
>>>> +    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
>>>> +    This means that no unrelated I2C transactions are allowed on the parent I2C
>>>> +    adapter for the complete multiplexed I2C transaction.
>>>> +    The properties of mux-locked and parent-locked multiplexers are discussed
>>>> +    in more detail in Documentation/i2c/i2c-topology.
>>>
>>> I am not sure about this. I think it will act like the gpmux driver here
>>> so I copied it from
>>> Documentation/devicetree/bindings/i2c/i2c-mux-gpmux.txt. Is this correct?
>>
>> I don't think it's wrong, but it might be a little bit too generic. The gpmux
>> binding cannot assume too much about the actual mux, but in this case we
>> know exactly how the mux operates. I.e. the initial 4-point list can drop
>> the points "during setup/releasing of the multiplexer" because those actions
>> happen as i2c transactions that are locking the parent adapter meaning that
>> nothing can disturb them.
>>
>> However, it would be very good to know what the actual deadlock is that this
>> mux-locked is fixing. And a description of that deadlock would fit nicely in
>> the commit message. I understood it as if you could trigger it quite easily?
>> Any chance that you could make another attempt to pinpoint it with some
>> printk-debugging or something, given that lockdep wasn't helpful?
> 
> The root cause was now identified by a colleague of mine. The following
> patch set was used along with the proposed patches:
> 
>    https://www.spinics.net/lists/linux-i2c/msg32324.html
> 
> As a result __i2c_transfer() ends up without enabled clocks. The problem
> is solved in our case by switching to the mainline solution:
> 
>    https://www.spinics.net/lists/linux-i2c/msg33890.html

Nice, thanks for keeping me in the loop!

> The question remains whether the proposed patch (along with the
> suggested modifications) should be applied nonetheless?

Maybe? I will not block it because there certainly are cases where it is
needed, but someone<tm> needs to make the adjustments.

Cheers,
Peter

Re: [PATCH v2 1/3] dt: bindings: i2c-mux-pca954x: add mux-locked property

[Rob Herring]

On Fri, May 04, 2018 at 03:04:47PM +0200, Bastian Stender wrote:
> Signed-off-by: Bastian Stender <bst@pengutronix.de>

Commit message needed.

> ---
>  .../devicetree/bindings/i2c/i2c-mux-pca954x.txt  | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index 34d91501342e..864ac91f8c1c 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -36,6 +36,22 @@ Optional Properties:
>      - first cell is the pin number
>      - second cell is used to specify flags.
>      See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  - mux-locked: If present, explicitly allow unrelated I2C transactions on the
> +    parent I2C adapter at these times:
> +    + during setup of the multiplexer
> +    + between setup of the multiplexer and the child bus I2C transaction
> +    + between the child bus I2C transaction and releasing of the multiplexer
> +    + during releasing of the multiplexer
> +
> +    However, I2C transactions to devices behind all I2C multiplexers connected
> +    to the same parent adapter that this multiplexer is connected to are blocked
> +    for the full duration of the complete multiplexed I2C transaction (i.e.
> +    including the times covered by the above list).
> +    If mux-locked is not present, the multiplexer is assumed to be parent-locked.
> +    This means that no unrelated I2C transactions are allowed on the parent I2C
> +    adapter for the complete multiplexed I2C transaction.
> +    The properties of mux-locked and parent-locked multiplexers are discussed
> +    in more detail in Documentation/i2c/i2c-topology.

Why are you copy-n-pasting paragraphs for an already defined property? 
Just refer to the definition.

Rob