On 10/29/2024 19:45, Mario Limonciello wrote: > On 10/29/2024 09:09, Shyam Sundar S K wrote: >> >> >> 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: > > Can you double check hardware documentation? I believe these should > generally be ranges, IE: > > case 0x70 .. 0x7f: Yes. I double checked it. The range would be applicable only for kicker programs and not all generations may have a kicker. Also, not all BIOSes support STB/S2D. So, these models have to be added only on need basis. Thanks, Shyam > >> 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. > > Yeah but it is closer to how all the arch/x86 code works too and at > least logical to a bystander. > >> 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. > > Great thanks! > >> >>> >>> 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: >>> >