[Public]
> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Sent: Thursday, January 20, 2022 12:56
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; open list:REAL TIME
> CLOCK (RTC) SUBSYSTEM <linux-rtc@xxxxxxxxxxxxxxx>; Jinke Fan
> <fanjinke@xxxxxxxx>; Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx>;
> Raul E Rangel <rrangel@xxxxxxxxxxxx>
> 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@xxxxxxxx>
> > > Cc: Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx>
> > > 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@xxxxxxxxxxxx>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > ---
> > > 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
