[PATCH 0/3] hwmon: (pmbus) add power from energy readings

[PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Kallas, Pawel]

Add support for reading EIN or EOUT registers and expose power calculated
from energy. This is more accurate than PIN and POUT power readings.
Readings are exposed in new hwmon files power1_average and power2_average.
Also add support for QUERY command that is needed to check availability
of EIN and EOUT reads and its data format. Only direct data format is
supported due to lack of test devices supporting other formats.

Kallas, Pawel (3):
  hwmon: (pmbus) add support for QUERY command
  hwmon: (pmbus) refactor sensor initialization
  hwmon: (pmbus) add EIN and EOUT readings

 Documentation/hwmon/pmbus-core.rst |   7 +
 drivers/hwmon/pmbus/pmbus.c        |  20 +++
 drivers/hwmon/pmbus/pmbus.h        |  19 +++
 drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
 4 files changed, 291 insertions(+), 16 deletions(-)


base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb

[PATCH 1/3] hwmon: (pmbus) add support for QUERY command

[Kallas, Pawel]

QUERY command is used for checking if given command is supported by
the device and what data format it uses. It is needed to check if
READ_EIN and READ_EOUT commands are supported.

Signed-off-by: Kallas, Pawel <pawel.kallas@intel.com>
---
 Documentation/hwmon/pmbus-core.rst |  7 +++++++
 drivers/hwmon/pmbus/pmbus.h        | 14 ++++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c   | 21 +++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/hwmon/pmbus-core.rst b/Documentation/hwmon/pmbus-core.rst
index e7e0c9ef10bec..6ba0a9d86f1f6 100644
--- a/Documentation/hwmon/pmbus-core.rst
+++ b/Documentation/hwmon/pmbus-core.rst
@@ -268,6 +268,13 @@ otherwise.
 This function calls the device specific write_byte function if defined to
 obtain the chip status. Therefore, it must _not_ be called from that function.
 
+::
+
+  int pmbus_query_register(struct i2c_client *client, int reg);
+
+Send pmbus QUERY command for specific register. Returns QUERY command
+response or negative value on fail.
+
 ::
 
   int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info);
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 75aa97b1ecc05..971554f40dba6 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -364,6 +364,19 @@ enum pmbus_fan_mode { percent = 0, rpm };
 #define PB_CML_FAULT_INVALID_DATA	BIT(6)
 #define PB_CML_FAULT_INVALID_COMMAND	BIT(7)
 
+/*
+ * QUERY
+ */
+#define PB_QUERY_COMMAND_MODE_MASK	0x1C
+
+#define PB_QUERY_COMMAND_MODE_LINEAR	0x00
+#define PB_QUERY_COMMAND_MODE_DIRECT	0x0C
+#define PB_QUERY_COMMAND_MODE_VID	0x14
+
+#define PB_QUERY_COMMAND_SUPPORTED_FOR_READ	BIT(5)
+#define PB_QUERY_COMMAND_SUPPORTED_FOR_WRITE	BIT(6)
+#define PB_QUERY_COMMAND_SUPPORTED	BIT(7)
+
 enum pmbus_sensor_classes {
 	PSC_VOLTAGE_IN = 0,
 	PSC_VOLTAGE_OUT,
@@ -492,6 +505,7 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
+int pmbus_query_register(struct i2c_client *client, int reg);
 int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info);
 const struct pmbus_driver_info *pmbus_get_driver_info(struct i2c_client
 						      *client);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index d462f732f3b40..4bcb70ab9b598 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -648,6 +648,27 @@ static int pmbus_get_status(struct i2c_client *client, int page, int reg)
 	return status;
 }
 
+int pmbus_query_register(struct i2c_client *client, int reg)
+{
+	int rv;
+	union i2c_smbus_data data;
+
+	data.block[0] = 1;
+	data.block[1] = reg;
+
+	rv = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			    I2C_SMBUS_WRITE, PMBUS_QUERY,
+			    I2C_SMBUS_BLOCK_PROC_CALL, &data);
+	if (rv < 0)
+		return rv;
+
+	if (data.block[0] != 1)
+		return -EIO;
+
+	return data.block[1];
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_query_register, PMBUS);
+
 static void pmbus_update_sensor_data(struct i2c_client *client, struct pmbus_sensor *sensor)
 {
 	if (sensor->data < 0 || sensor->update)

[PATCH 2/3] hwmon: (pmbus) refactor sensor initialization

[Kallas, Pawel]

Introduce separate function to initialize pmbus sensor attributes.
It can be used to initialize different sensor types.

Signed-off-by: Kallas, Pawel <pawel.kallas@intel.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 37 ++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 4bcb70ab9b598..6e3ec6a223b92 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1168,6 +1168,26 @@ static int pmbus_add_boolean(struct pmbus_data *data,
 	return pmbus_add_attribute(data, &a->dev_attr.attr);
 }
 
+static void pmbus_sensor_init(struct pmbus_sensor *sensor, const char *name, const char *type,
+			      int seq, int page, int phase, int reg,
+			      enum pmbus_sensor_classes class, bool update, bool convert)
+{
+	if (type)
+		snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
+			 name, seq, type);
+	else
+		snprintf(sensor->name, sizeof(sensor->name), "%s%d",
+			 name, seq);
+
+	sensor->page = page;
+	sensor->phase = phase;
+	sensor->reg = reg;
+	sensor->class = class;
+	sensor->update = update;
+	sensor->convert = convert;
+	sensor->data = -ENODATA;
+}
+
 static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
 					     const char *name, const char *type,
 					     int seq, int page, int phase,
@@ -1182,25 +1202,14 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
 	sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
 		return NULL;
-	a = &sensor->attribute;
 
-	if (type)
-		snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
-			 name, seq, type);
-	else
-		snprintf(sensor->name, sizeof(sensor->name), "%s%d",
-			 name, seq);
+	pmbus_sensor_init(sensor, name, type, seq, page, phase, reg, class, update, convert);
 
 	if (data->flags & PMBUS_WRITE_PROTECTED)
 		readonly = true;
 
-	sensor->page = page;
-	sensor->phase = phase;
-	sensor->reg = reg;
-	sensor->class = class;
-	sensor->update = update;
-	sensor->convert = convert;
-	sensor->data = -ENODATA;
+	a = &sensor->attribute;
+
 	pmbus_dev_attr_init(a, sensor->name,
 			    readonly ? 0444 : 0644,
 			    pmbus_show_sensor, pmbus_set_sensor);

[PATCH 3/3] hwmon: (pmbus) add EIN and EOUT readings

[Kallas, Pawel]

Add new sensor type to pmbus hwmon driver. The sensor reads EIN and
EOUT registers and calculates power based on energy reads. New hwmon
file names are power1_average for power from EIN and power2_average for
power from EOUT.
Perform automatic detection of support for EIN and EOUT in generic
pmbus device driver. Use QUERY pmbus command to check if EIN or
EOUT commands are supported and what format of data they use. Support
only direct format with coefficient m equal to 1. Other formats not
tested due to lack of devices supporting them.

Signed-off-by: Kallas, Pawel <pawel.kallas@intel.com>
---
 drivers/hwmon/pmbus/pmbus.c      |  20 +++
 drivers/hwmon/pmbus/pmbus.h      |   5 +
 drivers/hwmon/pmbus/pmbus_core.c | 203 ++++++++++++++++++++++++++++++-
 3 files changed, 226 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index ce2f020f09d74..81292541969a7 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -30,6 +30,7 @@ static void pmbus_find_sensor_groups(struct i2c_client *client,
 {
 	int page;
 	int fan_mode;
+	int rv;
 
 	/* Sensors detected on page 0 only */
 	if (pmbus_check_word_register(client, 0, PMBUS_READ_VIN))
@@ -74,6 +75,25 @@ static void pmbus_find_sensor_groups(struct i2c_client *client,
 					 PMBUS_STATUS_TEMPERATURE))
 			info->func[0] |= PMBUS_HAVE_STATUS_TEMP;
 
+	rv = pmbus_query_register(client, PMBUS_READ_EIN);
+	/* only direct format for EIN and EOUT supported */
+	if (rv > 0 && (rv & PB_QUERY_COMMAND_SUPPORTED) &&
+	    (rv & PB_QUERY_COMMAND_SUPPORTED_FOR_READ) &&
+		((rv & PB_QUERY_COMMAND_MODE_MASK) == PB_QUERY_COMMAND_MODE_DIRECT)) {
+		info->func[0] |= PMBUS_HAVE_EIN;
+		info->format[PSC_POWER_AVERAGE] = direct;
+		info->m[PSC_POWER_AVERAGE] = 1;
+	}
+
+	rv = pmbus_query_register(client, PMBUS_READ_EOUT);
+	if (rv > 0 && (rv & PB_QUERY_COMMAND_SUPPORTED) &&
+	    (rv & PB_QUERY_COMMAND_SUPPORTED_FOR_READ) &&
+		((rv & PB_QUERY_COMMAND_MODE_MASK) == PB_QUERY_COMMAND_MODE_DIRECT)) {
+		info->func[0] |= PMBUS_HAVE_EOUT;
+		info->format[PSC_POWER_AVERAGE] = direct;
+		info->m[PSC_POWER_AVERAGE] = 1;
+	}
+
 	/* Sensors detected on all pages */
 	for (page = 0; page < info->pages; page++) {
 		if (pmbus_check_word_register(client, page, PMBUS_READ_VOUT)) {
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 971554f40dba6..eaf0478d6ec15 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -94,6 +94,8 @@ enum pmbus_regs {
 	PMBUS_STATUS_FAN_12		= 0x81,
 	PMBUS_STATUS_FAN_34		= 0x82,
 
+	PMBUS_READ_EIN			= 0x86,
+	PMBUS_READ_EOUT			= 0x87,
 	PMBUS_READ_VIN			= 0x88,
 	PMBUS_READ_IIN			= 0x89,
 	PMBUS_READ_VCAP			= 0x8A,
@@ -383,6 +385,7 @@ enum pmbus_sensor_classes {
 	PSC_CURRENT_IN,
 	PSC_CURRENT_OUT,
 	PSC_POWER,
+	PSC_POWER_AVERAGE,
 	PSC_TEMPERATURE,
 	PSC_FAN,
 	PSC_PWM,
@@ -416,6 +419,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
 #define PMBUS_HAVE_SAMPLES	BIT(22)
+#define PMBUS_HAVE_EIN		BIT(23)
+#define PMBUS_HAVE_EOUT		BIT(24)
 
 #define PMBUS_PHASE_VIRTUAL	BIT(30)	/* Phases on this page are virtual */
 #define PMBUS_PAGE_VIRTUAL	BIT(31)	/* Page is virtual */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 6e3ec6a223b92..d6a6640a1779e 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -44,6 +44,15 @@ struct pmbus_sensor {
 #define to_pmbus_sensor(_attr) \
 	container_of(_attr, struct pmbus_sensor, attribute)
 
+struct pmbus_power_average_sensor {
+	struct pmbus_sensor sensor;
+	u64 energy_count;
+	u32 sample_count;
+};
+
+#define to_pmbus_power_average_sensor(_attr) \
+	container_of(to_pmbus_sensor(_attr), struct pmbus_power_average_sensor, sensor)
+
 struct pmbus_boolean {
 	char name[PMBUS_NAME_SIZE];	/* sysfs boolean name */
 	struct sensor_device_attribute attribute;
@@ -702,7 +711,7 @@ static s64 pmbus_reg2data_linear(struct pmbus_data *data,
 		val = val * 1000LL;
 
 	/* scale result to micro-units for power sensors */
-	if (sensor->class == PSC_POWER)
+	if (sensor->class == PSC_POWER || sensor->class == PSC_POWER_AVERAGE)
 		val = val * 1000LL;
 
 	if (exponent >= 0)
@@ -739,7 +748,7 @@ static s64 pmbus_reg2data_direct(struct pmbus_data *data,
 	}
 
 	/* scale result to micro-units for power sensors */
-	if (sensor->class == PSC_POWER) {
+	if (sensor->class == PSC_POWER || sensor->class == PSC_POWER_AVERAGE) {
 		R += 3;
 		b *= 1000;
 	}
@@ -814,6 +823,21 @@ static s64 pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor)
 	return val;
 }
 
+static u64 pmbus_energy_count(struct pmbus_data *data,
+			      struct pmbus_power_average_sensor *sensor,
+			      u32 accumulator_current,
+			      u8 rollover)
+{
+	s64 val;
+	u64 accumulator_max;
+
+	sensor->sensor.data = accumulator_current;
+	val = pmbus_reg2data(data, &sensor->sensor);
+	sensor->sensor.data = 0x7FFF;
+	accumulator_max = (u64)pmbus_reg2data(data, &sensor->sensor);
+	return rollover * accumulator_max + val;
+}
+
 #define MAX_MANTISSA	(1023 * 1000)
 #define MIN_MANTISSA	(511 * 1000)
 
@@ -1056,6 +1080,54 @@ static ssize_t pmbus_show_sensor(struct device *dev,
 	return ret;
 }
 
+static ssize_t pmbus_show_power_average_sensor(struct device *dev,
+					       struct device_attribute *devattr, char *buf)
+{
+	struct pmbus_power_average_sensor *sensor = to_pmbus_power_average_sensor(devattr);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_data *data = i2c_get_clientdata(client);
+	u64 last_energy_count = sensor->energy_count;
+	u32 last_sample_count = sensor->sample_count;
+	u8 buffer[I2C_SMBUS_BLOCK_MAX];
+	s16 accumulator_current;
+	u64 energy_count_diff;
+	u32 sample_count_diff;
+	ssize_t ret;
+
+	mutex_lock(&data->update_lock);
+	ret = pmbus_set_page(client, sensor->sensor.page, sensor->sensor.phase);
+	if (ret < 0)
+		goto unlock;
+
+	ret = i2c_smbus_read_i2c_block_data(client, sensor->sensor.reg, 8, buffer);
+	if (ret < 0)
+		goto unlock;
+
+	accumulator_current = buffer[1] + (buffer[2] << 8);
+	sensor->sample_count = buffer[4] + (buffer[5] << 8) + (buffer[6] << 16);
+	sensor->energy_count = pmbus_energy_count(data, sensor, accumulator_current, buffer[3]);
+
+	if (sensor->sample_count == last_sample_count) {
+		ret = -ENODATA;
+		goto unlock;
+	}
+	if (sensor->energy_count < last_energy_count)
+		energy_count_diff = sensor->energy_count +
+			(pmbus_energy_count(data, sensor, 0xFFFF, 0xFF) + 1) - last_energy_count;
+	else
+		energy_count_diff = sensor->energy_count - last_energy_count;
+
+	if (sensor->sample_count < last_sample_count)
+		sample_count_diff = sensor->sample_count + (0xFFFFFF + 1) - last_sample_count;
+	else
+		sample_count_diff = sensor->sample_count - last_sample_count;
+
+	ret = sysfs_emit(buf, "%llu\n", div_u64(energy_count_diff, sample_count_diff));
+unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
 static ssize_t pmbus_set_sensor(struct device *dev,
 				struct device_attribute *devattr,
 				const char *buf, size_t count)
@@ -1223,6 +1295,40 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
 	return sensor;
 }
 
+static struct pmbus_power_average_sensor
+*pmbus_add_power_average_sensor(struct pmbus_data *data, const char *name, const char *type,
+				int seq, int page, int phase, int reg,
+				enum pmbus_sensor_classes class, bool update, bool readonly,
+				bool convert)
+{
+	struct pmbus_power_average_sensor *sensor;
+	struct device_attribute *a;
+
+	sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return NULL;
+
+	pmbus_sensor_init(&sensor->sensor, name, type, seq, page, phase, reg, class, update,
+			  convert);
+
+	if (data->flags & PMBUS_WRITE_PROTECTED)
+		readonly = true;
+
+	a = &sensor->sensor.attribute;
+
+	pmbus_dev_attr_init(a, sensor->sensor.name,
+			    readonly ? 0444 : 0644,
+			    pmbus_show_power_average_sensor, NULL);
+
+	if (pmbus_add_attribute(data, &a->attr))
+		return NULL;
+
+	sensor->sensor.next = data->sensors;
+	data->sensors = &sensor->sensor;
+
+	return sensor;
+}
+
 static int pmbus_add_label(struct pmbus_data *data,
 			   const char *name, int seq,
 			   const char *lstring, int index, int phase)
@@ -1415,6 +1521,23 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 	return 0;
 }
 
+static int pmbus_add_power_average_sensor_attrs_one(struct i2c_client *client,
+						    struct pmbus_data *data,
+						    const struct pmbus_driver_info *info,
+						    const char *name, int index, int page,
+						    int phase,
+						    const struct pmbus_sensor_attr *attr,
+						    bool paged)
+{
+	struct pmbus_power_average_sensor *base;
+
+	base = pmbus_add_power_average_sensor(data, name, "average", index, page, phase,
+					      attr->reg, attr->class, true, true, true);
+	if (!base)
+		return -ENOMEM;
+	return 0;
+}
+
 static bool pmbus_sensor_is_paged(const struct pmbus_driver_info *info,
 				  const struct pmbus_sensor_attr *attr)
 {
@@ -1826,6 +1949,26 @@ static const struct pmbus_sensor_attr power_attributes[] = {
 	}
 };
 
+static const struct pmbus_sensor_attr power_average_attributes[] = {
+	{
+		.reg = PMBUS_READ_EIN,
+		.class = PSC_POWER_AVERAGE,
+		.paged = true,
+		.update = true,
+		.compare = true,
+		.label = "ein",
+		.func = PMBUS_HAVE_EIN,
+	}, {
+		.reg = PMBUS_READ_EOUT,
+		.class = PSC_POWER_AVERAGE,
+		.paged = true,
+		.update = true,
+		.compare = true,
+		.label = "eout",
+		.func = PMBUS_HAVE_EOUT,
+	}
+};
+
 /* Temperature atributes */
 
 static const struct pmbus_limit_attr temp_limit_attrs[] = {
@@ -2120,6 +2263,53 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 	return 0;
 }
 
+static int pmbus_add_power_average_sensor_attrs(struct i2c_client *client,
+						struct pmbus_data *data)
+{
+	const struct pmbus_sensor_attr *attrs = power_average_attributes;
+	const struct pmbus_driver_info *info = data->info;
+	int index = 1;
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(power_average_attributes); i++) {
+		int page, pages;
+		bool paged = pmbus_sensor_is_paged(info, attrs);
+
+		pages = paged ? info->pages : 1;
+		for (page = 0; page < pages; page++) {
+			if (info->func[page] & attrs->func) {
+				ret = pmbus_add_power_average_sensor_attrs_one(client, data, info,
+									       "power", index,
+									       page, 0xff, attrs,
+									       paged);
+				if (ret)
+					return ret;
+				index++;
+			}
+			if (info->phases[page]) {
+				int phase;
+
+				for (phase = 0; phase < info->phases[page]; phase++) {
+					if (!(info->pfunc[phase] & attrs->func))
+						continue;
+					ret = pmbus_add_power_average_sensor_attrs_one(client,
+										       data, info,
+										       "power",
+										       index,
+										       page, phase,
+										       attrs,
+										       paged);
+					if (ret)
+						return ret;
+					index++;
+				}
+			}
+		}
+		attrs++;
+	}
+	return 0;
+}
+
 struct pmbus_samples_attr {
 	int reg;
 	char *name;
@@ -2266,6 +2456,11 @@ static int pmbus_find_attributes(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Power average */
+	ret = pmbus_add_power_average_sensor_attrs(client, data);
+	if (ret)
+		return ret;
+
 	ret = pmbus_add_samples_attributes(client, data);
 	return ret;
 }
@@ -2305,6 +2500,10 @@ static const struct pmbus_class_attr_map class_attr_map[] = {
 		.class = PSC_TEMPERATURE,
 		.attr = temp_attributes,
 		.nattr = ARRAY_SIZE(temp_attributes),
+	}, {
+		.class = PSC_POWER_AVERAGE,
+		.attr = power_average_attributes,
+		.nattr = ARRAY_SIZE(power_average_attributes),
 	}
 };
 

Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Guenter Roeck]

On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
> Add support for reading EIN or EOUT registers and expose power calculated
> from energy. This is more accurate than PIN and POUT power readings.
> Readings are exposed in new hwmon files power1_average and power2_average.
> Also add support for QUERY command that is needed to check availability
> of EIN and EOUT reads and its data format. Only direct data format is
> supported due to lack of test devices supporting other formats.
> 

I don't think this is a good idea. EIN/EOUT report energy consumption,
not power. The "average" attributes as implemented don't really report
a reliable number since the averaging period is not defined. Also, kernel
drivers should not make up such numbers. I don't mind adding energy
attribute support, but that should be reported as what it is, energy.
What userspace does with it would then be a userspace concern; it can
calculate all kinds of averages from it as much as it wants.

Also, new attributes should not depend on query command support.
I don't mind adding support for that, but it would have to be independent
of energy attribute support.

Thanks,
Guenter

> Kallas, Pawel (3):
>   hwmon: (pmbus) add support for QUERY command
>   hwmon: (pmbus) refactor sensor initialization
>   hwmon: (pmbus) add EIN and EOUT readings
> 
>  Documentation/hwmon/pmbus-core.rst |   7 +
>  drivers/hwmon/pmbus/pmbus.c        |  20 +++
>  drivers/hwmon/pmbus/pmbus.h        |  19 +++
>  drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
>  4 files changed, 291 insertions(+), 16 deletions(-)
> 
> 
> base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb

Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Kallas, Pawel]

On 06-Jul-22 3:17 PM, Guenter Roeck wrote:
> On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
>> Add support for reading EIN or EOUT registers and expose power calculated
>> from energy. This is more accurate than PIN and POUT power readings.
>> Readings are exposed in new hwmon files power1_average and power2_average.
>> Also add support for QUERY command that is needed to check availability
>> of EIN and EOUT reads and its data format. Only direct data format is
>> supported due to lack of test devices supporting other formats.
>>
> I don't think this is a good idea. EIN/EOUT report energy consumption,
> not power.

According to PMBus-Specification-Rev-1-3-1-Part-II-20150313 "READ_EIN and
READ_EOUT commands provide information that can be used to calculate power
consumption". That is accumulator summing instantaneous input power
expressed in "watt-samples" and counter indicating number of samples.
The only reasonable thing that can be done with those values is 
calculating power.

> The "average" attributes as implemented don't really report
> a reliable number since the averaging period is not defined.

Agree, it is calculating average power since last read, which could be
incorrect with multiple consumers. However, this is the only possibility
without adding some timer logic.

> Also, kernel
> drivers should not make up such numbers. I don't mind adding energy
> attribute support, but that should be reported as what it is, energy.
> What userspace does with it would then be a userspace concern; it can
> calculate all kinds of averages from it as much as it wants.

Returning direct value of read registers would also work for our use case,
but it is not in line with sysfs interface.

> Also, new attributes should not depend on query command support.
> I don't mind adding support for that, but it would have to be independent
> of energy attribute support.
>
> Thanks,
> Guenter
>
>> Kallas, Pawel (3):
>>    hwmon: (pmbus) add support for QUERY command
>>    hwmon: (pmbus) refactor sensor initialization
>>    hwmon: (pmbus) add EIN and EOUT readings
>>
>>   Documentation/hwmon/pmbus-core.rst |   7 +
>>   drivers/hwmon/pmbus/pmbus.c        |  20 +++
>>   drivers/hwmon/pmbus/pmbus.h        |  19 +++
>>   drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
>>   4 files changed, 291 insertions(+), 16 deletions(-)
>>
>>
>> base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb

Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Guenter Roeck]

On Thu, Jul 07, 2022 at 04:01:54PM +0200, Kallas, Pawel wrote:
> On 06-Jul-22 3:17 PM, Guenter Roeck wrote:
> > On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
> > > Add support for reading EIN or EOUT registers and expose power calculated
> > > from energy. This is more accurate than PIN and POUT power readings.
> > > Readings are exposed in new hwmon files power1_average and power2_average.
> > > Also add support for QUERY command that is needed to check availability
> > > of EIN and EOUT reads and its data format. Only direct data format is
> > > supported due to lack of test devices supporting other formats.
> > > 
> > I don't think this is a good idea. EIN/EOUT report energy consumption,
> > not power.
> 
> According to PMBus-Specification-Rev-1-3-1-Part-II-20150313 "READ_EIN and
> READ_EOUT commands provide information that can be used to calculate power
> consumption". That is accumulator summing instantaneous input power
> expressed in "watt-samples" and counter indicating number of samples.
> The only reasonable thing that can be done with those values is calculating
> power.

Yes, but that is not the responsibility of the kernel. Just like we don't add
up power measurements to calculate energy, we don't take energy measurements
and calculate power consumption. Similar, we don't take voltage and current
measurements and report power consumption from it either.

> 
> > The "average" attributes as implemented don't really report
> > a reliable number since the averaging period is not defined.
> 
> Agree, it is calculating average power since last read, which could be
> incorrect with multiple consumers. However, this is the only possibility
> without adding some timer logic.

Another reason for doing it in userspace. Read energy every N seconds, and use
the difference to calculate average power consumption average over that time
period.

> 
> > Also, kernel
> > drivers should not make up such numbers. I don't mind adding energy
> > attribute support, but that should be reported as what it is, energy.
> > What userspace does with it would then be a userspace concern; it can
> > calculate all kinds of averages from it as much as it wants.
> 
> Returning direct value of read registers would also work for our use case,
> but it is not in line with sysfs interface.

I did not suggest that. Just use the "energyX_in" attributes.

Thanks,
Guenter

> 
> > Also, new attributes should not depend on query command support.
> > I don't mind adding support for that, but it would have to be independent
> > of energy attribute support.
> > 
> > Thanks,
> > Guenter
> > 
> > > Kallas, Pawel (3):
> > >    hwmon: (pmbus) add support for QUERY command
> > >    hwmon: (pmbus) refactor sensor initialization
> > >    hwmon: (pmbus) add EIN and EOUT readings
> > > 
> > >   Documentation/hwmon/pmbus-core.rst |   7 +
> > >   drivers/hwmon/pmbus/pmbus.c        |  20 +++
> > >   drivers/hwmon/pmbus/pmbus.h        |  19 +++
> > >   drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
> > >   4 files changed, 291 insertions(+), 16 deletions(-)
> > > 
> > > 
> > > base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb

Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Kallas, Pawel]

On 07-Jul-22 4:09 PM, Guenter Roeck wrote:
> On Thu, Jul 07, 2022 at 04:01:54PM +0200, Kallas, Pawel wrote:
>> On 06-Jul-22 3:17 PM, Guenter Roeck wrote:
>>> On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
>>>> Add support for reading EIN or EOUT registers and expose power calculated
>>>> from energy. This is more accurate than PIN and POUT power readings.
>>>> Readings are exposed in new hwmon files power1_average and power2_average.
>>>> Also add support for QUERY command that is needed to check availability
>>>> of EIN and EOUT reads and its data format. Only direct data format is
>>>> supported due to lack of test devices supporting other formats.
>>>>
>>> I don't think this is a good idea. EIN/EOUT report energy consumption,
>>> not power.
>> According to PMBus-Specification-Rev-1-3-1-Part-II-20150313 "READ_EIN and
>> READ_EOUT commands provide information that can be used to calculate power
>> consumption". That is accumulator summing instantaneous input power
>> expressed in "watt-samples" and counter indicating number of samples.
>> The only reasonable thing that can be done with those values is calculating
>> power.
> Yes, but that is not the responsibility of the kernel. Just like we don't add
> up power measurements to calculate energy, we don't take energy measurements
> and calculate power consumption. Similar, we don't take voltage and current
> measurements and report power consumption from it either.
>
>>> The "average" attributes as implemented don't really report
>>> a reliable number since the averaging period is not defined.
>> Agree, it is calculating average power since last read, which could be
>> incorrect with multiple consumers. However, this is the only possibility
>> without adding some timer logic.
> Another reason for doing it in userspace. Read energy every N seconds, and use
> the difference to calculate average power consumption average over that time
> period.
We cannot "read energy". Raw value from READ_EIN and READ_EOUT is not 
energy.
>>> Also, kernel
>>> drivers should not make up such numbers. I don't mind adding energy
>>> attribute support, but that should be reported as what it is, energy.
>>> What userspace does with it would then be a userspace concern; it can
>>> calculate all kinds of averages from it as much as it wants.
>> Returning direct value of read registers would also work for our use case,
>> but it is not in line with sysfs interface.
> I did not suggest that. Just use the "energyX_in" attributes.
Expressing raw value from READ_EIN or READ_EOUT is not in line with
sysfs interface, because "energyX_in" should have microJoules as unit.
Those commands have very specific format that is not actually energy.
Since the only sensible use case for those raw values is calculating power
we figured it would be better (and more accurate) to do it in kernel.
Also, if we just express raw value, the user would have to know data format
of the values for the device and know register format to decode the data.
>
> Thanks,
> Guenter
>
>>> Also, new attributes should not depend on query command support.
>>> I don't mind adding support for that, but it would have to be independent
>>> of energy attribute support.
>>>
>>> Thanks,
>>> Guenter
>>>
>>>> Kallas, Pawel (3):
>>>>     hwmon: (pmbus) add support for QUERY command
>>>>     hwmon: (pmbus) refactor sensor initialization
>>>>     hwmon: (pmbus) add EIN and EOUT readings
>>>>
>>>>    Documentation/hwmon/pmbus-core.rst |   7 +
>>>>    drivers/hwmon/pmbus/pmbus.c        |  20 +++
>>>>    drivers/hwmon/pmbus/pmbus.h        |  19 +++
>>>>    drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
>>>>    4 files changed, 291 insertions(+), 16 deletions(-)
>>>>
>>>>
>>>> base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb

Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Guenter Roeck]

On Thu, Jul 07, 2022 at 06:00:45PM +0200, Kallas, Pawel wrote:
> 
> On 07-Jul-22 4:09 PM, Guenter Roeck wrote:
> > On Thu, Jul 07, 2022 at 04:01:54PM +0200, Kallas, Pawel wrote:
> > > On 06-Jul-22 3:17 PM, Guenter Roeck wrote:
> > > > On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
> > > > > Add support for reading EIN or EOUT registers and expose power calculated
> > > > > from energy. This is more accurate than PIN and POUT power readings.
> > > > > Readings are exposed in new hwmon files power1_average and power2_average.
> > > > > Also add support for QUERY command that is needed to check availability
> > > > > of EIN and EOUT reads and its data format. Only direct data format is
> > > > > supported due to lack of test devices supporting other formats.
> > > > > 
> > > > I don't think this is a good idea. EIN/EOUT report energy consumption,
> > > > not power.
> > > According to PMBus-Specification-Rev-1-3-1-Part-II-20150313 "READ_EIN and
> > > READ_EOUT commands provide information that can be used to calculate power
> > > consumption". That is accumulator summing instantaneous input power
> > > expressed in "watt-samples" and counter indicating number of samples.
> > > The only reasonable thing that can be done with those values is calculating
> > > power.
> > Yes, but that is not the responsibility of the kernel. Just like we don't add
> > up power measurements to calculate energy, we don't take energy measurements
> > and calculate power consumption. Similar, we don't take voltage and current
> > measurements and report power consumption from it either.
> > 
> > > > The "average" attributes as implemented don't really report
> > > > a reliable number since the averaging period is not defined.
> > > Agree, it is calculating average power since last read, which could be
> > > incorrect with multiple consumers. However, this is the only possibility
> > > without adding some timer logic.
> > Another reason for doing it in userspace. Read energy every N seconds, and use
> > the difference to calculate average power consumption average over that time
> > period.
> We cannot "read energy". Raw value from READ_EIN and READ_EOUT is not
> energy.

Sure, it is an accumulation of power reading samples over time. as such,
it doesn't really even report a power average. Either case, any value
derived from it is all but worthless unless a well defined time interval
is available.  Unfortunately, such a time interval would require a kernel
timer, which would, at least in low power situations, have impact on the
power readings and is thus unacceptable. Maybe that is why later PMBus
specification introduced explicit READ_KWH_IN and READ_KWH_OUT commands.

> > > > Also, kernel
> > > > drivers should not make up such numbers. I don't mind adding energy
> > > > attribute support, but that should be reported as what it is, energy.
> > > > What userspace does with it would then be a userspace concern; it can
> > > > calculate all kinds of averages from it as much as it wants.
> > > Returning direct value of read registers would also work for our use case,
> > > but it is not in line with sysfs interface.
> > I did not suggest that. Just use the "energyX_in" attributes.
> Expressing raw value from READ_EIN or READ_EOUT is not in line with
> sysfs interface, because "energyX_in" should have microJoules as unit.
> Those commands have very specific format that is not actually energy.
> Since the only sensible use case for those raw values is calculating power
> we figured it would be better (and more accurate) to do it in kernel.
> Also, if we just express raw value, the user would have to know data format
> of the values for the device and know register format to decode the data.

A joule is one watt-second, and the registers accumulate power samples over
a period of time. Sure, dividing the reported values by the time interval
results in the average power consumption over that time interval. Just like
multiplying the average power consumption with the time interval results in
the energy consumption over that timer interval. If we say we can't
determine the energy because the accumulated values are just snapshots in
time, we just as well can't trust the average power calculated from it.

Anyway, I don't really see an acceptable solution. Reporting the average
power would require a periodic function running every second or so which
would at least potentially falsify the reported values, and if you say that
reporting the energy (which might still require a timer function, but less
frequently) isn't feasible I take you by your word.

Guenter

> > 
> > Thanks,
> > Guenter
> > 
> > > > Also, new attributes should not depend on query command support.
> > > > I don't mind adding support for that, but it would have to be independent
> > > > of energy attribute support.
> > > > 
> > > > Thanks,
> > > > Guenter
> > > > 
> > > > > Kallas, Pawel (3):
> > > > >     hwmon: (pmbus) add support for QUERY command
> > > > >     hwmon: (pmbus) refactor sensor initialization
> > > > >     hwmon: (pmbus) add EIN and EOUT readings
> > > > > 
> > > > >    Documentation/hwmon/pmbus-core.rst |   7 +
> > > > >    drivers/hwmon/pmbus/pmbus.c        |  20 +++
> > > > >    drivers/hwmon/pmbus/pmbus.h        |  19 +++
> > > > >    drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
> > > > >    4 files changed, 291 insertions(+), 16 deletions(-)
> > > > > 
> > > > > 
> > > > > base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb