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

[Cédric Le Goater]

On 9/8/21 8:46 PM, Peter Delevoryas wrote:
> 
> 
>> 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:

Ah yes. It fell through the cracks with PATCH 04 "aspeed/smc: Drop 
AspeedSMCController structure" which moves a lot of code around. I will
add it back in the next spin.

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

It is bogus. I changed it to : 

  s->regs[addr] = value & FMC_WDT2_CTRL_EN;

since we only care about the watchdog enablement status bit. So now : 

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


It would be interesting also to implement the WDT2 reset. We can link 
the WDT2 register memory region in the AST2600 FMC model to have the 
controls. We need to boot much faster else WDT2 will fire.

When merged, we can ask Peter for some help on understanding why it 
is so slow compared to the other AST2600 machines.  

The others FMC_WDT2 bits will need more work.

Thanks for the review and tests,

C.