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@xxxxxxxxxxxx> > > Mark, does this patch solve the issue for you? > >> Link: https://lore.kernel.org/platform-driver-x86/3b224c62-a1d8-41bd-aced-5825f5f20e66@xxxxxxx/ >> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW") >> Suggested-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >> --- >> >> 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) >> > >
