* [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
@ 2018-06-18 12:31 masahisa.kojima
2018-06-18 13:03 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: masahisa.kojima @ 2018-06-18 12:31 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
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) {
+ 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-18 12:31 [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization masahisa.kojima
@ 2018-06-18 13:03 ` Ard Biesheuvel
2018-06-18 14:29 ` Masahisa Kojima
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-06-18 13:03 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Yoshitoyo Osaki
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-18 13:03 ` Ard Biesheuvel
@ 2018-06-18 14:29 ` Masahisa Kojima
2018-06-18 15:51 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Masahisa Kojima @ 2018-06-18 14:29 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, Leif Lindholm, Yoshitoyo Osaki
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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-18 14:29 ` Masahisa Kojima
@ 2018-06-18 15:51 ` Ard Biesheuvel
2018-06-18 23:56 ` Masahisa Kojima
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-06-18 15:51 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Yoshitoyo Osaki
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
>> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-18 15:51 ` Ard Biesheuvel
@ 2018-06-18 23:56 ` Masahisa Kojima
2018-06-19 1:09 ` Masahisa Kojima
0 siblings, 1 reply; 8+ messages in thread
From: Masahisa Kojima @ 2018-06-18 23:56 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, Leif Lindholm, Yoshitoyo Osaki
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-18 23:56 ` Masahisa Kojima
@ 2018-06-19 1:09 ` Masahisa Kojima
2018-06-19 8:02 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Masahisa Kojima @ 2018-06-19 1:09 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, Leif Lindholm, Yoshitoyo Osaki
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-19 1:09 ` Masahisa Kojima
@ 2018-06-19 8:02 ` Ard Biesheuvel
2018-06-20 1:54 ` Masahisa Kojima
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-06-19 8:02 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: edk2-devel@lists.01.org, Leif Lindholm, Yoshitoyo Osaki
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization
2018-06-19 8:02 ` Ard Biesheuvel
@ 2018-06-20 1:54 ` Masahisa Kojima
0 siblings, 0 replies; 8+ messages in thread
From: Masahisa Kojima @ 2018-06-20 1:54 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, Leif Lindholm, Yoshitoyo Osaki
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-20 1:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-18 12:31 [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization masahisa.kojima
2018-06-18 13:03 ` Ard Biesheuvel
2018-06-18 14:29 ` Masahisa Kojima
2018-06-18 15:51 ` Ard Biesheuvel
2018-06-18 23:56 ` Masahisa Kojima
2018-06-19 1:09 ` Masahisa Kojima
2018-06-19 8:02 ` Ard Biesheuvel
2018-06-20 1:54 ` Masahisa Kojima
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox