* [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling
@ 2018-06-20 3:02 masahisa.kojima
2018-06-20 6:21 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: masahisa.kojima @ 2018-06-20 3:02 UTC (permalink / raw)
To: edk2-devel
Cc: ard.biesheuvel, leif.lindholm, Masahisa Kojima, Yoshitoyo Osaki
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));
+ } 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling
2018-06-20 3:02 [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling masahisa.kojima
@ 2018-06-20 6:21 ` Ard Biesheuvel
2018-06-20 6:30 ` Masahisa Kojima
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2018-06-20 6:21 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Yoshitoyo Osaki
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling
2018-06-20 6:21 ` Ard Biesheuvel
@ 2018-06-20 6:30 ` Masahisa Kojima
0 siblings, 0 replies; 3+ messages in thread
From: Masahisa Kojima @ 2018-06-20 6:30 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, Leif Lindholm, Yoshitoyo Osaki
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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-20 6:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-20 3:02 [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling masahisa.kojima
2018-06-20 6:21 ` Ard Biesheuvel
2018-06-20 6:30 ` Masahisa Kojima
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox