From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4003:c0f::244; helo=mail-ot0-x244.google.com; envelope-from=masahisa.kojima@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-ot0-x244.google.com (mail-ot0-x244.google.com [IPv6:2607:f8b0:4003:c0f::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 5F2D7210E12A3 for ; Mon, 18 Jun 2018 07:28:18 -0700 (PDT) Received: by mail-ot0-x244.google.com with SMTP id l15-v6so18691811oth.6 for ; Mon, 18 Jun 2018 07:28:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PSACe/QFz0qq2LXAT6FDpOnrPb33uB47J7OLy9pf1O4=; b=SUzZ7wDeJF1mgQlk53W3DwBnIIsk29jOxGDHmutWAmV7Fvcxg3cw4KoAu/EXpl0+FL H+kVLRkMy0VNvmvJ51sV9MjHULCZr/3wItMQDSCQ6xPqe2hsdkJmYoR3A5tHQKOTxShY 63iyXKmSGXe1Lt/domLxwVgxmWr9r+VwRL8+s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PSACe/QFz0qq2LXAT6FDpOnrPb33uB47J7OLy9pf1O4=; b=G5GmiJscd3zQ6olhOGgslSP26oXzQRfWylqVKZ1PV+bkiU3cK+tNVkdOAPf63kCkx7 xBz0nX6q/X5Ux9CQ2mgugMPpSzu4wN0aWiBzxbrG1Dn4wivV4QbAUSFDII/M1EYV5rF8 WhozXylN+IQNE2I5eu+AXH96J5ewm0YZP71aiCXV7r2J/GMDCMg9W8T+U4sl/uHeOWOL 5MWf7kuHniMldeEI/yyQ1TFQJhIRAuC843zVYZuREmPFdpgfGXSVDZV5+Jlmpj5k8x2a 1cDIsSARQvo62pTaxmw1cKphNrwSVuEvMoIVrMOtJ7pCyqyojm+ofcTkHIIGX/6+PEfH Uzfw== X-Gm-Message-State: APt69E1OvBSyFeadVJaYLkWujmV+t2CXsozvVrvE7wX3+/tWJbD4D/tf KiOsHvF1/xZdyxBaqI+aVPjK0RHOYOv4j97JM1Bkrg== X-Google-Smtp-Source: ADUXVKLvI0pSQuj67i8ks0hrxYLrWddvkSIMLUQlfnj1kj3YpeQ8rm99U1zPyREMbrjuCV6P9+X/mPO2x8A3TQlQCt0= X-Received: by 2002:a9d:4455:: with SMTP id f21-v6mr8534061otj.284.1529332097141; Mon, 18 Jun 2018 07:28:17 -0700 (PDT) MIME-Version: 1.0 References: <20180618123155.2141-1-masahisa.kojima@linaro.org> In-Reply-To: From: Masahisa Kojima Date: Mon, 18 Jun 2018 23:29:15 +0900 Message-ID: To: Ard Biesheuvel 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 14:28:18 -0000 Content-Type: text/plain; charset="UTF-8" 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 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 > >