All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] ic2: mux: pca9541: add delayed-release support
@ 2022-01-24 21:38 ` Zev Weiss
  0 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Guenter Roeck, Peter Rosin, Rob Herring
  Cc: devicetree, openbmc, linux-kernel, 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 LM25066
PMICs behind PCA9541-arbitrated busses, a setting of 10000 (10 ms)
reduces the median latency of reading hwmon sysfs files from 2.28 ms
to 0.99 ms (a 57% improvement).


Thanks,
Zev

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   | 10 ++++
 drivers/i2c/muxes/i2c-mux-pca9541.c           | 56 ++++++++++++++++---
 2 files changed, 57 insertions(+), 9 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH 0/2] ic2: mux: pca9541: add delayed-release support
@ 2022-01-24 21:38 ` Zev Weiss
  0 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Guenter Roeck, Peter Rosin, Rob Herring
  Cc: Zev Weiss, openbmc, linux-kernel, devicetree

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 LM25066
PMICs behind PCA9541-arbitrated busses, a setting of 10000 (10 ms)
reduces the median latency of reading hwmon sysfs files from 2.28 ms
to 0.99 ms (a 57% improvement).


Thanks,
Zev

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   | 10 ++++
 drivers/i2c/muxes/i2c-mux-pca9541.c           | 56 ++++++++++++++++---
 2 files changed, 57 insertions(+), 9 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
* [PATCH 1/2] i2c: mux: pca9541: add delayed-release support
  2022-01-24 21:38 ` Zev Weiss
@ 2022-01-24 21:38   ` Zev Weiss
  -1 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Guenter Roeck, Peter Rosin; +Cc: openbmc, linux-kernel, 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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 1/2] i2c: mux: pca9541: add delayed-release support
@ 2022-01-24 21:38   ` Zev Weiss
  0 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Guenter Roeck, Peter Rosin; +Cc: Zev Weiss, openbmc, linux-kernel

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 2/2] dt-bindings: i2c: add nxp, pca9541 release-delay-us property
  2022-01-24 21:38 ` Zev Weiss
@ 2022-01-24 21:38   ` Zev Weiss
  -1 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Rob Herring, devicetree
  Cc: openbmc, Guenter Roeck, linux-kernel, Zev Weiss, Peter Rosin

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
index 42bfc09c8918..c755da59d6ec 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.
@@ -13,9 +21,11 @@ Required Properties:
 
 Example:
 
+	#include <dt-bindings/mux/mux.h>
 	i2c-arbitrator@74 {
 		compatible = "nxp,pca9541";
 		reg = <0x74>;
+		release-delay-us = <20000>;
 
 		i2c-arb {
 			#address-cells = <1>;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH 2/2] dt-bindings: i2c: add nxp,pca9541 release-delay-us property
@ 2022-01-24 21:38   ` Zev Weiss
  0 siblings, 0 replies; 8+ messages in thread
From: Zev Weiss @ 2022-01-24 21:38 UTC (permalink / raw)
  To: linux-i2c, Rob Herring, devicetree
  Cc: Zev Weiss, openbmc, linux-kernel, Guenter Roeck, Peter Rosin

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt b/Documentation/devicetree/bindings/i2c/nxp,pca9541.txt
index 42bfc09c8918..c755da59d6ec 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.
@@ -13,9 +21,11 @@ Required Properties:
 
 Example:
 
+	#include <dt-bindings/mux/mux.h>
 	i2c-arbitrator@74 {
 		compatible = "nxp,pca9541";
 		reg = <0x74>;
+		release-delay-us = <20000>;
 
 		i2c-arb {
 			#address-cells = <1>;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] i2c: mux: pca9541: add delayed-release support
  2022-01-24 21:38   ` Zev Weiss
@ 2022-01-31  6:21     ` Joel Stanley
  -1 siblings, 0 replies; 8+ messages in thread
From: Joel Stanley @ 2022-01-31  6:21 UTC (permalink / raw)
  To: Zev Weiss
  Cc: open list:I2C SUBSYSTEM HOST DRIVERS, Guenter Roeck, Peter Rosin,
	OpenBMC Maillist, Linux Kernel Mailing List

On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> 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).

Sounds like a good idea to me!

>
> 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
>

On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> 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>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] i2c: mux: pca9541: add delayed-release support
@ 2022-01-31  6:21     ` Joel Stanley
  0 siblings, 0 replies; 8+ messages in thread
From: Joel Stanley @ 2022-01-31  6:21 UTC (permalink / raw)
  To: Zev Weiss
  Cc: Linux Kernel Mailing List, OpenBMC Maillist,
	open list:I2C SUBSYSTEM HOST DRIVERS, Guenter Roeck, Peter Rosin

On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> 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).

Sounds like a good idea to me!

>
> 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
>

On Mon, 24 Jan 2022 at 21:39, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> 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>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-31  6:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-24 21:38 [PATCH 0/2] ic2: mux: pca9541: add delayed-release support Zev Weiss
2022-01-24 21:38 ` Zev Weiss
2022-01-24 21:38 ` [PATCH 1/2] i2c: " Zev Weiss
2022-01-24 21:38   ` Zev Weiss
2022-01-31  6:21   ` Joel Stanley
2022-01-31  6:21     ` Joel Stanley
2022-01-24 21:38 ` [PATCH 2/2] dt-bindings: i2c: add nxp, pca9541 release-delay-us property Zev Weiss
2022-01-24 21:38   ` [PATCH 2/2] dt-bindings: i2c: add nxp,pca9541 " Zev Weiss

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.