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