| Message ID | 20220111225750.1699-1-mario.limonciello@amd.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtc: Fix the AltCentury for AMD platforms | expand |
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.
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. >
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&data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=CzgBBpAXk2F6Aj1xuYtnUgN12%2BN%2F8CdJa3nexix3eQY%3D&reserved=0 >>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F51192_Bolton_FCH_RRG.pdf&data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=l54ilxU2BSuV7L%2ByptnzFFOYqm1gN4M85rzdhmgEJro%3D&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. >> >
[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&data=04%7C01%7Cmario.limonciello%40am > d.com%7C3357df64bb864b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a > 82d994e183d%7C0%7C0%7C637783018746468575%7CUnknown%7CTWFpbGZ > sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M > n0%3D%7C3000&sdata=CzgBBpAXk2F6Aj1xuYtnUgN12%2BN%2F8CdJa3 > nexix3eQY%3D&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&data=04%7C01%7Cmario.limonciello%40amd.com%7C3357df64bb864 > b6ef63e08d9dc468eae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0% > 7C637783018746468575%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sda > ta=l54ilxU2BSuV7L%2ByptnzFFOYqm1gN4M85rzdhmgEJro%3D&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&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
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
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
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(-)