| Message ID | 20180618123155.2141-1-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [edk2] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization | expand |
On 18 June 2018 at 14:31, <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 > 2) DEBUG build > > 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. > > 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 set dummy date/time(01/01/2000 00:00:00) to > proceed succeeding SetTime 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. > > 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 | 32 ++++++++++++++++------ > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > index fb58e1feb4..7be0d23eea 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,26 @@ 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) { Wouldn't it be better to return EFI_DEVICE_ERROR in this case? > + InitTime.Second = 0; > + InitTime.Minute = 0; > + InitTime.Hour = 0; > + InitTime.Day = 1; > + InitTime.Month = 1; > + InitTime.Year = EPOCH_BASE; > + (VOID)LibSetTime (&InitTime); > + (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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ard, Thank you for comment. > Wouldn't it be better to return EFI_DEVICE_ERROR in this case? It is first option I come up with to fix this issue. But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime() performs GetTime()->update with user specified time->SetTime(), If GetTime() failes, SetTime() never called and user can not set time. # It really depends on the RTC device, we failed to set time in 70% devices. Another place to perform this dummy time/date setting is inside of LibRtcInitialize(). Current error occurs in setting time, and I prefer to add this process in GetTime(). On Mon, 18 Jun 2018 at 22:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 18 June 2018 at 14:31, <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 > > 2) DEBUG build > > > > 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. > > > > 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 set dummy date/time(01/01/2000 00:00:00) to > > proceed succeeding SetTime 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. > > > > 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 | 32 ++++++++++++++++------ > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > > index fb58e1feb4..7be0d23eea 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,26 @@ 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) { > > > Wouldn't it be better to return EFI_DEVICE_ERROR in this case? > > > + InitTime.Second = 0; > > + InitTime.Minute = 0; > > + InitTime.Hour = 0; > > + InitTime.Day = 1; > > + InitTime.Month = 1; > > + InitTime.Year = EPOCH_BASE; > > + (VOID)LibSetTime (&InitTime); > > + (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 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > Hi Ard, > > Thank you for comment. > >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case? > > It is first option I come up with to fix this issue. > But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime() > performs GetTime()->update with user specified time->SetTime(), > If GetTime() failes, SetTime() never called and user can not set time. > # It really depends on the RTC device, we failed to set time in 70% devices. > > Another place to perform this dummy time/date setting is inside of > LibRtcInitialize(). > Current error occurs in setting time, and I prefer to add this process > in GetTime(). > I think we should fix the shell command instead. Setting the time should be possible even if getting the time file, precisely for situations like this one. > On Mon, 18 Jun 2018 at 22:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> On 18 June 2018 at 14:31, <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 >> > 2) DEBUG build >> > >> > 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. >> > >> > 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 set dummy date/time(01/01/2000 00:00:00) to >> > proceed succeeding SetTime 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. >> > >> > 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 | 32 ++++++++++++++++------ >> > 1 file changed, 23 insertions(+), 9 deletions(-) >> > >> > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c >> > index fb58e1feb4..7be0d23eea 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,26 @@ 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) { >> >> >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case? >> >> > + InitTime.Second = 0; >> > + InitTime.Minute = 0; >> > + InitTime.Hour = 0; >> > + InitTime.Day = 1; >> > + InitTime.Month = 1; >> > + InitTime.Year = EPOCH_BASE; >> > + (VOID)LibSetTime (&InitTime); >> > + (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 >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, 19 Jun 2018 at 00:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > Hi Ard, > > > > Thank you for comment. > > > >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case? > > > > It is first option I come up with to fix this issue. > > But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime() > > performs GetTime()->update with user specified time->SetTime(), > > If GetTime() failes, SetTime() never called and user can not set time. > > # It really depends on the RTC device, we failed to set time in 70% devices. > > > > Another place to perform this dummy time/date setting is inside of > > LibRtcInitialize(). > > Current error occurs in setting time, and I prefer to add this process > > in GetTime(). > > > > I think we should fix the shell command instead. Setting the time > should be possible even if getting the time file, precisely for > situations like this one. OK, I agree with you. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ard,
This issue can be divided into following two issues.
1) Assertion occurs during boot time in DEBUG build, it depends on
the uninitialized RTC device state(~70%)
2) SetTime() fails if GetTime() fails
For issue 1), we should update pcf8563 driver, ignore retrieved data
and return error when the VL bit is 1.
For issue 2), it is better to update shell command as your comment.
But situation is a little complicated.
UEFI shell command can only set DATE or TIME separately.
To set DATE, current procedure is like following.
1) GetTime(retrieve both DATE and TIME)
2) update only DATE
3) Write back to RTC both DATE and TIME
So, current uefi shell can not set DATE/TIME without GetTime().
One possible solution is adding new command to set both DATE and TIME.
But it will change user experience and I'm not sure it is acceptable.
What do you think?
Regards,
Masahisa
On Tue, 19 Jun 2018 at 08:56, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> On Tue, 19 Jun 2018 at 00:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > Hi Ard,
> > >
> > > Thank you for comment.
> > >
> > >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case?
> > >
> > > It is first option I come up with to fix this issue.
> > > But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime()
> > > performs GetTime()->update with user specified time->SetTime(),
> > > If GetTime() failes, SetTime() never called and user can not set time.
> > > # It really depends on the RTC device, we failed to set time in 70% devices.
> > >
> > > Another place to perform this dummy time/date setting is inside of
> > > LibRtcInitialize().
> > > Current error occurs in setting time, and I prefer to add this process
> > > in GetTime().
> > >
> >
> > I think we should fix the shell command instead. Setting the time
> > should be possible even if getting the time file, precisely for
> > situations like this one.
>
> OK, I agree with you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 19 June 2018 at 03:09, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > Hi Ard, > > This issue can be divided into following two issues. > > 1) Assertion occurs during boot time in DEBUG build, it depends on > the uninitialized RTC device state(~70%) > 2) SetTime() fails if GetTime() fails > > For issue 1), we should update pcf8563 driver, ignore retrieved data > and return error when the VL bit is 1. > Well, if returning an error is preventing us from using the shell command, I'd rather work around it. > For issue 2), it is better to update shell command as your comment. > But situation is a little complicated. > UEFI shell command can only set DATE or TIME separately. > To set DATE, current procedure is like following. > 1) GetTime(retrieve both DATE and TIME) > 2) update only DATE > 3) Write back to RTC both DATE and TIME > So, current uefi shell can not set DATE/TIME without GetTime(). > One possible solution is adding new command to set both DATE and TIME. > But it will change user experience and I'm not sure it is acceptable. > What do you think? > Yes, that makes it a bit complicated. So could we simply return EPOCH_BASE/1/1 0:0:0 in case the VL bit is set? That allows the time/date commands to work right? Then, it is up to the user to set the correct time and data, and the VL bit will remain set until they do so. > On Tue, 19 Jun 2018 at 08:56, Masahisa Kojima > <masahisa.kojima@linaro.org> wrote: >> >> On Tue, 19 Jun 2018 at 00:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > >> > On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: >> > > Hi Ard, >> > > >> > > Thank you for comment. >> > > >> > >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case? >> > > >> > > It is first option I come up with to fix this issue. >> > > But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime() >> > > performs GetTime()->update with user specified time->SetTime(), >> > > If GetTime() failes, SetTime() never called and user can not set time. >> > > # It really depends on the RTC device, we failed to set time in 70% devices. >> > > >> > > Another place to perform this dummy time/date setting is inside of >> > > LibRtcInitialize(). >> > > Current error occurs in setting time, and I prefer to add this process >> > > in GetTime(). >> > > >> > >> > I think we should fix the shell command instead. Setting the time >> > should be possible even if getting the time file, precisely for >> > situations like this one. >> >> OK, I agree with you. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Ard, > So could we simply return EPOCH_BASE/1/1 0:0:0 in case the VL bit is > set? That allows the time/date commands to work right? Then, it is up > to the user to set the correct time and data, and the VL bit will > remain set until they do so. Yes, I can boot and set time/date correctly. I will submit v2 patch. Regards, Masahisa On Tue, 19 Jun 2018 at 17:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On 19 June 2018 at 03:09, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > > Hi Ard, > > > > This issue can be divided into following two issues. > > > > 1) Assertion occurs during boot time in DEBUG build, it depends on > > the uninitialized RTC device state(~70%) > > 2) SetTime() fails if GetTime() fails > > > > For issue 1), we should update pcf8563 driver, ignore retrieved data > > and return error when the VL bit is 1. > > > > Well, if returning an error is preventing us from using the shell > command, I'd rather work around it. > > > > For issue 2), it is better to update shell command as your comment. > > But situation is a little complicated. > > UEFI shell command can only set DATE or TIME separately. > > To set DATE, current procedure is like following. > > 1) GetTime(retrieve both DATE and TIME) > > 2) update only DATE > > 3) Write back to RTC both DATE and TIME > > So, current uefi shell can not set DATE/TIME without GetTime(). > > One possible solution is adding new command to set both DATE and TIME. > > But it will change user experience and I'm not sure it is acceptable. > > What do you think? > > > > Yes, that makes it a bit complicated. > > So could we simply return EPOCH_BASE/1/1 0:0:0 in case the VL bit is > set? That allows the time/date commands to work right? Then, it is up > to the user to set the correct time and data, and the VL bit will > remain set until they do so. > > > > > On Tue, 19 Jun 2018 at 08:56, Masahisa Kojima > > <masahisa.kojima@linaro.org> wrote: > >> > >> On Tue, 19 Jun 2018 at 00:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> > > >> > On 18 June 2018 at 16:29, Masahisa Kojima <masahisa.kojima@linaro.org> wrote: > >> > > Hi Ard, > >> > > > >> > > Thank you for comment. > >> > > > >> > >> Wouldn't it be better to return EFI_DEVICE_ERROR in this case? > >> > > > >> > > It is first option I come up with to fix this issue. > >> > > But edk2/ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c::CheckAndSetTime() > >> > > performs GetTime()->update with user specified time->SetTime(), > >> > > If GetTime() failes, SetTime() never called and user can not set time. > >> > > # It really depends on the RTC device, we failed to set time in 70% devices. > >> > > > >> > > Another place to perform this dummy time/date setting is inside of > >> > > LibRtcInitialize(). > >> > > Current error occurs in setting time, and I prefer to add this process > >> > > in GetTime(). > >> > > > >> > > >> > I think we should fix the shell command instead. Setting the time > >> > should be possible even if getting the time file, precisely for > >> > situations like this one. > >> > >> OK, I agree with you. _______________________________________________ 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..7be0d23eea 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,26 @@ 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)LibSetTime (&InitTime); + (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) {