[PATCH] rtc: Fix the AltCentury for AMD platforms

[PATCH] rtc: Fix the AltCentury for AMD platforms

[Mario Limonciello]

Setting the century forward has been failing on AMD platforms.
There was a previous attempt at fixing this for family 0x17 as part of
commit 7ad295d5196a ("rtc: Fix the AltCentury value on AMD/Hygon
platform") but this was later reverted due to some problems reported
that appeared to stem from an FW bug on a family 0x17 desktop system.

The same comments mentioned in the previous commit continue to apply
to the newer platforms as well.

```
MC146818 driver use function mc146818_set_time() to set register
RTC_FREQ_SELECT(RTC_REG_A)'s bit4-bit6 field which means divider stage
reset value on Intel platform to 0x7.

While AMD/Hygon RTC_REG_A(0Ah)'s bit4 is defined as DV0 [Reference]:
DV0 = 0 selects Bank 0, DV0 = 1 selects Bank 1. Bit5-bit6 is defined
as reserved.

DV0 is set to 1, it will select Bank 1, which will disable AltCentury
register(0x32) access. As UEFI pass acpi_gbl_FADT.century 0x32
(AltCentury), the CMOS write will be failed on code:
CMOS_WRITE(century, acpi_gbl_FADT.century).

Correct RTC_REG_A bank select bit(DV0) to 0 on AMD/Hygon CPUs, it will
enable AltCentury(0x32) register writing and finally setup century as
expected.
```

However in closer examination the change previously submitted was also
modifying bits 5 & 6 which are declared reserved in the AMD documentation.
So instead modify just the DV0 bank selection bit.

Being cognizant that there was a failure reported before, split the code
change out to a static function that can also be used for exclusions if
any regressions such as Mikhail's pop up again.

Cc: Jinke Fan <fanjinke@hygon.cn>
Cc: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Link: https://lore.kernel.org/all/CABXGCsMLob0DC25JS8wwAYydnDoHBSoMh2_YLPfqm3TTvDE-Zw@mail.gmail.com/
Link: https://www.amd.com/system/files/TechDocs/51192_Bolton_FCH_RRG.pdf
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/rtc/rtc-mc146818-lib.c | 16 +++++++++++++++-
 include/linux/mc146818rtc.h    |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
index dcfaf09946ee..3c8be2136703 100644
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -120,6 +120,17 @@ unsigned int mc146818_get_time(struct rtc_time *time)
 }
 EXPORT_SYMBOL_GPL(mc146818_get_time);
 
+/* AMD systems don't allow access to AltCentury with DV1 */
+static bool apply_amd_register_a_behavior(void)
+{
+#ifdef CONFIG_X86
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
+		return true;
+#endif
+	return false;
+}
+
 /* Set the current date and time in the real time clock. */
 int mc146818_set_time(struct rtc_time *time)
 {
@@ -191,7 +202,10 @@ int mc146818_set_time(struct rtc_time *time)
 	save_control = CMOS_READ(RTC_CONTROL);
 	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
 	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
-	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
+	if (apply_amd_register_a_behavior())
+		CMOS_WRITE((save_freq_select & ~RTC_AMD_BANK_SELECT), RTC_FREQ_SELECT);
+	else
+		CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
 
 #ifdef CONFIG_MACH_DECSTATION
 	CMOS_WRITE(real_yrs, RTC_DEC_YEAR);
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index 0661af17a758..1e0205811394 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -86,6 +86,8 @@ struct cmos_rtc_board_info {
    /* 2 values for divider stage reset, others for "testing purposes only" */
 #  define RTC_DIV_RESET1	0x60
 #  define RTC_DIV_RESET2	0x70
+   /* In AMD BKDG bit 5 and 6 are reserved, bit 4 is for select dv0 bank */
+#  define RTC_AMD_BANK_SELECT	0x10
   /* Periodic intr. / Square wave rate select. 0=none, 1=32.8kHz,... 15=2Hz */
 # define RTC_RATE_SELECT 	0x0F
 
-- 
2.25.1

Re: [PATCH] rtc: Fix the AltCentury for AMD platforms

[Limonciello, Mario]

On 1/11/2022 16:57, Mario Limonciello wrote:
> Setting the century forward has been failing on AMD platforms.
> There was a previous attempt at fixing this for family 0x17 as part of
> commit 7ad295d5196a ("rtc: Fix the AltCentury value on AMD/Hygon
> platform") but this was later reverted due to some problems reported
> that appeared to stem from an FW bug on a family 0x17 desktop system.
> 
> The same comments mentioned in the previous commit continue to apply
> to the newer platforms as well.
> 
> ```
> MC146818 driver use function mc146818_set_time() to set register
> RTC_FREQ_SELECT(RTC_REG_A)'s bit4-bit6 field which means divider stage
> reset value on Intel platform to 0x7.
> 
> While AMD/Hygon RTC_REG_A(0Ah)'s bit4 is defined as DV0 [Reference]:
> DV0 = 0 selects Bank 0, DV0 = 1 selects Bank 1. Bit5-bit6 is defined
> as reserved.
> 
> DV0 is set to 1, it will select Bank 1, which will disable AltCentury
> register(0x32) access. As UEFI pass acpi_gbl_FADT.century 0x32
> (AltCentury), the CMOS write will be failed on code:
> CMOS_WRITE(century, acpi_gbl_FADT.century).
> 
> Correct RTC_REG_A bank select bit(DV0) to 0 on AMD/Hygon CPUs, it will
> enable AltCentury(0x32) register writing and finally setup century as
> expected.
> ```
> 
> However in closer examination the change previously submitted was also
> modifying bits 5 & 6 which are declared reserved in the AMD documentation.
> So instead modify just the DV0 bank selection bit.
> 
> Being cognizant that there was a failure reported before, split the code
> change out to a static function that can also be used for exclusions if
> any regressions such as Mikhail's pop up again.
> 
> Cc: Jinke Fan <fanjinke@hygon.cn>
> Cc: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Link: https://lore.kernel.org/all/CABXGCsMLob0DC25JS8wwAYydnDoHBSoMh2_YLPfqm3TTvDE-Zw@mail.gmail.com/
> Link: https://www.amd.com/system/files/TechDocs/51192_Bolton_FCH_RRG.pdf
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/rtc/rtc-mc146818-lib.c | 16 +++++++++++++++-
>   include/linux/mc146818rtc.h    |  2 ++
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
> index dcfaf09946ee..3c8be2136703 100644
> --- a/drivers/rtc/rtc-mc146818-lib.c
> +++ b/drivers/rtc/rtc-mc146818-lib.c
> @@ -120,6 +120,17 @@ unsigned int mc146818_get_time(struct rtc_time *time)
>   }
>   EXPORT_SYMBOL_GPL(mc146818_get_time);
>   
> +/* AMD systems don't allow access to AltCentury with DV1 */
> +static bool apply_amd_register_a_behavior(void)
> +{
> +#ifdef CONFIG_X86
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>   /* Set the current date and time in the real time clock. */
>   int mc146818_set_time(struct rtc_time *time)
>   {
> @@ -191,7 +202,10 @@ int mc146818_set_time(struct rtc_time *time)
>   	save_control = CMOS_READ(RTC_CONTROL);
>   	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
>   	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
> -	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
> +	if (apply_amd_register_a_behavior())
> +		CMOS_WRITE((save_freq_select & ~RTC_AMD_BANK_SELECT), RTC_FREQ_SELECT);
> +	else
> +		CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
>   
>   #ifdef CONFIG_MACH_DECSTATION
>   	CMOS_WRITE(real_yrs, RTC_DEC_YEAR);
> diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
> index 0661af17a758..1e0205811394 100644
> --- a/include/linux/mc146818rtc.h
> +++ b/include/linux/mc146818rtc.h
> @@ -86,6 +86,8 @@ struct cmos_rtc_board_info {
>      /* 2 values for divider stage reset, others for "testing purposes only" */
>   #  define RTC_DIV_RESET1	0x60
>   #  define RTC_DIV_RESET2	0x70
> +   /* In AMD BKDG bit 5 and 6 are reserved, bit 4 is for select dv0 bank */
> +#  define RTC_AMD_BANK_SELECT	0x10
>     /* Periodic intr. / Square wave rate select. 0=none, 1=32.8kHz,... 15=2Hz */
>   # define RTC_RATE_SELECT 	0x0F
>   

Hi Alexandre, Alessandro,

Friendly ping on this request.

Also if it wasn't made clear in my commit message or by analyzing this 
code change, I do believe that at least part of the reason for the 
previous regression was because of mucking with reserved bits.  This 
patch is much more conservative.

In my testing I found similar behaviors from the old regression on a 
more modern platform when those bits were being touched.

Mikhail,

As you flagged the previous regression, would appreciate if you're able 
to test the new patch (although of course many things in your situation 
have changed now).

Thanks.

Re: [PATCH] rtc: Fix the AltCentury for AMD platforms

[Alexandre Belloni]

On 20/01/2022 12:27:39-0600, Limonciello, Mario wrote:
> On 1/11/2022 16:57, Mario Limonciello wrote:
> > Setting the century forward has been failing on AMD platforms.
> > There was a previous attempt at fixing this for family 0x17 as part of
> > commit 7ad295d5196a ("rtc: Fix the AltCentury value on AMD/Hygon
> > platform") but this was later reverted due to some problems reported
> > that appeared to stem from an FW bug on a family 0x17 desktop system.
> > 
> > The same comments mentioned in the previous commit continue to apply
> > to the newer platforms as well.
> > 
> > ```
> > MC146818 driver use function mc146818_set_time() to set register
> > RTC_FREQ_SELECT(RTC_REG_A)'s bit4-bit6 field which means divider stage
> > reset value on Intel platform to 0x7.
> > 
> > While AMD/Hygon RTC_REG_A(0Ah)'s bit4 is defined as DV0 [Reference]:
> > DV0 = 0 selects Bank 0, DV0 = 1 selects Bank 1. Bit5-bit6 is defined
> > as reserved.
> > 
> > DV0 is set to 1, it will select Bank 1, which will disable AltCentury
> > register(0x32) access. As UEFI pass acpi_gbl_FADT.century 0x32
> > (AltCentury), the CMOS write will be failed on code:
> > CMOS_WRITE(century, acpi_gbl_FADT.century).
> > 
> > Correct RTC_REG_A bank select bit(DV0) to 0 on AMD/Hygon CPUs, it will
> > enable AltCentury(0x32) register writing and finally setup century as
> > expected.
> > ```
> > 
> > However in closer examination the change previously submitted was also
> > modifying bits 5 & 6 which are declared reserved in the AMD documentation.
> > So instead modify just the DV0 bank selection bit.
> > 
> > Being cognizant that there was a failure reported before, split the code
> > change out to a static function that can also be used for exclusions if
> > any regressions such as Mikhail's pop up again.
> > 
> > Cc: Jinke Fan <fanjinke@hygon.cn>
> > Cc: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> > Link: https://lore.kernel.org/all/CABXGCsMLob0DC25JS8wwAYydnDoHBSoMh2_YLPfqm3TTvDE-Zw@mail.gmail.com/
> > Link: https://www.amd.com/system/files/TechDocs/51192_Bolton_FCH_RRG.pdf
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >   drivers/rtc/rtc-mc146818-lib.c | 16 +++++++++++++++-
> >   include/linux/mc146818rtc.h    |  2 ++
> >   2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
> > index dcfaf09946ee..3c8be2136703 100644
> > --- a/drivers/rtc/rtc-mc146818-lib.c
> > +++ b/drivers/rtc/rtc-mc146818-lib.c
> > @@ -120,6 +120,17 @@ unsigned int mc146818_get_time(struct rtc_time *time)
> >   }
> >   EXPORT_SYMBOL_GPL(mc146818_get_time);
> > +/* AMD systems don't allow access to AltCentury with DV1 */
> > +static bool apply_amd_register_a_behavior(void)
> > +{
> > +#ifdef CONFIG_X86
> > +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> > +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> > +		return true;
> > +#endif
> > +	return false;
> > +}
> > +
> >   /* Set the current date and time in the real time clock. */
> >   int mc146818_set_time(struct rtc_time *time)
> >   {
> > @@ -191,7 +202,10 @@ int mc146818_set_time(struct rtc_time *time)
> >   	save_control = CMOS_READ(RTC_CONTROL);
> >   	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
> >   	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
> > -	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
> > +	if (apply_amd_register_a_behavior())
> > +		CMOS_WRITE((save_freq_select & ~RTC_AMD_BANK_SELECT), RTC_FREQ_SELECT);
> > +	else
> > +		CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
> >   #ifdef CONFIG_MACH_DECSTATION
> >   	CMOS_WRITE(real_yrs, RTC_DEC_YEAR);
> > diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
> > index 0661af17a758..1e0205811394 100644
> > --- a/include/linux/mc146818rtc.h
> > +++ b/include/linux/mc146818rtc.h
> > @@ -86,6 +86,8 @@ struct cmos_rtc_board_info {
> >      /* 2 values for divider stage reset, others for "testing purposes only" */
> >   #  define RTC_DIV_RESET1	0x60
> >   #  define RTC_DIV_RESET2	0x70
> > +   /* In AMD BKDG bit 5 and 6 are reserved, bit 4 is for select dv0 bank */
> > +#  define RTC_AMD_BANK_SELECT	0x10
> >     /* Periodic intr. / Square wave rate select. 0=none, 1=32.8kHz,... 15=2Hz */
> >   # define RTC_RATE_SELECT 	0x0F
> 
> Hi Alexandre, Alessandro,
> 
> Friendly ping on this request.
> 

This was sent too close from the merge window to be applied.

> Also if it wasn't made clear in my commit message or by analyzing this code
> change, I do believe that at least part of the reason for the previous
> regression was because of mucking with reserved bits.  This patch is much
> more conservative.
> 
> In my testing I found similar behaviors from the old regression on a more
> modern platform when those bits were being touched.
> 
> Mikhail,
> 
> As you flagged the previous regression, would appreciate if you're able to
> test the new patch (although of course many things in your situation have
> changed now).
> 
> Thanks.
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] rtc: Fix the AltCentury for AMD platforms

[Limonciello, Mario]

On 1/20/2022 12:56, Alexandre Belloni wrote:
> On 20/01/2022 12:27:39-0600, Limonciello, Mario wrote:
>> On 1/11/2022 16:57, Mario Limonciello wrote:
>>> Setting the century forward has been failing on AMD platforms.
>>> There was a previous attempt at fixing this for family 0x17 as part of
>>> commit 7ad295d5196a ("rtc: Fix the AltCentury value on AMD/Hygon
>>> platform") but this was later reverted due to some problems reported
>>> that appeared to stem from an FW bug on a family 0x17 desktop system.
>>>
>>> The same comments mentioned in the previous commit continue to apply
>>> to the newer platforms as well.
>>>
>>> ```
>>> MC146818 driver use function mc146818_set_time() to set register
>>> RTC_FREQ_SELECT(RTC_REG_A)'s bit4-bit6 field which means divider stage
>>> reset value on Intel platform to 0x7.
>>>
>>> While AMD/Hygon RTC_REG_A(0Ah)'s bit4 is defined as DV0 [Reference]:
>>> DV0 = 0 selects Bank 0, DV0 = 1 selects Bank 1. Bit5-bit6 is defined
>>> as reserved.
>>>
>>> DV0 is set to 1, it will select Bank 1, which will disable AltCentury
>>> register(0x32) access. As UEFI pass acpi_gbl_FADT.century 0x32
>>> (AltCentury), the CMOS write will be failed on code:
>>> CMOS_WRITE(century, acpi_gbl_FADT.century).
>>>
>>> Correct RTC_REG_A bank select bit(DV0) to 0 on AMD/Hygon CPUs, it will
>>> enable AltCentury(0x32) register writing and finally setup century as
>>> expected.
>>> ```
>>>
>>> However in closer examination the change previously submitted was also
>>> modifying bits 5 & 6 which are declared reserved in the AMD documentation.
>>> So instead modify just the DV0 bank selection bit.
>>>
>>> Being cognizant that there was a failure reported before, split the code
>>> change out to a static function that can also be used for exclusions if
>>> any regressions such as Mikhail's pop up again.
>>>
>>> Cc: Jinke Fan <fanjinke@hygon.cn>
>>> Cc: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2FCABXGCsMLob0DC25JS8wwAYydnDoHBSoMh2_YLPfqm3TTvDE-Zw%40mail.gmail.com%2F&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CzgBBpAXk2F6Aj1xuYtnUgN12%2BN%2F8CdJa3nexix3eQY%3D&amp;reserved=0
>>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F51192_Bolton_FCH_RRG.pdf&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=l54ilxU2BSuV7L%2ByptnzFFOYqm1gN4M85rzdhmgEJro%3D&amp;reserved=0
>>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>    drivers/rtc/rtc-mc146818-lib.c | 16 +++++++++++++++-
>>>    include/linux/mc146818rtc.h    |  2 ++
>>>    2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
>>> index dcfaf09946ee..3c8be2136703 100644
>>> --- a/drivers/rtc/rtc-mc146818-lib.c
>>> +++ b/drivers/rtc/rtc-mc146818-lib.c
>>> @@ -120,6 +120,17 @@ unsigned int mc146818_get_time(struct rtc_time *time)
>>>    }
>>>    EXPORT_SYMBOL_GPL(mc146818_get_time);
>>> +/* AMD systems don't allow access to AltCentury with DV1 */
>>> +static bool apply_amd_register_a_behavior(void)
>>> +{
>>> +#ifdef CONFIG_X86
>>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
>>> +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
>>> +		return true;
>>> +#endif
>>> +	return false;
>>> +}
>>> +
>>>    /* Set the current date and time in the real time clock. */
>>>    int mc146818_set_time(struct rtc_time *time)
>>>    {
>>> @@ -191,7 +202,10 @@ int mc146818_set_time(struct rtc_time *time)
>>>    	save_control = CMOS_READ(RTC_CONTROL);
>>>    	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
>>>    	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
>>> -	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
>>> +	if (apply_amd_register_a_behavior())
>>> +		CMOS_WRITE((save_freq_select & ~RTC_AMD_BANK_SELECT), RTC_FREQ_SELECT);
>>> +	else
>>> +		CMOS_WRITE((save_freq_select|RTC_DIV_RESET2), RTC_FREQ_SELECT);
>>>    #ifdef CONFIG_MACH_DECSTATION
>>>    	CMOS_WRITE(real_yrs, RTC_DEC_YEAR);
>>> diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
>>> index 0661af17a758..1e0205811394 100644
>>> --- a/include/linux/mc146818rtc.h
>>> +++ b/include/linux/mc146818rtc.h
>>> @@ -86,6 +86,8 @@ struct cmos_rtc_board_info {
>>>       /* 2 values for divider stage reset, others for "testing purposes only" */
>>>    #  define RTC_DIV_RESET1	0x60
>>>    #  define RTC_DIV_RESET2	0x70
>>> +   /* In AMD BKDG bit 5 and 6 are reserved, bit 4 is for select dv0 bank */
>>> +#  define RTC_AMD_BANK_SELECT	0x10
>>>      /* Periodic intr. / Square wave rate select. 0=none, 1=32.8kHz,... 15=2Hz */
>>>    # define RTC_RATE_SELECT 	0x0F
>>
>> Hi Alexandre, Alessandro,
>>
>> Friendly ping on this request.
>>
> 
> This was sent too close from the merge window to be applied.

I suspected that for 5.17 - but could you at least review it for 5.18 
and ACK/NACK and if ACK put it in your -for-next?

> 
>> Also if it wasn't made clear in my commit message or by analyzing this code
>> change, I do believe that at least part of the reason for the previous
>> regression was because of mucking with reserved bits.  This patch is much
>> more conservative.
>>
>> In my testing I found similar behaviors from the old regression on a more
>> modern platform when those bits were being touched.
>>
>> Mikhail,
>>
>> As you flagged the previous regression, would appreciate if you're able to
>> test the new patch (although of course many things in your situation have
>> changed now).
>>
>> Thanks.
>>
> 

RE: [PATCH] rtc: Fix the AltCentury for AMD platforms

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Thursday, January 20, 2022 12:56
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; open list:REAL TIME
> CLOCK (RTC) SUBSYSTEM <linux-rtc@vger.kernel.org>; Jinke Fan
> <fanjinke@hygon.cn>; Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>;
> Raul E Rangel <rrangel@chromium.org>
> Subject: Re: [PATCH] rtc: Fix the AltCentury for AMD platforms
> 
> On 20/01/2022 12:27:39-0600, Limonciello, Mario wrote:
> > On 1/11/2022 16:57, Mario Limonciello wrote:
> > > Setting the century forward has been failing on AMD platforms.
> > > There was a previous attempt at fixing this for family 0x17 as part of
> > > commit 7ad295d5196a ("rtc: Fix the AltCentury value on AMD/Hygon
> > > platform") but this was later reverted due to some problems reported
> > > that appeared to stem from an FW bug on a family 0x17 desktop system.
> > >
> > > The same comments mentioned in the previous commit continue to
> apply
> > > to the newer platforms as well.
> > >
> > > ```
> > > MC146818 driver use function mc146818_set_time() to set register
> > > RTC_FREQ_SELECT(RTC_REG_A)'s bit4-bit6 field which means divider
> stage
> > > reset value on Intel platform to 0x7.
> > >
> > > While AMD/Hygon RTC_REG_A(0Ah)'s bit4 is defined as DV0 [Reference]:
> > > DV0 = 0 selects Bank 0, DV0 = 1 selects Bank 1. Bit5-bit6 is defined
> > > as reserved.
> > >
> > > DV0 is set to 1, it will select Bank 1, which will disable AltCentury
> > > register(0x32) access. As UEFI pass acpi_gbl_FADT.century 0x32
> > > (AltCentury), the CMOS write will be failed on code:
> > > CMOS_WRITE(century, acpi_gbl_FADT.century).
> > >
> > > Correct RTC_REG_A bank select bit(DV0) to 0 on AMD/Hygon CPUs, it will
> > > enable AltCentury(0x32) register writing and finally setup century as
> > > expected.
> > > ```
> > >
> > > However in closer examination the change previously submitted was also
> > > modifying bits 5 & 6 which are declared reserved in the AMD
> documentation.
> > > So instead modify just the DV0 bank selection bit.
> > >
> > > Being cognizant that there was a failure reported before, split the code
> > > change out to a static function that can also be used for exclusions if
> > > any regressions such as Mikhail's pop up again.
> > >
> > > Cc: Jinke Fan <fanjinke@hygon.cn>
> > > Cc: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> > > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fall%2FCABXGCsMLob0DC25JS8wwAYydnDoHBSoMh2_YLPfqm
> 3TTvDE-
> Zw%40mail.gmail.com%2F&amp;data=04%7C01%7Cmario.limonciello%40am
> d.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a
> 82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0%3D%7C3000&amp;sdata=CzgBBpAXk2F6Aj1xuYtnUgN12%2BN%2F8CdJa3
> nexix3eQY%3D&amp;reserved=0
> > > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F51192_Bolton_FCH_RRG.pd
> f&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864
> b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%
> 7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sda
> ta=l54ilxU2BSuV7L%2ByptnzFFOYqm1gN4M85rzdhmgEJro%3D&amp;reserve
> d=0
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/rtc/rtc-mc146818-lib.c | 16 +++++++++++++++-
> > >   include/linux/mc146818rtc.h    |  2 ++
> > >   2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
> > > index dcfaf09946ee..3c8be2136703 100644
> > > --- a/drivers/rtc/rtc-mc146818-lib.c
> > > +++ b/drivers/rtc/rtc-mc146818-lib.c
> > > @@ -120,6 +120,17 @@ unsigned int mc146818_get_time(struct rtc_time
> *time)
> > >   }
> > >   EXPORT_SYMBOL_GPL(mc146818_get_time);
> > > +/* AMD systems don't allow access to AltCentury with DV1 */
> > > +static bool apply_amd_register_a_behavior(void)
> > > +{
> > > +#ifdef CONFIG_X86
> > > +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> > > +	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
> > > +		return true;
> > > +#endif
> > > +	return false;
> > > +}
> > > +
> > >   /* Set the current date and time in the real time clock. */
> > >   int mc146818_set_time(struct rtc_time *time)
> > >   {
> > > @@ -191,7 +202,10 @@ int mc146818_set_time(struct rtc_time *time)
> > >   	save_control = CMOS_READ(RTC_CONTROL);
> > >   	CMOS_WRITE((save_control|RTC_SET), RTC_CONTROL);
> > >   	save_freq_select = CMOS_READ(RTC_FREQ_SELECT);
> > > -	CMOS_WRITE((save_freq_select|RTC_DIV_RESET2),
> RTC_FREQ_SELECT);
> > > +	if (apply_amd_register_a_behavior())
> > > +		CMOS_WRITE((save_freq_select &
> ~RTC_AMD_BANK_SELECT), RTC_FREQ_SELECT);
> > > +	else
> > > +		CMOS_WRITE((save_freq_select|RTC_DIV_RESET2),
> RTC_FREQ_SELECT);
> > >   #ifdef CONFIG_MACH_DECSTATION
> > >   	CMOS_WRITE(real_yrs, RTC_DEC_YEAR);
> > > diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
> > > index 0661af17a758..1e0205811394 100644
> > > --- a/include/linux/mc146818rtc.h
> > > +++ b/include/linux/mc146818rtc.h
> > > @@ -86,6 +86,8 @@ struct cmos_rtc_board_info {
> > >      /* 2 values for divider stage reset, others for "testing purposes only"
> */
> > >   #  define RTC_DIV_RESET1	0x60
> > >   #  define RTC_DIV_RESET2	0x70
> > > +   /* In AMD BKDG bit 5 and 6 are reserved, bit 4 is for select dv0 bank */
> > > +#  define RTC_AMD_BANK_SELECT	0x10
> > >     /* Periodic intr. / Square wave rate select. 0=none, 1=32.8kHz,...
> 15=2Hz */
> > >   # define RTC_RATE_SELECT 	0x0F
> >
> > Hi Alexandre, Alessandro,
> >
> > Friendly ping on this request.
> >
> 
> This was sent too close from the merge window to be applied.

HI Alexandre,

I checked and didn't see this come into master as most of the 5.18 trees came in.
Did this get forgotten?

Thanks,

> 
> > Also if it wasn't made clear in my commit message or by analyzing this code
> > change, I do believe that at least part of the reason for the previous
> > regression was because of mucking with reserved bits.  This patch is much
> > more conservative.
> >
> > In my testing I found similar behaviors from the old regression on a more
> > modern platform when those bits were being touched.
> >
> > Mikhail,
> >
> > As you flagged the previous regression, would appreciate if you're able to
> > test the new patch (although of course many things in your situation have
> > changed now).
> >
> > Thanks.
> >
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fboot
> lin.com%2F&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C3357
> df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%
> 7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0&amp;sdata=gvm24hJhVEsXrRQT6c1GtkMhi3q77Df1Biowb6gVdS8%3D&am
> p;reserved=0

Re: [PATCH] rtc: Fix the AltCentury for AMD platforms

[Alexandre Belloni]

On 29/03/2022 20:36:46+0000, Limonciello, Mario wrote:
> HI Alexandre,
> 
> I checked and didn't see this come into master as most of the 5.18 trees came in.
> Did this get forgotten?
> 

Not really, this was in my back log and actually the first one I wanted
to apply right now.

> Thanks,
> 
> > 
> > > Also if it wasn't made clear in my commit message or by analyzing this code
> > > change, I do believe that at least part of the reason for the previous
> > > regression was because of mucking with reserved bits.  This patch is much
> > > more conservative.
> > >
> > > In my testing I found similar behaviors from the old regression on a more
> > > modern platform when those bits were being touched.
> > >
> > > Mikhail,
> > >
> > > As you flagged the previous regression, would appreciate if you're able to
> > > test the new patch (although of course many things in your situation have
> > > changed now).
> > >
> > > Thanks.
> > >
> > 
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fboot
> > lin.com%2F&data=04%7C01%7Cmario.limonciello%40amd.com%7C3357
> > df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%
> > 7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> > 0&sdata=gvm24hJhVEsXrRQT6c1GtkMhi3q77Df1Biowb6gVdS8%3D&am
> > p;reserved=0

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com