On Tue, 18 Feb 2025, Suma Hegde wrote:
> Expose power reading and power limits via hwmon power sensors.
>
> Signed-off-by: Suma Hegde <suma.hegde@xxxxxxx>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
>
> ---
> Documentation/arch/x86/amd_hsmp.rst | 10 ++
> drivers/platform/x86/amd/hsmp/Makefile | 2 +-
> drivers/platform/x86/amd/hsmp/acpi.c | 3 +
> drivers/platform/x86/amd/hsmp/hsmp.h | 12 +-
> drivers/platform/x86/amd/hsmp/hwmon.c | 191 +++++++++++++++++++++++++
> drivers/platform/x86/amd/hsmp/plat.c | 3 +
> 6 files changed, 219 insertions(+), 2 deletions(-)
> 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..9e5fef538f4f 100644
> --- a/Documentation/arch/x86/amd_hsmp.rst
> +++ b/Documentation/arch/x86/amd_hsmp.rst
> @@ -117,6 +117,16 @@ for socket with ID00 is given below::
> }
>
>
> +HSMP HWMON interface
> +==================
> +HSMP power sensors are registered with hwmon interface.
> +Following files with 0444 permission are created.
> +- powerx_input
> +- powerx_cap_max
> +- powerx_cap
> +one powerx file is created for each socket. powerx_label is used to
> +identify the socket to which this info belongs.
> +
> An example
> ==========
>
> diff --git a/drivers/platform/x86/amd/hsmp/Makefile b/drivers/platform/x86/amd/hsmp/Makefile
> index 3175d8885e87..f63fdc12def1 100644
> --- a/drivers/platform/x86/amd/hsmp/Makefile
> +++ b/drivers/platform/x86/amd/hsmp/Makefile
> @@ -5,7 +5,7 @@
> #
>
> obj-$(CONFIG_AMD_HSMP) += hsmp_common.o
> -hsmp_common-objs := hsmp.o
> +hsmp_common-objs := hsmp.o hwmon.o
> obj-$(CONFIG_AMD_HSMP_PLAT) += amd_hsmp.o
> amd_hsmp-objs := plat.o
> obj-$(CONFIG_AMD_HSMP_ACPI) += hsmp_acpi.o
Please rebase.
Also, this probably lacks some hwmon depends on ?
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 0c54c91b5f1a..f73b6aedb986 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -342,6 +342,9 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
> if (ret)
> return ret;
> hsmp_pdev->is_probed = true;
> + ret = hsmp_create_sensor(&pdev->dev, hsmp_pdev);
> + if (ret)
> + dev_err(&pdev->dev, "Failed to create hwmon interface.\n");
> }
>
> return 0;
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 3dee0bb684c7..e0227247c995 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>
> @@ -26,7 +27,7 @@
> #define HSMP_CDEV_NAME "hsmp_cdev"
> #define HSMP_DEVNODE_NAME "hsmp"
>
> -#define DRIVER_VERSION "2.4"
> +#define DRIVER_VERSION "2.5"
>
> struct hsmp_mbaddr_info {
> u32 base_addr;
> @@ -49,12 +50,20 @@ struct hsmp_socket {
> int (*amd_hsmp_rdwr)(struct hsmp_socket *sock, u32 off, u32 *val, bool rw);
> };
>
> +struct hsmp_hwmon {
> + char (*p_label)[20];
> + const struct hwmon_channel_info *info[2];
> + struct hwmon_channel_info power_info;
> + struct hwmon_chip_info chip;
> +};
> +
> struct hsmp_plat_device {
> struct miscdevice mdev;
> struct hsmp_socket *sock;
> u32 proto_ver;
> u16 num_sockets;
> bool is_probed;
> + struct hsmp_hwmon hwmon;
> };
>
> int hsmp_cache_proto_ver(u16 sock_ind);
> @@ -65,4 +74,5 @@ 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);
> +int hsmp_create_sensor(struct device *dev, struct hsmp_plat_device *pdev);
> #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..a2925c53fbe9
> --- /dev/null
> +++ b/drivers/platform/x86/amd/hsmp/hwmon.c
> @@ -0,0 +1,191 @@
> +// 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 <asm-generic/int-ll64.h>
What is this for??
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +
> +#include "hsmp.h"
> +
> +#define NR_PWR_CHANNELS 3
> +#define HSMP_HWMON_NAME "amd_hsmp_hwmon"
> +
> +static const char * const power_label[] = {
> + "pinput",
> + "pcap",
> + "pcap_max"
> +};
> +
> +static int hsmp_hwmon_read_label(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, const char **str)
> +{
> + struct hsmp_plat_device *pdev = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_power:
> + *str = pdev->hwmon.p_label[channel];
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +static int hsmp_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct hsmp_message msg = { 0 };
> + int ret = -EOPNOTSUPP;
> +
> + switch (type) {
> + case hwmon_power:
> + switch (channel % NR_PWR_CHANNELS) {
> + case 1:
> + msg.sock_ind = channel / NR_PWR_CHANNELS;
> + msg.num_args = 1;
> + msg.args[0] = val;
> + msg.msg_id = HSMP_SET_SOCKET_POWER_LIMIT;
> + return hsmp_send_message(&msg);
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
That's quite much boilerplate where direct returns would avoid some of it.
Is checkpatch the cause for all this madness?? There's no point in
cascading breaks just to return in the end of the chain.
> + }
> +
> + return ret;
You don't need variable to return -EOPNOTSUPP; here ;-).
> +}
> +
> +static int hsmp_hwmon_read(struct device *dev,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct hsmp_message msg = { 0 };
> + int ret;
> +
> + msg.sock_ind = channel / NR_PWR_CHANNELS;
> + msg.response_sz = 1;
> +
> + switch (type) {
> + case hwmon_power:
> + switch (channel % NR_PWR_CHANNELS) {
> + case 0:
> + msg.msg_id = HSMP_GET_SOCKET_POWER;
> + break;
> + case 1:
> + msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT;
> + break;
> + case 2:
> + msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT_MAX;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + ret = hsmp_send_message(&msg);
> + if (!ret)
> + *val = msg.args[0];
> +
> + return ret;
> +}
> +
> +static umode_t hsmp_hwmon_is_visble(const void *data,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (type) {
> + case hwmon_power:
> + switch (channel % NR_PWR_CHANNELS) {
> + case 0:
> + case 2:
> + return 0444;
> + case 1:
> + return 0644;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
boilerplate
> +
> + return 0;
> +}
> +
> +const struct hwmon_ops hsmp_hwmon_ops = {
> + .read = hsmp_hwmon_read,
> + .read_string = hsmp_hwmon_read_label,
> + .is_visible = hsmp_hwmon_is_visble,
> + .write = hsmp_hwmon_write,
> +};
> +
> +static int hsmp_create_power_sensor(struct device *dev, struct hsmp_plat_device *pdev)
> +{
> + char (*p_label)[20];
> + u32 *p_config;
> + int j;
> +
> + p_config = devm_kcalloc(dev, pdev->num_sockets * NR_PWR_CHANNELS + 1,
> + sizeof(*p_config), GFP_KERNEL);
> + if (!p_config)
> + return -ENOMEM;
> +
> + p_label = devm_kcalloc(dev, NR_PWR_CHANNELS * pdev->num_sockets,
> + sizeof(*p_label), GFP_KERNEL);
> + if (!p_label)
> + return -ENOMEM;
> +
> + for (j = 0; j < pdev->num_sockets * NR_PWR_CHANNELS;) {
> + p_config[j] = HWMON_P_INPUT | HWMON_P_LABEL;
> + p_config[j + 1] = HWMON_P_CAP | HWMON_P_LABEL;
> + p_config[j + 2] = HWMON_P_CAP_MAX | HWMON_P_LABEL;
> + scnprintf(p_label[j], 20, "sock%u_%s", j / NR_PWR_CHANNELS, power_label[0]);
> + scnprintf(p_label[j + 1], 20, "sock%u_%s", j / NR_PWR_CHANNELS, power_label[1]);
> + scnprintf(p_label[j + 2], 20, "sock%u_%s", j / NR_PWR_CHANNELS, power_label[2]);
You could just devm_kasprintf() these and get rid of that magic 20. And do
you even have to alloc the array, it's going to 3 entries always?
> + j += NR_PWR_CHANNELS;
> + }
> +
> + pdev->hwmon.power_info.type = hwmon_power;
> + pdev->hwmon.power_info.config = p_config;
> + pdev->hwmon.info[0] = &pdev->hwmon.power_info;
> + pdev->hwmon.p_label = p_label;
> +
> + return 0;
> +}
> +
> +int hsmp_create_sensor(struct device *dev, struct hsmp_plat_device *pdev)
> +{
> + struct device *hwmon_dev;
> + int ret;
> +
> + ret = hsmp_create_power_sensor(dev, pdev);
> + if (ret)
> + return ret;
> +
> + pdev->hwmon.chip.ops = &hsmp_hwmon_ops;
> + pdev->hwmon.chip.info = pdev->hwmon.info;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, HSMP_HWMON_NAME,
> + pdev,
> + &pdev->hwmon.chip,
> + NULL);
> + if (IS_ERR(hwmon_dev))
> + return PTR_ERR(hwmon_dev);
> +
> + return 0;
> +}
> +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 63034408985c..2bb590a34642 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -220,6 +220,9 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
> return ret;
> }
> + ret = hsmp_create_sensor(&pdev->dev, hsmp_pdev);
> + if (ret)
> + dev_err(&pdev->dev, "Failed to create hwmon interface.\n");
>
> return hsmp_misc_register(&pdev->dev);
> }
>
--
i.
