* Re: [PATCH 1/2] i2c: mux: pca9541: add delayed-release support
2022-01-24 21:38 ` [PATCH 1/2] i2c: " Zev Weiss
@ 2022-01-31 6:21 ` Joel Stanley
0 siblings, 0 replies; 4+ 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] 4+ messages in thread