Messages in this thread | | | Date | Wed, 1 Mar 2023 20:54:40 -0600 | Subject | Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version | From | Mario Limonciello <> |
| |
On 3/1/23 20:50, Shyam Sundar S K wrote: > > > On 3/2/2023 8:12 AM, Mario Limonciello wrote: >> On 3/1/23 20:39, Shyam Sundar S K wrote: >>> Hi Mario, >>> >>> On 3/1/2023 9:01 PM, Limonciello, Mario wrote: >>>> [Public] >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: Hans de Goede <hdegoede@redhat.com> >>>>> Sent: Wednesday, March 1, 2023 09:28 >>>>> To: Limonciello, Mario <Mario.Limonciello@amd.com>; S-k, Shyam-sundar >>>>> <Shyam-sundar.S-k@amd.com> >>>>> Cc: Mark Gross <markgross@kernel.org>; platform-driver- >>>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org >>>>> Subject: Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for >>>>> checking >>>>> minimum SMU version >>>>> >>>>> Hi, >>>>> >>>>> On 3/1/23 16:08, Mario Limonciello wrote: >>>>>> In a few locations there is some boilerplate code for checking >>>>>> minimum SMU version. Switch this to a helper for this check. >>>>>> >>>>>> No intended functional changes. >>>>>> >>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>>>>> --- >>>>>> drivers/platform/x86/amd/pmc.c | 49 >>>>>> +++++++++++++++++----------------- >>>>>> 1 file changed, 24 insertions(+), 25 deletions(-) >>>>>> >>>>>> diff --git a/drivers/platform/x86/amd/pmc.c >>>>> b/drivers/platform/x86/amd/pmc.c >>>>>> index 2edaae04a691..c42fa47381c3 100644 >>>>>> --- a/drivers/platform/x86/amd/pmc.c >>>>>> +++ b/drivers/platform/x86/amd/pmc.c >>>>>> @@ -418,6 +418,22 @@ static int amd_pmc_get_smu_version(struct >>>>> amd_pmc_dev *dev) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static bool amd_pmc_verify_min_version(struct amd_pmc_dev *pdev, >>>>> int major, int minor) >>>>>> +{ >>>>>> + if (!pdev->major) { >>>>>> + int rc = amd_pmc_get_smu_version(pdev); >>>>>> + >>>>>> + if (rc) { >>>>>> + dev_warn(pdev->dev, "failed to read SMU version: >>>>> %d\n", rc); >>>>>> + return false; >>>>>> + } >>>>>> + } >>>>>> + if (pdev->major > major) >>>>>> + return true; >>>>>> + >>>>>> + return pdev->major == major && pdev->minor >= minor; >>>>>> +} >>>>>> + >>>>>> static ssize_t smu_fw_version_show(struct device *d, struct >>>>> device_attribute *attr, >>>>>> char *buf) >>>>>> { >>>>>> @@ -526,14 +542,7 @@ static int amd_pmc_idlemask_show(struct seq_file >>>>> *s, void *unused) >>>>>> struct amd_pmc_dev *dev = s->private; >>>>>> int rc; >>>>>> >>>>>> - /* we haven't yet read SMU version */ >>>>>> - if (!dev->major) { >>>>>> - rc = amd_pmc_get_smu_version(dev); >>>>>> - if (rc) >>>>>> - return rc; >>>>>> - } >>>>>> - >>>>>> - if (dev->major > 56 || (dev->major >= 55 && dev->minor >= 37)) { >>>>> >>>>> The 2 major checks here originally were not in sync, so for major == 55 >>>>> *and* major == 56 it would only succeed if minor >= 37. >>>>> >>>>> Where as after this patch for major == 56 it will now always succeed. >>>>> >>>>> This feels like a bug in the original code, but might have been >>>>> intentional ? Please verify this. >>>> >>>> @S-k, Shyam-sundar as the original author of that, can you please >>>> confirm? >>> >>> I cannot completely recall :-) It was something like if the major >>> version is greater than 56, there is no need to check the other part of >>> the "OR". >>> >>> which is kind of similar to what you are now doing in >>> amd_pmc_verify_min_version(). >> >> OK yeah, then I'll split this correction of the logic off to that in a >> separate patch to make this one "really no intended functional changes". >> >>> >>> Like we discussed off-list, we should have this boilerplate in place, so >>> that the future checks would not be duplicated. >> >> Something else I noticed that we probably need to consider is that there >> is no examination for the "program" version which may be important. >> >> We don't have any version checks for YC, but if we did for example YC A0 >> and YC B0 use program "0" or program "4" respectively so version checks >> could fall over. > > Checking for "program" version may not be required as A0/B0 are never > meant for production and IMO its a logical overhead. > > Do you have a specific case, were you felt the real usage of "program" > version?
A0/B0 of YC was just one example. The reason I mention it is I think technically it's possible that A0 is leveraged for an embedded product.
I know similar thing had occurred for Renoir leveraged products too.
> >> >> I'll add something like this in for v2 of the patch as well. >> >>> >>> Thanks, >>> Shyam >>> >>>> >>>>> >>>>> After verifying please post a v2 updating the commit message to >>>>> point out this functional change. >>>>> >>>> >>>> Sure, thanks. >>>> >>>>>> + if (amd_pmc_verify_min_version(dev, 55, 37)) { >>>>>> rc = amd_pmc_idlemask_read(dev, NULL, s); >>>>>> if (rc) >>>>>> return rc; >>>>>> @@ -686,15 +695,8 @@ static int amd_pmc_get_os_hint(struct >>>>> amd_pmc_dev *dev) >>>>>> static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev) >>>>>> { >>>>>> struct device *d; >>>>>> - int rc; >>>>>> >>>>>> - if (!pdev->major) { >>>>>> - rc = amd_pmc_get_smu_version(pdev); >>>>>> - if (rc) >>>>>> - return rc; >>>>>> - } >>>>>> - >>>>>> - if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65)) >>>>>> + if (amd_pmc_verify_min_version(pdev, 64, 66)) >>>>>> return 0; >>>>>> >>>>>> d = bus_find_device_by_name(&serio_bus, NULL, "serio0"); >>>>>> @@ -718,14 +720,10 @@ static int amd_pmc_verify_czn_rtc(struct >>>>> amd_pmc_dev *pdev, u32 *arg) >>>>>> struct rtc_time tm; >>>>>> int rc; >>>>>> >>>>>> - /* we haven't yet read SMU version */ >>>>>> - if (!pdev->major) { >>>>>> - rc = amd_pmc_get_smu_version(pdev); >>>>>> - if (rc) >>>>>> - return rc; >>>>>> - } >>>>>> + if (disable_workarounds) >>>>>> + return 0; >>>>>> >>>>>> - if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53)) >>>>>> + if (!amd_pmc_verify_min_version(pdev, 64, 53)) >>>>>> return 0; >>>>>> >>>>>> rtc_device = rtc_class_open("rtc0"); >>>>>> @@ -772,13 +770,14 @@ static void amd_pmc_s2idle_prepare(void) >>>>>> /* Reset and Start SMU logging - to monitor the s0i3 stats */ >>>>>> amd_pmc_setup_smu_logging(pdev); >>>>>> >>>>>> - /* Activate CZN specific platform bug workarounds */ >>>>>> - if (pdev->cpu_id == AMD_CPU_ID_CZN && !disable_workarounds) { >>>>>> + switch (pdev->cpu_id) { >>>>>> + case AMD_CPU_ID_CZN: >>>>>> rc = amd_pmc_verify_czn_rtc(pdev, &arg); >>>>>> if (rc) { >>>>>> dev_err(pdev->dev, "failed to set RTC: %d\n", rc); >>>>>> return; >>>>>> } >>>>>> + break; >>>>>> } >>>>>> >>>>>> msg = amd_pmc_get_os_hint(pdev); >>>>> >>>>> >>>>> Patch 2/2 looks good to me. >>>>> >>>>> Should I queue v2 (once posted) up as a fix for 6.3-rc# ? >>>> >>>> Yes please. If it makes it easier I can re-order the series so that >>>> we add a check in 1/2 and switch to the helper as 2/2. >>>> >>>> This might make it easier to take the LTS kernel too for stable, >>>> but I don't feel strongly. >>>> >>>>> >>>>> Regards, >>>>> >>>>> Hans >>
| |