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

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

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

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

Andrew did some experiments in the past and the two register sets
were evolving independently.

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

Aspeed tells me its an independent logic. So, I will drop the
model from this patchset.

Thanks,

C.

> On Oct 21, 2021, at 11:11 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
>>> 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 ?
> 
> Andrew did some experiments in the past and the two register sets
> were evolving independently.

I see, yeah I looked at some hardware today and haven’t finished the experiments,
but it did seem that way. Also asked some more knowledgeable people
on my team and they confirmed it was necessary to use FMC_WDT2.

> 
>>> 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.
> 
> Aspeed tells me its an independent logic. So, I will drop the
> model from this patchset.
> 

I see! Ok, thanks for investigating that!

> Thanks,
> 
> C.
>