[PATCH v2 0/8] platform/x86/amd/pmc: Updates to AMD PMC driver

[PATCH v2 0/8] platform/x86/amd/pmc: Updates to AMD PMC driver

[Shyam Sundar S K]

Updates include:
- Rework STB code and move into a separate file
- Update the code with new IP block information for newer SoCs
- Add STB support for new generation
- Add STB support for Ryzen desktop variants
- Updates to MAINTAINERS record.


v2:
----
 - Add Mario's Reviewed-by tags
 - Add amd_stb_update_args() to simplify code handling
 - use cpu_feature_enabled() instead of root port's cpu_id information.

Shyam Sundar S K (8):
  platform/x86/amd/pmc: Move STB functionality to a new file for better
    code organization
  platform/x86/amd/pmc: Update function names to align with new STB file
  platform/x86/amd/pmc: Define enum for S2D/PMC msg_port
  platform/x86/amd/pmc: Isolate STB code changes to a new file
  platform/x86/amd/pmc: Update IP information structure for newer SoCs
  platform/x86/amd/pmc: Update S2D message id for 1Ah Family 70h model
  platform/x86/amd/pmc: Add STB support for AMD Desktop variants
  MAINTAINERS: Change AMD PMF driver status to "Supported"

 MAINTAINERS                            |   2 +-
 drivers/platform/x86/amd/pmc/Makefile  |   2 +-
 drivers/platform/x86/amd/pmc/mp1_stb.c | 334 +++++++++++++++++++++++
 drivers/platform/x86/amd/pmc/pmc.c     | 349 ++++---------------------
 drivers/platform/x86/amd/pmc/pmc.h     |  13 +-
 5 files changed, 402 insertions(+), 298 deletions(-)
 create mode 100644 drivers/platform/x86/amd/pmc/mp1_stb.c

-- 
2.34.1

[PATCH v2 1/8] platform/x86/amd/pmc: Move STB functionality to a new file for better code organization

[Shyam Sundar S K]

As the SoC evolves with each generation, the dynamics between the PMC and
STB layers within the PMC driver are becoming increasingly complex, making
it challenging to manage both in a single file and maintain code
readability.

Additionally, during silicon bringup, the PMC functionality is often
enabled first, with STB functionality added later. This can lead to missed
updates in the driver, potentially causing issues.

To address these challenges, it's beneficial to move all STB-related
changes to a separate file. This approach will better accommodate newer
SoCs, provide improved flexibility for desktop variants, and facilitate
the collection of additional debug information through STB mechanisms.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/Makefile  |   2 +-
 drivers/platform/x86/amd/pmc/mp1_stb.c | 295 +++++++++++++++++++++++++
 drivers/platform/x86/amd/pmc/pmc.c     | 289 +-----------------------
 drivers/platform/x86/amd/pmc/pmc.h     |   9 +
 4 files changed, 310 insertions(+), 285 deletions(-)
 create mode 100644 drivers/platform/x86/amd/pmc/mp1_stb.c

diff --git a/drivers/platform/x86/amd/pmc/Makefile b/drivers/platform/x86/amd/pmc/Makefile
index f1d9ab19d24c..255d94ddf999 100644
--- a/drivers/platform/x86/amd/pmc/Makefile
+++ b/drivers/platform/x86/amd/pmc/Makefile
@@ -4,6 +4,6 @@
 # AMD Power Management Controller Driver
 #
 
-amd-pmc-objs := pmc.o pmc-quirks.o
+amd-pmc-objs := pmc.o pmc-quirks.o mp1_stb.o
 obj-$(CONFIG_AMD_PMC) += amd-pmc.o
 amd-pmc-$(CONFIG_AMD_MP2_STB) += mp2_stb.o
diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
new file mode 100644
index 000000000000..9a34dd94982c
--- /dev/null
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD MP1 Smart Trace Buffer (STB) Layer
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *          Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/amd_nb.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/uaccess.h>
+
+#include "pmc.h"
+
+/* STB Spill to DRAM Parameters */
+#define S2D_TELEMETRY_BYTES_MAX		0x100000U
+#define S2D_RSVD_RAM_SPACE		0x100000
+#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
+
+/* STB Registers */
+#define AMD_PMC_STB_PMI_0	0x03E30600
+#define AMD_PMC_STB_DUMMY_PC	0xC6000007
+
+/* STB Spill to DRAM Message Definition */
+#define STB_FORCE_FLUSH_DATA	0xCF
+#define FIFO_SIZE		4096
+
+static bool enable_stb;
+module_param(enable_stb, bool, 0644);
+MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
+
+static bool dump_custom_stb;
+module_param(dump_custom_stb, bool, 0644);
+MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
+
+enum s2d_arg {
+	S2D_TELEMETRY_SIZE = 0x01,
+	S2D_PHYS_ADDR_LOW,
+	S2D_PHYS_ADDR_HIGH,
+	S2D_NUM_SAMPLES,
+	S2D_DRAM_SIZE,
+};
+
+struct amd_pmc_stb_v2_data {
+	size_t size;
+	u8 data[] __counted_by(size);
+};
+
+int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
+{
+	int err;
+
+	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
+	if (err) {
+		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
+		return pcibios_err_to_errno(err);
+	}
+
+	return 0;
+}
+
+static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
+{
+	int i, err;
+
+	for (i = 0; i < FIFO_SIZE; i++) {
+		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
+		if (err) {
+			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
+				AMD_PMC_STB_PMI_0);
+			return pcibios_err_to_errno(err);
+		}
+	}
+
+	return 0;
+}
+
+static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
+{
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+	u32 size = FIFO_SIZE * sizeof(u32);
+	u32 *buf;
+	int rc;
+
+	buf = kzalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	rc = amd_pmc_read_stb(dev, buf);
+	if (rc) {
+		kfree(buf);
+		return rc;
+	}
+
+	filp->private_data = buf;
+	return rc;
+}
+
+static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
+					loff_t *pos)
+{
+	if (!filp->private_data)
+		return -EINVAL;
+
+	return simple_read_from_buffer(buf, size, pos, filp->private_data,
+				       FIFO_SIZE * sizeof(u32));
+}
+
+static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
+{
+	kfree(filp->private_data);
+	return 0;
+}
+
+static const struct file_operations amd_pmc_stb_debugfs_fops = {
+	.owner = THIS_MODULE,
+	.open = amd_pmc_stb_debugfs_open,
+	.read = amd_pmc_stb_debugfs_read,
+	.release = amd_pmc_stb_debugfs_release,
+};
+
+/* Enhanced STB Firmware Reporting Mechanism */
+static int amd_pmc_stb_handle_efr(struct file *filp)
+{
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+	struct amd_pmc_stb_v2_data *stb_data_arr;
+	u32 fsize;
+
+	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
+	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
+	if (!stb_data_arr)
+		return -ENOMEM;
+
+	stb_data_arr->size = fsize;
+	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
+	filp->private_data = stb_data_arr;
+
+	return 0;
+}
+
+static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
+{
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
+	struct amd_pmc_stb_v2_data *stb_data_arr;
+	int ret;
+
+	/* Write dummy postcode while reading the STB buffer */
+	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
+	if (ret)
+		dev_err(dev->dev, "error writing to STB: %d\n", ret);
+
+	/* Spill to DRAM num_samples uses separate SMU message port */
+	dev->msg_port = 1;
+
+	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
+	if (ret)
+		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
+
+	/*
+	 * We have a custom stb size and the PMFW is supposed to give
+	 * the enhanced dram size. Note that we land here only for the
+	 * platforms that support enhanced dram size reporting.
+	 */
+	if (dump_custom_stb)
+		return amd_pmc_stb_handle_efr(filp);
+
+	/* Get the num_samples to calculate the last push location */
+	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
+	/* Clear msg_port for other SMU operation */
+	dev->msg_port = 0;
+	if (ret) {
+		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
+		return ret;
+	}
+
+	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
+	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
+	if (!stb_data_arr)
+		return -ENOMEM;
+
+	stb_data_arr->size = fsize;
+
+	/*
+	 * Start capturing data from the last push location.
+	 * This is for general cases, where the stb limits
+	 * are meant for standard usage.
+	 */
+	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
+		/* First read oldest data starting 1 behind last write till end of ringbuffer */
+		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
+		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
+
+		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
+		/* Second copy the newer samples from offset 0 - last write */
+		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
+	} else {
+		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
+	}
+
+	filp->private_data = stb_data_arr;
+
+	return 0;
+}
+
+static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
+					   loff_t *pos)
+{
+	struct amd_pmc_stb_v2_data *data = filp->private_data;
+
+	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
+}
+
+static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
+{
+	kfree(filp->private_data);
+	return 0;
+}
+
+static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
+	.owner = THIS_MODULE,
+	.open = amd_pmc_stb_debugfs_open_v2,
+	.read = amd_pmc_stb_debugfs_read_v2,
+	.release = amd_pmc_stb_debugfs_release_v2,
+};
+
+static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
+{
+	switch (dev->cpu_id) {
+	case AMD_CPU_ID_YC:
+	case AMD_CPU_ID_CB:
+		dev->s2d_msg_id = 0xBE;
+		return true;
+	case AMD_CPU_ID_PS:
+		dev->s2d_msg_id = 0x85;
+		return true;
+	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
+	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
+		dev->s2d_msg_id = 0xDE;
+		return true;
+	default:
+		return false;
+	}
+}
+
+int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
+{
+	u32 phys_addr_low, phys_addr_hi;
+	u64 stb_phys_addr;
+	u32 size = 0;
+	int ret;
+
+	if (!enable_stb)
+		return 0;
+
+	if (amd_pmc_is_stb_supported(dev))
+		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
+				    &amd_pmc_stb_debugfs_fops_v2);
+	else
+		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
+				    &amd_pmc_stb_debugfs_fops);
+
+	/* Spill to DRAM feature uses separate SMU message port */
+	dev->msg_port = 1;
+
+	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
+	if (size != S2D_TELEMETRY_BYTES_MAX)
+		return -EIO;
+
+	/* Get DRAM size */
+	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
+	if (ret || !dev->dram_size)
+		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
+
+	/* Get STB DRAM address */
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
+
+	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
+
+	/* Clear msg_port for other SMU operation */
+	dev->msg_port = 0;
+
+	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
+	if (!dev->stb_virt_addr)
+		return -ENOMEM;
+
+	return 0;
+}
diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index bbb8edb62e00..a977ff1e52b5 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -44,20 +44,6 @@
 #define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
 #define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
 #define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
-#define AMD_PMC_STB_DUMMY_PC		0xC6000007
-
-/* STB S2D(Spill to DRAM) has different message port offset */
-#define AMD_S2D_REGISTER_MESSAGE	0xA20
-#define AMD_S2D_REGISTER_RESPONSE	0xA80
-#define AMD_S2D_REGISTER_ARGUMENT	0xA88
-
-/* STB Spill to DRAM Parameters */
-#define S2D_TELEMETRY_BYTES_MAX		0x100000U
-#define S2D_RSVD_RAM_SPACE		0x100000
-#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
-
-/* STB Spill to DRAM Message Definition */
-#define STB_FORCE_FLUSH_DATA		0xCF
 
 /* Base address of SMU for mapping physical address to virtual address */
 #define AMD_PMC_MAPPING_SIZE		0x01000
@@ -97,7 +83,6 @@
 
 #define DELAY_MIN_US		2000
 #define DELAY_MAX_US		3000
-#define FIFO_SIZE		4096
 
 enum amd_pmc_def {
 	MSG_TEST = 0x01,
@@ -105,19 +90,6 @@ enum amd_pmc_def {
 	MSG_OS_HINT_RN,
 };
 
-enum s2d_arg {
-	S2D_TELEMETRY_SIZE = 0x01,
-	S2D_PHYS_ADDR_LOW,
-	S2D_PHYS_ADDR_HIGH,
-	S2D_NUM_SAMPLES,
-	S2D_DRAM_SIZE,
-};
-
-struct amd_pmc_stb_v2_data {
-	size_t size;
-	u8 data[] __counted_by(size);
-};
-
 struct amd_pmc_bit_map {
 	const char *name;
 	u32 bit_mask;
@@ -149,22 +121,11 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
 	{}
 };
 
-static bool enable_stb;
-module_param(enable_stb, bool, 0644);
-MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
-
 static bool disable_workarounds;
 module_param(disable_workarounds, bool, 0644);
 MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
 
-static bool dump_custom_stb;
-module_param(dump_custom_stb, bool, 0644);
-MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
-
 static struct amd_pmc_dev pmc;
-static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
-static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
-static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -193,155 +154,6 @@ struct smu_metrics {
 	u64 timecondition_notmet_totaltime[32];
 } __packed;
 
-static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
-{
-	struct amd_pmc_dev *dev = filp->f_inode->i_private;
-	u32 size = FIFO_SIZE * sizeof(u32);
-	u32 *buf;
-	int rc;
-
-	buf = kzalloc(size, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	rc = amd_pmc_read_stb(dev, buf);
-	if (rc) {
-		kfree(buf);
-		return rc;
-	}
-
-	filp->private_data = buf;
-	return rc;
-}
-
-static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
-					loff_t *pos)
-{
-	if (!filp->private_data)
-		return -EINVAL;
-
-	return simple_read_from_buffer(buf, size, pos, filp->private_data,
-				       FIFO_SIZE * sizeof(u32));
-}
-
-static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
-{
-	kfree(filp->private_data);
-	return 0;
-}
-
-static const struct file_operations amd_pmc_stb_debugfs_fops = {
-	.owner = THIS_MODULE,
-	.open = amd_pmc_stb_debugfs_open,
-	.read = amd_pmc_stb_debugfs_read,
-	.release = amd_pmc_stb_debugfs_release,
-};
-
-/* Enhanced STB Firmware Reporting Mechanism */
-static int amd_pmc_stb_handle_efr(struct file *filp)
-{
-	struct amd_pmc_dev *dev = filp->f_inode->i_private;
-	struct amd_pmc_stb_v2_data *stb_data_arr;
-	u32 fsize;
-
-	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
-	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
-	if (!stb_data_arr)
-		return -ENOMEM;
-
-	stb_data_arr->size = fsize;
-	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
-	filp->private_data = stb_data_arr;
-
-	return 0;
-}
-
-static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
-{
-	struct amd_pmc_dev *dev = filp->f_inode->i_private;
-	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
-	struct amd_pmc_stb_v2_data *stb_data_arr;
-	int ret;
-
-	/* Write dummy postcode while reading the STB buffer */
-	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
-	if (ret)
-		dev_err(dev->dev, "error writing to STB: %d\n", ret);
-
-	/* Spill to DRAM num_samples uses separate SMU message port */
-	dev->msg_port = 1;
-
-	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
-	if (ret)
-		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
-
-	/*
-	 * We have a custom stb size and the PMFW is supposed to give
-	 * the enhanced dram size. Note that we land here only for the
-	 * platforms that support enhanced dram size reporting.
-	 */
-	if (dump_custom_stb)
-		return amd_pmc_stb_handle_efr(filp);
-
-	/* Get the num_samples to calculate the last push location */
-	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
-	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
-	if (ret) {
-		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
-		return ret;
-	}
-
-	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
-	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
-	if (!stb_data_arr)
-		return -ENOMEM;
-
-	stb_data_arr->size = fsize;
-
-	/*
-	 * Start capturing data from the last push location.
-	 * This is for general cases, where the stb limits
-	 * are meant for standard usage.
-	 */
-	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
-		/* First read oldest data starting 1 behind last write till end of ringbuffer */
-		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
-		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
-
-		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
-		/* Second copy the newer samples from offset 0 - last write */
-		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
-	} else {
-		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
-	}
-
-	filp->private_data = stb_data_arr;
-
-	return 0;
-}
-
-static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
-					   loff_t *pos)
-{
-	struct amd_pmc_stb_v2_data *data = filp->private_data;
-
-	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
-}
-
-static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
-{
-	kfree(filp->private_data);
-	return 0;
-}
-
-static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
-	.owner = THIS_MODULE,
-	.open = amd_pmc_stb_debugfs_open_v2,
-	.read = amd_pmc_stb_debugfs_read_v2,
-	.release = amd_pmc_stb_debugfs_release_v2,
-};
-
 static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
 {
 	switch (dev->cpu_id) {
@@ -350,18 +162,15 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
 	case AMD_CPU_ID_YC:
 	case AMD_CPU_ID_CB:
 		dev->num_ips = 12;
-		dev->s2d_msg_id = 0xBE;
 		dev->smu_msg = 0x538;
 		break;
 	case AMD_CPU_ID_PS:
 		dev->num_ips = 21;
-		dev->s2d_msg_id = 0x85;
 		dev->smu_msg = 0x538;
 		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
 	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
 		dev->num_ips = 22;
-		dev->s2d_msg_id = 0xDE;
 		dev->smu_msg = 0x938;
 		break;
 	}
@@ -625,20 +434,6 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
 	debugfs_remove_recursive(dev->dbgfs_dir);
 }
 
-static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
-{
-	switch (dev->cpu_id) {
-	case AMD_CPU_ID_YC:
-	case AMD_CPU_ID_CB:
-	case AMD_CPU_ID_PS:
-	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
-	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
-		return true;
-	default:
-		return false;
-	}
-}
-
 static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 {
 	dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL);
@@ -648,15 +443,6 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 			    &s0ix_stats_fops);
 	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
 			    &amd_pmc_idlemask_fops);
-	/* Enable STB only when the module_param is set */
-	if (enable_stb) {
-		if (amd_pmc_is_stb_supported(dev))
-			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
-					    &amd_pmc_stb_debugfs_fops_v2);
-		else
-			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
-					    &amd_pmc_stb_debugfs_fops);
-	}
 }
 
 static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
@@ -683,7 +469,7 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
 	dev_dbg(dev->dev, "AMD_%s_REGISTER_MESSAGE:%x\n", dev->msg_port ? "S2D" : "PMC", value);
 }
 
-static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
+int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
 {
 	int rc;
 	u32 val, message, argument, response;
@@ -975,69 +761,6 @@ static const struct pci_device_id pmc_pci_ids[] = {
 	{ }
 };
 
-static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
-{
-	u32 phys_addr_low, phys_addr_hi;
-	u64 stb_phys_addr;
-	u32 size = 0;
-	int ret;
-
-	/* Spill to DRAM feature uses separate SMU message port */
-	dev->msg_port = 1;
-
-	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
-	if (size != S2D_TELEMETRY_BYTES_MAX)
-		return -EIO;
-
-	/* Get DRAM size */
-	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
-	if (ret || !dev->dram_size)
-		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
-
-	/* Get STB DRAM address */
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
-
-	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
-
-	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
-
-	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
-	if (!dev->stb_virt_addr)
-		return -ENOMEM;
-
-	return 0;
-}
-
-static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
-{
-	int err;
-
-	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
-	if (err) {
-		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
-		return pcibios_err_to_errno(err);
-	}
-
-	return 0;
-}
-
-static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
-{
-	int i, err;
-
-	for (i = 0; i < FIFO_SIZE; i++) {
-		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
-		if (err) {
-			dev_err(dev->dev, "error reading data from stb: 0x%X\n", AMD_PMC_STB_PMI_0);
-			return pcibios_err_to_errno(err);
-		}
-	}
-
-	return 0;
-}
-
 static int amd_pmc_probe(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = &pmc;
@@ -1095,12 +818,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	/* Get num of IP blocks within the SoC */
 	amd_pmc_get_ip_info(dev);
 
-	if (enable_stb && amd_pmc_is_stb_supported(dev)) {
-		err = amd_pmc_s2d_init(dev);
-		if (err)
-			goto err_pci_dev_put;
-	}
-
 	platform_set_drvdata(pdev, dev);
 	if (IS_ENABLED(CONFIG_SUSPEND)) {
 		err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
@@ -1111,6 +828,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	amd_pmc_dbgfs_register(dev);
+	err = amd_pmc_s2d_init(dev);
+	if (err)
+		goto err_pci_dev_put;
+
 	if (IS_ENABLED(CONFIG_AMD_MP2_STB))
 		amd_mp2_stb_init(dev);
 	pm_report_max_hw_sleep(U64_MAX);
diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
index f1166d15c856..07fcb13a4136 100644
--- a/drivers/platform/x86/amd/pmc/pmc.h
+++ b/drivers/platform/x86/amd/pmc/pmc.h
@@ -70,4 +70,13 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
 #define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
 #define PCI_DEVICE_ID_AMD_MP2_STB	0x172c
 
+/* STB S2D(Spill to DRAM) has different message port offset */
+#define AMD_S2D_REGISTER_MESSAGE	0xA20
+#define AMD_S2D_REGISTER_RESPONSE	0xA80
+#define AMD_S2D_REGISTER_ARGUMENT	0xA88
+
+int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
+int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
+int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
+
 #endif /* PMC_H */
-- 
2.34.1

[PATCH v2 2/8] platform/x86/amd/pmc: Update function names to align with new STB file

[Shyam Sundar S K]

With STB now in a separate file, update the function names to match the
correct naming schema by removing the _pmc_ prefix where needed.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 61 +++++++++++++-------------
 drivers/platform/x86/amd/pmc/pmc.c     |  8 ++--
 drivers/platform/x86/amd/pmc/pmc.h     |  4 +-
 3 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 9a34dd94982c..5efec020ecac 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -47,12 +47,12 @@ enum s2d_arg {
 	S2D_DRAM_SIZE,
 };
 
-struct amd_pmc_stb_v2_data {
+struct amd_stb_v2_data {
 	size_t size;
 	u8 data[] __counted_by(size);
 };
 
-int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
+int amd_write_stb(struct amd_pmc_dev *dev, u32 data)
 {
 	int err;
 
@@ -65,7 +65,7 @@ int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
 	return 0;
 }
 
-static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
+static int amd_read_stb(struct amd_pmc_dev *dev, u32 *buf)
 {
 	int i, err;
 
@@ -81,7 +81,7 @@ static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
 	return 0;
 }
 
-static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
+static int amd_stb_debugfs_open(struct inode *inode, struct file *filp)
 {
 	struct amd_pmc_dev *dev = filp->f_inode->i_private;
 	u32 size = FIFO_SIZE * sizeof(u32);
@@ -92,7 +92,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
 	if (!buf)
 		return -ENOMEM;
 
-	rc = amd_pmc_read_stb(dev, buf);
+	rc = amd_read_stb(dev, buf);
 	if (rc) {
 		kfree(buf);
 		return rc;
@@ -102,8 +102,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
 	return rc;
 }
 
-static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
-					loff_t *pos)
+static ssize_t amd_stb_debugfs_read(struct file *filp, char __user *buf, size_t size, loff_t *pos)
 {
 	if (!filp->private_data)
 		return -EINVAL;
@@ -112,24 +111,24 @@ static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, siz
 				       FIFO_SIZE * sizeof(u32));
 }
 
-static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
+static int amd_stb_debugfs_release(struct inode *inode, struct file *filp)
 {
 	kfree(filp->private_data);
 	return 0;
 }
 
-static const struct file_operations amd_pmc_stb_debugfs_fops = {
+static const struct file_operations amd_stb_debugfs_fops = {
 	.owner = THIS_MODULE,
-	.open = amd_pmc_stb_debugfs_open,
-	.read = amd_pmc_stb_debugfs_read,
-	.release = amd_pmc_stb_debugfs_release,
+	.open = amd_stb_debugfs_open,
+	.read = amd_stb_debugfs_read,
+	.release = amd_stb_debugfs_release,
 };
 
 /* Enhanced STB Firmware Reporting Mechanism */
-static int amd_pmc_stb_handle_efr(struct file *filp)
+static int amd_stb_handle_efr(struct file *filp)
 {
 	struct amd_pmc_dev *dev = filp->f_inode->i_private;
-	struct amd_pmc_stb_v2_data *stb_data_arr;
+	struct amd_stb_v2_data *stb_data_arr;
 	u32 fsize;
 
 	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
@@ -144,15 +143,15 @@ static int amd_pmc_stb_handle_efr(struct file *filp)
 	return 0;
 }
 
-static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
+static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 {
 	struct amd_pmc_dev *dev = filp->f_inode->i_private;
 	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
-	struct amd_pmc_stb_v2_data *stb_data_arr;
+	struct amd_stb_v2_data *stb_data_arr;
 	int ret;
 
 	/* Write dummy postcode while reading the STB buffer */
-	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
+	ret = amd_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
 	if (ret)
 		dev_err(dev->dev, "error writing to STB: %d\n", ret);
 
@@ -169,7 +168,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	 * platforms that support enhanced dram size reporting.
 	 */
 	if (dump_custom_stb)
-		return amd_pmc_stb_handle_efr(filp);
+		return amd_stb_handle_efr(filp);
 
 	/* Get the num_samples to calculate the last push location */
 	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
@@ -209,28 +208,28 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	return 0;
 }
 
-static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
-					   loff_t *pos)
+static ssize_t amd_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
+				       loff_t *pos)
 {
-	struct amd_pmc_stb_v2_data *data = filp->private_data;
+	struct amd_stb_v2_data *data = filp->private_data;
 
 	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
 }
 
-static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
+static int amd_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
 {
 	kfree(filp->private_data);
 	return 0;
 }
 
-static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
+static const struct file_operations amd_stb_debugfs_fops_v2 = {
 	.owner = THIS_MODULE,
-	.open = amd_pmc_stb_debugfs_open_v2,
-	.read = amd_pmc_stb_debugfs_read_v2,
-	.release = amd_pmc_stb_debugfs_release_v2,
+	.open = amd_stb_debugfs_open_v2,
+	.read = amd_stb_debugfs_read_v2,
+	.release = amd_stb_debugfs_release_v2,
 };
 
-static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
+static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
 {
 	switch (dev->cpu_id) {
 	case AMD_CPU_ID_YC:
@@ -249,7 +248,7 @@ static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
 	}
 }
 
-int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
+int amd_s2d_init(struct amd_pmc_dev *dev)
 {
 	u32 phys_addr_low, phys_addr_hi;
 	u64 stb_phys_addr;
@@ -259,12 +258,12 @@ int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	if (!enable_stb)
 		return 0;
 
-	if (amd_pmc_is_stb_supported(dev))
+	if (amd_is_stb_supported(dev))
 		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
-				    &amd_pmc_stb_debugfs_fops_v2);
+				    &amd_stb_debugfs_fops_v2);
 	else
 		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
-				    &amd_pmc_stb_debugfs_fops);
+				    &amd_stb_debugfs_fops);
 
 	/* Spill to DRAM feature uses separate SMU message port */
 	dev->msg_port = 1;
diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index a977ff1e52b5..8e7c87505327 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -667,7 +667,7 @@ static void amd_pmc_s2idle_prepare(void)
 		return;
 	}
 
-	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
+	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
 	if (rc)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 }
@@ -686,7 +686,7 @@ static void amd_pmc_s2idle_check(void)
 	/* Dump the IdleMask before we add to the STB */
 	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
 
-	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
+	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
 	if (rc)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 }
@@ -713,7 +713,7 @@ static void amd_pmc_s2idle_restore(void)
 	/* Let SMU know that we are looking for stats */
 	amd_pmc_dump_data(pdev);
 
-	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
+	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
 	if (rc)
 		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
 
@@ -828,7 +828,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	amd_pmc_dbgfs_register(dev);
-	err = amd_pmc_s2d_init(dev);
+	err = amd_s2d_init(dev);
 	if (err)
 		goto err_pci_dev_put;
 
diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
index 07fcb13a4136..7e7f9170124c 100644
--- a/drivers/platform/x86/amd/pmc/pmc.h
+++ b/drivers/platform/x86/amd/pmc/pmc.h
@@ -75,8 +75,8 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
 #define AMD_S2D_REGISTER_RESPONSE	0xA80
 #define AMD_S2D_REGISTER_ARGUMENT	0xA88
 
-int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
-int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
+int amd_s2d_init(struct amd_pmc_dev *dev);
+int amd_write_stb(struct amd_pmc_dev *dev, u32 data);
 int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
 
 #endif /* PMC_H */
-- 
2.34.1

[PATCH v2 3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

[Shyam Sundar S K]

To distinguish between the PMC message port and the S2D (Spill to DRAM)
message port, replace the use of 0 and 1 with an enum.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 5efec020ecac..2b06861c479b 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -47,6 +47,11 @@ enum s2d_arg {
 	S2D_DRAM_SIZE,
 };
 
+enum s2d_msg_port {
+	MSG_PORT_PMC,
+	MSG_PORT_S2D,
+};
+
 struct amd_stb_v2_data {
 	size_t size;
 	u8 data[] __counted_by(size);
@@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 		dev_err(dev->dev, "error writing to STB: %d\n", ret);
 
 	/* Spill to DRAM num_samples uses separate SMU message port */
-	dev->msg_port = 1;
+	dev->msg_port = MSG_PORT_S2D;
 
 	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
 	if (ret)
@@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	/* Get the num_samples to calculate the last push location */
 	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
 	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
+	dev->msg_port = MSG_PORT_PMC;
 	if (ret) {
 		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
 		return ret;
@@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
 				    &amd_stb_debugfs_fops);
 
 	/* Spill to DRAM feature uses separate SMU message port */
-	dev->msg_port = 1;
+	dev->msg_port = MSG_PORT_S2D;
 
 	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
 	if (size != S2D_TELEMETRY_BYTES_MAX)
@@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
+	dev->msg_port = MSG_PORT_PMC;
 
 	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
 	if (!dev->stb_virt_addr)
-- 
2.34.1

[PATCH v2 4/8] platform/x86/amd/pmc: Isolate STB code changes to a new file

[Shyam Sundar S K]

Since S2D (Spill to DRAM) uses different message port offsets compared to
PMC message offsets for communication with PMFW, relocate the S2D macros
from pmc.c to a new file, mp1_stb.c, for better code organization.

Following this change, it is logical to introduce a new structure,
"struct stb_arg," to pass the message, argument, and response offset
details to PMFW via the amd_pmc_send_cmd() call. Additionally, move the
s2d_msg_id member from amd_pmc_dev into the new structure.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 33 +++++++++++++++++---------
 drivers/platform/x86/amd/pmc/pmc.c     | 12 +++++-----
 drivers/platform/x86/amd/pmc/pmc.h     | 14 ++++++-----
 3 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 2b06861c479b..1501793b9281 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -31,6 +31,11 @@
 #define STB_FORCE_FLUSH_DATA	0xCF
 #define FIFO_SIZE		4096
 
+/* STB S2D(Spill to DRAM) has different message port offset */
+#define AMD_S2D_REGISTER_MESSAGE	0xA20
+#define AMD_S2D_REGISTER_RESPONSE	0xA80
+#define AMD_S2D_REGISTER_ARGUMENT	0xA88
+
 static bool enable_stb;
 module_param(enable_stb, bool, 0644);
 MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
@@ -176,7 +181,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 		return amd_stb_handle_efr(filp);
 
 	/* Get the num_samples to calculate the last push location */
-	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
+	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->stb_arg.s2d_msg_id, true);
 	/* Clear msg_port for other SMU operation */
 	dev->msg_port = MSG_PORT_PMC;
 	if (ret) {
@@ -239,18 +244,24 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
 	switch (dev->cpu_id) {
 	case AMD_CPU_ID_YC:
 	case AMD_CPU_ID_CB:
-		dev->s2d_msg_id = 0xBE;
-		return true;
+		dev->stb_arg.s2d_msg_id = 0xBE;
+		break;
 	case AMD_CPU_ID_PS:
-		dev->s2d_msg_id = 0x85;
-		return true;
+		dev->stb_arg.s2d_msg_id = 0x85;
+		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
 	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
-		dev->s2d_msg_id = 0xDE;
-		return true;
+		dev->stb_arg.s2d_msg_id = 0xDE;
+		break;
 	default:
 		return false;
 	}
+
+	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
+	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
+	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
+
+	return true;
 }
 
 int amd_s2d_init(struct amd_pmc_dev *dev)
@@ -273,18 +284,18 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
 	/* Spill to DRAM feature uses separate SMU message port */
 	dev->msg_port = MSG_PORT_S2D;
 
-	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
+	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->stb_arg.s2d_msg_id, true);
 	if (size != S2D_TELEMETRY_BYTES_MAX)
 		return -EIO;
 
 	/* Get DRAM size */
-	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
+	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->stb_arg.s2d_msg_id, true);
 	if (ret || !dev->dram_size)
 		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
 
 	/* Get STB DRAM address */
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->stb_arg.s2d_msg_id, true);
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->stb_arg.s2d_msg_id, true);
 
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 8e7c87505327..f9900a03391a 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -450,9 +450,9 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
 	u32 value, message, argument, response;
 
 	if (dev->msg_port) {
-		message = AMD_S2D_REGISTER_MESSAGE;
-		argument = AMD_S2D_REGISTER_ARGUMENT;
-		response = AMD_S2D_REGISTER_RESPONSE;
+		message = dev->stb_arg.msg;
+		argument = dev->stb_arg.arg;
+		response = dev->stb_arg.resp;
 	} else {
 		message = dev->smu_msg;
 		argument = AMD_PMC_REGISTER_ARGUMENT;
@@ -477,9 +477,9 @@ int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool r
 	mutex_lock(&dev->lock);
 
 	if (dev->msg_port) {
-		message = AMD_S2D_REGISTER_MESSAGE;
-		argument = AMD_S2D_REGISTER_ARGUMENT;
-		response = AMD_S2D_REGISTER_RESPONSE;
+		message = dev->stb_arg.msg;
+		argument = dev->stb_arg.arg;
+		response = dev->stb_arg.resp;
 	} else {
 		message = dev->smu_msg;
 		argument = AMD_PMC_REGISTER_ARGUMENT;
diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
index 7e7f9170124c..d3c6730ebcd7 100644
--- a/drivers/platform/x86/amd/pmc/pmc.h
+++ b/drivers/platform/x86/amd/pmc/pmc.h
@@ -25,6 +25,13 @@ struct amd_mp2_dev {
 	bool is_stb_data;
 };
 
+struct stb_arg {
+	u32 s2d_msg_id;
+	u32 msg;
+	u32 arg;
+	u32 resp;
+};
+
 struct amd_pmc_dev {
 	void __iomem *regbase;
 	void __iomem *smu_virt_addr;
@@ -36,7 +43,6 @@ struct amd_pmc_dev {
 	u32 active_ips;
 	u32 dram_size;
 	u32 num_ips;
-	u32 s2d_msg_id;
 	u32 smu_msg;
 /* SMU version information */
 	u8 smu_program;
@@ -50,6 +56,7 @@ struct amd_pmc_dev {
 	struct quirk_entry *quirks;
 	bool disable_8042_wakeup;
 	struct amd_mp2_dev *mp2;
+	struct stb_arg stb_arg;
 };
 
 void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
@@ -70,11 +77,6 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
 #define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
 #define PCI_DEVICE_ID_AMD_MP2_STB	0x172c
 
-/* STB S2D(Spill to DRAM) has different message port offset */
-#define AMD_S2D_REGISTER_MESSAGE	0xA20
-#define AMD_S2D_REGISTER_RESPONSE	0xA80
-#define AMD_S2D_REGISTER_ARGUMENT	0xA88
-
 int amd_s2d_init(struct amd_pmc_dev *dev);
 int amd_write_stb(struct amd_pmc_dev *dev, u32 data);
 int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
-- 
2.34.1

[PATCH v2 5/8] platform/x86/amd/pmc: Update IP information structure for newer SoCs

[Shyam Sundar S K]

The latest AMD processors include additional IP blocks that must be turned
off before transitioning to low power. PMFW provides an interface to
retrieve debug information from each IP block, which is useful for
diagnosing issues if the system fails to enter or exit low power states,
or for profiling which IP block takes more time. Add support for using
this information within the driver.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index f9900a03391a..0bf4065153da 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -95,6 +95,35 @@ struct amd_pmc_bit_map {
 	u32 bit_mask;
 };
 
+static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
+	{"DISPLAY",     BIT(0)},
+	{"CPU",         BIT(1)},
+	{"GFX",         BIT(2)},
+	{"VDD",         BIT(3)},
+	{"VDD_CCX",     BIT(4)},
+	{"ACP",         BIT(5)},
+	{"VCN_0",       BIT(6)},
+	{"VCN_1",       BIT(7)},
+	{"ISP",         BIT(8)},
+	{"NBIO",        BIT(9)},
+	{"DF",          BIT(10)},
+	{"USB3_0",      BIT(11)},
+	{"USB3_1",      BIT(12)},
+	{"LAPIC",       BIT(13)},
+	{"USB3_2",      BIT(14)},
+	{"USB4_RT0",	BIT(15)},
+	{"USB4_RT1",	BIT(16)},
+	{"USB4_0",      BIT(17)},
+	{"USB4_1",      BIT(18)},
+	{"MPM",         BIT(19)},
+	{"JPEG_0",      BIT(20)},
+	{"JPEG_1",      BIT(21)},
+	{"IPU",         BIT(22)},
+	{"UMSCH",       BIT(23)},
+	{"VPE",         BIT(24)},
+	{}
+};
+
 static const struct amd_pmc_bit_map soc15_ip_blk[] = {
 	{"DISPLAY",	BIT(0)},
 	{"CPU",		BIT(1)},
@@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
 		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
 	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
-		dev->num_ips = 22;
+		if (boot_cpu_data.x86_model == 0x70)
+			dev->num_ips = 25;
+		else
+			dev->num_ips = 22;
 		dev->smu_msg = 0x938;
 		break;
 	}
@@ -338,9 +370,15 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
 
 	seq_puts(s, "\n=== Active time (in us) ===\n");
 	for (idx = 0 ; idx < dev->num_ips ; idx++) {
-		if (soc15_ip_blk[idx].bit_mask & dev->active_ips)
+		if (dev->cpu_id == PCI_DEVICE_ID_AMD_1AH_M20H_ROOT &&
+		    boot_cpu_data.x86_model == 0x70) {
+			if (soc15_ip_blk_v2[idx].bit_mask & dev->active_ips)
+				seq_printf(s, "%-8s : %lld\n", soc15_ip_blk_v2[idx].name,
+					   table.timecondition_notmet_lastcapture[idx]);
+		} else if (soc15_ip_blk[idx].bit_mask & dev->active_ips) {
 			seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
 				   table.timecondition_notmet_lastcapture[idx]);
+		}
 	}
 
 	return 0;
-- 
2.34.1

[PATCH v2 6/8] platform/x86/amd/pmc: Update S2D message id for 1Ah Family 70h model

[Shyam Sundar S K]

AMD's 1Ah family 70h model uses a different S2D (Spill to DRAM) message
ID. Update the driver with this information.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 1501793b9281..917c111b31c9 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -251,7 +251,10 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
 		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
 	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
-		dev->stb_arg.s2d_msg_id = 0xDE;
+		if (boot_cpu_data.x86_model == 0x70)
+			dev->stb_arg.s2d_msg_id = 0xF1;
+		else
+			dev->stb_arg.s2d_msg_id = 0xDE;
 		break;
 	default:
 		return false;
-- 
2.34.1

[PATCH v2 7/8] platform/x86/amd/pmc: Add STB support for AMD Desktop variants

[Shyam Sundar S K]

Previously, AMD's Ryzen Desktop SoCs did not include support for STB.
However, to accommodate this recent change, PMFW has implemented a new
message port pair mechanism for handling messages, arguments, and
responses, specifically designed for distinguishing from Mobile SoCs.
Therefore, it is necessary to update the driver to properly handle this
incoming change.

Add a new function amd_stb_update_args() to simply the arguments that
needs to be passed between S2D supported Mobile SoCs vs Desktop SoCs.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 31 +++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 917c111b31c9..6a3cfcbb614e 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -36,6 +36,11 @@
 #define AMD_S2D_REGISTER_RESPONSE	0xA80
 #define AMD_S2D_REGISTER_ARGUMENT	0xA88
 
+/* STB S2D(Spill to DRAM) message port offset for 44h model */
+#define AMD_GNR_REGISTER_MESSAGE	0x524
+#define AMD_GNR_REGISTER_RESPONSE	0x570
+#define AMD_GNR_REGISTER_ARGUMENT	0xA40
+
 static bool enable_stb;
 module_param(enable_stb, bool, 0644);
 MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
@@ -239,12 +244,31 @@ static const struct file_operations amd_stb_debugfs_fops_v2 = {
 	.release = amd_stb_debugfs_release_v2,
 };
 
+static void amd_stb_update_args(struct amd_pmc_dev *dev)
+{
+	if (cpu_feature_enabled(X86_FEATURE_ZEN5))
+		switch (boot_cpu_data.x86_model) {
+		case 0x44:
+			dev->stb_arg.msg = AMD_GNR_REGISTER_MESSAGE;
+			dev->stb_arg.arg = AMD_GNR_REGISTER_ARGUMENT;
+			dev->stb_arg.resp = AMD_GNR_REGISTER_RESPONSE;
+			return;
+	}
+
+	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
+	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
+	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
+}
+
 static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
 {
 	switch (dev->cpu_id) {
 	case AMD_CPU_ID_YC:
 	case AMD_CPU_ID_CB:
-		dev->stb_arg.s2d_msg_id = 0xBE;
+		if (boot_cpu_data.x86_model == 0x44)
+			dev->stb_arg.s2d_msg_id = 0x9B;
+		else
+			dev->stb_arg.s2d_msg_id = 0xBE;
 		break;
 	case AMD_CPU_ID_PS:
 		dev->stb_arg.s2d_msg_id = 0x85;
@@ -260,10 +284,7 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
 		return false;
 	}
 
-	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
-	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
-	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
-
+	amd_stb_update_args(dev);
 	return true;
 }
 
-- 
2.34.1

[PATCH v2 8/8] MAINTAINERS: Change AMD PMF driver status to "Supported"

[Shyam Sundar S K]

The AMD PMC driver is actively being developed, so the MAINTAINERS record
should reflect "Supported" instead of "Maintained." Update the MAINTAINERS
database to reflect this change.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index a27407950242..6594a74b3d11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1118,7 +1118,7 @@ F:	include/linux/pds/
 AMD PMC DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
-S:	Maintained
+S:	Supported
 F:	drivers/platform/x86/amd/pmc/
 
 AMD PMF DRIVER
-- 
2.34.1

Re: [PATCH v2 6/8] platform/x86/amd/pmc: Update S2D message id for 1Ah Family 70h model

[Mario Limonciello]

On 10/29/2024 10:54, Shyam Sundar S K wrote:
> AMD's 1Ah family 70h model uses a different S2D (Spill to DRAM) message
> ID. Update the driver with this information.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

amd_is_stb_supported() might also want a refactor in the future too with 
cpu_feature_enabled(), but as a minimal patch this makes sense.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/amd/pmc/mp1_stb.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 1501793b9281..917c111b31c9 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -251,7 +251,10 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>   		break;
>   	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>   	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> -		dev->stb_arg.s2d_msg_id = 0xDE;
> +		if (boot_cpu_data.x86_model == 0x70)
> +			dev->stb_arg.s2d_msg_id = 0xF1;
> +		else
> +			dev->stb_arg.s2d_msg_id = 0xDE;
>   		break;
>   	default:
>   		return false;

Re: [PATCH v2 7/8] platform/x86/amd/pmc: Add STB support for AMD Desktop variants

[Mario Limonciello]

On 10/29/2024 10:54, Shyam Sundar S K wrote:
> Previously, AMD's Ryzen Desktop SoCs did not include support for STB.
> However, to accommodate this recent change, PMFW has implemented a new
> message port pair mechanism for handling messages, arguments, and
> responses, specifically designed for distinguishing from Mobile SoCs.
> Therefore, it is necessary to update the driver to properly handle this
> incoming change.
> 
> Add a new function amd_stb_update_args() to simply the arguments that
> needs to be passed between S2D supported Mobile SoCs vs Desktop SoCs.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/amd/pmc/mp1_stb.c | 31 +++++++++++++++++++++-----
>   1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 917c111b31c9..6a3cfcbb614e 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -36,6 +36,11 @@
>   #define AMD_S2D_REGISTER_RESPONSE	0xA80
>   #define AMD_S2D_REGISTER_ARGUMENT	0xA88
>   
> +/* STB S2D(Spill to DRAM) message port offset for 44h model */
> +#define AMD_GNR_REGISTER_MESSAGE	0x524
> +#define AMD_GNR_REGISTER_RESPONSE	0x570
> +#define AMD_GNR_REGISTER_ARGUMENT	0xA40
> +
>   static bool enable_stb;
>   module_param(enable_stb, bool, 0644);
>   MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> @@ -239,12 +244,31 @@ static const struct file_operations amd_stb_debugfs_fops_v2 = {
>   	.release = amd_stb_debugfs_release_v2,
>   };
>   
> +static void amd_stb_update_args(struct amd_pmc_dev *dev)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN5))
> +		switch (boot_cpu_data.x86_model) {
> +		case 0x44:
> +			dev->stb_arg.msg = AMD_GNR_REGISTER_MESSAGE;
> +			dev->stb_arg.arg = AMD_GNR_REGISTER_ARGUMENT;
> +			dev->stb_arg.resp = AMD_GNR_REGISTER_RESPONSE;
> +			return;
> +	}
> +
> +	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
> +	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
> +	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
> +}
> +
>   static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>   {
>   	switch (dev->cpu_id) {
>   	case AMD_CPU_ID_YC:
>   	case AMD_CPU_ID_CB:
> -		dev->stb_arg.s2d_msg_id = 0xBE;
> +		if (boot_cpu_data.x86_model == 0x44)
> +			dev->stb_arg.s2d_msg_id = 0x9B;
> +		else
> +			dev->stb_arg.s2d_msg_id = 0xBE;
>   		break;
>   	case AMD_CPU_ID_PS:
>   		dev->stb_arg.s2d_msg_id = 0x85;
> @@ -260,10 +284,7 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>   		return false;
>   	}
>   
> -	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
> -	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
> -	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
> -
> +	amd_stb_update_args(dev);
>   	return true;
>   }
>   

Re: [PATCH v2 1/8] platform/x86/amd/pmc: Move STB functionality to a new file for better code organization

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> As the SoC evolves with each generation, the dynamics between the PMC and
> STB layers within the PMC driver are becoming increasingly complex, making
> it challenging to manage both in a single file and maintain code
> readability.
> 
> Additionally, during silicon bringup, the PMC functionality is often
> enabled first, with STB functionality added later. This can lead to missed
> updates in the driver, potentially causing issues.
>
> To address these challenges, it's beneficial to move all STB-related
> changes to a separate file. This approach will better accommodate newer
> SoCs, provide improved flexibility for desktop variants, and facilitate
> the collection of additional debug information through STB mechanisms.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/Makefile  |   2 +-
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 295 +++++++++++++++++++++++++
>  drivers/platform/x86/amd/pmc/pmc.c     | 289 +-----------------------
>  drivers/platform/x86/amd/pmc/pmc.h     |   9 +
>  4 files changed, 310 insertions(+), 285 deletions(-)
>  create mode 100644 drivers/platform/x86/amd/pmc/mp1_stb.c
> 
> diff --git a/drivers/platform/x86/amd/pmc/Makefile b/drivers/platform/x86/amd/pmc/Makefile
> index f1d9ab19d24c..255d94ddf999 100644
> --- a/drivers/platform/x86/amd/pmc/Makefile
> +++ b/drivers/platform/x86/amd/pmc/Makefile
> @@ -4,6 +4,6 @@
>  # AMD Power Management Controller Driver
>  #
>  
> -amd-pmc-objs := pmc.o pmc-quirks.o
> +amd-pmc-objs := pmc.o pmc-quirks.o mp1_stb.o
>  obj-$(CONFIG_AMD_PMC) += amd-pmc.o
>  amd-pmc-$(CONFIG_AMD_MP2_STB) += mp2_stb.o
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> new file mode 100644
> index 000000000000..9a34dd94982c
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AMD MP1 Smart Trace Buffer (STB) Layer
> + *
> + * Copyright (c) 2024, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + *          Sanket Goswami <Sanket.Goswami@amd.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <asm/amd_nb.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/uaccess.h>
> +
> +#include "pmc.h"
> +
> +/* STB Spill to DRAM Parameters */
> +#define S2D_TELEMETRY_BYTES_MAX		0x100000U
> +#define S2D_RSVD_RAM_SPACE		0x100000
> +#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
> +
> +/* STB Registers */
> +#define AMD_PMC_STB_PMI_0	0x03E30600

Is this still needed in pmc.c? I think all users moved to this file.

> +#define AMD_PMC_STB_DUMMY_PC	0xC6000007
> +
> +/* STB Spill to DRAM Message Definition */
> +#define STB_FORCE_FLUSH_DATA	0xCF
> +#define FIFO_SIZE		4096
> +
> +static bool enable_stb;
> +module_param(enable_stb, bool, 0644);
> +MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> +
> +static bool dump_custom_stb;
> +module_param(dump_custom_stb, bool, 0644);
> +MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
> +
> +enum s2d_arg {
> +	S2D_TELEMETRY_SIZE = 0x01,
> +	S2D_PHYS_ADDR_LOW,
> +	S2D_PHYS_ADDR_HIGH,
> +	S2D_NUM_SAMPLES,
> +	S2D_DRAM_SIZE,
> +};
> +
> +struct amd_pmc_stb_v2_data {
> +	size_t size;
> +	u8 data[] __counted_by(size);
> +};
> +
> +int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +{
> +	int err;
> +
> +	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
> +	if (err) {
> +		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
> +		return pcibios_err_to_errno(err);
> +	}
> +
> +	return 0;
> +}
> +
> +static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < FIFO_SIZE; i++) {
> +		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
> +		if (err) {
> +			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
> +				AMD_PMC_STB_PMI_0);
> +			return pcibios_err_to_errno(err);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> +{
> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> +	u32 size = FIFO_SIZE * sizeof(u32);
> +	u32 *buf;
> +	int rc;
> +
> +	buf = kzalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	rc = amd_pmc_read_stb(dev, buf);
> +	if (rc) {
> +		kfree(buf);
> +		return rc;
> +	}
> +
> +	filp->private_data = buf;
> +	return rc;
> +}
> +
> +static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> +					loff_t *pos)
> +{
> +	if (!filp->private_data)
> +		return -EINVAL;
> +
> +	return simple_read_from_buffer(buf, size, pos, filp->private_data,
> +				       FIFO_SIZE * sizeof(u32));
> +}
> +
> +static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
> +{
> +	kfree(filp->private_data);
> +	return 0;
> +}
> +
> +static const struct file_operations amd_pmc_stb_debugfs_fops = {
> +	.owner = THIS_MODULE,
> +	.open = amd_pmc_stb_debugfs_open,
> +	.read = amd_pmc_stb_debugfs_read,
> +	.release = amd_pmc_stb_debugfs_release,
> +};
> +
> +/* Enhanced STB Firmware Reporting Mechanism */
> +static int amd_pmc_stb_handle_efr(struct file *filp)
> +{
> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> +	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	u32 fsize;
> +
> +	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
> +	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
> +	if (!stb_data_arr)
> +		return -ENOMEM;
> +
> +	stb_data_arr->size = fsize;
> +	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
> +	filp->private_data = stb_data_arr;
> +
> +	return 0;
> +}
> +
> +static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> +{
> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> +	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> +	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	int ret;
> +
> +	/* Write dummy postcode while reading the STB buffer */
> +	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
> +	if (ret)
> +		dev_err(dev->dev, "error writing to STB: %d\n", ret);
> +
> +	/* Spill to DRAM num_samples uses separate SMU message port */
> +	dev->msg_port = 1;
> +
> +	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
> +	if (ret)
> +		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
> +
> +	/*
> +	 * We have a custom stb size and the PMFW is supposed to give
> +	 * the enhanced dram size. Note that we land here only for the
> +	 * platforms that support enhanced dram size reporting.
> +	 */
> +	if (dump_custom_stb)
> +		return amd_pmc_stb_handle_efr(filp);
> +
> +	/* Get the num_samples to calculate the last push location */
> +	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> +	/* Clear msg_port for other SMU operation */
> +	dev->msg_port = 0;
> +	if (ret) {
> +		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> +		return ret;
> +	}
> +
> +	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
> +	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
> +	if (!stb_data_arr)
> +		return -ENOMEM;
> +
> +	stb_data_arr->size = fsize;
> +
> +	/*
> +	 * Start capturing data from the last push location.
> +	 * This is for general cases, where the stb limits
> +	 * are meant for standard usage.
> +	 */
> +	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
> +		/* First read oldest data starting 1 behind last write till end of ringbuffer */
> +		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
> +		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
> +
> +		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> +		/* Second copy the newer samples from offset 0 - last write */
> +		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
> +	} else {
> +		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
> +	}
> +
> +	filp->private_data = stb_data_arr;
> +
> +	return 0;
> +}
> +
> +static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> +					   loff_t *pos)
> +{
> +	struct amd_pmc_stb_v2_data *data = filp->private_data;
> +
> +	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
> +}
> +
> +static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
> +{
> +	kfree(filp->private_data);
> +	return 0;
> +}
> +
> +static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
> +	.owner = THIS_MODULE,
> +	.open = amd_pmc_stb_debugfs_open_v2,
> +	.read = amd_pmc_stb_debugfs_read_v2,
> +	.release = amd_pmc_stb_debugfs_release_v2,
> +};
> +
> +static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
> +{
> +	switch (dev->cpu_id) {
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_CB:
> +		dev->s2d_msg_id = 0xBE;
> +		return true;
> +	case AMD_CPU_ID_PS:
> +		dev->s2d_msg_id = 0x85;
> +		return true;
> +	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> +		dev->s2d_msg_id = 0xDE;
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
> +{
> +	u32 phys_addr_low, phys_addr_hi;
> +	u64 stb_phys_addr;
> +	u32 size = 0;
> +	int ret;
> +
> +	if (!enable_stb)
> +		return 0;
> +
> +	if (amd_pmc_is_stb_supported(dev))
> +		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> +				    &amd_pmc_stb_debugfs_fops_v2);
> +	else
> +		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> +				    &amd_pmc_stb_debugfs_fops);
> +
> +	/* Spill to DRAM feature uses separate SMU message port */
> +	dev->msg_port = 1;
> +
> +	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
> +	if (size != S2D_TELEMETRY_BYTES_MAX)
> +		return -EIO;
> +
> +	/* Get DRAM size */
> +	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
> +	if (ret || !dev->dram_size)
> +		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
> +
> +	/* Get STB DRAM address */
> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
> +
> +	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> +
> +	/* Clear msg_port for other SMU operation */
> +	dev->msg_port = 0;
> +
> +	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
> +	if (!dev->stb_virt_addr)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index bbb8edb62e00..a977ff1e52b5 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -44,20 +44,6 @@
>  #define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
>  #define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
>  #define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
> -#define AMD_PMC_STB_DUMMY_PC		0xC6000007
> -
> -/* STB S2D(Spill to DRAM) has different message port offset */
> -#define AMD_S2D_REGISTER_MESSAGE	0xA20
> -#define AMD_S2D_REGISTER_RESPONSE	0xA80
> -#define AMD_S2D_REGISTER_ARGUMENT	0xA88
> -
> -/* STB Spill to DRAM Parameters */
> -#define S2D_TELEMETRY_BYTES_MAX		0x100000U
> -#define S2D_RSVD_RAM_SPACE		0x100000
> -#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
> -
> -/* STB Spill to DRAM Message Definition */
> -#define STB_FORCE_FLUSH_DATA		0xCF
>  
>  /* Base address of SMU for mapping physical address to virtual address */
>  #define AMD_PMC_MAPPING_SIZE		0x01000
> @@ -97,7 +83,6 @@
>  
>  #define DELAY_MIN_US		2000
>  #define DELAY_MAX_US		3000
> -#define FIFO_SIZE		4096
>  
>  enum amd_pmc_def {
>  	MSG_TEST = 0x01,
> @@ -105,19 +90,6 @@ enum amd_pmc_def {
>  	MSG_OS_HINT_RN,
>  };
>  
> -enum s2d_arg {
> -	S2D_TELEMETRY_SIZE = 0x01,
> -	S2D_PHYS_ADDR_LOW,
> -	S2D_PHYS_ADDR_HIGH,
> -	S2D_NUM_SAMPLES,
> -	S2D_DRAM_SIZE,
> -};
> -
> -struct amd_pmc_stb_v2_data {
> -	size_t size;
> -	u8 data[] __counted_by(size);
> -};
> -
>  struct amd_pmc_bit_map {
>  	const char *name;
>  	u32 bit_mask;
> @@ -149,22 +121,11 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>  	{}
>  };
>  
> -static bool enable_stb;
> -module_param(enable_stb, bool, 0644);
> -MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> -
>  static bool disable_workarounds;
>  module_param(disable_workarounds, bool, 0644);
>  MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
>  
> -static bool dump_custom_stb;
> -module_param(dump_custom_stb, bool, 0644);
> -MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
> -
>  static struct amd_pmc_dev pmc;
> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
> -static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
>  
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>  {
> @@ -193,155 +154,6 @@ struct smu_metrics {
>  	u64 timecondition_notmet_totaltime[32];
>  } __packed;
>  
> -static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> -{
> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	u32 size = FIFO_SIZE * sizeof(u32);
> -	u32 *buf;
> -	int rc;
> -
> -	buf = kzalloc(size, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	rc = amd_pmc_read_stb(dev, buf);
> -	if (rc) {
> -		kfree(buf);
> -		return rc;
> -	}
> -
> -	filp->private_data = buf;
> -	return rc;
> -}
> -
> -static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> -					loff_t *pos)
> -{
> -	if (!filp->private_data)
> -		return -EINVAL;
> -
> -	return simple_read_from_buffer(buf, size, pos, filp->private_data,
> -				       FIFO_SIZE * sizeof(u32));
> -}
> -
> -static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
> -{
> -	kfree(filp->private_data);
> -	return 0;
> -}
> -
> -static const struct file_operations amd_pmc_stb_debugfs_fops = {
> -	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open,
> -	.read = amd_pmc_stb_debugfs_read,
> -	.release = amd_pmc_stb_debugfs_release,
> -};
> -
> -/* Enhanced STB Firmware Reporting Mechanism */
> -static int amd_pmc_stb_handle_efr(struct file *filp)
> -{
> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> -	u32 fsize;
> -
> -	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
> -	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
> -	if (!stb_data_arr)
> -		return -ENOMEM;
> -
> -	stb_data_arr->size = fsize;
> -	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
> -	filp->private_data = stb_data_arr;
> -
> -	return 0;
> -}
> -
> -static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> -{
> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> -	int ret;
> -
> -	/* Write dummy postcode while reading the STB buffer */
> -	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
> -	if (ret)
> -		dev_err(dev->dev, "error writing to STB: %d\n", ret);
> -
> -	/* Spill to DRAM num_samples uses separate SMU message port */
> -	dev->msg_port = 1;
> -
> -	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
> -	if (ret)
> -		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
> -
> -	/*
> -	 * We have a custom stb size and the PMFW is supposed to give
> -	 * the enhanced dram size. Note that we land here only for the
> -	 * platforms that support enhanced dram size reporting.
> -	 */
> -	if (dump_custom_stb)
> -		return amd_pmc_stb_handle_efr(filp);
> -
> -	/* Get the num_samples to calculate the last push location */
> -	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> -	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> -	if (ret) {
> -		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> -		return ret;
> -	}
> -
> -	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
> -	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
> -	if (!stb_data_arr)
> -		return -ENOMEM;
> -
> -	stb_data_arr->size = fsize;
> -
> -	/*
> -	 * Start capturing data from the last push location.
> -	 * This is for general cases, where the stb limits
> -	 * are meant for standard usage.
> -	 */
> -	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
> -		/* First read oldest data starting 1 behind last write till end of ringbuffer */
> -		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
> -		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
> -
> -		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> -		/* Second copy the newer samples from offset 0 - last write */
> -		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
> -	} else {
> -		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
> -	}
> -
> -	filp->private_data = stb_data_arr;
> -
> -	return 0;
> -}
> -
> -static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> -					   loff_t *pos)
> -{
> -	struct amd_pmc_stb_v2_data *data = filp->private_data;
> -
> -	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
> -}
> -
> -static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
> -{
> -	kfree(filp->private_data);
> -	return 0;
> -}
> -
> -static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
> -	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open_v2,
> -	.read = amd_pmc_stb_debugfs_read_v2,
> -	.release = amd_pmc_stb_debugfs_release_v2,
> -};
> -
>  static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>  {
>  	switch (dev->cpu_id) {
> @@ -350,18 +162,15 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>  	case AMD_CPU_ID_YC:
>  	case AMD_CPU_ID_CB:
>  		dev->num_ips = 12;
> -		dev->s2d_msg_id = 0xBE;
>  		dev->smu_msg = 0x538;
>  		break;
>  	case AMD_CPU_ID_PS:
>  		dev->num_ips = 21;
> -		dev->s2d_msg_id = 0x85;
>  		dev->smu_msg = 0x538;
>  		break;
>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>  		dev->num_ips = 22;
> -		dev->s2d_msg_id = 0xDE;
>  		dev->smu_msg = 0x938;
>  		break;
>  	}
> @@ -625,20 +434,6 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>  	debugfs_remove_recursive(dev->dbgfs_dir);
>  }
>  
> -static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
> -{
> -	switch (dev->cpu_id) {
> -	case AMD_CPU_ID_YC:
> -	case AMD_CPU_ID_CB:
> -	case AMD_CPU_ID_PS:
> -	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> -	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>  static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  {
>  	dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL);
> @@ -648,15 +443,6 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  			    &s0ix_stats_fops);
>  	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
>  			    &amd_pmc_idlemask_fops);
> -	/* Enable STB only when the module_param is set */
> -	if (enable_stb) {
> -		if (amd_pmc_is_stb_supported(dev))
> -			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -					    &amd_pmc_stb_debugfs_fops_v2);
> -		else
> -			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -					    &amd_pmc_stb_debugfs_fops);
> -	}

Is it related to the logic change I ask about down below? It looks 
something that really ought to be done in a separate preparatory patch if 
that's the case since it seems entire independent of moving things to 
another file this patch is supposed to be all about.

>  }
>  
>  static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
> @@ -683,7 +469,7 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>  	dev_dbg(dev->dev, "AMD_%s_REGISTER_MESSAGE:%x\n", dev->msg_port ? "S2D" : "PMC", value);
>  }
>  
> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
> +int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
>  {
>  	int rc;
>  	u32 val, message, argument, response;
> @@ -975,69 +761,6 @@ static const struct pci_device_id pmc_pci_ids[] = {
>  	{ }
>  };
>  
> -static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
> -{
> -	u32 phys_addr_low, phys_addr_hi;
> -	u64 stb_phys_addr;
> -	u32 size = 0;
> -	int ret;
> -
> -	/* Spill to DRAM feature uses separate SMU message port */
> -	dev->msg_port = 1;
> -
> -	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
> -	if (size != S2D_TELEMETRY_BYTES_MAX)
> -		return -EIO;
> -
> -	/* Get DRAM size */
> -	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
> -	if (ret || !dev->dram_size)
> -		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
> -
> -	/* Get STB DRAM address */
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
> -
> -	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> -
> -	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> -
> -	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
> -	if (!dev->stb_virt_addr)
> -		return -ENOMEM;
> -
> -	return 0;
> -}
> -
> -static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> -{
> -	int err;
> -
> -	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
> -	if (err) {
> -		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
> -		return pcibios_err_to_errno(err);
> -	}
> -
> -	return 0;
> -}
> -
> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
> -{
> -	int i, err;
> -
> -	for (i = 0; i < FIFO_SIZE; i++) {
> -		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
> -		if (err) {
> -			dev_err(dev->dev, "error reading data from stb: 0x%X\n", AMD_PMC_STB_PMI_0);
> -			return pcibios_err_to_errno(err);
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static int amd_pmc_probe(struct platform_device *pdev)
>  {
>  	struct amd_pmc_dev *dev = &pmc;
> @@ -1095,12 +818,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	/* Get num of IP blocks within the SoC */
>  	amd_pmc_get_ip_info(dev);
>  
> -	if (enable_stb && amd_pmc_is_stb_supported(dev)) {
> -		err = amd_pmc_s2d_init(dev);
> -		if (err)
> -			goto err_pci_dev_put;
> -	}
> -
>  	platform_set_drvdata(pdev, dev);
>  	if (IS_ENABLED(CONFIG_SUSPEND)) {
>  		err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
> @@ -1111,6 +828,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	amd_pmc_dbgfs_register(dev);
> +	err = amd_pmc_s2d_init(dev);
> +	if (err)
> +		goto err_pci_dev_put;

Why isn't this logic change in a separate patch?

> +
>  	if (IS_ENABLED(CONFIG_AMD_MP2_STB))
>  		amd_mp2_stb_init(dev);
>  	pm_report_max_hw_sleep(U64_MAX);
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index f1166d15c856..07fcb13a4136 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -70,4 +70,13 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>  #define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
>  #define PCI_DEVICE_ID_AMD_MP2_STB	0x172c
>  
> +/* STB S2D(Spill to DRAM) has different message port offset */
> +#define AMD_S2D_REGISTER_MESSAGE	0xA20
> +#define AMD_S2D_REGISTER_RESPONSE	0xA80
> +#define AMD_S2D_REGISTER_ARGUMENT	0xA88
> +
> +int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
> +int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
> +int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
> +
>  #endif /* PMC_H */
> 

-- 
 i.

Re: [PATCH v2 2/8] platform/x86/amd/pmc: Update function names to align with new STB file

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> With STB now in a separate file, update the function names to match the
> correct naming schema by removing the _pmc_ prefix where needed.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 61 +++++++++++++-------------
>  drivers/platform/x86/amd/pmc/pmc.c     |  8 ++--
>  drivers/platform/x86/amd/pmc/pmc.h     |  4 +-
>  3 files changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 9a34dd94982c..5efec020ecac 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -47,12 +47,12 @@ enum s2d_arg {
>  	S2D_DRAM_SIZE,
>  };
>  
> -struct amd_pmc_stb_v2_data {
> +struct amd_stb_v2_data {
>  	size_t size;
>  	u8 data[] __counted_by(size);
>  };
>  
> -int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +int amd_write_stb(struct amd_pmc_dev *dev, u32 data)
>  {
>  	int err;
>  
> @@ -65,7 +65,7 @@ int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
>  	return 0;
>  }
>  
> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
> +static int amd_read_stb(struct amd_pmc_dev *dev, u32 *buf)

Why aren't these two consistent with the rest that start with amd_stb_?

And thanks for doing this in a patch separate from the move, it's just so 
much simpler to review them independently. :-)

>  {
>  	int i, err;
>  
> @@ -81,7 +81,7 @@ static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
>  	return 0;
>  }
>  
> -static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_open(struct inode *inode, struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>  	u32 size = FIFO_SIZE * sizeof(u32);
> @@ -92,7 +92,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	rc = amd_pmc_read_stb(dev, buf);
> +	rc = amd_read_stb(dev, buf);
>  	if (rc) {
>  		kfree(buf);
>  		return rc;
> @@ -102,8 +102,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>  	return rc;
>  }
>  
> -static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> -					loff_t *pos)
> +static ssize_t amd_stb_debugfs_read(struct file *filp, char __user *buf, size_t size, loff_t *pos)
>  {
>  	if (!filp->private_data)
>  		return -EINVAL;
> @@ -112,24 +111,24 @@ static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, siz
>  				       FIFO_SIZE * sizeof(u32));
>  }
>  
> -static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_release(struct inode *inode, struct file *filp)
>  {
>  	kfree(filp->private_data);
>  	return 0;
>  }
>  
> -static const struct file_operations amd_pmc_stb_debugfs_fops = {
> +static const struct file_operations amd_stb_debugfs_fops = {
>  	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open,
> -	.read = amd_pmc_stb_debugfs_read,
> -	.release = amd_pmc_stb_debugfs_release,
> +	.open = amd_stb_debugfs_open,
> +	.read = amd_stb_debugfs_read,
> +	.release = amd_stb_debugfs_release,
>  };
>  
>  /* Enhanced STB Firmware Reporting Mechanism */
> -static int amd_pmc_stb_handle_efr(struct file *filp)
> +static int amd_stb_handle_efr(struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	struct amd_stb_v2_data *stb_data_arr;
>  	u32 fsize;
>  
>  	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
> @@ -144,15 +143,15 @@ static int amd_pmc_stb_handle_efr(struct file *filp)
>  	return 0;
>  }
>  
> -static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>  	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	struct amd_stb_v2_data *stb_data_arr;
>  	int ret;
>  
>  	/* Write dummy postcode while reading the STB buffer */
> -	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
> +	ret = amd_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
>  	if (ret)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
> @@ -169,7 +168,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	 * platforms that support enhanced dram size reporting.
>  	 */
>  	if (dump_custom_stb)
> -		return amd_pmc_stb_handle_efr(filp);
> +		return amd_stb_handle_efr(filp);
>  
>  	/* Get the num_samples to calculate the last push location */
>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> @@ -209,28 +208,28 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> -static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> -					   loff_t *pos)
> +static ssize_t amd_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> +				       loff_t *pos)
>  {
> -	struct amd_pmc_stb_v2_data *data = filp->private_data;
> +	struct amd_stb_v2_data *data = filp->private_data;
>  
>  	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
>  }
>  
> -static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
>  {
>  	kfree(filp->private_data);
>  	return 0;
>  }
>  
> -static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
> +static const struct file_operations amd_stb_debugfs_fops_v2 = {
>  	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open_v2,
> -	.read = amd_pmc_stb_debugfs_read_v2,
> -	.release = amd_pmc_stb_debugfs_release_v2,
> +	.open = amd_stb_debugfs_open_v2,
> +	.read = amd_stb_debugfs_read_v2,
> +	.release = amd_stb_debugfs_release_v2,
>  };
>  
> -static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
> +static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>  {
>  	switch (dev->cpu_id) {
>  	case AMD_CPU_ID_YC:
> @@ -249,7 +248,7 @@ static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
>  	}
>  }
>  
> -int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
> +int amd_s2d_init(struct amd_pmc_dev *dev)

For consistency, amd_stb_s2d_init() ?

>  {
>  	u32 phys_addr_low, phys_addr_hi;
>  	u64 stb_phys_addr;
> @@ -259,12 +258,12 @@ int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	if (!enable_stb)
>  		return 0;
>  
> -	if (amd_pmc_is_stb_supported(dev))
> +	if (amd_is_stb_supported(dev))
>  		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -				    &amd_pmc_stb_debugfs_fops_v2);
> +				    &amd_stb_debugfs_fops_v2);
>  	else
>  		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -				    &amd_pmc_stb_debugfs_fops);
> +				    &amd_stb_debugfs_fops);

You might want to consider adding a patch to convert this to use ?: 
operator for the only arg that is changing so the entire call doesn't
need to be written twice nor is if/else needed.

-- 
 i.

>  
>  	/* Spill to DRAM feature uses separate SMU message port */
>  	dev->msg_port = 1;
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index a977ff1e52b5..8e7c87505327 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -667,7 +667,7 @@ static void amd_pmc_s2idle_prepare(void)
>  		return;
>  	}
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  }
> @@ -686,7 +686,7 @@ static void amd_pmc_s2idle_check(void)
>  	/* Dump the IdleMask before we add to the STB */
>  	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  }
> @@ -713,7 +713,7 @@ static void amd_pmc_s2idle_restore(void)
>  	/* Let SMU know that we are looking for stats */
>  	amd_pmc_dump_data(pdev);
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  
> @@ -828,7 +828,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	amd_pmc_dbgfs_register(dev);
> -	err = amd_pmc_s2d_init(dev);
> +	err = amd_s2d_init(dev);
>  	if (err)
>  		goto err_pci_dev_put;
>  
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index 07fcb13a4136..7e7f9170124c 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -75,8 +75,8 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>  #define AMD_S2D_REGISTER_RESPONSE	0xA80
>  #define AMD_S2D_REGISTER_ARGUMENT	0xA88
>  
> -int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
> -int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
> +int amd_s2d_init(struct amd_pmc_dev *dev);
> +int amd_write_stb(struct amd_pmc_dev *dev, u32 data);
>  int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>  
>  #endif /* PMC_H */
> 

Re: [PATCH v2 3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> To distinguish between the PMC message port and the S2D (Spill to DRAM)
> message port, replace the use of 0 and 1 with an enum.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 5efec020ecac..2b06861c479b 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -47,6 +47,11 @@ enum s2d_arg {
>  	S2D_DRAM_SIZE,
>  };
>  
> +enum s2d_msg_port {
> +	MSG_PORT_PMC,
> +	MSG_PORT_S2D,
> +};
> +
>  struct amd_stb_v2_data {
>  	size_t size;
>  	u8 data[] __counted_by(size);
> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
>  	/* Spill to DRAM num_samples uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>  	if (ret)
> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	/* Get the num_samples to calculate the last push location */
>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  	if (ret) {
>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>  		return ret;
> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>  				    &amd_stb_debugfs_fops);
>  
>  	/* Spill to DRAM feature uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  
>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>  	if (!dev->stb_virt_addr)

This change is incomplete, you missed all places using it:

if (dev->msg_port) { 

+ add helper for this:

dev->msg_port ? "S2D" : "PMC"

-- 
 i.

Re: [PATCH v2 5/8] platform/x86/amd/pmc: Update IP information structure for newer SoCs

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> The latest AMD processors include additional IP blocks that must be turned
> off before transitioning to low power. PMFW provides an interface to
> retrieve debug information from each IP block, which is useful for
> diagnosing issues if the system fails to enter or exit low power states,
> or for profiling which IP block takes more time. Add support for using
> this information within the driver.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index f9900a03391a..0bf4065153da 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -95,6 +95,35 @@ struct amd_pmc_bit_map {
>  	u32 bit_mask;
>  };
>  
> +static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
> +	{"DISPLAY",     BIT(0)},
> +	{"CPU",         BIT(1)},
> +	{"GFX",         BIT(2)},
> +	{"VDD",         BIT(3)},
> +	{"VDD_CCX",     BIT(4)},
> +	{"ACP",         BIT(5)},
> +	{"VCN_0",       BIT(6)},
> +	{"VCN_1",       BIT(7)},
> +	{"ISP",         BIT(8)},
> +	{"NBIO",        BIT(9)},
> +	{"DF",          BIT(10)},
> +	{"USB3_0",      BIT(11)},
> +	{"USB3_1",      BIT(12)},
> +	{"LAPIC",       BIT(13)},
> +	{"USB3_2",      BIT(14)},
> +	{"USB4_RT0",	BIT(15)},
> +	{"USB4_RT1",	BIT(16)},
> +	{"USB4_0",      BIT(17)},
> +	{"USB4_1",      BIT(18)},
> +	{"MPM",         BIT(19)},
> +	{"JPEG_0",      BIT(20)},
> +	{"JPEG_1",      BIT(21)},
> +	{"IPU",         BIT(22)},
> +	{"UMSCH",       BIT(23)},
> +	{"VPE",         BIT(24)},
> +	{}
> +};
> +
>  static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>  	{"DISPLAY",	BIT(0)},
>  	{"CPU",		BIT(1)},
> @@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>  		break;
>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> -		dev->num_ips = 22;
> +		if (boot_cpu_data.x86_model == 0x70)
> +			dev->num_ips = 25;
> +		else
> +			dev->num_ips = 22;

Could these use ARRAY_SIZE()? They're related to that array, aren't they?

>  		dev->smu_msg = 0x938;
>  		break;
>  	}
> @@ -338,9 +370,15 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
>  
>  	seq_puts(s, "\n=== Active time (in us) ===\n");
>  	for (idx = 0 ; idx < dev->num_ips ; idx++) {
> -		if (soc15_ip_blk[idx].bit_mask & dev->active_ips)
> +		if (dev->cpu_id == PCI_DEVICE_ID_AMD_1AH_M20H_ROOT &&
> +		    boot_cpu_data.x86_model == 0x70) {
> +			if (soc15_ip_blk_v2[idx].bit_mask & dev->active_ips)
> +				seq_printf(s, "%-8s : %lld\n", soc15_ip_blk_v2[idx].name,
> +					   table.timecondition_notmet_lastcapture[idx]);
> +		} else if (soc15_ip_blk[idx].bit_mask & dev->active_ips) {
>  			seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
>  				   table.timecondition_notmet_lastcapture[idx]);
> +		}

Why not have amd_pmc_get_ip_info() prepare a pointer into 'dev' to the 
relevant soc15_ip_blk_v2/soc15_ip_blk rather than trying to pick one here?

>  	}
>  
>  	return 0;
> 

-- 
 i.

Re: [PATCH v2 7/8] platform/x86/amd/pmc: Add STB support for AMD Desktop variants

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> Previously, AMD's Ryzen Desktop SoCs did not include support for STB.
> However, to accommodate this recent change, PMFW has implemented a new
> message port pair mechanism for handling messages, arguments, and
> responses, specifically designed for distinguishing from Mobile SoCs.
> Therefore, it is necessary to update the driver to properly handle this
> incoming change.
> 
> Add a new function amd_stb_update_args() to simply the arguments that
> needs to be passed between S2D supported Mobile SoCs vs Desktop SoCs.
> 
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 31 +++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 917c111b31c9..6a3cfcbb614e 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -36,6 +36,11 @@
>  #define AMD_S2D_REGISTER_RESPONSE	0xA80
>  #define AMD_S2D_REGISTER_ARGUMENT	0xA88
>  
> +/* STB S2D(Spill to DRAM) message port offset for 44h model */

Add space before (

I know there might be some older ones lying around with the incorrect 
spacing but lets not add any new ones.

> +#define AMD_GNR_REGISTER_MESSAGE	0x524
> +#define AMD_GNR_REGISTER_RESPONSE	0x570
> +#define AMD_GNR_REGISTER_ARGUMENT	0xA40
> +
>  static bool enable_stb;
>  module_param(enable_stb, bool, 0644);
>  MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> @@ -239,12 +244,31 @@ static const struct file_operations amd_stb_debugfs_fops_v2 = {
>  	.release = amd_stb_debugfs_release_v2,
>  };
>  
> +static void amd_stb_update_args(struct amd_pmc_dev *dev)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN5))
> +		switch (boot_cpu_data.x86_model) {
> +		case 0x44:
> +			dev->stb_arg.msg = AMD_GNR_REGISTER_MESSAGE;
> +			dev->stb_arg.arg = AMD_GNR_REGISTER_ARGUMENT;
> +			dev->stb_arg.resp = AMD_GNR_REGISTER_RESPONSE;
> +			return;

default branch missing.

> +	}

Eh? That's a weird indentation for sure. :-)

Both levels of this nested construct need braces, please.

-- 
 i.

> +
> +	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
> +	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
> +	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
> +}
> +
>  static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>  {
>  	switch (dev->cpu_id) {
>  	case AMD_CPU_ID_YC:
>  	case AMD_CPU_ID_CB:
> -		dev->stb_arg.s2d_msg_id = 0xBE;
> +		if (boot_cpu_data.x86_model == 0x44)
> +			dev->stb_arg.s2d_msg_id = 0x9B;
> +		else
> +			dev->stb_arg.s2d_msg_id = 0xBE;
>  		break;
>  	case AMD_CPU_ID_PS:
>  		dev->stb_arg.s2d_msg_id = 0x85;
> @@ -260,10 +284,7 @@ static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>  		return false;
>  	}
>  
> -	dev->stb_arg.msg = AMD_S2D_REGISTER_MESSAGE;
> -	dev->stb_arg.arg = AMD_S2D_REGISTER_ARGUMENT;
> -	dev->stb_arg.resp = AMD_S2D_REGISTER_RESPONSE;
> -
> +	amd_stb_update_args(dev);
>  	return true;
>  }
>  
> 

Re: [PATCH v2 3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

[Shyam Sundar S K]

On 11/1/2024 15:58, Ilpo Järvinen wrote:
> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> 
>> To distinguish between the PMC message port and the S2D (Spill to DRAM)
>> message port, replace the use of 0 and 1 with an enum.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> index 5efec020ecac..2b06861c479b 100644
>> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
>> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> @@ -47,6 +47,11 @@ enum s2d_arg {
>>  	S2D_DRAM_SIZE,
>>  };
>>  
>> +enum s2d_msg_port {
>> +	MSG_PORT_PMC,
>> +	MSG_PORT_S2D,
>> +};
>> +
>>  struct amd_stb_v2_data {
>>  	size_t size;
>>  	u8 data[] __counted_by(size);
>> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>>  
>>  	/* Spill to DRAM num_samples uses separate SMU message port */
>> -	dev->msg_port = 1;
>> +	dev->msg_port = MSG_PORT_S2D;
>>  
>>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>>  	if (ret)
>> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  	/* Get the num_samples to calculate the last push location */
>>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>>  	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> +	dev->msg_port = MSG_PORT_PMC;
>>  	if (ret) {
>>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>>  		return ret;
>> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>  				    &amd_stb_debugfs_fops);
>>  
>>  	/* Spill to DRAM feature uses separate SMU message port */
>> -	dev->msg_port = 1;
>> +	dev->msg_port = MSG_PORT_S2D;
>>  
>>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>  
>>  	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> +	dev->msg_port = MSG_PORT_PMC;
>>  
>>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>>  	if (!dev->stb_virt_addr)
> 
> This change is incomplete, you missed all places using it:
> 
> if (dev->msg_port) { 
> 
> + add helper for this:
> 
> dev->msg_port ? "S2D" : "PMC"
> 


I am not sure if I understand your comment fully. Can you please
elaborate?

Thanks,
Shyam

Re: [PATCH v2 1/8] platform/x86/amd/pmc: Move STB functionality to a new file for better code organization

[Shyam Sundar S K]

On 11/1/2024 15:45, Ilpo Järvinen wrote:
> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> 
>> As the SoC evolves with each generation, the dynamics between the PMC and
>> STB layers within the PMC driver are becoming increasingly complex, making
>> it challenging to manage both in a single file and maintain code
>> readability.
>>
>> Additionally, during silicon bringup, the PMC functionality is often
>> enabled first, with STB functionality added later. This can lead to missed
>> updates in the driver, potentially causing issues.
>>
>> To address these challenges, it's beneficial to move all STB-related
>> changes to a separate file. This approach will better accommodate newer
>> SoCs, provide improved flexibility for desktop variants, and facilitate
>> the collection of additional debug information through STB mechanisms.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmc/Makefile  |   2 +-
>>  drivers/platform/x86/amd/pmc/mp1_stb.c | 295 +++++++++++++++++++++++++
>>  drivers/platform/x86/amd/pmc/pmc.c     | 289 +-----------------------
>>  drivers/platform/x86/amd/pmc/pmc.h     |   9 +
>>  4 files changed, 310 insertions(+), 285 deletions(-)
>>  create mode 100644 drivers/platform/x86/amd/pmc/mp1_stb.c
>>
>> diff --git a/drivers/platform/x86/amd/pmc/Makefile b/drivers/platform/x86/amd/pmc/Makefile
>> index f1d9ab19d24c..255d94ddf999 100644
>> --- a/drivers/platform/x86/amd/pmc/Makefile
>> +++ b/drivers/platform/x86/amd/pmc/Makefile
>> @@ -4,6 +4,6 @@
>>  # AMD Power Management Controller Driver
>>  #
>>  
>> -amd-pmc-objs := pmc.o pmc-quirks.o
>> +amd-pmc-objs := pmc.o pmc-quirks.o mp1_stb.o
>>  obj-$(CONFIG_AMD_PMC) += amd-pmc.o
>>  amd-pmc-$(CONFIG_AMD_MP2_STB) += mp2_stb.o
>> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> new file mode 100644
>> index 000000000000..9a34dd94982c
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> @@ -0,0 +1,295 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * AMD MP1 Smart Trace Buffer (STB) Layer
>> + *
>> + * Copyright (c) 2024, Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> + *          Sanket Goswami <Sanket.Goswami@amd.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <asm/amd_nb.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include "pmc.h"
>> +
>> +/* STB Spill to DRAM Parameters */
>> +#define S2D_TELEMETRY_BYTES_MAX		0x100000U
>> +#define S2D_RSVD_RAM_SPACE		0x100000
>> +#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
>> +
>> +/* STB Registers */
>> +#define AMD_PMC_STB_PMI_0	0x03E30600
> 
> Is this still needed in pmc.c? I think all users moved to this file.
> 
>> +#define AMD_PMC_STB_DUMMY_PC	0xC6000007
>> +
>> +/* STB Spill to DRAM Message Definition */
>> +#define STB_FORCE_FLUSH_DATA	0xCF
>> +#define FIFO_SIZE		4096
>> +
>> +static bool enable_stb;
>> +module_param(enable_stb, bool, 0644);
>> +MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
>> +
>> +static bool dump_custom_stb;
>> +module_param(dump_custom_stb, bool, 0644);
>> +MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
>> +
>> +enum s2d_arg {
>> +	S2D_TELEMETRY_SIZE = 0x01,
>> +	S2D_PHYS_ADDR_LOW,
>> +	S2D_PHYS_ADDR_HIGH,
>> +	S2D_NUM_SAMPLES,
>> +	S2D_DRAM_SIZE,
>> +};
>> +
>> +struct amd_pmc_stb_v2_data {
>> +	size_t size;
>> +	u8 data[] __counted_by(size);
>> +};
>> +
>> +int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
>> +{
>> +	int err;
>> +
>> +	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
>> +	if (err) {
>> +		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
>> +		return pcibios_err_to_errno(err);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
>> +{
>> +	int i, err;
>> +
>> +	for (i = 0; i < FIFO_SIZE; i++) {
>> +		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
>> +		if (err) {
>> +			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
>> +				AMD_PMC_STB_PMI_0);
>> +			return pcibios_err_to_errno(err);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>> +{
>> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> +	u32 size = FIFO_SIZE * sizeof(u32);
>> +	u32 *buf;
>> +	int rc;
>> +
>> +	buf = kzalloc(size, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	rc = amd_pmc_read_stb(dev, buf);
>> +	if (rc) {
>> +		kfree(buf);
>> +		return rc;
>> +	}
>> +
>> +	filp->private_data = buf;
>> +	return rc;
>> +}
>> +
>> +static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
>> +					loff_t *pos)
>> +{
>> +	if (!filp->private_data)
>> +		return -EINVAL;
>> +
>> +	return simple_read_from_buffer(buf, size, pos, filp->private_data,
>> +				       FIFO_SIZE * sizeof(u32));
>> +}
>> +
>> +static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
>> +{
>> +	kfree(filp->private_data);
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations amd_pmc_stb_debugfs_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = amd_pmc_stb_debugfs_open,
>> +	.read = amd_pmc_stb_debugfs_read,
>> +	.release = amd_pmc_stb_debugfs_release,
>> +};
>> +
>> +/* Enhanced STB Firmware Reporting Mechanism */
>> +static int amd_pmc_stb_handle_efr(struct file *filp)
>> +{
>> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> +	struct amd_pmc_stb_v2_data *stb_data_arr;
>> +	u32 fsize;
>> +
>> +	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
>> +	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
>> +	if (!stb_data_arr)
>> +		return -ENOMEM;
>> +
>> +	stb_data_arr->size = fsize;
>> +	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
>> +	filp->private_data = stb_data_arr;
>> +
>> +	return 0;
>> +}
>> +
>> +static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>> +{
>> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> +	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
>> +	struct amd_pmc_stb_v2_data *stb_data_arr;
>> +	int ret;
>> +
>> +	/* Write dummy postcode while reading the STB buffer */
>> +	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
>> +	if (ret)
>> +		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>> +
>> +	/* Spill to DRAM num_samples uses separate SMU message port */
>> +	dev->msg_port = 1;
>> +
>> +	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>> +	if (ret)
>> +		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
>> +
>> +	/*
>> +	 * We have a custom stb size and the PMFW is supposed to give
>> +	 * the enhanced dram size. Note that we land here only for the
>> +	 * platforms that support enhanced dram size reporting.
>> +	 */
>> +	if (dump_custom_stb)
>> +		return amd_pmc_stb_handle_efr(filp);
>> +
>> +	/* Get the num_samples to calculate the last push location */
>> +	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>> +	/* Clear msg_port for other SMU operation */
>> +	dev->msg_port = 0;
>> +	if (ret) {
>> +		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
>> +	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
>> +	if (!stb_data_arr)
>> +		return -ENOMEM;
>> +
>> +	stb_data_arr->size = fsize;
>> +
>> +	/*
>> +	 * Start capturing data from the last push location.
>> +	 * This is for general cases, where the stb limits
>> +	 * are meant for standard usage.
>> +	 */
>> +	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
>> +		/* First read oldest data starting 1 behind last write till end of ringbuffer */
>> +		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
>> +		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
>> +
>> +		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
>> +		/* Second copy the newer samples from offset 0 - last write */
>> +		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
>> +	} else {
>> +		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
>> +	}
>> +
>> +	filp->private_data = stb_data_arr;
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
>> +					   loff_t *pos)
>> +{
>> +	struct amd_pmc_stb_v2_data *data = filp->private_data;
>> +
>> +	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
>> +}
>> +
>> +static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
>> +{
>> +	kfree(filp->private_data);
>> +	return 0;
>> +}
>> +
>> +static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
>> +	.owner = THIS_MODULE,
>> +	.open = amd_pmc_stb_debugfs_open_v2,
>> +	.read = amd_pmc_stb_debugfs_read_v2,
>> +	.release = amd_pmc_stb_debugfs_release_v2,
>> +};
>> +
>> +static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
>> +{
>> +	switch (dev->cpu_id) {
>> +	case AMD_CPU_ID_YC:
>> +	case AMD_CPU_ID_CB:
>> +		dev->s2d_msg_id = 0xBE;
>> +		return true;
>> +	case AMD_CPU_ID_PS:
>> +		dev->s2d_msg_id = 0x85;
>> +		return true;
>> +	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>> +		dev->s2d_msg_id = 0xDE;
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>> +{
>> +	u32 phys_addr_low, phys_addr_hi;
>> +	u64 stb_phys_addr;
>> +	u32 size = 0;
>> +	int ret;
>> +
>> +	if (!enable_stb)
>> +		return 0;
>> +
>> +	if (amd_pmc_is_stb_supported(dev))
>> +		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>> +				    &amd_pmc_stb_debugfs_fops_v2);
>> +	else
>> +		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>> +				    &amd_pmc_stb_debugfs_fops);
>> +
>> +	/* Spill to DRAM feature uses separate SMU message port */
>> +	dev->msg_port = 1;
>> +
>> +	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>> +	if (size != S2D_TELEMETRY_BYTES_MAX)
>> +		return -EIO;
>> +
>> +	/* Get DRAM size */
>> +	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>> +	if (ret || !dev->dram_size)
>> +		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
>> +
>> +	/* Get STB DRAM address */
>> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>> +
>> +	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>> +
>> +	/* Clear msg_port for other SMU operation */
>> +	dev->msg_port = 0;
>> +
>> +	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>> +	if (!dev->stb_virt_addr)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index bbb8edb62e00..a977ff1e52b5 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -44,20 +44,6 @@
>>  #define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
>>  #define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
>>  #define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
>> -#define AMD_PMC_STB_DUMMY_PC		0xC6000007
>> -
>> -/* STB S2D(Spill to DRAM) has different message port offset */
>> -#define AMD_S2D_REGISTER_MESSAGE	0xA20
>> -#define AMD_S2D_REGISTER_RESPONSE	0xA80
>> -#define AMD_S2D_REGISTER_ARGUMENT	0xA88
>> -
>> -/* STB Spill to DRAM Parameters */
>> -#define S2D_TELEMETRY_BYTES_MAX		0x100000U
>> -#define S2D_RSVD_RAM_SPACE		0x100000
>> -#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
>> -
>> -/* STB Spill to DRAM Message Definition */
>> -#define STB_FORCE_FLUSH_DATA		0xCF
>>  
>>  /* Base address of SMU for mapping physical address to virtual address */
>>  #define AMD_PMC_MAPPING_SIZE		0x01000
>> @@ -97,7 +83,6 @@
>>  
>>  #define DELAY_MIN_US		2000
>>  #define DELAY_MAX_US		3000
>> -#define FIFO_SIZE		4096
>>  
>>  enum amd_pmc_def {
>>  	MSG_TEST = 0x01,
>> @@ -105,19 +90,6 @@ enum amd_pmc_def {
>>  	MSG_OS_HINT_RN,
>>  };
>>  
>> -enum s2d_arg {
>> -	S2D_TELEMETRY_SIZE = 0x01,
>> -	S2D_PHYS_ADDR_LOW,
>> -	S2D_PHYS_ADDR_HIGH,
>> -	S2D_NUM_SAMPLES,
>> -	S2D_DRAM_SIZE,
>> -};
>> -
>> -struct amd_pmc_stb_v2_data {
>> -	size_t size;
>> -	u8 data[] __counted_by(size);
>> -};
>> -
>>  struct amd_pmc_bit_map {
>>  	const char *name;
>>  	u32 bit_mask;
>> @@ -149,22 +121,11 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>>  	{}
>>  };
>>  
>> -static bool enable_stb;
>> -module_param(enable_stb, bool, 0644);
>> -MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
>> -
>>  static bool disable_workarounds;
>>  module_param(disable_workarounds, bool, 0644);
>>  MODULE_PARM_DESC(disable_workarounds, "Disable workarounds for platform bugs");
>>  
>> -static bool dump_custom_stb;
>> -module_param(dump_custom_stb, bool, 0644);
>> -MODULE_PARM_DESC(dump_custom_stb, "Enable to dump full STB buffer");
>> -
>>  static struct amd_pmc_dev pmc;
>> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
>> -static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
>>  
>>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>>  {
>> @@ -193,155 +154,6 @@ struct smu_metrics {
>>  	u64 timecondition_notmet_totaltime[32];
>>  } __packed;
>>  
>> -static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>> -{
>> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> -	u32 size = FIFO_SIZE * sizeof(u32);
>> -	u32 *buf;
>> -	int rc;
>> -
>> -	buf = kzalloc(size, GFP_KERNEL);
>> -	if (!buf)
>> -		return -ENOMEM;
>> -
>> -	rc = amd_pmc_read_stb(dev, buf);
>> -	if (rc) {
>> -		kfree(buf);
>> -		return rc;
>> -	}
>> -
>> -	filp->private_data = buf;
>> -	return rc;
>> -}
>> -
>> -static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
>> -					loff_t *pos)
>> -{
>> -	if (!filp->private_data)
>> -		return -EINVAL;
>> -
>> -	return simple_read_from_buffer(buf, size, pos, filp->private_data,
>> -				       FIFO_SIZE * sizeof(u32));
>> -}
>> -
>> -static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
>> -{
>> -	kfree(filp->private_data);
>> -	return 0;
>> -}
>> -
>> -static const struct file_operations amd_pmc_stb_debugfs_fops = {
>> -	.owner = THIS_MODULE,
>> -	.open = amd_pmc_stb_debugfs_open,
>> -	.read = amd_pmc_stb_debugfs_read,
>> -	.release = amd_pmc_stb_debugfs_release,
>> -};
>> -
>> -/* Enhanced STB Firmware Reporting Mechanism */
>> -static int amd_pmc_stb_handle_efr(struct file *filp)
>> -{
>> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> -	struct amd_pmc_stb_v2_data *stb_data_arr;
>> -	u32 fsize;
>> -
>> -	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
>> -	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
>> -	if (!stb_data_arr)
>> -		return -ENOMEM;
>> -
>> -	stb_data_arr->size = fsize;
>> -	memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
>> -	filp->private_data = stb_data_arr;
>> -
>> -	return 0;
>> -}
>> -
>> -static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>> -{
>> -	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>> -	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
>> -	struct amd_pmc_stb_v2_data *stb_data_arr;
>> -	int ret;
>> -
>> -	/* Write dummy postcode while reading the STB buffer */
>> -	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
>> -	if (ret)
>> -		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>> -
>> -	/* Spill to DRAM num_samples uses separate SMU message port */
>> -	dev->msg_port = 1;
>> -
>> -	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>> -	if (ret)
>> -		dev_dbg_once(dev->dev, "S2D force flush not supported: %d\n", ret);
>> -
>> -	/*
>> -	 * We have a custom stb size and the PMFW is supposed to give
>> -	 * the enhanced dram size. Note that we land here only for the
>> -	 * platforms that support enhanced dram size reporting.
>> -	 */
>> -	if (dump_custom_stb)
>> -		return amd_pmc_stb_handle_efr(filp);
>> -
>> -	/* Get the num_samples to calculate the last push location */
>> -	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>> -	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> -	if (ret) {
>> -		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	fsize = min(num_samples, S2D_TELEMETRY_BYTES_MAX);
>> -	stb_data_arr = kmalloc(struct_size(stb_data_arr, data, fsize), GFP_KERNEL);
>> -	if (!stb_data_arr)
>> -		return -ENOMEM;
>> -
>> -	stb_data_arr->size = fsize;
>> -
>> -	/*
>> -	 * Start capturing data from the last push location.
>> -	 * This is for general cases, where the stb limits
>> -	 * are meant for standard usage.
>> -	 */
>> -	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
>> -		/* First read oldest data starting 1 behind last write till end of ringbuffer */
>> -		stb_rdptr_offset = num_samples % S2D_TELEMETRY_BYTES_MAX;
>> -		fsize = S2D_TELEMETRY_BYTES_MAX - stb_rdptr_offset;
>> -
>> -		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
>> -		/* Second copy the newer samples from offset 0 - last write */
>> -		memcpy_fromio(stb_data_arr->data + fsize, dev->stb_virt_addr, stb_rdptr_offset);
>> -	} else {
>> -		memcpy_fromio(stb_data_arr->data, dev->stb_virt_addr, fsize);
>> -	}
>> -
>> -	filp->private_data = stb_data_arr;
>> -
>> -	return 0;
>> -}
>> -
>> -static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
>> -					   loff_t *pos)
>> -{
>> -	struct amd_pmc_stb_v2_data *data = filp->private_data;
>> -
>> -	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
>> -}
>> -
>> -static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
>> -{
>> -	kfree(filp->private_data);
>> -	return 0;
>> -}
>> -
>> -static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
>> -	.owner = THIS_MODULE,
>> -	.open = amd_pmc_stb_debugfs_open_v2,
>> -	.read = amd_pmc_stb_debugfs_read_v2,
>> -	.release = amd_pmc_stb_debugfs_release_v2,
>> -};
>> -
>>  static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>>  {
>>  	switch (dev->cpu_id) {
>> @@ -350,18 +162,15 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>>  	case AMD_CPU_ID_YC:
>>  	case AMD_CPU_ID_CB:
>>  		dev->num_ips = 12;
>> -		dev->s2d_msg_id = 0xBE;
>>  		dev->smu_msg = 0x538;
>>  		break;
>>  	case AMD_CPU_ID_PS:
>>  		dev->num_ips = 21;
>> -		dev->s2d_msg_id = 0x85;
>>  		dev->smu_msg = 0x538;
>>  		break;
>>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>>  		dev->num_ips = 22;
>> -		dev->s2d_msg_id = 0xDE;
>>  		dev->smu_msg = 0x938;
>>  		break;
>>  	}
>> @@ -625,20 +434,6 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>  }
>>  
>> -static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
>> -{
>> -	switch (dev->cpu_id) {
>> -	case AMD_CPU_ID_YC:
>> -	case AMD_CPU_ID_CB:
>> -	case AMD_CPU_ID_PS:
>> -	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>> -	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>> -		return true;
>> -	default:
>> -		return false;
>> -	}
>> -}
>> -
>>  static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>>  {
>>  	dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL);
>> @@ -648,15 +443,6 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>>  			    &s0ix_stats_fops);
>>  	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
>>  			    &amd_pmc_idlemask_fops);
>> -	/* Enable STB only when the module_param is set */
>> -	if (enable_stb) {
>> -		if (amd_pmc_is_stb_supported(dev))
>> -			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>> -					    &amd_pmc_stb_debugfs_fops_v2);
>> -		else
>> -			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>> -					    &amd_pmc_stb_debugfs_fops);
>> -	}
> 
> Is it related to the logic change I ask about down below? It looks 
> something that really ought to be done in a separate preparatory patch if 
> that's the case since it seems entire independent of moving things to 
> another file this patch is supposed to be all about.
> 

There are tight deps if I have make a prep patch out of it. However I
ended up making this into two, you can take a look at v2 (but before
that I have some follow-ups, which are in the respective patches).

>>  }
>>  
>>  static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>> @@ -683,7 +469,7 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>>  	dev_dbg(dev->dev, "AMD_%s_REGISTER_MESSAGE:%x\n", dev->msg_port ? "S2D" : "PMC", value);
>>  }
>>  
>> -static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
>> +int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
>>  {
>>  	int rc;
>>  	u32 val, message, argument, response;
>> @@ -975,69 +761,6 @@ static const struct pci_device_id pmc_pci_ids[] = {
>>  	{ }
>>  };
>>  
>> -static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>> -{
>> -	u32 phys_addr_low, phys_addr_hi;
>> -	u64 stb_phys_addr;
>> -	u32 size = 0;
>> -	int ret;
>> -
>> -	/* Spill to DRAM feature uses separate SMU message port */
>> -	dev->msg_port = 1;
>> -
>> -	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>> -	if (size != S2D_TELEMETRY_BYTES_MAX)
>> -		return -EIO;
>> -
>> -	/* Get DRAM size */
>> -	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>> -	if (ret || !dev->dram_size)
>> -		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
>> -
>> -	/* Get STB DRAM address */
>> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, dev->s2d_msg_id, true);
>> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, dev->s2d_msg_id, true);
>> -
>> -	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>> -
>> -	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> -
>> -	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>> -	if (!dev->stb_virt_addr)
>> -		return -ENOMEM;
>> -
>> -	return 0;
>> -}
>> -
>> -static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
>> -{
>> -	int err;
>> -
>> -	err = amd_smn_write(0, AMD_PMC_STB_PMI_0, data);
>> -	if (err) {
>> -		dev_err(dev->dev, "failed to write data in stb: 0x%X\n", AMD_PMC_STB_PMI_0);
>> -		return pcibios_err_to_errno(err);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
>> -{
>> -	int i, err;
>> -
>> -	for (i = 0; i < FIFO_SIZE; i++) {
>> -		err = amd_smn_read(0, AMD_PMC_STB_PMI_0, buf++);
>> -		if (err) {
>> -			dev_err(dev->dev, "error reading data from stb: 0x%X\n", AMD_PMC_STB_PMI_0);
>> -			return pcibios_err_to_errno(err);
>> -		}
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static int amd_pmc_probe(struct platform_device *pdev)
>>  {
>>  	struct amd_pmc_dev *dev = &pmc;
>> @@ -1095,12 +818,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>  	/* Get num of IP blocks within the SoC */
>>  	amd_pmc_get_ip_info(dev);
>>  
>> -	if (enable_stb && amd_pmc_is_stb_supported(dev)) {
>> -		err = amd_pmc_s2d_init(dev);
>> -		if (err)
>> -			goto err_pci_dev_put;
>> -	}
>> -
>>  	platform_set_drvdata(pdev, dev);
>>  	if (IS_ENABLED(CONFIG_SUSPEND)) {
>>  		err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
>> @@ -1111,6 +828,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	amd_pmc_dbgfs_register(dev);
>> +	err = amd_pmc_s2d_init(dev);
>> +	if (err)
>> +		goto err_pci_dev_put;
> 
> Why isn't this logic change in a separate patch?
> 

makes sense. This movement of amd_pmc_s2d_init() after the debugfs
creation is required without that the debugfs root directories for the
driver would have not created.

Thanks,
Shyam

>> +
>>  	if (IS_ENABLED(CONFIG_AMD_MP2_STB))
>>  		amd_mp2_stb_init(dev);
>>  	pm_report_max_hw_sleep(U64_MAX);
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
>> index f1166d15c856..07fcb13a4136 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.h
>> +++ b/drivers/platform/x86/amd/pmc/pmc.h
>> @@ -70,4 +70,13 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>>  #define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
>>  #define PCI_DEVICE_ID_AMD_MP2_STB	0x172c
>>  
>> +/* STB S2D(Spill to DRAM) has different message port offset */
>> +#define AMD_S2D_REGISTER_MESSAGE	0xA20
>> +#define AMD_S2D_REGISTER_RESPONSE	0xA80
>> +#define AMD_S2D_REGISTER_ARGUMENT	0xA88
>> +
>> +int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
>> +int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
>> +int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>> +
>>  #endif /* PMC_H */
>>
> 

Re: [PATCH v2 5/8] platform/x86/amd/pmc: Update IP information structure for newer SoCs

[Shyam Sundar S K]

On 11/1/2024 17:34, Ilpo Järvinen wrote:
> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> 
>> The latest AMD processors include additional IP blocks that must be turned
>> off before transitioning to low power. PMFW provides an interface to
>> retrieve debug information from each IP block, which is useful for
>> diagnosing issues if the system fails to enter or exit low power states,
>> or for profiling which IP block takes more time. Add support for using
>> this information within the driver.
>>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index f9900a03391a..0bf4065153da 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -95,6 +95,35 @@ struct amd_pmc_bit_map {
>>  	u32 bit_mask;
>>  };
>>  
>> +static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
>> +	{"DISPLAY",     BIT(0)},
>> +	{"CPU",         BIT(1)},
>> +	{"GFX",         BIT(2)},
>> +	{"VDD",         BIT(3)},
>> +	{"VDD_CCX",     BIT(4)},
>> +	{"ACP",         BIT(5)},
>> +	{"VCN_0",       BIT(6)},
>> +	{"VCN_1",       BIT(7)},
>> +	{"ISP",         BIT(8)},
>> +	{"NBIO",        BIT(9)},
>> +	{"DF",          BIT(10)},
>> +	{"USB3_0",      BIT(11)},
>> +	{"USB3_1",      BIT(12)},
>> +	{"LAPIC",       BIT(13)},
>> +	{"USB3_2",      BIT(14)},
>> +	{"USB4_RT0",	BIT(15)},
>> +	{"USB4_RT1",	BIT(16)},
>> +	{"USB4_0",      BIT(17)},
>> +	{"USB4_1",      BIT(18)},
>> +	{"MPM",         BIT(19)},
>> +	{"JPEG_0",      BIT(20)},
>> +	{"JPEG_1",      BIT(21)},
>> +	{"IPU",         BIT(22)},
>> +	{"UMSCH",       BIT(23)},
>> +	{"VPE",         BIT(24)},
>> +	{}
>> +};
>> +
>>  static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>>  	{"DISPLAY",	BIT(0)},
>>  	{"CPU",		BIT(1)},
>> @@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>>  		break;
>>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>> -		dev->num_ips = 22;
>> +		if (boot_cpu_data.x86_model == 0x70)
>> +			dev->num_ips = 25;
>> +		else
>> +			dev->num_ips = 22;
> 
> Could these use ARRAY_SIZE()? They're related to that array, aren't they?

Yes, they are. ARRAY_SIZE() does return the number of elements in the
array but there is a catch,

both soc15_ip_blk[] and soc15_ip_blk_v2[] have a last empty element,
so when ARRAY_SIZE() is used we end up getting n+1 element (i.e along
with the last empty element). So, what would you advise?

1) remove the last empty element in the array? i.e. something like this?

static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
	{"DISPLAY",     BIT(0)},
	...
	{"VPE",         BIT(24)},
-   {} /* remove this */
};

2) or just do, `ARRAY_SIZE() - 1` so that we don't need to touch the
existing soc15_ip_blk[] and soc15_ip_blk_v2[].


I am inline with your comments on the other patches, will address them
in v3.

Thanks,
Shyam

> 
>>  		dev->smu_msg = 0x938;
>>  		break;
>>  	}
>> @@ -338,9 +370,15 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
>>  
>>  	seq_puts(s, "\n=== Active time (in us) ===\n");
>>  	for (idx = 0 ; idx < dev->num_ips ; idx++) {
>> -		if (soc15_ip_blk[idx].bit_mask & dev->active_ips)
>> +		if (dev->cpu_id == PCI_DEVICE_ID_AMD_1AH_M20H_ROOT &&
>> +		    boot_cpu_data.x86_model == 0x70) {
>> +			if (soc15_ip_blk_v2[idx].bit_mask & dev->active_ips)
>> +				seq_printf(s, "%-8s : %lld\n", soc15_ip_blk_v2[idx].name,
>> +					   table.timecondition_notmet_lastcapture[idx]);
>> +		} else if (soc15_ip_blk[idx].bit_mask & dev->active_ips) {
>>  			seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
>>  				   table.timecondition_notmet_lastcapture[idx]);
>> +		}
> 
> Why not have amd_pmc_get_ip_info() prepare a pointer into 'dev' to the 
> relevant soc15_ip_blk_v2/soc15_ip_blk rather than trying to pick one here?
> 

Makes sense. Ack.

>>  	}
>>  
>>  	return 0;
>>
> 

Re: [PATCH v2 3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

[Ilpo Järvinen]

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

On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
> On 11/1/2024 15:58, Ilpo Järvinen wrote:
> > On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> > 
> >> To distinguish between the PMC message port and the S2D (Spill to DRAM)
> >> message port, replace the use of 0 and 1 with an enum.
> >>
> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> index 5efec020ecac..2b06861c479b 100644
> >> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> @@ -47,6 +47,11 @@ enum s2d_arg {
> >>  	S2D_DRAM_SIZE,
> >>  };
> >>  
> >> +enum s2d_msg_port {
> >> +	MSG_PORT_PMC,
> >> +	MSG_PORT_S2D,
> >> +};
> >> +
> >>  struct amd_stb_v2_data {
> >>  	size_t size;
> >>  	u8 data[] __counted_by(size);
> >> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> >>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
> >>  
> >>  	/* Spill to DRAM num_samples uses separate SMU message port */
> >> -	dev->msg_port = 1;
> >> +	dev->msg_port = MSG_PORT_S2D;
> >>  
> >>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
> >>  	if (ret)
> >> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> >>  	/* Get the num_samples to calculate the last push location */
> >>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> >>  	/* Clear msg_port for other SMU operation */
> >> -	dev->msg_port = 0;
> >> +	dev->msg_port = MSG_PORT_PMC;
> >>  	if (ret) {
> >>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> >>  		return ret;
> >> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
> >>  				    &amd_stb_debugfs_fops);
> >>  
> >>  	/* Spill to DRAM feature uses separate SMU message port */
> >> -	dev->msg_port = 1;
> >> +	dev->msg_port = MSG_PORT_S2D;
> >>  
> >>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
> >>  	if (size != S2D_TELEMETRY_BYTES_MAX)
> >> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
> >>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> >>  
> >>  	/* Clear msg_port for other SMU operation */
> >> -	dev->msg_port = 0;
> >> +	dev->msg_port = MSG_PORT_PMC;
> >>  
> >>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
> >>  	if (!dev->stb_virt_addr)
> > 
> > This change is incomplete, you missed all places using it:
> > 
> > if (dev->msg_port) { 
> > 
> > + add helper for this:
> > 
> > dev->msg_port ? "S2D" : "PMC"
> > 
> 
> 
> I am not sure if I understand your comment fully. Can you please
> elaborate?

There are users of dev->msg_port that should be also touched by this 
change but weren't.

For the printing, I suggested a helper function which returns the correct 
string so you don't need to do the compare within print argument.

-- 
 i.

Re: [PATCH v2 5/8] platform/x86/amd/pmc: Update IP information structure for newer SoCs

[Ilpo Järvinen]

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

On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
> On 11/1/2024 17:34, Ilpo Järvinen wrote:
> > On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> > 
> >> The latest AMD processors include additional IP blocks that must be turned
> >> off before transitioning to low power. PMFW provides an interface to
> >> retrieve debug information from each IP block, which is useful for
> >> diagnosing issues if the system fails to enter or exit low power states,
> >> or for profiling which IP block takes more time. Add support for using
> >> this information within the driver.
> >>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
> >>  1 file changed, 40 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> >> index f9900a03391a..0bf4065153da 100644
> >> --- a/drivers/platform/x86/amd/pmc/pmc.c
> >> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> >> @@ -95,6 +95,35 @@ struct amd_pmc_bit_map {
> >>  	u32 bit_mask;
> >>  };
> >>  
> >> +static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
> >> +	{"DISPLAY",     BIT(0)},
> >> +	{"CPU",         BIT(1)},
> >> +	{"GFX",         BIT(2)},
> >> +	{"VDD",         BIT(3)},
> >> +	{"VDD_CCX",     BIT(4)},
> >> +	{"ACP",         BIT(5)},
> >> +	{"VCN_0",       BIT(6)},
> >> +	{"VCN_1",       BIT(7)},
> >> +	{"ISP",         BIT(8)},
> >> +	{"NBIO",        BIT(9)},
> >> +	{"DF",          BIT(10)},
> >> +	{"USB3_0",      BIT(11)},
> >> +	{"USB3_1",      BIT(12)},
> >> +	{"LAPIC",       BIT(13)},
> >> +	{"USB3_2",      BIT(14)},
> >> +	{"USB4_RT0",	BIT(15)},
> >> +	{"USB4_RT1",	BIT(16)},
> >> +	{"USB4_0",      BIT(17)},
> >> +	{"USB4_1",      BIT(18)},
> >> +	{"MPM",         BIT(19)},
> >> +	{"JPEG_0",      BIT(20)},
> >> +	{"JPEG_1",      BIT(21)},
> >> +	{"IPU",         BIT(22)},
> >> +	{"UMSCH",       BIT(23)},
> >> +	{"VPE",         BIT(24)},
> >> +	{}
> >> +};
> >> +
> >>  static const struct amd_pmc_bit_map soc15_ip_blk[] = {
> >>  	{"DISPLAY",	BIT(0)},
> >>  	{"CPU",		BIT(1)},
> >> @@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> >>  		break;
> >>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> >>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> >> -		dev->num_ips = 22;
> >> +		if (boot_cpu_data.x86_model == 0x70)
> >> +			dev->num_ips = 25;
> >> +		else
> >> +			dev->num_ips = 22;
> > 
> > Could these use ARRAY_SIZE()? They're related to that array, aren't they?
> 
> Yes, they are. ARRAY_SIZE() does return the number of elements in the
> array but there is a catch,
> 
> both soc15_ip_blk[] and soc15_ip_blk_v2[] have a last empty element,
> so when ARRAY_SIZE() is used we end up getting n+1 element (i.e along
> with the last empty element). So, what would you advise?
>
> 1) remove the last empty element in the array? i.e. something like this?
> 
> static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
> 	{"DISPLAY",     BIT(0)},
> 	...
> 	{"VPE",         BIT(24)},
> -   {} /* remove this */
> };
> 
> 2) or just do, `ARRAY_SIZE() - 1` so that we don't need to touch the
> existing soc15_ip_blk[] and soc15_ip_blk_v2[].

The iteration seems to always ->num_ips based so the terminator seems 
superfluous. The cases with smaller num_ips cannot use terminator anyway 
as they share the larger array.

I propose you make a separate change out removing the terminator and 
migrating to use ARRAY_SIZE() in the existing code, don't forget to add 
the #include for it. Then add just this new thing in this patch.

-- 
 i.


> 
> 
> I am inline with your comments on the other patches, will address them
> in v3.
> 
> Thanks,
> Shyam
> 
> > 
> >>  		dev->smu_msg = 0x938;
> >>  		break;
> >>  	}
> >> @@ -338,9 +370,15 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
> >>  
> >>  	seq_puts(s, "\n=== Active time (in us) ===\n");
> >>  	for (idx = 0 ; idx < dev->num_ips ; idx++) {
> >> -		if (soc15_ip_blk[idx].bit_mask & dev->active_ips)
> >> +		if (dev->cpu_id == PCI_DEVICE_ID_AMD_1AH_M20H_ROOT &&
> >> +		    boot_cpu_data.x86_model == 0x70) {
> >> +			if (soc15_ip_blk_v2[idx].bit_mask & dev->active_ips)
> >> +				seq_printf(s, "%-8s : %lld\n", soc15_ip_blk_v2[idx].name,
> >> +					   table.timecondition_notmet_lastcapture[idx]);
> >> +		} else if (soc15_ip_blk[idx].bit_mask & dev->active_ips) {
> >>  			seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
> >>  				   table.timecondition_notmet_lastcapture[idx]);
> >> +		}
> > 
> > Why not have amd_pmc_get_ip_info() prepare a pointer into 'dev' to the 
> > relevant soc15_ip_blk_v2/soc15_ip_blk rather than trying to pick one here?
> > 
> 
> Makes sense. Ack.
> 
> >>  	}
> >>  
> >>  	return 0;
> >>
> > 
> 

Re: [PATCH v2 5/8] platform/x86/amd/pmc: Update IP information structure for newer SoCs

[Shyam Sundar S K]

On 11/5/2024 15:29, Ilpo Järvinen wrote:
> On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
>> On 11/1/2024 17:34, Ilpo Järvinen wrote:
>>> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
>>>
>>>> The latest AMD processors include additional IP blocks that must be turned
>>>> off before transitioning to low power. PMFW provides an interface to
>>>> retrieve debug information from each IP block, which is useful for
>>>> diagnosing issues if the system fails to enter or exit low power states,
>>>> or for profiling which IP block takes more time. Add support for using
>>>> this information within the driver.
>>>>
>>>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>  drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>>>> index f9900a03391a..0bf4065153da 100644
>>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>>> @@ -95,6 +95,35 @@ struct amd_pmc_bit_map {
>>>>  	u32 bit_mask;
>>>>  };
>>>>  
>>>> +static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
>>>> +	{"DISPLAY",     BIT(0)},
>>>> +	{"CPU",         BIT(1)},
>>>> +	{"GFX",         BIT(2)},
>>>> +	{"VDD",         BIT(3)},
>>>> +	{"VDD_CCX",     BIT(4)},
>>>> +	{"ACP",         BIT(5)},
>>>> +	{"VCN_0",       BIT(6)},
>>>> +	{"VCN_1",       BIT(7)},
>>>> +	{"ISP",         BIT(8)},
>>>> +	{"NBIO",        BIT(9)},
>>>> +	{"DF",          BIT(10)},
>>>> +	{"USB3_0",      BIT(11)},
>>>> +	{"USB3_1",      BIT(12)},
>>>> +	{"LAPIC",       BIT(13)},
>>>> +	{"USB3_2",      BIT(14)},
>>>> +	{"USB4_RT0",	BIT(15)},
>>>> +	{"USB4_RT1",	BIT(16)},
>>>> +	{"USB4_0",      BIT(17)},
>>>> +	{"USB4_1",      BIT(18)},
>>>> +	{"MPM",         BIT(19)},
>>>> +	{"JPEG_0",      BIT(20)},
>>>> +	{"JPEG_1",      BIT(21)},
>>>> +	{"IPU",         BIT(22)},
>>>> +	{"UMSCH",       BIT(23)},
>>>> +	{"VPE",         BIT(24)},
>>>> +	{}
>>>> +};
>>>> +
>>>>  static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>>>>  	{"DISPLAY",	BIT(0)},
>>>>  	{"CPU",		BIT(1)},
>>>> @@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>>>>  		break;
>>>>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>>>>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>>>> -		dev->num_ips = 22;
>>>> +		if (boot_cpu_data.x86_model == 0x70)
>>>> +			dev->num_ips = 25;
>>>> +		else
>>>> +			dev->num_ips = 22;
>>>
>>> Could these use ARRAY_SIZE()? They're related to that array, aren't they?
>>
>> Yes, they are. ARRAY_SIZE() does return the number of elements in the
>> array but there is a catch,
>>
>> both soc15_ip_blk[] and soc15_ip_blk_v2[] have a last empty element,
>> so when ARRAY_SIZE() is used we end up getting n+1 element (i.e along
>> with the last empty element). So, what would you advise?
>>
>> 1) remove the last empty element in the array? i.e. something like this?
>>
>> static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
>> 	{"DISPLAY",     BIT(0)},
>> 	...
>> 	{"VPE",         BIT(24)},
>> -   {} /* remove this */
>> };
>>
>> 2) or just do, `ARRAY_SIZE() - 1` so that we don't need to touch the
>> existing soc15_ip_blk[] and soc15_ip_blk_v2[].
> 
> The iteration seems to always ->num_ips based so the terminator seems 
> superfluous. The cases with smaller num_ips cannot use terminator anyway 
> as they share the larger array.
> 
> I propose you make a separate change out removing the terminator and 
> migrating to use ARRAY_SIZE() in the existing code, don't forget to add 
> the #include for it. Then add just this new thing in this patch.
> 

Sure. Will do it.

Thanks,
Shyam

Re: [PATCH v2 3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

[Shyam Sundar S K]

On 11/5/2024 15:14, Ilpo Järvinen wrote:
> On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
>> On 11/1/2024 15:58, Ilpo Järvinen wrote:
>>> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
>>>
>>>> To distinguish between the PMC message port and the S2D (Spill to DRAM)
>>>> message port, replace the use of 0 and 1 with an enum.
>>>>
>>>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> index 5efec020ecac..2b06861c479b 100644
>>>> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> @@ -47,6 +47,11 @@ enum s2d_arg {
>>>>  	S2D_DRAM_SIZE,
>>>>  };
>>>>  
>>>> +enum s2d_msg_port {
>>>> +	MSG_PORT_PMC,
>>>> +	MSG_PORT_S2D,
>>>> +};
>>>> +
>>>>  struct amd_stb_v2_data {
>>>>  	size_t size;
>>>>  	u8 data[] __counted_by(size);
>>>> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>>>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>>>>  
>>>>  	/* Spill to DRAM num_samples uses separate SMU message port */
>>>> -	dev->msg_port = 1;
>>>> +	dev->msg_port = MSG_PORT_S2D;
>>>>  
>>>>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>>>>  	if (ret)
>>>> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>>>  	/* Get the num_samples to calculate the last push location */
>>>>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>>>>  	/* Clear msg_port for other SMU operation */
>>>> -	dev->msg_port = 0;
>>>> +	dev->msg_port = MSG_PORT_PMC;
>>>>  	if (ret) {
>>>>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>>>>  		return ret;
>>>> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>>>  				    &amd_stb_debugfs_fops);
>>>>  
>>>>  	/* Spill to DRAM feature uses separate SMU message port */
>>>> -	dev->msg_port = 1;
>>>> +	dev->msg_port = MSG_PORT_S2D;
>>>>  
>>>>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>>>>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>>>> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>>>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>>>  
>>>>  	/* Clear msg_port for other SMU operation */
>>>> -	dev->msg_port = 0;
>>>> +	dev->msg_port = MSG_PORT_PMC;
>>>>  
>>>>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>>>>  	if (!dev->stb_virt_addr)
>>>
>>> This change is incomplete, you missed all places using it:
>>>
>>> if (dev->msg_port) { 
>>>
>>> + add helper for this:
>>>
>>> dev->msg_port ? "S2D" : "PMC"
>>>
>>
>>
>> I am not sure if I understand your comment fully. Can you please
>> elaborate?
> 
> There are users of dev->msg_port that should be also touched by this 
> change but weren't.
> 
> For the printing, I suggested a helper function which returns the correct 
> string so you don't need to do the compare within print argument.
> 

Got it. I have addressed this helper function in 8/13 of v3 just sent
out now. Please have a look at it.

Thanks,
Shyam