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

[Marcin Wojtas <mw@semihalf.com>]

pt., 3 sie 2018 o 09:33 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> 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 a lot!
Marcin

>
> > ---
> > 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
> >