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

[Marcin Wojtas <mw@semihalf.com>]

czw., 2 sie 2018 o 16:59 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On 2 August 2018 at 16:49, Marcin Wojtas <mw@semihalf.com> wrote:
> > EDK2 code uses a single 64bit write to update SBSA watchdog
> > compare registers, however an access to mmio registers should
> > be 32bit for some SoCs. Current usage of MmioWrite64 resulted
> > in an unpredicted behavior. Fix this by modifying
> > WatchdogWriteCompareRegister routine to use two consecutive
> > 32bit writes to Watchdog Compare Register Low and High.
> >
>
> You describe it as if this is generally the case, but this is just a
> silicon bug, right?

Not sure if it's a bug, or simply SoC characterisctics to place SoC
registers to allow only mmio32 access to 32-bit registers. In any way,
even updated routine should be fine also for the ones capable of
mmio64 registers access. Do you have strong objections to the change?

Marcin

>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> > index 3180f01..b25d210 100644
> > --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> > +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> > @@ -56,7 +56,11 @@ WatchdogWriteCompareRegister (
> >    UINT64  Value
> >    )
> >  {
> > -  MmioWrite64 (GENERIC_WDOG_COMPARE_VALUE_REG, Value);
> > +  MmioWrite32 (GENERIC_WDOG_COMPARE_VALUE_REG, Value & MAX_UINT32);
> > +  MmioWrite32 (
> > +    GENERIC_WDOG_COMPARE_VALUE_REG + sizeof (UINT32),
> > +    (Value >> 32) & MAX_UINT32
> > +    );
> >  }
> >
> >  VOID
> > --
> > 2.7.4
> >