diff mbox series

[edk2] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization

Message ID 20180618123155.2141-1-masahisa.kojima@linaro.org
State Superseded
Headers show
Series [edk2] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization | expand

Commit Message

Masahisa Kojima June 18, 2018, 12:31 p.m. UTC
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(-)

-- 
2.14.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel June 18, 2018, 1:03 p.m. UTC | #1
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
Masahisa Kojima June 18, 2018, 2:29 p.m. UTC | #2
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
Ard Biesheuvel June 18, 2018, 3:51 p.m. UTC | #3
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
Masahisa Kojima June 18, 2018, 11:56 p.m. UTC | #4
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
Masahisa Kojima June 19, 2018, 1:09 a.m. UTC | #5
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
Ard Biesheuvel June 19, 2018, 8:02 a.m. UTC | #6
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
Masahisa Kojima June 20, 2018, 1:54 a.m. UTC | #7
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 mbox series

Patch

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) {