From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 364DF210E38C6 for ; Mon, 18 Jun 2018 08:51:41 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id a3-v6so12698096itd.0 for ; Mon, 18 Jun 2018 08:51:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Pcip2HO5YV29c+f106XlHxqOAepYkVZQrzIYX28un20=; b=F7UiwDAZYafpF5oWrWhNPS6p9eZizA+jDp5JaHxH+D32c6obP5QeN7mDLmhnstJjnY UzwlqLaR/ErUtYpWgcP9XlwSYEO4jrC4CUe2HiU8C72ITyeZ62dHwFZUwBzunUbWNBtp QhFAR9YjrebWnvYzzQsg2CXeKNBGuKqiDAbZk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Pcip2HO5YV29c+f106XlHxqOAepYkVZQrzIYX28un20=; b=U5PNa31oHMfAiGEZGXxln6be7BnrO4dMKuMAItRo84A+Fjwu4qapF9lp9vOde+MNlP 8GIm9p0h3+WCHcv25NqVe2G4A1Ta1z2qAIFc75eyyZQVMAKUpWMhPlnsswBE99w31V/b 1qc6PkQPeL/8y+SmykOnGYHvSI91tEMtUuS7bY4pZ2OxgxHRJFo95b518Tbn1hBl3l4B BB/FEQwljulf7HgBmVbubQWFWUsdOypW7I5u1CK1JMaHusnELMst3zqPYiJOaQsKk4NY 2Ryk+ZqYRM9mmj2fzX4Fd8+cgnYDtXTx0t9CNuGKGi9IhmaxO1JDwmbBhbBtMIR+SCyZ QgXg== X-Gm-Message-State: APt69E2luCWeiODZhMdGjvJrX3fRGeM5nWoDfHXO9I+C8la01SDMh8uD yZ0mM4BfqI76bLxI1lkgylZiKwKyx5lItvdcwO+Fkw== X-Google-Smtp-Source: ADUXVKL3OaGi7PAo7nkq4Oz9IGPjokYcr4SUurS7jGJgsXNhzHmz/Sv6wzJwwP28HVE/6gyNiJu8/FevbmwoG8e2xr0= X-Received: by 2002:a24:1d0e:: with SMTP id 14-v6mr9853053itj.50.1529337100480; Mon, 18 Jun 2018 08:51:40 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 18 Jun 2018 08:51:40 -0700 (PDT) In-Reply-To: References: <20180618123155.2141-1-masahisa.kojima@linaro.org> From: Ard Biesheuvel Date: Mon, 18 Jun 2018 17:51:40 +0200 Message-ID: To: Masahisa Kojima Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Yoshitoyo Osaki Subject: Re: [PATCH] Silicon/NXP/Pcf8563RealTimeClockLib: add rtc device initialization X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jun 2018 15:51:41 -0000 Content-Type: text/plain; charset="UTF-8" On 18 June 2018 at 16:29, Masahisa Kojima 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 wrote: >> >> On 18 June 2018 at 14:31, wrote: >> > From: Masahisa Kojima >> > >> > 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 >> > Signed-off-by: Yoshitoyo Osaki >> > --- >> > .../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 >> > #include >> > #include >> > +#include >> > #include >> > >> > #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 >> >