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:c06::243; helo=mail-oi0-x243.google.com; envelope-from=masahisa.kojima@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-oi0-x243.google.com (mail-oi0-x243.google.com [IPv6:2607:f8b0:4003:c06::243]) (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 809EB21199B1A for ; Tue, 19 Jun 2018 23:30:21 -0700 (PDT) Received: by mail-oi0-x243.google.com with SMTP id 188-v6so1997536oid.12 for ; Tue, 19 Jun 2018 23:30:21 -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=AkSPaC1VJeYwjh/9ecxGdq/iv3DVLuBaXrd1NWEthFw=; b=haQkDr0tLhoxjV0BNrFNVDYk2mzV9AyDZqIlMHF+yXMgOfKqJCwcsdUcx/b4Jifx+H evIdd3M4jp6FZv5ZeXZLKXgRQv7pLBq0NHazWmZ2tviPL8wTuyuJIvG8CstAAbDFCalz 7jKFSGkFtHKsRai4OWrgC7W4HBd0zsmkOHSy8= 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=AkSPaC1VJeYwjh/9ecxGdq/iv3DVLuBaXrd1NWEthFw=; b=ncT9Wg7z4pN292bb0UI3B0muQyKPNS66MCdQuQNllRM0D89cbBSzGHklVssgSNC7kd rNjZjG5V6Z4ncFYnozYK/ZejDVPoGAMSH8/OpL/szSPwyj9sOZ1Uc8Z/u/hFmXTeuErU wdPH50cfsdE3mOxfH4wZNXXJUC/a0IgNSyMOwN3xkR5dcwoW8x1YOvYQ9jLNTm8pFus1 +isvfOB9kBkfTnvUyQ2apfHtRw221QtJkf9ldQH20kvUt2/9BfjvSYPBf9nRb9yjqIqH +bCz5WUkzFnqluEXKI4P9ZiS6FGqdWtwhMXCCF6Ve3d/P3pJlz8tk7Ycgllik2nsfdFN AN0g== X-Gm-Message-State: APt69E04M2vzIaiXua3zM/0StoYvIkaVOVW5+9ZG1GsbKS07Qpz1sDOU RO93wpBiSfrr6ZCqKffc+zgvI2XMQ7fjitImzi7YyQ== X-Google-Smtp-Source: ADUXVKLCbiP0XNDLTEyh6R2TUluNYMpyq4wIJfApkBmZRyiz7NJy3XYmIkIgsBXsbYGJsV+H72zBC2PLtE2bTYG5j/c= X-Received: by 2002:aca:aa54:: with SMTP id t81-v6mr11479221oie.30.1529476220427; Tue, 19 Jun 2018 23:30:20 -0700 (PDT) MIME-Version: 1.0 References: <20180620030240.2409-1-masahisa.kojima@linaro.org> In-Reply-To: From: Masahisa Kojima Date: Wed, 20 Jun 2018 15:30:09 +0900 Message-ID: To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Leif Lindholm , Yoshitoyo Osaki Subject: Re: [PATCH v2] Silicon/NXP/Pcf8563RealTimeClockLib: add Voltage-low handling 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: Wed, 20 Jun 2018 06:30:21 -0000 Content-Type: text/plain; charset="UTF-8" Hi Ard, Yes, you are correct. I will send update patch. Thank you. On Wed, 20 Jun 2018 at 15:21, Ard Biesheuvel wrote: > > On 20 June 2018 at 05:02, 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 with VL bit(1) > > 2) DEBUG build > > > > UEFI boot fails with assertion when it satisfies above conditions. > > > > Aside form boot failure, 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. > > With that, simply returning EFI_DEVICE_ERROR in LibGetTime() > > is not applicable to VL bit handling. > > > > 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 return dummy date/time(01/01/2000 00:00:00) to > > proceed succeeding 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. > > > > Note that if VL bit is 1, GetTime() returns always > > 01/01/2000 00:00:00 until user sets date/time. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Masahisa Kojima > > Signed-off-by: Yoshitoyo Osaki > > --- > > .../Pcf8563RealTimeClockLib.c | 31 +++++++++++++++------- > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c > > index fb58e1feb4..9f49ae91de 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,25 @@ 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)CopyMem (Time, &InitTime, sizeof(EFI_TIME)); > > I don't think we need InitTime or this CopyMem(). instead, we can just > set the Time-> fields directly, no? > > > + } 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 > >