From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Pete Batard <pete@akeo.ie>
Cc: Leif Lindholm <leif.lindholm@linaro.org>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH 1/1] EmbeddedPkg/Library: Add VirtualRealTimeClockLib
Date: Wed, 20 Feb 2019 08:42:51 +0100 [thread overview]
Message-ID: <CAKv+Gu-uYJ6Uu=0fR3cPPVyJdEzNyKEGnL=Pard7b+q9ANLYCw@mail.gmail.com> (raw)
In-Reply-To: <327e7ce8-b300-e239-0d47-13ee925711b0@akeo.ie>
On Fri, 15 Feb 2019 at 15:57, Pete Batard <pete@akeo.ie> wrote:
>
> On 2019-02-15 14:39, Ard Biesheuvel wrote:
> > On Fri, 15 Feb 2019 at 11:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On Tue, 12 Feb 2019 at 19:14, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >>>
> >>> On Mon, Feb 04, 2019 at 12:47:36PM +0000, Pete Batard wrote:
> >>>> This is designed to be used on platforms where a a real RTC is not
> >>>> available and relies on an RtcEpochSeconds variable having been set or,
> >>>> if that is not the case, falls back to using the epoch embedded at
> >>>> compilation time.
> >>>>
> >>>> Note that, in order to keep things simple for the setting of the
> >>>> compilation time variable, only GCC environments with UNIX-like shells
> >>>> and where a 'date' command is available are meant to be supported for
> >>>> now.
> >>>>
> >>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>> Signed-off-by: Pete Batard <pete@akeo.ie>
> >>>
> >>> On the whole, this looks good to me.
> >>> One addition we'll need, so that we can build this library standalone
> >>> is an entry in EmbeddedPkg.dsc:
> >>>
> >>> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
> >>> index 4d9e6399d5..dc5040e611 100644
> >>> --- a/EmbeddedPkg/EmbeddedPkg.dsc
> >>> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
> >>> @@ -218,6 +218,7 @@ [Components.common]
> >>> EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> >>> EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> >>> EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
> >>> + EmbeddedPkg/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf
> >>> EmbeddedPkg/EmbeddedMonotonicCounter/EmbeddedMonotonicCounter.inf
> >>> EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
> >>>
> >>> I don't have any strong opinions on either of Phil's suggestions, but
> >>> if you could give some feedback on those and fold the above in, this
> >>> could go in.
> >>>
> >>
> >> WIth this addition
> >>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> Pushed as 1b261a705f94..64a17fadcb79
> >
> > OK, there is a problem with this code:
> >
> > +EFI_STATUS
> > +EFIAPI
> > +LibGetTime (
> > + OUT EFI_TIME *Time,
> > + OUT EFI_TIME_CAPABILITIES *Capabilities
> > + )
> > +{
> > + EFI_STATUS Status;
> > + UINT32 EpochSeconds;
> >
> > EpochSeconds is declared here, and updated depending on time zone and
> > DST settings. However, the resulting value is never used anywhere.
>
> You're right.
>
> Looks like I forgot to merge all the use of EpochSeconds into
> ElapsedSeconds, from the code I copy/pasted.
>
> I'm very sorry about this, as it's something I should have picked up
> before sending this patch for review.
> > It is not clear to me what the correct fix is, so Pete, could you
> > please look into this?
>
> I'll send a fix for this as soon as I have a chance. Thanks for pointing
> the mistake.
>
Any progress here?
next prev parent reply other threads:[~2019-02-20 7:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-04 12:47 [PATCH 0/1] EmbeddedPkg/Library: Add VirtualRealTimeClockLib Pete Batard
2019-02-04 12:47 ` [PATCH 1/1] " Pete Batard
2019-02-05 19:57 ` Philippe Mathieu-Daudé
2019-02-15 0:09 ` Pete Batard
2019-02-12 18:14 ` Leif Lindholm
2019-02-15 0:12 ` Pete Batard
2019-02-15 9:57 ` Leif Lindholm
2019-02-15 10:07 ` Ard Biesheuvel
2019-02-15 13:39 ` Ard Biesheuvel
2019-02-15 14:57 ` Pete Batard
2019-02-15 15:09 ` Ard Biesheuvel
2019-02-20 7:42 ` Ard Biesheuvel [this message]
2020-05-14 7:45 ` [edk2] " Philippe Mathieu-Daudé
2020-05-14 13:14 ` Pete Batard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKv+Gu-uYJ6Uu=0fR3cPPVyJdEzNyKEGnL=Pard7b+q9ANLYCw@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox