[PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Mario Limonciello]

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)) {
+	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);
-- 
2.34.1

[PATCH 2/2] platform/x86/amd: pmc: Add a check for PMFW version on PS platform

[Mario Limonciello]

When running s2idle on PS if using PMFW older than 76.49.0 we will
trip up some bugs that will lead to freezes outside of the control
of Linux.  Set a guard that we only can enter HW sleep on 76.49.0
or newer for PS.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd/pmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
index c42fa47381c3..e7c6c06b2ccd 100644
--- a/drivers/platform/x86/amd/pmc.c
+++ b/drivers/platform/x86/amd/pmc.c
@@ -778,6 +778,14 @@ static void amd_pmc_s2idle_prepare(void)
 			return;
 		}
 		break;
+	case AMD_CPU_ID_PS:
+		if (!amd_pmc_verify_min_version(pdev, 76, 49)) {
+			dev_warn(pdev->dev,
+				 "hardware sleep unsupported with firmware %u.%u.%u\n",
+				 pdev->major, pdev->minor, pdev->rev);
+			return;
+		}
+		break;
 	}
 
 	msg = amd_pmc_get_os_hint(pdev);
-- 
2.34.1

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Hans de Goede]

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.

After verifying please post a v2 updating the commit message to
point out this functional change.

> +	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#  ?

Regards,

Hans

RE: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Limonciello, Mario]

[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?

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

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Hans de Goede]

Hi,

On 3/1/23 16:31, 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?
> 
>>
>> 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.

I'm fine with keeping the order as is, I'm also fine with swapping it :)

Regards,

Hans

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Shyam Sundar S K]

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().

Like we discussed off-list, we should have this boilerplate in place, so
that the future checks would not be duplicated.

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

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Mario Limonciello]

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.

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

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Shyam Sundar S K]

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?

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

Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

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