Thank you very much, Guenter! This patch boosted performance A LOT:
scanning one temperature reading through ~10 aforementioned PMBus
devices now takes 36 ms instead of ~2 secs!
- Alex Qiu
On Fri, Sep 4, 2020 at 9:33 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> Caching register values can be very expensive for PMBus chips. Some
> modern chips may have 10 or more pages, with several sensors supported
> per page. For example, MAX16601 creates more than 90 sysfs attributes.
> Register caching for such chips is time consuming, especially if only a
> few attributes are read on a regular basis. For MAX16601, it was observed
> that it can take up to two seconds to read all attributes on a slow I2C
> bus. In this situation, register caching results in the opposite of its
> intention: It increases the number of I2C operations, in some cases
> substantially, and it results in large latency when trying to access
> individual sensor data.
>
> Drop all register caching to solve the problem. Since it is no longer
> necessary, drop status register mapping as part of the change, and specify
> status registers directly.
>
> Cc: Alex Qiu <xqiu@xxxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Reviewed-by: Alex Qiu <xqiu@xxxxxxxxxx>
Tested-by: Alex Qiu <xqiu@xxxxxxxxxx>
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 206 +++++++++++++------------------
> 1 file changed, 89 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 44535add3a4a..9ab6a8ac5b40 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -16,7 +16,6 @@
> #include <linux/i2c.h>
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> -#include <linux/jiffies.h>
> #include <linux/pmbus.h>
> #include <linux/regulator/driver.h>
> #include <linux/regulator/machine.h>
> @@ -27,21 +26,6 @@
> * with each call to krealloc
> */
> #define PMBUS_ATTR_ALLOC_SIZE 32
> -
> -/*
> - * Index into status register array, per status register group
> - */
> -#define PB_STATUS_BASE 0
> -#define PB_STATUS_VOUT_BASE (PB_STATUS_BASE + PMBUS_PAGES)
> -#define PB_STATUS_IOUT_BASE (PB_STATUS_VOUT_BASE + PMBUS_PAGES)
> -#define PB_STATUS_FAN_BASE (PB_STATUS_IOUT_BASE + PMBUS_PAGES)
> -#define PB_STATUS_FAN34_BASE (PB_STATUS_FAN_BASE + PMBUS_PAGES)
> -#define PB_STATUS_TEMP_BASE (PB_STATUS_FAN34_BASE + PMBUS_PAGES)
> -#define PB_STATUS_INPUT_BASE (PB_STATUS_TEMP_BASE + PMBUS_PAGES)
> -#define PB_STATUS_VMON_BASE (PB_STATUS_INPUT_BASE + 1)
> -
> -#define PB_NUM_STATUS_REG (PB_STATUS_VMON_BASE + 1)
> -
> #define PMBUS_NAME_SIZE 24
>
> struct pmbus_sensor {
> @@ -77,6 +61,21 @@ struct pmbus_label {
> #define to_pmbus_label(_attr) \
> container_of(_attr, struct pmbus_label, attribute)
>
> +/* Macros for converting between sensor index and register/page/status mask */
> +
> +#define PB_STATUS_MASK 0xffff
> +#define PB_REG_SHIFT 16
> +#define PB_REG_MASK 0x3ff
> +#define PB_PAGE_SHIFT 26
> +#define PB_PAGE_MASK 0x3f
> +
> +#define pb_reg_to_index(page, reg, mask) (((page) << PB_PAGE_SHIFT) | \
> + ((reg) << PB_REG_SHIFT) | (mask))
> +
> +#define pb_index_to_page(index) (((index) >> PB_PAGE_SHIFT) & PB_PAGE_MASK)
> +#define pb_index_to_reg(index) (((index) >> PB_REG_SHIFT) & PB_REG_MASK)
> +#define pb_index_to_mask(index) ((index) & PB_STATUS_MASK)
> +
> struct pmbus_data {
> struct device *dev;
> struct device *hwmon_dev;
> @@ -97,14 +96,6 @@ struct pmbus_data {
> struct pmbus_sensor *sensors;
>
> struct mutex update_lock;
> - bool valid;
> - unsigned long last_updated; /* in jiffies */
> -
> - /*
> - * A single status register covers multiple attributes,
> - * so we keep them all together.
> - */
> - u16 status[PB_NUM_STATUS_REG];
>
> bool has_status_word; /* device uses STATUS_WORD register */
> int (*read_status)(struct i2c_client *client, int page);
> @@ -143,8 +134,10 @@ static const int pmbus_fan_command_registers[] = {
> void pmbus_clear_cache(struct i2c_client *client)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> + struct pmbus_sensor *sensor;
>
> - data->valid = false;
> + for (sensor = data->sensors; sensor; sensor = sensor->next)
> + sensor->data = -ENODATA;
> }
> EXPORT_SYMBOL_GPL(pmbus_clear_cache);
>
> @@ -560,68 +553,29 @@ const struct pmbus_driver_info *pmbus_get_driver_info(struct i2c_client *client)
> }
> EXPORT_SYMBOL_GPL(pmbus_get_driver_info);
>
> -static struct _pmbus_status {
> - u32 func;
> - u16 base;
> - u16 reg;
> -} pmbus_status[] = {
> - { PMBUS_HAVE_STATUS_VOUT, PB_STATUS_VOUT_BASE, PMBUS_STATUS_VOUT },
> - { PMBUS_HAVE_STATUS_IOUT, PB_STATUS_IOUT_BASE, PMBUS_STATUS_IOUT },
> - { PMBUS_HAVE_STATUS_TEMP, PB_STATUS_TEMP_BASE,
> - PMBUS_STATUS_TEMPERATURE },
> - { PMBUS_HAVE_STATUS_FAN12, PB_STATUS_FAN_BASE, PMBUS_STATUS_FAN_12 },
> - { PMBUS_HAVE_STATUS_FAN34, PB_STATUS_FAN34_BASE, PMBUS_STATUS_FAN_34 },
> -};
> -
> -static struct pmbus_data *pmbus_update_device(struct device *dev)
> +static int pmbus_get_status(struct i2c_client *client, int page, int reg)
> {
> - struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> - const struct pmbus_driver_info *info = data->info;
> - struct pmbus_sensor *sensor;
> -
> - mutex_lock(&data->update_lock);
> - if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> - int i, j;
> -
> - for (i = 0; i < info->pages; i++) {
> - data->status[PB_STATUS_BASE + i]
> - = data->read_status(client, i);
> - for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
> - struct _pmbus_status *s = &pmbus_status[j];
> -
> - if (!(info->func[i] & s->func))
> - continue;
> - data->status[s->base + i]
> - = _pmbus_read_byte_data(client, i,
> - s->reg);
> - }
> - }
> + int status;
>
> - if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
> - data->status[PB_STATUS_INPUT_BASE]
> - = _pmbus_read_byte_data(client, 0,
> - PMBUS_STATUS_INPUT);
> -
> - if (info->func[0] & PMBUS_HAVE_STATUS_VMON)
> - data->status[PB_STATUS_VMON_BASE]
> - = _pmbus_read_byte_data(client, 0,
> - PMBUS_VIRT_STATUS_VMON);
> -
> - for (sensor = data->sensors; sensor; sensor = sensor->next) {
> - if (!data->valid || sensor->update)
> - sensor->data
> - = _pmbus_read_word_data(client,
> - sensor->page,
> - sensor->phase,
> - sensor->reg);
> - }
> - pmbus_clear_faults(client);
> - data->last_updated = jiffies;
> - data->valid = 1;
> + switch (reg) {
> + case PMBUS_STATUS_WORD:
> + status = data->read_status(client, page);
> + break;
> + default:
> + status = _pmbus_read_byte_data(client, page, reg);
> + break;
> }
> - mutex_unlock(&data->update_lock);
> - return data;
> + if (status < 0)
> + pmbus_clear_faults(client);
> + return status;
> +}
> +
> +static void pmbus_update_sensor_data(struct i2c_client *client, struct pmbus_sensor *sensor)
> +{
> + if (sensor->data < 0 || sensor->update)
> + sensor->data = _pmbus_read_word_data(client, sensor->page,
> + sensor->phase, sensor->reg);
> }
>
> /*
> @@ -919,38 +873,53 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
> * If a negative value is stored in any of the referenced registers, this value
> * reflects an error code which will be returned.
> */
> -static int pmbus_get_boolean(struct pmbus_data *data, struct pmbus_boolean *b,
> +static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
> int index)
> {
> + struct pmbus_data *data = i2c_get_clientdata(client);
> struct pmbus_sensor *s1 = b->s1;
> struct pmbus_sensor *s2 = b->s2;
> - u16 reg = (index >> 16) & 0xffff;
> - u16 mask = index & 0xffff;
> + u16 mask = pb_index_to_mask(index);
> + u8 page = pb_index_to_page(index);
> + u16 reg = pb_index_to_reg(index);
> int ret, status;
> u16 regval;
>
> - status = data->status[reg];
> - if (status < 0)
> - return status;
> + mutex_lock(&data->update_lock);
> + status = pmbus_get_status(client, page, reg);
> + if (status < 0) {
> + ret = status;
> + goto unlock;
> + }
> +
> + if (s1)
> + pmbus_update_sensor_data(client, s1);
> + if (s2)
> + pmbus_update_sensor_data(client, s2);
>
> regval = status & mask;
> if (!s1 && !s2) {
> ret = !!regval;
> } else if (!s1 || !s2) {
> WARN(1, "Bad boolean descriptor %p: s1=%p, s2=%p\n", b, s1, s2);
> - return 0;
> } else {
> s64 v1, v2;
>
> - if (s1->data < 0)
> - return s1->data;
> - if (s2->data < 0)
> - return s2->data;
> + if (s1->data < 0) {
> + ret = s1->data;
> + goto unlock;
> + }
> + if (s2->data < 0) {
> + ret = s2->data;
> + goto unlock;
> + }
>
> v1 = pmbus_reg2data(data, s1);
> v2 = pmbus_reg2data(data, s2);
> ret = !!(regval && v1 >= v2);
> }
> +unlock:
> + mutex_unlock(&data->update_lock);
> return ret;
> }
>
> @@ -959,10 +928,10 @@ static ssize_t pmbus_show_boolean(struct device *dev,
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct pmbus_boolean *boolean = to_pmbus_boolean(attr);
> - struct pmbus_data *data = pmbus_update_device(dev);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> int val;
>
> - val = pmbus_get_boolean(data, boolean, attr->index);
> + val = pmbus_get_boolean(client, boolean, attr->index);
> if (val < 0)
> return val;
> return snprintf(buf, PAGE_SIZE, "%d\n", val);
> @@ -971,9 +940,11 @@ static ssize_t pmbus_show_boolean(struct device *dev,
> static ssize_t pmbus_show_sensor(struct device *dev,
> struct device_attribute *devattr, char *buf)
> {
> - struct pmbus_data *data = pmbus_update_device(dev);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> + struct pmbus_data *data = i2c_get_clientdata(client);
>
> + pmbus_update_sensor_data(client, sensor);
> if (sensor->data < 0)
> return sensor->data;
>
> @@ -1068,7 +1039,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
> const char *name, const char *type, int seq,
> struct pmbus_sensor *s1,
> struct pmbus_sensor *s2,
> - u16 reg, u16 mask)
> + u8 page, u16 reg, u16 mask)
> {
> struct pmbus_boolean *boolean;
> struct sensor_device_attribute *a;
> @@ -1084,7 +1055,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
> boolean->s1 = s1;
> boolean->s2 = s2;
> pmbus_attr_init(a, boolean->name, 0444, pmbus_show_boolean, NULL,
> - (reg << 16) | mask);
> + pb_reg_to_index(page, reg, mask));
>
> return pmbus_add_attribute(data, &a->dev_attr.attr);
> }
> @@ -1121,6 +1092,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> sensor->class = class;
> sensor->update = update;
> sensor->convert = convert;
> + sensor->data = -ENODATA;
> pmbus_dev_attr_init(a, sensor->name,
> readonly ? 0444 : 0644,
> pmbus_show_sensor, pmbus_set_sensor);
> @@ -1201,7 +1173,7 @@ struct pmbus_sensor_attr {
> bool compare; /* true if compare function needed */
> u32 func; /* sensor mask */
> u32 sfunc; /* sensor status mask */
> - int sbase; /* status base register */
> + int sreg; /* status register */
> const struct pmbus_limit_attr *limit;/* limit registers */
> };
>
> @@ -1239,7 +1211,7 @@ static int pmbus_add_limit_attrs(struct i2c_client *client,
> : NULL,
> attr->compare ? l->low ? base : curr
> : NULL,
> - attr->sbase + page, l->sbit);
> + page, attr->sreg, l->sbit);
> if (ret)
> return ret;
> have_alarm = 1;
> @@ -1289,7 +1261,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
> pmbus_check_status_register(client, page)) {
> ret = pmbus_add_boolean(data, name, "alarm", index,
> NULL, NULL,
> - PB_STATUS_BASE + page,
> + page, PMBUS_STATUS_WORD,
> attr->gbit);
> if (ret)
> return ret;
> @@ -1477,7 +1449,7 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> .label = "vin",
> .func = PMBUS_HAVE_VIN,
> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> - .sbase = PB_STATUS_INPUT_BASE,
> + .sreg = PMBUS_STATUS_INPUT,
> .gbit = PB_STATUS_VIN_UV,
> .limit = vin_limit_attrs,
> .nlimit = ARRAY_SIZE(vin_limit_attrs),
> @@ -1487,7 +1459,7 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> .label = "vmon",
> .func = PMBUS_HAVE_VMON,
> .sfunc = PMBUS_HAVE_STATUS_VMON,
> - .sbase = PB_STATUS_VMON_BASE,
> + .sreg = PMBUS_VIRT_STATUS_VMON,
> .limit = vmon_limit_attrs,
> .nlimit = ARRAY_SIZE(vmon_limit_attrs),
> }, {
> @@ -1502,7 +1474,7 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> .paged = true,
> .func = PMBUS_HAVE_VOUT,
> .sfunc = PMBUS_HAVE_STATUS_VOUT,
> - .sbase = PB_STATUS_VOUT_BASE,
> + .sreg = PMBUS_STATUS_VOUT,
> .gbit = PB_STATUS_VOUT_OV,
> .limit = vout_limit_attrs,
> .nlimit = ARRAY_SIZE(vout_limit_attrs),
> @@ -1581,7 +1553,7 @@ static const struct pmbus_sensor_attr current_attributes[] = {
> .label = "iin",
> .func = PMBUS_HAVE_IIN,
> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> - .sbase = PB_STATUS_INPUT_BASE,
> + .sreg = PMBUS_STATUS_INPUT,
> .gbit = PB_STATUS_INPUT,
> .limit = iin_limit_attrs,
> .nlimit = ARRAY_SIZE(iin_limit_attrs),
> @@ -1592,7 +1564,7 @@ static const struct pmbus_sensor_attr current_attributes[] = {
> .paged = true,
> .func = PMBUS_HAVE_IOUT,
> .sfunc = PMBUS_HAVE_STATUS_IOUT,
> - .sbase = PB_STATUS_IOUT_BASE,
> + .sreg = PMBUS_STATUS_IOUT,
> .gbit = PB_STATUS_IOUT_OC,
> .limit = iout_limit_attrs,
> .nlimit = ARRAY_SIZE(iout_limit_attrs),
> @@ -1666,7 +1638,7 @@ static const struct pmbus_sensor_attr power_attributes[] = {
> .label = "pin",
> .func = PMBUS_HAVE_PIN,
> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> - .sbase = PB_STATUS_INPUT_BASE,
> + .sreg = PMBUS_STATUS_INPUT,
> .gbit = PB_STATUS_INPUT,
> .limit = pin_limit_attrs,
> .nlimit = ARRAY_SIZE(pin_limit_attrs),
> @@ -1677,7 +1649,7 @@ static const struct pmbus_sensor_attr power_attributes[] = {
> .paged = true,
> .func = PMBUS_HAVE_POUT,
> .sfunc = PMBUS_HAVE_STATUS_IOUT,
> - .sbase = PB_STATUS_IOUT_BASE,
> + .sreg = PMBUS_STATUS_IOUT,
> .limit = pout_limit_attrs,
> .nlimit = ARRAY_SIZE(pout_limit_attrs),
> }
> @@ -1796,7 +1768,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
> .compare = true,
> .func = PMBUS_HAVE_TEMP,
> .sfunc = PMBUS_HAVE_STATUS_TEMP,
> - .sbase = PB_STATUS_TEMP_BASE,
> + .sreg = PMBUS_STATUS_TEMPERATURE,
> .gbit = PB_STATUS_TEMPERATURE,
> .limit = temp_limit_attrs,
> .nlimit = ARRAY_SIZE(temp_limit_attrs),
> @@ -1808,7 +1780,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
> .compare = true,
> .func = PMBUS_HAVE_TEMP2,
> .sfunc = PMBUS_HAVE_STATUS_TEMP,
> - .sbase = PB_STATUS_TEMP_BASE,
> + .sreg = PMBUS_STATUS_TEMPERATURE,
> .gbit = PB_STATUS_TEMPERATURE,
> .limit = temp_limit_attrs2,
> .nlimit = ARRAY_SIZE(temp_limit_attrs2),
> @@ -1820,7 +1792,7 @@ static const struct pmbus_sensor_attr temp_attributes[] = {
> .compare = true,
> .func = PMBUS_HAVE_TEMP3,
> .sfunc = PMBUS_HAVE_STATUS_TEMP,
> - .sbase = PB_STATUS_TEMP_BASE,
> + .sreg = PMBUS_STATUS_TEMPERATURE,
> .gbit = PB_STATUS_TEMPERATURE,
> .limit = temp_limit_attrs3,
> .nlimit = ARRAY_SIZE(temp_limit_attrs3),
> @@ -1945,19 +1917,19 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> if ((info->func[page] & pmbus_fan_status_flags[f]) &&
> pmbus_check_byte_register(client,
> page, pmbus_fan_status_registers[f])) {
> - int base;
> + int reg;
>
> if (f > 1) /* fan 3, 4 */
> - base = PB_STATUS_FAN34_BASE + page;
> + reg = PMBUS_STATUS_FAN_34;
> else
> - base = PB_STATUS_FAN_BASE + page;
> + reg = PMBUS_STATUS_FAN_12;
> ret = pmbus_add_boolean(data, "fan",
> - "alarm", index, NULL, NULL, base,
> + "alarm", index, NULL, NULL, page, reg,
> PB_FAN_FAN1_WARNING >> (f & 1));
> if (ret)
> return ret;
> ret = pmbus_add_boolean(data, "fan",
> - "fault", index, NULL, NULL, base,
> + "fault", index, NULL, NULL, page, reg,
> PB_FAN_FAN1_FAULT >> (f & 1));
> if (ret)
> return ret;
> --
> 2.17.1
>
