[PATCH 0/4] Updates to amd-pmc driver

[PATCH 0/4] Updates to amd-pmc driver

[Shyam Sundar S K]

This patch series includes:

- Move PMC driver to separate directory
- Read SMU version at the time of probe to avoid duplication of code
- Add a Firmware command that can force flush the Spill to DRAM contents.
- override the STB dump size with a custom one using a module_param


Shyam Sundar S K (4):
  platform/x86/amd/pmc: Move PMC driver to separate directory
  platform/x86/amd/pmc: Read SMU version at the time of probe
  platform/x86/amd/pmc: Add PMFW command id to support S2D force flush
  platform/x86/amd/pmc: Add dump_custom_stb module parameter

 MAINTAINERS                                   |  2 +-
 drivers/platform/x86/amd/Kconfig              | 16 +---
 drivers/platform/x86/amd/Makefile             |  3 +-
 drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++
 drivers/platform/x86/amd/pmc/Makefile         |  8 ++
 .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
 drivers/platform/x86/amd/{ => pmc}/pmc.c      | 80 ++++++++-----------
 drivers/platform/x86/amd/{ => pmc}/pmc.h      |  1 +
 8 files changed, 66 insertions(+), 63 deletions(-)
 create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
 create mode 100644 drivers/platform/x86/amd/pmc/Makefile
 rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
 rename drivers/platform/x86/amd/{ => pmc}/pmc.c (96%)
 rename drivers/platform/x86/amd/{ => pmc}/pmc.h (98%)

-- 
2.25.1

[PATCH 1/4] platform/x86/amd/pmc: Move PMC driver to separate directory

[Shyam Sundar S K]

With latest commits having PMC code spread across multiple files, it would
be easier to maintain them in a separate directory under amd/pmc.

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>
---
 MAINTAINERS                                   |  2 +-
 drivers/platform/x86/amd/Kconfig              | 16 +---------------
 drivers/platform/x86/amd/Makefile             |  3 +--
 drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++++++++++++++++
 drivers/platform/x86/amd/pmc/Makefile         |  8 ++++++++
 .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
 drivers/platform/x86/amd/{ => pmc}/pmc.c      |  0
 drivers/platform/x86/amd/{ => pmc}/pmc.h      |  0
 8 files changed, 30 insertions(+), 18 deletions(-)
 create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
 create mode 100644 drivers/platform/x86/amd/pmc/Makefile
 rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
 rename drivers/platform/x86/amd/{ => pmc}/pmc.c (100%)
 rename drivers/platform/x86/amd/{ => pmc}/pmc.h (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0f966f05fb0d..366d4b4c9c8f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1006,7 +1006,7 @@ AMD PMC DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
-F:	drivers/platform/x86/amd/pmc.c
+F:	drivers/platform/x86/amd/pmc/
 
 AMD PMF DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index d9685aef0887..55f3a2fc6aec 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -4,21 +4,7 @@
 #
 
 source "drivers/platform/x86/amd/pmf/Kconfig"
-
-config AMD_PMC
-	tristate "AMD SoC PMC driver"
-	depends on ACPI && PCI && RTC_CLASS && AMD_NB
-	select SERIO
-	help
-	  The driver provides support for AMD Power Management Controller
-	  primarily responsible for S2Idle transactions that are driven from
-	  a platform firmware running on SMU. This driver also provides a debug
-	  mechanism to investigate the S2Idle transactions and failures.
-
-	  Say Y or M here if you have a notebook powered by AMD RYZEN CPU/APU.
-
-	  If you choose to compile this driver as a module the module will be
-	  called amd-pmc.
+source "drivers/platform/x86/amd/pmc/Kconfig"
 
 config AMD_HSMP
 	tristate "AMD HSMP Driver"
diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile
index 65732f0a3913..f04932b7a7d1 100644
--- a/drivers/platform/x86/amd/Makefile
+++ b/drivers/platform/x86/amd/Makefile
@@ -4,8 +4,7 @@
 # AMD x86 Platform-Specific Drivers
 #
 
-amd-pmc-y			:= pmc.o pmc-quirks.o
-obj-$(CONFIG_AMD_PMC)		+= amd-pmc.o
+obj-$(CONFIG_AMD_PMC)		+= pmc/
 amd_hsmp-y			:= hsmp.o
 obj-$(CONFIG_AMD_HSMP)		+= amd_hsmp.o
 obj-$(CONFIG_AMD_PMF)		+= pmf/
diff --git a/drivers/platform/x86/amd/pmc/Kconfig b/drivers/platform/x86/amd/pmc/Kconfig
new file mode 100644
index 000000000000..8db9e82de00b
--- /dev/null
+++ b/drivers/platform/x86/amd/pmc/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# AMD PMC Driver
+#
+
+config AMD_PMC
+	tristate "AMD SoC PMC driver"
+	depends on ACPI && PCI && RTC_CLASS && AMD_NB
+	select SERIO
+	help
+	  The driver provides support for AMD Power Management Controller
+	  primarily responsible for S2Idle transactions that are driven from
+	  a platform firmware running on SMU. This driver also provides a debug
+	  mechanism to investigate the S2Idle transactions and failures.
+
+	  Say Y or M here if you have a notebook powered by AMD RYZEN CPU/APU.
+
+	  If you choose to compile this driver as a module the module will be
+	  called amd-pmc.
diff --git a/drivers/platform/x86/amd/pmc/Makefile b/drivers/platform/x86/amd/pmc/Makefile
new file mode 100644
index 000000000000..4aaa29d351c9
--- /dev/null
+++ b/drivers/platform/x86/amd/pmc/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/x86/amd/pmc
+# AMD Power Management Controller Driver
+#
+
+amd-pmc-objs := pmc.o pmc-quirks.o
+obj-$(CONFIG_AMD_PMC) += amd-pmc.o
diff --git a/drivers/platform/x86/amd/pmc-quirks.c b/drivers/platform/x86/amd/pmc/pmc-quirks.c
similarity index 100%
rename from drivers/platform/x86/amd/pmc-quirks.c
rename to drivers/platform/x86/amd/pmc/pmc-quirks.c
diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
similarity index 100%
rename from drivers/platform/x86/amd/pmc.c
rename to drivers/platform/x86/amd/pmc/pmc.c
diff --git a/drivers/platform/x86/amd/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
similarity index 100%
rename from drivers/platform/x86/amd/pmc.h
rename to drivers/platform/x86/amd/pmc/pmc.h
-- 
2.25.1

[PATCH 2/4] platform/x86/amd/pmc: Read SMU version at the time of probe

[Shyam Sundar S K]

Currently the SMU version is being read at multiple places, unify all
of them and get the SMU version at the time of probe.

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 | 39 +++++-------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index c1e788b67a74..043451fabbbe 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -405,12 +405,6 @@ static ssize_t smu_fw_version_show(struct device *d, struct device_attribute *at
 {
 	struct amd_pmc_dev *dev = dev_get_drvdata(d);
 
-	if (!dev->major) {
-		int rc = amd_pmc_get_smu_version(dev);
-
-		if (rc)
-			return rc;
-	}
 	return sysfs_emit(buf, "%u.%u.%u\n", dev->major, dev->minor, dev->rev);
 }
 
@@ -419,12 +413,6 @@ static ssize_t smu_program_show(struct device *d, struct device_attribute *attr,
 {
 	struct amd_pmc_dev *dev = dev_get_drvdata(d);
 
-	if (!dev->major) {
-		int rc = amd_pmc_get_smu_version(dev);
-
-		if (rc)
-			return rc;
-	}
 	return sysfs_emit(buf, "%u\n", dev->smu_program);
 }
 
@@ -526,16 +514,9 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
 				 struct seq_file *s)
 {
 	u32 val;
-	int rc;
 
 	switch (pdev->cpu_id) {
 	case AMD_CPU_ID_CZN:
-		/* we haven't yet read SMU version */
-		if (!pdev->major) {
-			rc = amd_pmc_get_smu_version(pdev);
-			if (rc)
-				return rc;
-		}
 		if (pdev->major > 56 || (pdev->major >= 55 && pdev->minor >= 37))
 			val = amd_pmc_reg_read(pdev, AMD_PMC_SCRATCH_REG_CZN);
 		else
@@ -717,13 +698,6 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
 static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
 {
 	struct device *d;
-	int rc;
-
-	if (!pdev->major) {
-		rc = amd_pmc_get_smu_version(pdev);
-		if (rc)
-			return rc;
-	}
 
 	if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
 		return 0;
@@ -749,13 +723,6 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
 	struct rtc_time tm;
 	int rc;
 
-	/* we haven't yet read SMU version */
-	if (!pdev->major) {
-		rc = amd_pmc_get_smu_version(pdev);
-		if (rc)
-			return rc;
-	}
-
 	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
 		return 0;
 
@@ -1059,6 +1026,12 @@ static int amd_pmc_probe(struct platform_device *pdev)
 
 	mutex_init(&dev->lock);
 
+	err = amd_pmc_get_smu_version(dev);
+	if (err) {
+		dev_err(dev->dev, "error reading SMU version\n");
+		goto err_pci_dev_put;
+	}
+
 	if (enable_stb && amd_pmc_is_stb_supported(dev)) {
 		err = amd_pmc_s2d_init(dev);
 		if (err)
-- 
2.25.1

[PATCH 3/4] platform/x86/amd/pmc: Add PMFW command id to support S2D force flush

[Shyam Sundar S K]

Recent PMFW have the capability that can force flush the FIFO
contents to DRAM on sending a command id via the mailbox. Add this support
to 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 043451fabbbe..45f7d21276bb 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -55,6 +55,9 @@
 #define S2D_TELEMETRY_BYTES_MAX		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
 #define AMD_PMC_BASE_ADDR_OFFSET	0x10000
@@ -236,7 +239,7 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
 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 *buf, fsize, num_samples, stb_rdptr_offset = 0;
+	u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
 	int ret;
 
 	/* Write dummy postcode while reading the STB buffer */
@@ -251,6 +254,10 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	/* 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_warn_once(dev->dev, "S2D force flush not supported\n");
+
 	/* 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 */
-- 
2.25.1

[PATCH 4/4] platform/x86/amd/pmc: Add dump_custom_stb module parameter

[Shyam Sundar S K]

There have been instances when the default size (1M) of the STB is not
sufficient to get the complete traces of the failure. In such scenarios
we can use a module_param to enable full trace that shall contain more
debugging data. This is not a regular case and hence not enabling this
capability by default.

Co-developed-by: Harsh Jain <Harsh.Jain@amd.com>
Signed-off-by: Harsh Jain <Harsh.Jain@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 | 34 +++++++++++++++++++-----------
 drivers/platform/x86/amd/pmc/pmc.h |  1 +
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 45f7d21276bb..06d901a8f6e5 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -53,6 +53,7 @@
 
 /* STB Spill to DRAM Parameters */
 #define S2D_TELEMETRY_BYTES_MAX		0x100000
+#define S2D_TELEMETRY_FSIZE_MAX		0x200000
 #define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
 
 /* STB Spill to DRAM Message Definition */
@@ -160,6 +161,10 @@ 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);
@@ -239,7 +244,7 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
 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 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
+	u32 *buf, num_samples, val, stb_rdptr_offset = 0;
 	int ret;
 
 	/* Write dummy postcode while reading the STB buffer */
@@ -247,10 +252,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	if (ret)
 		dev_err(dev->dev, "error writing to STB: %d\n", ret);
 
-	buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	/* Spill to DRAM num_samples uses separate SMU message port */
 	dev->msg_port = 1;
 
@@ -264,20 +265,27 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	dev->msg_port = 0;
 	if (ret) {
 		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
-		kfree(buf);
 		return ret;
 	}
 
 	/* Start capturing data from the last push location */
-	if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
-		fsize  = S2D_TELEMETRY_BYTES_MAX;
-		stb_rdptr_offset = num_samples - fsize;
+	if (dump_custom_stb &&
+	    (dev->dram_size - S2D_TELEMETRY_BYTES_MAX <= S2D_TELEMETRY_FSIZE_MAX)) {
+		dev->fsize = dev->dram_size - S2D_TELEMETRY_BYTES_MAX;
+		stb_rdptr_offset = 0;
+	} else if (num_samples > S2D_TELEMETRY_BYTES_MAX) {
+		dev->fsize  = S2D_TELEMETRY_BYTES_MAX;
+		stb_rdptr_offset = num_samples - dev->fsize;
 	} else {
-		fsize = num_samples;
+		dev->fsize = num_samples;
 		stb_rdptr_offset = 0;
 	}
 
-	memcpy_fromio(buf, dev->stb_virt_addr + stb_rdptr_offset, fsize);
+	buf = kzalloc(dev->fsize, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	memcpy_fromio(buf, dev->stb_virt_addr + stb_rdptr_offset, dev->fsize);
 	filp->private_data = buf;
 
 	return 0;
@@ -286,11 +294,13 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
 					   loff_t *pos)
 {
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+
 	if (!filp->private_data)
 		return -EINVAL;
 
 	return simple_read_from_buffer(buf, size, pos, filp->private_data,
-					S2D_TELEMETRY_BYTES_MAX);
+					dev->fsize);
 }
 
 static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
index c27bd6a5642f..f73d265430b8 100644
--- a/drivers/platform/x86/amd/pmc/pmc.h
+++ b/drivers/platform/x86/amd/pmc/pmc.h
@@ -26,6 +26,7 @@ struct amd_pmc_dev {
 	u32 dram_size;
 	u32 num_ips;
 	u32 s2d_msg_id;
+	u32 fsize;
 /* SMU version information */
 	u8 smu_program;
 	u8 major;
-- 
2.25.1

Re: [PATCH 2/4] platform/x86/amd/pmc: Read SMU version at the time of probe

[Limonciello, Mario]

On 8/11/2023 6:21 AM, Shyam Sundar S K wrote:
> Currently the SMU version is being read at multiple places, unify all
> of them and get the SMU version at the time of probe.
> 
> 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 | 39 +++++-------------------------
>   1 file changed, 6 insertions(+), 33 deletions(-)
> 

It's actually quite intentional that the SMU version isn't read at probe 
but rather at first use.  The reason is that it increased boot time in a 
measurable way.

> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index c1e788b67a74..043451fabbbe 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -405,12 +405,6 @@ static ssize_t smu_fw_version_show(struct device *d, struct device_attribute *at
>   {
>   	struct amd_pmc_dev *dev = dev_get_drvdata(d);
>   
> -	if (!dev->major) {
> -		int rc = amd_pmc_get_smu_version(dev);
> -
> -		if (rc)
> -			return rc;
> -	}
>   	return sysfs_emit(buf, "%u.%u.%u\n", dev->major, dev->minor, dev->rev);
>   }
>   
> @@ -419,12 +413,6 @@ static ssize_t smu_program_show(struct device *d, struct device_attribute *attr,
>   {
>   	struct amd_pmc_dev *dev = dev_get_drvdata(d);
>   
> -	if (!dev->major) {
> -		int rc = amd_pmc_get_smu_version(dev);
> -
> -		if (rc)
> -			return rc;
> -	}
>   	return sysfs_emit(buf, "%u\n", dev->smu_program);
>   }
>   
> @@ -526,16 +514,9 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
>   				 struct seq_file *s)
>   {
>   	u32 val;
> -	int rc;
>   
>   	switch (pdev->cpu_id) {
>   	case AMD_CPU_ID_CZN:
> -		/* we haven't yet read SMU version */
> -		if (!pdev->major) {
> -			rc = amd_pmc_get_smu_version(pdev);
> -			if (rc)
> -				return rc;
> -		}
>   		if (pdev->major > 56 || (pdev->major >= 55 && pdev->minor >= 37))
>   			val = amd_pmc_reg_read(pdev, AMD_PMC_SCRATCH_REG_CZN);
>   		else
> @@ -717,13 +698,6 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
>   static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>   {
>   	struct device *d;
> -	int rc;
> -
> -	if (!pdev->major) {
> -		rc = amd_pmc_get_smu_version(pdev);
> -		if (rc)
> -			return rc;
> -	}
>   
>   	if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
>   		return 0;
> @@ -749,13 +723,6 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>   	struct rtc_time tm;
>   	int rc;
>   
> -	/* we haven't yet read SMU version */
> -	if (!pdev->major) {
> -		rc = amd_pmc_get_smu_version(pdev);
> -		if (rc)
> -			return rc;
> -	}
> -
>   	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
>   		return 0;
>   
> @@ -1059,6 +1026,12 @@ static int amd_pmc_probe(struct platform_device *pdev)
>   
>   	mutex_init(&dev->lock);
>   
> +	err = amd_pmc_get_smu_version(dev);
> +	if (err) {
> +		dev_err(dev->dev, "error reading SMU version\n");
> +		goto err_pci_dev_put;
> +	}
> +
>   	if (enable_stb && amd_pmc_is_stb_supported(dev)) {
>   		err = amd_pmc_s2d_init(dev);
>   		if (err)

Re: [PATCH 0/4] Updates to amd-pmc driver

[Hans de Goede]

Hi Shyam,

On 8/11/23 13:21, Shyam Sundar S K wrote:
> This patch series includes:
> 
> - Move PMC driver to separate directory

This one seems to rely on the state of the amd-pmc code in pdx86/fixes
which is also the state in Linus' master but not the state in pdx86/for-next
which is based on 6.5-rc1 where as e.g. :

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=e8ef8dd28c4c4b86cd3010ff42c79582f766862e

Which adds the pmc-quirks.c file was added later in the fixes branch.

So it is best to do this moving into a separate dir directly
after 6.6-rc1 is out, at which point fixes and for-next will have
both been merged.

> - Read SMU version at the time of probe to avoid duplication of code

Please address the comments made on this patch (or drop it).

I assume the rest relies on this patch, so I'm going to drop this
entire series from my queue now.

Regards,

Hans



> - Add a Firmware command that can force flush the Spill to DRAM contents.
> - override the STB dump size with a custom one using a module_param
> 
> 
> Shyam Sundar S K (4):
>   platform/x86/amd/pmc: Move PMC driver to separate directory
>   platform/x86/amd/pmc: Read SMU version at the time of probe
>   platform/x86/amd/pmc: Add PMFW command id to support S2D force flush
>   platform/x86/amd/pmc: Add dump_custom_stb module parameter
> 
>  MAINTAINERS                                   |  2 +-
>  drivers/platform/x86/amd/Kconfig              | 16 +---
>  drivers/platform/x86/amd/Makefile             |  3 +-
>  drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++
>  drivers/platform/x86/amd/pmc/Makefile         |  8 ++
>  .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
>  drivers/platform/x86/amd/{ => pmc}/pmc.c      | 80 ++++++++-----------
>  drivers/platform/x86/amd/{ => pmc}/pmc.h      |  1 +
>  8 files changed, 66 insertions(+), 63 deletions(-)
>  create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
>  create mode 100644 drivers/platform/x86/amd/pmc/Makefile
>  rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
>  rename drivers/platform/x86/amd/{ => pmc}/pmc.c (96%)
>  rename drivers/platform/x86/amd/{ => pmc}/pmc.h (98%)
> 

Re: [PATCH 0/4] Updates to amd-pmc driver

[Hans de Goede]

Hi Again,

On 8/21/23 14:57, Hans de Goede wrote:
> Hi Shyam,
> 
> On 8/11/23 13:21, Shyam Sundar S K wrote:
>> This patch series includes:
>>
>> - Move PMC driver to separate directory
> 
> This one seems to rely on the state of the amd-pmc code in pdx86/fixes
> which is also the state in Linus' master but not the state in pdx86/for-next
> which is based on 6.5-rc1 where as e.g. :
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=e8ef8dd28c4c4b86cd3010ff42c79582f766862e
> 
> Which adds the pmc-quirks.c file was added later in the fixes branch.
> 
> So it is best to do this moving into a separate dir directly
> after 6.6-rc1 is out, at which point fixes and for-next will have
> both been merged.

So I have just back-merged pdx86/fixes into review-hans (future pdx86/for-next)
because some pending mellanox changes need this to avoid conflicts.

As such I think I can pick this patch (the patch moving things into its
own subdir) now.

I think it might be a good idea to merge this patch for the upcoming
6.6 window so that we have the moving of files behind us.

Shyam, shall I merge 1/4 into review-hans / for 6.6-rc1 now ?

Regards,

Hans




> 
>> - Read SMU version at the time of probe to avoid duplication of code
> 
> Please address the comments made on this patch (or drop it).
> 
> I assume the rest relies on this patch, so I'm going to drop this
> entire series from my queue now.
> 
> Regards,
> 
> Hans
> 
> 
> 
>> - Add a Firmware command that can force flush the Spill to DRAM contents.
>> - override the STB dump size with a custom one using a module_param
>>
>>
>> Shyam Sundar S K (4):
>>   platform/x86/amd/pmc: Move PMC driver to separate directory
>>   platform/x86/amd/pmc: Read SMU version at the time of probe
>>   platform/x86/amd/pmc: Add PMFW command id to support S2D force flush
>>   platform/x86/amd/pmc: Add dump_custom_stb module parameter
>>
>>  MAINTAINERS                                   |  2 +-
>>  drivers/platform/x86/amd/Kconfig              | 16 +---
>>  drivers/platform/x86/amd/Makefile             |  3 +-
>>  drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++
>>  drivers/platform/x86/amd/pmc/Makefile         |  8 ++
>>  .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
>>  drivers/platform/x86/amd/{ => pmc}/pmc.c      | 80 ++++++++-----------
>>  drivers/platform/x86/amd/{ => pmc}/pmc.h      |  1 +
>>  8 files changed, 66 insertions(+), 63 deletions(-)
>>  create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
>>  create mode 100644 drivers/platform/x86/amd/pmc/Makefile
>>  rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.c (96%)
>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.h (98%)
>>

Re: [PATCH 2/4] platform/x86/amd/pmc: Read SMU version at the time of probe

[Shyam Sundar S K]

Hi Mario,

On 8/11/2023 5:34 PM, Limonciello, Mario wrote:
> 
> 
> On 8/11/2023 6:21 AM, Shyam Sundar S K wrote:
>> Currently the SMU version is being read at multiple places, unify all
>> of them and get the SMU version at the time of probe.
>>
>> 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 | 39 +++++-------------------------
>>   1 file changed, 6 insertions(+), 33 deletions(-)
>>
> 
> It's actually quite intentional that the SMU version isn't read at probe
> but rather at first use.  The reason is that it increased boot time in a
> measurable way.

Apologies. I missed to respond back on this. I will drop this patch in v2.

Thanks,
Shyam

> 
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>> b/drivers/platform/x86/amd/pmc/pmc.c
>> index c1e788b67a74..043451fabbbe 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -405,12 +405,6 @@ static ssize_t smu_fw_version_show(struct device
>> *d, struct device_attribute *at
>>   {
>>       struct amd_pmc_dev *dev = dev_get_drvdata(d);
>>   -    if (!dev->major) {
>> -        int rc = amd_pmc_get_smu_version(dev);
>> -
>> -        if (rc)
>> -            return rc;
>> -    }
>>       return sysfs_emit(buf, "%u.%u.%u\n", dev->major, dev->minor,
>> dev->rev);
>>   }
>>   @@ -419,12 +413,6 @@ static ssize_t smu_program_show(struct device
>> *d, struct device_attribute *attr,
>>   {
>>       struct amd_pmc_dev *dev = dev_get_drvdata(d);
>>   -    if (!dev->major) {
>> -        int rc = amd_pmc_get_smu_version(dev);
>> -
>> -        if (rc)
>> -            return rc;
>> -    }
>>       return sysfs_emit(buf, "%u\n", dev->smu_program);
>>   }
>>   @@ -526,16 +514,9 @@ static int amd_pmc_idlemask_read(struct
>> amd_pmc_dev *pdev, struct device *dev,
>>                    struct seq_file *s)
>>   {
>>       u32 val;
>> -    int rc;
>>         switch (pdev->cpu_id) {
>>       case AMD_CPU_ID_CZN:
>> -        /* we haven't yet read SMU version */
>> -        if (!pdev->major) {
>> -            rc = amd_pmc_get_smu_version(pdev);
>> -            if (rc)
>> -                return rc;
>> -        }
>>           if (pdev->major > 56 || (pdev->major >= 55 && pdev->minor >=
>> 37))
>>               val = amd_pmc_reg_read(pdev, AMD_PMC_SCRATCH_REG_CZN);
>>           else
>> @@ -717,13 +698,6 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev
>> *dev)
>>   static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>>   {
>>       struct device *d;
>> -    int rc;
>> -
>> -    if (!pdev->major) {
>> -        rc = amd_pmc_get_smu_version(pdev);
>> -        if (rc)
>> -            return rc;
>> -    }
>>         if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
>>           return 0;
>> @@ -749,13 +723,6 @@ static int amd_pmc_verify_czn_rtc(struct
>> amd_pmc_dev *pdev, u32 *arg)
>>       struct rtc_time tm;
>>       int rc;
>>   -    /* we haven't yet read SMU version */
>> -    if (!pdev->major) {
>> -        rc = amd_pmc_get_smu_version(pdev);
>> -        if (rc)
>> -            return rc;
>> -    }
>> -
>>       if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
>>           return 0;
>>   @@ -1059,6 +1026,12 @@ static int amd_pmc_probe(struct
>> platform_device *pdev)
>>         mutex_init(&dev->lock);
>>   +    err = amd_pmc_get_smu_version(dev);
>> +    if (err) {
>> +        dev_err(dev->dev, "error reading SMU version\n");
>> +        goto err_pci_dev_put;
>> +    }
>> +
>>       if (enable_stb && amd_pmc_is_stb_supported(dev)) {
>>           err = amd_pmc_s2d_init(dev);
>>           if (err)

Re: [PATCH 0/4] Updates to amd-pmc driver

[Shyam Sundar S K]

Hi Hans,

On 8/21/2023 11:22 PM, Hans de Goede wrote:
> Hi Again,
> 
> On 8/21/23 14:57, Hans de Goede wrote:
>> Hi Shyam,
>>
>> On 8/11/23 13:21, Shyam Sundar S K wrote:
>>> This patch series includes:
>>>
>>> - Move PMC driver to separate directory
>>
>> This one seems to rely on the state of the amd-pmc code in pdx86/fixes
>> which is also the state in Linus' master but not the state in pdx86/for-next
>> which is based on 6.5-rc1 where as e.g. :
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=e8ef8dd28c4c4b86cd3010ff42c79582f766862e
>>
>> Which adds the pmc-quirks.c file was added later in the fixes branch.
>>
>> So it is best to do this moving into a separate dir directly
>> after 6.6-rc1 is out, at which point fixes and for-next will have
>> both been merged.
> 
> So I have just back-merged pdx86/fixes into review-hans (future pdx86/for-next)
> because some pending mellanox changes need this to avoid conflicts.
> 
> As such I think I can pick this patch (the patch moving things into its
> own subdir) now.
> 
> I think it might be a good idea to merge this patch for the upcoming
> 6.6 window so that we have the moving of files behind us.
> 
> Shyam, shall I merge 1/4 into review-hans / for 6.6-rc1 now ?

Yes please. I will drop 2/4 as per Mario's remark. Once you have 1/4
into review-hans, I will rebase my changes on top of it and send 3/4,
4/4 as v2.

Will that work for you?

Thanks,
Shyam

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>>> - Read SMU version at the time of probe to avoid duplication of code
>>
>> Please address the comments made on this patch (or drop it).
>>
>> I assume the rest relies on this patch, so I'm going to drop this
>> entire series from my queue now.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> - Add a Firmware command that can force flush the Spill to DRAM contents.
>>> - override the STB dump size with a custom one using a module_param
>>>
>>>
>>> Shyam Sundar S K (4):
>>>   platform/x86/amd/pmc: Move PMC driver to separate directory
>>>   platform/x86/amd/pmc: Read SMU version at the time of probe
>>>   platform/x86/amd/pmc: Add PMFW command id to support S2D force flush
>>>   platform/x86/amd/pmc: Add dump_custom_stb module parameter
>>>
>>>  MAINTAINERS                                   |  2 +-
>>>  drivers/platform/x86/amd/Kconfig              | 16 +---
>>>  drivers/platform/x86/amd/Makefile             |  3 +-
>>>  drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++
>>>  drivers/platform/x86/amd/pmc/Makefile         |  8 ++
>>>  .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
>>>  drivers/platform/x86/amd/{ => pmc}/pmc.c      | 80 ++++++++-----------
>>>  drivers/platform/x86/amd/{ => pmc}/pmc.h      |  1 +
>>>  8 files changed, 66 insertions(+), 63 deletions(-)
>>>  create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
>>>  create mode 100644 drivers/platform/x86/amd/pmc/Makefile
>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.c (96%)
>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.h (98%)
>>>
> 

Re: [PATCH 0/4] Updates to amd-pmc driver

[Hans de Goede]

Hi,

On 8/22/23 06:11, Shyam Sundar S K wrote:
> Hi Hans,
> 
> On 8/21/2023 11:22 PM, Hans de Goede wrote:
>> Hi Again,
>>
>> On 8/21/23 14:57, Hans de Goede wrote:
>>> Hi Shyam,
>>>
>>> On 8/11/23 13:21, Shyam Sundar S K wrote:
>>>> This patch series includes:
>>>>
>>>> - Move PMC driver to separate directory
>>>
>>> This one seems to rely on the state of the amd-pmc code in pdx86/fixes
>>> which is also the state in Linus' master but not the state in pdx86/for-next
>>> which is based on 6.5-rc1 where as e.g. :
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=e8ef8dd28c4c4b86cd3010ff42c79582f766862e
>>>
>>> Which adds the pmc-quirks.c file was added later in the fixes branch.
>>>
>>> So it is best to do this moving into a separate dir directly
>>> after 6.6-rc1 is out, at which point fixes and for-next will have
>>> both been merged.
>>
>> So I have just back-merged pdx86/fixes into review-hans (future pdx86/for-next)
>> because some pending mellanox changes need this to avoid conflicts.
>>
>> As such I think I can pick this patch (the patch moving things into its
>> own subdir) now.
>>
>> I think it might be a good idea to merge this patch for the upcoming
>> 6.6 window so that we have the moving of files behind us.
>>
>> Shyam, shall I merge 1/4 into review-hans / for 6.6-rc1 now ?
> 
> Yes please. I will drop 2/4 as per Mario's remark. Once you have 1/4
> into review-hans,

I have just pushed 1/4 to review-hans.

> I will rebase my changes on top of it and send 3/4,
> 4/4 as v2.
> 
> Will that work for you?

Yes sounds good, thank you.

Regards,

Hans






>>>> - Read SMU version at the time of probe to avoid duplication of code
>>>
>>> Please address the comments made on this patch (or drop it).
>>>
>>> I assume the rest relies on this patch, so I'm going to drop this
>>> entire series from my queue now.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>> - Add a Firmware command that can force flush the Spill to DRAM contents.
>>>> - override the STB dump size with a custom one using a module_param
>>>>
>>>>
>>>> Shyam Sundar S K (4):
>>>>   platform/x86/amd/pmc: Move PMC driver to separate directory
>>>>   platform/x86/amd/pmc: Read SMU version at the time of probe
>>>>   platform/x86/amd/pmc: Add PMFW command id to support S2D force flush
>>>>   platform/x86/amd/pmc: Add dump_custom_stb module parameter
>>>>
>>>>  MAINTAINERS                                   |  2 +-
>>>>  drivers/platform/x86/amd/Kconfig              | 16 +---
>>>>  drivers/platform/x86/amd/Makefile             |  3 +-
>>>>  drivers/platform/x86/amd/pmc/Kconfig          | 19 +++++
>>>>  drivers/platform/x86/amd/pmc/Makefile         |  8 ++
>>>>  .../platform/x86/amd/{ => pmc}/pmc-quirks.c   |  0
>>>>  drivers/platform/x86/amd/{ => pmc}/pmc.c      | 80 ++++++++-----------
>>>>  drivers/platform/x86/amd/{ => pmc}/pmc.h      |  1 +
>>>>  8 files changed, 66 insertions(+), 63 deletions(-)
>>>>  create mode 100644 drivers/platform/x86/amd/pmc/Kconfig
>>>>  create mode 100644 drivers/platform/x86/amd/pmc/Makefile
>>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc-quirks.c (100%)
>>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.c (96%)
>>>>  rename drivers/platform/x86/amd/{ => pmc}/pmc.h (98%)
>>>>
>>
>