| Message ID | 20241028070438.1548737-8-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmc: Updates to AMD PMC driver | expand |
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@amd.com> > Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > 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: 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; } if (!dev->stb_arg.s2d_msg_id) { pr_warn("unsupported platform"); return false; } 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:
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@amd.com> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> 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: >
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@amd.com> >>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com> >>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>> --- >>> 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: > 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: >>
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@amd.com> >>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com> >>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >>>> --- >>>> 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: >>> >
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) { + 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: