[PATCH v3 0/4] Updates to AMD PMC driver

[PATCH v3 0/4] Updates to AMD PMC driver

[Shyam Sundar S K]

Series includes fixes and feature updates to the PMC driver.
Series has been split into 4 patches to incorporate the review
remarks from v2.

v3:
 - Based on review-hans branch
 - Pass true/false instead of 1/0 the amd_pmc_send_cmd()
 - Add helper for supported cpu id check as suggested by Ilpo.

v2:
 - Based on review-hans branch
 - Add a switch for cpu-id check based on feedback from Mario.

Shyam Sundar S K (4):
  platform/x86/amd: pmc: Pass true/false to bool argument
  platform/x86/amd: pmc: Get STB DRAM size from PMFW
  platform/x86/amd: pmc: Add helper function to check the cpu id
  platform/x86/amd: pmc: Update metrics table info for Pink Sardine

 drivers/platform/x86/amd/pmc.c | 123 ++++++++++++++++++++++++++-------
 1 file changed, 99 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] platform/x86/amd: pmc: Pass true/false to bool argument

[Shyam Sundar S K]

Pass true/false to the bool argument of the amd_pmc_send_cmd() function,
instead of 1/0 to match the function signature.

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.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 427905714f79..f28c295b47dd 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -261,7 +261,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	dev->msg_port = 1;
 
 	/* Get the num_samples to calculate the last push location */
-	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, 1);
+	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, true);
 	/* Clear msg_port for other SMU operation */
 	dev->msg_port = 0;
 	if (ret) {
@@ -317,15 +317,15 @@ static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
 
 	/* Get Active devices list from SMU */
 	if (!dev->active_ips)
-		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
+		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, true);
 
 	/* Get dram address */
 	if (!dev->smu_virt_addr) {
 		u32 phys_addr_low, phys_addr_hi;
 		u64 smu_phys_addr;
 
-		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
-		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
+		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, true);
+		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, true);
 		smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 		dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr,
@@ -335,8 +335,8 @@ static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
 	}
 
 	/* Start the logging */
-	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, 0);
-	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, 0);
+	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, false);
+	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, false);
 
 	return 0;
 }
@@ -377,7 +377,7 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
 	if (dev->cpu_id == AMD_CPU_ID_PCO)
 		return -ENODEV;
 
-	rc = amd_pmc_send_cmd(dev, 0, &val, SMU_MSG_GETSMUVERSION, 1);
+	rc = amd_pmc_send_cmd(dev, 0, &val, SMU_MSG_GETSMUVERSION, true);
 	if (rc)
 		return rc;
 
@@ -794,7 +794,7 @@ static void amd_pmc_s2idle_prepare(void)
 	}
 
 	msg = amd_pmc_get_os_hint(pdev);
-	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
+	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, false);
 	if (rc) {
 		dev_err(pdev->dev, "suspend failed: %d\n", rc);
 		return;
@@ -829,7 +829,7 @@ static int amd_pmc_dump_data(struct amd_pmc_dev *pdev)
 	if (pdev->cpu_id == AMD_CPU_ID_PCO)
 		return -ENODEV;
 
-	return amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, 0);
+	return amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, false);
 }
 
 static void amd_pmc_s2idle_restore(void)
@@ -839,7 +839,7 @@ static void amd_pmc_s2idle_restore(void)
 	u8 msg;
 
 	msg = amd_pmc_get_os_hint(pdev);
-	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, 0);
+	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, false);
 	if (rc)
 		dev_err(pdev->dev, "resume failed: %d\n", rc);
 
@@ -899,13 +899,13 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	/* Spill to DRAM feature uses separate SMU message port */
 	dev->msg_port = 1;
 
-	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, 1);
+	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, true);
 	if (size != S2D_TELEMETRY_BYTES_MAX)
 		return -EIO;
 
 	/* Get STB DRAM address */
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, 1);
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, 1);
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
+	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
 
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
-- 
2.25.1

Re: [PATCH v3 1/4] platform/x86/amd: pmc: Pass true/false to bool argument

[Ilpo Järvinen]

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

On Tue, 16 May 2023, Shyam Sundar S K wrote:

> Pass true/false to the bool argument of the amd_pmc_send_cmd() function,
> instead of 1/0 to match the function signature.
> 
> 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: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.


> ---
>  drivers/platform/x86/amd/pmc.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 427905714f79..f28c295b47dd 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -261,7 +261,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	dev->msg_port = 1;
>  
>  	/* Get the num_samples to calculate the last push location */
> -	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, 1);
> +	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, true);
>  	/* Clear msg_port for other SMU operation */
>  	dev->msg_port = 0;
>  	if (ret) {
> @@ -317,15 +317,15 @@ static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
>  
>  	/* Get Active devices list from SMU */
>  	if (!dev->active_ips)
> -		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
> +		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, true);
>  
>  	/* Get dram address */
>  	if (!dev->smu_virt_addr) {
>  		u32 phys_addr_low, phys_addr_hi;
>  		u64 smu_phys_addr;
>  
> -		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
> -		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
> +		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, true);
> +		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, true);
>  		smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  		dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr,
> @@ -335,8 +335,8 @@ static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
>  	}
>  
>  	/* Start the logging */
> -	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, 0);
> -	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, 0);
> +	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, false);
> +	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, false);
>  
>  	return 0;
>  }
> @@ -377,7 +377,7 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
>  	if (dev->cpu_id == AMD_CPU_ID_PCO)
>  		return -ENODEV;
>  
> -	rc = amd_pmc_send_cmd(dev, 0, &val, SMU_MSG_GETSMUVERSION, 1);
> +	rc = amd_pmc_send_cmd(dev, 0, &val, SMU_MSG_GETSMUVERSION, true);
>  	if (rc)
>  		return rc;
>  
> @@ -794,7 +794,7 @@ static void amd_pmc_s2idle_prepare(void)
>  	}
>  
>  	msg = amd_pmc_get_os_hint(pdev);
> -	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
> +	rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, false);
>  	if (rc) {
>  		dev_err(pdev->dev, "suspend failed: %d\n", rc);
>  		return;
> @@ -829,7 +829,7 @@ static int amd_pmc_dump_data(struct amd_pmc_dev *pdev)
>  	if (pdev->cpu_id == AMD_CPU_ID_PCO)
>  		return -ENODEV;
>  
> -	return amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, 0);
> +	return amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, false);
>  }
>  
>  static void amd_pmc_s2idle_restore(void)
> @@ -839,7 +839,7 @@ static void amd_pmc_s2idle_restore(void)
>  	u8 msg;
>  
>  	msg = amd_pmc_get_os_hint(pdev);
> -	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, 0);
> +	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, false);
>  	if (rc)
>  		dev_err(pdev->dev, "resume failed: %d\n", rc);
>  
> @@ -899,13 +899,13 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	/* Spill to DRAM feature uses separate SMU message port */
>  	dev->msg_port = 1;
>  
> -	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, 1);
> +	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, true);
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>  		return -EIO;
>  
>  	/* Get STB DRAM address */
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, 1);
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, 1);
> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
> +	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
>  
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);

[PATCH v3 2/4] platform/x86/amd: pmc: Get STB DRAM size from PMFW

[Shyam Sundar S K]

Recent PMFW's have support for querying the STB DRAM size. 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.c | 37 +++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index f28c295b47dd..e2439fda5c02 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -115,6 +115,7 @@ enum s2d_arg {
 	S2D_PHYS_ADDR_LOW,
 	S2D_PHYS_ADDR_HIGH,
 	S2D_NUM_SAMPLES,
+	S2D_DRAM_SIZE,
 };
 
 struct amd_pmc_bit_map {
@@ -147,6 +148,7 @@ struct amd_pmc_dev {
 	u32 base_addr;
 	u32 cpu_id;
 	u32 active_ips;
+	u32 dram_size;
 /* SMU version information */
 	u8 smu_program;
 	u8 major;
@@ -890,11 +892,39 @@ static const struct pci_device_id pmc_pci_ids[] = {
 	{ }
 };
 
+static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
+{
+	int ret;
+
+	switch (dev->cpu_id) {
+	case AMD_CPU_ID_YC:
+		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {
+			ret = -EINVAL;
+			goto err_dram_size;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		goto err_dram_size;
+	}
+
+	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, STB_SPILL_TO_DRAM, true);
+	if (ret || !dev->dram_size)
+		goto err_dram_size;
+
+	return 0;
+
+err_dram_size:
+	dev_err(dev->dev, "DRAM size command not supported for this platform\n");
+	return ret;
+}
+
 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;
@@ -903,6 +933,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	if (size != S2D_TELEMETRY_BYTES_MAX)
 		return -EIO;
 
+	/* Get DRAM size */
+	ret = amd_pmc_get_dram_size(dev);
+	if (ret)
+		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
+
 	/* Get STB DRAM address */
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
 	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
@@ -912,7 +947,7 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	/* Clear msg_port for other SMU operation */
 	dev->msg_port = 0;
 
-	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, S2D_TELEMETRY_DRAMBYTES_MAX);
+	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
 	if (!dev->stb_virt_addr)
 		return -ENOMEM;
 
-- 
2.25.1

Re: [PATCH v3 2/4] platform/x86/amd: pmc: Get STB DRAM size from PMFW

[Ilpo Järvinen]

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

On Tue, 16 May 2023, Shyam Sundar S K wrote:

> Recent PMFW's have support for querying the STB DRAM size. 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>

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

-- 
 i.

> ---
>  drivers/platform/x86/amd/pmc.c | 37 +++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index f28c295b47dd..e2439fda5c02 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -115,6 +115,7 @@ enum s2d_arg {
>  	S2D_PHYS_ADDR_LOW,
>  	S2D_PHYS_ADDR_HIGH,
>  	S2D_NUM_SAMPLES,
> +	S2D_DRAM_SIZE,
>  };
>  
>  struct amd_pmc_bit_map {
> @@ -147,6 +148,7 @@ struct amd_pmc_dev {
>  	u32 base_addr;
>  	u32 cpu_id;
>  	u32 active_ips;
> +	u32 dram_size;
>  /* SMU version information */
>  	u8 smu_program;
>  	u8 major;
> @@ -890,11 +892,39 @@ static const struct pci_device_id pmc_pci_ids[] = {
>  	{ }
>  };
>  
> +static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
> +{
> +	int ret;
> +
> +	switch (dev->cpu_id) {
> +	case AMD_CPU_ID_YC:
> +		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {
> +			ret = -EINVAL;
> +			goto err_dram_size;
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err_dram_size;
> +	}
> +
> +	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, STB_SPILL_TO_DRAM, true);
> +	if (ret || !dev->dram_size)
> +		goto err_dram_size;
> +
> +	return 0;
> +
> +err_dram_size:
> +	dev_err(dev->dev, "DRAM size command not supported for this platform\n");
> +	return ret;
> +}
> +
>  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;
> @@ -903,6 +933,11 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>  		return -EIO;
>  
> +	/* Get DRAM size */
> +	ret = amd_pmc_get_dram_size(dev);
> +	if (ret)
> +		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
> +
>  	/* Get STB DRAM address */
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
>  	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
> @@ -912,7 +947,7 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	/* Clear msg_port for other SMU operation */
>  	dev->msg_port = 0;
>  
> -	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, S2D_TELEMETRY_DRAMBYTES_MAX);
> +	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>  	if (!dev->stb_virt_addr)
>  		return -ENOMEM;
>  
> 

[PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Shyam Sundar S K]

Add a helper routine to check the underlying cpu id, that can be used
across the PMC driver to remove the duplicate code.

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.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index e2439fda5c02..7e5e6afb3410 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
 	debugfs_remove_recursive(dev->dbgfs_dir);
 }
 
+static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
+{
+	switch (dev->cpu_id) {
+	case AMD_CPU_ID_YC:
+	case AMD_CPU_ID_CB:
+	case AMD_CPU_ID_PS:
+		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);
@@ -575,8 +587,7 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 			    &amd_pmc_idlemask_fops);
 	/* Enable STB only when the module_param is set */
 	if (enable_stb) {
-		if (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB ||
-		    dev->cpu_id == AMD_CPU_ID_PS)
+		if (amd_pmc_check_sup_cpuid(dev))
 			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
 					    &amd_pmc_stb_debugfs_fops_v2);
 		else
@@ -1036,7 +1047,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 
 	mutex_init(&dev->lock);
 
-	if (enable_stb && (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB)) {
+	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
 		err = amd_pmc_s2d_init(dev);
 		if (err)
 			goto err_pci_dev_put;
-- 
2.25.1

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Ilpo Järvinen]

On Tue, 16 May 2023, Shyam Sundar S K wrote:

> Add a helper routine to check the underlying cpu id, that can be used
> across the PMC driver to remove the duplicate code.
> 
> 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.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index e2439fda5c02..7e5e6afb3410 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>  	debugfs_remove_recursive(dev->dbgfs_dir);
>  }
>  
> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)

Does sup refer to "supported" or some other acronym? If the latter, 
you should mention/open it in the changelog and/or in a comment. If the 
former, the function naming seems too generic (an observation entirely 
based on how/where the function is used, you're not exactly verbose on 
what this actually checks for other than what looks like a set of CPU 
IDs but clearly there's more behind it).

-- 
 i.


> +{
> +	switch (dev->cpu_id) {
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_CB:
> +	case AMD_CPU_ID_PS:
> +		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);
> @@ -575,8 +587,7 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  			    &amd_pmc_idlemask_fops);
>  	/* Enable STB only when the module_param is set */
>  	if (enable_stb) {
> -		if (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB ||
> -		    dev->cpu_id == AMD_CPU_ID_PS)
> +		if (amd_pmc_check_sup_cpuid(dev))
>  			debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>  					    &amd_pmc_stb_debugfs_fops_v2);
>  		else
> @@ -1036,7 +1047,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  
>  	mutex_init(&dev->lock);
>  
> -	if (enable_stb && (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB)) {
> +	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
>  		err = amd_pmc_s2d_init(dev);
>  		if (err)
>  			goto err_pci_dev_put;
> 

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Shyam Sundar S K]

On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
> On Tue, 16 May 2023, Shyam Sundar S K wrote:
> 
>> Add a helper routine to check the underlying cpu id, that can be used
>> across the PMC driver to remove the duplicate code.
>>
>> 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.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>> index e2439fda5c02..7e5e6afb3410 100644
>> --- a/drivers/platform/x86/amd/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc.c
>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>  }
>>  
>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
> 
> Does sup refer to "supported" or some other acronym? If the latter,

Yes, please read that as "supported"

> you should mention/open it in the changelog and/or in a comment. If the 
> former, the function naming seems too generic (an observation entirely 
> based on how/where the function is used, you're not exactly verbose on 
> what this actually checks for other than what looks like a set of CPU 
> IDs but clearly there's more behind it).

OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?

> 

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Hans de Goede]

Hi Shyam,

On 5/25/23 11:46, Shyam Sundar S K wrote:
> 
> 
> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
>>
>>> Add a helper routine to check the underlying cpu id, that can be used
>>> across the PMC driver to remove the duplicate code.
>>>
>>> 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.c | 17 ++++++++++++++---
>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>>> index e2439fda5c02..7e5e6afb3410 100644
>>> --- a/drivers/platform/x86/amd/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc.c
>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>>  }
>>>  
>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
>>
>> Does sup refer to "supported" or some other acronym? If the latter,
> 
> Yes, please read that as "supported"
> 
>> you should mention/open it in the changelog and/or in a comment. If the 
>> former, the function naming seems too generic (an observation entirely 
>> based on how/where the function is used, you're not exactly verbose on 
>> what this actually checks for other than what looks like a set of CPU 
>> IDs but clearly there's more behind it).
> 
> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?

Yes that should be fine.

Regards,

Hans

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Ilpo Järvinen]

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

On Thu, 25 May 2023, Shyam Sundar S K wrote:
> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
> > On Tue, 16 May 2023, Shyam Sundar S K wrote:
> > 
> >> Add a helper routine to check the underlying cpu id, that can be used
> >> across the PMC driver to remove the duplicate code.
> >>
> >> 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.c | 17 ++++++++++++++---
> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> >> index e2439fda5c02..7e5e6afb3410 100644
> >> --- a/drivers/platform/x86/amd/pmc.c
> >> +++ b/drivers/platform/x86/amd/pmc.c
> >> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
> >>  	debugfs_remove_recursive(dev->dbgfs_dir);
> >>  }
> >>  
> >> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
> > 
> > Does sup refer to "supported" or some other acronym? If the latter,
> 
> Yes, please read that as "supported"
> 
> > you should mention/open it in the changelog and/or in a comment. If the 
> > former, the function naming seems too generic (an observation entirely 
> > based on how/where the function is used, you're not exactly verbose on 
> > what this actually checks for other than what looks like a set of CPU 
> > IDs but clearly there's more behind it).
> 
> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?

This makes things odder, it gets used in two places:

	if (enable_stb) {
		if (amd_pmc_check_sup_cpuid(dev))
			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
		else
			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
	}

What about that else branch (PMC is not supported so who does that make 
sense when the file is called pmc.c)? And here:

static int amd_pmc_probe(...)
{
	...
	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
		err = amd_pmc_s2d_init(dev);
		if (err)
			...goto + returns error
	}


If enable_stb is not set, pmc not being supported is not going to return 
error?


-- 
 i.

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Shyam Sundar S K]

Hi Ilpo,

On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
>>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
>>>
>>>> Add a helper routine to check the underlying cpu id, that can be used
>>>> across the PMC driver to remove the duplicate code.
>>>>
>>>> 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.c | 17 ++++++++++++++---
>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>>>> index e2439fda5c02..7e5e6afb3410 100644
>>>> --- a/drivers/platform/x86/amd/pmc.c
>>>> +++ b/drivers/platform/x86/amd/pmc.c
>>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>>>  }
>>>>  
>>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
>>>
>>> Does sup refer to "supported" or some other acronym? If the latter,
>>
>> Yes, please read that as "supported"
>>
>>> you should mention/open it in the changelog and/or in a comment. If the 
>>> former, the function naming seems too generic (an observation entirely 
>>> based on how/where the function is used, you're not exactly verbose on 
>>> what this actually checks for other than what looks like a set of CPU 
>>> IDs but clearly there's more behind it).
>>
>> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
> 
> This makes things odder, it gets used in two places:
> 
> 	if (enable_stb) {
> 		if (amd_pmc_check_sup_cpuid(dev))
> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
> 		else
> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
> 	}
> 
> What about that else branch (PMC is not supported so who does that make 
> sense when the file is called pmc.c)? And here:

I did not understand the actual concern. STB is an on-demand debug
feature and that can only be enabled when enable_stb module param is set.

The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
(with the right PMFW support) supported is pre-Rembrandt, then Spill to
DRAM is not supported. So reading the STB buffer is a different
mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().

But the platforms after Rmebrandt, supports spilling to DRAM, and that
has been handled in amd_pmc_stb_debugfs_fops_v2().

What am I missing in your comments?


> 
> static int amd_pmc_probe(...)
> {
> 	...
> 	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
> 		err = amd_pmc_s2d_init(dev);
> 		if (err)
> 			...goto + returns error
> 	}
> 
> 
> If enable_stb is not set, pmc not being supported is not going to return 
> error?
> 
> 

here we return only whne there is failure in s2d_init() - right?

And yes, if enable_stb is not set, there is no need to init the s2d path.

Thanks,
Shyam

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Ilpo Järvinen]

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

On Thu, 25 May 2023, Shyam Sundar S K wrote:
> On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
> > On Thu, 25 May 2023, Shyam Sundar S K wrote:
> >> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
> >>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
> >>>
> >>>> Add a helper routine to check the underlying cpu id, that can be used
> >>>> across the PMC driver to remove the duplicate code.
> >>>>
> >>>> 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.c | 17 ++++++++++++++---
> >>>>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> >>>> index e2439fda5c02..7e5e6afb3410 100644
> >>>> --- a/drivers/platform/x86/amd/pmc.c
> >>>> +++ b/drivers/platform/x86/amd/pmc.c
> >>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
> >>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
> >>>>  }
> >>>>  
> >>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
> >>>
> >>> Does sup refer to "supported" or some other acronym? If the latter,
> >>
> >> Yes, please read that as "supported"
> >>
> >>> you should mention/open it in the changelog and/or in a comment. If the 
> >>> former, the function naming seems too generic (an observation entirely 
> >>> based on how/where the function is used, you're not exactly verbose on 
> >>> what this actually checks for other than what looks like a set of CPU 
> >>> IDs but clearly there's more behind it).
> >>
> >> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
> > 
> > This makes things odder, it gets used in two places:
> > 
> > 	if (enable_stb) {
> > 		if (amd_pmc_check_sup_cpuid(dev))
> > 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
> > 		else
> > 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
> > 	}
> > 
> > What about that else branch (PMC is not supported so who does that make 
> > sense when the file is called pmc.c)? And here:
> 
> I did not understand the actual concern.

The file is cammed pmc.c and states "AMD SoC Power Management Controller 
Driver", so PMC, right?

You propose adding function called amd_pmc_is_cpu_supported() which to me 
reads "is PMC supported on this CPU?" since you don't have anything else 
in the function name to quality a sub-feature that would be be tested for 
supported or not.

It begs a question, why probe doesn't always return error when PMC is not 
supported by the CPU? Can you see the problem now?

> STB is an on-demand debug
> feature and that can only be enabled when enable_stb module param is set.
> 
> The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
> (with the right PMFW support) supported is pre-Rembrandt, then Spill to
> DRAM is not supported. So reading the STB buffer is a different
> mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().
> But the platforms after Rmebrandt, supports spilling to DRAM, and that
> has been handled in amd_pmc_stb_debugfs_fops_v2().

This kind of information should be stated the changelog up front.

So is that function testing support for Spill to DRAM? Clearly, 
Spill-to-DRAM != PMC, that's the second problem here related to function 
naming.

> What am I missing in your comments?
> 
> 
> > 
> > static int amd_pmc_probe(...)
> > {
> > 	...
> > 	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
> > 		err = amd_pmc_s2d_init(dev);
> > 		if (err)
> > 			...goto + returns error
> > 	}
> > 
> > 
> > If enable_stb is not set, pmc not being supported is not going to return 
> > error?
> > 
> > 
> 
> here we return only whne there is failure in s2d_init() - right?
> 
> And yes, if enable_stb is not set, there is no need to init the s2d path.

s2d is short for Spill to DRAM I guess?

So in both occassions amd_pmc_check_sup_cpuid() testing support for s2d 
rather than PMC (it certainly looks that way)? If so, name the function 
accordingly (I suggest amd_pmc_s2d_supported()) and put a little bit more 
explanation into the changelog and we're done here.


-- 
 i.

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Shyam Sundar S K]

Hi Ilpo,

On 5/25/2023 4:14 PM, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>> On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
>>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>>>> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
>>>>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
>>>>>
>>>>>> Add a helper routine to check the underlying cpu id, that can be used
>>>>>> across the PMC driver to remove the duplicate code.
>>>>>>
>>>>>> 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.c | 17 ++++++++++++++---
>>>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>>>>>> index e2439fda5c02..7e5e6afb3410 100644
>>>>>> --- a/drivers/platform/x86/amd/pmc.c
>>>>>> +++ b/drivers/platform/x86/amd/pmc.c
>>>>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>>>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>>>>>  }
>>>>>>  
>>>>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
>>>>>
>>>>> Does sup refer to "supported" or some other acronym? If the latter,
>>>>
>>>> Yes, please read that as "supported"
>>>>
>>>>> you should mention/open it in the changelog and/or in a comment. If the 
>>>>> former, the function naming seems too generic (an observation entirely 
>>>>> based on how/where the function is used, you're not exactly verbose on 
>>>>> what this actually checks for other than what looks like a set of CPU 
>>>>> IDs but clearly there's more behind it).
>>>>
>>>> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
>>>
>>> This makes things odder, it gets used in two places:
>>>
>>> 	if (enable_stb) {
>>> 		if (amd_pmc_check_sup_cpuid(dev))
>>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
>>> 		else
>>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
>>> 	}
>>>
>>> What about that else branch (PMC is not supported so who does that make 
>>> sense when the file is called pmc.c)? And here:
>>
>> I did not understand the actual concern.
> 
> The file is cammed pmc.c and states "AMD SoC Power Management Controller 
> Driver", so PMC, right?

Yes.

> 
> You propose adding function called amd_pmc_is_cpu_supported() which to me 
> reads "is PMC supported on this CPU?" since you don't have anything else 
> in the function name to quality a sub-feature that would be be tested for 
> supported or not.

The function naming convention across this file is amd_pmc_*. So would
like to have it as "amd_pmc_is_cpu_supported()". And yes, this should be
generic helper function that should be used across the PMC and STB
functions interchangeably, as the underlying CPU where it runs remains
the same.

> 
> It begs a question, why probe doesn't always return error when PMC is not 
> supported by the CPU? Can you see the problem now?

PMC driver probe happens based on the _HID amd_pmc_acpi_ids[] and probe
failures are handled.

The only intention to look for CPU ID's through the PCI root port is
handle the Firmware changes across CPU generations.

Hope this clears the question.

> 
>> STB is an on-demand debug
>> feature and that can only be enabled when enable_stb module param is set.
>>
>> The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
>> (with the right PMFW support) supported is pre-Rembrandt, then Spill to
>> DRAM is not supported. So reading the STB buffer is a different
>> mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().
>> But the platforms after Rmebrandt, supports spilling to DRAM, and that
>> has been handled in amd_pmc_stb_debugfs_fops_v2().
> 
> This kind of information should be stated the changelog up front.

I don't think I am touching that part of the code to explain this stuff
in the changelog.

Let us purely keep this as a helper function to check the underlying CPU
that can be used across the entire pmc.c file. Makes sense?

> 
> So is that function testing support for Spill to DRAM? Clearly, 
> Spill-to-DRAM != PMC, that's the second problem here related to function 
> naming.

PMC driver is a FW assisted driver for S2Idle path on AMD platforms. But
for whatever reason if the Supend/resume fails to happen, we need to
have a debug mechanims to address the field issues.

Since it's FW assisted, the FW also provides a way to know what happened
behind the scenes and that debug mechanism is called STB (Smart Trace
Buffer).

Intially when we started the STB was supposed to be a small interface
and it has evolved a lot over time. And maybe at times you will see that
PMC and STB are used in conjunction.

If its becoming confusing for the community, maybe I will come up with a
way to decouple PMC and STB sometime soon.

Thoughts?

> 
>> What am I missing in your comments?
>>
>>
>>>
>>> static int amd_pmc_probe(...)
>>> {
>>> 	...
>>> 	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
>>> 		err = amd_pmc_s2d_init(dev);
>>> 		if (err)
>>> 			...goto + returns error
>>> 	}
>>>
>>>
>>> If enable_stb is not set, pmc not being supported is not going to return 
>>> error?
>>>
>>>
>>
>> here we return only whne there is failure in s2d_init() - right?
>>
>> And yes, if enable_stb is not set, there is no need to init the s2d path.
> 
> s2d is short for Spill to DRAM I guess?

Yes.

> 
> So in both occassions amd_pmc_check_sup_cpuid() testing support for s2d 
> rather than PMC (it certainly looks that way)? If so, name the function 
> accordingly (I suggest amd_pmc_s2d_supported()) and put a little bit more 
> explanation into the changelog and we're done here.
> 
> 

IMHO. Based on the above details, amd_pmc_is_cpu_supported() should be
generic name. Do you still see a concern?

Thanks,
Shyam

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Ilpo Järvinen]

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

On Thu, 25 May 2023, Shyam Sundar S K wrote:
> On 5/25/2023 4:14 PM, Ilpo Järvinen wrote:
> > On Thu, 25 May 2023, Shyam Sundar S K wrote:
> >> On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
> >>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
> >>>> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
> >>>>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
> >>>>>
> >>>>>> Add a helper routine to check the underlying cpu id, that can be used
> >>>>>> across the PMC driver to remove the duplicate code.
> >>>>>>
> >>>>>> 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.c | 17 ++++++++++++++---
> >>>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> >>>>>> index e2439fda5c02..7e5e6afb3410 100644
> >>>>>> --- a/drivers/platform/x86/amd/pmc.c
> >>>>>> +++ b/drivers/platform/x86/amd/pmc.c
> >>>>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
> >>>>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
> >>>>>>  }
> >>>>>>  
> >>>>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
> >>>>>
> >>>>> Does sup refer to "supported" or some other acronym? If the latter,
> >>>>
> >>>> Yes, please read that as "supported"
> >>>>
> >>>>> you should mention/open it in the changelog and/or in a comment. If the 
> >>>>> former, the function naming seems too generic (an observation entirely 
> >>>>> based on how/where the function is used, you're not exactly verbose on 
> >>>>> what this actually checks for other than what looks like a set of CPU 
> >>>>> IDs but clearly there's more behind it).
> >>>>
> >>>> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
> >>>
> >>> This makes things odder, it gets used in two places:
> >>>
> >>> 	if (enable_stb) {
> >>> 		if (amd_pmc_check_sup_cpuid(dev))
> >>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
> >>> 		else
> >>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
> >>> 	}
> >>>
> >>> What about that else branch (PMC is not supported so who does that make 
> >>> sense when the file is called pmc.c)? And here:
> >>
> >> I did not understand the actual concern.
> > 
> > The file is cammed pmc.c and states "AMD SoC Power Management Controller 
> > Driver", so PMC, right?
> 
> Yes.
> 
> > 
> > You propose adding function called amd_pmc_is_cpu_supported() which to me 
> > reads "is PMC supported on this CPU?" since you don't have anything else 
> > in the function name to quality a sub-feature that would be be tested for 
> > supported or not.
> 
> The function naming convention across this file is amd_pmc_*. So would
> like to have it as "amd_pmc_is_cpu_supported()". And yes, this should be
> generic helper function that should be used across the PMC and STB
> functions interchangeably, as the underlying CPU where it runs remains
> the same.

Okay, so I read this as stating that its testing for a larger set of 
features than what can be read from the code.

> > It begs a question, why probe doesn't always return error when PMC is not 
> > supported by the CPU? Can you see the problem now?
> 
> PMC driver probe happens based on the _HID amd_pmc_acpi_ids[] and probe
> failures are handled.
> 
> The only intention to look for CPU ID's through the PCI root port is
> handle the Firmware changes across CPU generations.
>
> Hope this clears the question.
> 
> > 
> >> STB is an on-demand debug
> >> feature and that can only be enabled when enable_stb module param is set.
> >>
> >> The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
> >> (with the right PMFW support) supported is pre-Rembrandt, then Spill to
> >> DRAM is not supported. So reading the STB buffer is a different
> >> mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().
> >> But the platforms after Rmebrandt, supports spilling to DRAM, and that
> >> has been handled in amd_pmc_stb_debugfs_fops_v2().
> > 
> > This kind of information should be stated the changelog up front.
> 
> I don't think I am touching that part of the code to explain this stuff
> in the changelog.
> 
> Let us purely keep this as a helper function to check the underlying CPU
> that can be used across the entire pmc.c file. Makes sense?
> 
> > 
> > So is that function testing support for Spill to DRAM? Clearly, 
> > Spill-to-DRAM != PMC, that's the second problem here related to function 
> > naming.
> 
> PMC driver is a FW assisted driver for S2Idle path on AMD platforms. But
> for whatever reason if the Supend/resume fails to happen, we need to
> have a debug mechanims to address the field issues.
> 
> Since it's FW assisted, the FW also provides a way to know what happened
> behind the scenes and that debug mechanism is called STB (Smart Trace
> Buffer).
> 
> Intially when we started the STB was supposed to be a small interface
> and it has evolved a lot over time. And maybe at times you will see that
> PMC and STB are used in conjunction.
> 
> If its becoming confusing for the community, maybe I will come up with a
> way to decouple PMC and STB sometime soon.
> 
> Thoughts?

This kinda digressed and didn't answer my question at all (you mentioned 
Spill to DRAM zero times in your reply but went to something called STB). 
But it could just that I'm not familiar enough with all the details here.

My question boiled down to if this (your own words) is true or not:

"The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
(with the right PMFW support) supported is pre-Rembrandt, then Spill 
to DRAM is not supported. ... But the platforms after Rmebrandt, supports 
spilling to DRAM, ..."

If the code is solely used for testing whether Spill to DRAM is supported 
or not, it feels odd to name it something more generic than that. But 
given what you said above, I guess the answer here is that it can be used 
to test Spill to DRAM among other things, and this particular patch 
just doesn't do those other things so it looks odd but is okay still.
Is that the correct interpretation here?

> >>> static int amd_pmc_probe(...)
> >>> {
> >>> 	...
> >>> 	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
> >>> 		err = amd_pmc_s2d_init(dev);
> >>> 		if (err)
> >>> 			...goto + returns error
> >>> 	}
> >>>
> >>>
> >>> If enable_stb is not set, pmc not being supported is not going to return 
> >>> error?
> >>>
> >>>
> >>
> >> here we return only whne there is failure in s2d_init() - right?
> >>
> >> And yes, if enable_stb is not set, there is no need to init the s2d path.
> > 
> > s2d is short for Spill to DRAM I guess?
> 
> Yes.
> 
> > 
> > So in both occassions amd_pmc_check_sup_cpuid() testing support for s2d 
> > rather than PMC (it certainly looks that way)? If so, name the function 
> > accordingly (I suggest amd_pmc_s2d_supported()) and put a little bit more 
> > explanation into the changelog and we're done here.
> > 
> > 
> 
> IMHO. Based on the above details, amd_pmc_is_cpu_supported() should be
> generic name. Do you still see a concern?

Given you seem to be certain there's no error or some detail missing, I 
won't object it being the way you want to put it.


-- 
 i.

Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

[Shyam Sundar S K]

Hi Ilpo,

On 5/25/2023 5:28 PM, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>> On 5/25/2023 4:14 PM, Ilpo Järvinen wrote:
>>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>>>> On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
>>>>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>>>>>> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
>>>>>>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
>>>>>>>
>>>>>>>> Add a helper routine to check the underlying cpu id, that can be used
>>>>>>>> across the PMC driver to remove the duplicate code.
>>>>>>>>
>>>>>>>> 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.c | 17 ++++++++++++++---
>>>>>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>>>>>>>> index e2439fda5c02..7e5e6afb3410 100644
>>>>>>>> --- a/drivers/platform/x86/amd/pmc.c
>>>>>>>> +++ b/drivers/platform/x86/amd/pmc.c
>>>>>>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>>>>>>>  	debugfs_remove_recursive(dev->dbgfs_dir);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
>>>>>>>
>>>>>>> Does sup refer to "supported" or some other acronym? If the latter,
>>>>>>
>>>>>> Yes, please read that as "supported"
>>>>>>
>>>>>>> you should mention/open it in the changelog and/or in a comment. If the 
>>>>>>> former, the function naming seems too generic (an observation entirely 
>>>>>>> based on how/where the function is used, you're not exactly verbose on 
>>>>>>> what this actually checks for other than what looks like a set of CPU 
>>>>>>> IDs but clearly there's more behind it).
>>>>>>
>>>>>> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
>>>>>
>>>>> This makes things odder, it gets used in two places:
>>>>>
>>>>> 	if (enable_stb) {
>>>>> 		if (amd_pmc_check_sup_cpuid(dev))
>>>>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
>>>>> 		else
>>>>> 			debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
>>>>> 	}
>>>>>
>>>>> What about that else branch (PMC is not supported so who does that make 
>>>>> sense when the file is called pmc.c)? And here:
>>>>
>>>> I did not understand the actual concern.
>>>
>>> The file is cammed pmc.c and states "AMD SoC Power Management Controller 
>>> Driver", so PMC, right?
>>
>> Yes.
>>
>>>
>>> You propose adding function called amd_pmc_is_cpu_supported() which to me 
>>> reads "is PMC supported on this CPU?" since you don't have anything else 
>>> in the function name to quality a sub-feature that would be be tested for 
>>> supported or not.
>>
>> The function naming convention across this file is amd_pmc_*. So would
>> like to have it as "amd_pmc_is_cpu_supported()". And yes, this should be
>> generic helper function that should be used across the PMC and STB
>> functions interchangeably, as the underlying CPU where it runs remains
>> the same.
> 
> Okay, so I read this as stating that its testing for a larger set of 
> features than what can be read from the code.
> 
>>> It begs a question, why probe doesn't always return error when PMC is not 
>>> supported by the CPU? Can you see the problem now?
>>
>> PMC driver probe happens based on the _HID amd_pmc_acpi_ids[] and probe
>> failures are handled.
>>
>> The only intention to look for CPU ID's through the PCI root port is
>> handle the Firmware changes across CPU generations.
>>
>> Hope this clears the question.
>>
>>>
>>>> STB is an on-demand debug
>>>> feature and that can only be enabled when enable_stb module param is set.
>>>>
>>>> The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
>>>> (with the right PMFW support) supported is pre-Rembrandt, then Spill to
>>>> DRAM is not supported. So reading the STB buffer is a different
>>>> mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().
>>>> But the platforms after Rmebrandt, supports spilling to DRAM, and that
>>>> has been handled in amd_pmc_stb_debugfs_fops_v2().
>>>
>>> This kind of information should be stated the changelog up front.
>>
>> I don't think I am touching that part of the code to explain this stuff
>> in the changelog.
>>
>> Let us purely keep this as a helper function to check the underlying CPU
>> that can be used across the entire pmc.c file. Makes sense?
>>
>>>
>>> So is that function testing support for Spill to DRAM? Clearly, 
>>> Spill-to-DRAM != PMC, that's the second problem here related to function 
>>> naming.
>>
>> PMC driver is a FW assisted driver for S2Idle path on AMD platforms. But
>> for whatever reason if the Supend/resume fails to happen, we need to
>> have a debug mechanims to address the field issues.
>>
>> Since it's FW assisted, the FW also provides a way to know what happened
>> behind the scenes and that debug mechanism is called STB (Smart Trace
>> Buffer).
>>
>> Intially when we started the STB was supposed to be a small interface
>> and it has evolved a lot over time. And maybe at times you will see that
>> PMC and STB are used in conjunction.
>>
>> If its becoming confusing for the community, maybe I will come up with a
>> way to decouple PMC and STB sometime soon.
>>
>> Thoughts?
> 
> This kinda digressed and didn't answer my question at all (you mentioned 
> Spill to DRAM zero times in your reply but went to something called STB). 
> But it could just that I'm not familiar enough with all the details here.
> 
> My question boiled down to if this (your own words) is true or not:
> 
> "The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
> (with the right PMFW support) supported is pre-Rembrandt, then Spill 
> to DRAM is not supported. ... But the platforms after Rmebrandt, supports 
> spilling to DRAM, ..."
> 
> If the code is solely used for testing whether Spill to DRAM is supported 
> or not, it feels odd to name it something more generic than that. But 
> given what you said above, I guess the answer here is that it can be used 
> to test Spill to DRAM among other things, and this particular patch 
> just doesn't do those other things so it looks odd but is okay still.
> Is that the correct interpretation here?
> 

There are 3 parts to it:

a) PMC driver[1] used without STB enabled
b) PMC driver used with STB enabled (but on older platforms before to
Rembrandt - where Spilling to DRAM is *not supported*[3])
c) PMC driver used with STB enabled (with platforms starting Rembrandt
and later - where the spilling to DRAM is *supported* [4])

Since we would need helper checks across all 3 cases, it should be okay
to name it as amd_pmc_is_cpu_supported().

[1] which is meant for s2idle suspend/resume transitions
[2] STB (Smart Trace Buffer) which is a shared buffer across several IP
blocks within the SoC.
[3] handled in amd_pmc_stb_debugfs_fops
[4] handled in amd_pmc_stb_debugfs_fops_v2




>>>>> static int amd_pmc_probe(...)
>>>>> {
>>>>> 	...
>>>>> 	if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
>>>>> 		err = amd_pmc_s2d_init(dev);
>>>>> 		if (err)
>>>>> 			...goto + returns error
>>>>> 	}
>>>>>
>>>>>
>>>>> If enable_stb is not set, pmc not being supported is not going to return 
>>>>> error?
>>>>>
>>>>>
>>>>
>>>> here we return only whne there is failure in s2d_init() - right?
>>>>
>>>> And yes, if enable_stb is not set, there is no need to init the s2d path.
>>>
>>> s2d is short for Spill to DRAM I guess?
>>
>> Yes.
>>
>>>
>>> So in both occassions amd_pmc_check_sup_cpuid() testing support for s2d 
>>> rather than PMC (it certainly looks that way)? If so, name the function 
>>> accordingly (I suggest amd_pmc_s2d_supported()) and put a little bit more 
>>> explanation into the changelog and we're done here.
>>>
>>>
>>
>> IMHO. Based on the above details, amd_pmc_is_cpu_supported() should be
>> generic name. Do you still see a concern?
> 
> Given you seem to be certain there's no error or some detail missing, I 
> won't object it being the way you want to put it.
> 
> 

Thank you, have sent a v4.

Thanks,
Shyam

[PATCH v3 4/4] platform/x86/amd: pmc: Update metrics table info for Pink Sardine

[Shyam Sundar S K]

Starting from Pink Sardine, number of IP blocks were added to the SoC
and the PMFW has the ability to give debug stats on each the IP blocks
after a S0ix cycle within part of the SMU metrics table. Add this new
capability to the driver.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc.c | 53 ++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index 7e5e6afb3410..0e67325a5aec 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -45,7 +45,6 @@
 #define AMD_PMC_STB_DUMMY_PC		0xC6000007
 
 /* STB S2D(Spill to DRAM) has different message port offset */
-#define STB_SPILL_TO_DRAM		0xBE
 #define AMD_S2D_REGISTER_MESSAGE	0xA20
 #define AMD_S2D_REGISTER_RESPONSE	0xA80
 #define AMD_S2D_REGISTER_ARGUMENT	0xA88
@@ -99,7 +98,6 @@
 #define PMC_MSG_DELAY_MIN_US		50
 #define RESPONSE_REGISTER_LOOP_MAX	20000
 
-#define SOC_SUBSYSTEM_IP_MAX	12
 #define DELAY_MIN_US		2000
 #define DELAY_MAX_US		3000
 #define FIFO_SIZE		4096
@@ -133,9 +131,18 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
 	{"ISP",		BIT(6)},
 	{"NBIO",	BIT(7)},
 	{"DF",		BIT(8)},
-	{"USB0",	BIT(9)},
-	{"USB1",	BIT(10)},
+	{"USB3_0",	BIT(9)},
+	{"USB3_1",	BIT(10)},
 	{"LAPIC",	BIT(11)},
+	{"USB3_2",	BIT(12)},
+	{"USB3_3",	BIT(13)},
+	{"USB3_4",	BIT(14)},
+	{"USB4_0",	BIT(15)},
+	{"USB4_1",	BIT(16)},
+	{"MPM",		BIT(17)},
+	{"JPEG",	BIT(18)},
+	{"IPU",		BIT(19)},
+	{"UMSCH",	BIT(20)},
 	{}
 };
 
@@ -149,6 +156,8 @@ struct amd_pmc_dev {
 	u32 cpu_id;
 	u32 active_ips;
 	u32 dram_size;
+	u32 num_ips;
+	u32 s2d_msg_id;
 /* SMU version information */
 	u8 smu_program;
 	u8 major;
@@ -196,8 +205,8 @@ struct smu_metrics {
 	u64 timein_s0i3_totaltime;
 	u64 timein_swdrips_lastcapture;
 	u64 timein_swdrips_totaltime;
-	u64 timecondition_notmet_lastcapture[SOC_SUBSYSTEM_IP_MAX];
-	u64 timecondition_notmet_totaltime[SOC_SUBSYSTEM_IP_MAX];
+	u64 timecondition_notmet_lastcapture[32];
+	u64 timecondition_notmet_totaltime[32];
 } __packed;
 
 static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
@@ -263,7 +272,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	dev->msg_port = 1;
 
 	/* Get the num_samples to calculate the last push location */
-	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, true);
+	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) {
@@ -310,6 +319,23 @@ static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
 	.release = amd_pmc_stb_debugfs_release_v2,
 };
 
+static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
+{
+	switch (dev->cpu_id) {
+	case AMD_CPU_ID_PCO:
+	case AMD_CPU_ID_RN:
+	case AMD_CPU_ID_YC:
+	case AMD_CPU_ID_CB:
+		dev->num_ips = 12;
+		dev->s2d_msg_id = 0xBE;
+		break;
+	case AMD_CPU_ID_PS:
+		dev->num_ips = 21;
+		dev->s2d_msg_id = 0x85;
+		break;
+	}
+}
+
 static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
 {
 	if (dev->cpu_id == AMD_CPU_ID_PCO) {
@@ -471,7 +497,7 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
 		   table.timeto_resume_to_os_lastcapture);
 
 	seq_puts(s, "\n=== Active time (in us) ===\n");
-	for (idx = 0 ; idx < SOC_SUBSYSTEM_IP_MAX ; idx++) {
+	for (idx = 0 ; idx < dev->num_ips ; idx++) {
 		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]);
@@ -919,7 +945,7 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
 		goto err_dram_size;
 	}
 
-	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, STB_SPILL_TO_DRAM, true);
+	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
 	if (ret || !dev->dram_size)
 		goto err_dram_size;
 
@@ -940,7 +966,10 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 	/* Spill to DRAM feature uses separate SMU message port */
 	dev->msg_port = 1;
 
-	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, true);
+	/* Get num of IP blocks within the SoC */
+	amd_pmc_get_ip_info(dev);
+
+	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
 	if (size != S2D_TELEMETRY_BYTES_MAX)
 		return -EIO;
 
@@ -950,8 +979,8 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
 
 	/* Get STB DRAM address */
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
-	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
+	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);
 
-- 
2.25.1

Re: [PATCH v3 4/4] platform/x86/amd: pmc: Update metrics table info for Pink Sardine

[Ilpo Järvinen]

On Tue, 16 May 2023, Shyam Sundar S K wrote:

> Starting from Pink Sardine, number of IP blocks were added to the SoC
> and the PMFW has the ability to give debug stats on each the IP blocks
> after a S0ix cycle within part of the SMU metrics table. Add this new
> capability to the driver.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc.c | 53 ++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 7e5e6afb3410..0e67325a5aec 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -45,7 +45,6 @@
>  #define AMD_PMC_STB_DUMMY_PC		0xC6000007
>  
>  /* STB S2D(Spill to DRAM) has different message port offset */
> -#define STB_SPILL_TO_DRAM		0xBE
>  #define AMD_S2D_REGISTER_MESSAGE	0xA20
>  #define AMD_S2D_REGISTER_RESPONSE	0xA80
>  #define AMD_S2D_REGISTER_ARGUMENT	0xA88
> @@ -99,7 +98,6 @@
>  #define PMC_MSG_DELAY_MIN_US		50
>  #define RESPONSE_REGISTER_LOOP_MAX	20000
>  
> -#define SOC_SUBSYSTEM_IP_MAX	12
>  #define DELAY_MIN_US		2000
>  #define DELAY_MAX_US		3000
>  #define FIFO_SIZE		4096
> @@ -133,9 +131,18 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>  	{"ISP",		BIT(6)},
>  	{"NBIO",	BIT(7)},
>  	{"DF",		BIT(8)},
> -	{"USB0",	BIT(9)},
> -	{"USB1",	BIT(10)},
> +	{"USB3_0",	BIT(9)},
> +	{"USB3_1",	BIT(10)},
>  	{"LAPIC",	BIT(11)},
> +	{"USB3_2",	BIT(12)},
> +	{"USB3_3",	BIT(13)},
> +	{"USB3_4",	BIT(14)},
> +	{"USB4_0",	BIT(15)},
> +	{"USB4_1",	BIT(16)},
> +	{"MPM",		BIT(17)},
> +	{"JPEG",	BIT(18)},
> +	{"IPU",		BIT(19)},
> +	{"UMSCH",	BIT(20)},
>  	{}
>  };
>  
> @@ -149,6 +156,8 @@ struct amd_pmc_dev {
>  	u32 cpu_id;
>  	u32 active_ips;
>  	u32 dram_size;
> +	u32 num_ips;
> +	u32 s2d_msg_id;
>  /* SMU version information */
>  	u8 smu_program;
>  	u8 major;
> @@ -196,8 +205,8 @@ struct smu_metrics {
>  	u64 timein_s0i3_totaltime;
>  	u64 timein_swdrips_lastcapture;
>  	u64 timein_swdrips_totaltime;
> -	u64 timecondition_notmet_lastcapture[SOC_SUBSYSTEM_IP_MAX];
> -	u64 timecondition_notmet_totaltime[SOC_SUBSYSTEM_IP_MAX];
> +	u64 timecondition_notmet_lastcapture[32];
> +	u64 timecondition_notmet_totaltime[32];
>  } __packed;
>  
>  static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> @@ -263,7 +272,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	dev->msg_port = 1;
>  
>  	/* Get the num_samples to calculate the last push location */
> -	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, STB_SPILL_TO_DRAM, true);
> +	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) {
> @@ -310,6 +319,23 @@ static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
>  	.release = amd_pmc_stb_debugfs_release_v2,
>  };
>  
> +static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> +{
> +	switch (dev->cpu_id) {
> +	case AMD_CPU_ID_PCO:
> +	case AMD_CPU_ID_RN:
> +	case AMD_CPU_ID_YC:
> +	case AMD_CPU_ID_CB:
> +		dev->num_ips = 12;
> +		dev->s2d_msg_id = 0xBE;

Changelog doesn't seem to mention the addition of ->s2d_msg_id at all but 
only describes the num_ips change.

> +		break;
> +	case AMD_CPU_ID_PS:
> +		dev->num_ips = 21;
> +		dev->s2d_msg_id = 0x85;
> +		break;
> +	}
> +}
> +
>  static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
>  {
>  	if (dev->cpu_id == AMD_CPU_ID_PCO) {
> @@ -471,7 +497,7 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
>  		   table.timeto_resume_to_os_lastcapture);
>  
>  	seq_puts(s, "\n=== Active time (in us) ===\n");
> -	for (idx = 0 ; idx < SOC_SUBSYSTEM_IP_MAX ; idx++) {
> +	for (idx = 0 ; idx < dev->num_ips ; idx++) {
>  		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]);
> @@ -919,7 +945,7 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
>  		goto err_dram_size;
>  	}
>  
> -	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, STB_SPILL_TO_DRAM, true);
> +	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>  	if (ret || !dev->dram_size)
>  		goto err_dram_size;
>  
> @@ -940,7 +966,10 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	/* Spill to DRAM feature uses separate SMU message port */
>  	dev->msg_port = 1;
>  
> -	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, STB_SPILL_TO_DRAM, true);
> +	/* Get num of IP blocks within the SoC */
> +	amd_pmc_get_ip_info(dev);
> +
> +	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>  		return -EIO;
>  
> @@ -950,8 +979,8 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
>  
>  	/* Get STB DRAM address */
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low, STB_SPILL_TO_DRAM, true);
> -	amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi, STB_SPILL_TO_DRAM, true);
> +	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);
>  
> 

-- 
 i.

Re: [PATCH v3 0/4] Updates to AMD PMC driver

[Hans de Goede]

Hi Shyam,

On 5/16/23 11:13, Shyam Sundar S K wrote:
> Series includes fixes and feature updates to the PMC driver.
> Series has been split into 4 patches to incorporate the review
> remarks from v2.
> 
> v3:
>  - Based on review-hans branch
>  - Pass true/false instead of 1/0 the amd_pmc_send_cmd()
>  - Add helper for supported cpu id check as suggested by Ilpo.

Thank you for the new version.

Can you please prepare a v4 addressing Ilpo's small remarks
on patches 3/4 and 4/4 ?

Regards,

Hans




> 
> v2:
>  - Based on review-hans branch
>  - Add a switch for cpu-id check based on feedback from Mario.
> 
> Shyam Sundar S K (4):
>   platform/x86/amd: pmc: Pass true/false to bool argument
>   platform/x86/amd: pmc: Get STB DRAM size from PMFW
>   platform/x86/amd: pmc: Add helper function to check the cpu id
>   platform/x86/amd: pmc: Update metrics table info for Pink Sardine
> 
>  drivers/platform/x86/amd/pmc.c | 123 ++++++++++++++++++++++++++-------
>  1 file changed, 99 insertions(+), 24 deletions(-)
>