Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/9/2023 10:39, Shyam Sundar S K wrote:
> 
> 
> On 10/9/2023 8:25 PM, Mario Limonciello wrote:
>> On 10/9/2023 09:47, 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.
>>>
>>> Add a missing amd_pmc_get_smu_version() call to
>>> amd_pmc_get_dram_size().
>>>
>>> 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>
>>> ---
>>> based on review-ilpo, can be added on top of recent stb changes
>>
>> Hmm, so this is actually going to mean that we end up with a round
>> trip call to the SMU as part of probe to get the version when STB is
>> initialized.
>>
>> I think that's going to translate to a longer amd_pmc startup time,
>> which could be problematic if a platform leaves STB enabled by default.
>>
>> At a minimum I would suggest to only do the version check when it's
>> one that needs it (IE AMD_CPU_ID_YC right now).  But otherwise this
>> s2d initialization can't happen the first time it's used?
> 
> I remember the long boot time problems, but was not that confined to
> only CZN based platforms?
> 

They're reported originally on CZN platforms.  If I was to hypothesize 
the root cause it is probably that the SMU is busy servicing other 
things during kernel's boot up.

If that's the case then there is no reason to believe it's fixed in 
follow on platforms.

> Thanks,
> Shyam
> 
>>
>>>
>>>    drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>>> b/drivers/platform/x86/amd/pmc/pmc.c
>>> index 1424c03c1f03..92adf4523736 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct
>>> amd_pmc_dev *dev)
>>>    {
>>>        int ret;
>>>    +    ret = amd_pmc_get_smu_version(dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>        switch (dev->cpu_id) {
>>>        case AMD_CPU_ID_YC:
>>>            if (!(dev->major > 90 || (dev->major == 90 && dev->minor >
>>> 39))) {
>>