Re: [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers

[Peter Delevoryas]

> On Sep 6, 2021, at 11:58 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> 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 sucessfully within a certain amount of time. This
> function is called alternate boot (ABR) in the FMC controllers.
> 
> On AST2400/AST2500, ABR is enabled by hardware strapping in SCU70 to
> enable the 2nd watchdog timer, on AST2600, through register SCU510.
> If the boot on the the main flash succeeds, the firmware should
> disable the 2nd watchdog timer. If not, the BMC is reset and the CE0
> and CE1 mappings are swapped to restart the BMC from the 2nd flash.
> 
> On the AST2600, the registers controlling the 2nd watchdog timer were
> moved from the watchdog register to the FMC controller. Add simple
> read/write handlers for these to let the FW disable the 2nd watchdog
> without error.
> 
> Reported-by: Peter Delevoryas <pdel@fb.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ssi/aspeed_smc.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 331a2c544635..c9990f069ea4 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -124,6 +124,13 @@
> /* SPI dummy cycle data */
> #define R_DUMMY_DATA      (0x54 / 4)
> 
> +/* FMC_WDT2 Control/Status Register for Alternate Boot (AST2600) */
> +#define R_FMC_WDT2_CTRL   (0x64 / 4)
> +#define   FMC_WDT2_CTRL_ALT_BOOT_MODE    BIT(6) /* O: 2 chips 1: 1 chip */
> +#define   FMC_WDT2_CTRL_SINGLE_BOOT_MODE BIT(5)
> +#define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary 1: alternate */
> +#define   FMC_WDT2_CTRL_EN               BIT(0)
> +
> /* DMA Control/Status Register */
> #define R_DMA_CTRL        (0x80 / 4)
> #define   DMA_CTRL_REQUEST      (1 << 31)
> @@ -263,12 +270,18 @@ static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, 
> uint32_t value);
> 
> #define ASPEED_SMC_FEATURE_DMA       0x1
> #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> +#define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> 
> static inline bool aspeed_smc_has_dma(const AspeedSMCState *s)
> {
>     return !!(s->ctrl->features & ASPEED_SMC_FEATURE_DMA);
> }
> 
> +static inline bool aspeed_smc_has_wdt_control(const AspeedSMCState *s)
> +{
> +    return !!(s->ctrl->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
> +}
> +
> static const AspeedSMCController controllers[] = {
>     {
>         .name              = "aspeed.smc-ast2400",
> @@ -1019,6 +1032,7 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr 
> addr, unsigned int size)
>         addr == R_CE_CMD_CTRL ||
>         addr == R_INTR_CTRL ||
>         addr == R_DUMMY_DATA ||
> +        (aspeed_smc_has_wdt_control(s) && addr == R_FMC_WDT2_CTRL) ||
>         (aspeed_smc_has_dma(s) && addr == R_DMA_CTRL) ||
>         (aspeed_smc_has_dma(s) && addr == R_DMA_FLASH_ADDR) ||
>         (aspeed_smc_has_dma(s) && addr == R_DMA_DRAM_ADDR) ||
> @@ -1350,6 +1364,8 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, 
> uint64_t data,
>         s->regs[addr] = value & 0xff;
>     } else if (addr == R_DUMMY_DATA) {
>         s->regs[addr] = value & 0xff;
> +    } else if (aspeed_smc_has_wdt_control(s) && addr == R_FMC_WDT2_CTRL) {
> +        s->regs[addr] = value & 0xb;
>     } else if (addr == R_INTR_CTRL) {
>         s->regs[addr] = value;
>     } else if (aspeed_smc_has_dma(s) && addr == R_DMA_CTRL) {
> -- 
> 2.31.1
> 

Looks good to me!

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

One thing: should we enable this feature (ASPEED_SMC_FEATURE_WDT_CONTROL)
on any of the SMC controller models? I wanted to test this with the
Fuji image and machine type I added, and I needed the following diff
to enable this:

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 331a2c5446..b5d835d488 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -388,7 +388,7 @@ static const AspeedSMCController controllers[] = {
         .segments          = aspeed_segments_ast2600_fmc,
         .flash_window_base = ASPEED26_SOC_FMC_FLASH_BASE,
         .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA,
+        .features          = ASPEED_SMC_FEATURE_DMA | 
ASPEED_SMC_FEATURE_WDT_CONTROL,
         .dma_flash_mask    = 0x0FFFFFFC,
         .dma_dram_mask     = 0x3FFFFFFC,
         .nregs             = ASPEED_SMC_R_MAX,

Without this, Fuji would try to clear BIT(0) of R_FMC_WDT2_CTRL,
but then the default aspeed_smc_read() value would return 0xFFFFFFF.

Error: unable to disable the 2nd watchdog: FMC_WDT2=0xFFFFFFFF

And then with these changes added, the writes and reads work
so that BIT(0) appears to have been cleared:

Disabled the 2nd watchdog (FMC_WDT2) successfully.

root@bmc-oob:~# devmem 0x1e620064
0x00000000
root@bmc-oob:~# devmem 0x1e620064 32 0xffffffff
root@bmc-oob:~# devmem 0x1e620064
0x0000000B

I don’t totally understand why the mask for the register is 0xb though?

>>> bin(0xb)
‘0b1011'

This doesn’t seem to match the macro BIT(...) #define’s included.
Those #define’s are correct (I cross-referenced with the datasheet
to double check), wouldn’t it be something like this? (zero’s for
the reserved bits?)

>>> hex(0b1111111101110001)
'0xff71'

Thanks,
Peter