[PATCH 0/6] updates to amd-pmc driver

[PATCH 0/6] updates to amd-pmc driver

[Shyam Sundar S K]

Couple of existing issues on the completion codes to SMU
and a newer way to get the s0ix statistics are introduced.

Also, add acpi ids for current and future revisions of the
pmc controller.

Shyam Sundar S K (6):
  platform/x86: amd-pmc: Fix command completion code
  platform/x86: amd-pmc: Fix SMU firmware reporting mechanism
  platform/x86: amd-pmc: Add support for logging SMU metrics
  platform/x86: amd-pmc: Add support for logging s0ix counters
  platform/x86: amd-pmc: Add support for ACPI ID AMDI0006
  platform/x86: amd-pmc: Add new acpi id for future PMC controllers

 drivers/platform/x86/amd-pmc.c | 216 +++++++++++++++++++++++++++++----
 1 file changed, 195 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH 1/6] platform/x86: amd-pmc: Fix command completion code

[Shyam Sundar S K]

The protocol to submit a job request to SMU is to wait for
AMD_PMC_REGISTER_RESPONSE to return 1,meaning SMU is ready to take
requests. PMC driver has to make sure that the response code is always
AMD_PMC_RESULT_OK before making any command submissions.

Also, when we submit a message to SMU, we have to wait until it processes
the request. Adding a read_poll_timeout() check as this was missing in
the existing code.

Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index b9da58ee9b1e..9c8a53120767 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -140,7 +140,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
 
 	/* Wait until we get a valid response */
 	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
-				val, val > 0, PMC_MSG_DELAY_MIN_US,
+				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
 				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
 	if (rc) {
 		dev_err(dev->dev, "failed to talk to SMU\n");
@@ -156,6 +156,14 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
 	/* Write message ID to message ID register */
 	msg = (dev->cpu_id == AMD_CPU_ID_RN) ? MSG_OS_HINT_RN : MSG_OS_HINT_PCO;
 	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
+	/* Wait until we get a valid response */
+	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
+				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
+				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
+	if (rc) {
+		dev_err(dev->dev, "SMU response timed out\n");
+		return rc;
+	}
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 1/6] platform/x86: amd-pmc: Fix command completion code

[Hans de Goede]

Hi,

On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
> The protocol to submit a job request to SMU is to wait for
> AMD_PMC_REGISTER_RESPONSE to return 1,meaning SMU is ready to take
> requests. PMC driver has to make sure that the response code is always
> AMD_PMC_RESULT_OK before making any command submissions.
> 
> Also, when we submit a message to SMU, we have to wait until it processes
> the request. Adding a read_poll_timeout() check as this was missing in
> the existing code.
> 
> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/platform/x86/amd-pmc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index b9da58ee9b1e..9c8a53120767 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -140,7 +140,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>  
>  	/* Wait until we get a valid response */
>  	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
> -				val, val > 0, PMC_MSG_DELAY_MIN_US,
> +				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
>  				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>  	if (rc) {
>  		dev_err(dev->dev, "failed to talk to SMU\n");
> @@ -156,6 +156,14 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>  	/* Write message ID to message ID register */
>  	msg = (dev->cpu_id == AMD_CPU_ID_RN) ? MSG_OS_HINT_RN : MSG_OS_HINT_PCO;
>  	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
> +	/* Wait until we get a valid response */
> +	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
> +				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
> +				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
> +	if (rc) {
> +		dev_err(dev->dev, "SMU response timed out\n");
> +		return rc;
> +	}
>  	return 0;
>  }
>  
> 

Re: [PATCH 1/6] platform/x86: amd-pmc: Fix command completion code

[Raul E Rangel]

On Thu, Jun 17, 2021 at 05:00:35PM +0530, Shyam Sundar S K wrote:
> The protocol to submit a job request to SMU is to wait for
> AMD_PMC_REGISTER_RESPONSE to return 1,meaning SMU is ready to take
> requests. PMC driver has to make sure that the response code is always
> AMD_PMC_RESULT_OK before making any command submissions.
> 
> Also, when we submit a message to SMU, we have to wait until it processes
> the request. Adding a read_poll_timeout() check as this was missing in
> the existing code.
> 
> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/amd-pmc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index b9da58ee9b1e..9c8a53120767 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -140,7 +140,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>  
>  	/* Wait until we get a valid response */
>  	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
> -				val, val > 0, PMC_MSG_DELAY_MIN_US,
> +				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
Isn't `val > 0` correct? With your change we will continue to poll even
when the SMU returns `AMD_PMC_RESULT_FAILED` or any of the other return
codes.

Or does the response register always return a val > 0?
>  				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>  	if (rc) {
>  		dev_err(dev->dev, "failed to talk to SMU\n");

If you make the change suggested above, we should check  `val` after
checking `rc`:

	if (val != AMD_PMC_RESULT_OK) {
		dev_err(dev->dev, "SMU is not ready\n");
		return -EBUSY;
	}
> @@ -156,6 +156,14 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>  	/* Write message ID to message ID register */
>  	msg = (dev->cpu_id == AMD_CPU_ID_RN) ? MSG_OS_HINT_RN : MSG_OS_HINT_PCO;
>  	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
> +	/* Wait until we get a valid response */
> +	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
> +				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
> +				PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
> +	if (rc) {
> +		dev_err(dev->dev, "SMU response timed out\n");
> +		return rc;
> +	}
>  	return 0;
>  }
>  

[PATCH 2/6] platform/x86: amd-pmc: Fix SMU firmware reporting mechanism

[Shyam Sundar S K]

It was lately understood that the current mechanism available in the
driver to get SMU firmware info works only on internal SMU builds and
there is a separate way to get all the SMU logging counters (addressed
in the next patch). Hence remove all the smu info shown via debugfs as it
is no more useful.

Also, use dump registers routine only at one place i.e. after the command
submission to SMU is done.

Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 9c8a53120767..ce0e2ad94d09 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -52,7 +52,6 @@
 #define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
 #define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
 
-#define AMD_SMU_FW_VERSION		0x0
 #define PMC_MSG_DELAY_MIN_US		100
 #define RESPONSE_REGISTER_LOOP_MAX	200
 
@@ -88,11 +87,6 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
 #ifdef CONFIG_DEBUG_FS
 static int smu_fw_info_show(struct seq_file *s, void *unused)
 {
-	struct amd_pmc_dev *dev = s->private;
-	u32 value;
-
-	value = ioread32(dev->smu_base + AMD_SMU_FW_VERSION);
-	seq_printf(s, "SMU FW Info: %x\n", value);
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
@@ -164,6 +158,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
 		dev_err(dev->dev, "SMU response timed out\n");
 		return rc;
 	}
+	amd_pmc_dump_registers(dev);
 	return 0;
 }
 
@@ -176,7 +171,6 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 	if (rc)
 		dev_err(pdev->dev, "suspend failed\n");
 
-	amd_pmc_dump_registers(pdev);
 	return 0;
 }
 
@@ -189,7 +183,6 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 	if (rc)
 		dev_err(pdev->dev, "resume failed\n");
 
-	amd_pmc_dump_registers(pdev);
 	return 0;
 }
 
@@ -256,17 +249,11 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	pci_dev_put(rdev);
 	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
 
-	dev->smu_base = devm_ioremap(dev->dev, base_addr, AMD_PMC_MAPPING_SIZE);
-	if (!dev->smu_base)
-		return -ENOMEM;
-
 	dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
 				    AMD_PMC_MAPPING_SIZE);
 	if (!dev->regbase)
 		return -ENOMEM;
 
-	amd_pmc_dump_registers(dev);
-
 	platform_set_drvdata(pdev, dev);
 	amd_pmc_dbgfs_register(dev);
 	return 0;
-- 
2.25.1

Re: [PATCH 2/6] platform/x86: amd-pmc: Fix SMU firmware reporting mechanism

[Hans de Goede]

Hi,

On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
> It was lately understood that the current mechanism available in the
> driver to get SMU firmware info works only on internal SMU builds and
> there is a separate way to get all the SMU logging counters (addressed
> in the next patch). Hence remove all the smu info shown via debugfs as it
> is no more useful.
> 
> Also, use dump registers routine only at one place i.e. after the command
> submission to SMU is done.
> 
> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd-pmc.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 9c8a53120767..ce0e2ad94d09 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -52,7 +52,6 @@
>  #define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
>  #define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
>  
> -#define AMD_SMU_FW_VERSION		0x0
>  #define PMC_MSG_DELAY_MIN_US		100
>  #define RESPONSE_REGISTER_LOOP_MAX	200
>  
> @@ -88,11 +87,6 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
>  #ifdef CONFIG_DEBUG_FS
>  static int smu_fw_info_show(struct seq_file *s, void *unused)
>  {
> -	struct amd_pmc_dev *dev = s->private;
> -	u32 value;
> -
> -	value = ioread32(dev->smu_base + AMD_SMU_FW_VERSION);
> -	seq_printf(s, "SMU FW Info: %x\n", value);
>  	return 0;
>  }
>  DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
> @@ -164,6 +158,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>  		dev_err(dev->dev, "SMU response timed out\n");
>  		return rc;
>  	}
> +	amd_pmc_dump_registers(dev);

Is this addition instead of a removal intentional ?  If this is intentional
this really should be done in a separate commit, with a commit message
explaining why this change is being made.

>  	return 0;
>  }
>  
> @@ -176,7 +171,6 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>  	if (rc)
>  		dev_err(pdev->dev, "suspend failed\n");
>  
> -	amd_pmc_dump_registers(pdev);
>  	return 0;
>  }
>  
> @@ -189,7 +183,6 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>  	if (rc)
>  		dev_err(pdev->dev, "resume failed\n");
>  
> -	amd_pmc_dump_registers(pdev);
>  	return 0;
>  }
>  
> @@ -256,17 +249,11 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	pci_dev_put(rdev);
>  	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
>  
> -	dev->smu_base = devm_ioremap(dev->dev, base_addr, AMD_PMC_MAPPING_SIZE);
> -	if (!dev->smu_base)
> -		return -ENOMEM;
> -

Since you are now no longer setting this, I assume that this is also
not used anywhere anymore? And thus the smu_base variable should also
be removed from the "dev" struct.

>  	dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
>  				    AMD_PMC_MAPPING_SIZE);
>  	if (!dev->regbase)
>  		return -ENOMEM;
>  
> -	amd_pmc_dump_registers(dev);
> -
>  	platform_set_drvdata(pdev, dev);
>  	amd_pmc_dbgfs_register(dev);
>  	return 0;


Regards,

Hans


p.s.

I don't have time to review the rest of this series today. I'll continue
reviewing this later (probably on Monday).

Re: [PATCH 2/6] platform/x86: amd-pmc: Fix SMU firmware reporting mechanism

[Limonciello, Mario]

On 6/17/2021 10:02, Hans de Goede wrote:
> Hi,
> 
> On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
>> It was lately understood that the current mechanism available in the
>> driver to get SMU firmware info works only on internal SMU builds and
>> there is a separate way to get all the SMU logging counters (addressed
>> in the next patch). Hence remove all the smu info shown via debugfs as it
>> is no more useful.
>>
>> Also, use dump registers routine only at one place i.e. after the command
>> submission to SMU is done.
>>
>> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>   drivers/platform/x86/amd-pmc.c | 15 +--------------
>>   1 file changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
>> index 9c8a53120767..ce0e2ad94d09 100644
>> --- a/drivers/platform/x86/amd-pmc.c
>> +++ b/drivers/platform/x86/amd-pmc.c
>> @@ -52,7 +52,6 @@
>>   #define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
>>   #define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
>>   
>> -#define AMD_SMU_FW_VERSION		0x0
>>   #define PMC_MSG_DELAY_MIN_US		100
>>   #define RESPONSE_REGISTER_LOOP_MAX	200
>>   
>> @@ -88,11 +87,6 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
>>   #ifdef CONFIG_DEBUG_FS
>>   static int smu_fw_info_show(struct seq_file *s, void *unused)
>>   {
>> -	struct amd_pmc_dev *dev = s->private;
>> -	u32 value;
>> -
>> -	value = ioread32(dev->smu_base + AMD_SMU_FW_VERSION);
>> -	seq_printf(s, "SMU FW Info: %x\n", value);
>>   	return 0;
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
>> @@ -164,6 +158,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>>   		dev_err(dev->dev, "SMU response timed out\n");
>>   		return rc;
>>   	}
>> +	amd_pmc_dump_registers(dev);
> 
> Is this addition instead of a removal intentional ?  If this is intentional
> this really should be done in a separate commit, with a commit message
> explaining why this change is being made.
> 
>>   	return 0;
>>   }
>>   
>> @@ -176,7 +171,6 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>>   	if (rc)
>>   		dev_err(pdev->dev, "suspend failed\n");
>>   
>> -	amd_pmc_dump_registers(pdev);
>>   	return 0;
>>   }
>>   
>> @@ -189,7 +183,6 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>>   	if (rc)
>>   		dev_err(pdev->dev, "resume failed\n");
>>   
>> -	amd_pmc_dump_registers(pdev);
>>   	return 0;
>>   }
>>   
>> @@ -256,17 +249,11 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>   	pci_dev_put(rdev);
>>   	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
>>   
>> -	dev->smu_base = devm_ioremap(dev->dev, base_addr, AMD_PMC_MAPPING_SIZE);
>> -	if (!dev->smu_base)
>> -		return -ENOMEM;
>> -
> 
> Since you are now no longer setting this, I assume that this is also
> not used anywhere anymore? And thus the smu_base variable should also
> be removed from the "dev" struct.

It's removed in the next patch.  I suppose when Shyam re-spins you're 
looking to have it removed in "this" patch instead.

> 
>>   	dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
>>   				    AMD_PMC_MAPPING_SIZE);
>>   	if (!dev->regbase)
>>   		return -ENOMEM;
>>   
>> -	amd_pmc_dump_registers(dev);
>> -
>>   	platform_set_drvdata(pdev, dev);
>>   	amd_pmc_dbgfs_register(dev);
>>   	return 0;
> 
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> I don't have time to review the rest of this series today. I'll continue
> reviewing this later (probably on Monday).
> 
> 

Re: [PATCH 2/6] platform/x86: amd-pmc: Fix SMU firmware reporting mechanism

[Hans de Goede]

Hi,

On 6/17/21 6:56 PM, Limonciello, Mario wrote:
> On 6/17/2021 10:02, Hans de Goede wrote:
>> Hi,
>>
>> On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
>>> It was lately understood that the current mechanism available in the
>>> driver to get SMU firmware info works only on internal SMU builds and
>>> there is a separate way to get all the SMU logging counters (addressed
>>> in the next patch). Hence remove all the smu info shown via debugfs as it
>>> is no more useful.
>>>
>>> Also, use dump registers routine only at one place i.e. after the command
>>> submission to SMU is done.
>>>
>>> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>> ---
>>>   drivers/platform/x86/amd-pmc.c | 15 +--------------
>>>   1 file changed, 1 insertion(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
>>> index 9c8a53120767..ce0e2ad94d09 100644
>>> --- a/drivers/platform/x86/amd-pmc.c
>>> +++ b/drivers/platform/x86/amd-pmc.c
>>> @@ -52,7 +52,6 @@
>>>   #define AMD_CPU_ID_PCO            AMD_CPU_ID_RV
>>>   #define AMD_CPU_ID_CZN            AMD_CPU_ID_RN
>>>   -#define AMD_SMU_FW_VERSION        0x0
>>>   #define PMC_MSG_DELAY_MIN_US        100
>>>   #define RESPONSE_REGISTER_LOOP_MAX    200
>>>   @@ -88,11 +87,6 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
>>>   #ifdef CONFIG_DEBUG_FS
>>>   static int smu_fw_info_show(struct seq_file *s, void *unused)
>>>   {
>>> -    struct amd_pmc_dev *dev = s->private;
>>> -    u32 value;
>>> -
>>> -    value = ioread32(dev->smu_base + AMD_SMU_FW_VERSION);
>>> -    seq_printf(s, "SMU FW Info: %x\n", value);
>>>       return 0;
>>>   }
>>>   DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
>>> @@ -164,6 +158,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>>>           dev_err(dev->dev, "SMU response timed out\n");
>>>           return rc;
>>>       }
>>> +    amd_pmc_dump_registers(dev);
>>
>> Is this addition instead of a removal intentional ?  If this is intentional
>> this really should be done in a separate commit, with a commit message
>> explaining why this change is being made.
>>
>>>       return 0;
>>>   }
>>>   @@ -176,7 +171,6 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>>>       if (rc)
>>>           dev_err(pdev->dev, "suspend failed\n");
>>>   -    amd_pmc_dump_registers(pdev);
>>>       return 0;
>>>   }
>>>   @@ -189,7 +183,6 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>>>       if (rc)
>>>           dev_err(pdev->dev, "resume failed\n");
>>>   -    amd_pmc_dump_registers(pdev);
>>>       return 0;
>>>   }
>>>   @@ -256,17 +249,11 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>>       pci_dev_put(rdev);
>>>       base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
>>>   -    dev->smu_base = devm_ioremap(dev->dev, base_addr, AMD_PMC_MAPPING_SIZE);
>>> -    if (!dev->smu_base)
>>> -        return -ENOMEM;
>>> -
>>
>> Since you are now no longer setting this, I assume that this is also
>> not used anywhere anymore? And thus the smu_base variable should also
>> be removed from the "dev" struct.
> 
> It's removed in the next patch. 

Ah I missed that.

> I suppose when Shyam re-spins you're looking to have it removed in "this" patch instead.

Yes please.

Regards,

Hans




> 
>>
>>>       dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
>>>                       AMD_PMC_MAPPING_SIZE);
>>>       if (!dev->regbase)
>>>           return -ENOMEM;
>>>   -    amd_pmc_dump_registers(dev);
>>> -
>>>       platform_set_drvdata(pdev, dev);
>>>       amd_pmc_dbgfs_register(dev);
>>>       return 0;
>>
>>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> I don't have time to review the rest of this series today. I'll continue
>> reviewing this later (probably on Monday).
>>
>>
> 

Re: [PATCH 2/6] platform/x86: amd-pmc: Fix SMU firmware reporting mechanism

[Shyam Sundar S K]

On 6/17/2021 11:01 PM, Hans de Goede wrote:
> Hi,
> 
> On 6/17/21 6:56 PM, Limonciello, Mario wrote:
>> On 6/17/2021 10:02, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
>>>> It was lately understood that the current mechanism available in the
>>>> driver to get SMU firmware info works only on internal SMU builds and
>>>> there is a separate way to get all the SMU logging counters (addressed
>>>> in the next patch). Hence remove all the smu info shown via debugfs as it
>>>> is no more useful.
>>>>
>>>> Also, use dump registers routine only at one place i.e. after the command
>>>> submission to SMU is done.
>>>>
>>>> Fixes: 156ec4731cb2 ("platform/x86: amd-pmc: Add AMD platform support for S2Idle")
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>   drivers/platform/x86/amd-pmc.c | 15 +--------------
>>>>   1 file changed, 1 insertion(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
>>>> index 9c8a53120767..ce0e2ad94d09 100644
>>>> --- a/drivers/platform/x86/amd-pmc.c
>>>> +++ b/drivers/platform/x86/amd-pmc.c
>>>> @@ -52,7 +52,6 @@
>>>>   #define AMD_CPU_ID_PCO            AMD_CPU_ID_RV
>>>>   #define AMD_CPU_ID_CZN            AMD_CPU_ID_RN
>>>>   -#define AMD_SMU_FW_VERSION        0x0
>>>>   #define PMC_MSG_DELAY_MIN_US        100
>>>>   #define RESPONSE_REGISTER_LOOP_MAX    200
>>>>   @@ -88,11 +87,6 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>   static int smu_fw_info_show(struct seq_file *s, void *unused)
>>>>   {
>>>> -    struct amd_pmc_dev *dev = s->private;
>>>> -    u32 value;
>>>> -
>>>> -    value = ioread32(dev->smu_base + AMD_SMU_FW_VERSION);
>>>> -    seq_printf(s, "SMU FW Info: %x\n", value);
>>>>       return 0;
>>>>   }
>>>>   DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
>>>> @@ -164,6 +158,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
>>>>           dev_err(dev->dev, "SMU response timed out\n");
>>>>           return rc;
>>>>       }
>>>> +    amd_pmc_dump_registers(dev);
>>>
>>> Is this addition instead of a removal intentional ?  If this is intentional
>>> this really should be done in a separate commit, with a commit message
>>> explaining why this change is being made.
>>>
>>>>       return 0;
>>>>   }
>>>>   @@ -176,7 +171,6 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>>>>       if (rc)
>>>>           dev_err(pdev->dev, "suspend failed\n");
>>>>   -    amd_pmc_dump_registers(pdev);
>>>>       return 0;
>>>>   }
>>>>   @@ -189,7 +183,6 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>>>>       if (rc)
>>>>           dev_err(pdev->dev, "resume failed\n");
>>>>   -    amd_pmc_dump_registers(pdev);
>>>>       return 0;
>>>>   }
>>>>   @@ -256,17 +249,11 @@ static int amd_pmc_probe(struct platform_device *pdev)
>>>>       pci_dev_put(rdev);
>>>>       base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
>>>>   -    dev->smu_base = devm_ioremap(dev->dev, base_addr, AMD_PMC_MAPPING_SIZE);
>>>> -    if (!dev->smu_base)
>>>> -        return -ENOMEM;
>>>> -
>>>
>>> Since you are now no longer setting this, I assume that this is also
>>> not used anywhere anymore? And thus the smu_base variable should also
>>> be removed from the "dev" struct.
>>
>> It's removed in the next patch. 
> 
> Ah I missed that.
> 
>> I suppose when Shyam re-spins you're looking to have it removed in "this" patch instead.
> 
> Yes please.

OK. Thanks! Shall re-spin a v2 once you review the series on Monday.

Thanks,
Shyam

[PATCH 3/6] platform/x86: amd-pmc: Add support for logging SMU metrics

[Shyam Sundar S K]

SMU provides a way to dump the s0ix debug statistics in the form of a
metrics table via a of set special mailbox commands.

Add support to the driver which can send these commands to SMU and expose
the information received via debugfs. The information contains the s0ix
entry/exit, active time of each IP block etc.

As a side note, SMU subsystem logging is not supported on Picasso based
SoC's.

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

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index ce0e2ad94d09..bb067324644d 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -46,6 +46,14 @@
 #define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
 #define AMD_PMC_RESULT_FAILED                0xFF
 
+/* SMU Message Definations */
+#define SMU_MSG_GETSMUVERSION		0x02
+#define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
+#define SMU_MSG_LOG_GETDRAM_ADDR_LO	0x05
+#define SMU_MSG_LOG_START		0x06
+#define SMU_MSG_LOG_RESET		0x07
+#define SMU_MSG_LOG_DUMP_DATA		0x08
+#define SMU_MSG_GET_SUP_CONSTRAINTS	0x09
 /* List of supported CPU ids */
 #define AMD_CPU_ID_RV			0x15D0
 #define AMD_CPU_ID_RN			0x1630
@@ -55,17 +63,42 @@
 #define PMC_MSG_DELAY_MIN_US		100
 #define RESPONSE_REGISTER_LOOP_MAX	200
 
+#define SOC_SUBSYSTEM_IP_MAX	12
+#define DELAY_MIN_US		2000
+#define DELAY_MAX_US		3000
 enum amd_pmc_def {
 	MSG_TEST = 0x01,
 	MSG_OS_HINT_PCO,
 	MSG_OS_HINT_RN,
 };
 
+struct amd_pmc_bit_map {
+	const char *name;
+	u32 bit_mask;
+};
+
+static const struct amd_pmc_bit_map soc15_ip_blk[] = {
+	{"DISPLAY",	BIT(0)},
+	{"CPU",		BIT(1)},
+	{"GFX",		BIT(2)},
+	{"VDD",		BIT(3)},
+	{"ACP",		BIT(4)},
+	{"VCN",		BIT(5)},
+	{"ISP",		BIT(6)},
+	{"NBIO",	BIT(7)},
+	{"DF",		BIT(8)},
+	{"USB0",	BIT(9)},
+	{"USB1",	BIT(10)},
+	{"LAPIC",	BIT(11)},
+	{}
+};
+
 struct amd_pmc_dev {
 	void __iomem *regbase;
-	void __iomem *smu_base;
+	void __iomem *smu_virt_addr;
 	u32 base_addr;
 	u32 cpu_id;
+	u32 active_ips;
 	struct device *dev;
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbgfs_dir;
@@ -73,6 +106,7 @@ struct amd_pmc_dev {
 };
 
 static struct amd_pmc_dev pmc;
+static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -84,9 +118,50 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
 	iowrite32(val, dev->regbase + reg_offset);
 }
 
+struct smu_metrics {
+	u32 table_version;
+	u32 hint_count;
+	u32 s0i3_cyclecount;
+	u32 timein_s0i2;
+	u64 timeentering_s0i3_lastcapture;
+	u64 timeentering_s0i3_totaltime;
+	u64 timeto_resume_to_os_lastcapture;
+	u64 timeto_resume_to_os_totaltime;
+	u64 timein_s0i3_lastcapture;
+	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];
+} __packed;
+
 #ifdef CONFIG_DEBUG_FS
 static int smu_fw_info_show(struct seq_file *s, void *unused)
 {
+	struct amd_pmc_dev *dev = s->private;
+	struct smu_metrics table;
+	u32 value;
+	int idx;
+
+	if (dev->cpu_id == AMD_CPU_ID_PCO)
+		return -EINVAL;
+
+	memcpy_fromio(&table, dev->smu_virt_addr, sizeof(struct smu_metrics));
+
+	seq_puts(s, "\n=== SMU Statistics ===\n");
+	seq_printf(s, "Table Version: %d\n", table.table_version);
+	seq_printf(s, "Hint Count: %d\n", table.hint_count);
+	seq_printf(s, "S0i3 Cycle Count: %d\n", table.s0i3_cyclecount);
+	seq_printf(s, "Time (in us) to S0i3: %lld\n", table.timeentering_s0i3_lastcapture);
+	seq_printf(s, "Time (in us) in S0i3: %lld\n", table.timein_s0i3_lastcapture);
+
+	seq_puts(s, "\n=== Active time (in us) ===\n");
+	for (idx = 0 ; idx < SOC_SUBSYSTEM_IP_MAX ; 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]);
+	}
+
 	return 0;
 }
 DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
@@ -112,6 +187,32 @@ static inline void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
 }
 #endif /* CONFIG_DEBUG_FS */
 
+static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
+{
+	u32 phys_addr_low, phys_addr_hi;
+	u64 smu_phys_addr;
+
+	if (dev->cpu_id == AMD_CPU_ID_PCO)
+		return -EINVAL;
+
+	/* Get Active devices list from SMU */
+	amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
+
+	/* Get dram address */
+	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);
+	smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
+
+	dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr, sizeof(struct smu_metrics));
+	if (!dev->smu_virt_addr)
+		return -ENOMEM;
+
+	/* Start the logging */
+	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, 0);
+
+	return 0;
+}
+
 static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
 {
 	u32 value;
@@ -126,10 +227,9 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
 	dev_dbg(dev->dev, "AMD_PMC_REGISTER_MESSAGE:%x\n", value);
 }
 
-static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
+static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set, u32 *data, u8 msg, bool ret)
 {
 	int rc;
-	u8 msg;
 	u32 val;
 
 	/* Wait until we get a valid response */
@@ -148,8 +248,8 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
 	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, set);
 
 	/* Write message ID to message ID register */
-	msg = (dev->cpu_id == AMD_CPU_ID_RN) ? MSG_OS_HINT_RN : MSG_OS_HINT_PCO;
 	amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
+
 	/* Wait until we get a valid response */
 	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
 				val, val == AMD_PMC_RESULT_OK, PMC_MSG_DELAY_MIN_US,
@@ -158,16 +258,40 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, bool set)
 		dev_err(dev->dev, "SMU response timed out\n");
 		return rc;
 	}
+
+	if (ret) {
+		/* PMFW may take longer time to return back the data */
+		usleep_range(DELAY_MIN_US, 10 * DELAY_MAX_US);
+		*data = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);
+	}
+
 	amd_pmc_dump_registers(dev);
 	return 0;
 }
 
+static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
+{
+	switch (dev->cpu_id) {
+	case AMD_CPU_ID_PCO:
+		return MSG_OS_HINT_PCO;
+	case AMD_CPU_ID_RN:
+		return MSG_OS_HINT_RN;
+	}
+	return -EINVAL;
+}
+
 static int __maybe_unused amd_pmc_suspend(struct device *dev)
 {
 	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
 	int rc;
+	u8 msg;
+
+	/* Reset and Start SMU logging - to monitor the s0i3 stats */
+	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
+	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
 
-	rc = amd_pmc_send_cmd(pdev, 1);
+	msg = amd_pmc_get_os_hint(pdev);
+	rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
 	if (rc)
 		dev_err(pdev->dev, "suspend failed\n");
 
@@ -178,8 +302,13 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 {
 	struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
 	int rc;
+	u8 msg;
+
+	/* Let SMU know that we are looking for stats */
+	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_DUMP_DATA, 0);
 
-	rc = amd_pmc_send_cmd(pdev, 0);
+	msg = amd_pmc_get_os_hint(pdev);
+	rc = amd_pmc_send_cmd(pdev, 0, NULL, msg, 0);
 	if (rc)
 		dev_err(pdev->dev, "resume failed\n");
 
@@ -202,8 +331,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = &pmc;
 	struct pci_dev *rdev;
-	u32 base_addr_lo;
-	u32 base_addr_hi;
+	u32 base_addr_lo, base_addr_hi;
 	u64 base_addr;
 	int err;
 	u32 val;
@@ -254,6 +382,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	if (!dev->regbase)
 		return -ENOMEM;
 
+	/* Use SMU to get the s0i3 debug stats */
+	err = amd_pmc_setup_smu_logging(dev);
+	if (err)
+		dev_err(dev->dev, "SMU debugging info not supported on this platform\n");
 	platform_set_drvdata(pdev, dev);
 	amd_pmc_dbgfs_register(dev);
 	return 0;
-- 
2.25.1

[PATCH 4/6] platform/x86: amd-pmc: Add support for logging s0ix counters

[Shyam Sundar S K]

Even the FCH SSC registers provides certain level of information
about the s0ix entry and exit times which comes handy when the SMU
fails to report the statistics via the mailbox communication.

This information is captured via a new debugfs file "s0ix_stats".
A non-zero entry in this counters would mean that the system entered
the s0ix state.

If s0ix entry time and exit time don't change during suspend to idle,
the silicon has not entered the deepest state.

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

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index bb067324644d..174f067f0756 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -46,6 +46,15 @@
 #define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
 #define AMD_PMC_RESULT_FAILED                0xFF
 
+/* FCH SSC Registers */
+#define FCH_S0I3_ENTRY_TIME_L_OFFSET	0x30
+#define FCH_S0I3_ENTRY_TIME_H_OFFSET	0x34
+#define FCH_S0I3_EXIT_TIME_L_OFFSET	0x38
+#define FCH_S0I3_EXIT_TIME_H_OFFSET	0x3C
+#define FCH_SSC_MAPPING_SIZE		0x800
+#define FCH_BASE_PHY_ADDR_LOW		0xFED81100
+#define FCH_BASE_PHY_ADDR_HIGH		0x00000000
+
 /* SMU Message Definations */
 #define SMU_MSG_GETSMUVERSION		0x02
 #define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
@@ -96,6 +105,7 @@ static const struct amd_pmc_bit_map soc15_ip_blk[] = {
 struct amd_pmc_dev {
 	void __iomem *regbase;
 	void __iomem *smu_virt_addr;
+	void __iomem *fch_virt_addr;
 	u32 base_addr;
 	u32 cpu_id;
 	u32 active_ips;
@@ -140,7 +150,6 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
 {
 	struct amd_pmc_dev *dev = s->private;
 	struct smu_metrics table;
-	u32 value;
 	int idx;
 
 	if (dev->cpu_id == AMD_CPU_ID_PCO)
@@ -166,6 +175,29 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(smu_fw_info);
 
+static int s0ix_stats_show(struct seq_file *s, void *unused)
+{
+	struct amd_pmc_dev *dev = s->private;
+	u64 entry_time, exit_time, residency;
+
+	entry_time = ioread32(dev->fch_virt_addr + FCH_S0I3_ENTRY_TIME_H_OFFSET);
+	entry_time = entry_time << 32 | ioread32(dev->fch_virt_addr + FCH_S0I3_ENTRY_TIME_L_OFFSET);
+
+	exit_time = ioread32(dev->fch_virt_addr + FCH_S0I3_EXIT_TIME_H_OFFSET);
+	exit_time = exit_time << 32 | ioread32(dev->fch_virt_addr + FCH_S0I3_EXIT_TIME_L_OFFSET);
+
+	/* It's in 48MHz. We need to convert it to unit of 100ns */
+	residency = (exit_time - entry_time) * 10 / 48;
+
+	seq_puts(s, "=== S0ix statistics ===\n");
+	seq_printf(s, "S0ix Entry Time: %lld\n", entry_time);
+	seq_printf(s, "S0ix Exit Time: %lld\n", exit_time);
+	seq_printf(s, "Residency Time: %lld\n", residency);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(s0ix_stats);
+
 static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
 {
 	debugfs_remove_recursive(dev->dbgfs_dir);
@@ -176,6 +208,8 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 	dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL);
 	debugfs_create_file("smu_fw_info", 0644, dev->dbgfs_dir, dev,
 			    &smu_fw_info_fops);
+	debugfs_create_file("s0ix_stats", 0644, dev->dbgfs_dir, dev,
+			    &s0ix_stats_fops);
 }
 #else
 static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
@@ -332,7 +366,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	struct amd_pmc_dev *dev = &pmc;
 	struct pci_dev *rdev;
 	u32 base_addr_lo, base_addr_hi;
-	u64 base_addr;
+	u64 base_addr, fch_phys_addr;
 	int err;
 	u32 val;
 
@@ -382,6 +416,14 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	if (!dev->regbase)
 		return -ENOMEM;
 
+	/* Use FCH registers to get the S0ix stats */
+	base_addr_lo = FCH_BASE_PHY_ADDR_LOW;
+	base_addr_hi = FCH_BASE_PHY_ADDR_HIGH;
+	fch_phys_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
+	dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
+	if (!dev->fch_virt_addr)
+		return -ENOMEM;
+
 	/* Use SMU to get the s0i3 debug stats */
 	err = amd_pmc_setup_smu_logging(dev);
 	if (err)
-- 
2.25.1

[PATCH 5/6] platform/x86: amd-pmc: Add support for ACPI ID AMDI0006

[Shyam Sundar S K]

Some newer BIOSes have added another ACPI ID for the uPEP device.
SMU statistics behave identically on this device.

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

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 174f067f0756..e024fd36bd26 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -443,6 +443,7 @@ static int amd_pmc_remove(struct platform_device *pdev)
 
 static const struct acpi_device_id amd_pmc_acpi_ids[] = {
 	{"AMDI0005", 0},
+	{"AMDI0006", 0},
 	{"AMD0004", 0},
 	{ }
 };
-- 
2.25.1

[PATCH 6/6] platform/x86: amd-pmc: Add new acpi id for future PMC controllers

[Shyam Sundar S K]

The upcoming PMC controller would have a newer acpi id, add that to
the supported acpid device list.

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

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index e024fd36bd26..c26ac561c0d4 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -68,6 +68,7 @@
 #define AMD_CPU_ID_RN			0x1630
 #define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
 #define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
+#define AMD_CPU_ID_YC			0x14B5
 
 #define PMC_MSG_DELAY_MIN_US		100
 #define RESPONSE_REGISTER_LOOP_MAX	200
@@ -309,6 +310,7 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
 	case AMD_CPU_ID_PCO:
 		return MSG_OS_HINT_PCO;
 	case AMD_CPU_ID_RN:
+	case AMD_CPU_ID_YC:
 		return MSG_OS_HINT_RN;
 	}
 	return -EINVAL;
@@ -354,6 +356,7 @@ static const struct dev_pm_ops amd_pmc_pm_ops = {
 };
 
 static const struct pci_device_id pmc_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_YC) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_CZN) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_RN) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_PCO) },
@@ -444,6 +447,7 @@ static int amd_pmc_remove(struct platform_device *pdev)
 static const struct acpi_device_id amd_pmc_acpi_ids[] = {
 	{"AMDI0005", 0},
 	{"AMDI0006", 0},
+	{"AMDI0007", 0},
 	{"AMD0004", 0},
 	{ }
 };
-- 
2.25.1

Re: [PATCH 0/6] updates to amd-pmc driver

[Hans de Goede]

Hi,

On 6/17/21 1:30 PM, Shyam Sundar S K wrote:
> Couple of existing issues on the completion codes to SMU
> and a newer way to get the s0ix statistics are introduced.
> 
> Also, add acpi ids for current and future revisions of the
> pmc controller.
> 
> Shyam Sundar S K (6):
>   platform/x86: amd-pmc: Fix command completion code
>   platform/x86: amd-pmc: Fix SMU firmware reporting mechanism
>   platform/x86: amd-pmc: Add support for logging SMU metrics
>   platform/x86: amd-pmc: Add support for logging s0ix counters
>   platform/x86: amd-pmc: Add support for ACPI ID AMDI0006
>   platform/x86: amd-pmc: Add new acpi id for future PMC controllers

Thanks, other then my remarks on patch 2/6, the rest of
the series looks good to me, so you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

To patches 1/6 and [3-6]/6 .

Regards,

Hans