Re: [v3 2/3] platform/x86/amd/hsmp: Report power via hwmon sensors

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9741 bytes --]

On Wed, 30 Apr 2025, Suma Hegde wrote:

> Expose power reading and power limits via hwmon power sensors.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v2:
> 1. Remove hwmon related documentation content from patch 3 and add that content
>    in this patch. Add space before starting paranthesis in power1_xxx,
>    remove extra blank line before HSMP HWMON interface section.
> 2. Remove double semicolon at the end of hsmp_create_sensor()
> 3. Place hsmp_create_sensor() stub's code right after the function
> 4. Include err.h and types.h header files in hwmon.c
> 5. Initialize struct hsmp_message using {} instead of {0} everywhere
> 6. Convert power value to milli watt before writing it to SMU
> 7. Remove return 0 from hsmp_hwmon_is_visble()
> 8. Multiply power value by 1000 before sending it back to hwmon
>    subsystem
> 9. Add semicolon after hsmp_info in hsmp_chip_info declaration
> 10.Replace IS_ERR() with PTR_ERR_OR_ZERO()
> 11. uintptr_t is retained to avoid compiler warning
> 12. remove parenthesis around sock_ind parameter in
>     devm_hwmon_device_register_with_info()
> 
> Changes since v1:
> 1. Move hsmp_create_sensor() call to init_acpi() in acpi.c and init_platform_device() in plat.c
> 2. Pass u16 as parameter instead of void * in hsmp_create_sensor()
> 3. Change dev_err() print after hsmp_create_sensor()
> 4. Add CONFIG_HWMON dependency in Makefile
> 5. Add #if IS_REACHABLE(CONFIG_HWMON) condition check in hsmp.h
> 6. Remove hsmp_hwmon struct in hsmp.h and add hwmon_channel_info and hwmon_chip_info to
>    hwmon.c file as static variables
> 7. Change argument to devm_hwmon_device_register_with_info()
> 8. Remove hsmp_create_power_sensor() and define power info statically. Instead of multiple channel,
>    use single channel with different attributes.
> 9. Replace switch with if in hsmp_hwmon_is_visble()
> 10. Remove referencing channel related code in hsmp_hwmon_read() and add code for attribute checking.
> 11. Replace switch with if in hsmp_hwmon_read()
> 12. Remove hsmp_hwmon_read_label().
> 13. Update hsmp_hwmon_write() to remove switch and code related to channel
> 14. Remove int-ll64.h header
> 15. Update the documentation
> 
>  Documentation/arch/x86/amd_hsmp.rst    |   8 ++
>  drivers/platform/x86/amd/hsmp/Makefile |   1 +
>  drivers/platform/x86/amd/hsmp/acpi.c   |   4 +
>  drivers/platform/x86/amd/hsmp/hsmp.h   |   8 +-
>  drivers/platform/x86/amd/hsmp/hwmon.c  | 122 +++++++++++++++++++++++++
>  drivers/platform/x86/amd/hsmp/plat.c   |   5 +
>  6 files changed, 147 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/platform/x86/amd/hsmp/hwmon.c
> 
> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
> index 2fd917638e42..3ef3e0a71df9 100644
> --- a/Documentation/arch/x86/amd_hsmp.rst
> +++ b/Documentation/arch/x86/amd_hsmp.rst
> @@ -116,6 +116,14 @@ for socket with ID00 is given below::
>  			})
>  		}
>  
> +HSMP HWMON interface
> +====================
> +HSMP power sensors are registered with the hwmon interface. A separate hwmon
> +directory is created for each socket and the following files are generated
> +within the hwmon directory.
> +- power1_input (read only)
> +- power1_cap_max (read only)
> +- power1_cap (read, write)
>  
>  An example
>  ==========
> diff --git a/drivers/platform/x86/amd/hsmp/Makefile b/drivers/platform/x86/amd/hsmp/Makefile
> index 0759bbcd13f6..ce8342e71f50 100644
> --- a/drivers/platform/x86/amd/hsmp/Makefile
> +++ b/drivers/platform/x86/amd/hsmp/Makefile
> @@ -6,6 +6,7 @@
>  
>  obj-$(CONFIG_AMD_HSMP)			+= hsmp_common.o
>  hsmp_common-y				:= hsmp.o
> +hsmp_common-$(CONFIG_HWMON)		+= hwmon.o
>  obj-$(CONFIG_AMD_HSMP_PLAT)		+= amd_hsmp.o
>  amd_hsmp-y				:= plat.o
>  obj-$(CONFIG_AMD_HSMP_ACPI)		+= hsmp_acpi.o
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 12f4950afcd9..93b413e0a6e6 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -281,6 +281,10 @@ static int init_acpi(struct device *dev)
>  			dev_err(dev, "Failed to init metric table\n");
>  	}
>  
> +	ret = hsmp_create_sensor(dev, sock_ind);
> +	if (ret)
> +		dev_err(dev, "Failed to register HSMP sensors with hwmon\n");
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 7877cb97993b..02eeebfcb165 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -12,6 +12,7 @@
>  
>  #include <linux/compiler_types.h>
>  #include <linux/device.h>
> +#include <linux/hwmon.h>
>  #include <linux/miscdevice.h>
>  #include <linux/pci.h>
>  #include <linux/semaphore.h>
> @@ -25,7 +26,7 @@
>  #define HSMP_DEVNODE_NAME	"hsmp"
>  #define ACPI_HSMP_DEVICE_HID    "AMDI0097"
>  
> -#define DRIVER_VERSION		"2.4"
> +#define DRIVER_VERSION		"2.5"
>  
>  struct hsmp_mbaddr_info {
>  	u32 base_addr;
> @@ -63,4 +64,9 @@ int hsmp_misc_register(struct device *dev);
>  int hsmp_get_tbl_dram_base(u16 sock_ind);
>  ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
>  struct hsmp_plat_device *get_hsmp_pdev(void);
> +#if IS_REACHABLE(CONFIG_HWMON)
> +int hsmp_create_sensor(struct device *dev, u16 sock_ind);
> +#else
> +int hsmp_create_sensor(struct device *dev, u16 sock_ind) { return 0; }
> +#endif
>  #endif /* HSMP_H */
> diff --git a/drivers/platform/x86/amd/hsmp/hwmon.c b/drivers/platform/x86/amd/hsmp/hwmon.c
> new file mode 100644
> index 000000000000..93827c38f169
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/hwmon.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD HSMP hwmon support
> + * Copyright (c) 2025, AMD.
> + * All Rights Reserved.
> + *
> + * This file provides hwmon implementation for HSMP interface.
> + */
> +
> +#include <asm/amd_hsmp.h>
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/types.h>
> +
> +#include "hsmp.h"
> +
> +#define HSMP_HWMON_NAME		"amd_hsmp_hwmon"
> +
> +static int hsmp_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
> +{
> +	u16 sock_ind = (uintptr_t)dev_get_drvdata(dev);
> +	struct hsmp_message msg = {};
> +
> +	if (type != hwmon_power)
> +		return -EOPNOTSUPP;
> +
> +	if (attr != hwmon_power_cap)
> +		return -EOPNOTSUPP;
> +
> +	msg.num_args = 1;
> +	/* Convert the power value to mWatt from µWatt */
> +	msg.args[0] = val / 1000;

MICROWATT_PER_MILLIWATT, don't forget to add the #include. Comment is 
unnecessary then.

> +	msg.msg_id = HSMP_SET_SOCKET_POWER_LIMIT;
> +	msg.sock_ind = sock_ind;
> +	return hsmp_send_message(&msg);
> +}
> +
> +static int hsmp_hwmon_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	u16 sock_ind = (uintptr_t)dev_get_drvdata(dev);
> +	struct hsmp_message msg = {};
> +	int ret;
> +
> +	if (type != hwmon_power)
> +		return -EOPNOTSUPP;
> +
> +	msg.sock_ind = sock_ind;
> +	msg.response_sz = 1;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		msg.msg_id = HSMP_GET_SOCKET_POWER;
> +		break;
> +	case hwmon_power_cap:
> +		msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT;
> +		break;
> +	case hwmon_power_cap_max:
> +		msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT_MAX;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = hsmp_send_message(&msg);
> +	if (!ret)
> +		/* Convert the power value to µWatt from mWatt */
> +		*val = msg.args[0] * 1000;

Ditto.

> +
> +	return ret;
> +}
> +
> +static umode_t hsmp_hwmon_is_visble(const void *data,
> +				    enum hwmon_sensor_types type,
> +				    u32 attr, int channel)
> +{
> +	if (type != hwmon_power)
> +		return 0;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		return 0444;
> +	case hwmon_power_cap:
> +		return 0644;
> +	case hwmon_power_cap_max:
> +		return 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct hwmon_ops hsmp_hwmon_ops = {
> +	.read = hsmp_hwmon_read,
> +	.is_visible = hsmp_hwmon_is_visble,
> +	.write	= hsmp_hwmon_write,
> +};
> +
> +static const struct hwmon_channel_info * const hsmp_info[] = {
> +	HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_CAP | HWMON_P_CAP_MAX),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info hsmp_chip_info = {
> +	.ops = &hsmp_hwmon_ops,
> +	.info = hsmp_info,
> +};
> +
> +int hsmp_create_sensor(struct device *dev, u16 sock_ind)
> +{
> +	struct device *hwmon_dev;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, HSMP_HWMON_NAME,
> +							 (void *)(uintptr_t)sock_ind,
> +							 &hsmp_chip_info,
> +							 NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +EXPORT_SYMBOL_NS(hsmp_create_sensor, "AMD_HSMP");
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index 4f03fdf988c1..0881d7e01936 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -189,6 +189,11 @@ static int init_platform_device(struct device *dev)
>  			if (ret)
>  				dev_err(dev, "Failed to init metric table\n");
>  		}
> +
> +		/* Register with hwmon interface for reporting power */
> +		ret = hsmp_create_sensor(dev, i);
> +		if (ret)
> +			dev_err(dev, "Failed to register HSMP sensors with hwmon\n");
>  	}
>  
>  	return 0;
> 

-- 
 i.