Re: [PATCH v3] hwmon: (pmbus): Introduce page_change_delay

[William Kennington]

On Thu, Apr 3, 2025 at 5:28 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Thu, Apr 03, 2025 at 02:12:46PM -0700, William A. Kennington III wrote:
> > We have some buggy pmbus devices that require a delay after performing a
> > page change operation before trying to issue more commands to the
> > device.
> >
> > This allows for a configurable delay after page changes, but not
> > affecting other read or write operations.
> >
> > Signed-off-by: William A. Kennington III <william@xxxxxxxxxxxxxxx>
> > ---
> > V1 -> V2: Simplify how the backoff time is stored and computed
> > V2 -> V3: Use the BIT macro
> >
> >  drivers/hwmon/pmbus/pmbus.h      |  1 +
> >  drivers/hwmon/pmbus/pmbus_core.c | 67 +++++++++++++++-----------------
> >  2 files changed, 33 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index ddb19c9726d6..742dafc44390 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -482,6 +482,7 @@ struct pmbus_driver_info {
> >        */
> >       int access_delay;               /* in microseconds */
> >       int write_delay;                /* in microseconds */
> > +     int page_change_delay;          /* in microseconds */
> >  };
> >
> >  /* Regulator ops */
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 787683e83db6..3aa5851610b2 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -114,8 +114,8 @@ struct pmbus_data {
> >
> >       int vout_low[PMBUS_PAGES];      /* voltage low margin */
> >       int vout_high[PMBUS_PAGES];     /* voltage high margin */
> > -     ktime_t write_time;             /* Last SMBUS write timestamp */
> > -     ktime_t access_time;            /* Last SMBUS access timestamp */
> > +
> > +     ktime_t next_access_backoff;    /* Wait until at least this time */
> >  };
> >
> >  struct pmbus_debugfs_entry {
> > @@ -170,33 +170,30 @@ EXPORT_SYMBOL_NS_GPL(pmbus_set_update, "PMBUS");
> >  static void pmbus_wait(struct i2c_client *client)
> >  {
> >       struct pmbus_data *data = i2c_get_clientdata(client);
> > -     const struct pmbus_driver_info *info = data->info;
> > -     s64 delta;
> > -
> > -     if (info->access_delay) {
> > -             delta = ktime_us_delta(ktime_get(), data->access_time);
> > -
> > -             if (delta < info->access_delay)
> > -                     fsleep(info->access_delay - delta);
> > -     } else if (info->write_delay) {
> > -             delta = ktime_us_delta(ktime_get(), data->write_time);
> > +     s64 delay = ktime_us_delta(data->next_access_backoff, ktime_get());
> >
> > -             if (delta < info->write_delay)
> > -                     fsleep(info->write_delay - delta);
> > -     }
> > +     if (delay > 0)
> > +             fsleep(delay);
> >  }
> >
> > -/* Sets the last accessed timestamp for pmbus_wait */
> > -static void pmbus_update_ts(struct i2c_client *client, bool write_op)
> > +#define PMBUS_OP_READ BIT(0)
> > +#define PMBUS_OP_WRITE BIT(1)
> > +#define PMBUS_OP_PAGE_CHANGE BIT(2)
>
> I guess you really don't like tabs. Ok, no problem, I can fix that up when
> I apply the patch. Either case, please move the defines ahead of the first
> code block.

Done

>
> > +
> > +/* Sets the last operation timestamp for pmbus_wait */
> > +static void pmbus_update_ts(struct i2c_client *client, int op)
> >  {
> >       struct pmbus_data *data = i2c_get_clientdata(client);
> >       const struct pmbus_driver_info *info = data->info;
> > +     int delay = info->access_delay;
>
> Hmm, this is a functional change. It always sets the minimum wait
> time to access_delay, even if the operation is a write. I guess
> it makes sense because otherwise there would be no delay after
> a write if only access_delay is set. However, that means that
> PMBUS_OP_READ is not really needed anymore and can be dropped.
>
> This should be explained in the patch description.

Done

>
> >
> > -     if (info->access_delay) {
> > -             data->access_time = ktime_get();
> > -     } else if (info->write_delay && write_op) {
> > -             data->write_time = ktime_get();
> > -     }
> > +     if (op & (PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE))
> > +             delay = max(delay, info->write_delay);
> > +     if (op & PMBUS_OP_PAGE_CHANGE)
> > +             delay = max(delay, info->page_change_delay);
> > +
>
> I would have set PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE in the calling code,
> but this is ok too. However, please make it

Sure, I'll just move it to the callsite

>
>         if (op & (PMBUS_OP_WRITE | PMBUS_OP_PAGE_CHANGE)) {
>                 delay = max(delay, info->write_delay);
>                 if (op & PMBUS_OP_PAGE_CHANGE)
>                         delay = max(delay, info->page_change_delay);
>         }
>
> After dropping PMBUS_OP_READ, that can be simplified further to
>
>         if (op) {
>                 ...
>         }
>
> > +     if (delay > 0)
> > +             data->next_access_backoff = ktime_add_us(ktime_get(), delay);
> >  }
> >
> >  int pmbus_set_page(struct i2c_client *client, int page, int phase)
> > @@ -211,13 +208,13 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
> >           data->info->pages > 1 && page != data->currpage) {
> >               pmbus_wait(client);
> >               rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> > -             pmbus_update_ts(client, true);
> > +             pmbus_update_ts(client, PMBUS_OP_PAGE_CHANGE);
> >               if (rv < 0)
> >                       return rv;
> >
> >               pmbus_wait(client);
> >               rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> > -             pmbus_update_ts(client, false);
> > +             pmbus_update_ts(client, PMBUS_OP_READ);
> >               if (rv < 0)
> >                       return rv;
> >
> > @@ -231,7 +228,7 @@ int pmbus_set_page(struct i2c_client *client, int page, int phase)
> >               pmbus_wait(client);
> >               rv = i2c_smbus_write_byte_data(client, PMBUS_PHASE,
> >                                              phase);
> > -             pmbus_update_ts(client, true);
> > +             pmbus_update_ts(client, PMBUS_OP_WRITE);
> >               if (rv)
> >                       return rv;
> >       }
> > @@ -251,7 +248,7 @@ int pmbus_write_byte(struct i2c_client *client, int page, u8 value)
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_write_byte(client, value);
> > -     pmbus_update_ts(client, true);
> > +     pmbus_update_ts(client, PMBUS_OP_WRITE);
> >
> >       return rv;
> >  }
> > @@ -286,7 +283,7 @@ int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_write_word_data(client, reg, word);
> > -     pmbus_update_ts(client, true);
> > +     pmbus_update_ts(client, PMBUS_OP_WRITE);
> >
> >       return rv;
> >  }
> > @@ -408,7 +405,7 @@ int pmbus_read_word_data(struct i2c_client *client, int page, int phase, u8 reg)
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_read_word_data(client, reg);
> > -     pmbus_update_ts(client, false);
> > +     pmbus_update_ts(client, PMBUS_OP_READ);
> >
> >       return rv;
> >  }
> > @@ -471,7 +468,7 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_read_byte_data(client, reg);
> > -     pmbus_update_ts(client, false);
> > +     pmbus_update_ts(client, PMBUS_OP_READ);
> >
> >       return rv;
> >  }
> > @@ -487,7 +484,7 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_write_byte_data(client, reg, value);
> > -     pmbus_update_ts(client, true);
> > +     pmbus_update_ts(client, PMBUS_OP_WRITE);
> >
> >       return rv;
> >  }
> > @@ -523,7 +520,7 @@ static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
> >
> >       pmbus_wait(client);
> >       rv = i2c_smbus_read_block_data(client, reg, data_buf);
> > -     pmbus_update_ts(client, false);
> > +     pmbus_update_ts(client, PMBUS_OP_READ);
> >
> >       return rv;
> >  }
> > @@ -2530,7 +2527,7 @@ static int pmbus_read_coefficients(struct i2c_client *client,
> >       rv = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >                           I2C_SMBUS_WRITE, PMBUS_COEFFICIENTS,
> >                           I2C_SMBUS_BLOCK_PROC_CALL, &data);
> > -     pmbus_update_ts(client, true);
> > +     pmbus_update_ts(client, PMBUS_OP_READ | PMBUS_OP_WRITE);
>
> I'd argue that this does not warrant a PMBUS_OP_WRITE in the first place.
> From the chip's perspective, the operation is complete after the data
> is returned. This is just as much a write as any other SMBus read operation
> (a write of an address followed by a read). If you think otherwise, please
> explain.
>
> Either case, the change warrants an explanation in the patch description.

The previous behavior was to treat this as a write though? I updated
the description about picking the maximum delay in the code change
above, but this specific instance is still classified the same.

I think technically we shouldn't do a single smbus transfer, but do
the write followed by read with a write delay injected between them. I
don't want to make that change here but it doesn't make sense to
ignore the write delay IMHO.

>
> Thanks,
> Guenter