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:
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:
