Re: [edk2-platforms PATCH 6/6] Marvell: RealTimeClockLib: Rework LibGetWakeupTime/LibSetWakeupTime

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 24 May 2021 at 07:29, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Apply multiple fixes to the Marvell RealTimeClockLib wakeup
> library callbacks.
>
> LibGetWakeupTime:
> * Add input parameters validation
> * Fix 'Pending' value check
>
> LibSetWakeupTime:
> * Allow disabling the wakeup timer regardless the input 'Time' value
> * Use more generic 'Time' value verification, which is more strict
>   than the replaced custom one.
> * Use proper alarm mask for 'Pending' signalling
>
> With above the ACS3.0 FWTS and SCT timer tests pass cleanly.
>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.h |  2 +-
>  Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c | 29 ++++++++++----------
>  2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.h b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.h
> index 7fa1d092e4..c33e63d107 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.h
> @@ -17,7 +17,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #define RTC_BRIDGE_TIMING_CTRL0_REG_OFFS        0x80
>  #define RTC_BRIDGE_TIMING_CTRL1_REG_OFFS        0x84
>  #define RTC_IRQ_STATUS_REG                      0x90
> -#define RTC_IRQ_ALARM_MASK                      0x1
> +#define RTC_IRQ_ALARM_MASK                      0x2
>  #define RTC_WRITE_PERIOD_DELAY_MASK             0xFFFF
>  #define RTC_WRITE_PERIOD_DELAY_DEFAULT          0x3FF
>  #define RTC_WRITE_SETUP_DELAY_MASK              (0xFFFF << 16)
> diff --git a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> index a48d44ed83..49c9385d53 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/RealTimeClockLib/RealTimeClockLib.c
> @@ -140,11 +140,15 @@ LibGetWakeupTime (
>  {
>    UINT32 WakeupSeconds;
>
> +  if (Time == NULL || Enabled == NULL || Pending == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    *Enabled = MmioRead32 (mArmadaRtcBase + RTC_IRQ_2_CONFIG_REG) & RTC_IRQ_ALARM_EN;
>
>    *Pending = MmioRead32 (mArmadaRtcBase + RTC_IRQ_STATUS_REG) & RTC_IRQ_ALARM_MASK;
>    // Ack pending alarm
> -  if (Pending) {
> +  if (*Pending) {
>      MmioWrite32 (mArmadaRtcBase + RTC_IRQ_STATUS_REG, RTC_IRQ_ALARM_MASK);
>    }
>
> @@ -176,14 +180,14 @@ LibSetWakeupTime (
>  {
>    UINTN       WakeupSeconds;
>
> -  //
> -  // Because the Armada RTC uses a 32-bit counter for seconds,
> -  // the maximum time span is just over 136 years.
> -  // Time is stored in Unix Epoch format, so it starts in 1970,
> -  // Therefore it can not exceed the year 2106.
> -  //
> -  if ((Time->Year < 1970) || (Time->Year >= 2106)) {
> -    return EFI_UNSUPPORTED;
> +  // Handle timer disabling case
> +  if (!Enabled) {
> +    RtcDelayedWrite (RTC_IRQ_2_CONFIG_REG, 0);
> +    return EFI_SUCCESS;
> +  }
> +
> +  if (Time == NULL || !IsTimeValid (Time)) {
> +    return EFI_INVALID_PARAMETER;
>    }
>
>    // Convert time to raw seconds
> @@ -195,11 +199,8 @@ LibSetWakeupTime (
>    // Issue delayed write to alarm register
>    RtcDelayedWrite (RTC_ALARM_2_REG, (UINT32)WakeupSeconds);
>
> -  if (Enabled) {
> -    MmioWrite32 (mArmadaRtcBase + RTC_IRQ_2_CONFIG_REG, RTC_IRQ_ALARM_EN);
> -  } else {
> -    MmioWrite32 (mArmadaRtcBase + RTC_IRQ_2_CONFIG_REG, 0);
> -  }
> +  // Enable wakeup timer
> +  RtcDelayedWrite (RTC_IRQ_2_CONFIG_REG, RTC_IRQ_ALARM_EN);
>
>    return EFI_SUCCESS;
>  }
> --
> 2.29.0
>