| Message ID | 20210907065822.1152443-2-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed/smc: Cleanups and QOMification | expand |
> 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
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.
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) {
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(+)