Re: [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
On 9/8/21 8:46 PM, Peter Delevoryas wrote:
>
>
>> On Sep 6, 2021, at 11:58 PM, Cédric Le Goater 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
>> Signed-off-by: Cédric Le Goater
>> ---
>> 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_MODEBIT(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
>
> 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 = 0x1000,
> -.features = ASPEED_SMC_FEATURE_DMA,
> +.features = ASPEED_SMC_FEATURE_DMA |
> ASPEED_SMC_FEATURE_WDT_CONTROL,
> .dma_flash_mask= 0x0FFC,
> .dma_dram_mask = 0x3FFC,
> .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 0xFFF.
>
> Error: unable to disable the 2nd watchdog: FMC_WDT2=0x
>
> And then with these changes added, the writes and reads work
> so that BIT(0) appears
Re: [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
> On Sep 6, 2021, at 11:58 PM, Cédric Le Goater 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
> Signed-off-by: Cédric Le Goater
> ---
> 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_MODEBIT(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
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 = 0x1000,
-.features = ASPEED_SMC_FEATURE_DMA,
+.features = ASPEED_SMC_FEATURE_DMA |
ASPEED_SMC_FEATURE_WDT_CONTROL,
.dma_flash_mask= 0x0FFC,
.dma_dram_mask = 0x3FFC,
.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 0xFFF.
Error: unable to disable the 2nd watchdog: FMC_WDT2=0x
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
0x
root@bmc-oob:~# devmem 0x1e620064 32 0x
root@bmc-oob:~# devmem 0x1e620064
0x000B
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(...)
[PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
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
Signed-off-by: Cédric Le Goater
---
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_MODEBIT(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
