[PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

Hello,

This series adds support for a new pca9541 device-tree property
("release-delay-us"), which delays releasing ownership of the bus
after a transaction for a configurable duration, anticipating that
another transaction may follow shortly.  By avoiding a
release/reacquisition between transactions, this can provide a
substantial performance improvement for back-to-back operations -- on
a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
ms) reduces the median latency the psusensor daemon's hwmon sysfs file
reads from 2.28 ms to 0.99 ms (a 57% improvement).


Thanks,
Zev

Changes since v1 [0]:
 - removed spurious #include line from dt-bindings example

[0] https://lore.kernel.org/linux-i2c/20220124213850.3766-1-zev@bewilderbeest.net/

Zev Weiss (2):
  i2c: mux: pca9541: add delayed-release support
  dt-bindings: i2c: add nxp,pca9541 release-delay-us property

 .../devicetree/bindings/i2c/nxp,pca9541.txt   |  9 +++
 drivers/i2c/muxes/i2c-mux-pca9541.c           | 56 ++++++++++++++++---
 2 files changed, 56 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

Hello,

This series adds support for a new pca9541 device-tree property
("release-delay-us"), which delays releasing ownership of the bus
after a transaction for a configurable duration, anticipating that
another transaction may follow shortly.  By avoiding a
release/reacquisition between transactions, this can provide a
substantial performance improvement for back-to-back operations -- on
a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
ms) reduces the median latency the psusensor daemon's hwmon sysfs file
reads from 2.28 ms to 0.99 ms (a 57% improvement).


Thanks,
Zev

Changes since v1 [0]:
 - removed spurious #include line from dt-bindings example

[0] https://lore.kernel.org/linux-i2c/20220124213850.3766-1-zev@bewilderbeest.net/

Zev Weiss (2):
  i2c: mux: pca9541: add delayed-release support
  dt-bindings: i2c: add nxp,pca9541 release-delay-us property

 .../devicetree/bindings/i2c/nxp,pca9541.txt   |  9 +++
 drivers/i2c/muxes/i2c-mux-pca9541.c           | 56 ++++++++++++++++---
 2 files changed, 56 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] i2c: mux: pca9541: add delayed-release support

[Zev Weiss]

For heavily-utilized i2c busses, the overhead of reacquiring bus
ownership on every transaction can be quite substantial.  By delaying
the release of the bus (in anticipation of another transaction needing
to re-acquire ownership in the near future), we can reduce the
overhead significantly -- a subsequent transaction that arrives within
the delay window can merely verify that we still own it.

The new "release-delay-us" DT property specifies a number of
microseconds to wait after a transaction before releasing the bus
(zero by default so as to preserve the existing behavior of releasing
it immediately).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 56 ++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6daec8d3d331..76269bf9f9ca 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -19,6 +19,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/jiffies.h>
@@ -75,6 +76,8 @@ struct pca9541 {
 	struct i2c_client *client;
 	unsigned long select_timeout;
 	unsigned long arb_timeout;
+	unsigned long release_delay;
+	struct delayed_work release_work;
 };
 
 static const struct i2c_device_id pca9541_id[] = {
@@ -127,8 +130,11 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
  * Arbitration management functions
  */
 
-/* Release bus. Also reset NTESTON and BUSINIT if it was set. */
-static void pca9541_release_bus(struct i2c_client *client)
+/*
+ * Release bus. Also reset NTESTON and BUSINIT if it was set.
+ * client->adapter must already be locked.
+ */
+static void __pca9541_release_bus(struct i2c_client *client)
 {
 	int reg;
 
@@ -138,6 +144,13 @@ static void pca9541_release_bus(struct i2c_client *client)
 				  (reg & PCA9541_CTL_NBUSON) >> 1);
 }
 
+static void pca9541_release_bus(struct i2c_client *client)
+{
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	__pca9541_release_bus(client);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+}
+
 /*
  * Arbitration is defined as a two-step process. A bus master can only activate
  * the slave bus if it owns it; otherwise it has to request ownership first.
@@ -254,6 +267,9 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
 	unsigned long timeout = jiffies + ARB2_TIMEOUT;
 		/* give up after this time */
 
+	if (data->release_delay)
+		cancel_delayed_work_sync(&data->release_work);
+
 	data->arb_timeout = jiffies + ARB_TIMEOUT;
 		/* force bus ownership after this time */
 
@@ -276,10 +292,21 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
 	struct pca9541 *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 
-	pca9541_release_bus(client);
+	if (data->release_delay)
+		schedule_delayed_work(&data->release_work, data->release_delay);
+	else
+		__pca9541_release_bus(client);
+
 	return 0;
 }
 
+static void pca9541_release_workfn(struct work_struct *work)
+{
+	struct pca9541 *data = container_of(work, struct pca9541, release_work.work);
+
+	pca9541_release_bus(data->client);
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -289,18 +316,13 @@ static int pca9541_probe(struct i2c_client *client,
 	struct i2c_adapter *adap = client->adapter;
 	struct i2c_mux_core *muxc;
 	struct pca9541 *data;
+	u32 release_delay_us;
 	int ret;
 
 	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
 
-	/*
-	 * I2C accesses are unprotected here.
-	 * We have to lock the I2C segment before releasing the bus.
-	 */
-	i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
 	pca9541_release_bus(client);
-	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
 
 	/* Create mux adapter */
 
@@ -313,6 +335,14 @@ static int pca9541_probe(struct i2c_client *client,
 	data = i2c_mux_priv(muxc);
 	data->client = client;
 
+	if (!device_property_read_u32(&client->dev, "release-delay-us", &release_delay_us)) {
+		data->release_delay = usecs_to_jiffies(release_delay_us);
+		ret = devm_delayed_work_autocancel(&client->dev, &data->release_work,
+						   pca9541_release_workfn);
+		if (ret)
+			return ret;
+	}
+
 	i2c_set_clientdata(client, muxc);
 
 	ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
@@ -328,6 +358,14 @@ static int pca9541_probe(struct i2c_client *client,
 static int pca9541_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct pca9541 *data = i2c_mux_priv(muxc);
+
+	/*
+	 * Ensure the bus is released (in case the device gets destroyed
+	 * before release_work runs).
+	 */
+	if (data->release_delay)
+		pca9541_release_bus(client);
 
 	i2c_mux_del_adapters(muxc);
 	return 0;
-- 
2.34.1

[PATCH v2 1/2] i2c: mux: pca9541: add delayed-release support

[Zev Weiss]

For heavily-utilized i2c busses, the overhead of reacquiring bus
ownership on every transaction can be quite substantial.  By delaying
the release of the bus (in anticipation of another transaction needing
to re-acquire ownership in the near future), we can reduce the
overhead significantly -- a subsequent transaction that arrives within
the delay window can merely verify that we still own it.

The new "release-delay-us" DT property specifies a number of
microseconds to wait after a transaction before releasing the bus
(zero by default so as to preserve the existing behavior of releasing
it immediately).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 56 ++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6daec8d3d331..76269bf9f9ca 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -19,6 +19,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/jiffies.h>
@@ -75,6 +76,8 @@ struct pca9541 {
 	struct i2c_client *client;
 	unsigned long select_timeout;
 	unsigned long arb_timeout;
+	unsigned long release_delay;
+	struct delayed_work release_work;
 };
 
 static const struct i2c_device_id pca9541_id[] = {
@@ -127,8 +130,11 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
  * Arbitration management functions
  */
 
-/* Release bus. Also reset NTESTON and BUSINIT if it was set. */
-static void pca9541_release_bus(struct i2c_client *client)
+/*
+ * Release bus. Also reset NTESTON and BUSINIT if it was set.
+ * client->adapter must already be locked.
+ */
+static void __pca9541_release_bus(struct i2c_client *client)
 {
 	int reg;
 
@@ -138,6 +144,13 @@ static void pca9541_release_bus(struct i2c_client *client)
 				  (reg & PCA9541_CTL_NBUSON) >> 1);
 }
 
+static void pca9541_release_bus(struct i2c_client *client)
+{
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	__pca9541_release_bus(client);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+}
+
 /*
  * Arbitration is defined as a two-step process. A bus master can only activate
  * the slave bus if it owns it; otherwise it has to request ownership first.
@@ -254,6 +267,9 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
 	unsigned long timeout = jiffies + ARB2_TIMEOUT;
 		/* give up after this time */
 
+	if (data->release_delay)
+		cancel_delayed_work_sync(&data->release_work);
+
 	data->arb_timeout = jiffies + ARB_TIMEOUT;
 		/* force bus ownership after this time */
 
@@ -276,10 +292,21 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
 	struct pca9541 *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 
-	pca9541_release_bus(client);
+	if (data->release_delay)
+		schedule_delayed_work(&data->release_work, data->release_delay);
+	else
+		__pca9541_release_bus(client);
+
 	return 0;
 }
 
+static void pca9541_release_workfn(struct work_struct *work)
+{
+	struct pca9541 *data = container_of(work, struct pca9541, release_work.work);
+
+	pca9541_release_bus(data->client);
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -289,18 +316,13 @@ static int pca9541_probe(struct i2c_client *client,
 	struct i2c_adapter *adap = client->adapter;
 	struct i2c_mux_core *muxc;
 	struct pca9541 *data;
+	u32 release_delay_us;
 	int ret;
 
 	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
 
-	/*
-	 * I2C accesses are unprotected here.
-	 * We have to lock the I2C segment before releasing the bus.
-	 */
-	i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
 	pca9541_release_bus(client);
-	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
 
 	/* Create mux adapter */
 
@@ -313,6 +335,14 @@ static int pca9541_probe(struct i2c_client *client,
 	data = i2c_mux_priv(muxc);
 	data->client = client;
 
+	if (!device_property_read_u32(&client->dev, "release-delay-us", &release_delay_us)) {
+		data->release_delay = usecs_to_jiffies(release_delay_us);
+		ret = devm_delayed_work_autocancel(&client->dev, &data->release_work,
+						   pca9541_release_workfn);
+		if (ret)
+			return ret;
+	}
+
 	i2c_set_clientdata(client, muxc);
 
 	ret = i2c_mux_add_adapter(muxc, 0, 0, 0);
@@ -328,6 +358,14 @@ static int pca9541_probe(struct i2c_client *client,
 static int pca9541_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct pca9541 *data = i2c_mux_priv(muxc);
+
+	/*
+	 * Ensure the bus is released (in case the device gets destroyed
+	 * before release_work runs).
+	 */
+	if (data->release_delay)
+		pca9541_release_bus(client);
 
 	i2c_mux_del_adapters(muxc);
 	return 0;
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: i2c: add nxp,pca9541 release-delay-us property

[Zev Weiss]

This property can be used to reduce arbitration overhead on busy i2c
busses by retaining ownership for a brief period in anticipation of
another transaction in the near future.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 Documentation/devicetree/bindings/i2c/nxp,pca9541.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
index 42bfc09c8918..9e8819593271 100644
--- a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
+++ b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
@@ -6,6 +6,14 @@ Required Properties:
 
   - reg: The I2C address of the device.
 
+Optional Properties:
+
+ - release-delay-us: the number of microseconds to delay before
+   releasing the bus after a transaction.  If unspecified the default
+   is zero (the bus is released immediately).  Non-zero values can
+   reduce arbitration overhead for back-to-back transactions, at the
+   cost of delaying the other master's access to the bus.
+
   The following required properties are defined externally:
 
   - I2C arbitration bus node. See i2c-arb.txt in this directory.
@@ -16,6 +24,7 @@ Example:
 	i2c-arbitrator@74 {
 		compatible = "nxp,pca9541";
 		reg = <0x74>;
+		release-delay-us = <20000>;
 
 		i2c-arb {
 			#address-cells = <1>;
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: i2c: add nxp, pca9541 release-delay-us property

[Zev Weiss]

This property can be used to reduce arbitration overhead on busy i2c
busses by retaining ownership for a brief period in anticipation of
another transaction in the near future.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 Documentation/devicetree/bindings/i2c/nxp,pca9541.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
index 42bfc09c8918..9e8819593271 100644
--- a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
+++ b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
@@ -6,6 +6,14 @@ Required Properties:
 
   - reg: The I2C address of the device.
 
+Optional Properties:
+
+ - release-delay-us: the number of microseconds to delay before
+   releasing the bus after a transaction.  If unspecified the default
+   is zero (the bus is released immediately).  Non-zero values can
+   reduce arbitration overhead for back-to-back transactions, at the
+   cost of delaying the other master's access to the bus.
+
   The following required properties are defined externally:
 
   - I2C arbitration bus node. See i2c-arb.txt in this directory.
@@ -16,6 +24,7 @@ Example:
 	i2c-arbitrator@74 {
 		compatible = "nxp,pca9541";
 		reg = <0x74>;
+		release-delay-us = <20000>;
 
 		i2c-arb {
 			#address-cells = <1>;
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: i2c: add nxp,pca9541 release-delay-us property

[Rob Herring]

On Mon, 31 Jan 2022 16:18:10 -0800, Zev Weiss wrote:
> This property can be used to reduce arbitration overhead on busy i2c
> busses by retaining ownership for a brief period in anticipation of
> another transaction in the near future.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  Documentation/devicetree/bindings/i2c/nxp,pca9541.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] dt-bindings: i2c: add nxp,pca9541 release-delay-us property

[Rob Herring]

On Mon, 31 Jan 2022 16:18:10 -0800, Zev Weiss wrote:
> This property can be used to reduce arbitration overhead on busy i2c
> busses by retaining ownership for a brief period in anticipation of
> another transaction in the near future.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  Documentation/devicetree/bindings/i2c/nxp,pca9541.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>Hello,
>
>This series adds support for a new pca9541 device-tree property
>("release-delay-us"), which delays releasing ownership of the bus
>after a transaction for a configurable duration, anticipating that
>another transaction may follow shortly.  By avoiding a
>release/reacquisition between transactions, this can provide a
>substantial performance improvement for back-to-back operations -- on
>a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>reads from 2.28 ms to 0.99 ms (a 57% improvement).
>

Ping...Guenter, any thoughts on this?


Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>Hello,
>
>This series adds support for a new pca9541 device-tree property
>("release-delay-us"), which delays releasing ownership of the bus
>after a transaction for a configurable duration, anticipating that
>another transaction may follow shortly.  By avoiding a
>release/reacquisition between transactions, this can provide a
>substantial performance improvement for back-to-back operations -- on
>a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>reads from 2.28 ms to 0.99 ms (a 57% improvement).
>

Ping...Guenter, any thoughts on this?


Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Guenter Roeck]

On 2/28/22 00:43, Zev Weiss wrote:
> On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>> Hello,
>>
>> This series adds support for a new pca9541 device-tree property
>> ("release-delay-us"), which delays releasing ownership of the bus
>> after a transaction for a configurable duration, anticipating that
>> another transaction may follow shortly.  By avoiding a
>> release/reacquisition between transactions, this can provide a
>> substantial performance improvement for back-to-back operations -- on
>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>
> 
> Ping...Guenter, any thoughts on this?
> 

It sounds reasonable to me, but I don't have access to hardware anymore
to test it, so I have no means to confirm that it actually works.

Guenter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Guenter Roeck]

On 2/28/22 00:43, Zev Weiss wrote:
> On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>> Hello,
>>
>> This series adds support for a new pca9541 device-tree property
>> ("release-delay-us"), which delays releasing ownership of the bus
>> after a transaction for a configurable duration, anticipating that
>> another transaction may follow shortly.  By avoiding a
>> release/reacquisition between transactions, this can provide a
>> substantial performance improvement for back-to-back operations -- on
>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>
> 
> Ping...Guenter, any thoughts on this?
> 

It sounds reasonable to me, but I don't have access to hardware anymore
to test it, so I have no means to confirm that it actually works.

Guenter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Mon, Feb 28, 2022 at 05:57:27AM PST, Guenter Roeck wrote:
>On 2/28/22 00:43, Zev Weiss wrote:
>>On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>>>Hello,
>>>
>>>This series adds support for a new pca9541 device-tree property
>>>("release-delay-us"), which delays releasing ownership of the bus
>>>after a transaction for a configurable duration, anticipating that
>>>another transaction may follow shortly.  By avoiding a
>>>release/reacquisition between transactions, this can provide a
>>>substantial performance improvement for back-to-back operations -- on
>>>a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>
>>
>>Ping...Guenter, any thoughts on this?
>>
>
>It sounds reasonable to me, but I don't have access to hardware anymore
>to test it, so I have no means to confirm that it actually works.
>

Ack, thanks.  In that case, what's the path forward on getting changes 
to this driver merged?  I see sign-offs from Wolfram and Peter on the 
last few commits that touched it -- any input from the i2c/i2c-mux 
maintainers?


Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Mon, Feb 28, 2022 at 05:57:27AM PST, Guenter Roeck wrote:
>On 2/28/22 00:43, Zev Weiss wrote:
>>On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>>>Hello,
>>>
>>>This series adds support for a new pca9541 device-tree property
>>>("release-delay-us"), which delays releasing ownership of the bus
>>>after a transaction for a configurable duration, anticipating that
>>>another transaction may follow shortly.  By avoiding a
>>>release/reacquisition between transactions, this can provide a
>>>substantial performance improvement for back-to-back operations -- on
>>>a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>
>>
>>Ping...Guenter, any thoughts on this?
>>
>
>It sounds reasonable to me, but I don't have access to hardware anymore
>to test it, so I have no means to confirm that it actually works.
>

Ack, thanks.  In that case, what's the path forward on getting changes 
to this driver merged?  I see sign-offs from Wolfram and Peter on the 
last few commits that touched it -- any input from the i2c/i2c-mux 
maintainers?


Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]


> Ack, thanks.  In that case, what's the path forward on getting changes to
> this driver merged?  I see sign-offs from Wolfram and Peter on the last few
> commits that touched it -- any input from the i2c/i2c-mux maintainers?

For the i2c mux subsystem, I usually wait for Peter's review.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]


> Ack, thanks.  In that case, what's the path forward on getting changes to
> this driver merged?  I see sign-offs from Wolfram and Peter on the last few
> commits that touched it -- any input from the i2c/i2c-mux maintainers?

For the i2c mux subsystem, I usually wait for Peter's review.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Guenter Roeck]

On 2/28/22 09:11, Zev Weiss wrote:
> On Mon, Feb 28, 2022 at 05:57:27AM PST, Guenter Roeck wrote:
>> On 2/28/22 00:43, Zev Weiss wrote:
>>> On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>>>> Hello,
>>>>
>>>> This series adds support for a new pca9541 device-tree property
>>>> ("release-delay-us"), which delays releasing ownership of the bus
>>>> after a transaction for a configurable duration, anticipating that
>>>> another transaction may follow shortly.  By avoiding a
>>>> release/reacquisition between transactions, this can provide a
>>>> substantial performance improvement for back-to-back operations -- on
>>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>>
>>>
>>> Ping...Guenter, any thoughts on this?
>>>
>>
>> It sounds reasonable to me, but I don't have access to hardware anymore
>> to test it, so I have no means to confirm that it actually works.
>>
> 
> Ack, thanks.  In that case, what's the path forward on getting changes to this driver merged?  I see sign-offs from Wolfram and Peter on the last few commits that touched it -- any input from the i2c/i2c-mux maintainers?
> 

The i2c/i2c-mux maintainers will need to accept it, and you'll need
approval for the DT changes from a DT maintainer (presumably Rob).

Guenter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Guenter Roeck]

On 2/28/22 09:11, Zev Weiss wrote:
> On Mon, Feb 28, 2022 at 05:57:27AM PST, Guenter Roeck wrote:
>> On 2/28/22 00:43, Zev Weiss wrote:
>>> On Mon, Jan 31, 2022 at 04:18:08PM PST, Zev Weiss wrote:
>>>> Hello,
>>>>
>>>> This series adds support for a new pca9541 device-tree property
>>>> ("release-delay-us"), which delays releasing ownership of the bus
>>>> after a transaction for a configurable duration, anticipating that
>>>> another transaction may follow shortly.  By avoiding a
>>>> release/reacquisition between transactions, this can provide a
>>>> substantial performance improvement for back-to-back operations -- on
>>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>>
>>>
>>> Ping...Guenter, any thoughts on this?
>>>
>>
>> It sounds reasonable to me, but I don't have access to hardware anymore
>> to test it, so I have no means to confirm that it actually works.
>>
> 
> Ack, thanks.  In that case, what's the path forward on getting changes to this driver merged?  I see sign-offs from Wolfram and Peter on the last few commits that touched it -- any input from the i2c/i2c-mux maintainers?
> 

The i2c/i2c-mux maintainers will need to accept it, and you'll need
approval for the DT changes from a DT maintainer (presumably Rob).

Guenter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Peter Rosin]

On 2022-02-01 01:18, Zev Weiss wrote:
> Hello,
> 
> This series adds support for a new pca9541 device-tree property
> ("release-delay-us"), which delays releasing ownership of the bus
> after a transaction for a configurable duration, anticipating that
> another transaction may follow shortly.  By avoiding a
> release/reacquisition between transactions, this can provide a
> substantial performance improvement for back-to-back operations -- on
> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
> reads from 2.28 ms to 0.99 ms (a 57% improvement).

Hi!

Sorry for the late reply. It seems I'm forever swamped...

There is a risk with this scheme. If you have two (or more) of these
chips on the same bus, and there are clients behind these two chips
that have the same address, accesses to one of the clients might
"leak through" the other arb to an unexpected client when its arb
is in it's release-delay state.

In other words, it is no coincidence that the segment lock is held
over the whole acquire-access-release cycle.

Sure, you can always say "don't add a release-delay when ...", but I
see no such documentation.

I have no access to this HW either.

Cheers,
Peter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Peter Rosin]

On 2022-02-01 01:18, Zev Weiss wrote:
> Hello,
> 
> This series adds support for a new pca9541 device-tree property
> ("release-delay-us"), which delays releasing ownership of the bus
> after a transaction for a configurable duration, anticipating that
> another transaction may follow shortly.  By avoiding a
> release/reacquisition between transactions, this can provide a
> substantial performance improvement for back-to-back operations -- on
> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
> reads from 2.28 ms to 0.99 ms (a 57% improvement).

Hi!

Sorry for the late reply. It seems I'm forever swamped...

There is a risk with this scheme. If you have two (or more) of these
chips on the same bus, and there are clients behind these two chips
that have the same address, accesses to one of the clients might
"leak through" the other arb to an unexpected client when its arb
is in it's release-delay state.

In other words, it is no coincidence that the segment lock is held
over the whole acquire-access-release cycle.

Sure, you can always say "don't add a release-delay when ...", but I
see no such documentation.

I have no access to this HW either.

Cheers,
Peter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

Hi Peter,

Thanks for the reply!  (More below.)

On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>On 2022-02-01 01:18, Zev Weiss wrote:
>> Hello,
>>
>> This series adds support for a new pca9541 device-tree property
>> ("release-delay-us"), which delays releasing ownership of the bus
>> after a transaction for a configurable duration, anticipating that
>> another transaction may follow shortly.  By avoiding a
>> release/reacquisition between transactions, this can provide a
>> substantial performance improvement for back-to-back operations -- on
>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>
>Hi!
>
>Sorry for the late reply. It seems I'm forever swamped...
>
>There is a risk with this scheme. If you have two (or more) of these
>chips on the same bus, and there are clients behind these two chips
>that have the same address, accesses to one of the clients might
>"leak through" the other arb to an unexpected client when its arb
>is in it's release-delay state.
>
>In other words, it is no coincidence that the segment lock is held
>over the whole acquire-access-release cycle.

That's not a scenario I had considered, but I think I see what you're 
saying.  Just to make sure I'm understanding correctly, the problematic 
situation you're describing would involve multiple (sibling, not 
parent/child cascaded) arbiters at distinct addresses on the same 
(master-side) bus, in effect acting as a sort of "distributed mux" in 
addition to arbitrating between multiple attached masters?  (So kind of 
an M-to-N arrangement between M masters and N busses.)

In which case if more than one of the arbiters had their downstream 
busses simultaneously connected to the same master (as could result if 
one were still connected due to a delayed release while the master had 
already started a subsequent transaction via another arbiter), the 
resulting "combined" bus could end up with address collisions between 
devices downstream of the arbiters if there are common addresses in use 
between the downstream busses.

>
>Sure, you can always say "don't add a release-delay when ...", but I
>see no such documentation.
>

Assuming I haven't misunderstood the above, would expanding the 
description of the property in the DT binding like so be sufficient?

  - release-delay-us: the number of microseconds to delay before
    releasing the bus after a transaction.  If unspecified the default
    is zero (the bus is released immediately).  Non-zero values can
    reduce arbitration overhead for back-to-back transactions, at the
    cost of delaying the other master's access to the bus.

    If this property is employed on hardware with multiple parallel
    (not cascaded) arbiters selecting between multiple downstream
    busses, address conflicts can occur if a device on one of the
    downstream busses uses the same address as a device on another
    downstream bus.  This property should thus only be used if either
    (a) there is only one arbiter on the bus, (b) multiple arbiters are
    strictly cascaded to a single downstream bus, or (c) all of the
    devices on all downstream busses use addresses that are unique
    across all of those busses.

If so I'll send a v3 with that change shortly.

Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

Hi Peter,

Thanks for the reply!  (More below.)

On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>On 2022-02-01 01:18, Zev Weiss wrote:
>> Hello,
>>
>> This series adds support for a new pca9541 device-tree property
>> ("release-delay-us"), which delays releasing ownership of the bus
>> after a transaction for a configurable duration, anticipating that
>> another transaction may follow shortly.  By avoiding a
>> release/reacquisition between transactions, this can provide a
>> substantial performance improvement for back-to-back operations -- on
>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>
>Hi!
>
>Sorry for the late reply. It seems I'm forever swamped...
>
>There is a risk with this scheme. If you have two (or more) of these
>chips on the same bus, and there are clients behind these two chips
>that have the same address, accesses to one of the clients might
>"leak through" the other arb to an unexpected client when its arb
>is in it's release-delay state.
>
>In other words, it is no coincidence that the segment lock is held
>over the whole acquire-access-release cycle.

That's not a scenario I had considered, but I think I see what you're 
saying.  Just to make sure I'm understanding correctly, the problematic 
situation you're describing would involve multiple (sibling, not 
parent/child cascaded) arbiters at distinct addresses on the same 
(master-side) bus, in effect acting as a sort of "distributed mux" in 
addition to arbitrating between multiple attached masters?  (So kind of 
an M-to-N arrangement between M masters and N busses.)

In which case if more than one of the arbiters had their downstream 
busses simultaneously connected to the same master (as could result if 
one were still connected due to a delayed release while the master had 
already started a subsequent transaction via another arbiter), the 
resulting "combined" bus could end up with address collisions between 
devices downstream of the arbiters if there are common addresses in use 
between the downstream busses.

>
>Sure, you can always say "don't add a release-delay when ...", but I
>see no such documentation.
>

Assuming I haven't misunderstood the above, would expanding the 
description of the property in the DT binding like so be sufficient?

  - release-delay-us: the number of microseconds to delay before
    releasing the bus after a transaction.  If unspecified the default
    is zero (the bus is released immediately).  Non-zero values can
    reduce arbitration overhead for back-to-back transactions, at the
    cost of delaying the other master's access to the bus.

    If this property is employed on hardware with multiple parallel
    (not cascaded) arbiters selecting between multiple downstream
    busses, address conflicts can occur if a device on one of the
    downstream busses uses the same address as a device on another
    downstream bus.  This property should thus only be used if either
    (a) there is only one arbiter on the bus, (b) multiple arbiters are
    strictly cascaded to a single downstream bus, or (c) all of the
    devices on all downstream busses use addresses that are unique
    across all of those busses.

If so I'll send a v3 with that change shortly.

Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Peter Rosin]

On 2022-02-28 23:38, Zev Weiss wrote:
> Hi Peter,
> 
> Thanks for the reply!  (More below.)
> 
> On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>> On 2022-02-01 01:18, Zev Weiss wrote:
>>> Hello,
>>>
>>> This series adds support for a new pca9541 device-tree property
>>> ("release-delay-us"), which delays releasing ownership of the bus
>>> after a transaction for a configurable duration, anticipating that
>>> another transaction may follow shortly.  By avoiding a
>>> release/reacquisition between transactions, this can provide a
>>> substantial performance improvement for back-to-back operations -- on
>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>
>> Hi!
>>
>> Sorry for the late reply. It seems I'm forever swamped...
>>
>> There is a risk with this scheme. If you have two (or more) of these
>> chips on the same bus, and there are clients behind these two chips
>> that have the same address, accesses to one of the clients might
>> "leak through" the other arb to an unexpected client when its arb
>> is in it's release-delay state.
>>
>> In other words, it is no coincidence that the segment lock is held
>> over the whole acquire-access-release cycle.
> 
> That's not a scenario I had considered, but I think I see what you're 
> saying.  Just to make sure I'm understanding correctly, the problematic 
> situation you're describing would involve multiple (sibling, not 
> parent/child cascaded) arbiters at distinct addresses on the same 
> (master-side) bus, in effect acting as a sort of "distributed mux" in 
> addition to arbitrating between multiple attached masters?  (So kind of 
> an M-to-N arrangement between M masters and N busses.)
> 
> In which case if more than one of the arbiters had their downstream 
> busses simultaneously connected to the same master (as could result if 
> one were still connected due to a delayed release while the master had 
> already started a subsequent transaction via another arbiter), the 
> resulting "combined" bus could end up with address collisions between 
> devices downstream of the arbiters if there are common addresses in use 
> between the downstream busses.

Yes, you understood me correctly. That is, if I understood you
correctly :-)

>>
>> Sure, you can always say "don't add a release-delay when ...", but I
>> see no such documentation.
>>
> 
> Assuming I haven't misunderstood the above, would expanding the 
> description of the property in the DT binding like so be sufficient?
> 
>   - release-delay-us: the number of microseconds to delay before
>     releasing the bus after a transaction.  If unspecified the default
>     is zero (the bus is released immediately).  Non-zero values can
>     reduce arbitration overhead for back-to-back transactions, at the
>     cost of delaying the other master's access to the bus.
> 
>     If this property is employed on hardware with multiple parallel
>     (not cascaded) arbiters selecting between multiple downstream
>     busses, address conflicts can occur if a device on one of the
>     downstream busses uses the same address as a device on another
>     downstream bus.  This property should thus only be used if either
>     (a) there is only one arbiter on the bus, (b) multiple arbiters are
>     strictly cascaded to a single downstream bus, or (c) all of the
>     devices on all downstream busses use addresses that are unique
>     across all of those busses.
> 
> If so I'll send a v3 with that change shortly.

I'm not sure this is a bindings thing or a quality of implementation
issue. You could imagine an implementation where the arb is
opportunistically left connected for the release-delay, but that the
arb is disconnected immediately if/when there is a change of the bus
topology, possibly only if the topology change in turn cause address
conflicts. The implication is that I'm not sure if this caveat should
be described in the bindings documentation or elsewhere. It sure would
be convenient to see it with the bindings, because if it is elsewhere
I'm sure more people will fail to take notice.

Another risk with the scheme is that you possibly lock out the other
master for so long time that you trigger it to force its way in, thus
possible wrecking some transaction. Or is the PCA9541 defending against
such wreckage on "hostile" takeovers? (I too lack the HW and time to
tinker with this.) If so, that might of course happen anyway, but it
might be so much more common if the bus is left connected.

Cheers,
Peter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Peter Rosin]

On 2022-02-28 23:38, Zev Weiss wrote:
> Hi Peter,
> 
> Thanks for the reply!  (More below.)
> 
> On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>> On 2022-02-01 01:18, Zev Weiss wrote:
>>> Hello,
>>>
>>> This series adds support for a new pca9541 device-tree property
>>> ("release-delay-us"), which delays releasing ownership of the bus
>>> after a transaction for a configurable duration, anticipating that
>>> another transaction may follow shortly.  By avoiding a
>>> release/reacquisition between transactions, this can provide a
>>> substantial performance improvement for back-to-back operations -- on
>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>
>> Hi!
>>
>> Sorry for the late reply. It seems I'm forever swamped...
>>
>> There is a risk with this scheme. If you have two (or more) of these
>> chips on the same bus, and there are clients behind these two chips
>> that have the same address, accesses to one of the clients might
>> "leak through" the other arb to an unexpected client when its arb
>> is in it's release-delay state.
>>
>> In other words, it is no coincidence that the segment lock is held
>> over the whole acquire-access-release cycle.
> 
> That's not a scenario I had considered, but I think I see what you're 
> saying.  Just to make sure I'm understanding correctly, the problematic 
> situation you're describing would involve multiple (sibling, not 
> parent/child cascaded) arbiters at distinct addresses on the same 
> (master-side) bus, in effect acting as a sort of "distributed mux" in 
> addition to arbitrating between multiple attached masters?  (So kind of 
> an M-to-N arrangement between M masters and N busses.)
> 
> In which case if more than one of the arbiters had their downstream 
> busses simultaneously connected to the same master (as could result if 
> one were still connected due to a delayed release while the master had 
> already started a subsequent transaction via another arbiter), the 
> resulting "combined" bus could end up with address collisions between 
> devices downstream of the arbiters if there are common addresses in use 
> between the downstream busses.

Yes, you understood me correctly. That is, if I understood you
correctly :-)

>>
>> Sure, you can always say "don't add a release-delay when ...", but I
>> see no such documentation.
>>
> 
> Assuming I haven't misunderstood the above, would expanding the 
> description of the property in the DT binding like so be sufficient?
> 
>   - release-delay-us: the number of microseconds to delay before
>     releasing the bus after a transaction.  If unspecified the default
>     is zero (the bus is released immediately).  Non-zero values can
>     reduce arbitration overhead for back-to-back transactions, at the
>     cost of delaying the other master's access to the bus.
> 
>     If this property is employed on hardware with multiple parallel
>     (not cascaded) arbiters selecting between multiple downstream
>     busses, address conflicts can occur if a device on one of the
>     downstream busses uses the same address as a device on another
>     downstream bus.  This property should thus only be used if either
>     (a) there is only one arbiter on the bus, (b) multiple arbiters are
>     strictly cascaded to a single downstream bus, or (c) all of the
>     devices on all downstream busses use addresses that are unique
>     across all of those busses.
> 
> If so I'll send a v3 with that change shortly.

I'm not sure this is a bindings thing or a quality of implementation
issue. You could imagine an implementation where the arb is
opportunistically left connected for the release-delay, but that the
arb is disconnected immediately if/when there is a change of the bus
topology, possibly only if the topology change in turn cause address
conflicts. The implication is that I'm not sure if this caveat should
be described in the bindings documentation or elsewhere. It sure would
be convenient to see it with the bindings, because if it is elsewhere
I'm sure more people will fail to take notice.

Another risk with the scheme is that you possibly lock out the other
master for so long time that you trigger it to force its way in, thus
possible wrecking some transaction. Or is the PCA9541 defending against
such wreckage on "hostile" takeovers? (I too lack the HW and time to
tinker with this.) If so, that might of course happen anyway, but it
might be so much more common if the bus is left connected.

Cheers,
Peter

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Wed, Mar 02, 2022 at 06:43:31AM PST, Peter Rosin wrote:
>On 2022-02-28 23:38, Zev Weiss wrote:
>> Hi Peter,
>>
>> Thanks for the reply!  (More below.)
>>
>> On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>>> On 2022-02-01 01:18, Zev Weiss wrote:
>>>> Hello,
>>>>
>>>> This series adds support for a new pca9541 device-tree property
>>>> ("release-delay-us"), which delays releasing ownership of the bus
>>>> after a transaction for a configurable duration, anticipating that
>>>> another transaction may follow shortly.  By avoiding a
>>>> release/reacquisition between transactions, this can provide a
>>>> substantial performance improvement for back-to-back operations -- on
>>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>
>>> Hi!
>>>
>>> Sorry for the late reply. It seems I'm forever swamped...
>>>
>>> There is a risk with this scheme. If you have two (or more) of these
>>> chips on the same bus, and there are clients behind these two chips
>>> that have the same address, accesses to one of the clients might
>>> "leak through" the other arb to an unexpected client when its arb
>>> is in it's release-delay state.
>>>
>>> In other words, it is no coincidence that the segment lock is held
>>> over the whole acquire-access-release cycle.
>>
>> That's not a scenario I had considered, but I think I see what you're
>> saying.  Just to make sure I'm understanding correctly, the problematic
>> situation you're describing would involve multiple (sibling, not
>> parent/child cascaded) arbiters at distinct addresses on the same
>> (master-side) bus, in effect acting as a sort of "distributed mux" in
>> addition to arbitrating between multiple attached masters?  (So kind of
>> an M-to-N arrangement between M masters and N busses.)
>>
>> In which case if more than one of the arbiters had their downstream
>> busses simultaneously connected to the same master (as could result if
>> one were still connected due to a delayed release while the master had
>> already started a subsequent transaction via another arbiter), the
>> resulting "combined" bus could end up with address collisions between
>> devices downstream of the arbiters if there are common addresses in use
>> between the downstream busses.
>
>Yes, you understood me correctly. That is, if I understood you
>correctly :-)
>

Okay, thanks for confirming.

>>>
>>> Sure, you can always say "don't add a release-delay when ...", but I
>>> see no such documentation.
>>>
>>
>> Assuming I haven't misunderstood the above, would expanding the
>> description of the property in the DT binding like so be sufficient?
>>
>>   - release-delay-us: the number of microseconds to delay before
>>     releasing the bus after a transaction.  If unspecified the default
>>     is zero (the bus is released immediately).  Non-zero values can
>>     reduce arbitration overhead for back-to-back transactions, at the
>>     cost of delaying the other master's access to the bus.
>>
>>     If this property is employed on hardware with multiple parallel
>>     (not cascaded) arbiters selecting between multiple downstream
>>     busses, address conflicts can occur if a device on one of the
>>     downstream busses uses the same address as a device on another
>>     downstream bus.  This property should thus only be used if either
>>     (a) there is only one arbiter on the bus, (b) multiple arbiters are
>>     strictly cascaded to a single downstream bus, or (c) all of the
>>     devices on all downstream busses use addresses that are unique
>>     across all of those busses.
>>
>> If so I'll send a v3 with that change shortly.
>
>I'm not sure this is a bindings thing or a quality of implementation
>issue. You could imagine an implementation where the arb is
>opportunistically left connected for the release-delay, but that the
>arb is disconnected immediately if/when there is a change of the bus
>topology, possibly only if the topology change in turn cause address
>conflicts. The implication is that I'm not sure if this caveat should
>be described in the bindings documentation or elsewhere. It sure would
>be convenient to see it with the bindings, because if it is elsewhere
>I'm sure more people will fail to take notice.
>

Yeah, I was thinking along similar lines -- bindings don't seem like 
exactly the "right" place for it, but perhaps the pragmatic place in 
order for people to actually see it.  I guess maybe this sort of gets 
back into questions of what exactly DT should describe and to what 
extent this is a property of the hardware vs. the software running on 
it...

Automating an earlier release in the event of a potential conflict being 
detected would be nice, though given the possibility of devices on the 
bus that the kernel doesn't know about (e.g. things driven directly from 
userspace via the i2c-dev interface) it doesn't seem entirely feasible 
to do that soundly.  A conservative approximation (e.g. releasing on the 
next transaction on the parent physical bus that's not from the same 
arbiter's virtual bus) seems like it could probably retain most of the 
benefit of the delayed release, though implementing it would probably 
require some plumbing in the i2c code that I'm guessing doesn't 
currently exist.  Thoughts?

>Another risk with the scheme is that you possibly lock out the other
>master for so long time that you trigger it to force its way in, thus
>possible wrecking some transaction. Or is the PCA9541 defending against
>such wreckage on "hostile" takeovers? (I too lack the HW and time to
>tinker with this.) If so, that might of course happen anyway, but it
>might be so much more common if the bus is left connected.
>

Yeah, this is sort of what the "at the cost of..." bit in the 
dt-bindings description was getting at, if a bit obliquely.  It's a risk 
that's not unique to this feature being in use, but yes, using it does 
of course increase the risk.  For what my datapoint is worth, it hasn't 
been problematic in my use of it thus far in a system with two fairly 
heavily-utilized busses (and a handful of more lightly used ones) each 
shared by two masters, both of which are Linux hosts running this driver 
with a release delay of 10ms.


Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Wed, Mar 02, 2022 at 06:43:31AM PST, Peter Rosin wrote:
>On 2022-02-28 23:38, Zev Weiss wrote:
>> Hi Peter,
>>
>> Thanks for the reply!  (More below.)
>>
>> On Mon, Feb 28, 2022 at 01:54:09PM PST, Peter Rosin wrote:
>>> On 2022-02-01 01:18, Zev Weiss wrote:
>>>> Hello,
>>>>
>>>> This series adds support for a new pca9541 device-tree property
>>>> ("release-delay-us"), which delays releasing ownership of the bus
>>>> after a transaction for a configurable duration, anticipating that
>>>> another transaction may follow shortly.  By avoiding a
>>>> release/reacquisition between transactions, this can provide a
>>>> substantial performance improvement for back-to-back operations -- on
>>>> a Delta AHE-50DC (ASPEED AST1250) system running OpenBMC with dozens
>>>> of LM25066 PMICs on PCA9541-arbitrated busses, a setting of 10000 (10
>>>> ms) reduces the median latency the psusensor daemon's hwmon sysfs file
>>>> reads from 2.28 ms to 0.99 ms (a 57% improvement).
>>>
>>> Hi!
>>>
>>> Sorry for the late reply. It seems I'm forever swamped...
>>>
>>> There is a risk with this scheme. If you have two (or more) of these
>>> chips on the same bus, and there are clients behind these two chips
>>> that have the same address, accesses to one of the clients might
>>> "leak through" the other arb to an unexpected client when its arb
>>> is in it's release-delay state.
>>>
>>> In other words, it is no coincidence that the segment lock is held
>>> over the whole acquire-access-release cycle.
>>
>> That's not a scenario I had considered, but I think I see what you're
>> saying.  Just to make sure I'm understanding correctly, the problematic
>> situation you're describing would involve multiple (sibling, not
>> parent/child cascaded) arbiters at distinct addresses on the same
>> (master-side) bus, in effect acting as a sort of "distributed mux" in
>> addition to arbitrating between multiple attached masters?  (So kind of
>> an M-to-N arrangement between M masters and N busses.)
>>
>> In which case if more than one of the arbiters had their downstream
>> busses simultaneously connected to the same master (as could result if
>> one were still connected due to a delayed release while the master had
>> already started a subsequent transaction via another arbiter), the
>> resulting "combined" bus could end up with address collisions between
>> devices downstream of the arbiters if there are common addresses in use
>> between the downstream busses.
>
>Yes, you understood me correctly. That is, if I understood you
>correctly :-)
>

Okay, thanks for confirming.

>>>
>>> Sure, you can always say "don't add a release-delay when ...", but I
>>> see no such documentation.
>>>
>>
>> Assuming I haven't misunderstood the above, would expanding the
>> description of the property in the DT binding like so be sufficient?
>>
>>   - release-delay-us: the number of microseconds to delay before
>>     releasing the bus after a transaction.  If unspecified the default
>>     is zero (the bus is released immediately).  Non-zero values can
>>     reduce arbitration overhead for back-to-back transactions, at the
>>     cost of delaying the other master's access to the bus.
>>
>>     If this property is employed on hardware with multiple parallel
>>     (not cascaded) arbiters selecting between multiple downstream
>>     busses, address conflicts can occur if a device on one of the
>>     downstream busses uses the same address as a device on another
>>     downstream bus.  This property should thus only be used if either
>>     (a) there is only one arbiter on the bus, (b) multiple arbiters are
>>     strictly cascaded to a single downstream bus, or (c) all of the
>>     devices on all downstream busses use addresses that are unique
>>     across all of those busses.
>>
>> If so I'll send a v3 with that change shortly.
>
>I'm not sure this is a bindings thing or a quality of implementation
>issue. You could imagine an implementation where the arb is
>opportunistically left connected for the release-delay, but that the
>arb is disconnected immediately if/when there is a change of the bus
>topology, possibly only if the topology change in turn cause address
>conflicts. The implication is that I'm not sure if this caveat should
>be described in the bindings documentation or elsewhere. It sure would
>be convenient to see it with the bindings, because if it is elsewhere
>I'm sure more people will fail to take notice.
>

Yeah, I was thinking along similar lines -- bindings don't seem like 
exactly the "right" place for it, but perhaps the pragmatic place in 
order for people to actually see it.  I guess maybe this sort of gets 
back into questions of what exactly DT should describe and to what 
extent this is a property of the hardware vs. the software running on 
it...

Automating an earlier release in the event of a potential conflict being 
detected would be nice, though given the possibility of devices on the 
bus that the kernel doesn't know about (e.g. things driven directly from 
userspace via the i2c-dev interface) it doesn't seem entirely feasible 
to do that soundly.  A conservative approximation (e.g. releasing on the 
next transaction on the parent physical bus that's not from the same 
arbiter's virtual bus) seems like it could probably retain most of the 
benefit of the delayed release, though implementing it would probably 
require some plumbing in the i2c code that I'm guessing doesn't 
currently exist.  Thoughts?

>Another risk with the scheme is that you possibly lock out the other
>master for so long time that you trigger it to force its way in, thus
>possible wrecking some transaction. Or is the PCA9541 defending against
>such wreckage on "hostile" takeovers? (I too lack the HW and time to
>tinker with this.) If so, that might of course happen anyway, but it
>might be so much more common if the bus is left connected.
>

Yeah, this is sort of what the "at the cost of..." bit in the 
dt-bindings description was getting at, if a bit obliquely.  It's a risk 
that's not unique to this feature being in use, but yes, using it does 
of course increase the risk.  For what my datapoint is worth, it hasn't 
been problematic in my use of it thus far in a system with two fairly 
heavily-utilized busses (and a handful of more lightly used ones) each 
shared by two masters, both of which are Linux hosts running this driver 
with a release delay of 10ms.


Thanks,
Zev

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]


Can someone give me a summary What is the status of this series?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]


Can someone give me a summary What is the status of this series?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/2] ic2: mux: pca9541: add delayed-release support

[Zev Weiss]

On Fri, Mar 18, 2022 at 04:00:34AM PDT, Wolfram Sang wrote:
>
>Can someone give me a summary What is the status of this series?
>

I had been hoping Peter might offer any further thoughts on my last 
email regarding the feasibility/proper approach for implementing some 
sort of automated avoidance of the problem scenario he pointed out -- or 
alternately, if we think a written warning in the bindings is 
sufficient, I can send a v3 with that incorporated.


Thanks,
Zev