[PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Shyam Sundar S K]

amd_pmc_get_dram_size() is used to get the DRAM size information. But in
the current code, mailbox command to get the DRAM size info is sent based
on the values of dev->major and dev->minor.

But dev->major and dev->minor will have either junk or zero assigned to
them until at least once a call to amd_pmc_get_smu_version() is made which
ideally populates dev->major and dev->minor.

Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
amd_pmc_get_dram_size() would solve, but that has a downside of elevating
the boot times.

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 getting DRAM size behavior 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.

Simplest way to address this is to remove amd_pmc_get_dram_size() function
and directly call the "get dram size" command in the amd_pmc_s2d_init().

Reported-by: Mark Hasemeyer <markhas@chromium.org>
Closes: 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>
---
v4:
- Based on review-ilpo branch (tip commit: 94ace9eda882)
- Add Mark as "Reported-by:"
- Add more commit log notes.

v3:
- Based on review-ilpo branch
- Remove amd_pmc_get_dram_size() function
- Remove prints that are not noisy

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

 drivers/platform/x86/amd/pmc/pmc.c | 31 ++----------------------------
 1 file changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index cd6ac04c1468..c3104714b480 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -964,33 +964,6 @@ 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, dev->s2d_msg_id, 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;
@@ -1009,8 +982,8 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
 		return -EIO;
 
 	/* Get DRAM size */
-	ret = amd_pmc_get_dram_size(dev);
-	if (ret)
+	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
+	if (ret || !dev->dram_size)
 		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
 
 	/* Get STB DRAM address */
-- 
2.25.1

Re: [PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Mario Limonciello]

On 11/16/2023 11:01, Shyam Sundar S K wrote:
> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> the current code, mailbox command to get the DRAM size info is sent based
> on the values of dev->major and dev->minor.
> 
> But dev->major and dev->minor will have either junk or zero assigned to
> them until at least once a call to amd_pmc_get_smu_version() is made which
> ideally populates dev->major and dev->minor.
> 
> Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
> amd_pmc_get_dram_size() would solve, but that has a downside of elevating
> the boot times.
> 
> 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 getting DRAM size behavior 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.
> 
> Simplest way to address this is to remove amd_pmc_get_dram_size() function
> and directly call the "get dram size" command in the amd_pmc_s2d_init().
> 
> Reported-by: Mark Hasemeyer <markhas@chromium.org>
> Closes: 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>

Ilpo when committing can you please add a stable tag for this?

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
> v4:
> - Based on review-ilpo branch (tip commit: 94ace9eda882)
> - Add Mark as "Reported-by:"
> - Add more commit log notes.
> 
> v3:
> - Based on review-ilpo branch
> - Remove amd_pmc_get_dram_size() function
> - Remove prints that are not noisy
> 
> v2:
> - Based on review-ilpo branch
> - Drop calling get smu version from probe.
> 
>   drivers/platform/x86/amd/pmc/pmc.c | 31 ++----------------------------
>   1 file changed, 2 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index cd6ac04c1468..c3104714b480 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -964,33 +964,6 @@ 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, dev->s2d_msg_id, 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;
> @@ -1009,8 +982,8 @@ static int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>   		return -EIO;
>   
>   	/* Get DRAM size */
> -	ret = amd_pmc_get_dram_size(dev);
> -	if (ret)
> +	ret = amd_pmc_send_cmd(dev, S2D_DRAM_SIZE, &dev->dram_size, dev->s2d_msg_id, true);
> +	if (ret || !dev->dram_size)
>   		dev->dram_size = S2D_TELEMETRY_DRAMBYTES_MAX;
>   
>   	/* Get STB DRAM address */

Re: [PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Ilpo Järvinen]

On Thu, 16 Nov 2023, Mario Limonciello wrote:

> On 11/16/2023 11:01, Shyam Sundar S K wrote:
> > amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> > the current code, mailbox command to get the DRAM size info is sent based
> > on the values of dev->major and dev->minor.
> > 
> > But dev->major and dev->minor will have either junk or zero assigned to
> > them until at least once a call to amd_pmc_get_smu_version() is made which
> > ideally populates dev->major and dev->minor.
> > 
> > Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
> > amd_pmc_get_dram_size() would solve, but that has a downside of elevating
> > the boot times.
> > 
> > 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 getting DRAM size behavior 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.
> > 
> > Simplest way to address this is to remove amd_pmc_get_dram_size() function
> > and directly call the "get dram size" command in the amd_pmc_s2d_init().
> > 
> > Reported-by: Mark Hasemeyer <markhas@chromium.org>
> > Closes:
> > 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>
> 
> Ilpo when committing can you please add a stable tag for this?
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll add it, thanks.

-- 
 i.

Re: [PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Ilpo Järvinen]

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

> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> the current code, mailbox command to get the DRAM size info is sent based
> on the values of dev->major and dev->minor.
> 
> But dev->major and dev->minor will have either junk or zero assigned to
> them until at least once a call to amd_pmc_get_smu_version() is made which
> ideally populates dev->major and dev->minor.
> 
> Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
> amd_pmc_get_dram_size() would solve, but that has a downside of elevating
> the boot times.
> 
> 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 getting DRAM size behavior 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.
> 
> Simplest way to address this is to remove amd_pmc_get_dram_size() function
> and directly call the "get dram size" command in the amd_pmc_s2d_init().
> 
> Reported-by: Mark Hasemeyer <markhas@chromium.org>
> Closes: 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>
> ---
> v4:
> - Based on review-ilpo branch (tip commit: 94ace9eda882)
> - Add Mark as "Reported-by:"
> - Add more commit log notes.

Thank, applied now to review-ilpo branch. I had to reflow your commit 
message because the lines were too long (try to remain within 72 
characters in the future). I also made other minor adjustments to the 
commit message.


-- 
 i.

Re: [PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Shyam Sundar S K]

Hi Ilpo,

On 11/17/2023 3:41 PM, Ilpo Järvinen wrote:
> On Thu, 16 Nov 2023, Shyam Sundar S K wrote:
> 
>> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
>> the current code, mailbox command to get the DRAM size info is sent based
>> on the values of dev->major and dev->minor.
>>
>> But dev->major and dev->minor will have either junk or zero assigned to
>> them until at least once a call to amd_pmc_get_smu_version() is made which
>> ideally populates dev->major and dev->minor.
>>
>> Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
>> amd_pmc_get_dram_size() would solve, but that has a downside of elevating
>> the boot times.
>>
>> 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 getting DRAM size behavior 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.
>>
>> Simplest way to address this is to remove amd_pmc_get_dram_size() function
>> and directly call the "get dram size" command in the amd_pmc_s2d_init().
>>
>> Reported-by: Mark Hasemeyer <markhas@chromium.org>
>> Closes: 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>
>> ---
>> v4:
>> - Based on review-ilpo branch (tip commit: 94ace9eda882)
>> - Add Mark as "Reported-by:"
>> - Add more commit log notes.
> 
> Thank, applied now to review-ilpo branch. I had to reflow your commit 
> message because the lines were too long (try to remain within 72 
> characters in the future). I also made other minor adjustments to the 
> commit message.
> 
> 

Thank you for the rewords :-)

on the commit message part, you prefer 72 or 75 characters?

Because I did use, checkpatch with "--strict" and did not find it
complaining.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v6.7-rc2#n3275

Thanks,
Shyam

Re: [PATCH v4] platform/x86/amd/pmc: adjust getting DRAM size behavior

[Ilpo Järvinen]

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

On Mon, 20 Nov 2023, Shyam Sundar S K wrote:
> On 11/17/2023 3:41 PM, Ilpo Järvinen wrote:
> > On Thu, 16 Nov 2023, Shyam Sundar S K wrote:
> > 
> >> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> >> the current code, mailbox command to get the DRAM size info is sent based
> >> on the values of dev->major and dev->minor.
> >>
> >> But dev->major and dev->minor will have either junk or zero assigned to
> >> them until at least once a call to amd_pmc_get_smu_version() is made which
> >> ideally populates dev->major and dev->minor.
> >>
> >> Ideally to suffice this, adding a amd_pmc_get_smu_version() call to
> >> amd_pmc_get_dram_size() would solve, but that has a downside of elevating
> >> the boot times.
> >>
> >> 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 getting DRAM size behavior 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.
> >>
> >> Simplest way to address this is to remove amd_pmc_get_dram_size() function
> >> and directly call the "get dram size" command in the amd_pmc_s2d_init().
> >>
> >> Reported-by: Mark Hasemeyer <markhas@chromium.org>
> >> Closes: 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>
> >> ---
> >> v4:
> >> - Based on review-ilpo branch (tip commit: 94ace9eda882)
> >> - Add Mark as "Reported-by:"
> >> - Add more commit log notes.
> > 
> > Thank, applied now to review-ilpo branch. I had to reflow your commit 
> > message because the lines were too long (try to remain within 72 
> > characters in the future). I also made other minor adjustments to the 
> > commit message.
> 
> Thank you for the rewords :-)
> 
> on the commit message part, you prefer 72 or 75 characters?
> Because I did use, checkpatch with "--strict" and did not find it
> complaining.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v6.7-rc2#n3275

I'd have said 72 characters but since you now pointed out checkpatch seems 
to have only a slightly higher limit we can go along with that 75 
characters. I think that 72 char limit is derived from 80-2*4 (4 char wide 
margins on both sides on 80 char wide terminal). Be aware though some 
other maintainers do require 72 chars.

And obviously for URLs (or other stuff) that should be kept on a single 
line, you have the license to break that limit even if checkpatch would 
complain but I guess you knew that :-).

-- 
 i.