[PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[Joel Stanley]

Three of these are fixes for ast2600 models that I found when testing
master. The forth is a usability improvement that is helpful when
diagnosing why a watchdog is biting.

v3 adds some comments and fixes whitespace, and r-b from Alex. Thanks
for the review Alex.

v2 fixes some review comments from Cédric and adds his r-b.

Joel Stanley (4):
  aspeed/sdmc: Make ast2600 default 1G
  aspeed/scu: Fix W1C behavior
  watchdog/aspeed: Improve watchdog timeout message
  watchdog/aspeed: Fix AST2600 frequency behaviour

 hw/misc/aspeed_scu.c             | 15 +++++++++++----
 hw/misc/aspeed_sdmc.c            |  6 +++---
 hw/watchdog/wdt_aspeed.c         | 24 +++++++++++++++++++-----
 include/hw/watchdog/wdt_aspeed.h |  1 +
 4 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.24.0

[PATCH v3 1/4] aspeed/sdmc: Make ast2600 default 1G

[Joel Stanley]

Most boards have this much.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/misc/aspeed_sdmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index f3a63a2e01db..2df3244b53c8 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -208,10 +208,10 @@ static int ast2600_rambits(AspeedSDMCState *s)
     }
 
     /* use a common default */
-    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 512M",
+    warn_report("Invalid RAM size 0x%" PRIx64 ". Using default 1024M",
                 s->ram_size);
-    s->ram_size = 512 << 20;
-    return ASPEED_SDMC_AST2600_512MB;
+    s->ram_size = 1024 << 20;
+    return ASPEED_SDMC_AST2600_1024MB;
 }
 
 static void aspeed_sdmc_reset(DeviceState *dev)
-- 
2.24.0

[PATCH v3 2/4] aspeed/scu: Fix W1C behavior

[Joel Stanley]

This models the clock write one to clear registers, and fixes up some
incorrect behavior in all of the write to clear registers.

There was also a typo in one of the register definitions.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v3: Beef up the comments
---
 hw/misc/aspeed_scu.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 717509bc5460..5518168e48b4 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -98,7 +98,7 @@
 #define AST2600_CLK_STOP_CTRL     TO_REG(0x80)
 #define AST2600_CLK_STOP_CTRL_CLR TO_REG(0x84)
 #define AST2600_CLK_STOP_CTRL2     TO_REG(0x90)
-#define AST2600_CLK_STOP_CTR2L_CLR TO_REG(0x94)
+#define AST2600_CLK_STOP_CTRL2_CLR TO_REG(0x94)
 #define AST2600_SDRAM_HANDSHAKE   TO_REG(0x100)
 #define AST2600_HPLL_PARAM        TO_REG(0x200)
 #define AST2600_HPLL_EXT          TO_REG(0x204)
@@ -532,11 +532,13 @@ static uint64_t aspeed_ast2600_scu_read(void *opaque, hwaddr offset,
     return s->regs[reg];
 }
 
-static void aspeed_ast2600_scu_write(void *opaque, hwaddr offset, uint64_t data,
+static void aspeed_ast2600_scu_write(void *opaque, hwaddr offset, uint64_t data64,
                                      unsigned size)
 {
     AspeedSCUState *s = ASPEED_SCU(opaque);
     int reg = TO_REG(offset);
+    /* Truncate here so bitwise operations below behave as expected */
+    uint32_t data = data64;
 
     if (reg >= ASPEED_AST2600_SCU_NR_REGS) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -563,15 +565,20 @@ static void aspeed_ast2600_scu_write(void *opaque, hwaddr offset, uint64_t data,
         /* fall through */
     case AST2600_SYS_RST_CTRL:
     case AST2600_SYS_RST_CTRL2:
+    case AST2600_CLK_STOP_CTRL:
+    case AST2600_CLK_STOP_CTRL2:
         /* W1S (Write 1 to set) registers */
         s->regs[reg] |= data;
         return;
     case AST2600_SYS_RST_CTRL_CLR:
     case AST2600_SYS_RST_CTRL2_CLR:
+    case AST2600_CLK_STOP_CTRL_CLR:
+    case AST2600_CLK_STOP_CTRL2_CLR:
     case AST2600_HW_STRAP1_CLR:
     case AST2600_HW_STRAP2_CLR:
-        /* W1C (Write 1 to clear) registers */
-        s->regs[reg] &= ~data;
+        /* W1C (Write 1 to clear) registers are offset by one address from
+         * the data register */
+        s->regs[reg - 1] &= ~data;
         return;
 
     case AST2600_RNG_DATA:
-- 
2.24.0

[PATCH v3 3/4] watchdog/aspeed: Improve watchdog timeout message

[Joel Stanley]

Users benefit from knowing which watchdog timer has expired. The address
of the watchdog's registers unambiguously indicates which has expired,
so log that.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Use HWADDR_PRIx
v3: Fix spacing
---
 hw/watchdog/wdt_aspeed.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 145be6f99ce2..d283d07d6546 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -219,7 +219,8 @@ static void aspeed_wdt_timer_expired(void *dev)
         return;
     }
 
-    qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n");
+    qemu_log_mask(CPU_LOG_RESET, "Watchdog timer %" HWADDR_PRIx " expired.\n",
+                  s->iomem.addr);
     watchdog_perform_action();
     timer_del(s->timer);
 }
-- 
2.24.0

[PATCH v3 4/4] watchdog/aspeed: Fix AST2600 frequency behaviour

[Joel Stanley]

The AST2600 control register sneakily changed the meaning of bit 4
without anyone noticing. It no longer controls the 1MHz vs APB clock
select, and instead always runs at 1MHz.

The AST2500 was always 1MHz too, but it retained bit 4, making it read
only. We can model both using the same fixed 1MHz calculation.

Fixes: 6b2b2a703cad ("hw: wdt_aspeed: Add AST2600 support")
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Fix Fixes line in commit message
---
 hw/watchdog/wdt_aspeed.c         | 21 +++++++++++++++++----
 include/hw/watchdog/wdt_aspeed.h |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index d283d07d6546..122aa8daaadf 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -93,11 +93,11 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr offset, unsigned size)
 
 }
 
-static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
+static void aspeed_wdt_reload(AspeedWDTState *s)
 {
     uint64_t reload;
 
-    if (pclk) {
+    if (!(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK)) {
         reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
                           s->pclk_freq);
     } else {
@@ -109,6 +109,16 @@ static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
     }
 }
 
+static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
+{
+    uint64_t reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
+
+    if (aspeed_wdt_is_enabled(s)) {
+        timer_mod(s->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + reload);
+    }
+}
+
+
 static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
                              unsigned size)
 {
@@ -130,13 +140,13 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
     case WDT_RESTART:
         if ((data & 0xFFFF) == WDT_RESTART_MAGIC) {
             s->regs[WDT_STATUS] = s->regs[WDT_RELOAD_VALUE];
-            aspeed_wdt_reload(s, !(s->regs[WDT_CTRL] & WDT_CTRL_1MHZ_CLK));
+            awc->wdt_reload(s);
         }
         break;
     case WDT_CTRL:
         if (enable && !aspeed_wdt_is_enabled(s)) {
             s->regs[WDT_CTRL] = data;
-            aspeed_wdt_reload(s, !(data & WDT_CTRL_1MHZ_CLK));
+            awc->wdt_reload(s);
         } else if (!enable && aspeed_wdt_is_enabled(s)) {
             s->regs[WDT_CTRL] = data;
             timer_del(s->timer);
@@ -283,6 +293,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass, void *data)
     awc->offset = 0x20;
     awc->ext_pulse_width_mask = 0xff;
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
+    awc->wdt_reload = aspeed_wdt_reload;
 }
 
 static const TypeInfo aspeed_2400_wdt_info = {
@@ -317,6 +328,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass, void *data)
     awc->ext_pulse_width_mask = 0xfffff;
     awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
+    awc->wdt_reload = aspeed_wdt_reload_1mhz;
 }
 
 static const TypeInfo aspeed_2500_wdt_info = {
@@ -336,6 +348,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass, void *data)
     awc->ext_pulse_width_mask = 0xfffff; /* TODO */
     awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
     awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
+    awc->wdt_reload = aspeed_wdt_reload_1mhz;
 }
 
 static const TypeInfo aspeed_2600_wdt_info = {
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index dfedd7662dd1..819c22993a6e 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -47,6 +47,7 @@ typedef struct AspeedWDTClass {
     uint32_t ext_pulse_width_mask;
     uint32_t reset_ctrl_reg;
     void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
+    void (*wdt_reload)(AspeedWDTState *s);
 }  AspeedWDTClass;
 
 #endif /* WDT_ASPEED_H */
-- 
2.24.0

Re: [PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[Peter Maydell]

On Wed, 13 Nov 2019 at 22:13, Joel Stanley <joel@jms.id.au> wrote:
>
> Three of these are fixes for ast2600 models that I found when testing
> master. The forth is a usability improvement that is helpful when
> diagnosing why a watchdog is biting.
>
> v3 adds some comments and fixes whitespace, and r-b from Alex. Thanks
> for the review Alex.
>
> v2 fixes some review comments from Cédric and adds his r-b.
>
> Joel Stanley (4):
>   aspeed/sdmc: Make ast2600 default 1G
>   aspeed/scu: Fix W1C behavior
>   watchdog/aspeed: Improve watchdog timeout message
>   watchdog/aspeed: Fix AST2600 frequency behaviour

Were you wanting any of these (or any of the other aspeed
patches currently on the list) to go into 4.2? None of them
are specifically marked as for-4.2 bugfixes, and I don't
know enough about aspeed to be able to judge their importance.

thanks
-- PMM

Re: [PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[Cédric Le Goater]

On 14/11/2019 15:04, Peter Maydell wrote:
> On Wed, 13 Nov 2019 at 22:13, Joel Stanley <joel@jms.id.au> wrote:
>>
>> Three of these are fixes for ast2600 models that I found when testing
>> master. The forth is a usability improvement that is helpful when
>> diagnosing why a watchdog is biting.
>>
>> v3 adds some comments and fixes whitespace, and r-b from Alex. Thanks
>> for the review Alex.
>>
>> v2 fixes some review comments from Cédric and adds his r-b.
>>
>> Joel Stanley (4):
>>   aspeed/sdmc: Make ast2600 default 1G
>>   aspeed/scu: Fix W1C behavior
>>   watchdog/aspeed: Improve watchdog timeout message
>>   watchdog/aspeed: Fix AST2600 frequency behaviour
> 
> Were you wanting any of these (or any of the other aspeed
> patches currently on the list) to go into 4.2? None of them
> are specifically marked as for-4.2 bugfixes, and I don't
> know enough about aspeed to be able to judge their importance.

I think we are done for 4.2, although these are relatively 
simple fixes that could go in 4.2. Nothing critical. As you
wish.

We are preparing, and reviewing, the initial 5.0 patchset 
which will contain the I2C pool buffer model plus the fixes 
Joel and I sent.

Thanks,
C.

Re: [PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[Peter Maydell]

On Thu, 14 Nov 2019 at 14:17, Cédric Le Goater <clg@kaod.org> wrote:
> We are preparing, and reviewing, the initial 5.0 patchset
> which will contain the I2C pool buffer model plus the fixes
> Joel and I sent.

OK. I currently have in my 'maybe I should do something with this'
list these patchsets:
aspeed/i2c: Add support for pool and DMA transfer modes
arm/aspeed: Watchdog and SDRAM fixes
aspeed: AST2600 SMC fixes and tacoma-bmc machine
aspeed: rework inter model link properties

(I had a quick scan through and there's nothing in any of
them that looked odd to me, though I haven't done a detailed
review). Should I just assume that you'll send out a rollup
patchset with all of those once 5.0 is out ? Alternatively
I could apply them to the target-arm.for-5.0 branch I'm
currently keeping, which will become the first target-arm
pullreq once 5.0 reopens, if you prefer (and if you've
got on-list reviewed-by tags for them all).

thanks
-- PMM

Re: [PATCH v3 0/4] arm/aspeed: Watchdog and SDRAM fixes

[Cédric Le Goater]

On 19/11/2019 14:44, Peter Maydell wrote:
> On Thu, 14 Nov 2019 at 14:17, Cédric Le Goater <clg@kaod.org> wrote:
>> We are preparing, and reviewing, the initial 5.0 patchset
>> which will contain the I2C pool buffer model plus the fixes
>> Joel and I sent.
> 
> OK. I currently have in my 'maybe I should do something with this'
> list these patchsets:
> aspeed/i2c: Add support for pool and DMA transfer modes
> arm/aspeed: Watchdog and SDRAM fixes
> aspeed: AST2600 SMC fixes and tacoma-bmc machine
> aspeed: rework inter model link properties
> 
> (I had a quick scan through and there's nothing in any of
> them that looked odd to me, though I haven't done a detailed
> review). Should I just assume that you'll send out a rollup
> patchset with all of those once 5.0 is out ? Alternatively
> I could apply them to the target-arm.for-5.0 branch I'm
> currently keeping, which will become the first target-arm
> pullreq once 5.0 reopens, if you prefer (and if you've
> got on-list reviewed-by tags for them all).

I have a larger list of patches with reviewed-by : 

Cédric Le Goater - aspeed: Change the "nic" property definition
Cédric Le Goater - aspeed: Change the "scu" property definition
PanNengyuan - gpio: fix memory leak in aspeed_gpio_init()
Cédric Le Goater - aspeed: Add support for the tacoma-bmc board
Cédric Le Goater - aspeed: Remove AspeedBoardConfig array and use AspeedMachineClass
Cédric Le Goater - aspeed/smc: Add AST2600 timings registers
Cédric Le Goater - aspeed/smc: Do not map disabled segment on the AST2600
Cédric Le Goater - aspeed/smc: Restore default AHB window mapping at reset
Joel Stanley - watchdog/aspeed: Fix AST2600 frequency behaviour
Joel Stanley - watchdog/aspeed: Improve watchdog timeout message
Joel Stanley - aspeed/scu: Fix W1C behavior
Joel Stanley - aspeed/sdmc: Make ast2600 default 1G
Cédric Le Goater - aspeed/i2c: Add trace events
Cédric Le Goater - aspeed/i2c: Add support for DMA transfers
Cédric Le Goater - aspeed: Add a DRAM memory region at the SoC level
Cédric Le Goater - aspeed/i2c: Check SRAM enablement on AST2500
Cédric Le Goater - aspeed/i2c: Add support for pool buffer transfers


Tests run fine on travis.

I will send them this afternoon.

Thanks,
C.