Re: [PATCH v2 0/5] aspeed/smc: Improve support for the alternate boot function

[Peter Delevoryas]

> On Oct 18, 2021, at 6:26 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello,
> 
> The Aspeed SoCs have a dual boot function for firmware fail-over
> recovery. The system auto-reboots from the second flash if the main
> flash does not boot successfully within a certain amount of time. This
> function is called alternate boot (ABR) in the FMC controllers.
> 
> On the AST2600, the ABR registers controlling the 2nd watchdog timer
> were moved from the watchdog register to the FMC controller. To
> control WDT2 through the FMC model register set, this series creates a
> local address space on top of WDT2 memory region.
> 
> To test on the fuji-bmc machine, run :
> 
>    devmem 0x1e620064
>    devmem 0x1e78504C 
> 
>    devmem 0x1e620064 32 0xffffffff
>    devmem 0x1e620064
>    devmem 0x1e78504C

This looks good to me! I looked at the whole
patch series, I think all the changes look right.

By the way, just to make sure I’m understanding correctly:

The AST2400 datasheet shows only 2 watchdog timers, and
the first to be used as the primary system deadlock
reset (but still reboot from the primary flash), and the
second watchdog is designated as an alternate boot
watchdog, which reboots from secondary flash and is
only enabled if there’s a specific hw strap pin enabled,
and the second watchdog is usually disabled once booting
is successful, right?

The AST2600 datasheet shows there’s 8 watchdogs (but
we only have 4 declared in QEMU? I see only the first
four support external reset signals, maybe that’s why?)
but it doesn’t seem to say explicitly that the 2nd
watchdog is the alternate boot watchdog, it’s probably
just implied that the user read the AST2400/AST2500 docs
right? And the FMC registers are just an alias to write
to these watchdog 2 registers? Just curious, is it
strictly necessary to use the FMC registers to disable
the alternate boot watchdog, or could you just use the
old address, 0x1e78504C? In our OpenBMC initialization
for Fuji, we’re using the FMC registers, but would
it still work if we used the old addresses? Just curious,
the more I think about it, it seems odd to me that these
FMC watchdog registers exist if they’re just an alias.

Also, I was wondering: does the alternate boot
watchdog actually switch the flash device or flash
region that we boot from, or does it just reboot from
the primary partition? I don’t see anything in
watchdog_perform_action() that obviously indicates we’re
actually switching to a secondary flash, so I was curious
about that.

Thanks for adding this though! This is very useful, we’re
using QEMU more and more for testing, especially the
boot process, so more accurate emulation of this functionality
is great.

Thanks,
Peter

Reviewed-by: Peter Delevoryas <pdel@fb.com>

> 
> Thanks
> 
> C.
> 
> Changes since v2:
> 
> - introduce a container region for the WDT2 register address space
> - introduce a container region for the flash mmio address space
> 
> Cédric Le Goater (5):
>  aspeed/wdt: Introduce a container for the MMIO region
>  aspeed: Initialize the watchdog device models before the FMC models
>  aspeed/smc: Improve support for the alternate boot function
>  aspeed/smc: Use a container for the flash mmio address space
>  speed/sdhci: Add trace events
> 
> include/hw/ssi/aspeed_smc.h      |  5 +-
> include/hw/watchdog/wdt_aspeed.h |  1 +
> hw/arm/aspeed_ast2600.c          | 38 +++++++-------
> hw/arm/aspeed_soc.c              | 36 ++++++-------
> hw/sd/aspeed_sdhci.c             |  5 ++
> hw/ssi/aspeed_smc.c              | 89 +++++++++++++++++++++++++++++---
> hw/watchdog/wdt_aspeed.c         |  6 ++-
> hw/sd/trace-events               |  4 ++
> hw/ssi/trace-events              |  1 +
> 9 files changed, 141 insertions(+), 44 deletions(-)
> 
> -- 
> 2.31.1
> 
> 
>