* [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit
@ 2018-08-02 20:50 Marcin Wojtas
2018-08-03 7:33 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Marcin Wojtas @ 2018-08-02 20:50 UTC (permalink / raw)
To: edk2-devel
Cc: feng.tian, michael.d.kinney, liming.gao, leif.lindholm,
ard.biesheuvel, nadavh, mw, jsd
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>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit
2018-08-02 20:50 [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit Marcin Wojtas
@ 2018-08-03 7:33 ` Ard Biesheuvel
2018-08-03 9:41 ` Marcin Wojtas
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2018-08-03 7:33 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel@lists.01.org, Tian, Feng, Kinney, Michael D,
Gao, Liming, Leif Lindholm, Nadav Haklai, Jan Dąbroś
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit
2018-08-03 7:33 ` Ard Biesheuvel
@ 2018-08-03 9:41 ` Marcin Wojtas
0 siblings, 0 replies; 3+ messages in thread
From: Marcin Wojtas @ 2018-08-03 9:41 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-01, Tian, Feng, Kinney, Michael D, Gao, Liming,
Leif Lindholm, nadavh, jsd@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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-03 9:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-02 20:50 [PATCH v2 1/1] ArmPkg/GenericWatchdogDxe: Split 64bit register write to 2x32bit Marcin Wojtas
2018-08-03 7:33 ` Ard Biesheuvel
2018-08-03 9:41 ` Marcin Wojtas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox