| Message ID | 20180620030240.2409-1-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [edk2,v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling | expand |
On 20 June 2018 at 05:02, <masahisa.kojima@linaro.org> wrote: > From: Masahisa Kojima <masahisa.kojima@linaro.org> > > BcdToDecimal8() in LibGetTime() asserts with the > following condition. > 1) RTC device has not been initialized yet, RTC device > returns indeterminate value with VL bit(1) > 2) DEBUG build > > UEFI boot fails with assertion when it satisfies above conditions. > > Aside form boot failure, UEFI shell commands "date/time" expect > that getting time from RTC should success when user sets the time. > ShellCommandRunTime() performs GetTime()->update time->SetTime(), > if the first GetTime() fails, user can not set time. > With that, simply returning EFI_DEVICE_ERROR in LibGetTime() > is not applicable to VL bit handling. > > To avoid this situation, even if it only occurs in DEBUG build, > RTC driver should check the VL bit in the VL_seconds register. > This VL bit is voltage-low detector, it means integrity of the > clock information is not guaranteed if it sets to 1. In this > case, driver return dummy date/time(01/01/2000 00:00:00) to > proceed succeeding process. > > linux driver also checks this bit when driver gets the time > from RTC. If VL bit is 1, linux driver discard the retreived > time data. > > Note that if VL bit is 1, GetTime() returns always > 01/01/2000 00:00:00 until user sets date/time. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com> > --- > .../Pcf8563RealTimeClockLib.c | 31 +++++++++++++++------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > index fb58e1feb4..9f49ae91de 100644 > --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > @@ -19,11 +19,13 @@ > #include <Library/UefiBootServicesTableLib.h> > #include <Library/UefiLib.h> > #include <Library/UefiRuntimeLib.h> > +#include <Library/BaseMemoryLib.h> > #include <Protocol/I2cMaster.h> > > #define SLAVE_ADDRESS (FixedPcdGet8 (PcdI2cSlaveAddress)) > #define PCF8563_DATA_REG_OFFSET 0x2 > > +#define PCF8563_CLOCK_INVALID 0x80 > #define PCF8563_SECONDS_MASK 0x7f > #define PCF8563_MINUTES_MASK 0x7f > #define PCF8563_HOURS_MASK 0x3f > @@ -95,6 +97,7 @@ LibGetTime ( > RTC_DATETIME DateTime; > EFI_STATUS Status; > UINT8 Reg; > + EFI_TIME InitTime; > > if (Time == NULL) { > return EFI_INVALID_PARAMETER; > @@ -122,15 +125,25 @@ LibGetTime ( > return EFI_DEVICE_ERROR; > } > > - Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); > - Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); > - Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); > - Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); > - Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); > - Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; > - > - if (DateTime.Century_months & PCF8563_CENTURY_MASK) { > - Time->Year += 100; > + if ((DateTime.VL_seconds & PCF8563_CLOCK_INVALID) != 0) { > + InitTime.Second = 0; > + InitTime.Minute = 0; > + InitTime.Hour = 0; > + InitTime.Day = 1; > + InitTime.Month = 1; > + InitTime.Year = EPOCH_BASE; > + (VOID)CopyMem (Time, &InitTime, sizeof(EFI_TIME)); I don't think we need InitTime or this CopyMem(). instead, we can just set the Time-> fields directly, no? > + } else { > + Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); > + Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); > + Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); > + Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); > + Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); > + Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; > + > + if (DateTime.Century_months & PCF8563_CENTURY_MASK) { > + Time->Year += 100; > + } > } > > if (Capabilities != NULL) { > -- > 2.14.2 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ard, Yes, you are correct. I will send update patch. Thank you. On Wed, 20 Jun 2018 at 15:21, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 20 June 2018 at 05:02, <masahisa.kojima@linaro.org> wrote: > > From: Masahisa Kojima <masahisa.kojima@linaro.org> > > > > BcdToDecimal8() in LibGetTime() asserts with the > > following condition. > > 1) RTC device has not been initialized yet, RTC device > > returns indeterminate value with VL bit(1) > > 2) DEBUG build > > > > UEFI boot fails with assertion when it satisfies above conditions. > > > > Aside form boot failure, UEFI shell commands "date/time" expect > > that getting time from RTC should success when user sets the time. > > ShellCommandRunTime() performs GetTime()->update time->SetTime(), > > if the first GetTime() fails, user can not set time. > > With that, simply returning EFI_DEVICE_ERROR in LibGetTime() > > is not applicable to VL bit handling. > > > > To avoid this situation, even if it only occurs in DEBUG build, > > RTC driver should check the VL bit in the VL_seconds register. > > This VL bit is voltage-low detector, it means integrity of the > > clock information is not guaranteed if it sets to 1. In this > > case, driver return dummy date/time(01/01/2000 00:00:00) to > > proceed succeeding process. > > > > linux driver also checks this bit when driver gets the time > > from RTC. If VL bit is 1, linux driver discard the retreived > > time data. > > > > Note that if VL bit is 1, GetTime() returns always > > 01/01/2000 00:00:00 until user sets date/time. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@socionext.com> > > --- > > .../Pcf8563RealTimeClockLib.c | 31 +++++++++++++++------- > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > > index fb58e1feb4..9f49ae91de 100644 > > --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > > +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > > @@ -19,11 +19,13 @@ > > #include <Library/UefiBootServicesTableLib.h> > > #include <Library/UefiLib.h> > > #include <Library/UefiRuntimeLib.h> > > +#include <Library/BaseMemoryLib.h> > > #include <Protocol/I2cMaster.h> > > > > #define SLAVE_ADDRESS (FixedPcdGet8 (PcdI2cSlaveAddress)) > > #define PCF8563_DATA_REG_OFFSET 0x2 > > > > +#define PCF8563_CLOCK_INVALID 0x80 > > #define PCF8563_SECONDS_MASK 0x7f > > #define PCF8563_MINUTES_MASK 0x7f > > #define PCF8563_HOURS_MASK 0x3f > > @@ -95,6 +97,7 @@ LibGetTime ( > > RTC_DATETIME DateTime; > > EFI_STATUS Status; > > UINT8 Reg; > > + EFI_TIME InitTime; > > > > if (Time == NULL) { > > return EFI_INVALID_PARAMETER; > > @@ -122,15 +125,25 @@ LibGetTime ( > > return EFI_DEVICE_ERROR; > > } > > > > - Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); > > - Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); > > - Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); > > - Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); > > - Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); > > - Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; > > - > > - if (DateTime.Century_months & PCF8563_CENTURY_MASK) { > > - Time->Year += 100; > > + if ((DateTime.VL_seconds & PCF8563_CLOCK_INVALID) != 0) { > > + InitTime.Second = 0; > > + InitTime.Minute = 0; > > + InitTime.Hour = 0; > > + InitTime.Day = 1; > > + InitTime.Month = 1; > > + InitTime.Year = EPOCH_BASE; > > + (VOID)CopyMem (Time, &InitTime, sizeof(EFI_TIME)); > > I don't think we need InitTime or this CopyMem(). instead, we can just > set the Time-> fields directly, no? > > > + } else { > > + Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); > > + Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); > > + Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); > > + Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); > > + Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); > > + Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; > > + > > + if (DateTime.Century_months & PCF8563_CENTURY_MASK) { > > + Time->Year += 100; > > + } > > } > > > > if (Capabilities != NULL) { > > -- > > 2.14.2 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c index fb58e1feb4..9f49ae91de 100644 --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c @@ -19,11 +19,13 @@ #include <Library/UefiBootServicesTableLib.h> #include <Library/UefiLib.h> #include <Library/UefiRuntimeLib.h> +#include <Library/BaseMemoryLib.h> #include <Protocol/I2cMaster.h> #define SLAVE_ADDRESS (FixedPcdGet8 (PcdI2cSlaveAddress)) #define PCF8563_DATA_REG_OFFSET 0x2 +#define PCF8563_CLOCK_INVALID 0x80 #define PCF8563_SECONDS_MASK 0x7f #define PCF8563_MINUTES_MASK 0x7f #define PCF8563_HOURS_MASK 0x3f @@ -95,6 +97,7 @@ LibGetTime ( RTC_DATETIME DateTime; EFI_STATUS Status; UINT8 Reg; + EFI_TIME InitTime; if (Time == NULL) { return EFI_INVALID_PARAMETER; @@ -122,15 +125,25 @@ LibGetTime ( return EFI_DEVICE_ERROR; } - Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); - Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); - Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); - Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); - Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); - Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; - - if (DateTime.Century_months & PCF8563_CENTURY_MASK) { - Time->Year += 100; + if ((DateTime.VL_seconds & PCF8563_CLOCK_INVALID) != 0) { + InitTime.Second = 0; + InitTime.Minute = 0; + InitTime.Hour = 0; + InitTime.Day = 1; + InitTime.Month = 1; + InitTime.Year = EPOCH_BASE; + (VOID)CopyMem (Time, &InitTime, sizeof(EFI_TIME)); + } else { + Time->Second = BcdToDecimal8 (DateTime.VL_seconds & PCF8563_SECONDS_MASK); + Time->Minute = BcdToDecimal8 (DateTime.Minutes & PCF8563_MINUTES_MASK); + Time->Hour = BcdToDecimal8 (DateTime.Hours & PCF8563_HOURS_MASK); + Time->Day = BcdToDecimal8 (DateTime.Days & PCF8563_DAYS_MASK); + Time->Month = BcdToDecimal8 (DateTime.Century_months & PCF8563_MONTHS_MASK); + Time->Year = BcdToDecimal8 (DateTime.Years) + EPOCH_BASE; + + if (DateTime.Century_months & PCF8563_CENTURY_MASK) { + Time->Year += 100; + } } if (Capabilities != NULL) {