Re: [v4 3/3] platform/x86/amd/hsmp: acpi: Add sysfs files to display HSMP telemetry

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

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

On Tue, 6 May 2025, Suma Hegde wrote:

> Make frequently fetched telemetry available via sysfs. These parameters
> do not fit in hwmon sensor model, hence make them available via sysfs.
> 
> Create following sysfs files per acpi device node.
> * c0_residency_input
> * prochot_status
> * smu_fw_version
> * protocol_version
> * ddr_max_bw(GB/s)
> * ddr_utilised_bw_input(GB/s)
> * ddr_utilised_bw_perc_input(%)
> * mclk_input(MHz)
> * fclk_input(MHz)
> * clk_fmax(MHz)
> * clk_fmin(MHz)
> * cclk_freq_limit_input(MHz)
> * pwr_current_active_freq_limit(MHz)
> * pwr_current_active_freq_limit_source
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v3:
> 1. include array_size.h
> 2. include bitfield.h and bits.h
> 3. Remove bitops.h
> 4. Remove comma from terminator
> 5. Change { 0 } to {} for initializing struct hsmp_message
> 6. Change src_ind = src_ind >> 1 to src_ind >>= 1;

Thanks for doing the broken down change list, it helped my memory to 
recall the patch context. :-)

For the entire series,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I'll give these a few days if somebody has still something to say about 
the new version.

--
 i.

> Changes since v2:
> 1. Change commit mesage and description
> 2. Update documentation
> 3. Remove hwmon related documentation changes from this patch
> 4. Define FIELD_GET_U32, use it everywhere where casting to u32 is needed
> 5. Define masks and use FIELD_GET() directly instead of defining function like
>    macros
> 6. Return early on error cases from all the functions
> 7. Add comma after hsmp agent in freqlimit_srcnames[]
> 8. Make index as int in hsmp_freq_limit_source_show(), remove
>    unnecessary intialization and change the printing format
> 9. Change int i to unsigned int i and return early on error in
>    hsmp_msg_get_nargs()
> 11. Change n to num_args in hsmp_msg_get_nargs()
> 12. Change __attr to _attr in to_hsmp_sys_attr()
> 13. Change "%u" to "%lu" in sysfs_emit to avoid compiler warnings
> 
> Changes since v1:
> 1. Add linux/bitops.h
> 2. Define DDR_MAX_BW, DDR_UTIL_BW DDR_UTIL_BW_PERC FW_VER_MAJOR FW_VER_MINOR FW_VER_DEBUG FMAX
>    FMIN FREQ_LIMIT FREQ_SRC_IND and use them in functions.
> 3. Return early in hsmp_msg_get_nargs()
> 4. Change while loop to for loop in hsmp_freq_limit_source_show()
> 5. Correct the GENMASK size in hsmp_ddr_util_bw_show()[bit 19:8, instead
>    of bit 20:8]
> 
>  Documentation/arch/x86/amd_hsmp.rst  |  22 +++
>  drivers/platform/x86/amd/hsmp/acpi.c | 262 +++++++++++++++++++++++++++
>  drivers/platform/x86/amd/hsmp/hsmp.c |  23 +++
>  drivers/platform/x86/amd/hsmp/hsmp.h |   1 +
>  4 files changed, 308 insertions(+)
> 
> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
> index 3ef3e0a71df9..a094f55c10b0 100644
> --- a/Documentation/arch/x86/amd_hsmp.rst
> +++ b/Documentation/arch/x86/amd_hsmp.rst
> @@ -71,6 +71,28 @@ Note: lseek() is not supported as entire metrics table is read.
>  Metrics table definitions will be documented as part of Public PPR.
>  The same is defined in the amd_hsmp.h header.
>  
> +2. HSMP telemetry sysfs files
> +
> +Following sysfs files are available at /sys/devices/platform/AMDI0097:0X/.
> +
> +* c0_residency_input: Percentage of cores in C0 state.
> +* prochot_status: Reports 1 if the processor is at thermal threshold value,
> +  0 otherwise.
> +* smu_fw_version: SMU firmware version.
> +* protocol_version: HSMP interface version.
> +* ddr_max_bw: Theoretical maximum DDR bandwidth in GB/s.
> +* ddr_utilised_bw_input: Current utilized DDR bandwidth in GB/s.
> +* ddr_utilised_bw_perc_input(%): Percentage of current utilized DDR bandwidth.
> +* mclk_input: Memory clock in MHz.
> +* fclk_input: Fabric clock in MHz.
> +* clk_fmax: Maximum frequency of socket in MHz.
> +* clk_fmin: Minimum frequency of socket in MHz.
> +* cclk_freq_limit_input: Core clock frequency limit per socket in MHz.
> +* pwr_current_active_freq_limit: Current active frequency limit of socket
> +  in MHz.
> +* pwr_current_active_freq_limit_source: Source of current active frequency
> +  limit.
> +
>  ACPI device object format
>  =========================
>  The ACPI object format expected from the amd_hsmp driver
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 93b413e0a6e6..e9075b129213 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -12,6 +12,9 @@
>  #include <asm/amd_hsmp.h>
>  
>  #include <linux/acpi.h>
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
>  #include <linux/device.h>
>  #include <linux/dev_printk.h>
>  #include <linux/ioport.h>
> @@ -36,6 +39,11 @@
>  
>  static struct hsmp_plat_device *hsmp_pdev;
>  
> +struct hsmp_sys_attr {
> +	struct device_attribute dattr;
> +	u32 msg_id;
> +};
> +
>  static int amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
>  			      u32 *value, bool write)
>  {
> @@ -243,6 +251,215 @@ static umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
>  	return 0;
>  }
>  
> +static umode_t hsmp_is_sock_dev_attr_visible(struct kobject *kobj,
> +					     struct attribute *attr, int id)
> +{
> +	return attr->mode;
> +}
> +
> +#define to_hsmp_sys_attr(_attr) container_of(_attr, struct hsmp_sys_attr, dattr)
> +
> +static ssize_t hsmp_msg_resp32_show(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n", data);
> +}
> +
> +#define DDR_MAX_BW_MASK		GENMASK(31, 20)
> +#define DDR_UTIL_BW_MASK	GENMASK(19, 8)
> +#define DDR_UTIL_BW_PERC_MASK	GENMASK(7, 0)
> +#define FW_VER_MAJOR_MASK	GENMASK(23, 16)
> +#define FW_VER_MINOR_MASK	GENMASK(15, 8)
> +#define FW_VER_DEBUG_MASK	GENMASK(7, 0)
> +#define FMAX_MASK		GENMASK(31, 16)
> +#define FMIN_MASK		GENMASK(15, 0)
> +#define FREQ_LIMIT_MASK		GENMASK(31, 16)
> +#define FREQ_SRC_IND_MASK	GENMASK(15, 0)
> +
> +static ssize_t hsmp_ddr_max_bw_show(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(DDR_MAX_BW_MASK, data));
> +}
> +
> +static ssize_t hsmp_ddr_util_bw_show(struct device *dev, struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(DDR_UTIL_BW_MASK, data));
> +}
> +
> +static ssize_t hsmp_ddr_util_bw_perc_show(struct device *dev, struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(DDR_UTIL_BW_PERC_MASK, data));
> +}
> +
> +static ssize_t hsmp_msg_fw_ver_show(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu.%lu.%lu\n",
> +			  FIELD_GET(FW_VER_MAJOR_MASK, data),
> +			  FIELD_GET(FW_VER_MINOR_MASK, data),
> +			  FIELD_GET(FW_VER_DEBUG_MASK, data));
> +}
> +
> +static ssize_t hsmp_fclk_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data[2];
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, data, 2);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n", data[0]);
> +}
> +
> +static ssize_t hsmp_mclk_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data[2];
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, data, 2);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n", data[1]);
> +}
> +
> +static ssize_t hsmp_clk_fmax_show(struct device *dev, struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(FMAX_MASK, data));
> +}
> +
> +static ssize_t hsmp_clk_fmin_show(struct device *dev, struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(FMIN_MASK, data));
> +}
> +
> +static ssize_t hsmp_freq_limit_show(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(FREQ_LIMIT_MASK, data));
> +}
> +
> +static const char * const freqlimit_srcnames[] = {
> +	"cHTC-Active",
> +	"PROCHOT",
> +	"TDC limit",
> +	"PPT Limit",
> +	"OPN Max",
> +	"Reliability Limit",
> +	"APML Agent",
> +	"HSMP Agent",
> +};
> +
> +static ssize_t hsmp_freq_limit_source_show(struct device *dev, struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct hsmp_sys_attr *hattr = to_hsmp_sys_attr(attr);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	unsigned int index;
> +	int len = 0;
> +	u16 src_ind;
> +	u32 data;
> +	int ret;
> +
> +	ret = hsmp_msg_get_nargs(sock->sock_ind, hattr->msg_id, &data, 1);
> +	if (ret)
> +		return ret;
> +
> +	src_ind = FIELD_GET(FREQ_SRC_IND_MASK, data);
> +	for (index = 0; index < ARRAY_SIZE(freqlimit_srcnames); index++) {
> +		if (!src_ind)
> +			break;
> +		if (src_ind & 1)
> +			len += sysfs_emit_at(buf, len, "%s\n", freqlimit_srcnames[index]);
> +		src_ind >>= 1;
> +	}
> +	return len;
> +}
> +
>  static int init_acpi(struct device *dev)
>  {
>  	u16 sock_ind;
> @@ -285,6 +502,8 @@ static int init_acpi(struct device *dev)
>  	if (ret)
>  		dev_err(dev, "Failed to register HSMP sensors with hwmon\n");
>  
> +	dev_set_drvdata(dev, &hsmp_pdev->sock[sock_ind]);
> +
>  	return ret;
>  }
>  
> @@ -299,9 +518,52 @@ static const struct bin_attribute *hsmp_attr_list[] = {
>  	NULL
>  };
>  
> +#define HSMP_DEV_ATTR(_name, _msg_id, _show, _mode)	\
> +static struct hsmp_sys_attr hattr_##_name = {		\
> +	.dattr = __ATTR(_name, _mode, _show, NULL),	\
> +	.msg_id = _msg_id,				\
> +}
> +
> +HSMP_DEV_ATTR(c0_residency_input, HSMP_GET_C0_PERCENT, hsmp_msg_resp32_show, 0444);
> +HSMP_DEV_ATTR(prochot_status, HSMP_GET_PROC_HOT, hsmp_msg_resp32_show, 0444);
> +HSMP_DEV_ATTR(smu_fw_version, HSMP_GET_SMU_VER, hsmp_msg_fw_ver_show, 0444);
> +HSMP_DEV_ATTR(protocol_version, HSMP_GET_PROTO_VER, hsmp_msg_resp32_show, 0444);
> +HSMP_DEV_ATTR(cclk_freq_limit_input, HSMP_GET_CCLK_THROTTLE_LIMIT, hsmp_msg_resp32_show, 0444);
> +HSMP_DEV_ATTR(ddr_max_bw, HSMP_GET_DDR_BANDWIDTH, hsmp_ddr_max_bw_show, 0444);
> +HSMP_DEV_ATTR(ddr_utilised_bw_input, HSMP_GET_DDR_BANDWIDTH, hsmp_ddr_util_bw_show, 0444);
> +HSMP_DEV_ATTR(ddr_utilised_bw_perc_input, HSMP_GET_DDR_BANDWIDTH, hsmp_ddr_util_bw_perc_show, 0444);
> +HSMP_DEV_ATTR(fclk_input, HSMP_GET_FCLK_MCLK, hsmp_fclk_show, 0444);
> +HSMP_DEV_ATTR(mclk_input, HSMP_GET_FCLK_MCLK, hsmp_mclk_show, 0444);
> +HSMP_DEV_ATTR(clk_fmax, HSMP_GET_SOCKET_FMAX_FMIN, hsmp_clk_fmax_show, 0444);
> +HSMP_DEV_ATTR(clk_fmin, HSMP_GET_SOCKET_FMAX_FMIN, hsmp_clk_fmin_show, 0444);
> +HSMP_DEV_ATTR(pwr_current_active_freq_limit, HSMP_GET_SOCKET_FREQ_LIMIT,
> +	      hsmp_freq_limit_show, 0444);
> +HSMP_DEV_ATTR(pwr_current_active_freq_limit_source, HSMP_GET_SOCKET_FREQ_LIMIT,
> +	      hsmp_freq_limit_source_show, 0444);
> +
> +static struct attribute *hsmp_dev_attr_list[] = {
> +	&hattr_c0_residency_input.dattr.attr,
> +	&hattr_prochot_status.dattr.attr,
> +	&hattr_smu_fw_version.dattr.attr,
> +	&hattr_protocol_version.dattr.attr,
> +	&hattr_cclk_freq_limit_input.dattr.attr,
> +	&hattr_ddr_max_bw.dattr.attr,
> +	&hattr_ddr_utilised_bw_input.dattr.attr,
> +	&hattr_ddr_utilised_bw_perc_input.dattr.attr,
> +	&hattr_fclk_input.dattr.attr,
> +	&hattr_mclk_input.dattr.attr,
> +	&hattr_clk_fmax.dattr.attr,
> +	&hattr_clk_fmin.dattr.attr,
> +	&hattr_pwr_current_active_freq_limit.dattr.attr,
> +	&hattr_pwr_current_active_freq_limit_source.dattr.attr,
> +	NULL
> +};
> +
>  static const struct attribute_group hsmp_attr_grp = {
>  	.bin_attrs_new = hsmp_attr_list,
> +	.attrs = hsmp_dev_attr_list,
>  	.is_bin_visible = hsmp_is_sock_attr_visible,
> +	.is_visible = hsmp_is_sock_dev_attr_visible,
>  };
>  
>  static const struct attribute_group *hsmp_groups[] = {
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 3df34d7436a9..1f0cda87b6e6 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -228,6 +228,29 @@ int hsmp_send_message(struct hsmp_message *msg)
>  }
>  EXPORT_SYMBOL_NS_GPL(hsmp_send_message, "AMD_HSMP");
>  
> +int hsmp_msg_get_nargs(u16 sock_ind, u32 msg_id, u32 *data, u8 num_args)
> +{
> +	struct hsmp_message msg = {};
> +	unsigned int i;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +	msg.msg_id = msg_id;
> +	msg.sock_ind = sock_ind;
> +	msg.response_sz = num_args;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_args; i++)
> +		data[i] = msg.args[i];
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_msg_get_nargs, "AMD_HSMP");
> +
>  int hsmp_test(u16 sock_ind, u32 value)
>  {
>  	struct hsmp_message msg = { 0 };
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 02eeebfcb165..027db8e1de12 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -69,4 +69,5 @@ int hsmp_create_sensor(struct device *dev, u16 sock_ind);
>  #else
>  int hsmp_create_sensor(struct device *dev, u16 sock_ind) { return 0; }
>  #endif
> +int hsmp_msg_get_nargs(u16 sock_ind, u32 msg_id, u32 *data, u8 num_args);
>  #endif /* HSMP_H */
>