Re: [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 2 August 2018 at 22:50, Marcin Wojtas <mw@semihalf.com> wrote:
> According to the SBSA specification the Watchdog Compare
> Register is split into two separate 32bit registers.
> EDK2 code uses a single 64bit transaction to update
> them, which can be problematic, depending on the SoC
> implementation and could result in an unpredicted behavior.
>
> Fix this by modifying WatchdogWriteCompareRegister routine to
> use two consecutive 32bit writes to the Watchdog Compare Register
> Low and High, using new dedicated macros.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Pushed as dd4cae4d82c7

Thanks

> ---
> Changelog v1 -> v2:
> - use separate macros for WCV register low and high
> - improve commit message
> - add Leif's RB
>
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h    | 3 ++-
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
> index 9e2aebc..4f42c56 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdog.h
> @@ -20,7 +20,8 @@
>  // Control Frame:
>  #define GENERIC_WDOG_CONTROL_STATUS_REG       ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x000)
>  #define GENERIC_WDOG_OFFSET_REG               ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x008)
> -#define GENERIC_WDOG_COMPARE_VALUE_REG        ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x010)
> +#define GENERIC_WDOG_COMPARE_VALUE_REG_LOW    ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x010)
> +#define GENERIC_WDOG_COMPARE_VALUE_REG_HIGH   ((UINTN)FixedPcdGet64 (PcdGenericWatchdogControlBase) + 0x014)
>
>  // Values of bit 0 of the Control/Status Register
>  #define GENERIC_WDOG_ENABLED          1
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 3180f01..8ccf153 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -56,7 +56,8 @@ WatchdogWriteCompareRegister (
>    UINT64  Value
>    )
>  {
> -  MmioWrite64 (GENERIC_WDOG_COMPARE_VALUE_REG, Value);
> +  MmioWrite32 (GENERIC_WDOG_COMPARE_VALUE_REG_LOW, Value & MAX_UINT32);
> +  MmioWrite32 (GENERIC_WDOG_COMPARE_VALUE_REG_HIGH, (Value >> 32) & MAX_UINT32);
>  }
>
>  VOID
> --
> 2.7.4
>