[PATCH v3 1/3] platform/x86/amd/hsmp: create plat specific struct

[PATCH v3 1/3] platform/x86/amd/hsmp: create plat specific struct

[Suma Hegde]

Having a separate platform device structure helps in future, to
contain platform specific variables and other data.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. defined HSMP_CDEV_NAME and HSMP_DEVNODE_NAME macros
Changes since v2:
1. moved num_sockets variable to plat_dev structure

 drivers/platform/x86/amd/hsmp.c | 61 ++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 31382ef52efb..99727cd705cf 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -47,9 +47,22 @@
 #define HSMP_INDEX_REG		0xc4
 #define HSMP_DATA_REG		0xc8
 
-static struct semaphore *hsmp_sem;
+#define HSMP_CDEV_NAME		"hsmp_cdev"
+#define HSMP_DEVNODE_NAME	"hsmp"
 
-static struct miscdevice hsmp_device;
+struct hsmp_socket {
+	struct semaphore hsmp_sem;
+	u16 sock_ind;
+};
+
+struct hsmp_plat_device {
+	struct miscdevice hsmp_device;
+	struct hsmp_socket *sock;
+	struct device *dev;
+	u16 num_sockets;
+};
+
+static struct hsmp_plat_device plat_dev;
 
 static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
 			 u32 *value, bool write)
@@ -188,6 +201,7 @@ static int validate_message(struct hsmp_message *msg)
 
 int hsmp_send_message(struct hsmp_message *msg)
 {
+	struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
 	struct amd_northbridge *nb;
 	int ret;
 
@@ -208,14 +222,13 @@ int hsmp_send_message(struct hsmp_message *msg)
 	 * 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));
+	ret = down_timeout(&sock->hsmp_sem, msecs_to_jiffies(HSMP_MSG_TIMEOUT));
 	if (ret < 0)
 		return ret;
 
 	ret = __hsmp_send_message(nb->root, msg);
 
-	up(&hsmp_sem[msg->sock_ind]);
+	up(&sock->hsmp_sem);
 
 	return ret;
 }
@@ -321,28 +334,31 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 {
 	int i;
 
-	hsmp_sem = devm_kzalloc(&pdev->dev,
-				(amd_nb_num() * sizeof(struct semaphore)),
-				GFP_KERNEL);
-	if (!hsmp_sem)
+	plat_dev.sock = devm_kzalloc(&pdev->dev,
+				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
+				     GFP_KERNEL);
+	if (!plat_dev.sock)
 		return -ENOMEM;
+	plat_dev.dev = &pdev->dev;
 
-	for (i = 0; i < amd_nb_num(); i++)
-		sema_init(&hsmp_sem[i], 1);
+	for (i = 0; i < plat_dev.num_sockets; i++) {
+		sema_init(&plat_dev.sock[i].hsmp_sem, 1);
+		plat_dev.sock[i].sock_ind = i;
+	}
 
-	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;
+	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
+	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
+	plat_dev.hsmp_device.fops	= &hsmp_fops;
+	plat_dev.hsmp_device.parent	= &pdev->dev;
+	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
+	plat_dev.hsmp_device.mode	= 0644;
 
-	return misc_register(&hsmp_device);
+	return misc_register(&plat_dev.hsmp_device);
 }
 
 static void hsmp_pltdrv_remove(struct platform_device *pdev)
 {
-	misc_deregister(&hsmp_device);
+	misc_deregister(&plat_dev.hsmp_device);
 }
 
 static struct platform_driver amd_hsmp_driver = {
@@ -358,7 +374,6 @@ 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) {
@@ -371,12 +386,12 @@ static int __init hsmp_plt_init(void)
 	 * 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)
+	plat_dev.num_sockets = amd_nb_num();
+	if (plat_dev.num_sockets == 0)
 		return ret;
 
 	/* Test the hsmp interface on each socket */
-	for (i = 0; i < num_sockets; i++) {
+	for (i = 0; i < plat_dev.num_sockets; i++) {
 		ret = hsmp_test(i, 0xDEADBEEF);
 		if (ret) {
 			pr_err("HSMP is not supported on Fam:%x model:%x\n",
-- 
2.25.1

[PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Suma Hegde]

AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
all the system management information from SMU.

The metrics table is made available as hexadecimal sysfs binary file
under per socket sysfs directory created at
/sys/devices/platform/amd_hsmp/socket%d/metrics_bin

Metrics table definitions will be documented as part of Public PPR.
The same is defined in the amd_hsmp.h header.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
this patch
2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
   HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
   definition in amd_hsmp.h files
3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
4. Add hsmp_metric_table_visible() function 
5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
   and code is also modified slightly
6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
7. Change from cleanup label to deregister label
8. Add dev_err print in hsmp_get_tbl_dram_base()
9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
12. Correct ATRR typo error
13. Change sprintf to snprintf
14. Check metrics table support only against HSMP_PROTO_VER6
Changes since v2:
1. squash documentation patch into this patch
2. change from num_sockets to plat_dev.num_sockets

 Documentation/arch/x86/amd_hsmp.rst  |  16 +++
 arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
 drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
 3 files changed, 302 insertions(+), 3 deletions(-)

diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
index 440e4b645a1c..a4c308784818 100644
--- a/Documentation/arch/x86/amd_hsmp.rst
+++ b/Documentation/arch/x86/amd_hsmp.rst
@@ -41,6 +41,22 @@ In-kernel integration:
  * Locking across callers is taken care by the driver.
 
 
+HSMP sysfs interface
+====================
+
+1. Metrics table binary sysfs
+
+AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
+all the system management information from SMU.
+
+The metrics table is made available as hexadecimal sysfs binary file
+under per socket sysfs directory created at
+/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
+
+Metrics table definitions will be documented as part of Public PPR.
+The same is defined in the amd_hsmp.h header.
+
+
 An example
 ==========
 
diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index 769b939444ae..f3479b768fb9 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -47,6 +47,9 @@ enum hsmp_message_ids {
 	HSMP_SET_PCI_RATE,		/* 20h Control link rate on PCIe devices */
 	HSMP_SET_POWER_MODE,		/* 21h Select power efficiency profile policy */
 	HSMP_SET_PSTATE_MAX_MIN,	/* 22h Set the max and min DF P-State  */
+	HSMP_GET_METRIC_TABLE_VER,	/* 23h Get metrics table version */
+	HSMP_GET_METRIC_TABLE,		/* 24h Get metrics table */
+	HSMP_GET_METRIC_TABLE_DRAM_ADDR,/* 25h Get metrics table dram address */
 	HSMP_MSG_ID_MAX,
 };
 
@@ -64,6 +67,14 @@ enum hsmp_msg_type {
 	HSMP_GET  = 1,
 };
 
+enum hsmp_proto_versions {
+	HSMP_PROTO_VER2	= 2,
+	HSMP_PROTO_VER3,
+	HSMP_PROTO_VER4,
+	HSMP_PROTO_VER5,
+	HSMP_PROTO_VER6
+};
+
 struct hsmp_msg_desc {
 	int num_args;
 	int response_sz;
@@ -295,6 +306,104 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
 	 * input: args[0] = min df pstate[15:8] + max df pstate[7:0]
 	 */
 	{1, 0, HSMP_SET},
+
+	/*
+	 * HSMP_GET_METRIC_TABLE_VER, num_args = 0, response_sz = 1
+	 * output: args[0] = metrics table version
+	 */
+	{0, 1, HSMP_GET},
+
+	/*
+	 * HSMP_GET_METRIC_TABLE, num_args = 0, response_sz = 0
+	 */
+	{0, 0, HSMP_GET},
+
+	/*
+	 * HSMP_GET_METRIC_TABLE_DRAM_ADDR, num_args = 0, response_sz = 2
+	 * output: args[0] = lower 32 bits of the address
+	 * output: args[1] = upper 32 bits of the address
+	 */
+	{0, 2, HSMP_GET},
+};
+
+/* Metrics table for EPYC socket(supported only from proto version 6) */
+struct hsmp_metric_table {
+	__u32 accumulation_counter;
+
+	//TEMPERATURE
+	__u32 max_socket_temperature;
+	__u32 max_vr_temperature;
+	__u32 max_hbm_temperature;
+	__u64 max_socket_temperature_acc;
+	__u64 max_vr_temperature_acc;
+	__u64 max_hbm_temperature_acc;
+
+	//POWER
+	__u32 socket_power_limit;
+	__u32 max_socket_power_limit;
+	__u32 socket_power;
+
+	//ENERGY
+	__u64 timestamp;
+	__u64 socket_energy_acc;
+	__u64 ccd_energy_acc;
+	__u64 xcd_energy_acc;
+	__u64 aid_energy_acc;
+	__u64 hbm_energy_acc;
+
+	//FREQUENCY
+	__u32 cclk_frequency_limit;
+	__u32 gfxclk_frequency_limit;
+	__u32 fclk_frequency;
+	__u32 uclk_frequency;
+	__u32 socclk_frequency[4];
+	__u32 vclk_frequency[4];
+	__u32 dclk_frequency[4];
+	__u32 lclk_frequency[4];
+	__u64 gfxclk_frequency_acc[8];
+	__u64 cclk_frequency_acc[96];
+
+	//FREQUENCY RANGE
+	__u32 max_cclk_frequency;
+	__u32 min_cclk_frequency;
+	__u32 max_gfxclk_frequency;
+	__u32 min_gfxclk_frequency;
+	__u32 fclk_frequency_table[4];
+	__u32 uclk_frequency_table[4];
+	__u32 socclk_frequency_table[4];
+	__u32 vclk_frequency_table[4];
+	__u32 dclk_frequency_table[4];
+	__u32 lclk_frequency_table[4];
+	__u32 max_lclk_dpm_range;
+	__u32 min_lclk_dpm_range;
+
+	//XGMI
+	__u32 xgmi_width;
+	__u32 xgmi_bitrate;
+	__u64 xgmi_read_bandwidth_acc[8];
+	__u64 xgmi_write_bandwidth_acc[8];
+
+	//ACTIVITY
+	__u32 socket_c0_residency;
+	__u32 socket_gfx_busy;
+	__u32 dram_bandwidth_utilization;
+	__u64 socket_c0_residency_acc;
+	__u64 socket_gfx_busy_acc;
+	__u64 dram_bandwidth_acc;
+	__u32 max_dram_bandwidth;
+	__u64 dram_bandwidth_utilization_acc;
+	__u64 pcie_bandwidth_acc[4];
+
+	//THROTTLERS
+	__u32 prochot_residency_acc;
+	__u32 ppt_residency_acc;
+	__u32 socket_thm_residency_acc;
+	__u32 vr_thm_residency_acc;
+	__u32 hbm_thm_residency_acc;
+	__u32 spare;
+
+	// New Items at end to maintain driver compatibility
+	__u32 gfxclk_frequency[8];
 };
 
 /* Reset to default packing */
diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 99727cd705cf..fc6fba18844e 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -20,7 +20,7 @@
 #include <linux/semaphore.h>
 
 #define DRIVER_NAME		"amd_hsmp"
-#define DRIVER_VERSION		"1.0"
+#define DRIVER_VERSION		"2.0"
 
 /* HSMP Status / Error codes */
 #define HSMP_STATUS_NOT_READY	0x00
@@ -51,6 +51,8 @@
 #define HSMP_DEVNODE_NAME	"hsmp"
 
 struct hsmp_socket {
+	struct bin_attribute hsmp_attr;
+	void __iomem *metric_tbl_addr;
 	struct semaphore hsmp_sem;
 	u16 sock_ind;
 };
@@ -59,6 +61,7 @@ struct hsmp_plat_device {
 	struct miscdevice hsmp_device;
 	struct hsmp_socket *sock;
 	struct device *dev;
+	u32 proto_ver;
 	u16 num_sockets;
 };
 
@@ -330,9 +333,160 @@ static const struct file_operations hsmp_fops = {
 	.compat_ioctl	= hsmp_ioctl,
 };
 
+static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
+				    struct bin_attribute *bin_attr, char *buf,
+				    loff_t off, size_t count)
+{
+	struct hsmp_socket *sock = bin_attr->private;
+	struct hsmp_message msg = { 0 };
+	int ret;
+
+	if (!count || count > sizeof(struct hsmp_metric_table))
+		return 0;
+
+	msg.msg_id	= HSMP_GET_METRIC_TABLE;
+	msg.sock_ind	= sock->sock_ind;
+
+	ret = hsmp_send_message(&msg);
+	if (ret)
+		return ret;
+	memcpy(buf, sock->metric_tbl_addr, count);
+
+	return count;
+}
+
+static int hsmp_get_tbl_dram_base(u16 sock_ind)
+{
+	struct hsmp_socket *sock = &plat_dev.sock[sock_ind];
+	struct hsmp_message msg = { 0 };
+	phys_addr_t dram_addr;
+	int ret;
+
+	msg.sock_ind	= sock_ind;
+	msg.response_sz	= hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
+	msg.msg_id	= HSMP_GET_METRIC_TABLE_DRAM_ADDR;
+
+	ret = hsmp_send_message(&msg);
+	if (ret)
+		return ret;
+
+	/*
+	 * calculate the metric table DRAM address from lower and upper 32 bits
+	 * sent from SMU and ioremap it to virtual address.
+	 */
+	dram_addr = msg.args[0] | ((u64)(msg.args[1]) << 32);
+	if (!dram_addr) {
+		dev_err(plat_dev.dev, "Invalid dram address for metric table\n");
+		return -ENOMEM;
+	}
+	sock->metric_tbl_addr = devm_ioremap(plat_dev.dev, dram_addr,
+					     sizeof(struct hsmp_metric_table));
+	if (!sock->metric_tbl_addr) {
+		dev_err(plat_dev.dev, "Failed to ioremap metric table addr\n");
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static umode_t hsmp_metric_table_visible(struct kobject *kobj, struct bin_attribute *battr, int id)
+{
+	struct hsmp_socket *sock = battr->private;
+	int ret;
+
+	if (plat_dev.proto_ver == HSMP_PROTO_VER6) {
+		ret = hsmp_get_tbl_dram_base(sock->sock_ind);
+		if (ret)
+			return 0;
+		return battr->attr.mode;
+	} else {
+		return 0;
+	}
+}
+
+#define HSMP_ATTR_NAME_SIZE	25
+#define HSMP_GRP_NAME_SIZE	15
+#define NUM_ATTRS		1
+static int hsmp_init_metric_tbl_bin_attr(struct bin_attribute **hattrs, int sock_ind)
+{
+	struct bin_attribute *hattr = &plat_dev.sock[sock_ind].hsmp_attr;
+	char *name;
+
+	sysfs_attr_init(&plat_dev.sock[sock_ind].hsmp_attr);
+	name = devm_kzalloc(plat_dev.dev, HSMP_ATTR_NAME_SIZE, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+	snprintf(name, HSMP_ATTR_NAME_SIZE, "metrics_bin");
+	hattr->attr.name	= name;
+	hattr->attr.mode	= 0444;
+	hattr->read		= hsmp_metric_tbl_read;
+	hattr->size		= sizeof(struct hsmp_metric_table);
+	hattr->private		= &plat_dev.sock[sock_ind];
+	hattrs[0]		= hattr;
+
+	return 0;
+}
+
+static int hsmp_create_sysfs_file(void)
+{
+	const struct attribute_group **hsmp_attr_grps;
+	struct bin_attribute **hsmp_bin_attrs;
+	struct attribute_group *attr_grp;
+	int ret, i;
+
+	hsmp_attr_grps = devm_kzalloc(plat_dev.dev, sizeof(struct attribute_group *) *
+				      (plat_dev.num_sockets + 1), GFP_KERNEL);
+	if (!hsmp_attr_grps)
+		return -ENOMEM;
+
+	for (i = 0; i < plat_dev.num_sockets; i++) {
+		attr_grp = devm_kzalloc(plat_dev.dev, sizeof(struct attribute_group), GFP_KERNEL);
+		if (!attr_grp)
+			return -ENOMEM;
+
+		attr_grp->name = devm_kzalloc(plat_dev.dev, HSMP_GRP_NAME_SIZE, GFP_KERNEL);
+		if (!attr_grp->name)
+			return -ENOMEM;
+		snprintf((char *)attr_grp->name, HSMP_GRP_NAME_SIZE, "socket%d", i);
+
+		hsmp_bin_attrs = devm_kzalloc(plat_dev.dev, sizeof(struct bin_attribute *) *
+					      (NUM_ATTRS + 1), GFP_KERNEL);
+		if (!hsmp_bin_attrs)
+			return -ENOMEM;
+
+		attr_grp->bin_attrs		= hsmp_bin_attrs;
+		attr_grp->is_bin_visible	= hsmp_metric_table_visible;
+		hsmp_attr_grps[i]		= attr_grp;
+
+		ret = hsmp_init_metric_tbl_bin_attr(hsmp_bin_attrs, i);
+		if (ret)
+			return ret;
+	}
+	ret = devm_device_add_groups(plat_dev.dev, hsmp_attr_grps);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int hsmp_cache_proto_ver(void)
+{
+	struct hsmp_message msg = { 0 };
+	int ret;
+
+	msg.msg_id	= HSMP_GET_PROTO_VER;
+	msg.sock_ind	= 0;
+	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
+
+	ret = hsmp_send_message(&msg);
+	if (!ret)
+		plat_dev.proto_ver = msg.args[0];
+
+	return ret;
+}
+
 static int hsmp_pltdrv_probe(struct platform_device *pdev)
 {
-	int i;
+	int ret, i;
 
 	plat_dev.sock = devm_kzalloc(&pdev->dev,
 				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
@@ -353,7 +507,27 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
 	plat_dev.hsmp_device.mode	= 0644;
 
-	return misc_register(&plat_dev.hsmp_device);
+	ret = misc_register(&plat_dev.hsmp_device);
+	if (ret)
+		return ret;
+
+	ret = hsmp_cache_proto_ver();
+	if (ret) {
+		dev_err(plat_dev.dev, "Failed to read HSMP protocol version\n");
+		goto deregister;
+	}
+
+	ret = hsmp_create_sysfs_file();
+	if (ret) {
+		dev_err(plat_dev.dev, "Failed to create sysfs file\n");
+		goto deregister;
+	}
+
+	return 0;
+
+deregister:
+	misc_deregister(&plat_dev.hsmp_device);
+	return ret;
 }
 
 static void hsmp_pltdrv_remove(struct platform_device *pdev)
-- 
2.25.1

[PATCH v3 3/3] platform/x86/amd/hsmp: improve the error log

[Suma Hegde]

1. Change print message during platform init to a more meaningful
   clear message.
2. Return the error code returned by hsmp_test() itself, rather then
   returning a common EOPNOTSUPP error.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. changed commit message
Changes since v2:
No change

 drivers/platform/x86/amd/hsmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index fc6fba18844e..c4d324436a40 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -568,10 +568,10 @@ static int __init hsmp_plt_init(void)
 	for (i = 0; i < plat_dev.num_sockets; i++) {
 		ret = hsmp_test(i, 0xDEADBEEF);
 		if (ret) {
-			pr_err("HSMP is not supported on Fam:%x model:%x\n",
+			pr_err("HSMP test message failed 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;
+			pr_err("Is HSMP disabled in BIOS ?\n");
+			return ret;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH v3 1/3] platform/x86/amd/hsmp: create plat specific struct

[Ilpo Järvinen]

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

On Tue, 19 Sep 2023, Suma Hegde wrote:

> Having a separate platform device structure helps in future, to
> contain platform specific variables and other data.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> Changes since v1:
> 1. defined HSMP_CDEV_NAME and HSMP_DEVNODE_NAME macros
> Changes since v2:
> 1. moved num_sockets variable to plat_dev structure
> 
>  drivers/platform/x86/amd/hsmp.c | 61 ++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 31382ef52efb..99727cd705cf 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -47,9 +47,22 @@
>  #define HSMP_INDEX_REG		0xc4
>  #define HSMP_DATA_REG		0xc8
>  
> -static struct semaphore *hsmp_sem;
> +#define HSMP_CDEV_NAME		"hsmp_cdev"
> +#define HSMP_DEVNODE_NAME	"hsmp"
>  
> -static struct miscdevice hsmp_device;
> +struct hsmp_socket {
> +	struct semaphore hsmp_sem;
> +	u16 sock_ind;
> +};
> +
> +struct hsmp_plat_device {
> +	struct miscdevice hsmp_device;
> +	struct hsmp_socket *sock;
> +	struct device *dev;
> +	u16 num_sockets;
> +};
> +
> +static struct hsmp_plat_device plat_dev;
>  
>  static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
>  			 u32 *value, bool write)
> @@ -188,6 +201,7 @@ static int validate_message(struct hsmp_message *msg)
>  
>  int hsmp_send_message(struct hsmp_message *msg)
>  {
> +	struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
>  	struct amd_northbridge *nb;
>  	int ret;
>  
> @@ -208,14 +222,13 @@ int hsmp_send_message(struct hsmp_message *msg)
>  	 * 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));
> +	ret = down_timeout(&sock->hsmp_sem, msecs_to_jiffies(HSMP_MSG_TIMEOUT));
>  	if (ret < 0)
>  		return ret;
>  
>  	ret = __hsmp_send_message(nb->root, msg);
>  
> -	up(&hsmp_sem[msg->sock_ind]);
> +	up(&sock->hsmp_sem);
>  
>  	return ret;
>  }
> @@ -321,28 +334,31 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
>  	int i;
>  
> -	hsmp_sem = devm_kzalloc(&pdev->dev,
> -				(amd_nb_num() * sizeof(struct semaphore)),
> -				GFP_KERNEL);
> -	if (!hsmp_sem)
> +	plat_dev.sock = devm_kzalloc(&pdev->dev,
> +				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> +				     GFP_KERNEL);
> +	if (!plat_dev.sock)
>  		return -ENOMEM;
> +	plat_dev.dev = &pdev->dev;
>  
> -	for (i = 0; i < amd_nb_num(); i++)
> -		sema_init(&hsmp_sem[i], 1);
> +	for (i = 0; i < plat_dev.num_sockets; i++) {
> +		sema_init(&plat_dev.sock[i].hsmp_sem, 1);
> +		plat_dev.sock[i].sock_ind = i;
> +	}
>  
> -	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;
> +	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> +	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> +	plat_dev.hsmp_device.fops	= &hsmp_fops;
> +	plat_dev.hsmp_device.parent	= &pdev->dev;
> +	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
> +	plat_dev.hsmp_device.mode	= 0644;
>  
> -	return misc_register(&hsmp_device);
> +	return misc_register(&plat_dev.hsmp_device);
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
>  {
> -	misc_deregister(&hsmp_device);
> +	misc_deregister(&plat_dev.hsmp_device);
>  }
>  
>  static struct platform_driver amd_hsmp_driver = {
> @@ -358,7 +374,6 @@ 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) {
> @@ -371,12 +386,12 @@ static int __init hsmp_plt_init(void)
>  	 * 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)
> +	plat_dev.num_sockets = amd_nb_num();
> +	if (plat_dev.num_sockets == 0)
>  		return ret;
>  
>  	/* Test the hsmp interface on each socket */
> -	for (i = 0; i < num_sockets; i++) {
> +	for (i = 0; i < plat_dev.num_sockets; i++) {
>  		ret = hsmp_test(i, 0xDEADBEEF);
>  		if (ret) {
>  			pr_err("HSMP is not supported on Fam:%x model:%x\n",
> 

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

-- 
 i.

Re: [PATCH v3 3/3] platform/x86/amd/hsmp: improve the error log

[Ilpo Järvinen]

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

On Tue, 19 Sep 2023, Suma Hegde wrote:

> 1. Change print message during platform init to a more meaningful
>    clear message.
> 2. Return the error code returned by hsmp_test() itself, rather then
>    returning a common EOPNOTSUPP error.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> Changes since v1:
> 1. changed commit message
> Changes since v2:
> No change
> 
>  drivers/platform/x86/amd/hsmp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index fc6fba18844e..c4d324436a40 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -568,10 +568,10 @@ static int __init hsmp_plt_init(void)
>  	for (i = 0; i < plat_dev.num_sockets; i++) {
>  		ret = hsmp_test(i, 0xDEADBEEF);
>  		if (ret) {
> -			pr_err("HSMP is not supported on Fam:%x model:%x\n",
> +			pr_err("HSMP test message failed 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;
> +			pr_err("Is HSMP disabled in BIOS ?\n");
> +			return ret;
>  		}
>  	}
>  
> 

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

-- 
 i.

Re: [PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Ilpo Järvinen]

On Tue, 19 Sep 2023, Suma Hegde wrote:

> AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
> all the system management information from SMU.
> 
> The metrics table is made available as hexadecimal sysfs binary file
> under per socket sysfs directory created at
> /sys/devices/platform/amd_hsmp/socket%d/metrics_bin
> 
> Metrics table definitions will be documented as part of Public PPR.
> The same is defined in the amd_hsmp.h header.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> Changes since v1:
> 1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
> this patch
> 2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
>    HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
>    definition in amd_hsmp.h files
> 3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
> 4. Add hsmp_metric_table_visible() function 
> 5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
>    and code is also modified slightly
> 6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
> 7. Change from cleanup label to deregister label
> 8. Add dev_err print in hsmp_get_tbl_dram_base()
> 9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
> 10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
> 11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
> 12. Correct ATRR typo error
> 13. Change sprintf to snprintf
> 14. Check metrics table support only against HSMP_PROTO_VER6
> Changes since v2:
> 1. squash documentation patch into this patch
> 2. change from num_sockets to plat_dev.num_sockets
> 
>  Documentation/arch/x86/amd_hsmp.rst  |  16 +++
>  arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
>  drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
>  3 files changed, 302 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
> index 440e4b645a1c..a4c308784818 100644
> --- a/Documentation/arch/x86/amd_hsmp.rst
> +++ b/Documentation/arch/x86/amd_hsmp.rst
> @@ -41,6 +41,22 @@ In-kernel integration:
>   * Locking across callers is taken care by the driver.
>  
>  
> +HSMP sysfs interface
> +====================
> +
> +1. Metrics table binary sysfs
> +
> +AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
> +all the system management information from SMU.
> +
> +The metrics table is made available as hexadecimal sysfs binary file
> +under per socket sysfs directory created at
> +/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
> +
> +Metrics table definitions will be documented as part of Public PPR.
> +The same is defined in the amd_hsmp.h header.
> +
> +
>  An example
>  ==========
>  

I'd have expected to have the sysfs documentation appear under 
Documentation/ABI/testing/sysfs-...


The code change itself seems pretty okay now.

-- 
 i.

> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
> index 769b939444ae..f3479b768fb9 100644
> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
> @@ -47,6 +47,9 @@ enum hsmp_message_ids {
>  	HSMP_SET_PCI_RATE,		/* 20h Control link rate on PCIe devices */
>  	HSMP_SET_POWER_MODE,		/* 21h Select power efficiency profile policy */
>  	HSMP_SET_PSTATE_MAX_MIN,	/* 22h Set the max and min DF P-State  */
> +	HSMP_GET_METRIC_TABLE_VER,	/* 23h Get metrics table version */
> +	HSMP_GET_METRIC_TABLE,		/* 24h Get metrics table */
> +	HSMP_GET_METRIC_TABLE_DRAM_ADDR,/* 25h Get metrics table dram address */
>  	HSMP_MSG_ID_MAX,
>  };
>  
> @@ -64,6 +67,14 @@ enum hsmp_msg_type {
>  	HSMP_GET  = 1,
>  };
>  
> +enum hsmp_proto_versions {
> +	HSMP_PROTO_VER2	= 2,
> +	HSMP_PROTO_VER3,
> +	HSMP_PROTO_VER4,
> +	HSMP_PROTO_VER5,
> +	HSMP_PROTO_VER6
> +};
> +
>  struct hsmp_msg_desc {
>  	int num_args;
>  	int response_sz;
> @@ -295,6 +306,104 @@ static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>  	 * input: args[0] = min df pstate[15:8] + max df pstate[7:0]
>  	 */
>  	{1, 0, HSMP_SET},
> +
> +	/*
> +	 * HSMP_GET_METRIC_TABLE_VER, num_args = 0, response_sz = 1
> +	 * output: args[0] = metrics table version
> +	 */
> +	{0, 1, HSMP_GET},
> +
> +	/*
> +	 * HSMP_GET_METRIC_TABLE, num_args = 0, response_sz = 0
> +	 */
> +	{0, 0, HSMP_GET},
> +
> +	/*
> +	 * HSMP_GET_METRIC_TABLE_DRAM_ADDR, num_args = 0, response_sz = 2
> +	 * output: args[0] = lower 32 bits of the address
> +	 * output: args[1] = upper 32 bits of the address
> +	 */
> +	{0, 2, HSMP_GET},
> +};
> +
> +/* Metrics table for EPYC socket(supported only from proto version 6) */
> +struct hsmp_metric_table {
> +	__u32 accumulation_counter;
> +
> +	//TEMPERATURE
> +	__u32 max_socket_temperature;
> +	__u32 max_vr_temperature;
> +	__u32 max_hbm_temperature;
> +	__u64 max_socket_temperature_acc;
> +	__u64 max_vr_temperature_acc;
> +	__u64 max_hbm_temperature_acc;
> +
> +	//POWER
> +	__u32 socket_power_limit;
> +	__u32 max_socket_power_limit;
> +	__u32 socket_power;
> +
> +	//ENERGY
> +	__u64 timestamp;
> +	__u64 socket_energy_acc;
> +	__u64 ccd_energy_acc;
> +	__u64 xcd_energy_acc;
> +	__u64 aid_energy_acc;
> +	__u64 hbm_energy_acc;
> +
> +	//FREQUENCY
> +	__u32 cclk_frequency_limit;
> +	__u32 gfxclk_frequency_limit;
> +	__u32 fclk_frequency;
> +	__u32 uclk_frequency;
> +	__u32 socclk_frequency[4];
> +	__u32 vclk_frequency[4];
> +	__u32 dclk_frequency[4];
> +	__u32 lclk_frequency[4];
> +	__u64 gfxclk_frequency_acc[8];
> +	__u64 cclk_frequency_acc[96];
> +
> +	//FREQUENCY RANGE
> +	__u32 max_cclk_frequency;
> +	__u32 min_cclk_frequency;
> +	__u32 max_gfxclk_frequency;
> +	__u32 min_gfxclk_frequency;
> +	__u32 fclk_frequency_table[4];
> +	__u32 uclk_frequency_table[4];
> +	__u32 socclk_frequency_table[4];
> +	__u32 vclk_frequency_table[4];
> +	__u32 dclk_frequency_table[4];
> +	__u32 lclk_frequency_table[4];
> +	__u32 max_lclk_dpm_range;
> +	__u32 min_lclk_dpm_range;
> +
> +	//XGMI
> +	__u32 xgmi_width;
> +	__u32 xgmi_bitrate;
> +	__u64 xgmi_read_bandwidth_acc[8];
> +	__u64 xgmi_write_bandwidth_acc[8];
> +
> +	//ACTIVITY
> +	__u32 socket_c0_residency;
> +	__u32 socket_gfx_busy;
> +	__u32 dram_bandwidth_utilization;
> +	__u64 socket_c0_residency_acc;
> +	__u64 socket_gfx_busy_acc;
> +	__u64 dram_bandwidth_acc;
> +	__u32 max_dram_bandwidth;
> +	__u64 dram_bandwidth_utilization_acc;
> +	__u64 pcie_bandwidth_acc[4];
> +
> +	//THROTTLERS
> +	__u32 prochot_residency_acc;
> +	__u32 ppt_residency_acc;
> +	__u32 socket_thm_residency_acc;
> +	__u32 vr_thm_residency_acc;
> +	__u32 hbm_thm_residency_acc;
> +	__u32 spare;
> +
> +	// New Items at end to maintain driver compatibility
> +	__u32 gfxclk_frequency[8];
>  };
>  
>  /* Reset to default packing */
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 99727cd705cf..fc6fba18844e 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -20,7 +20,7 @@
>  #include <linux/semaphore.h>
>  
>  #define DRIVER_NAME		"amd_hsmp"
> -#define DRIVER_VERSION		"1.0"
> +#define DRIVER_VERSION		"2.0"
>  
>  /* HSMP Status / Error codes */
>  #define HSMP_STATUS_NOT_READY	0x00
> @@ -51,6 +51,8 @@
>  #define HSMP_DEVNODE_NAME	"hsmp"
>  
>  struct hsmp_socket {
> +	struct bin_attribute hsmp_attr;
> +	void __iomem *metric_tbl_addr;
>  	struct semaphore hsmp_sem;
>  	u16 sock_ind;
>  };
> @@ -59,6 +61,7 @@ struct hsmp_plat_device {
>  	struct miscdevice hsmp_device;
>  	struct hsmp_socket *sock;
>  	struct device *dev;
> +	u32 proto_ver;
>  	u16 num_sockets;
>  };
>  
> @@ -330,9 +333,160 @@ static const struct file_operations hsmp_fops = {
>  	.compat_ioctl	= hsmp_ioctl,
>  };
>  
> +static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *bin_attr, char *buf,
> +				    loff_t off, size_t count)
> +{
> +	struct hsmp_socket *sock = bin_attr->private;
> +	struct hsmp_message msg = { 0 };
> +	int ret;
> +
> +	if (!count || count > sizeof(struct hsmp_metric_table))
> +		return 0;
> +
> +	msg.msg_id	= HSMP_GET_METRIC_TABLE;
> +	msg.sock_ind	= sock->sock_ind;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (ret)
> +		return ret;
> +	memcpy(buf, sock->metric_tbl_addr, count);
> +
> +	return count;
> +}
> +
> +static int hsmp_get_tbl_dram_base(u16 sock_ind)
> +{
> +	struct hsmp_socket *sock = &plat_dev.sock[sock_ind];
> +	struct hsmp_message msg = { 0 };
> +	phys_addr_t dram_addr;
> +	int ret;
> +
> +	msg.sock_ind	= sock_ind;
> +	msg.response_sz	= hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
> +	msg.msg_id	= HSMP_GET_METRIC_TABLE_DRAM_ADDR;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * calculate the metric table DRAM address from lower and upper 32 bits
> +	 * sent from SMU and ioremap it to virtual address.
> +	 */
> +	dram_addr = msg.args[0] | ((u64)(msg.args[1]) << 32);
> +	if (!dram_addr) {
> +		dev_err(plat_dev.dev, "Invalid dram address for metric table\n");
> +		return -ENOMEM;
> +	}
> +	sock->metric_tbl_addr = devm_ioremap(plat_dev.dev, dram_addr,
> +					     sizeof(struct hsmp_metric_table));
> +	if (!sock->metric_tbl_addr) {
> +		dev_err(plat_dev.dev, "Failed to ioremap metric table addr\n");
> +		return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +static umode_t hsmp_metric_table_visible(struct kobject *kobj, struct bin_attribute *battr, int id)
> +{
> +	struct hsmp_socket *sock = battr->private;
> +	int ret;
> +
> +	if (plat_dev.proto_ver == HSMP_PROTO_VER6) {
> +		ret = hsmp_get_tbl_dram_base(sock->sock_ind);
> +		if (ret)
> +			return 0;
> +		return battr->attr.mode;
> +	} else {
> +		return 0;
> +	}
> +}
> +
> +#define HSMP_ATTR_NAME_SIZE	25
> +#define HSMP_GRP_NAME_SIZE	15
> +#define NUM_ATTRS		1
> +static int hsmp_init_metric_tbl_bin_attr(struct bin_attribute **hattrs, int sock_ind)
> +{
> +	struct bin_attribute *hattr = &plat_dev.sock[sock_ind].hsmp_attr;
> +	char *name;
> +
> +	sysfs_attr_init(&plat_dev.sock[sock_ind].hsmp_attr);
> +	name = devm_kzalloc(plat_dev.dev, HSMP_ATTR_NAME_SIZE, GFP_KERNEL);
> +	if (!name)
> +		return -ENOMEM;
> +	snprintf(name, HSMP_ATTR_NAME_SIZE, "metrics_bin");
> +	hattr->attr.name	= name;
> +	hattr->attr.mode	= 0444;
> +	hattr->read		= hsmp_metric_tbl_read;
> +	hattr->size		= sizeof(struct hsmp_metric_table);
> +	hattr->private		= &plat_dev.sock[sock_ind];
> +	hattrs[0]		= hattr;
> +
> +	return 0;
> +}
> +
> +static int hsmp_create_sysfs_file(void)
> +{
> +	const struct attribute_group **hsmp_attr_grps;
> +	struct bin_attribute **hsmp_bin_attrs;
> +	struct attribute_group *attr_grp;
> +	int ret, i;
> +
> +	hsmp_attr_grps = devm_kzalloc(plat_dev.dev, sizeof(struct attribute_group *) *
> +				      (plat_dev.num_sockets + 1), GFP_KERNEL);
> +	if (!hsmp_attr_grps)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < plat_dev.num_sockets; i++) {
> +		attr_grp = devm_kzalloc(plat_dev.dev, sizeof(struct attribute_group), GFP_KERNEL);
> +		if (!attr_grp)
> +			return -ENOMEM;
> +
> +		attr_grp->name = devm_kzalloc(plat_dev.dev, HSMP_GRP_NAME_SIZE, GFP_KERNEL);
> +		if (!attr_grp->name)
> +			return -ENOMEM;
> +		snprintf((char *)attr_grp->name, HSMP_GRP_NAME_SIZE, "socket%d", i);
> +
> +		hsmp_bin_attrs = devm_kzalloc(plat_dev.dev, sizeof(struct bin_attribute *) *
> +					      (NUM_ATTRS + 1), GFP_KERNEL);
> +		if (!hsmp_bin_attrs)
> +			return -ENOMEM;
> +
> +		attr_grp->bin_attrs		= hsmp_bin_attrs;
> +		attr_grp->is_bin_visible	= hsmp_metric_table_visible;
> +		hsmp_attr_grps[i]		= attr_grp;
> +
> +		ret = hsmp_init_metric_tbl_bin_attr(hsmp_bin_attrs, i);
> +		if (ret)
> +			return ret;
> +	}
> +	ret = devm_device_add_groups(plat_dev.dev, hsmp_attr_grps);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int hsmp_cache_proto_ver(void)
> +{
> +	struct hsmp_message msg = { 0 };
> +	int ret;
> +
> +	msg.msg_id	= HSMP_GET_PROTO_VER;
> +	msg.sock_ind	= 0;
> +	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (!ret)
> +		plat_dev.proto_ver = msg.args[0];
> +
> +	return ret;
> +}
> +
>  static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
> -	int i;
> +	int ret, i;
>  
>  	plat_dev.sock = devm_kzalloc(&pdev->dev,
>  				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> @@ -353,7 +507,27 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
>  	plat_dev.hsmp_device.mode	= 0644;
>  
> -	return misc_register(&plat_dev.hsmp_device);
> +	ret = misc_register(&plat_dev.hsmp_device);
> +	if (ret)
> +		return ret;
> +
> +	ret = hsmp_cache_proto_ver();
> +	if (ret) {
> +		dev_err(plat_dev.dev, "Failed to read HSMP protocol version\n");
> +		goto deregister;
> +	}
> +
> +	ret = hsmp_create_sysfs_file();
> +	if (ret) {
> +		dev_err(plat_dev.dev, "Failed to create sysfs file\n");
> +		goto deregister;
> +	}
> +
> +	return 0;
> +
> +deregister:
> +	misc_deregister(&plat_dev.hsmp_device);
> +	return ret;
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
> 

Re: [PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Hans de Goede]

Hi Ilpo, Suma,

On 9/19/23 15:07, Ilpo Järvinen wrote:
> On Tue, 19 Sep 2023, Suma Hegde wrote:
> 
>> AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
>> all the system management information from SMU.
>>
>> The metrics table is made available as hexadecimal sysfs binary file
>> under per socket sysfs directory created at
>> /sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>>
>> Metrics table definitions will be documented as part of Public PPR.
>> The same is defined in the amd_hsmp.h header.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> ---
>> Changes since v1:
>> 1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
>> this patch
>> 2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
>>    HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
>>    definition in amd_hsmp.h files
>> 3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
>> 4. Add hsmp_metric_table_visible() function 
>> 5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
>>    and code is also modified slightly
>> 6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
>> 7. Change from cleanup label to deregister label
>> 8. Add dev_err print in hsmp_get_tbl_dram_base()
>> 9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
>> 10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
>> 11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
>> 12. Correct ATRR typo error
>> 13. Change sprintf to snprintf
>> 14. Check metrics table support only against HSMP_PROTO_VER6
>> Changes since v2:
>> 1. squash documentation patch into this patch
>> 2. change from num_sockets to plat_dev.num_sockets
>>
>>  Documentation/arch/x86/amd_hsmp.rst  |  16 +++
>>  arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
>>  drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
>>  3 files changed, 302 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
>> index 440e4b645a1c..a4c308784818 100644
>> --- a/Documentation/arch/x86/amd_hsmp.rst
>> +++ b/Documentation/arch/x86/amd_hsmp.rst
>> @@ -41,6 +41,22 @@ In-kernel integration:
>>   * Locking across callers is taken care by the driver.
>>  
>>  
>> +HSMP sysfs interface
>> +====================
>> +
>> +1. Metrics table binary sysfs
>> +
>> +AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
>> +all the system management information from SMU.
>> +
>> +The metrics table is made available as hexadecimal sysfs binary file
>> +under per socket sysfs directory created at
>> +/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>> +
>> +Metrics table definitions will be documented as part of Public PPR.
>> +The same is defined in the amd_hsmp.h header.
>> +
>> +
>>  An example
>>  ==========
>>  
> 
> I'd have expected to have the sysfs documentation appear under 
> Documentation/ABI/testing/sysfs-...

Actually it is somewhat normal for sysfs files to be paired
together with other documentation when there is more extensive
documentation then just the sysfs files, see e.g. :

Documentation/admin-guide/laptops/thinkpad-acpi.rst

So there is precedent for this and I think it make sense
to keep all the documentation in one place, rather then to add
a Documentation/ABI/testing/sysfs-platform-amd-hsmp file
just for the sysfs attributes .

OTOH people are used to look for sysfs attribute documentation
in a place like Documentation/ABI/testing/sysfs-platform-amd-hsmp,
but I think that keeping all the docs together is more important.

So I have a slight preference for keeping this as is and
just merging v3 of this series as is.

Ilpo, what do you think ?


Regards,

Hans

Re: [PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Ilpo Järvinen]

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

On Thu, 21 Sep 2023, Hans de Goede wrote:
> On 9/19/23 15:07, Ilpo Järvinen wrote:
> > On Tue, 19 Sep 2023, Suma Hegde wrote:
> > 
> >> AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
> >> all the system management information from SMU.
> >>
> >> The metrics table is made available as hexadecimal sysfs binary file
> >> under per socket sysfs directory created at
> >> /sys/devices/platform/amd_hsmp/socket%d/metrics_bin
> >>
> >> Metrics table definitions will be documented as part of Public PPR.
> >> The same is defined in the amd_hsmp.h header.
> >>
> >> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> >> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> >> ---
> >> Changes since v1:
> >> 1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
> >> this patch
> >> 2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
> >>    HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
> >>    definition in amd_hsmp.h files
> >> 3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
> >> 4. Add hsmp_metric_table_visible() function 
> >> 5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
> >>    and code is also modified slightly
> >> 6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
> >> 7. Change from cleanup label to deregister label
> >> 8. Add dev_err print in hsmp_get_tbl_dram_base()
> >> 9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
> >> 10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
> >> 11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
> >> 12. Correct ATRR typo error
> >> 13. Change sprintf to snprintf
> >> 14. Check metrics table support only against HSMP_PROTO_VER6
> >> Changes since v2:
> >> 1. squash documentation patch into this patch
> >> 2. change from num_sockets to plat_dev.num_sockets
> >>
> >>  Documentation/arch/x86/amd_hsmp.rst  |  16 +++
> >>  arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
> >>  drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
> >>  3 files changed, 302 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
> >> index 440e4b645a1c..a4c308784818 100644
> >> --- a/Documentation/arch/x86/amd_hsmp.rst
> >> +++ b/Documentation/arch/x86/amd_hsmp.rst
> >> @@ -41,6 +41,22 @@ In-kernel integration:
> >>   * Locking across callers is taken care by the driver.
> >>  
> >>  
> >> +HSMP sysfs interface
> >> +====================
> >> +
> >> +1. Metrics table binary sysfs
> >> +
> >> +AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
> >> +all the system management information from SMU.
> >> +
> >> +The metrics table is made available as hexadecimal sysfs binary file
> >> +under per socket sysfs directory created at
> >> +/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
> >> +
> >> +Metrics table definitions will be documented as part of Public PPR.
> >> +The same is defined in the amd_hsmp.h header.
> >> +
> >> +
> >>  An example
> >>  ==========
> >>  
> > 
> > I'd have expected to have the sysfs documentation appear under 
> > Documentation/ABI/testing/sysfs-...
> 
> Actually it is somewhat normal for sysfs files to be paired
> together with other documentation when there is more extensive
> documentation then just the sysfs files, see e.g. :
> 
> Documentation/admin-guide/laptops/thinkpad-acpi.rst

Okay but that seems to result in attempting to handle deprecation 
within that file too which feels wrong beyond just hacing documentation in 
an unusual location.

> So there is precedent for this and I think it make sense
> to keep all the documentation in one place, rather then to add
> a Documentation/ABI/testing/sysfs-platform-amd-hsmp file
> just for the sysfs attributes .
> 
> OTOH people are used to look for sysfs attribute documentation
> in a place like Documentation/ABI/testing/sysfs-platform-amd-hsmp,
> but I think that keeping all the docs together is more important.
> 
> So I have a slight preference for keeping this as is and
> just merging v3 of this series as is.
> 
> Ilpo, what do you think ?

I don't have strong preference myself if you think the current location 
is fine. So feel free to merge it as is.

-- 
 i.

Re: [PATCH v3 2/3] platform/x86/amd/hsmp: add support for metrics tbl

[Hans de Goede]

HI,

On 9/22/23 10:56, Ilpo Järvinen wrote:
> On Thu, 21 Sep 2023, Hans de Goede wrote:
>> On 9/19/23 15:07, Ilpo Järvinen wrote:
>>> On Tue, 19 Sep 2023, Suma Hegde wrote:
>>>
>>>> AMD MI300 MCM provides GET_METRICS_TABLE message to retrieve
>>>> all the system management information from SMU.
>>>>
>>>> The metrics table is made available as hexadecimal sysfs binary file
>>>> under per socket sysfs directory created at
>>>> /sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>>>>
>>>> Metrics table definitions will be documented as part of Public PPR.
>>>> The same is defined in the amd_hsmp.h header.
>>>>
>>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>>> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>>> ---
>>>> Changes since v1:
>>>> 1. Remove HSMP_DEVNODE_NAME and HSMP_CDEV_NAME macro definitions in
>>>> this patch
>>>> 2. Remove extra space in comments for HSMP_GET_METRIC_TABLE_VER,
>>>>    HSMP_GET_METRIC_TABLE and HSMP_GET_METRIC_TABLE_DRAM_ADDR enum
>>>>    definition in amd_hsmp.h files
>>>> 3. Change check, count == 0 to !count in hsmp_metric_tbl_read() function
>>>> 4. Add hsmp_metric_table_visible() function 
>>>> 5. hsmp_create_metric_tbl_sysfs_file() is renamed as hsmp_init_metric_tbl_bin_attr()
>>>>    and code is also modified slightly
>>>> 6. Modify hsmp_create_sysfs_file() to use devm_device_add_groups()
>>>> 7. Change from cleanup label to deregister label
>>>> 8. Add dev_err print in hsmp_get_tbl_dram_base()
>>>> 9. Reword "Unable to Failed" in hsmp_get_tbl_dram_base()
>>>> 10. Add HSMP_GRP_NAME_SIZE and NUM_ATTRS macros
>>>> 11. Remove sysfs cleanup code in hsmp_pltdrv_remove()
>>>> 12. Correct ATRR typo error
>>>> 13. Change sprintf to snprintf
>>>> 14. Check metrics table support only against HSMP_PROTO_VER6
>>>> Changes since v2:
>>>> 1. squash documentation patch into this patch
>>>> 2. change from num_sockets to plat_dev.num_sockets
>>>>
>>>>  Documentation/arch/x86/amd_hsmp.rst  |  16 +++
>>>>  arch/x86/include/uapi/asm/amd_hsmp.h | 109 ++++++++++++++++
>>>>  drivers/platform/x86/amd/hsmp.c      | 180 ++++++++++++++++++++++++++-
>>>>  3 files changed, 302 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/arch/x86/amd_hsmp.rst b/Documentation/arch/x86/amd_hsmp.rst
>>>> index 440e4b645a1c..a4c308784818 100644
>>>> --- a/Documentation/arch/x86/amd_hsmp.rst
>>>> +++ b/Documentation/arch/x86/amd_hsmp.rst
>>>> @@ -41,6 +41,22 @@ In-kernel integration:
>>>>   * Locking across callers is taken care by the driver.
>>>>  
>>>>  
>>>> +HSMP sysfs interface
>>>> +====================
>>>> +
>>>> +1. Metrics table binary sysfs
>>>> +
>>>> +AMD MI300A MCM provides GET_METRICS_TABLE message to retrieve
>>>> +all the system management information from SMU.
>>>> +
>>>> +The metrics table is made available as hexadecimal sysfs binary file
>>>> +under per socket sysfs directory created at
>>>> +/sys/devices/platform/amd_hsmp/socket%d/metrics_bin
>>>> +
>>>> +Metrics table definitions will be documented as part of Public PPR.
>>>> +The same is defined in the amd_hsmp.h header.
>>>> +
>>>> +
>>>>  An example
>>>>  ==========
>>>>  
>>>
>>> I'd have expected to have the sysfs documentation appear under 
>>> Documentation/ABI/testing/sysfs-...
>>
>> Actually it is somewhat normal for sysfs files to be paired
>> together with other documentation when there is more extensive
>> documentation then just the sysfs files, see e.g. :
>>
>> Documentation/admin-guide/laptops/thinkpad-acpi.rst
> 
> Okay but that seems to result in attempting to handle deprecation 
> within that file too which feels wrong beyond just hacing documentation in 
> an unusual location.
> 
>> So there is precedent for this and I think it make sense
>> to keep all the documentation in one place, rather then to add
>> a Documentation/ABI/testing/sysfs-platform-amd-hsmp file
>> just for the sysfs attributes .
>>
>> OTOH people are used to look for sysfs attribute documentation
>> in a place like Documentation/ABI/testing/sysfs-platform-amd-hsmp,
>> but I think that keeping all the docs together is more important.
>>
>> So I have a slight preference for keeping this as is and
>> just merging v3 of this series as is.
>>
>> Ilpo, what do you think ?
> 
> I don't have strong preference myself if you think the current location 
> is fine. So feel free to merge it as is.

I'm traveling this week.

Ilpo, if you're fine with this series as is, can you merge it please ?

Regards,

Hans

Re: [PATCH v3 1/3] platform/x86/amd/hsmp: create plat specific struct

[Ilpo Järvinen]

On Tue, 19 Sep 2023 09:20:55 +0000, Suma Hegde wrote:

> Having a separate platform device structure helps in future, to
> contain platform specific variables and other data.
> 
> 


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

Once I've run some tests on the review-ilpo branch the patches
there will be added to the platform-drivers-x86/for-next branch
and eventually will be included in the pdx86 pull-request to
Linus for the next merge-window.

The list of commits applied:
[1/3] platform/x86/amd/hsmp: create plat specific struct
      commit: 6448b90731aabbfb0e351b11be37af94c157dda4
[2/3] platform/x86/amd/hsmp: add support for metrics tbl
      commit: 0f21042c11c7fa9053d5af1797f5a9380579f4dd
[3/3] platform/x86/amd/hsmp: improve the error log
      commit: bd2b5ff0df3da9b75e684d99cd5b1ed4e74e24db

--
 i.

Re: [PATCH v3 1/3] platform/x86/amd/hsmp: create plat specific struct

[Ilpo Järvinen]

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

On Tue, 26 Sep 2023, Ilpo Järvinen wrote:

> On Tue, 19 Sep 2023 09:20:55 +0000, Suma Hegde wrote:
> 
> > Having a separate platform device structure helps in future, to
> > contain platform specific variables and other data.
> > 
> > 
> 
> 
> Thank you for your contribution, it has been applied to my local
> review-ilpo branch. Note it will show up in the public
> platform-drivers-x86/review-ilpo branch only once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on the review-ilpo branch the patches
> there will be added to the platform-drivers-x86/for-next branch
> and eventually will be included in the pdx86 pull-request to
> Linus for the next merge-window.
> 
> The list of commits applied:
> [1/3] platform/x86/amd/hsmp: create plat specific struct
>       commit: 6448b90731aabbfb0e351b11be37af94c157dda4
> [2/3] platform/x86/amd/hsmp: add support for metrics tbl
>       commit: 0f21042c11c7fa9053d5af1797f5a9380579f4dd

This ended up breaking the build and for some reason lkp had not caught it
(probably didn't build the series at all since the build fails already 
with allyesconfig).

I'll fix this in review-ilpo branch for you but please try to build test 
in the future to add a little bit of redundancy into build testing.

> [3/3] platform/x86/amd/hsmp: improve the error log
>       commit: bd2b5ff0df3da9b75e684d99cd5b1ed4e74e24db

-- 
 i.