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

["Cédric Le Goater" <clg@kaod.org>]

On 10/20/21 06:57, Peter Delevoryas wrote:
> 
> 
>> 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?

Yes. I think WDT2 was activated in uboot on these platforms.

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

Indeed. The datasheet also says :

   Watchdog Timer (WDT) includes 4 sets of 32-bit decrement
   counters,

which might have induced us in error :) I will include a fix
for it.

> but it doesn’t seem to say explicitly that the 2nd
> watchdog is the alternate boot watchdog, 

True. That's my assumption for the model and we could also
instantiate a new watchdog in the SMC/FMC model.

> it’s probably
> just implied that the user read the AST2400/AST2500 docs right? 

I think Aspeed is cleaning up the WDT logic by moving "exotic"
features to other controllers. that would be why some registers
of WDT1 and WDT2 are exposed in the FMC register space for 4B
detection and alternate boot :

   FMC60: FMC WDT1 Control/Status Register for Address Mode Detection
   FMC64: FMC WDT2 Control/Status Register for Alternate Boot
   FMC68: FMC WDT2 Timer Reload Value Register
   FMC6C: FMC WDT2 Timer Restart Register

and the FMC also has a new signal/pin : GPIOY6/FWSPIABR to handle ABR.
That's the most important change.


> And the FMC registers are just an alias to write
> to these watchdog 2 registers? 

If this is the same watchdog mapped into the FMC, I would say yes
and the logic generate load/stores transactions on the AHB bus.
Adding an address space for the WDT registers in the model is the
closer we can get without implementing the bus protocol.

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

Hey, this is something to try on HW and check how both register
sets evolve. Would you have time ?

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

We should ask the HW designers.

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

No. This is not modeled.

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

It is certainly feasible but it would require some thinking on
how the models interact with one another.

If a FMC_WDT2 watchdog model is owned by the SMC model, it would
be simpler. That's seem to be going in the direction of your
questions :)

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

Good. That's the goal.

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

It's worth checking with the HW designers before pushing anything.

Thanks,

C.


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