[PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[Shyam Sundar S K]

After talking to the PMFW team, its understood that the "get dram size"
mbox command would only be supported on specific platforms (like Mendocino)
and not all. So adjust the behavior of amd_pmc_get_dram_size() function
such that,

- if that's Rembrandt or Mendocino and the underlying PMFW knows how
to execute the "get dram size" command it shall give the custom dram size.

- if the underlying FW does not report the dram size, we just proceed
further and assign the default dram size.

Cc: Mark Hasemeyer <markhas@chromium.org>
Link: https://lore.kernel.org/platform-driver-x86/3b224c62-a1d8-41bd-aced-5825f5f20e66@amd.com/
Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---

v2:
- Based on review-ilpo branch
- Drop calling get smu version from probe.

 drivers/platform/x86/amd/pmc/pmc.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index cd6ac04c1468..501c72c7d34c 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -968,17 +968,8 @@ 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;
+	if (dev->cpu_id != AMD_CPU_ID_YC)
 		goto err_dram_size;
-	}
 
 	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
 	if (ret || !dev->dram_size)
-- 
2.25.1

Re: [PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[Mario Limonciello]

On 11/9/2023 00:17, Shyam Sundar S K wrote:
> After talking to the PMFW team, its understood that the "get dram size"
> mbox command would only be supported on specific platforms (like Mendocino)
> and not all. So adjust the behavior of amd_pmc_get_dram_size() function
> such that,
> 
> - if that's Rembrandt or Mendocino and the underlying PMFW knows how
> to execute the "get dram size" command it shall give the custom dram size.
> 
> - if the underlying FW does not report the dram size, we just proceed
> further and assign the default dram size.
> 
> Cc: Mark Hasemeyer <markhas@chromium.org>
> Link: https://lore.kernel.org/platform-driver-x86/3b224c62-a1d8-41bd-aced-5825f5f20e66@amd.com/
> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> 
> v2:
> - Based on review-ilpo branch
> - Drop calling get smu version from probe.
> 
>   drivers/platform/x86/amd/pmc/pmc.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index cd6ac04c1468..501c72c7d34c 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -968,17 +968,8 @@ 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;
> +	if (dev->cpu_id != AMD_CPU_ID_YC)
>   		goto err_dram_size;
> -	}

I notice that the error at err_dram_size is really noisy.  Thinking 
about how this command only works on RMB or MDN I think you should also 
downgrade it to debug.

>   
>   	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>   	if (ret || !dev->dram_size)

Re: [PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[Ilpo Järvinen]

On Thu, 9 Nov 2023, Shyam Sundar S K wrote:

> After talking to the PMFW team, its understood that the "get dram size"
> mbox command would only be supported on specific platforms (like Mendocino)
> and not all. So adjust the behavior of amd_pmc_get_dram_size() function
> such that,
> 
> - if that's Rembrandt or Mendocino and the underlying PMFW knows how
> to execute the "get dram size" command it shall give the custom dram size.
> 
> - if the underlying FW does not report the dram size, we just proceed
> further and assign the default dram size.

This commit message lacks the description of the problem we have the Fixes 
tag for. Please explain also that problem as it's very much related.

> Cc: Mark Hasemeyer <markhas@chromium.org>

Mark, does this patch solve the issue for you?

> Link: https://lore.kernel.org/platform-driver-x86/3b224c62-a1d8-41bd-aced-5825f5f20e66@amd.com/
> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> 
> v2:
> - Based on review-ilpo branch
> - Drop calling get smu version from probe.
> 
>  drivers/platform/x86/amd/pmc/pmc.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index cd6ac04c1468..501c72c7d34c 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -968,17 +968,8 @@ 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;
> +	if (dev->cpu_id != AMD_CPU_ID_YC)
>  		goto err_dram_size;

This now ends up returning uninitialized ret variable. I'd have expected 
compiler to warn you about it...??

It also still prints the dev_err() after jumping to the label. If we know 
dram size not supported, dev_err is not really correct level (I'd say 
dev_dbg at most but better would be to not print anything, IMO).

Thinking it more though, it would make more sense to initialize the 
default dram_size within this function to make it easier to track the code 
+ call this function amd_pmc_init_dram_size() and make it void since its 
return value is not really used for anything else than setting the default
dram_size.

> -	}
>  
>  	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>  	if (ret || !dev->dram_size)
> 


-- 
 i.

Re: [PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[Mark Hasemeyer]

> Mark, does this patch solve the issue for you?

The patch as-is does not compile as you suggested. But beyond the
unitialized `ret`, the patch does solve the issue for me.

Re: [PATCH v2] platform/x86/amd/pmc: adjust amd_pmc_get_dram_size() behavior

[Shyam Sundar S K]

Hi Mark, Ilpo,

Apologies for the delay

On 11/13/2023 6:40 PM, Ilpo Järvinen wrote:
> On Thu, 9 Nov 2023, Shyam Sundar S K wrote:
> 
>> After talking to the PMFW team, its understood that the "get dram size"
>> mbox command would only be supported on specific platforms (like Mendocino)
>> and not all. So adjust the behavior of amd_pmc_get_dram_size() function
>> such that,
>>
>> - if that's Rembrandt or Mendocino and the underlying PMFW knows how
>> to execute the "get dram size" command it shall give the custom dram size.
>>
>> - if the underlying FW does not report the dram size, we just proceed
>> further and assign the default dram size.
> 
> This commit message lacks the description of the problem we have the Fixes 
> tag for. Please explain also that problem as it's very much related.
> 
>> Cc: Mark Hasemeyer <markhas@chromium.org>
> 
> Mark, does this patch solve the issue for you?
> 
>> Link: https://lore.kernel.org/platform-driver-x86/3b224c62-a1d8-41bd-aced-5825f5f20e66@amd.com/
>> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
>> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>
>> v2:
>> - Based on review-ilpo branch
>> - Drop calling get smu version from probe.
>>
>>  drivers/platform/x86/amd/pmc/pmc.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index cd6ac04c1468..501c72c7d34c 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -968,17 +968,8 @@ 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;
>> +	if (dev->cpu_id != AMD_CPU_ID_YC)
>>  		goto err_dram_size;
> 
> This now ends up returning uninitialized ret variable. I'd have expected 
> compiler to warn you about it...??
> 
> It also still prints the dev_err() after jumping to the label. If we know 
> dram size not supported, dev_err is not really correct level (I'd say 
> dev_dbg at most but better would be to not print anything, IMO).
> 
> Thinking it more though, it would make more sense to initialize the 
> default dram_size within this function to make it easier to track the code 
> + call this function amd_pmc_init_dram_size() and make it void since its 
> return value is not really used for anything else than setting the default
> dram_size.

I have sent a simplified version now. Can you please take a look.

Thanks,
Shyam

> 
>> -	}
>>  
>>  	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
>>  	if (ret || !dev->dram_size)
>>
> 
>