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

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

[Marcin Wojtas]

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.

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

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

[Ard Biesheuvel]

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?

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

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

[Marcin Wojtas]

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

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

[Ard Biesheuvel]

On 2 August 2018 at 17:12, Marcin Wojtas <mw@semihalf.com> wrote:
> 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?
>

To be fair, the SBSA v5.0 does describe this register as

0x010-0x013  WCV[31:0]   Watchdog compare value. Read/write registers
0x014-0x017  WCV[63:32]  containing the current value in the watchdog
                         compare register.

so I guess it is implied that any implementation should tolerate this
value being written as 2 32-bit quantities.

Leif?


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

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

[Leif Lindholm]

On Thu, Aug 02, 2018 at 08:32:18PM +0200, Ard Biesheuvel wrote:
> On 2 August 2018 at 17:12, Marcin Wojtas <mw@semihalf.com> wrote:
> > 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?
> >
> 
> To be fair, the SBSA v5.0 does describe this register as
> 
> 0x010-0x013  WCV[31:0]   Watchdog compare value. Read/write registers
> 0x014-0x017  WCV[63:32]  containing the current value in the watchdog
>                          compare register.
> 
> so I guess it is implied that any implementation should tolerate this
> value being written as 2 32-bit quantities.
> 
> Leif?

Yep, the way the SBSA describes it, these are two separate 32-bit
registers, making accessing them with a single transaction ends up
being implementation defined behaviour.

We should maybe add separate _HIGH and _LOW #defines for the two rather
than calculating register addresses inline?

But apart from that:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

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