* [PATCH v4 1/2] platforms/x86: Add AMD system management interface
@ 2022-02-09 18:44 Naveen Krishna Chatradhi
2022-02-09 18:44 ` [PATCH v4 2/2] Documentation: Add x86/amd_hsmp driver Naveen Krishna Chatradhi
2022-02-09 21:08 ` [PATCH v4 1/2] platforms/x86: Add AMD system management interface Nathan Fontenot
0 siblings, 2 replies; 13+ messages in thread
From: Naveen Krishna Chatradhi @ 2022-02-09 18:44 UTC (permalink / raw)
To: platform-driver-x86
Cc: hdegoede, carlos.bilbao, siva.sathappan, suma.hegde,
Naveen Krishna Chatradhi
From: Suma Hegde <suma.hegde@amd.com>
Recent Fam19h EPYC server line of processors from AMD support system
management functionality via HSMP (Host System Management Port) interface.
The Host System Management Port (HSMP) is an interface to provide
OS-level software with access to system management functions via a
set of mailbox registers.
More details on the interface can be found in chapter
"7 Host System Management Port (HSMP)" of the following PPR
https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
This patch adds new amd_hsmp module under the drivers/platforms/x86/
which creates miscdevice with an IOCTL interface to the user space.
/dev/hsmp is for running the hsmp mailbox commands.
Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
Reviewed-by: Carlos Bilbao <carlos.bilbao@amd.com>
---
Changes since v3:
remove change ids
Changes since v2:
1. Updated MAINTAINERS page
2. Added message description table with num_args, response_sz and type fields.
This will be useful for user space applications and the driver
to validate the inputs.
3. removed the model check and added a statement in the documentation.
Changes since v1:
1. Add supported model check
. This interface is supported only on server line of CPUs.
2. Handle Reserved messages
3. Add brief descriptions of the Messages
4. Add Carlos Bilbao's reviewed-by
.../userspace-api/ioctl/ioctl-number.rst | 2 +
MAINTAINERS | 9 +
arch/x86/include/asm/amd_hsmp.h | 16 +
arch/x86/include/uapi/asm/amd_hsmp.h | 94 ++++
drivers/platform/x86/Kconfig | 13 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/amd_hsmp.c | 417 ++++++++++++++++++
7 files changed, 552 insertions(+)
create mode 100644 arch/x86/include/asm/amd_hsmp.h
create mode 100644 arch/x86/include/uapi/asm/amd_hsmp.h
create mode 100644 drivers/platform/x86/amd_hsmp.c
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 687efcf245c1..663e316d320c 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -372,6 +372,8 @@ Code Seq# Include File Comments
<mailto:thomas@winischhofer.net>
0xF6 all LTTng Linux Trace Toolkit Next Generation
<mailto:mathieu.desnoyers@efficios.com>
+0xF8 all arch/x86/include/uapi/asm/amd_hsmp.h AMD HSMP EPYC system management interface driver
+ <mailto:nchatrad@amd.com>
0xFD all linux/dm-ioctl.h
0xFE all linux/isst_if.h
==== ===== ======================================================= ================================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index f41088418aae..bb2fc501d13f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -989,6 +989,15 @@ L: platform-driver-x86@vger.kernel.org
S: Maintained
F: drivers/platform/x86/amd-pmc.*
+AMD HSMP DRIVER
+M: Chatradhi Naveen Krishna <naveenkrishna.chatradhi@amd.com>
+R: Bilbao, Carlos <carlos.bilbao@amd.com>
+L: platform-driver-x86@vger.kernel.org
+S: Maintained
+F: arch/x86/include/asm/amd_hsmp.h
+F: arch/x86/include/uapi/asm/amd_hsmp.h
+F: drivers/platform/x86/amd_hsmp.c
+
AMD POWERPLAY AND SWSMU
M: Evan Quan <evan.quan@amd.com>
L: amd-gfx@lists.freedesktop.org
diff --git a/arch/x86/include/asm/amd_hsmp.h b/arch/x86/include/asm/amd_hsmp.h
new file mode 100644
index 000000000000..db2846bb3c37
--- /dev/null
+++ b/arch/x86/include/asm/amd_hsmp.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _ASM_X86_AMD_HSMP_H_
+#define _ASM_X86_AMD_HSMP_H_
+
+#include <uapi/asm/amd_hsmp.h>
+
+#if (defined(CONFIG_AMD_HSMP) || defined(CONFIG_AMD_HSMP_MODULE))
+int hsmp_send_message(struct hsmp_message *msg);
+#else
+int hsmp_send_message(struct hsmp_message *msg)
+{
+ return -ENODEV;
+}
+#endif
+#endif /*_ASM_X86_AMD_HSMP_H_*/
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
new file mode 100644
index 000000000000..d1cee2c2040c
--- /dev/null
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -0,0 +1,94 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _UAPI_ASM_X86_AMD_HSMP_H_
+#define _UAPI_ASM_X86_AMD_HSMP_H_
+
+#include <linux/types.h>
+
+#pragma pack(4)
+
+#define HSMP_MAX_MSG_LEN 8
+
+/*
+ * HSMP Messages supported
+ */
+enum hsmp_message_ids {
+ HSMP_TEST = 1, /* 01h Increments input value by 1 */
+ HSMP_GET_SMU_VER, /* 02h SMU FW version */
+ HSMP_GET_PROTO_VER, /* 03h HSMP interface version */
+ HSMP_GET_SOCKET_POWER, /* 04h average package power consumption */
+ HSMP_SET_SOCKET_POWER_LIMIT, /* 05h Set the socket power limit */
+ HSMP_GET_SOCKET_POWER_LIMIT, /* 06h Get current socket power limit */
+ HSMP_GET_SOCKET_POWER_LIMIT_MAX,/* 07h Get maximum socket power value */
+ HSMP_SET_BOOST_LIMIT, /* 08h Set a core maximum frequency limit */
+ HSMP_SET_BOOST_LIMIT_SOCKET, /* 09h Set socket maximum frequency level */
+ HSMP_GET_BOOST_LIMIT, /* 0Ah Get current frequency limit */
+ HSMP_GET_PROC_HOT, /* 0Bh Get PROCHOT status */
+ HSMP_SET_XGMI_LINK_WIDTH, /* 0Ch Set max and min width of xGMI Link */
+ HSMP_SET_DF_PSTATE, /* 0Dh Alter APEnable/Disable messages behavior */
+ HSMP_SET_AUTO_DF_PSTATE, /* 0Eh Enable DF P-State Performance Boost algorithm */
+ HSMP_GET_FCLK_MCLK, /* 0Fh Get FCLK and MEMCLK for current socket */
+ HSMP_GET_CCLK_THROTTLE_LIMIT, /* 10h Get CCLK frequency limit in socket */
+ HSMP_GET_C0_PERCENT, /* 11h Get average C0 residency in socket */
+ HSMP_SET_NBIO_DPM_LEVEL, /* 12h Set max/min LCLK DPM Level for a given NBIO */
+ /* 13h Reserved */
+ HSMP_GET_DDR_BANDWIDTH = 0x14, /* 14h Get theoretical maximum and current DDR Bandwidth */
+ HSMP_GET_TEMP_MONITOR, /* 15h Get per-DIMM temperature and refresh rates */
+ HSMP_MSG_ID_MAX,
+};
+
+struct hsmp_message {
+ __u32 msg_id; /* Message ID */
+ __u16 num_args; /* Number of arguments in message */
+ __u16 response_sz; /* Number of expected response words */
+ __u32 args[HSMP_MAX_MSG_LEN]; /* Argument(s) */
+ __u32 response[HSMP_MAX_MSG_LEN]; /* Response word(s) */
+ __u16 sock_ind; /* socket number */
+};
+
+struct hsmp_msg_desc {
+ int num_args;
+ int response_sz;
+ int type;
+};
+
+enum hsmp_msg_type {
+ RSVD = -1,
+ SET = 0,
+ GET = 1,
+};
+
+static const struct hsmp_msg_desc msg_desc_table[] = {
+ /* num_args, response_size, type */
+ {0, 0, RSVD}, /* RESERVED */
+ {1, 1, GET}, /* HSMP_TEST */
+ {0, 1, GET}, /* HSMP_GET_SMU_VER */
+ {0, 1, GET}, /* HSMP_GET_PROTO_VER */
+ {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
+ {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
+ {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
+ {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
+ {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
+ {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
+ {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
+ {0, 1, GET}, /* HSMP_GET_PROC_HOT */
+ {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
+ {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
+ {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
+ {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
+ {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
+ {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
+ {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
+ {0, 0, RSVD}, /* RESERVED */
+ {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
+ {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
+};
+
+/* Reset to default packing */
+#pragma pack()
+
+/* Define unique ioctl command for hsmp msgs using generic _IOWR */
+#define HSMP_BASE_IOCTL_NR 0xF8
+#define HSMP_IOCTL_CMD _IOWR(HSMP_BASE_IOCTL_NR, 0, struct hsmp_message)
+
+#endif /*_ASM_X86_AMD_HSMP_H_*/
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 24deeeb29af2..0906c36ea07b 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -210,6 +210,19 @@ config AMD_PMC
If you choose to compile this driver as a module the module will be
called amd-pmc.
+config AMD_HSMP
+ tristate "AMD HSMP Driver"
+ depends on AMD_NB && X86_64
+ help
+ The driver provides a way for user space tools to monitor and manage
+ system management functionality on EPYC server CPUs from AMD.
+
+ Host System Management Port (HSMP) interface is a mailbox interface
+ between the x86 core and the System Management Unit (SMU) firmware.
+
+ If you choose to compile this driver as a module the module will be
+ called amd_hsmp.
+
config ADV_SWBUTTON
tristate "Advantech ACPI Software Button Driver"
depends on ACPI && INPUT
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index c12a9b044fd8..b3a93a5053a3 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_ACER_WMI) += acer-wmi.o
# AMD
obj-$(CONFIG_AMD_PMC) += amd-pmc.o
+obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o
# Advantech
obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
diff --git a/drivers/platform/x86/amd_hsmp.c b/drivers/platform/x86/amd_hsmp.c
new file mode 100644
index 000000000000..858f194e5a4c
--- /dev/null
+++ b/drivers/platform/x86/amd_hsmp.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD HSMP Platform Driver
+ * Copyright (c) 2022, AMD.
+ * All Rights Reserved.
+ *
+ * This file provides a device implementation for HSMP interface
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/amd_hsmp.h>
+#include <asm/amd_nb.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/semaphore.h>
+
+#define DRIVER_NAME "amd_hsmp"
+#define DRIVER_VERSION "1.0"
+
+/* HSMP Status / Error codes */
+#define HSMP_STATUS_NOT_READY 0x00
+#define HSMP_STATUS_OK 0x01
+#define HSMP_ERR_INVALID_MSG 0xFE
+#define HSMP_ERR_INVALID_INPUT 0xFF
+
+/* Timeout in millsec */
+#define HSMP_MSG_TIMEOUT 100
+#define HSMP_SHORT_SLEEP 1
+
+#define HSMP_WR true
+#define HSMP_RD false
+
+/*
+ * To access specific HSMP mailbox register, s/w writes the SMN address of HSMP mailbox
+ * register into the SMN_INDEX register, and reads/writes the SMN_DATA reg.
+ * Below are required SMN address for HSMP Mailbox register offsets in SMU address space
+ */
+#define SMN_HSMP_MSG_ID 0x3B10534
+#define SMN_HSMP_MSG_RESP 0x3B10980
+#define SMN_HSMP_MSG_DATA 0x3B109E0
+
+#define HSMP_INDEX_REG 0xc4
+#define HSMP_DATA_REG 0xc8
+
+static struct semaphore *hsmp_sem;
+
+static struct miscdevice hsmp_device;
+
+static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
+ u32 *value, bool write)
+{
+ int ret;
+
+ ret = pci_write_config_dword(root, HSMP_INDEX_REG, address);
+ if (ret)
+ return ret;
+
+ ret = (write ? pci_write_config_dword(root, HSMP_DATA_REG, *value)
+ : pci_read_config_dword(root, HSMP_DATA_REG, value));
+
+ return ret;
+}
+
+/*
+ * Send a message to the HSMP port via PCI-e config space registers.
+ *
+ * The caller is expected to zero out any unused arguments.
+ * If a response is expected, the number of response words should be greater than 0.
+ *
+ * Returns 0 for success and populates the requested number of arguments.
+ * Returns a negative error code for failure.
+ */
+static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
+{
+ unsigned long timeout, short_sleep;
+ u32 mbox_status;
+ u32 arg_num;
+ int ret;
+
+ /* Clear the status register */
+ mbox_status = HSMP_STATUS_NOT_READY;
+ ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_WR);
+ if (ret) {
+ pr_err("Error %d clearing mailbox status register\n", ret);
+ return ret;
+ }
+
+ arg_num = 0;
+ /* Write any message arguments */
+ while (arg_num < msg->num_args) {
+ ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (arg_num << 2),
+ &msg->args[arg_num], HSMP_WR);
+ if (ret) {
+ pr_err("Error %d writing message argument %d\n", ret, arg_num);
+ return ret;
+ }
+ arg_num++;
+ }
+
+ /* Write the message ID which starts the operation */
+ ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_ID, &msg->msg_id, HSMP_WR);
+ if (ret) {
+ pr_err("Error %d writing message ID %u\n", ret, msg->msg_id);
+ return ret;
+ }
+
+ /*
+ * Depending on when the trigger write completes relative to the SMU
+ * firmware 1 ms cycle, the operation may take from tens of us to 1 ms
+ * to complete. Some operations may take more. Therefore we will try
+ * a few short duration sleeps and switch to long sleeps if we don't
+ * succeed quickly.
+ */
+ short_sleep = jiffies + msecs_to_jiffies(HSMP_SHORT_SLEEP);
+ timeout = jiffies + msecs_to_jiffies(HSMP_MSG_TIMEOUT);
+
+ while (time_before(jiffies, timeout)) {
+ ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_RD);
+ if (ret) {
+ pr_err("Error %d reading mailbox status\n", ret);
+ return ret;
+ }
+
+ if (mbox_status != HSMP_STATUS_NOT_READY)
+ break;
+ if (time_before(jiffies, short_sleep))
+ usleep_range(50, 100);
+ else
+ usleep_range(1000, 2000);
+ }
+
+ if (unlikely(mbox_status == HSMP_STATUS_NOT_READY)) {
+ return -ETIMEDOUT;
+ } else if (unlikely(mbox_status == HSMP_ERR_INVALID_MSG)) {
+ return -ENOMSG;
+ } else if (unlikely(mbox_status == HSMP_ERR_INVALID_INPUT)) {
+ return -EINVAL;
+ } else if (unlikely(mbox_status != HSMP_STATUS_OK)) {
+ pr_err("Message ID %u unknown failure (status = 0x%X)\n",
+ msg->msg_id, mbox_status);
+ return -EIO;
+ }
+
+ /* SMU has responded OK. Read response data */
+ arg_num = 0;
+ while (arg_num < msg->response_sz) {
+ ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (arg_num << 2),
+ &msg->response[arg_num], HSMP_RD);
+ if (ret) {
+ pr_err("Error %d reading response %u for message ID:%u\n",
+ ret, arg_num, msg->msg_id);
+ break;
+ }
+ arg_num++;
+ }
+
+ return ret;
+}
+
+static int validate_message(struct hsmp_message *msg)
+{
+ /* Message ID within valid range of message IDs */
+ if (msg->msg_id < HSMP_TEST || msg->msg_id >= HSMP_MSG_ID_MAX)
+ return -EINVAL;
+
+ /* Reserved message IDs */
+ if (msg_desc_table[msg->msg_id].type == RSVD)
+ return -EINVAL;
+
+ /* Message with number of arguments and response size not matching the HSMP spec */
+ if (msg->num_args != msg_desc_table[msg->msg_id].num_args ||
+ msg->response_sz != msg_desc_table[msg->msg_id].response_sz)
+ return -EINVAL;
+
+ return 0;
+}
+
+int hsmp_send_message(struct hsmp_message *msg)
+{
+ struct amd_northbridge *nb;
+ int ret;
+
+ if (!msg)
+ return -EINVAL;
+
+ nb = node_to_amd_nb(msg->sock_ind);
+ if (!nb || !nb->root)
+ return -ENODEV;
+
+ if (validate_message(msg))
+ return -EINVAL;
+
+ /*
+ * The time taken by smu operation to complete is between
+ * 10us to 1ms. Sometime it may take more time.
+ * In SMP system timeout of 100 millisecs should
+ * be enough for the previous thread to finish the operation
+ */
+ ret = down_timeout(&hsmp_sem[msg->sock_ind],
+ msecs_to_jiffies(HSMP_MSG_TIMEOUT));
+ if (ret < 0)
+ return ret;
+
+ ret = __hsmp_send_message(nb->root, msg);
+
+ up(&hsmp_sem[msg->sock_ind]);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(hsmp_send_message);
+
+static int hsmp_test(u16 sock_ind)
+{
+ struct hsmp_message msg = { 0 };
+ struct amd_northbridge *nb;
+ int ret = -ENODEV;
+
+ nb = node_to_amd_nb(sock_ind);
+ if (!nb || !nb->root)
+ return ret;
+
+ /*
+ * Test the hsmp port by performing TEST command. The test message takes
+ * one argument and returns the value of that argument + 1.
+ */
+ msg.sock_ind = sock_ind;
+ msg.response_sz = 1;
+ msg.num_args = 1;
+ msg.msg_id = HSMP_TEST;
+ msg.args[0] = 0xDEADBEEF;
+
+ ret = __hsmp_send_message(nb->root, &msg);
+ if (ret)
+ return ret;
+
+ if (msg.response[0] != (msg.args[0] + 1)) {
+ pr_err("Socket %d test message failed, Expected 0x%08X, received 0x%08X\n",
+ sock_ind, msg.args[0] + 1, msg.response[0]);
+ return -EBADE;
+ }
+
+ return 0;
+}
+
+static long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ int __user *arguser = (int __user *)arg;
+ struct hsmp_message msg = { 0 };
+ int ret;
+
+ if (copy_struct_from_user(&msg, sizeof(msg), arguser, sizeof(struct hsmp_message)))
+ return -EFAULT;
+
+ if (validate_message(&msg))
+ return -EINVAL;
+
+ switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
+ case FMODE_WRITE:
+ /*
+ * Device is opened in O_WRONLY mode
+ * Execute only set/configure commands
+ */
+ if (msg_desc_table[msg.msg_id].type != SET)
+ return -EINVAL;
+ break;
+ case FMODE_READ:
+ /*
+ * Device is opened in O_RDONLY mode
+ * Execute only get/monitor commands
+ */
+ if (msg_desc_table[msg.msg_id].type != GET)
+ return -EINVAL;
+ break;
+ case FMODE_READ | FMODE_WRITE:
+ /*
+ * Device is opened in O_RDWR mode
+ * Execute both get/monitor and set/configure commands
+ */
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = hsmp_send_message(&msg);
+ if (ret)
+ return ret;
+
+ if (msg_desc_table[msg.msg_id].response_sz > 0) {
+ /* Copy results back to user for get/monitor commands */
+ if (copy_to_user(arguser, &msg, sizeof(struct hsmp_message)))
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static const struct file_operations hsmp_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = hsmp_ioctl,
+ .compat_ioctl = hsmp_ioctl,
+};
+
+static int hsmp_pltdrv_probe(struct platform_device *pdev)
+{
+ int ret, i;
+
+ hsmp_sem = devm_kzalloc(&pdev->dev,
+ (amd_nb_num() * sizeof(struct semaphore)),
+ GFP_KERNEL);
+ if (!hsmp_sem)
+ return -ENOMEM;
+
+ for (i = 0; i < amd_nb_num(); i++)
+ sema_init(&hsmp_sem[i], 1);
+
+ hsmp_device.name = "hsmp_cdev";
+ hsmp_device.minor = MISC_DYNAMIC_MINOR;
+ hsmp_device.fops = &hsmp_fops;
+ hsmp_device.parent = &pdev->dev;
+ hsmp_device.nodename = "hsmp";
+ hsmp_device.mode = 0644;
+
+ ret = misc_register(&hsmp_device);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int hsmp_pltdrv_remove(struct platform_device *pdev)
+{
+ misc_deregister(&hsmp_device);
+
+ return 0;
+}
+
+static struct platform_driver amd_hsmp_driver = {
+ .probe = hsmp_pltdrv_probe,
+ .remove = hsmp_pltdrv_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static struct platform_device *amd_hsmp_platdev;
+
+static int __init hsmp_plt_init(void)
+{
+ int ret = -ENODEV;
+ u16 num_sockets;
+ int i;
+
+ if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
+ pr_err("HSMP is not supported on Family:%x model:%x\n",
+ boot_cpu_data.x86, boot_cpu_data.x86_model);
+ return ret;
+ }
+
+ /*
+ * amd_nb_num() returns number of SMN/DF interfaces present in the system
+ * if we have N SMN/DF interfaces that ideally means N sockets
+ */
+ num_sockets = amd_nb_num();
+ if (num_sockets == 0)
+ return ret;
+
+ /* Test the hsmp interface on each socket */
+ for (i = 0; i < num_sockets; i++) {
+ ret = hsmp_test(i);
+ if (ret) {
+ pr_err("HSMP is not supported on Fam:%x model:%x\n",
+ boot_cpu_data.x86, boot_cpu_data.x86_model);
+ pr_err("Or Is HSMP disabled in BIOS ?\n");
+ return -EOPNOTSUPP;
+ }
+ }
+
+ ret = platform_driver_register(&amd_hsmp_driver);
+ if (ret)
+ return ret;
+
+ amd_hsmp_platdev = platform_device_alloc(DRIVER_NAME, -1);
+ if (!amd_hsmp_platdev) {
+ ret = -ENOMEM;
+ goto drv_unregister;
+ }
+
+ ret = platform_device_add(amd_hsmp_platdev);
+ if (ret) {
+ platform_device_put(amd_hsmp_platdev);
+ goto drv_unregister;
+ }
+
+ return 0;
+
+drv_unregister:
+ platform_driver_unregister(&amd_hsmp_driver);
+ return ret;
+}
+
+static void __exit hsmp_plt_exit(void)
+{
+ platform_device_unregister(amd_hsmp_platdev);
+ platform_driver_unregister(&amd_hsmp_driver);
+}
+
+device_initcall(hsmp_plt_init);
+module_exit(hsmp_plt_exit);
+
+MODULE_DESCRIPTION("AMD HSMP Platform Interface Driver");
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 2/2] Documentation: Add x86/amd_hsmp driver
2022-02-09 18:44 [PATCH v4 1/2] platforms/x86: Add AMD system management interface Naveen Krishna Chatradhi
@ 2022-02-09 18:44 ` Naveen Krishna Chatradhi
2022-02-09 21:08 ` [PATCH v4 1/2] platforms/x86: Add AMD system management interface Nathan Fontenot
1 sibling, 0 replies; 13+ messages in thread
From: Naveen Krishna Chatradhi @ 2022-02-09 18:44 UTC (permalink / raw)
To: platform-driver-x86
Cc: hdegoede, carlos.bilbao, siva.sathappan, suma.hegde,
Naveen Krishna Chatradhi
This documentation for amd_hsmp driver explains how to use the
device interface.
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v3:
remove change ids
Changes since v2:
Add statement saying the HSMP interface is supported only on
server cpu models from AMD.
Changes since v1:
None
Documentation/x86/amd_hsmp.rst | 86 ++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/x86/amd_hsmp.rst
diff --git a/Documentation/x86/amd_hsmp.rst b/Documentation/x86/amd_hsmp.rst
new file mode 100644
index 000000000000..5dc2619ec3f0
--- /dev/null
+++ b/Documentation/x86/amd_hsmp.rst
@@ -0,0 +1,86 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============================================
+AMD HSMP interface
+============================================
+
+Newer Fam19h EPYC server line of processors from AMD support system
+management functionality via HSMP (Host System Management Port).
+
+The Host System Management Port (HSMP) is an interface to provide
+OS-level software with access to system management functions via a
+set of mailbox registers.
+
+More details on the interface can be found in chapter
+"7 Host System Management Port (HSMP)" of the following PPR
+https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
+
+HSMP interface is supported on AMD EPYC server CPU models only.
+
+
+HSMP device
+============================================
+
+amd_hsmp driver under the drivers/platforms/x86/ creates miscdevice
+/dev/hsmp to let user space programs run hsmp mailbox commands.
+
+$ ls -al /dev/hsmp
+crw-r--r-- 1 root root 10, 123 Jan 21 21:41 /dev/hsmp
+
+Characteristics of the dev node:
+ * Write mode is used for running set/configure commands
+ * Read mode is used for running get/status monitor commands
+
+Access restrictions:
+ * Only root user is allowed to open the file in write mode.
+ * The file can be opened in read mode by all the users.
+
+In-kernel integration:
+ * Other subsystems in the kernel can use the exported transport
+ function hsmp_send_message().
+ * Locking across callers is taken care by the driver.
+
+
+An example
+==========
+
+To access hsmp device from a C program.
+First, you need to include the headers::
+
+ #include <linux/amd_hsmp.h>
+Which defines the supported messages/message IDs.
+
+Next thing, open the device file, as follows::
+
+ int file;
+
+ file = open("/dev/hsmp", O_RDWR);
+ if (file < 0) {
+ /* ERROR HANDLING; you can check errno to see what went wrong */
+ exit(1);
+ }
+
+The following IOCTL is defined:
+
+``ioctl(file, HSMP_IOCTL_CMD, struct hsmp_message *msg)``
+ The argument is a pointer to a::
+
+ struct hsmp_message {
+ __u32 msg_id; /* Message ID */
+ __u16 num_args; /* Number of arguments in message */
+ __u16 response_sz; /* Number of expected response words */
+ __u32 args[HSMP_MAX_MSG_LEN]; /* Argument(s) */
+ __u32 response[HSMP_MAX_MSG_LEN]; /* Response word(s) */
+ __u16 sock_ind; /* socket number */
+ };
+
+The ioctl would return a non-zero on failure; you can read errno to see
+what happened. The transaction returns 0 on success.
+
+More details on the interface can be found in chapter
+"7 Host System Management Port (HSMP)" of the following PPR
+https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
+
+User space C-APIs are made available by linking against the esmi library,
+which is provided by the E-SMS project https://developer.amd.com/e-sms/.
+See: https://github.com/amd/esmi_ib_library
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-09 18:44 [PATCH v4 1/2] platforms/x86: Add AMD system management interface Naveen Krishna Chatradhi
2022-02-09 18:44 ` [PATCH v4 2/2] Documentation: Add x86/amd_hsmp driver Naveen Krishna Chatradhi
@ 2022-02-09 21:08 ` Nathan Fontenot
2022-02-10 1:10 ` Song Liu
1 sibling, 1 reply; 13+ messages in thread
From: Nathan Fontenot @ 2022-02-09 21:08 UTC (permalink / raw)
To: Naveen Krishna Chatradhi, platform-driver-x86
Cc: hdegoede, carlos.bilbao, siva.sathappan, suma.hegde
On 2/9/22 12:44, Naveen Krishna Chatradhi wrote:
> From: Suma Hegde <suma.hegde@amd.com>
>
> Recent Fam19h EPYC server line of processors from AMD support system
> management functionality via HSMP (Host System Management Port) interface.
>
> The Host System Management Port (HSMP) is an interface to provide
> OS-level software with access to system management functions via a
> set of mailbox registers.
>
> More details on the interface can be found in chapter
> "7 Host System Management Port (HSMP)" of the following PPR
> https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
>
> This patch adds new amd_hsmp module under the drivers/platforms/x86/
> which creates miscdevice with an IOCTL interface to the user space.
> /dev/hsmp is for running the hsmp mailbox commands.
>
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> Reviewed-by: Carlos Bilbao <carlos.bilbao@amd.com>
> ---
> Changes since v3:
> remove change ids
> Changes since v2:
> 1. Updated MAINTAINERS page
> 2. Added message description table with num_args, response_sz and type fields.
> This will be useful for user space applications and the driver
> to validate the inputs.
> 3. removed the model check and added a statement in the documentation.
>
> Changes since v1:
> 1. Add supported model check
> . This interface is supported only on server line of CPUs.
> 2. Handle Reserved messages
> 3. Add brief descriptions of the Messages
> 4. Add Carlos Bilbao's reviewed-by
>
>
> .../userspace-api/ioctl/ioctl-number.rst | 2 +
> MAINTAINERS | 9 +
> arch/x86/include/asm/amd_hsmp.h | 16 +
> arch/x86/include/uapi/asm/amd_hsmp.h | 94 ++++
> drivers/platform/x86/Kconfig | 13 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/amd_hsmp.c | 417 ++++++++++++++++++
> 7 files changed, 552 insertions(+)
> create mode 100644 arch/x86/include/asm/amd_hsmp.h
> create mode 100644 arch/x86/include/uapi/asm/amd_hsmp.h
> create mode 100644 drivers/platform/x86/amd_hsmp.c
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 687efcf245c1..663e316d320c 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -372,6 +372,8 @@ Code Seq# Include File Comments
> <mailto:thomas@winischhofer.net>
> 0xF6 all LTTng Linux Trace Toolkit Next Generation
> <mailto:mathieu.desnoyers@efficios.com>
> +0xF8 all arch/x86/include/uapi/asm/amd_hsmp.h AMD HSMP EPYC system management interface driver
> + <mailto:nchatrad@amd.com>
> 0xFD all linux/dm-ioctl.h
> 0xFE all linux/isst_if.h
> ==== ===== ======================================================= ================================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f41088418aae..bb2fc501d13f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -989,6 +989,15 @@ L: platform-driver-x86@vger.kernel.org
> S: Maintained
> F: drivers/platform/x86/amd-pmc.*
>
> +AMD HSMP DRIVER
> +M: Chatradhi Naveen Krishna <naveenkrishna.chatradhi@amd.com>
> +R: Bilbao, Carlos <carlos.bilbao@amd.com>
> +L: platform-driver-x86@vger.kernel.org
> +S: Maintained
> +F: arch/x86/include/asm/amd_hsmp.h
> +F: arch/x86/include/uapi/asm/amd_hsmp.h
> +F: drivers/platform/x86/amd_hsmp.c
> +
> AMD POWERPLAY AND SWSMU
> M: Evan Quan <evan.quan@amd.com>
> L: amd-gfx@lists.freedesktop.org
> diff --git a/arch/x86/include/asm/amd_hsmp.h b/arch/x86/include/asm/amd_hsmp.h
> new file mode 100644
> index 000000000000..db2846bb3c37
> --- /dev/null
> +++ b/arch/x86/include/asm/amd_hsmp.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +#ifndef _ASM_X86_AMD_HSMP_H_
> +#define _ASM_X86_AMD_HSMP_H_
> +
> +#include <uapi/asm/amd_hsmp.h>
> +
> +#if (defined(CONFIG_AMD_HSMP) || defined(CONFIG_AMD_HSMP_MODULE))
> +int hsmp_send_message(struct hsmp_message *msg);
> +#else
> +int hsmp_send_message(struct hsmp_message *msg)
> +{
> + return -ENODEV;
> +}
> +#endif
> +#endif /*_ASM_X86_AMD_HSMP_H_*/
> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
> new file mode 100644
> index 000000000000..d1cee2c2040c
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +
> +#ifndef _UAPI_ASM_X86_AMD_HSMP_H_
> +#define _UAPI_ASM_X86_AMD_HSMP_H_
> +
> +#include <linux/types.h>
> +
> +#pragma pack(4)
> +
> +#define HSMP_MAX_MSG_LEN 8
> +
> +/*
> + * HSMP Messages supported
> + */
> +enum hsmp_message_ids {
> + HSMP_TEST = 1, /* 01h Increments input value by 1 */
> + HSMP_GET_SMU_VER, /* 02h SMU FW version */
> + HSMP_GET_PROTO_VER, /* 03h HSMP interface version */
> + HSMP_GET_SOCKET_POWER, /* 04h average package power consumption */
> + HSMP_SET_SOCKET_POWER_LIMIT, /* 05h Set the socket power limit */
> + HSMP_GET_SOCKET_POWER_LIMIT, /* 06h Get current socket power limit */
> + HSMP_GET_SOCKET_POWER_LIMIT_MAX,/* 07h Get maximum socket power value */
> + HSMP_SET_BOOST_LIMIT, /* 08h Set a core maximum frequency limit */
> + HSMP_SET_BOOST_LIMIT_SOCKET, /* 09h Set socket maximum frequency level */
> + HSMP_GET_BOOST_LIMIT, /* 0Ah Get current frequency limit */
> + HSMP_GET_PROC_HOT, /* 0Bh Get PROCHOT status */
> + HSMP_SET_XGMI_LINK_WIDTH, /* 0Ch Set max and min width of xGMI Link */
> + HSMP_SET_DF_PSTATE, /* 0Dh Alter APEnable/Disable messages behavior */
> + HSMP_SET_AUTO_DF_PSTATE, /* 0Eh Enable DF P-State Performance Boost algorithm */
> + HSMP_GET_FCLK_MCLK, /* 0Fh Get FCLK and MEMCLK for current socket */
> + HSMP_GET_CCLK_THROTTLE_LIMIT, /* 10h Get CCLK frequency limit in socket */
> + HSMP_GET_C0_PERCENT, /* 11h Get average C0 residency in socket */
> + HSMP_SET_NBIO_DPM_LEVEL, /* 12h Set max/min LCLK DPM Level for a given NBIO */
> + /* 13h Reserved */
> + HSMP_GET_DDR_BANDWIDTH = 0x14, /* 14h Get theoretical maximum and current DDR Bandwidth */
> + HSMP_GET_TEMP_MONITOR, /* 15h Get per-DIMM temperature and refresh rates */
> + HSMP_MSG_ID_MAX,
> +};
> +
> +struct hsmp_message {
> + __u32 msg_id; /* Message ID */
> + __u16 num_args; /* Number of arguments in message */
> + __u16 response_sz; /* Number of expected response words */
> + __u32 args[HSMP_MAX_MSG_LEN]; /* Argument(s) */
> + __u32 response[HSMP_MAX_MSG_LEN]; /* Response word(s) */
> + __u16 sock_ind; /* socket number */
> +};
> +
> +struct hsmp_msg_desc {
> + int num_args;
> + int response_sz;
> + int type;
> +};
> +
> +enum hsmp_msg_type {
> + RSVD = -1,
> + SET = 0,
> + GET = 1,
> +};
> +
> +static const struct hsmp_msg_desc msg_desc_table[] = {
> + /* num_args, response_size, type */
> + {0, 0, RSVD}, /* RESERVED */
> + {1, 1, GET}, /* HSMP_TEST */
> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
> + {0, 0, RSVD}, /* RESERVED */
> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
> +};
The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
validating user data. These aren't part of the user API.
Perhaps these should be defined in the driver itself instead of being a part of
the uapi header.
-Nathan
> +
> +/* Reset to default packing */
> +#pragma pack()
> +
> +/* Define unique ioctl command for hsmp msgs using generic _IOWR */
> +#define HSMP_BASE_IOCTL_NR 0xF8
> +#define HSMP_IOCTL_CMD _IOWR(HSMP_BASE_IOCTL_NR, 0, struct hsmp_message)
> +
> +#endif /*_ASM_X86_AMD_HSMP_H_*/
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 24deeeb29af2..0906c36ea07b 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -210,6 +210,19 @@ config AMD_PMC
> If you choose to compile this driver as a module the module will be
> called amd-pmc.
>
> +config AMD_HSMP
> + tristate "AMD HSMP Driver"
> + depends on AMD_NB && X86_64
> + help
> + The driver provides a way for user space tools to monitor and manage
> + system management functionality on EPYC server CPUs from AMD.
> +
> + Host System Management Port (HSMP) interface is a mailbox interface
> + between the x86 core and the System Management Unit (SMU) firmware.
> +
> + If you choose to compile this driver as a module the module will be
> + called amd_hsmp.
> +
> config ADV_SWBUTTON
> tristate "Advantech ACPI Software Button Driver"
> depends on ACPI && INPUT
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index c12a9b044fd8..b3a93a5053a3 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_ACER_WMI) += acer-wmi.o
>
> # AMD
> obj-$(CONFIG_AMD_PMC) += amd-pmc.o
> +obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o
>
> # Advantech
> obj-$(CONFIG_ADV_SWBUTTON) += adv_swbutton.o
> diff --git a/drivers/platform/x86/amd_hsmp.c b/drivers/platform/x86/amd_hsmp.c
> new file mode 100644
> index 000000000000..858f194e5a4c
> --- /dev/null
> +++ b/drivers/platform/x86/amd_hsmp.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD HSMP Platform Driver
> + * Copyright (c) 2022, AMD.
> + * All Rights Reserved.
> + *
> + * This file provides a device implementation for HSMP interface
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <asm/amd_hsmp.h>
> +#include <asm/amd_nb.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/semaphore.h>
> +
> +#define DRIVER_NAME "amd_hsmp"
> +#define DRIVER_VERSION "1.0"
> +
> +/* HSMP Status / Error codes */
> +#define HSMP_STATUS_NOT_READY 0x00
> +#define HSMP_STATUS_OK 0x01
> +#define HSMP_ERR_INVALID_MSG 0xFE
> +#define HSMP_ERR_INVALID_INPUT 0xFF
> +
> +/* Timeout in millsec */
> +#define HSMP_MSG_TIMEOUT 100
> +#define HSMP_SHORT_SLEEP 1
> +
> +#define HSMP_WR true
> +#define HSMP_RD false
> +
> +/*
> + * To access specific HSMP mailbox register, s/w writes the SMN address of HSMP mailbox
> + * register into the SMN_INDEX register, and reads/writes the SMN_DATA reg.
> + * Below are required SMN address for HSMP Mailbox register offsets in SMU address space
> + */
> +#define SMN_HSMP_MSG_ID 0x3B10534
> +#define SMN_HSMP_MSG_RESP 0x3B10980
> +#define SMN_HSMP_MSG_DATA 0x3B109E0
> +
> +#define HSMP_INDEX_REG 0xc4
> +#define HSMP_DATA_REG 0xc8
> +
> +static struct semaphore *hsmp_sem;
> +
> +static struct miscdevice hsmp_device;
> +
> +static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
> + u32 *value, bool write)
> +{
> + int ret;
> +
> + ret = pci_write_config_dword(root, HSMP_INDEX_REG, address);
> + if (ret)
> + return ret;
> +
> + ret = (write ? pci_write_config_dword(root, HSMP_DATA_REG, *value)
> + : pci_read_config_dword(root, HSMP_DATA_REG, value));
> +
> + return ret;
> +}
> +
> +/*
> + * Send a message to the HSMP port via PCI-e config space registers.
> + *
> + * The caller is expected to zero out any unused arguments.
> + * If a response is expected, the number of response words should be greater than 0.
> + *
> + * Returns 0 for success and populates the requested number of arguments.
> + * Returns a negative error code for failure.
> + */
> +static int __hsmp_send_message(struct pci_dev *root, struct hsmp_message *msg)
> +{
> + unsigned long timeout, short_sleep;
> + u32 mbox_status;
> + u32 arg_num;
> + int ret;
> +
> + /* Clear the status register */
> + mbox_status = HSMP_STATUS_NOT_READY;
> + ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_WR);
> + if (ret) {
> + pr_err("Error %d clearing mailbox status register\n", ret);
> + return ret;
> + }
> +
> + arg_num = 0;
> + /* Write any message arguments */
> + while (arg_num < msg->num_args) {
> + ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (arg_num << 2),
> + &msg->args[arg_num], HSMP_WR);
> + if (ret) {
> + pr_err("Error %d writing message argument %d\n", ret, arg_num);
> + return ret;
> + }
> + arg_num++;
> + }
> +
> + /* Write the message ID which starts the operation */
> + ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_ID, &msg->msg_id, HSMP_WR);
> + if (ret) {
> + pr_err("Error %d writing message ID %u\n", ret, msg->msg_id);
> + return ret;
> + }
> +
> + /*
> + * Depending on when the trigger write completes relative to the SMU
> + * firmware 1 ms cycle, the operation may take from tens of us to 1 ms
> + * to complete. Some operations may take more. Therefore we will try
> + * a few short duration sleeps and switch to long sleeps if we don't
> + * succeed quickly.
> + */
> + short_sleep = jiffies + msecs_to_jiffies(HSMP_SHORT_SLEEP);
> + timeout = jiffies + msecs_to_jiffies(HSMP_MSG_TIMEOUT);
> +
> + while (time_before(jiffies, timeout)) {
> + ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_RESP, &mbox_status, HSMP_RD);
> + if (ret) {
> + pr_err("Error %d reading mailbox status\n", ret);
> + return ret;
> + }
> +
> + if (mbox_status != HSMP_STATUS_NOT_READY)
> + break;
> + if (time_before(jiffies, short_sleep))
> + usleep_range(50, 100);
> + else
> + usleep_range(1000, 2000);
> + }
> +
> + if (unlikely(mbox_status == HSMP_STATUS_NOT_READY)) {
> + return -ETIMEDOUT;
> + } else if (unlikely(mbox_status == HSMP_ERR_INVALID_MSG)) {
> + return -ENOMSG;
> + } else if (unlikely(mbox_status == HSMP_ERR_INVALID_INPUT)) {
> + return -EINVAL;
> + } else if (unlikely(mbox_status != HSMP_STATUS_OK)) {
> + pr_err("Message ID %u unknown failure (status = 0x%X)\n",
> + msg->msg_id, mbox_status);
> + return -EIO;
> + }
> +
> + /* SMU has responded OK. Read response data */
> + arg_num = 0;
> + while (arg_num < msg->response_sz) {
> + ret = amd_hsmp_rdwr(root, SMN_HSMP_MSG_DATA + (arg_num << 2),
> + &msg->response[arg_num], HSMP_RD);
> + if (ret) {
> + pr_err("Error %d reading response %u for message ID:%u\n",
> + ret, arg_num, msg->msg_id);
> + break;
> + }
> + arg_num++;
> + }
> +
> + return ret;
> +}
> +
> +static int validate_message(struct hsmp_message *msg)
> +{
> + /* Message ID within valid range of message IDs */
> + if (msg->msg_id < HSMP_TEST || msg->msg_id >= HSMP_MSG_ID_MAX)
> + return -EINVAL;
> +
> + /* Reserved message IDs */
> + if (msg_desc_table[msg->msg_id].type == RSVD)
> + return -EINVAL;
> +
> + /* Message with number of arguments and response size not matching the HSMP spec */
> + if (msg->num_args != msg_desc_table[msg->msg_id].num_args ||
> + msg->response_sz != msg_desc_table[msg->msg_id].response_sz)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +int hsmp_send_message(struct hsmp_message *msg)
> +{
> + struct amd_northbridge *nb;
> + int ret;
> +
> + if (!msg)
> + return -EINVAL;
> +
> + nb = node_to_amd_nb(msg->sock_ind);
> + if (!nb || !nb->root)
> + return -ENODEV;
> +
> + if (validate_message(msg))
> + return -EINVAL;
> +
> + /*
> + * The time taken by smu operation to complete is between
> + * 10us to 1ms. Sometime it may take more time.
> + * In SMP system timeout of 100 millisecs should
> + * be enough for the previous thread to finish the operation
> + */
> + ret = down_timeout(&hsmp_sem[msg->sock_ind],
> + msecs_to_jiffies(HSMP_MSG_TIMEOUT));
> + if (ret < 0)
> + return ret;
> +
> + ret = __hsmp_send_message(nb->root, msg);
> +
> + up(&hsmp_sem[msg->sock_ind]);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(hsmp_send_message);
> +
> +static int hsmp_test(u16 sock_ind)
> +{
> + struct hsmp_message msg = { 0 };
> + struct amd_northbridge *nb;
> + int ret = -ENODEV;
> +
> + nb = node_to_amd_nb(sock_ind);
> + if (!nb || !nb->root)
> + return ret;
Small nit. This is the only place you return 'ret', it's initialized before
it's returned elsewhere in the function. You could just return -ENODEV here
and avoid initializing ret above.
> +
> + /*
> + * Test the hsmp port by performing TEST command. The test message takes
> + * one argument and returns the value of that argument + 1.
> + */
> + msg.sock_ind = sock_ind;
> + msg.response_sz = 1;
> + msg.num_args = 1;
> + msg.msg_id = HSMP_TEST;
> + msg.args[0] = 0xDEADBEEF;
> +
> + ret = __hsmp_send_message(nb->root, &msg);
> + if (ret)
> + return ret;
> +
> + if (msg.response[0] != (msg.args[0] + 1)) {
> + pr_err("Socket %d test message failed, Expected 0x%08X, received 0x%08X\n",
> + sock_ind, msg.args[0] + 1, msg.response[0]);
> + return -EBADE;
> + }
> +
> + return 0;
> +}
> +
> +static long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> +{
> + int __user *arguser = (int __user *)arg;
> + struct hsmp_message msg = { 0 };
> + int ret;
> +
> + if (copy_struct_from_user(&msg, sizeof(msg), arguser, sizeof(struct hsmp_message)))
> + return -EFAULT;
> +
> + if (validate_message(&msg))
> + return -EINVAL;
> +
> + switch (fp->f_mode & (FMODE_WRITE | FMODE_READ)) {
> + case FMODE_WRITE:
> + /*
> + * Device is opened in O_WRONLY mode
> + * Execute only set/configure commands
> + */
> + if (msg_desc_table[msg.msg_id].type != SET)
> + return -EINVAL;
> + break;
> + case FMODE_READ:
> + /*
> + * Device is opened in O_RDONLY mode
> + * Execute only get/monitor commands
> + */
> + if (msg_desc_table[msg.msg_id].type != GET)
> + return -EINVAL;
> + break;
> + case FMODE_READ | FMODE_WRITE:
> + /*
> + * Device is opened in O_RDWR mode
> + * Execute both get/monitor and set/configure commands
> + */
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = hsmp_send_message(&msg);
> + if (ret)
> + return ret;
> +
> + if (msg_desc_table[msg.msg_id].response_sz > 0) {
> + /* Copy results back to user for get/monitor commands */
> + if (copy_to_user(arguser, &msg, sizeof(struct hsmp_message)))
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +static const struct file_operations hsmp_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = hsmp_ioctl,
> + .compat_ioctl = hsmp_ioctl,
> +};
> +
> +static int hsmp_pltdrv_probe(struct platform_device *pdev)
> +{
> + int ret, i;
> +
> + hsmp_sem = devm_kzalloc(&pdev->dev,
> + (amd_nb_num() * sizeof(struct semaphore)),
> + GFP_KERNEL);
> + if (!hsmp_sem)
> + return -ENOMEM;
> +
> + for (i = 0; i < amd_nb_num(); i++)
> + sema_init(&hsmp_sem[i], 1);
> +
> + hsmp_device.name = "hsmp_cdev";
> + hsmp_device.minor = MISC_DYNAMIC_MINOR;
> + hsmp_device.fops = &hsmp_fops;
> + hsmp_device.parent = &pdev->dev;
> + hsmp_device.nodename = "hsmp";
> + hsmp_device.mode = 0644;
> +
> + ret = misc_register(&hsmp_device);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int hsmp_pltdrv_remove(struct platform_device *pdev)
> +{
> + misc_deregister(&hsmp_device);
> +
> + return 0;
> +}
> +
> +static struct platform_driver amd_hsmp_driver = {
> + .probe = hsmp_pltdrv_probe,
> + .remove = hsmp_pltdrv_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + },
> +};
> +
> +static struct platform_device *amd_hsmp_platdev;
> +
> +static int __init hsmp_plt_init(void)
> +{
> + int ret = -ENODEV;
> + u16 num_sockets;
> + int i;
> +
> + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
> + pr_err("HSMP is not supported on Family:%x model:%x\n",
> + boot_cpu_data.x86, boot_cpu_data.x86_model);
> + return ret;
> + }
> +
> + /*
> + * amd_nb_num() returns number of SMN/DF interfaces present in the system
> + * if we have N SMN/DF interfaces that ideally means N sockets
> + */
> + num_sockets = amd_nb_num();
> + if (num_sockets == 0)
> + return ret;
> +
> + /* Test the hsmp interface on each socket */
> + for (i = 0; i < num_sockets; i++) {
> + ret = hsmp_test(i);
> + if (ret) {
> + pr_err("HSMP is not supported on Fam:%x model:%x\n",
> + boot_cpu_data.x86, boot_cpu_data.x86_model);
> + pr_err("Or Is HSMP disabled in BIOS ?\n");
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + ret = platform_driver_register(&amd_hsmp_driver);
> + if (ret)
> + return ret;
> +
> + amd_hsmp_platdev = platform_device_alloc(DRIVER_NAME, -1);
> + if (!amd_hsmp_platdev) {
> + ret = -ENOMEM;
> + goto drv_unregister;
> + }
> +
> + ret = platform_device_add(amd_hsmp_platdev);
> + if (ret) {
> + platform_device_put(amd_hsmp_platdev);
> + goto drv_unregister;
> + }
> +
> + return 0;
> +
> +drv_unregister:
> + platform_driver_unregister(&amd_hsmp_driver);
> + return ret;
> +}
> +
> +static void __exit hsmp_plt_exit(void)
> +{
> + platform_device_unregister(amd_hsmp_platdev);
> + platform_driver_unregister(&amd_hsmp_driver);
> +}
> +
> +device_initcall(hsmp_plt_init);
> +module_exit(hsmp_plt_exit);
> +
> +MODULE_DESCRIPTION("AMD HSMP Platform Interface Driver");
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-09 21:08 ` [PATCH v4 1/2] platforms/x86: Add AMD system management interface Nathan Fontenot
@ 2022-02-10 1:10 ` Song Liu
2022-02-10 19:52 ` Nathan Fontenot
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2022-02-10 1:10 UTC (permalink / raw)
To: Nathan Fontenot
Cc: Naveen Krishna Chatradhi, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
>
[...]
> > +
> > +static const struct hsmp_msg_desc msg_desc_table[] = {
> > + /* num_args, response_size, type */
> > + {0, 0, RSVD}, /* RESERVED */
> > + {1, 1, GET}, /* HSMP_TEST */
> > + {0, 1, GET}, /* HSMP_GET_SMU_VER */
> > + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
> > + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
> > + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
> > + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
> > + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
> > + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
> > + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
> > + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
> > + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
> > + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
> > + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
> > + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
> > + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
> > + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
> > + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
> > + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
> > + {0, 0, RSVD}, /* RESERVED */
> > + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
> > + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
> > +};
>
> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
> validating user data. These aren't part of the user API.
>
> Perhaps these should be defined in the driver itself instead of being a part of
> the uapi header.
This was my idea. While I agree it is a little weird to have these
tables in a uapi
header, I think it is helpful to give the user some reference about
proper num_args
and response_size for each message. I don't have a better idea to achieve this.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-10 1:10 ` Song Liu
@ 2022-02-10 19:52 ` Nathan Fontenot
2022-02-10 20:32 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Fontenot @ 2022-02-10 19:52 UTC (permalink / raw)
To: Song Liu
Cc: Naveen Krishna Chatradhi, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On 2/9/22 19:10, Song Liu wrote:
> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
>>
> [...]
>>> +
>>> +static const struct hsmp_msg_desc msg_desc_table[] = {
>>> + /* num_args, response_size, type */
>>> + {0, 0, RSVD}, /* RESERVED */
>>> + {1, 1, GET}, /* HSMP_TEST */
>>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
>>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
>>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
>>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
>>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
>>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
>>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
>>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
>>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
>>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
>>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
>>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
>>> + {0, 0, RSVD}, /* RESERVED */
>>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
>>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
>>> +};
>>
>> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
>> validating user data. These aren't part of the user API.
>>
>> Perhaps these should be defined in the driver itself instead of being a part of
>> the uapi header.
>
> This was my idea. While I agree it is a little weird to have these
> tables in a uapi
> header, I think it is helpful to give the user some reference about
> proper num_args
> and response_size for each message. I don't have a better idea to achieve this.
>
I like the idea to give users a reference on args and responses for each HSMP function.
If this table is kept in the uapi header perhaps we should add a short description of
what the expected args and responses are for each HSMP function with a pointer to the
full documentation of the HSMP functions in the PPR.
-Nathan
> Thanks,
> Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-10 19:52 ` Nathan Fontenot
@ 2022-02-10 20:32 ` Song Liu
2022-02-10 20:42 ` Nathan Fontenot
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2022-02-10 20:32 UTC (permalink / raw)
To: Nathan Fontenot
Cc: Naveen Krishna Chatradhi, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@amd.com> wrote:
>
> On 2/9/22 19:10, Song Liu wrote:
> > On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
> >>
> > [...]
> >>> +
> >>> +static const struct hsmp_msg_desc msg_desc_table[] = {
> >>> + /* num_args, response_size, type */
> >>> + {0, 0, RSVD}, /* RESERVED */
> >>> + {1, 1, GET}, /* HSMP_TEST */
> >>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
> >>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
> >>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
> >>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
> >>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
> >>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
> >>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
> >>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
> >>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
> >>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
> >>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
> >>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
> >>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
> >>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
> >>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
> >>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
> >>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
> >>> + {0, 0, RSVD}, /* RESERVED */
> >>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
> >>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
> >>> +};
> >>
> >> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
> >> validating user data. These aren't part of the user API.
> >>
> >> Perhaps these should be defined in the driver itself instead of being a part of
> >> the uapi header.
> >
> > This was my idea. While I agree it is a little weird to have these
> > tables in a uapi
> > header, I think it is helpful to give the user some reference about
> > proper num_args
> > and response_size for each message. I don't have a better idea to achieve this.
> >
>
> I like the idea to give users a reference on args and responses for each HSMP function.
> If this table is kept in the uapi header perhaps we should add a short description of
> what the expected args and responses are for each HSMP function with a pointer to the
> full documentation of the HSMP functions in the PPR.
I guess we can use unions do give full descriptions, like:
struct hsmp_message {
__u32 msg_id; /* Message ID */
__u16 num_args; /* Number of arguments
in message */
__u16 response_sz; /* Number of expected
response words */
union {
struct {
__u32 args[HSMP_MAX_MSG_LEN];
}; /* ensure size of args */
struct {
__u32 test_arg1;
} hsmp_test;
/* args for other commands */
} args;
union {
struct {
__u32 response[HSMP_MAX_MSG_LEN];
}; /* ensure size of response */
struct {
__u32 test_response1; /* or better name */
} hsmp_test;
/* reponse for other commands */
} response;
__u16 sock_ind; /* socket number */
};
btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
have at most 2 args/responses.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-10 20:32 ` Song Liu
@ 2022-02-10 20:42 ` Nathan Fontenot
2022-02-10 21:40 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Fontenot @ 2022-02-10 20:42 UTC (permalink / raw)
To: Song Liu
Cc: Naveen Krishna Chatradhi, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On 2/10/22 14:32, Song Liu wrote:
> On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@amd.com> wrote:
>>
>> On 2/9/22 19:10, Song Liu wrote:
>>> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
>>>>
>>> [...]
>>>>> +
>>>>> +static const struct hsmp_msg_desc msg_desc_table[] = {
>>>>> + /* num_args, response_size, type */
>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>> + {1, 1, GET}, /* HSMP_TEST */
>>>>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
>>>>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
>>>>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
>>>>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
>>>>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
>>>>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
>>>>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
>>>>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
>>>>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
>>>>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
>>>>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
>>>>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
>>>>> +};
>>>>
>>>> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
>>>> validating user data. These aren't part of the user API.
>>>>
>>>> Perhaps these should be defined in the driver itself instead of being a part of
>>>> the uapi header.
>>>
>>> This was my idea. While I agree it is a little weird to have these
>>> tables in a uapi
>>> header, I think it is helpful to give the user some reference about
>>> proper num_args
>>> and response_size for each message. I don't have a better idea to achieve this.
>>>
>>
>> I like the idea to give users a reference on args and responses for each HSMP function.
>> If this table is kept in the uapi header perhaps we should add a short description of
>> what the expected args and responses are for each HSMP function with a pointer to the
>> full documentation of the HSMP functions in the PPR.
>
> I guess we can use unions do give full descriptions, like:
>
> struct hsmp_message {
> __u32 msg_id; /* Message ID */
> __u16 num_args; /* Number of arguments
> in message */
> __u16 response_sz; /* Number of expected
> response words */
> union {
> struct {
> __u32 args[HSMP_MAX_MSG_LEN];
> }; /* ensure size of args */
> struct {
> __u32 test_arg1;
> } hsmp_test;
> /* args for other commands */
> } args;
> union {
> struct {
> __u32 response[HSMP_MAX_MSG_LEN];
> }; /* ensure size of response */
> struct {
> __u32 test_response1; /* or better name */
> } hsmp_test;
> /* reponse for other commands */
> } response;
> __u16 sock_ind; /* socket number */
> };
>
I was thinking of keeping the msg_desc_table as is, just provide more details
about the expected args and responses in a comment. I think creating a union
of structs for each HSMP function (and there are more functions coming) would
get a bit messy.
> btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
> have at most 2 args/responses.
The PPR spec defines the args and responses as having up to 8 so we ned to keep
the max length at 8. No current HSMP has more than 2 though.
-Nathan
>
> Thanks,
> Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-10 20:42 ` Nathan Fontenot
@ 2022-02-10 21:40 ` Song Liu
2022-02-14 15:32 ` Chatradhi, Naveen Krishna
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2022-02-10 21:40 UTC (permalink / raw)
To: Nathan Fontenot
Cc: Naveen Krishna Chatradhi, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On Thu, Feb 10, 2022 at 12:42 PM Nathan Fontenot <nafonten@amd.com> wrote:
>
> On 2/10/22 14:32, Song Liu wrote:
> > On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@amd.com> wrote:
> >>
> >> On 2/9/22 19:10, Song Liu wrote:
> >>> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
> >>>>
> >>> [...]
> >>>>> +
> >>>>> +static const struct hsmp_msg_desc msg_desc_table[] = {
> >>>>> + /* num_args, response_size, type */
> >>>>> + {0, 0, RSVD}, /* RESERVED */
> >>>>> + {1, 1, GET}, /* HSMP_TEST */
> >>>>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
> >>>>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
> >>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
> >>>>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
> >>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
> >>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
> >>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
> >>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
> >>>>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
> >>>>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
> >>>>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
> >>>>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
> >>>>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
> >>>>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
> >>>>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
> >>>>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
> >>>>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
> >>>>> + {0, 0, RSVD}, /* RESERVED */
> >>>>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
> >>>>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
> >>>>> +};
> >>>>
> >>>> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
> >>>> validating user data. These aren't part of the user API.
> >>>>
> >>>> Perhaps these should be defined in the driver itself instead of being a part of
> >>>> the uapi header.
> >>>
> >>> This was my idea. While I agree it is a little weird to have these
> >>> tables in a uapi
> >>> header, I think it is helpful to give the user some reference about
> >>> proper num_args
> >>> and response_size for each message. I don't have a better idea to achieve this.
> >>>
> >>
> >> I like the idea to give users a reference on args and responses for each HSMP function.
> >> If this table is kept in the uapi header perhaps we should add a short description of
> >> what the expected args and responses are for each HSMP function with a pointer to the
> >> full documentation of the HSMP functions in the PPR.
> >
> > I guess we can use unions do give full descriptions, like:
> >
> > struct hsmp_message {
> > __u32 msg_id; /* Message ID */
> > __u16 num_args; /* Number of arguments
> > in message */
> > __u16 response_sz; /* Number of expected
> > response words */
> > union {
> > struct {
> > __u32 args[HSMP_MAX_MSG_LEN];
> > }; /* ensure size of args */
> > struct {
> > __u32 test_arg1;
> > } hsmp_test;
> > /* args for other commands */
> > } args;
> > union {
> > struct {
> > __u32 response[HSMP_MAX_MSG_LEN];
> > }; /* ensure size of response */
> > struct {
> > __u32 test_response1; /* or better name */
> > } hsmp_test;
> > /* reponse for other commands */
> > } response;
> > __u16 sock_ind; /* socket number */
> > };
> >
>
> I was thinking of keeping the msg_desc_table as is, just provide more details
> about the expected args and responses in a comment. I think creating a union
> of structs for each HSMP function (and there are more functions coming) would
> get a bit messy.
Yeah, I think msg_desc_table with detailed comments also works.
>
> > btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
> > have at most 2 args/responses.
>
> The PPR spec defines the args and responses as having up to 8 so we ned to keep
> the max length at 8. No current HSMP has more than 2 though.
Got it. Thanks for the explanation.
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-10 21:40 ` Song Liu
@ 2022-02-14 15:32 ` Chatradhi, Naveen Krishna
2022-02-14 17:21 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Chatradhi, Naveen Krishna @ 2022-02-14 15:32 UTC (permalink / raw)
To: Song Liu, Nathan Fontenot
Cc: platform-driver-x86, Hans de Goede, carlos.bilbao,
siva.sathappan, suma.hegde
Hi Song,
On 2/11/2022 3:10 AM, Song Liu wrote:
> [CAUTION: External Email]
>
> On Thu, Feb 10, 2022 at 12:42 PM Nathan Fontenot <nafonten@amd.com> wrote:
>> On 2/10/22 14:32, Song Liu wrote:
>>> On Thu, Feb 10, 2022 at 11:52 AM Nathan Fontenot <nafonten@amd.com> wrote:
>>>> On 2/9/22 19:10, Song Liu wrote:
>>>>> On Wed, Feb 9, 2022 at 1:08 PM Nathan Fontenot <nafonten@amd.com> wrote:
>>>>> [...]
>>>>>>> +
>>>>>>> +static const struct hsmp_msg_desc msg_desc_table[] = {
>>>>>>> + /* num_args, response_size, type */
>>>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>>>> + {1, 1, GET}, /* HSMP_TEST */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_SMU_VER */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_PROTO_VER */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_SOCKET_POWER_LIMIT */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_SOCKET_POWER_LIMIT_MAX */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_BOOST_LIMIT_SOCKET */
>>>>>>> + {1, 1, GET}, /* HSMP_GET_BOOST_LIMIT */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_PROC_HOT */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_XGMI_LINK_WIDTH */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_DF_PSTATE */
>>>>>>> + {0, 0, SET}, /* HSMP_SET_AUTO_DF_PSTATE */
>>>>>>> + {0, 2, GET}, /* HSMP_GET_FCLK_MCLK */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_CCLK_THROTTLE_LIMIT */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_C0_PERCENT */
>>>>>>> + {1, 0, SET}, /* HSMP_SET_NBIO_DPM_LEVEL */
>>>>>>> + {0, 0, RSVD}, /* RESERVED */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_DDR_BANDWIDTH */
>>>>>>> + {0, 1, GET}, /* HSMP_GET_TEMP_MONITOR */
>>>>>>> +};
>>>>>> The hsmp_msg_desc, hsmp_msg_type, and msg_desc_table are used by the driver for
>>>>>> validating user data. These aren't part of the user API.
>>>>>>
>>>>>> Perhaps these should be defined in the driver itself instead of being a part of
>>>>>> the uapi header.
>>>>> This was my idea. While I agree it is a little weird to have these
>>>>> tables in a uapi
>>>>> header, I think it is helpful to give the user some reference about
>>>>> proper num_args
>>>>> and response_size for each message. I don't have a better idea to achieve this.
>>>>>
>>>> I like the idea to give users a reference on args and responses for each HSMP function.
>>>> If this table is kept in the uapi header perhaps we should add a short description of
>>>> what the expected args and responses are for each HSMP function with a pointer to the
>>>> full documentation of the HSMP functions in the PPR.
>>> I guess we can use unions do give full descriptions, like:
>>>
>>> struct hsmp_message {
>>> __u32 msg_id; /* Message ID */
>>> __u16 num_args; /* Number of arguments
>>> in message */
>>> __u16 response_sz; /* Number of expected
>>> response words */
>>> union {
>>> struct {
>>> __u32 args[HSMP_MAX_MSG_LEN];
>>> }; /* ensure size of args */
>>> struct {
>>> __u32 test_arg1;
>>> } hsmp_test;
>>> /* args for other commands */
>>> } args;
>>> union {
>>> struct {
>>> __u32 response[HSMP_MAX_MSG_LEN];
>>> }; /* ensure size of response */
>>> struct {
>>> __u32 test_response1; /* or better name */
>>> } hsmp_test;
>>> /* reponse for other commands */
>>> } response;
>>> __u16 sock_ind; /* socket number */
>>> };
>>>
>> I was thinking of keeping the msg_desc_table as is, just provide more details
>> about the expected args and responses in a comment. I think creating a union
>> of structs for each HSMP function (and there are more functions coming) would
>> get a bit messy.
> Yeah, I think msg_desc_table with detailed comments also works.
HSMP mailbox messages are evolving and each platform defines a supported
list of messages.
On a given platform these messages are described in the PPR.
Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public
https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
Bringing detailed description of these messages from PPR into the kernel
would be a
duplicating effort. Which will also bring in challenges such as
maintaining the details
for every supported platform and submitting kernel patches for every
platform.
We would like to avoid bringing more details of these messages to the
kernel documentation.
Such a structure can be described as part of esmi_oob_library to ease
user space tool development.
>
>>> btw: do we really need HSMP_MAX_MSG_LEN of 8? The list above
>>> have at most 2 args/responses.
>> The PPR spec defines the args and responses as having up to 8 so we ned to keep
>> the max length at 8. No current HSMP has more than 2 though.
> Got it. Thanks for the explanation.
>
> Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-14 15:32 ` Chatradhi, Naveen Krishna
@ 2022-02-14 17:21 ` Song Liu
2022-02-15 17:24 ` Nathan Fontenot
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2022-02-14 17:21 UTC (permalink / raw)
To: Chatradhi, Naveen Krishna
Cc: Nathan Fontenot, platform-driver-x86, Hans de Goede,
carlos.bilbao, siva.sathappan, suma.hegde
On Mon, Feb 14, 2022 at 7:32 AM Chatradhi, Naveen Krishna
<nchatrad@amd.com> wrote:
>
> Hi Song,
>
> On 2/11/2022 3:10 AM, Song Liu wrote:
[...]
>
> HSMP mailbox messages are evolving and each platform defines a supported
> list of messages.
>
> On a given platform these messages are described in the PPR.
>
> Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public
>
> https://www.amd.com/system/files/TechDocs/55898_B1_pub_0.50.zip
>
>
> Bringing detailed description of these messages from PPR into the kernel
> would be a
>
> duplicating effort. Which will also bring in challenges such as
> maintaining the details
>
> for every supported platform and submitting kernel patches for every
> platform.
>
>
> We would like to avoid bringing more details of these messages to the
> kernel documentation.
>
> Such a structure can be described as part of esmi_oob_library to ease
> user space tool development.
I agree there is extra effort to adding extra logic and documentations to
the kernel. How about we ship current version with a few minor changes:
1). msg_desc_table is in the header, so please prefix it with hsmp_
2) as Nathan suggested, add more comments to msg_desc_table. Maybe
something like:
/*
* HSMP_GET_FCLK_MCLK,
* output arg0 = fclk (MHz); arg1 = mclk (MHz)
*/
{0, 2, GET},
Besides these, I have a question. Per the hardware design, args and
reponse in hsmp_message share the same registers. Shall we make
them in/out argments in hsmp_message and save 256 bytes per
hsmp_message?
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-14 17:21 ` Song Liu
@ 2022-02-15 17:24 ` Nathan Fontenot
2022-02-15 18:21 ` Song Liu
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Fontenot @ 2022-02-15 17:24 UTC (permalink / raw)
To: Song Liu, Chatradhi, Naveen Krishna
Cc: platform-driver-x86, Hans de Goede, carlos.bilbao,
siva.sathappan, suma.hegde
On 2/14/22 11:21, Song Liu wrote:
> On Mon, Feb 14, 2022 at 7:32 AM Chatradhi, Naveen Krishna
> <nchatrad@amd.com> wrote:
>>
>> Hi Song,
>>
>> On 2/11/2022 3:10 AM, Song Liu wrote:
> [...]
>>
>> HSMP mailbox messages are evolving and each platform defines a supported
>> list of messages.
>>
>> On a given platform these messages are described in the PPR.
>>
>> Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F55898_B1_pub_0.50.zip&data=04%7C01%7CNathan.Fontenot%40amd.com%7C6026990f93ed41d04d0608d9efde79a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804561503265585%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=o5BR%2FgHpjnNVa9zVu75I3RyzO8WNB4L7FMEvLNbzZVY%3D&reserved=0
>>
>>
>> Bringing detailed description of these messages from PPR into the kernel
>> would be a
>>
>> duplicating effort. Which will also bring in challenges such as
>> maintaining the details
>>
>> for every supported platform and submitting kernel patches for every
>> platform.
>>
>>
>> We would like to avoid bringing more details of these messages to the
>> kernel documentation.
>>
>> Such a structure can be described as part of esmi_oob_library to ease
>> user space tool development.
>
> I agree there is extra effort to adding extra logic and documentations to
> the kernel. How about we ship current version with a few minor changes:
> 1). msg_desc_table is in the header, so please prefix it with hsmp_
> 2) as Nathan suggested, add more comments to msg_desc_table. Maybe
> something like:
> /*
> * HSMP_GET_FCLK_MCLK,
> * output arg0 = fclk (MHz); arg1 = mclk (MHz)
> */
Agreed, this is the type of update I was thinking of.
-Nathan
> {0, 2, GET},
>
> Besides these, I have a question. Per the hardware design, args and
> reponse in hsmp_message share the same registers. Shall we make
> them in/out argments in hsmp_message and save 256 bytes per
> hsmp_message?
>
> Thanks,
> Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-15 17:24 ` Nathan Fontenot
@ 2022-02-15 18:21 ` Song Liu
2022-02-16 13:42 ` Chatradhi, Naveen Krishna
0 siblings, 1 reply; 13+ messages in thread
From: Song Liu @ 2022-02-15 18:21 UTC (permalink / raw)
To: Nathan Fontenot, Hans de Goede, Chatradhi, Naveen Krishna
Cc: platform-driver-x86, carlos.bilbao, siva.sathappan, suma.hegde
Hi Naveen Krishna,
On Tue, Feb 15, 2022 at 9:24 AM Nathan Fontenot <nafonten@amd.com> wrote:
>
> On 2/14/22 11:21, Song Liu wrote:
> > On Mon, Feb 14, 2022 at 7:32 AM Chatradhi, Naveen Krishna
> > <nchatrad@amd.com> wrote:
> >>
> >> Hi Song,
> >>
> >> On 2/11/2022 3:10 AM, Song Liu wrote:
> > [...]
> >>
> >> HSMP mailbox messages are evolving and each platform defines a supported
> >> list of messages.
> >>
> >> On a given platform these messages are described in the PPR.
> >>
> >> Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public
> >>
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F55898_B1_pub_0.50.zip&data=04%7C01%7CNathan.Fontenot%40amd.com%7C6026990f93ed41d04d0608d9efde79a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804561503265585%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=o5BR%2FgHpjnNVa9zVu75I3RyzO8WNB4L7FMEvLNbzZVY%3D&reserved=0
> >>
> >>
> >> Bringing detailed description of these messages from PPR into the kernel
> >> would be a
> >>
> >> duplicating effort. Which will also bring in challenges such as
> >> maintaining the details
> >>
> >> for every supported platform and submitting kernel patches for every
> >> platform.
> >>
> >>
> >> We would like to avoid bringing more details of these messages to the
> >> kernel documentation.
> >>
> >> Such a structure can be described as part of esmi_oob_library to ease
> >> user space tool development.
> >
> > I agree there is extra effort to adding extra logic and documentations to
> > the kernel. How about we ship current version with a few minor changes:
> > 1). msg_desc_table is in the header, so please prefix it with hsmp_
> > 2) as Nathan suggested, add more comments to msg_desc_table. Maybe
> > something like:
> > /*
> > * HSMP_GET_FCLK_MCLK,
> > * output arg0 = fclk (MHz); arg1 = mclk (MHz)
> > */
>
> Agreed, this is the type of update I was thinking of.
>
> -Nathan
>
> > {0, 2, GET},
> >
> > Besides these, I have a question. Per the hardware design, args and
> > reponse in hsmp_message share the same registers. Shall we make
> > them in/out argments in hsmp_message and save 256 bytes per
> > hsmp_message?
Do these make sense to you? We are hoping to back port these changes
to unblock our tests. But we need them to be applied to pdx86 first.
Thanks,
Song
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] platforms/x86: Add AMD system management interface
2022-02-15 18:21 ` Song Liu
@ 2022-02-16 13:42 ` Chatradhi, Naveen Krishna
0 siblings, 0 replies; 13+ messages in thread
From: Chatradhi, Naveen Krishna @ 2022-02-16 13:42 UTC (permalink / raw)
To: Song Liu, Nathan Fontenot, Hans de Goede
Cc: platform-driver-x86, carlos.bilbao, siva.sathappan, suma.hegde
Hi Song,
On 2/15/2022 11:51 PM, Song Liu wrote:
> [CAUTION: External Email]
>
> Hi Naveen Krishna,
>
> On Tue, Feb 15, 2022 at 9:24 AM Nathan Fontenot <nafonten@amd.com> wrote:
>> On 2/14/22 11:21, Song Liu wrote:
>>> On Mon, Feb 14, 2022 at 7:32 AM Chatradhi, Naveen Krishna
>>> <nchatrad@amd.com> wrote:
>>>> Hi Song,
>>>>
>>>> On 2/11/2022 3:10 AM, Song Liu wrote:
>>> [...]
>>>> HSMP mailbox messages are evolving and each platform defines a supported
>>>> list of messages.
>>>>
>>>> On a given platform these messages are described in the PPR.
>>>>
>>>> Eg: Milan PPR has "7 Host System Management Port (HSMP)", is made public
>>>>
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F55898_B1_pub_0.50.zip&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C97254107b02b4ae0c30308d9f0b004d4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805461640162776%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=SLrynPAkyTvYswBnj%2BK%2Ff3ELedFn5iOJ6xZtgGcC5XM%3D&reserved=0
>>>>
>>>>
>>>> Bringing detailed description of these messages from PPR into the kernel
>>>> would be a
>>>>
>>>> duplicating effort. Which will also bring in challenges such as
>>>> maintaining the details
>>>>
>>>> for every supported platform and submitting kernel patches for every
>>>> platform.
>>>>
>>>>
>>>> We would like to avoid bringing more details of these messages to the
>>>> kernel documentation.
>>>>
>>>> Such a structure can be described as part of esmi_oob_library to ease
>>>> user space tool development.
>>> I agree there is extra effort to adding extra logic and documentations to
>>> the kernel. How about we ship current version with a few minor changes:
>>> 1). msg_desc_table is in the header, so please prefix it with hsmp_
>>> 2) as Nathan suggested, add more comments to msg_desc_table. Maybe
>>> something like:
>>> /*
>>> * HSMP_GET_FCLK_MCLK,
>>> * output arg0 = fclk (MHz); arg1 = mclk (MHz)
>>> */
>> Agreed, this is the type of update I was thinking of.
>>
>> -Nathan
>>
>>> {0, 2, GET},
>>>
>>> Besides these, I have a question. Per the hardware design, args and
>>> reponse in hsmp_message share the same registers. Shall we make
>>> them in/out argments in hsmp_message and save 256 bytes per
>>> hsmp_message?
> Do these make sense to you? We are hoping to back port these changes
> to unblock our tests. But we need them to be applied to pdx86 first.
Agreed on using one of buf array for both arguments and response data. I've
submitted the next version with these comments addressed.
>
> Thanks,
> Song
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-02-16 13:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 18:44 [PATCH v4 1/2] platforms/x86: Add AMD system management interface Naveen Krishna Chatradhi
2022-02-09 18:44 ` [PATCH v4 2/2] Documentation: Add x86/amd_hsmp driver Naveen Krishna Chatradhi
2022-02-09 21:08 ` [PATCH v4 1/2] platforms/x86: Add AMD system management interface Nathan Fontenot
2022-02-10 1:10 ` Song Liu
2022-02-10 19:52 ` Nathan Fontenot
2022-02-10 20:32 ` Song Liu
2022-02-10 20:42 ` Nathan Fontenot
2022-02-10 21:40 ` Song Liu
2022-02-14 15:32 ` Chatradhi, Naveen Krishna
2022-02-14 17:21 ` Song Liu
2022-02-15 17:24 ` Nathan Fontenot
2022-02-15 18:21 ` Song Liu
2022-02-16 13:42 ` Chatradhi, Naveen Krishna
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.