Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization

[Masahisa Kojima <masahisa.kojima@linaro.org>]

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.