On 10/28/2024 22:18, Mario Limonciello wrote: > On 10/28/2024 02:04, Shyam Sundar S K wrote: >> Previously, AMD's Ryzen Desktop SoCs did not include support for STB. >> However, to accommodate this recent change, PMFW has implemented a new >> message port pair mechanism for handling messages, arguments, and >> responses, specifically designed for distinguishing from Mobile SoCs. >> Therefore, it is necessary to update the driver to properly handle this >> incoming change. >> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx> >> --- >> drivers/platform/x86/amd/pmc/mp1_stb.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c >> b/drivers/platform/x86/amd/pmc/mp1_stb.c >> index 917c111b31c9..bafc07556283 100644 >> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c >> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c >> @@ -36,6 +36,11 @@ >> #define AMD_S2D_REGISTER_RESPONSE 0xA80 >> #define AMD_S2D_REGISTER_ARGUMENT 0xA88 >> +/* STB S2D(Spill to DRAM) message port offset for 44h model */ >> +#define AMD_GNR_REGISTER_MESSAGE 0x524 >> +#define AMD_GNR_REGISTER_RESPONSE 0x570 >> +#define AMD_GNR_REGISTER_ARGUMENT 0xA40 >> + >> static bool enable_stb; >> module_param(enable_stb, bool, 0644); >> MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism"); >> @@ -244,6 +249,13 @@ static bool amd_is_stb_supported(struct >> amd_pmc_dev *dev) >> switch (dev->cpu_id) { >> case AMD_CPU_ID_YC: >> case AMD_CPU_ID_CB: >> + if (boot_cpu_data.x86_model == 0x44) { > > It's unfortunate that this information can't be discovered from the > F/W, because this code is now turning into: > That's true. > switch(dev->cpu_id) > case FOO: > if (boot_cpu_data.x86_model == BAR) > > That's pretty ugly IMO. > > If you're doing to have a check like that, I think it's better to just > ditch the cpu_id (root port PCI ID detection) and go all in on x86 > checks like this instead: > > if (cpu_feature_enabled(X86_FEATURE_ZEN4)) { > switch (boot_cpu_data.x86_model) > case 0x60 .. 0x6f: > //set up args > break; > default: > break; > } else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) { > switch (boot_cpu_data.x86_model) > case 0x40 .. 0x44: > //set up args > break; > default: > break; > } The only benefit I see is instead of using cpu_id, using cpu_feature_enabled() to know the underlying generation. We have to update two things with the evolution of family, i.e. s2d_msg_id that is changed (and getting changed..) from each generation/family/model and next is the passing the right arguments to PMFW (i.e. msg/arg/resp). But the catch is, the s2d_msg_id is tied to the model, for which we will still have to depend of boot_cpu_data.x86_model() to get to know the information. So, the code will turn something like this: { if (cpu_feature_enabled(X86_FEATURE_ZEN2)) { dev->stb_arg.s2d_msg_id = 0xBE; } else if (cpu_feature_enabled(X86_FEATURE_ZEN3)) { dev->stb_arg.s2d_msg_id = 0xBE; } else if (cpu_feature_enabled(X86_FEATURE_ZEN4)) { switch (boot_cpu_data.x86_model) { case 0x60: dev->stb_arg.s2d_msg_id = 0xBE; break; default: dev->stb_arg.s2d_msg_id = 0x85; break; } } else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) { switch (boot_cpu_data.x86_model) { case 0x24: dev->stb_arg.s2d_msg_id = 0xDE; break; case 0x70: dev->stb_arg.s2d_msg_id = 0xF1; break; case 0x44: dev->stb_arg.s2d_msg_id = 0x9B; //update stb args break; default: break; } } //update stb args } IMO, this still looks clumsy. So, I will take your input of using cpu_feature_enabled() and drop cpu_id from root port.. but, I have a code simplification that will be addressed in v2. > > if (!dev->stb_arg.s2d_msg_id) { > pr_warn("unsupported platform"); > return false; > } > This is not required, as we will end up having unnecessary spew when this function gets triggered on platforms that wont support Spill to DRAM, i.e Cezzane or lower. I have looked your comments on 6/8. Will address them with cpu_feature_enabled() + code simplification. Thanks, Shyam > return true; > > Then each time a new one is added it's a lot easier to follow what > it's really matching. > >> + dev->stb_arg.s2d_msg_id = 0x9B; >> + dev->stb_arg.msg = AMD_GNR_REGISTER_MESSAGE; >> + dev->stb_arg.arg = AMD_GNR_REGISTER_ARGUMENT; >> + dev->stb_arg.resp = AMD_GNR_REGISTER_RESPONSE; >> + return true; >> + } >> dev->stb_arg.s2d_msg_id = 0xBE; >> break; >> case AMD_CPU_ID_PS: >