| Message ID | 20231009144748.24054-1-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism | expand |
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? > > 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))) {
On Mon, 9 Oct 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. > > 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 > > 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))) { Does this really belong here? Correct me if I recall wrong but the reason why amd_pmc_get_smu_version() was not always done during init was that it added noticeable delay to boot? Based on that, I kind of assumed it's generic thing (and a such, unrelated to amd_pmc_get_dram_size()) so why is this call this deep in the call chain? Perhaps amd_pmc_s2d_init() would be more appropriate place for it since you now make the call unconditional anyway for that code path?
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? 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))) { >
On 10/9/2023 8:29 PM, Ilpo Järvinen wrote: > On Mon, 9 Oct 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. >> >> 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 >> >> 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))) { > > Does this really belong here? Correct me if I recall wrong but the reason > why amd_pmc_get_smu_version() was not always done during init was that it > added noticeable delay to boot? Based on that, I kind of assumed it's > generic thing (and a such, unrelated to amd_pmc_get_dram_size()) so why is > this call this deep in the call chain? Perhaps amd_pmc_s2d_init() would > be more appropriate place for it since you now make the call > unconditional anyway for that code path? > Yes you are right. I see a remark from Mario also on the similar lines. The best thing is, let me go back to our FW team to understand when do they report dram_size and what is the implication of calling get_smu_version(). Thanks, Shyam
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))) { >>
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))) {
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 drivers/platform/x86/amd/pmc/pmc.c | 4 ++++ 1 file changed, 4 insertions(+)