All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] aspeed/smc: Cleanups and QOMification
@ 2021-09-07  6:58 Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers Cédric Le Goater
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

Hello,

It looks big but there are no major changes.

This series adds a simple support for the 2nd watchdog registers which
were moved under the AST2600 FMC controller to deactivate the
alternate boot function. Then come cleanups of the AspeedSMC model
mostly removing a useless AspeedSMCController structure (that's the
noisy part) and QOMifying the AspeedSMCFlash structure.

Thanks,

C.

Cédric Le Goater (10):
  aspeed/smc: Add watchdog Control/Status Registers
  aspeed/smc: Introduce aspeed_smc_error() helper
  aspeed/smc: Stop using the model name for the memory regions
  aspeed/smc: Drop AspeedSMCController structure
  aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash
  aspeed/smc: Remove the 'size' attribute from AspeedSMCFlash
  aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs'
  aspeed/smc: QOMify AspeedSMCFlash
  aspeed/smc: Add default reset values
  aspeed/smc: Introduce an addr_width() class handler

 include/hw/ssi/aspeed_smc.h |   81 ++-
 hw/arm/aspeed.c             |   18 +-
 hw/arm/aspeed_ast2600.c     |    4 +-
 hw/arm/aspeed_soc.c         |    4 +-
 hw/ssi/aspeed_smc.c         | 1129 ++++++++++++++++++++---------------
 5 files changed, 701 insertions(+), 535 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-08 18:46   ` Peter Delevoryas
  2021-09-07  6:58 ` [PATCH 02/10] aspeed/smc: Introduce aspeed_smc_error() helper Cédric Le Goater
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Peter Delevoryas, Alistair Francis, qemu-devel, qemu-arm,
	Cédric Le Goater

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



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 02/10] aspeed/smc: Introduce aspeed_smc_error() helper
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 03/10] aspeed/smc: Stop using the model name for the memory regions Cédric Le Goater
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

It unifies the errors reported by the Aspeed SMC model and also
removes some use of ctrl->name which will help us for the next
patches.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 97 +++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 52 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index c9990f069ea4..b4d92d75a694 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -512,6 +512,9 @@ static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
     }
 }
 
+#define aspeed_smc_error(fmt, ...)                                      \
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__, ## __VA_ARGS__)
+
 static bool aspeed_smc_flash_overlap(const AspeedSMCState *s,
                                      const AspeedSegments *new,
                                      int cs)
@@ -528,11 +531,11 @@ static bool aspeed_smc_flash_overlap(const AspeedSMCState *s,
 
         if (new->addr + new->size > seg.addr &&
             new->addr < seg.addr + seg.size) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: new segment CS%d [ 0x%"
-                          HWADDR_PRIx" - 0x%"HWADDR_PRIx" ] overlaps with "
-                          "CS%d [ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
-                          s->ctrl->name, cs, new->addr, new->addr + new->size,
-                          i, seg.addr, seg.addr + seg.size);
+            aspeed_smc_error("new segment CS%d [ 0x%"
+                             HWADDR_PRIx" - 0x%"HWADDR_PRIx" ] overlaps with "
+                             "CS%d [ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
+                             cs, new->addr, new->addr + new->size,
+                             i, seg.addr, seg.addr + seg.size);
             return true;
         }
     }
@@ -567,9 +570,8 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
 
     /* The start address of CS0 is read-only */
     if (cs == 0 && seg.addr != s->ctrl->flash_window_base) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Tried to change CS0 start address to 0x%"
-                      HWADDR_PRIx "\n", s->ctrl->name, seg.addr);
+        aspeed_smc_error("Tried to change CS0 start address to 0x%"
+                         HWADDR_PRIx, seg.addr);
         seg.addr = s->ctrl->flash_window_base;
         new = s->ctrl->segment_to_reg(s, &seg);
     }
@@ -583,9 +585,8 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
         cs == s->ctrl->max_peripherals &&
         seg.addr + seg.size != s->ctrl->segments[cs].addr +
         s->ctrl->segments[cs].size) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: Tried to change CS%d end address to 0x%"
-                      HWADDR_PRIx "\n", s->ctrl->name, cs, seg.addr + seg.size);
+        aspeed_smc_error("Tried to change CS%d end address to 0x%"
+                         HWADDR_PRIx, cs, seg.addr + seg.size);
         seg.size = s->ctrl->segments[cs].addr + s->ctrl->segments[cs].size -
             seg.addr;
         new = s->ctrl->segment_to_reg(s, &seg);
@@ -595,17 +596,17 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
     if (seg.size &&
         (seg.addr + seg.size <= s->ctrl->flash_window_base ||
          seg.addr > s->ctrl->flash_window_base + s->ctrl->flash_window_size)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: new segment for CS%d is invalid : "
-                      "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
-                      s->ctrl->name, cs, seg.addr, seg.addr + seg.size);
+        aspeed_smc_error("new segment for CS%d is invalid : "
+                         "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
+                         cs, seg.addr, seg.addr + seg.size);
         return;
     }
 
     /* Check start address vs. alignment */
     if (seg.size && !QEMU_IS_ALIGNED(seg.addr, seg.size)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: new segment for CS%d is not "
-                      "aligned : [ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
-                      s->ctrl->name, cs, seg.addr, seg.addr + seg.size);
+        aspeed_smc_error("new segment for CS%d is not "
+                         "aligned : [ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
+                         cs, seg.addr, seg.addr + seg.size);
     }
 
     /* And segments should not overlap (in the specs) */
@@ -618,16 +619,15 @@ static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
 static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr addr,
                                               unsigned size)
 {
-    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u"
-                  PRIx64 "\n", __func__, addr, size);
+    aspeed_smc_error("To 0x%" HWADDR_PRIx " of size %u" PRIx64, addr, size);
     return 0;
 }
 
 static void aspeed_smc_flash_default_write(void *opaque, hwaddr addr,
                                            uint64_t data, unsigned size)
 {
-    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
-                  PRIx64 "\n", __func__, addr, size, data);
+    aspeed_smc_error("To 0x%" HWADDR_PRIx " of size %u: 0x%" PRIx64,
+                     addr, size, data);
 }
 
 static const MemoryRegionOps aspeed_smc_flash_default_ops = {
@@ -670,8 +670,8 @@ static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
     }
 
     if (!cmd) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: no command defined for mode %d\n",
-                      __func__, aspeed_smc_flash_mode(fl));
+        aspeed_smc_error("no command defined for mode %d",
+                         aspeed_smc_flash_mode(fl));
     }
 
     return cmd;
@@ -715,11 +715,9 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
 
     s->ctrl->reg_to_segment(s, s->regs[R_SEG_ADDR0 + fl->id], &seg);
     if ((addr % seg.size) != addr) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid address 0x%08x for CS%d segment : "
-                      "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]\n",
-                      s->ctrl->name, addr, fl->id, seg.addr,
-                      seg.addr + seg.size);
+        aspeed_smc_error("invalid address 0x%08x for CS%d segment : "
+                         "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
+                         addr, fl->id, seg.addr, seg.addr + seg.size);
         addr %= seg.size;
     }
 
@@ -795,8 +793,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
         aspeed_smc_flash_unselect(fl);
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
-                      __func__, aspeed_smc_flash_mode(fl));
+        aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
     }
 
     trace_aspeed_smc_flash_read(fl->id, addr, size, ret,
@@ -913,8 +910,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
                                  aspeed_smc_flash_mode(fl));
 
     if (!aspeed_smc_is_writable(fl)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
-                      HWADDR_PRIx "\n", __func__, addr);
+        aspeed_smc_error("flash is not writable at 0x%" HWADDR_PRIx, addr);
         return;
     }
 
@@ -939,8 +935,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
         aspeed_smc_flash_unselect(fl);
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid flash mode %d\n",
-                      __func__, aspeed_smc_flash_mode(fl));
+        aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
     }
 }
 
@@ -1066,7 +1061,7 @@ static uint8_t aspeed_smc_hclk_divisor(uint8_t hclk_mask)
         }
     }
 
-    qemu_log_mask(LOG_GUEST_ERROR, "invalid HCLK mask %x", hclk_mask);
+    aspeed_smc_error("invalid HCLK mask %x", hclk_mask);
     return 0;
 }
 
@@ -1146,8 +1141,7 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
     uint32_t data;
 
     if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s: invalid direction for DMA checksum\n",  __func__);
+        aspeed_smc_error("invalid direction for DMA checksum");
         return;
     }
 
@@ -1159,8 +1153,8 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
         data = address_space_ldl_le(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
                                     MEMTXATTRS_UNSPECIFIED, &result);
         if (result != MEMTX_OK) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
-                          __func__, s->regs[R_DMA_FLASH_ADDR]);
+            aspeed_smc_error("Flash read failed @%08x",
+                             s->regs[R_DMA_FLASH_ADDR]);
             return;
         }
         trace_aspeed_smc_dma_checksum(s->regs[R_DMA_FLASH_ADDR], data);
@@ -1195,32 +1189,32 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
             data = address_space_ldl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
                                         MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
-                              __func__, s->regs[R_DMA_DRAM_ADDR]);
+                aspeed_smc_error("DRAM read failed @%08x",
+                                 s->regs[R_DMA_DRAM_ADDR]);
                 return;
             }
 
             address_space_stl_le(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
                                  data, MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed @%08x\n",
-                              __func__, s->regs[R_DMA_FLASH_ADDR]);
+                aspeed_smc_error("Flash write failed @%08x",
+                                 s->regs[R_DMA_FLASH_ADDR]);
                 return;
             }
         } else {
             data = address_space_ldl_le(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
                                         MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
-                              __func__, s->regs[R_DMA_FLASH_ADDR]);
+                aspeed_smc_error("Flash read failed @%08x",
+                                 s->regs[R_DMA_FLASH_ADDR]);
                 return;
             }
 
             address_space_stl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
                                  data, MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
-                              __func__, s->regs[R_DMA_DRAM_ADDR]);
+                aspeed_smc_error("DRAM write failed @%08x",
+                                 s->regs[R_DMA_DRAM_ADDR]);
                 return;
             }
         }
@@ -1280,7 +1274,7 @@ static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint32_t dma_ctrl)
     }
 
     if (aspeed_smc_dma_in_progress(s)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA in progress\n",  __func__);
+        aspeed_smc_error("DMA in progress !");
         return;
     }
 
@@ -1302,7 +1296,7 @@ static inline bool aspeed_smc_dma_granted(AspeedSMCState *s)
     }
 
     if (!(s->regs[R_DMA_CTRL] & DMA_CTRL_GRANT)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA not granted\n",  __func__);
+        aspeed_smc_error("DMA not granted");
         return false;
     }
 
@@ -1327,7 +1321,7 @@ static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t dma_ctrl)
     }
 
     if (!aspeed_smc_dma_granted(s)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA not granted\n",  __func__);
+        aspeed_smc_error("DMA not granted");
         return;
     }
 
@@ -1433,8 +1427,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
     /* Enforce some real HW limits */
     if (s->num_cs > s->ctrl->max_peripherals) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: num_cs cannot exceed: %d\n",
-                      __func__, s->ctrl->max_peripherals);
+        aspeed_smc_error("num_cs cannot exceed: %d", s->ctrl->max_peripherals);
         s->num_cs = s->ctrl->max_peripherals;
     }
 
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 03/10] aspeed/smc: Stop using the model name for the memory regions
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 02/10] aspeed/smc: Introduce aspeed_smc_error() helper Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure Cédric Le Goater
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

There is no real reason to use this name. It's simply nice to have in
the monitor output but it's a burden for the following patch which
removes the AspeedSMCController structure describing the controller.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index b4d92d75a694..b6684d6f38d0 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1391,20 +1391,15 @@ static const MemoryRegionOps aspeed_smc_ops = {
  */
 static void aspeed_smc_dma_setup(AspeedSMCState *s, Error **errp)
 {
-    char *name;
-
     if (!s->dram_mr) {
         error_setg(errp, TYPE_ASPEED_SMC ": 'dram' link not set");
         return;
     }
 
-    name = g_strdup_printf("%s-dma-flash", s->ctrl->name);
-    address_space_init(&s->flash_as, &s->mmio_flash, name);
-    g_free(name);
-
-    name = g_strdup_printf("%s-dma-dram", s->ctrl->name);
-    address_space_init(&s->dram_as, s->dram_mr, name);
-    g_free(name);
+    address_space_init(&s->flash_as, &s->mmio_flash,
+                       TYPE_ASPEED_SMC ".dma-flash");
+    address_space_init(&s->dram_as, s->dram_mr,
+                       TYPE_ASPEED_SMC ".dma-dram");
 }
 
 static void aspeed_smc_realize(DeviceState *dev, Error **errp)
@@ -1445,7 +1440,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
     /* The memory region for the controller registers */
     memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
-                          s->ctrl->name, s->ctrl->nregs * 4);
+                          TYPE_ASPEED_SMC, s->ctrl->nregs * 4);
     sysbus_init_mmio(sbd, &s->mmio);
 
     /*
@@ -1453,12 +1448,12 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
      * window in which the flash modules are mapped. The size and
      * address depends on the SoC model and controller type.
      */
-    snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
-
     memory_region_init_io(&s->mmio_flash, OBJECT(s),
-                          &aspeed_smc_flash_default_ops, s, name,
+                          &aspeed_smc_flash_default_ops, s,
+                          TYPE_ASPEED_SMC ".flash",
                           s->ctrl->flash_window_size);
-    memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s), name,
+    memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s),
+                             TYPE_ASPEED_SMC ".flash",
                              &s->mmio_flash, 0, s->ctrl->flash_window_size);
     sysbus_init_mmio(sbd, &s->mmio_flash_alias);
 
@@ -1474,7 +1469,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < s->ctrl->max_peripherals; ++i) {
         AspeedSMCFlash *fl = &s->flashes[i];
 
-        snprintf(name, sizeof(name), "%s.%d", s->ctrl->name, i);
+        snprintf(name, sizeof(name), TYPE_ASPEED_SMC ".flash.%d", i);
 
         fl->id = i;
         fl->controller = s;
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (2 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 03/10] aspeed/smc: Stop using the model name for the memory regions Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  7:28   ` Philippe Mathieu-Daudé
  2021-09-07  6:58 ` [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash Cédric Le Goater
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

The characteristics of the Aspeed controllers are described in a
AspeedSMCController structure which is redundant with the
AspeedSMCClass. Move all attributes under the class and adapt the code
to use class attributes instead.

This is a large change but it is functionally equivalent.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  64 ++-
 hw/arm/aspeed_ast2600.c     |   4 +-
 hw/arm/aspeed_soc.c         |   4 +-
 hw/ssi/aspeed_smc.c         | 859 ++++++++++++++++++++----------------
 4 files changed, 510 insertions(+), 421 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index cdaf165300b6..0ea536a44c3a 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -29,35 +29,7 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
-typedef struct AspeedSegments {
-    hwaddr addr;
-    uint32_t size;
-} AspeedSegments;
-
 struct AspeedSMCState;
-typedef struct AspeedSMCController {
-    const char *name;
-    uint8_t r_conf;
-    uint8_t r_ce_ctrl;
-    uint8_t r_ctrl0;
-    uint8_t r_timings;
-    uint8_t nregs_timings;
-    uint8_t conf_enable_w0;
-    uint8_t max_peripherals;
-    const AspeedSegments *segments;
-    hwaddr flash_window_base;
-    uint32_t flash_window_size;
-    uint32_t features;
-    hwaddr dma_flash_mask;
-    hwaddr dma_dram_mask;
-    uint32_t nregs;
-    uint32_t (*segment_to_reg)(const struct AspeedSMCState *s,
-                               const AspeedSegments *seg);
-    void (*reg_to_segment)(const struct AspeedSMCState *s, uint32_t reg,
-                           AspeedSegments *seg);
-    void (*dma_ctrl)(struct AspeedSMCState *s, uint32_t value);
-} AspeedSMCController;
-
 typedef struct AspeedSMCFlash {
     struct AspeedSMCState *controller;
 
@@ -71,18 +43,11 @@ typedef struct AspeedSMCFlash {
 #define TYPE_ASPEED_SMC "aspeed.smc"
 OBJECT_DECLARE_TYPE(AspeedSMCState, AspeedSMCClass, ASPEED_SMC)
 
-struct AspeedSMCClass {
-    SysBusDevice parent_obj;
-    const AspeedSMCController *ctrl;
-};
-
 #define ASPEED_SMC_R_MAX        (0x100 / 4)
 
 struct AspeedSMCState {
     SysBusDevice parent_obj;
 
-    const AspeedSMCController *ctrl;
-
     MemoryRegion mmio;
     MemoryRegion mmio_flash;
     MemoryRegion mmio_flash_alias;
@@ -115,4 +80,33 @@ struct AspeedSMCState {
     uint8_t snoop_dummies;
 };
 
+typedef struct AspeedSegments {
+    hwaddr addr;
+    uint32_t size;
+} AspeedSegments;
+
+struct AspeedSMCClass {
+    SysBusDeviceClass parent_obj;
+
+    uint8_t r_conf;
+    uint8_t r_ce_ctrl;
+    uint8_t r_ctrl0;
+    uint8_t r_timings;
+    uint8_t nregs_timings;
+    uint8_t conf_enable_w0;
+    uint8_t max_peripherals;
+    const AspeedSegments *segments;
+    hwaddr flash_window_base;
+    uint32_t flash_window_size;
+    uint32_t features;
+    hwaddr dma_flash_mask;
+    hwaddr dma_dram_mask;
+    uint32_t nregs;
+    uint32_t (*segment_to_reg)(const AspeedSMCState *s,
+                               const AspeedSegments *seg);
+    void (*reg_to_segment)(const AspeedSMCState *s, uint32_t reg,
+                           AspeedSegments *seg);
+    void (*dma_ctrl)(AspeedSMCState *s, uint32_t value);
+};
+
 #endif /* ASPEED_SMC_H */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 9d70e8e060c6..c69f27dff62a 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -352,7 +352,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 0, sc->memmap[ASPEED_DEV_FMC]);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 1,
-                    s->fmc.ctrl->flash_window_base);
+                    ASPEED_SMC_GET_CLASS(&s->fmc)->flash_window_base);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_FMC));
 
@@ -367,7 +367,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0,
                         sc->memmap[ASPEED_DEV_SPI1 + i]);
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 1,
-                        s->spi[i].ctrl->flash_window_base);
+                        ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
     }
 
     /* EHCI */
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index ed84502e238a..4f013dd5cd9c 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -310,7 +310,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 0, sc->memmap[ASPEED_DEV_FMC]);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->fmc), 1,
-                    s->fmc.ctrl->flash_window_base);
+                    ASPEED_SMC_GET_CLASS(&s->fmc)->flash_window_base);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_FMC));
 
@@ -323,7 +323,7 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 0,
                         sc->memmap[ASPEED_DEV_SPI1 + i]);
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi[i]), 1,
-                        s->spi[i].ctrl->flash_window_base);
+                        ASPEED_SMC_GET_CLASS(&s->spi[i])->flash_window_base);
     }
 
     /* EHCI */
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index b6684d6f38d0..ae14c384b60c 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -169,11 +169,6 @@
 #define ASPEED_SMC_R_SPI_MAX (0x20 / 4)
 #define ASPEED_SMC_R_SMC_MAX (0x20 / 4)
 
-#define ASPEED_SOC_SMC_FLASH_BASE   0x10000000
-#define ASPEED_SOC_FMC_FLASH_BASE   0x20000000
-#define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
-#define ASPEED_SOC_SPI2_FLASH_BASE  0x38000000
-
 /*
  * DMA DRAM addresses should be 4 bytes aligned and the valid address
  * range is 0x40000000 - 0x5FFFFFFF (AST2400)
@@ -186,8 +181,8 @@
  *   0: 4 bytes
  *   0x7FFFFF: 32M bytes
  */
-#define DMA_DRAM_ADDR(s, val)   ((val) & (s)->ctrl->dma_dram_mask)
-#define DMA_FLASH_ADDR(s, val)  ((val) & (s)->ctrl->dma_flash_mask)
+#define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
+#define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
 #define DMA_LENGTH(val)         ((val) & 0x01FFFFFC)
 
 /* Flash opcodes. */
@@ -201,315 +196,25 @@
  * controller. These can be changed when board is initialized with the
  * Segment Address Registers.
  */
-static const AspeedSegments aspeed_segments_legacy[] = {
-    { 0x10000000, 32 * 1024 * 1024 },
-};
-
-static const AspeedSegments aspeed_segments_fmc[] = {
-    { 0x20000000, 64 * 1024 * 1024 }, /* start address is readonly */
-    { 0x24000000, 32 * 1024 * 1024 },
-    { 0x26000000, 32 * 1024 * 1024 },
-    { 0x28000000, 32 * 1024 * 1024 },
-    { 0x2A000000, 32 * 1024 * 1024 }
-};
-
-static const AspeedSegments aspeed_segments_spi[] = {
-    { 0x30000000, 64 * 1024 * 1024 },
-};
-
-static const AspeedSegments aspeed_segments_ast2500_fmc[] = {
-    { 0x20000000, 128 * 1024 * 1024 }, /* start address is readonly */
-    { 0x28000000,  32 * 1024 * 1024 },
-    { 0x2A000000,  32 * 1024 * 1024 },
-};
-
-static const AspeedSegments aspeed_segments_ast2500_spi1[] = {
-    { 0x30000000, 32 * 1024 * 1024 }, /* start address is readonly */
-    { 0x32000000, 96 * 1024 * 1024 }, /* end address is readonly */
-};
-
-static const AspeedSegments aspeed_segments_ast2500_spi2[] = {
-    { 0x38000000, 32 * 1024 * 1024 }, /* start address is readonly */
-    { 0x3A000000, 96 * 1024 * 1024 }, /* end address is readonly */
-};
-static uint32_t aspeed_smc_segment_to_reg(const AspeedSMCState *s,
-                                          const AspeedSegments *seg);
-static void aspeed_smc_reg_to_segment(const AspeedSMCState *s, uint32_t reg,
-                                      AspeedSegments *seg);
-static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
-
-/*
- * AST2600 definitions
- */
-#define ASPEED26_SOC_FMC_FLASH_BASE   0x20000000
-#define ASPEED26_SOC_SPI_FLASH_BASE   0x30000000
-#define ASPEED26_SOC_SPI2_FLASH_BASE  0x50000000
-
-static const AspeedSegments aspeed_segments_ast2600_fmc[] = {
-    { 0x0, 128 * MiB }, /* start address is readonly */
-    { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
-    { 0x0, 0 }, /* disabled */
-};
-
-static const AspeedSegments aspeed_segments_ast2600_spi1[] = {
-    { 0x0, 128 * MiB }, /* start address is readonly */
-    { 0x0, 0 }, /* disabled */
-};
-
-static const AspeedSegments aspeed_segments_ast2600_spi2[] = {
-    { 0x0, 128 * MiB }, /* start address is readonly */
-    { 0x0, 0 }, /* disabled */
-    { 0x0, 0 }, /* disabled */
-};
-
-static uint32_t aspeed_2600_smc_segment_to_reg(const AspeedSMCState *s,
-                                               const AspeedSegments *seg);
-static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
-                                           uint32_t reg, AspeedSegments *seg);
-static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
+static const AspeedSegments aspeed_2400_fmc_segments[];
+static const AspeedSegments aspeed_2400_spi1_segments[];
+static const AspeedSegments aspeed_2500_fmc_segments[];
+static const AspeedSegments aspeed_2500_spi1_segments[];
+static const AspeedSegments aspeed_2500_spi2_segments[];
+static const AspeedSegments aspeed_2600_fmc_segments[];
 
 #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",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 1,
-        .segments          = aspeed_segments_legacy,
-        .flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
-        .flash_window_size = 0x6000000,
-        .features          = 0x0,
-        .nregs             = ASPEED_SMC_R_SMC_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.fmc-ast2400",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 5,
-        .segments          = aspeed_segments_fmc,
-        .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA,
-        .dma_flash_mask    = 0x0FFFFFFC,
-        .dma_dram_mask     = 0x1FFFFFFC,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.spi1-ast2400",
-        .r_conf            = R_SPI_CONF,
-        .r_ce_ctrl         = 0xff,
-        .r_ctrl0           = R_SPI_CTRL0,
-        .r_timings         = R_SPI_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = SPI_CONF_ENABLE_W0,
-        .max_peripherals   = 1,
-        .segments          = aspeed_segments_spi,
-        .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = 0x0,
-        .nregs             = ASPEED_SMC_R_SPI_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.fmc-ast2500",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 3,
-        .segments          = aspeed_segments_ast2500_fmc,
-        .flash_window_base = ASPEED_SOC_FMC_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA,
-        .dma_flash_mask    = 0x0FFFFFFC,
-        .dma_dram_mask     = 0x3FFFFFFC,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.spi1-ast2500",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 2,
-        .segments          = aspeed_segments_ast2500_spi1,
-        .flash_window_base = ASPEED_SOC_SPI_FLASH_BASE,
-        .flash_window_size = 0x8000000,
-        .features          = 0x0,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.spi2-ast2500",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 2,
-        .segments          = aspeed_segments_ast2500_spi2,
-        .flash_window_base = ASPEED_SOC_SPI2_FLASH_BASE,
-        .flash_window_size = 0x8000000,
-        .features          = 0x0,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.fmc-ast2600",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 1,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 3,
-        .segments          = aspeed_segments_ast2600_fmc,
-        .flash_window_base = ASPEED26_SOC_FMC_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA,
-        .dma_flash_mask    = 0x0FFFFFFC,
-        .dma_dram_mask     = 0x3FFFFFFC,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_2600_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_2600_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_2600_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.spi1-ast2600",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 2,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 2,
-        .segments          = aspeed_segments_ast2600_spi1,
-        .flash_window_base = ASPEED26_SOC_SPI_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA |
-                             ASPEED_SMC_FEATURE_DMA_GRANT,
-        .dma_flash_mask    = 0x0FFFFFFC,
-        .dma_dram_mask     = 0x3FFFFFFC,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_2600_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_2600_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_2600_smc_dma_ctrl,
-    }, {
-        .name              = "aspeed.spi2-ast2600",
-        .r_conf            = R_CONF,
-        .r_ce_ctrl         = R_CE_CTRL,
-        .r_ctrl0           = R_CTRL0,
-        .r_timings         = R_TIMINGS,
-        .nregs_timings     = 3,
-        .conf_enable_w0    = CONF_ENABLE_W0,
-        .max_peripherals   = 3,
-        .segments          = aspeed_segments_ast2600_spi2,
-        .flash_window_base = ASPEED26_SOC_SPI2_FLASH_BASE,
-        .flash_window_size = 0x10000000,
-        .features          = ASPEED_SMC_FEATURE_DMA |
-                             ASPEED_SMC_FEATURE_DMA_GRANT,
-        .dma_flash_mask    = 0x0FFFFFFC,
-        .dma_dram_mask     = 0x3FFFFFFC,
-        .nregs             = ASPEED_SMC_R_MAX,
-        .segment_to_reg    = aspeed_2600_smc_segment_to_reg,
-        .reg_to_segment    = aspeed_2600_smc_reg_to_segment,
-        .dma_ctrl          = aspeed_2600_smc_dma_ctrl,
-    },
-};
-
-/*
- * The Segment Registers of the AST2400 and AST2500 have a 8MB
- * unit. The address range of a flash SPI peripheral is encoded with
- * absolute addresses which should be part of the overall controller
- * window.
- */
-static uint32_t aspeed_smc_segment_to_reg(const AspeedSMCState *s,
-                                          const AspeedSegments *seg)
-{
-    uint32_t reg = 0;
-    reg |= ((seg->addr >> 23) & SEG_START_MASK) << SEG_START_SHIFT;
-    reg |= (((seg->addr + seg->size) >> 23) & SEG_END_MASK) << SEG_END_SHIFT;
-    return reg;
-}
-
-static void aspeed_smc_reg_to_segment(const AspeedSMCState *s,
-                                      uint32_t reg, AspeedSegments *seg)
-{
-    seg->addr = ((reg >> SEG_START_SHIFT) & SEG_START_MASK) << 23;
-    seg->size = (((reg >> SEG_END_SHIFT) & SEG_END_MASK) << 23) - seg->addr;
-}
-
-/*
- * The Segment Registers of the AST2600 have a 1MB unit. The address
- * range of a flash SPI peripheral is encoded with offsets in the overall
- * controller window. The previous SoC AST2400 and AST2500 used
- * absolute addresses. Only bits [27:20] are relevant and the end
- * address is an upper bound limit.
- */
-#define AST2600_SEG_ADDR_MASK 0x0ff00000
-
-static uint32_t aspeed_2600_smc_segment_to_reg(const AspeedSMCState *s,
-                                               const AspeedSegments *seg)
+static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
 {
-    uint32_t reg = 0;
-
-    /* Disabled segments have a nil register */
-    if (!seg->size) {
-        return 0;
-    }
-
-    reg |= (seg->addr & AST2600_SEG_ADDR_MASK) >> 16; /* start offset */
-    reg |= (seg->addr + seg->size - 1) & AST2600_SEG_ADDR_MASK; /* end offset */
-    return reg;
+    return !!(asc->features & ASPEED_SMC_FEATURE_DMA);
 }
 
-static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
-                                           uint32_t reg, AspeedSegments *seg)
+static inline bool aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
 {
-    uint32_t start_offset = (reg << 16) & AST2600_SEG_ADDR_MASK;
-    uint32_t end_offset = reg & AST2600_SEG_ADDR_MASK;
-
-    if (reg) {
-        seg->addr = s->ctrl->flash_window_base + start_offset;
-        seg->size = end_offset + MiB - start_offset;
-    } else {
-        seg->addr = s->ctrl->flash_window_base;
-        seg->size = 0;
-    }
+    return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
 }
 
 #define aspeed_smc_error(fmt, ...)                                      \
@@ -519,15 +224,16 @@ static bool aspeed_smc_flash_overlap(const AspeedSMCState *s,
                                      const AspeedSegments *new,
                                      int cs)
 {
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     AspeedSegments seg;
     int i;
 
-    for (i = 0; i < s->ctrl->max_peripherals; i++) {
+    for (i = 0; i < asc->max_peripherals; i++) {
         if (i == cs) {
             continue;
         }
 
-        s->ctrl->reg_to_segment(s, s->regs[R_SEG_ADDR0 + i], &seg);
+        asc->reg_to_segment(s, s->regs[R_SEG_ADDR0 + i], &seg);
 
         if (new->addr + new->size > seg.addr &&
             new->addr < seg.addr + seg.size) {
@@ -545,14 +251,15 @@ static bool aspeed_smc_flash_overlap(const AspeedSMCState *s,
 static void aspeed_smc_flash_set_segment_region(AspeedSMCState *s, int cs,
                                                 uint64_t regval)
 {
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     AspeedSMCFlash *fl = &s->flashes[cs];
     AspeedSegments seg;
 
-    s->ctrl->reg_to_segment(s, regval, &seg);
+    asc->reg_to_segment(s, regval, &seg);
 
     memory_region_transaction_begin();
     memory_region_set_size(&fl->mmio, seg.size);
-    memory_region_set_address(&fl->mmio, seg.addr - s->ctrl->flash_window_base);
+    memory_region_set_address(&fl->mmio, seg.addr - asc->flash_window_base);
     memory_region_set_enabled(&fl->mmio, !!seg.size);
     memory_region_transaction_commit();
 
@@ -562,40 +269,41 @@ static void aspeed_smc_flash_set_segment_region(AspeedSMCState *s, int cs,
 static void aspeed_smc_flash_set_segment(AspeedSMCState *s, int cs,
                                          uint64_t new)
 {
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     AspeedSegments seg;
 
-    s->ctrl->reg_to_segment(s, new, &seg);
+    asc->reg_to_segment(s, new, &seg);
 
     trace_aspeed_smc_flash_set_segment(cs, new, seg.addr, seg.addr + seg.size);
 
     /* The start address of CS0 is read-only */
-    if (cs == 0 && seg.addr != s->ctrl->flash_window_base) {
+    if (cs == 0 && seg.addr != asc->flash_window_base) {
         aspeed_smc_error("Tried to change CS0 start address to 0x%"
                          HWADDR_PRIx, seg.addr);
-        seg.addr = s->ctrl->flash_window_base;
-        new = s->ctrl->segment_to_reg(s, &seg);
+        seg.addr = asc->flash_window_base;
+        new = asc->segment_to_reg(s, &seg);
     }
 
     /*
      * The end address of the AST2500 spi controllers is also
      * read-only.
      */
-    if ((s->ctrl->segments == aspeed_segments_ast2500_spi1 ||
-         s->ctrl->segments == aspeed_segments_ast2500_spi2) &&
-        cs == s->ctrl->max_peripherals &&
-        seg.addr + seg.size != s->ctrl->segments[cs].addr +
-        s->ctrl->segments[cs].size) {
+    if ((asc->segments == aspeed_2500_spi1_segments ||
+         asc->segments == aspeed_2500_spi2_segments) &&
+        cs == asc->max_peripherals &&
+        seg.addr + seg.size != asc->segments[cs].addr +
+        asc->segments[cs].size) {
         aspeed_smc_error("Tried to change CS%d end address to 0x%"
                          HWADDR_PRIx, cs, seg.addr + seg.size);
-        seg.size = s->ctrl->segments[cs].addr + s->ctrl->segments[cs].size -
+        seg.size = asc->segments[cs].addr + asc->segments[cs].size -
             seg.addr;
-        new = s->ctrl->segment_to_reg(s, &seg);
+        new = asc->segment_to_reg(s, &seg);
     }
 
     /* Keep the segment in the overall flash window */
     if (seg.size &&
-        (seg.addr + seg.size <= s->ctrl->flash_window_base ||
-         seg.addr > s->ctrl->flash_window_base + s->ctrl->flash_window_size)) {
+        (seg.addr + seg.size <= asc->flash_window_base ||
+         seg.addr > asc->flash_window_base + asc->flash_window_size)) {
         aspeed_smc_error("new segment for CS%d is invalid : "
                          "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
                          cs, seg.addr, seg.addr + seg.size);
@@ -680,8 +388,9 @@ static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
 static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
 
-    if (s->ctrl->segments == aspeed_segments_spi) {
+    if (asc->segments == aspeed_2400_spi1_segments) {
         return s->regs[s->r_ctrl0] & CTRL_AST2400_SPI_4BYTE;
     } else {
         return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->id));
@@ -711,9 +420,10 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
                                               uint32_t addr)
 {
     const AspeedSMCState *s = fl->controller;
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     AspeedSegments seg;
 
-    s->ctrl->reg_to_segment(s, s->regs[R_SEG_ADDR0 + fl->id], &seg);
+    asc->reg_to_segment(s, s->regs[R_SEG_ADDR0 + fl->id], &seg);
     if ((addr % seg.size) != addr) {
         aspeed_smc_error("invalid address 0x%08x for CS%d segment : "
                          "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
@@ -973,6 +683,7 @@ static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value)
 static void aspeed_smc_reset(DeviceState *d)
 {
     AspeedSMCState *s = ASPEED_SMC(d);
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     int i;
 
     memset(s->regs, 0, sizeof s->regs);
@@ -984,13 +695,13 @@ static void aspeed_smc_reset(DeviceState *d)
     }
 
     /* setup the default segment register values and regions for all */
-    for (i = 0; i < s->ctrl->max_peripherals; ++i) {
+    for (i = 0; i < asc->max_peripherals; ++i) {
         aspeed_smc_flash_set_segment_region(s, i,
-                    s->ctrl->segment_to_reg(s, &s->ctrl->segments[i]));
+                    asc->segment_to_reg(s, &asc->segments[i]));
     }
 
     /* HW strapping flash type for the AST2600 controllers  */
-    if (s->ctrl->segments == aspeed_segments_ast2600_fmc) {
+    if (asc->segments == aspeed_2600_fmc_segments) {
         /* flash type is fixed to SPI for all */
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
@@ -998,7 +709,7 @@ static void aspeed_smc_reset(DeviceState *d)
     }
 
     /* HW strapping flash type for FMC controllers  */
-    if (s->ctrl->segments == aspeed_segments_ast2500_fmc) {
+    if (asc->segments == aspeed_2500_fmc_segments) {
         /* flash type is fixed to SPI for CE0 and CE1 */
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
@@ -1006,7 +717,7 @@ static void aspeed_smc_reset(DeviceState *d)
 
     /* HW strapping for AST2400 FMC controllers (SCU70). Let's use the
      * configuration of the palmetto-bmc machine */
-    if (s->ctrl->segments == aspeed_segments_fmc) {
+    if (asc->segments == aspeed_2400_fmc_segments) {
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
     }
 
@@ -1017,25 +728,26 @@ static void aspeed_smc_reset(DeviceState *d)
 static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(opaque);
 
     addr >>= 2;
 
     if (addr == s->r_conf ||
         (addr >= s->r_timings &&
-         addr < s->r_timings + s->ctrl->nregs_timings) ||
+         addr < s->r_timings + asc->nregs_timings) ||
         addr == s->r_ce_ctrl ||
         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) ||
-        (aspeed_smc_has_dma(s) && addr == R_DMA_LEN) ||
-        (aspeed_smc_has_dma(s) && addr == R_DMA_CHECKSUM) ||
+        (aspeed_smc_has_wdt_control(asc) && addr == R_FMC_WDT2_CTRL) ||
+        (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
+        (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR) ||
+        (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR) ||
+        (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) ||
+        (aspeed_smc_has_dma(asc) && addr == R_DMA_CHECKSUM) ||
         (addr >= R_SEG_ADDR0 &&
-         addr < R_SEG_ADDR0 + s->ctrl->max_peripherals) ||
-        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_peripherals)) {
+         addr < R_SEG_ADDR0 + asc->max_peripherals) ||
+        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + asc->max_peripherals)) {
 
         trace_aspeed_smc_read(addr, size, s->regs[addr]);
 
@@ -1291,7 +1003,9 @@ static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint32_t dma_ctrl)
 
 static inline bool aspeed_smc_dma_granted(AspeedSMCState *s)
 {
-    if (!(s->ctrl->features & ASPEED_SMC_FEATURE_DMA_GRANT)) {
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
+
+    if (!(asc->features & ASPEED_SMC_FEATURE_DMA_GRANT)) {
         return true;
     }
 
@@ -1333,6 +1047,7 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
                              unsigned int size)
 {
     AspeedSMCState *s = ASPEED_SMC(opaque);
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     uint32_t value = data;
 
     addr >>= 2;
@@ -1341,14 +1056,14 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
 
     if (addr == s->r_conf ||
         (addr >= s->r_timings &&
-         addr < s->r_timings + s->ctrl->nregs_timings) ||
+         addr < s->r_timings + asc->nregs_timings) ||
         addr == s->r_ce_ctrl) {
         s->regs[addr] = value;
     } else if (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs) {
         int cs = addr - s->r_ctrl0;
         aspeed_smc_flash_update_ctrl(&s->flashes[cs], value);
     } else if (addr >= R_SEG_ADDR0 &&
-               addr < R_SEG_ADDR0 + s->ctrl->max_peripherals) {
+               addr < R_SEG_ADDR0 + asc->max_peripherals) {
         int cs = addr - R_SEG_ADDR0;
 
         if (value != s->regs[R_SEG_ADDR0 + cs]) {
@@ -1358,19 +1073,19 @@ 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) {
+    } else if (aspeed_smc_has_wdt_control(asc) && 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) {
-        s->ctrl->dma_ctrl(s, value);
-    } else if (aspeed_smc_has_dma(s) && addr == R_DMA_DRAM_ADDR &&
+    } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) {
+        asc->dma_ctrl(s, value);
+    } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR &&
                aspeed_smc_dma_granted(s)) {
-        s->regs[addr] = DMA_DRAM_ADDR(s, value);
-    } else if (aspeed_smc_has_dma(s) && addr == R_DMA_FLASH_ADDR &&
+        s->regs[addr] = DMA_DRAM_ADDR(asc, value);
+    } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR &&
                aspeed_smc_dma_granted(s)) {
-        s->regs[addr] = DMA_FLASH_ADDR(s, value);
-    } else if (aspeed_smc_has_dma(s) && addr == R_DMA_LEN &&
+        s->regs[addr] = DMA_FLASH_ADDR(asc, value);
+    } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
                aspeed_smc_dma_granted(s)) {
         s->regs[addr] = DMA_LENGTH(value);
     } else {
@@ -1406,24 +1121,22 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedSMCState *s = ASPEED_SMC(dev);
-    AspeedSMCClass *mc = ASPEED_SMC_GET_CLASS(s);
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     int i;
     char name[32];
     hwaddr offset = 0;
 
-    s->ctrl = mc->ctrl;
-
     /* keep a copy under AspeedSMCState to speed up accesses */
-    s->r_conf = s->ctrl->r_conf;
-    s->r_ce_ctrl = s->ctrl->r_ce_ctrl;
-    s->r_ctrl0 = s->ctrl->r_ctrl0;
-    s->r_timings = s->ctrl->r_timings;
-    s->conf_enable_w0 = s->ctrl->conf_enable_w0;
+    s->r_conf = asc->r_conf;
+    s->r_ce_ctrl = asc->r_ce_ctrl;
+    s->r_ctrl0 = asc->r_ctrl0;
+    s->r_timings = asc->r_timings;
+    s->conf_enable_w0 = asc->conf_enable_w0;
 
     /* Enforce some real HW limits */
-    if (s->num_cs > s->ctrl->max_peripherals) {
-        aspeed_smc_error("num_cs cannot exceed: %d", s->ctrl->max_peripherals);
-        s->num_cs = s->ctrl->max_peripherals;
+    if (s->num_cs > asc->max_peripherals) {
+        aspeed_smc_error("num_cs cannot exceed: %d", asc->max_peripherals);
+        s->num_cs = asc->max_peripherals;
     }
 
     /* DMA irq. Keep it first for the initialization in the SoC */
@@ -1440,7 +1153,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
     /* The memory region for the controller registers */
     memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
-                          TYPE_ASPEED_SMC, s->ctrl->nregs * 4);
+                          TYPE_ASPEED_SMC, asc->nregs * 4);
     sysbus_init_mmio(sbd, &s->mmio);
 
     /*
@@ -1451,13 +1164,13 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->mmio_flash, OBJECT(s),
                           &aspeed_smc_flash_default_ops, s,
                           TYPE_ASPEED_SMC ".flash",
-                          s->ctrl->flash_window_size);
+                          asc->flash_window_size);
     memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s),
                              TYPE_ASPEED_SMC ".flash",
-                             &s->mmio_flash, 0, s->ctrl->flash_window_size);
+                             &s->mmio_flash, 0, asc->flash_window_size);
     sysbus_init_mmio(sbd, &s->mmio_flash_alias);
 
-    s->flashes = g_new0(AspeedSMCFlash, s->ctrl->max_peripherals);
+    s->flashes = g_new0(AspeedSMCFlash, asc->max_peripherals);
 
     /*
      * Let's create a sub memory region for each possible peripheral. All
@@ -1466,14 +1179,14 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
      * module behind to handle the memory accesses. This depends on
      * the board configuration.
      */
-    for (i = 0; i < s->ctrl->max_peripherals; ++i) {
+    for (i = 0; i < asc->max_peripherals; ++i) {
         AspeedSMCFlash *fl = &s->flashes[i];
 
         snprintf(name, sizeof(name), TYPE_ASPEED_SMC ".flash.%d", i);
 
         fl->id = i;
         fl->controller = s;
-        fl->size = s->ctrl->segments[i].size;
+        fl->size = asc->segments[i].size;
         memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
                               fl, name, fl->size);
         memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
@@ -1481,7 +1194,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     }
 
     /* DMA support */
-    if (aspeed_smc_has_dma(s)) {
+    if (aspeed_smc_has_dma(asc)) {
         aspeed_smc_dma_setup(s, errp);
     }
 }
@@ -1509,13 +1222,11 @@ static Property aspeed_smc_properties[] = {
 static void aspeed_smc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    AspeedSMCClass *mc = ASPEED_SMC_CLASS(klass);
 
     dc->realize = aspeed_smc_realize;
     dc->reset = aspeed_smc_reset;
     device_class_set_props(dc, aspeed_smc_properties);
     dc->vmsd = &vmstate_aspeed_smc;
-    mc->ctrl = data;
 }
 
 static const TypeInfo aspeed_smc_info = {
@@ -1523,23 +1234,407 @@ static const TypeInfo aspeed_smc_info = {
     .parent         = TYPE_SYS_BUS_DEVICE,
     .instance_size  = sizeof(AspeedSMCState),
     .class_size     = sizeof(AspeedSMCClass),
+    .class_init     = aspeed_smc_class_init,
     .abstract       = true,
 };
 
-static void aspeed_smc_register_types(void)
+
+/*
+ * The Segment Registers of the AST2400 and AST2500 have a 8MB
+ * unit. The address range of a flash SPI peripheral is encoded with
+ * absolute addresses which should be part of the overall controller
+ * window.
+ */
+static uint32_t aspeed_smc_segment_to_reg(const AspeedSMCState *s,
+                                          const AspeedSegments *seg)
 {
-    int i;
+    uint32_t reg = 0;
+    reg |= ((seg->addr >> 23) & SEG_START_MASK) << SEG_START_SHIFT;
+    reg |= (((seg->addr + seg->size) >> 23) & SEG_END_MASK) << SEG_END_SHIFT;
+    return reg;
+}
 
-    type_register_static(&aspeed_smc_info);
-    for (i = 0; i < ARRAY_SIZE(controllers); ++i) {
-        TypeInfo ti = {
-            .name       = controllers[i].name,
-            .parent     = TYPE_ASPEED_SMC,
-            .class_init = aspeed_smc_class_init,
-            .class_data = (void *)&controllers[i],
-        };
-        type_register(&ti);
+static void aspeed_smc_reg_to_segment(const AspeedSMCState *s,
+                                      uint32_t reg, AspeedSegments *seg)
+{
+    seg->addr = ((reg >> SEG_START_SHIFT) & SEG_START_MASK) << 23;
+    seg->size = (((reg >> SEG_END_SHIFT) & SEG_END_MASK) << 23) - seg->addr;
+}
+
+static const AspeedSegments aspeed_2400_smc_segments[] = {
+    { 0x10000000, 32 * MiB },
+};
+
+static void aspeed_2400_smc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2400 SMC Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 1;
+    asc->segments          = aspeed_2400_smc_segments;
+    asc->flash_window_base = 0x10000000;
+    asc->flash_window_size = 0x6000000;
+    asc->features          = 0x0;
+    asc->nregs             = ASPEED_SMC_R_SMC_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2400_smc_info = {
+    .name =  "aspeed.smc-ast2400",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2400_smc_class_init,
+};
+
+static const AspeedSegments aspeed_2400_fmc_segments[] = {
+    { 0x20000000, 64 * MiB }, /* start address is readonly */
+    { 0x24000000, 32 * MiB },
+    { 0x26000000, 32 * MiB },
+    { 0x28000000, 32 * MiB },
+    { 0x2A000000, 32 * MiB }
+};
+
+static void aspeed_2400_fmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2400 FMC Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 5;
+    asc->segments          = aspeed_2400_fmc_segments;
+    asc->flash_window_base = 0x20000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = ASPEED_SMC_FEATURE_DMA;
+    asc->dma_flash_mask    = 0x0FFFFFFC;
+    asc->dma_dram_mask     = 0x1FFFFFFC;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2400_fmc_info = {
+    .name =  "aspeed.fmc-ast2400",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2400_fmc_class_init,
+};
+
+static const AspeedSegments aspeed_2400_spi1_segments[] = {
+    { 0x30000000, 64 * MiB },
+};
+
+static void aspeed_2400_spi1_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2400 SPI1 Controller";
+    asc->r_conf            = R_SPI_CONF;
+    asc->r_ce_ctrl         = 0xff;
+    asc->r_ctrl0           = R_SPI_CTRL0;
+    asc->r_timings         = R_SPI_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = SPI_CONF_ENABLE_W0;
+    asc->max_peripherals   = 1;
+    asc->segments          = aspeed_2400_spi1_segments;
+    asc->flash_window_base = 0x30000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = 0x0;
+    asc->nregs             = ASPEED_SMC_R_SPI_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2400_spi1_info = {
+    .name =  "aspeed.spi1-ast2400",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2400_spi1_class_init,
+};
+
+static const AspeedSegments aspeed_2500_fmc_segments[] = {
+    { 0x20000000, 128 * MiB }, /* start address is readonly */
+    { 0x28000000,  32 * MiB },
+    { 0x2A000000,  32 * MiB },
+};
+
+static void aspeed_2500_fmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 FMC Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 3;
+    asc->segments          = aspeed_2500_fmc_segments;
+    asc->flash_window_base = 0x20000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = ASPEED_SMC_FEATURE_DMA;
+    asc->dma_flash_mask    = 0x0FFFFFFC;
+    asc->dma_dram_mask     = 0x3FFFFFFC;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2500_fmc_info = {
+    .name =  "aspeed.fmc-ast2500",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2500_fmc_class_init,
+};
+
+static const AspeedSegments aspeed_2500_spi1_segments[] = {
+    { 0x30000000, 32 * MiB }, /* start address is readonly */
+    { 0x32000000, 96 * MiB }, /* end address is readonly */
+};
+
+static void aspeed_2500_spi1_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 SPI1 Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 2;
+    asc->segments          = aspeed_2500_spi1_segments;
+    asc->flash_window_base = 0x30000000;
+    asc->flash_window_size = 0x8000000;
+    asc->features          = 0x0;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2500_spi1_info = {
+    .name =  "aspeed.spi1-ast2500",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2500_spi1_class_init,
+};
+
+static const AspeedSegments aspeed_2500_spi2_segments[] = {
+    { 0x38000000, 32 * MiB }, /* start address is readonly */
+    { 0x3A000000, 96 * MiB }, /* end address is readonly */
+};
+
+static void aspeed_2500_spi2_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 SPI2 Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 2;
+    asc->segments          = aspeed_2500_spi2_segments;
+    asc->flash_window_base = 0x38000000;
+    asc->flash_window_size = 0x8000000;
+    asc->features          = 0x0;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2500_spi2_info = {
+    .name =  "aspeed.spi2-ast2500",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2500_spi2_class_init,
+};
+
+/*
+ * The Segment Registers of the AST2600 have a 1MB unit. The address
+ * range of a flash SPI peripheral is encoded with offsets in the overall
+ * controller window. The previous SoC AST2400 and AST2500 used
+ * absolute addresses. Only bits [27:20] are relevant and the end
+ * address is an upper bound limit.
+ */
+#define AST2600_SEG_ADDR_MASK 0x0ff00000
+
+static uint32_t aspeed_2600_smc_segment_to_reg(const AspeedSMCState *s,
+                                               const AspeedSegments *seg)
+{
+    uint32_t reg = 0;
+
+    /* Disabled segments have a nil register */
+    if (!seg->size) {
+        return 0;
+    }
+
+    reg |= (seg->addr & AST2600_SEG_ADDR_MASK) >> 16; /* start offset */
+    reg |= (seg->addr + seg->size - 1) & AST2600_SEG_ADDR_MASK; /* end offset */
+    return reg;
+}
+
+static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
+                                           uint32_t reg, AspeedSegments *seg)
+{
+    uint32_t start_offset = (reg << 16) & AST2600_SEG_ADDR_MASK;
+    uint32_t end_offset = reg & AST2600_SEG_ADDR_MASK;
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
+
+    if (reg) {
+        seg->addr = asc->flash_window_base + start_offset;
+        seg->size = end_offset + MiB - start_offset;
+    } else {
+        seg->addr = asc->flash_window_base;
+        seg->size = 0;
     }
 }
 
+static const AspeedSegments aspeed_2600_fmc_segments[] = {
+    { 0x0, 128 * MiB }, /* start address is readonly */
+    { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
+    { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2600_fmc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 FMC Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 1;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 3;
+    asc->segments          = aspeed_2600_fmc_segments;
+    asc->flash_window_base = 0x20000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = ASPEED_SMC_FEATURE_DMA;
+    asc->dma_flash_mask    = 0x0FFFFFFC;
+    asc->dma_dram_mask     = 0x3FFFFFFC;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_2600_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_2600_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2600_fmc_info = {
+    .name =  "aspeed.fmc-ast2600",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2600_fmc_class_init,
+};
+
+static const AspeedSegments aspeed_2600_spi1_segments[] = {
+    { 0x0, 128 * MiB }, /* start address is readonly */
+    { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2600_spi1_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 SPI1 Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 2;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 2;
+    asc->segments          = aspeed_2600_spi1_segments;
+    asc->flash_window_base = 0x30000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = ASPEED_SMC_FEATURE_DMA |
+                             ASPEED_SMC_FEATURE_DMA_GRANT;
+    asc->dma_flash_mask    = 0x0FFFFFFC;
+    asc->dma_dram_mask     = 0x3FFFFFFC;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_2600_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_2600_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2600_spi1_info = {
+    .name =  "aspeed.spi1-ast2600",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2600_spi1_class_init,
+};
+
+static const AspeedSegments aspeed_2600_spi2_segments[] = {
+    { 0x0, 128 * MiB }, /* start address is readonly */
+    { 0x0, 0 }, /* disabled */
+    { 0x0, 0 }, /* disabled */
+};
+
+static void aspeed_2600_spi2_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedSMCClass *asc = ASPEED_SMC_CLASS(klass);
+
+    dc->desc               = "Aspeed 2600 SPI2 Controller";
+    asc->r_conf            = R_CONF;
+    asc->r_ce_ctrl         = R_CE_CTRL;
+    asc->r_ctrl0           = R_CTRL0;
+    asc->r_timings         = R_TIMINGS;
+    asc->nregs_timings     = 3;
+    asc->conf_enable_w0    = CONF_ENABLE_W0;
+    asc->max_peripherals   = 3;
+    asc->segments          = aspeed_2600_spi2_segments;
+    asc->flash_window_base = 0x50000000;
+    asc->flash_window_size = 0x10000000;
+    asc->features          = ASPEED_SMC_FEATURE_DMA |
+                             ASPEED_SMC_FEATURE_DMA_GRANT;
+    asc->dma_flash_mask    = 0x0FFFFFFC;
+    asc->dma_dram_mask     = 0x3FFFFFFC;
+    asc->nregs             = ASPEED_SMC_R_MAX;
+    asc->segment_to_reg    = aspeed_2600_smc_segment_to_reg;
+    asc->reg_to_segment    = aspeed_2600_smc_reg_to_segment;
+    asc->dma_ctrl          = aspeed_2600_smc_dma_ctrl;
+}
+
+static const TypeInfo aspeed_2600_spi2_info = {
+    .name =  "aspeed.spi2-ast2600",
+    .parent = TYPE_ASPEED_SMC,
+    .class_init = aspeed_2600_spi2_class_init,
+};
+
+static void aspeed_smc_register_types(void)
+{
+    type_register_static(&aspeed_smc_info);
+    type_register_static(&aspeed_2400_smc_info);
+    type_register_static(&aspeed_2400_fmc_info);
+    type_register_static(&aspeed_2400_spi1_info);
+    type_register_static(&aspeed_2500_fmc_info);
+    type_register_static(&aspeed_2500_spi1_info);
+    type_register_static(&aspeed_2500_spi2_info);
+    type_register_static(&aspeed_2600_fmc_info);
+    type_register_static(&aspeed_2600_spi1_info);
+    type_register_static(&aspeed_2600_spi2_info);
+}
+
 type_init(aspeed_smc_register_types)
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (3 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  8:36   ` Philippe Mathieu-Daudé
  2021-09-07  6:58 ` [PATCH 06/10] aspeed/smc: Remove the 'size' " Cédric Le Goater
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

There is no use for it.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  1 -
 hw/arm/aspeed.c             | 11 +++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 0ea536a44c3a..f32f66f9a838 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
     uint32_t size;
 
     MemoryRegion mmio;
-    DeviceState *flash;
 } AspeedSMCFlash;
 
 #define TYPE_ASPEED_SMC "aspeed.smc"
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 95ce4b1670ac..64c3a7fb66db 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
     int i ;
 
     for (i = 0; i < s->num_cs; ++i) {
-        AspeedSMCFlash *fl = &s->flashes[i];
         DriveInfo *dinfo = drive_get_next(IF_MTD);
         qemu_irq cs_line;
+        DeviceState *dev;
 
-        fl->flash = qdev_new(flashtype);
+        dev = qdev_new(flashtype);
         if (dinfo) {
-            qdev_prop_set_drive(fl->flash, "drive",
-                                blk_by_legacy_dinfo(dinfo));
+            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
         }
-        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
+        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
 
-        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
+        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
         sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
     }
 }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 06/10] aspeed/smc: Remove the 'size' attribute from AspeedSMCFlash
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (4 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  7:20   ` Philippe Mathieu-Daudé
  2021-09-07  6:58 ` [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs' Cédric Le Goater
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

It's only useful to compute the initial size of the boot_rom region.
Use memory_region_size() instead.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h | 1 -
 hw/arm/aspeed.c             | 7 ++++---
 hw/ssi/aspeed_smc.c         | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index f32f66f9a838..097bb6aaf5f8 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -34,7 +34,6 @@ typedef struct AspeedSMCFlash {
     struct AspeedSMCState *controller;
 
     uint8_t id;
-    uint32_t size;
 
     MemoryRegion mmio;
 } AspeedSMCFlash;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 64c3a7fb66db..ebe11a2e5245 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -376,6 +376,7 @@ static void aspeed_machine_init(MachineState *machine)
     if (drive0) {
         AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
         MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+        uint64_t size = memory_region_size(&fl->mmio);
 
         /*
          * create a ROM region using the default mapping window size of
@@ -385,15 +386,15 @@ static void aspeed_machine_init(MachineState *machine)
          */
         if (ASPEED_MACHINE(machine)->mmio_exec) {
             memory_region_init_alias(boot_rom, NULL, "aspeed.boot_rom",
-                                     &fl->mmio, 0, fl->size);
+                                     &fl->mmio, 0, size);
             memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
                                         boot_rom);
         } else {
             memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
-                                   fl->size, &error_abort);
+                                   size, &error_abort);
             memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
                                         boot_rom);
-            write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
+            write_boot_rom(drive0, FIRMWARE_ADDR, size, &error_abort);
         }
     }
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index ae14c384b60c..6d0984cc1d17 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1186,11 +1186,10 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
         fl->id = i;
         fl->controller = s;
-        fl->size = asc->segments[i].size;
         memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
-                              fl, name, fl->size);
+                              fl, name, asc->segments[i].size);
         memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
-        offset += fl->size;
+        offset += asc->segments[i].size;
     }
 
     /* DMA support */
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs'
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (5 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 06/10] aspeed/smc: Remove the 'size' " Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  7:17   ` Philippe Mathieu-Daudé
  2021-09-07  6:58 ` [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash Cédric Le Goater
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

'cs' is a more appropriate name to index SPI flash devices.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  2 +-
 hw/ssi/aspeed_smc.c         | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 097bb6aaf5f8..40b6926b3e02 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -33,7 +33,7 @@ struct AspeedSMCState;
 typedef struct AspeedSMCFlash {
     struct AspeedSMCState *controller;
 
-    uint8_t id;
+    uint8_t cs;
 
     MemoryRegion mmio;
 } AspeedSMCFlash;
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 6d0984cc1d17..a47719946941 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -352,20 +352,20 @@ static inline int aspeed_smc_flash_mode(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
 
-    return s->regs[s->r_ctrl0 + fl->id] & CTRL_CMD_MODE_MASK;
+    return s->regs[s->r_ctrl0 + fl->cs] & CTRL_CMD_MODE_MASK;
 }
 
 static inline bool aspeed_smc_is_writable(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
 
-    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->id));
+    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + fl->cs));
 }
 
 static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
-    int cmd = (s->regs[s->r_ctrl0 + fl->id] >> CTRL_CMD_SHIFT) & CTRL_CMD_MASK;
+    int cmd = (s->regs[s->r_ctrl0 + fl->cs] >> CTRL_CMD_SHIFT) & CTRL_CMD_MASK;
 
     /*
      * In read mode, the default SPI command is READ (0x3). In other
@@ -393,7 +393,7 @@ static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
     if (asc->segments == aspeed_2400_spi1_segments) {
         return s->regs[s->r_ctrl0] & CTRL_AST2400_SPI_4BYTE;
     } else {
-        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->id));
+        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->cs));
     }
 }
 
@@ -401,9 +401,9 @@ static void aspeed_smc_flash_do_select(AspeedSMCFlash *fl, bool unselect)
 {
     AspeedSMCState *s = fl->controller;
 
-    trace_aspeed_smc_flash_select(fl->id, unselect ? "un" : "");
+    trace_aspeed_smc_flash_select(fl->cs, unselect ? "un" : "");
 
-    qemu_set_irq(s->cs_lines[fl->id], unselect);
+    qemu_set_irq(s->cs_lines[fl->cs], unselect);
 }
 
 static void aspeed_smc_flash_select(AspeedSMCFlash *fl)
@@ -423,11 +423,11 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     AspeedSegments seg;
 
-    asc->reg_to_segment(s, s->regs[R_SEG_ADDR0 + fl->id], &seg);
+    asc->reg_to_segment(s, s->regs[R_SEG_ADDR0 + fl->cs], &seg);
     if ((addr % seg.size) != addr) {
         aspeed_smc_error("invalid address 0x%08x for CS%d segment : "
                          "[ 0x%"HWADDR_PRIx" - 0x%"HWADDR_PRIx" ]",
-                         addr, fl->id, seg.addr, seg.addr + seg.size);
+                         addr, fl->cs, seg.addr, seg.addr + seg.size);
         addr %= seg.size;
     }
 
@@ -437,7 +437,7 @@ static uint32_t aspeed_smc_check_segment_addr(const AspeedSMCFlash *fl,
 static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
-    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
+    uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->cs];
     uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
     uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
     uint32_t dummies = ((dummy_high << 2) | dummy_low) * 8;
@@ -506,7 +506,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
         aspeed_smc_error("invalid flash mode %d", aspeed_smc_flash_mode(fl));
     }
 
-    trace_aspeed_smc_flash_read(fl->id, addr, size, ret,
+    trace_aspeed_smc_flash_read(fl->cs, addr, size, ret,
                                 aspeed_smc_flash_mode(fl));
     return ret;
 }
@@ -563,7 +563,7 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
     AspeedSMCState *s = fl->controller;
     uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
 
-    trace_aspeed_smc_do_snoop(fl->id, s->snoop_index, s->snoop_dummies,
+    trace_aspeed_smc_do_snoop(fl->cs, s->snoop_index, s->snoop_dummies,
                               (uint8_t) data & 0xff);
 
     if (s->snoop_index == SNOOP_OFF) {
@@ -616,7 +616,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
     AspeedSMCState *s = fl->controller;
     int i;
 
-    trace_aspeed_smc_flash_write(fl->id, addr, size, data,
+    trace_aspeed_smc_flash_write(fl->cs, addr, size, data,
                                  aspeed_smc_flash_mode(fl));
 
     if (!aspeed_smc_is_writable(fl)) {
@@ -668,12 +668,12 @@ static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value)
     unselect = (value & CTRL_CMD_MODE_MASK) != CTRL_USERMODE;
 
     /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */
-    if (!(s->regs[s->r_ctrl0 + fl->id] & CTRL_CE_STOP_ACTIVE) &&
+    if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) &&
         value & CTRL_CE_STOP_ACTIVE) {
         unselect = true;
     }
 
-    s->regs[s->r_ctrl0 + fl->id] = value;
+    s->regs[s->r_ctrl0 + fl->cs] = value;
 
     s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START;
 
@@ -1184,7 +1184,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
         snprintf(name, sizeof(name), TYPE_ASPEED_SMC ".flash.%d", i);
 
-        fl->id = i;
+        fl->cs = i;
         fl->controller = s;
         memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
                               fl, name, asc->segments[i].size);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (6 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs' Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  7:24   ` Philippe Mathieu-Daudé
  2021-09-07  6:58 ` [PATCH 09/10] aspeed/smc: Add default reset values Cédric Le Goater
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

AspeedSMCFlash is a little object representing the AHB memory window
through which the contents of a flash device can be accessed with
MMIOs.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h | 13 +++++--
 hw/ssi/aspeed_smc.c         | 78 +++++++++++++++++++++++++++++++++----
 2 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 40b6926b3e02..ee943228b96f 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -30,18 +30,23 @@
 #include "qom/object.h"
 
 struct AspeedSMCState;
-typedef struct AspeedSMCFlash {
-    struct AspeedSMCState *controller;
 
+#define TYPE_ASPEED_SMC_FLASH "aspeed.smc.flash"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedSMCFlash, ASPEED_SMC_FLASH)
+struct AspeedSMCFlash {
+    SysBusDevice parent_obj;
+
+    struct AspeedSMCState *controller;
     uint8_t cs;
 
     MemoryRegion mmio;
-} AspeedSMCFlash;
+};
 
 #define TYPE_ASPEED_SMC "aspeed.smc"
 OBJECT_DECLARE_TYPE(AspeedSMCState, AspeedSMCClass, ASPEED_SMC)
 
 #define ASPEED_SMC_R_MAX        (0x100 / 4)
+#define ASPEED_SMC_CS_MAX       5
 
 struct AspeedSMCState {
     SysBusDevice parent_obj;
@@ -72,7 +77,7 @@ struct AspeedSMCState {
     MemoryRegion *dram_mr;
     AddressSpace dram_as;
 
-    AspeedSMCFlash *flashes;
+    AspeedSMCFlash flashes[ASPEED_SMC_CS_MAX];
 
     uint8_t snoop_index;
     uint8_t snoop_dummies;
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index a47719946941..8e6b326f1fc9 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1101,6 +1101,18 @@ static const MemoryRegionOps aspeed_smc_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static void aspeed_smc_instance_init(Object *obj)
+{
+    AspeedSMCState *s = ASPEED_SMC(obj);
+    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
+    int i;
+
+    for (i = 0; i < asc->max_peripherals; i++) {
+        object_initialize_child(obj, "flash[*]", &s->flashes[i],
+                                TYPE_ASPEED_SMC_FLASH);
+    }
+}
+
 /*
  * Initialize the custom address spaces for DMAs
  */
@@ -1123,7 +1135,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     AspeedSMCState *s = ASPEED_SMC(dev);
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     int i;
-    char name[32];
     hwaddr offset = 0;
 
     /* keep a copy under AspeedSMCState to speed up accesses */
@@ -1170,8 +1181,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
                              &s->mmio_flash, 0, asc->flash_window_size);
     sysbus_init_mmio(sbd, &s->mmio_flash_alias);
 
-    s->flashes = g_new0(AspeedSMCFlash, asc->max_peripherals);
-
     /*
      * Let's create a sub memory region for each possible peripheral. All
      * have a configurable memory segment in the overall flash mapping
@@ -1182,12 +1191,17 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < asc->max_peripherals; ++i) {
         AspeedSMCFlash *fl = &s->flashes[i];
 
-        snprintf(name, sizeof(name), TYPE_ASPEED_SMC ".flash.%d", i);
+        if (!object_property_set_link(OBJECT(fl), "controller", OBJECT(s),
+                                      errp)) {
+            return;
+        }
+        if (!object_property_set_uint(OBJECT(fl), "cs", i, errp)) {
+            return;
+        }
+        if (!sysbus_realize(SYS_BUS_DEVICE(fl), errp)) {
+            return;
+        }
 
-        fl->cs = i;
-        fl->controller = s;
-        memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
-                              fl, name, asc->segments[i].size);
         memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
         offset += asc->segments[i].size;
     }
@@ -1231,12 +1245,59 @@ static void aspeed_smc_class_init(ObjectClass *klass, void *data)
 static const TypeInfo aspeed_smc_info = {
     .name           = TYPE_ASPEED_SMC,
     .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_init  = aspeed_smc_instance_init,
     .instance_size  = sizeof(AspeedSMCState),
     .class_size     = sizeof(AspeedSMCClass),
     .class_init     = aspeed_smc_class_init,
     .abstract       = true,
 };
 
+static void aspeed_smc_flash_realize(DeviceState *dev, Error **errp)
+{
+    AspeedSMCFlash *s = ASPEED_SMC_FLASH(dev);
+    AspeedSMCClass *asc;
+    char name[32];
+
+    if (!s->controller) {
+        error_setg(errp, TYPE_ASPEED_SMC_FLASH ": 'controller' link not set");
+        return;
+    }
+
+    asc = ASPEED_SMC_GET_CLASS(s->controller);
+
+    snprintf(name, sizeof(name), TYPE_ASPEED_SMC_FLASH ".%d", s->cs);
+
+    /*
+     * Use the default segment value to size the memory region. This
+     * can be changed by FW at runtime.
+     */
+    memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_flash_ops,
+                          s, name, asc->segments[s->cs].size);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
+}
+
+static Property aspeed_smc_flash_properties[] = {
+    DEFINE_PROP_UINT8("cs", AspeedSMCFlash, cs, 0),
+    DEFINE_PROP_LINK("controller", AspeedSMCFlash, controller, TYPE_ASPEED_SMC,
+                     AspeedSMCState *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_smc_flash_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "Aspeed SMC Flash device region";
+    dc->realize = aspeed_smc_flash_realize;
+    device_class_set_props(dc, aspeed_smc_flash_properties);
+}
+
+static const TypeInfo aspeed_smc_flash_info = {
+    .name           = TYPE_ASPEED_SMC_FLASH,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(AspeedSMCFlash),
+    .class_init     = aspeed_smc_flash_class_init,
+};
 
 /*
  * The Segment Registers of the AST2400 and AST2500 have a 8MB
@@ -1624,6 +1685,7 @@ static const TypeInfo aspeed_2600_spi2_info = {
 
 static void aspeed_smc_register_types(void)
 {
+    type_register_static(&aspeed_smc_flash_info);
     type_register_static(&aspeed_smc_info);
     type_register_static(&aspeed_2400_smc_info);
     type_register_static(&aspeed_2400_fmc_info);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 09/10] aspeed/smc: Add default reset values
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (7 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  6:58 ` [PATCH 10/10] aspeed/smc: Introduce an addr_width() class handler Cédric Le Goater
  2021-09-07  7:17 ` [PATCH 00/10] aspeed/smc: Cleanups and QOMification Joel Stanley
  10 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

This simplifies the reset handler and has the benefit to remove some
bad use of the segments array as an identifier of the controller model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  1 +
 hw/ssi/aspeed_smc.c         | 52 +++++++++++++++++++------------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index ee943228b96f..a1ca0e65c405 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -98,6 +98,7 @@ struct AspeedSMCClass {
     uint8_t nregs_timings;
     uint8_t conf_enable_w0;
     uint8_t max_peripherals;
+    const uint32_t *resets;
     const AspeedSegments *segments;
     hwaddr flash_window_base;
     uint32_t flash_window_size;
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 8e6b326f1fc9..59b62987db21 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -196,12 +196,9 @@
  * controller. These can be changed when board is initialized with the
  * Segment Address Registers.
  */
-static const AspeedSegments aspeed_2400_fmc_segments[];
 static const AspeedSegments aspeed_2400_spi1_segments[];
-static const AspeedSegments aspeed_2500_fmc_segments[];
 static const AspeedSegments aspeed_2500_spi1_segments[];
 static const AspeedSegments aspeed_2500_spi2_segments[];
-static const AspeedSegments aspeed_2600_fmc_segments[];
 
 #define ASPEED_SMC_FEATURE_DMA       0x1
 #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
@@ -686,7 +683,11 @@ static void aspeed_smc_reset(DeviceState *d)
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
     int i;
 
-    memset(s->regs, 0, sizeof s->regs);
+    if (asc->resets) {
+        memcpy(s->regs, asc->resets, sizeof s->regs);
+    } else {
+        memset(s->regs, 0, sizeof s->regs);
+    }
 
     /* Unselect all peripherals */
     for (i = 0; i < s->num_cs; ++i) {
@@ -700,27 +701,6 @@ static void aspeed_smc_reset(DeviceState *d)
                     asc->segment_to_reg(s, &asc->segments[i]));
     }
 
-    /* HW strapping flash type for the AST2600 controllers  */
-    if (asc->segments == aspeed_2600_fmc_segments) {
-        /* flash type is fixed to SPI for all */
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE2);
-    }
-
-    /* HW strapping flash type for FMC controllers  */
-    if (asc->segments == aspeed_2500_fmc_segments) {
-        /* flash type is fixed to SPI for CE0 and CE1 */
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
-    }
-
-    /* HW strapping for AST2400 FMC controllers (SCU70). Let's use the
-     * configuration of the palmetto-bmc machine */
-    if (asc->segments == aspeed_2400_fmc_segments) {
-        s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
-    }
-
     s->snoop_index = SNOOP_OFF;
     s->snoop_dummies = 0;
 }
@@ -1354,6 +1334,14 @@ static const TypeInfo aspeed_2400_smc_info = {
     .class_init = aspeed_2400_smc_class_init,
 };
 
+static const uint32_t aspeed_2400_fmc_resets[ASPEED_SMC_R_MAX] = {
+    /*
+     * CE0 and CE1 types are HW strapped in SCU70. Do it here to
+     * simplify the model.
+     */
+    [R_CONF] = CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0,
+};
+
 static const AspeedSegments aspeed_2400_fmc_segments[] = {
     { 0x20000000, 64 * MiB }, /* start address is readonly */
     { 0x24000000, 32 * MiB },
@@ -1376,6 +1364,7 @@ static void aspeed_2400_fmc_class_init(ObjectClass *klass, void *data)
     asc->conf_enable_w0    = CONF_ENABLE_W0;
     asc->max_peripherals   = 5;
     asc->segments          = aspeed_2400_fmc_segments;
+    asc->resets            = aspeed_2400_fmc_resets;
     asc->flash_window_base = 0x20000000;
     asc->flash_window_size = 0x10000000;
     asc->features          = ASPEED_SMC_FEATURE_DMA;
@@ -1426,6 +1415,11 @@ static const TypeInfo aspeed_2400_spi1_info = {
     .class_init = aspeed_2400_spi1_class_init,
 };
 
+static const uint32_t aspeed_2500_fmc_resets[ASPEED_SMC_R_MAX] = {
+    [R_CONF] = (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0 |
+                CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1),
+};
+
 static const AspeedSegments aspeed_2500_fmc_segments[] = {
     { 0x20000000, 128 * MiB }, /* start address is readonly */
     { 0x28000000,  32 * MiB },
@@ -1446,6 +1440,7 @@ static void aspeed_2500_fmc_class_init(ObjectClass *klass, void *data)
     asc->conf_enable_w0    = CONF_ENABLE_W0;
     asc->max_peripherals   = 3;
     asc->segments          = aspeed_2500_fmc_segments;
+    asc->resets            = aspeed_2500_fmc_resets;
     asc->flash_window_base = 0x20000000;
     asc->flash_window_size = 0x10000000;
     asc->features          = ASPEED_SMC_FEATURE_DMA;
@@ -1571,6 +1566,12 @@ static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
     }
 }
 
+static const uint32_t aspeed_2600_fmc_resets[ASPEED_SMC_R_MAX] = {
+    [R_CONF] = (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0 |
+                CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1 |
+                CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE2),
+};
+
 static const AspeedSegments aspeed_2600_fmc_segments[] = {
     { 0x0, 128 * MiB }, /* start address is readonly */
     { 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
@@ -1591,6 +1592,7 @@ static void aspeed_2600_fmc_class_init(ObjectClass *klass, void *data)
     asc->conf_enable_w0    = CONF_ENABLE_W0;
     asc->max_peripherals   = 3;
     asc->segments          = aspeed_2600_fmc_segments;
+    asc->resets            = aspeed_2600_fmc_resets;
     asc->flash_window_base = 0x20000000;
     asc->flash_window_size = 0x10000000;
     asc->features          = ASPEED_SMC_FEATURE_DMA;
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 10/10] aspeed/smc: Introduce an addr_width() class handler
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (8 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 09/10] aspeed/smc: Add default reset values Cédric Le Goater
@ 2021-09-07  6:58 ` Cédric Le Goater
  2021-09-07  7:17 ` [PATCH 00/10] aspeed/smc: Cleanups and QOMification Joel Stanley
  10 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  6:58 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Cédric Le Goater

The AST2400 SPI controller has a transitional HW interface and it
stores the address width currently in use in a different register than
all the other SMC controllers. It needs special handling when working
in 4B mode.

Make it clear through a class handler. This also removes another use
of the segments array.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |  1 +
 hw/ssi/aspeed_smc.c         | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index a1ca0e65c405..8dc81294988e 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -111,6 +111,7 @@ struct AspeedSMCClass {
     void (*reg_to_segment)(const AspeedSMCState *s, uint32_t reg,
                            AspeedSegments *seg);
     void (*dma_ctrl)(AspeedSMCState *s, uint32_t value);
+    int (*addr_width)(const AspeedSMCState *s);
 };
 
 #endif /* ASPEED_SMC_H */
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 59b62987db21..da4222b96dc4 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -196,7 +196,6 @@
  * controller. These can be changed when board is initialized with the
  * Segment Address Registers.
  */
-static const AspeedSegments aspeed_2400_spi1_segments[];
 static const AspeedSegments aspeed_2500_spi1_segments[];
 static const AspeedSegments aspeed_2500_spi2_segments[];
 
@@ -382,15 +381,15 @@ static inline int aspeed_smc_flash_cmd(const AspeedSMCFlash *fl)
     return cmd;
 }
 
-static inline int aspeed_smc_flash_is_4byte(const AspeedSMCFlash *fl)
+static inline int aspeed_smc_flash_addr_width(const AspeedSMCFlash *fl)
 {
     const AspeedSMCState *s = fl->controller;
     AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
 
-    if (asc->segments == aspeed_2400_spi1_segments) {
-        return s->regs[s->r_ctrl0] & CTRL_AST2400_SPI_4BYTE;
+    if (asc->addr_width) {
+        return asc->addr_width(s);
     } else {
-        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->cs));
+        return s->regs[s->r_ce_ctrl] & (1 << (CTRL_EXTENDED0 + fl->cs)) ? 4 : 3;
     }
 }
 
@@ -450,7 +449,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
 {
     const AspeedSMCState *s = fl->controller;
     uint8_t cmd = aspeed_smc_flash_cmd(fl);
-    int i = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
+    int i = aspeed_smc_flash_addr_width(fl);
 
     /* Flash access can not exceed CS segment */
     addr = aspeed_smc_check_segment_addr(fl, addr);
@@ -558,7 +557,7 @@ static bool aspeed_smc_do_snoop(AspeedSMCFlash *fl,  uint64_t data,
                                 unsigned size)
 {
     AspeedSMCState *s = fl->controller;
-    uint8_t addr_width = aspeed_smc_flash_is_4byte(fl) ? 4 : 3;
+    uint8_t addr_width = aspeed_smc_flash_addr_width(fl);
 
     trace_aspeed_smc_do_snoop(fl->cs, s->snoop_index, s->snoop_dummies,
                               (uint8_t) data & 0xff);
@@ -1386,6 +1385,11 @@ static const AspeedSegments aspeed_2400_spi1_segments[] = {
     { 0x30000000, 64 * MiB },
 };
 
+static int aspeed_2400_spi1_addr_width(const AspeedSMCState *s)
+{
+    return s->regs[R_SPI_CTRL0] & CTRL_AST2400_SPI_4BYTE ? 4 : 3;
+}
+
 static void aspeed_2400_spi1_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1407,6 +1411,7 @@ static void aspeed_2400_spi1_class_init(ObjectClass *klass, void *data)
     asc->segment_to_reg    = aspeed_smc_segment_to_reg;
     asc->reg_to_segment    = aspeed_smc_reg_to_segment;
     asc->dma_ctrl          = aspeed_smc_dma_ctrl;
+    asc->addr_width        = aspeed_2400_spi1_addr_width;
 }
 
 static const TypeInfo aspeed_2400_spi1_info = {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 00/10] aspeed/smc: Cleanups and QOMification
  2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
                   ` (9 preceding siblings ...)
  2021-09-07  6:58 ` [PATCH 10/10] aspeed/smc: Introduce an addr_width() class handler Cédric Le Goater
@ 2021-09-07  7:17 ` Joel Stanley
  10 siblings, 0 replies; 22+ messages in thread
From: Joel Stanley @ 2021-09-07  7:17 UTC (permalink / raw)
  To: Cédric Le Goater
  Cc: Andrew Jeffery, Peter Maydell, Alistair Francis, QEMU Developers,
	qemu-arm

Hi Cédric,

On Tue, 7 Sept 2021 at 06:58, Cédric Le Goater <clg@kaod.org> wrote:
> This series adds a simple support for the 2nd watchdog registers which
> were moved under the AST2600 FMC controller to deactivate the
> alternate boot function. Then come cleanups of the AspeedSMC model
> mostly removing a useless AspeedSMCController structure (that's the
> noisy part) and QOMifying the AspeedSMCFlash structure.

I had a read through and it makes sense to me.

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs'
  2021-09-07  6:58 ` [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs' Cédric Le Goater
@ 2021-09-07  7:17   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07  7:17 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> 'cs' is a more appropriate name to index SPI flash devices.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h |  2 +-
>  hw/ssi/aspeed_smc.c         | 30 +++++++++++++++---------------
>  2 files changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] aspeed/smc: Remove the 'size' attribute from AspeedSMCFlash
  2021-09-07  6:58 ` [PATCH 06/10] aspeed/smc: Remove the 'size' " Cédric Le Goater
@ 2021-09-07  7:20   ` Philippe Mathieu-Daudé
  2021-09-07  7:34     ` Cédric Le Goater
  0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07  7:20 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> It's only useful to compute the initial size of the boot_rom region.

"AspeedSMCFlash::size is only used to compute the initial size
 of the boot_rom region. Not very useful, so directly call
 memory_region_size() instead."?

> Use memory_region_size() instead.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h | 1 -
>  hw/arm/aspeed.c             | 7 ++++---
>  hw/ssi/aspeed_smc.c         | 5 ++---
>  3 files changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash
  2021-09-07  6:58 ` [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash Cédric Le Goater
@ 2021-09-07  7:24   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07  7:24 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> AspeedSMCFlash is a little object representing the AHB memory window
> through which the contents of a flash device can be accessed with
> MMIOs.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h | 13 +++++--
>  hw/ssi/aspeed_smc.c         | 78 +++++++++++++++++++++++++++++++++----
>  2 files changed, 79 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure
  2021-09-07  6:58 ` [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure Cédric Le Goater
@ 2021-09-07  7:28   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07  7:28 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> The characteristics of the Aspeed controllers are described in a
> AspeedSMCController structure which is redundant with the
> AspeedSMCClass. Move all attributes under the class and adapt the code
> to use class attributes instead.
> 
> This is a large change but it is functionally equivalent.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h |  64 ++-
>  hw/arm/aspeed_ast2600.c     |   4 +-
>  hw/arm/aspeed_soc.c         |   4 +-
>  hw/ssi/aspeed_smc.c         | 859 ++++++++++++++++++++----------------
>  4 files changed, 510 insertions(+), 421 deletions(-)

Overall LGTM, I might do a more detailed pass later.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 06/10] aspeed/smc: Remove the 'size' attribute from AspeedSMCFlash
  2021-09-07  7:20   ` Philippe Mathieu-Daudé
@ 2021-09-07  7:34     ` Cédric Le Goater
  0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  7:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 9:20 AM, Philippe Mathieu-Daudé wrote:
> On 9/7/21 8:58 AM, Cédric Le Goater wrote:
>> It's only useful to compute the initial size of the boot_rom region.
> 
> "AspeedSMCFlash::size is only used to compute the initial size
>  of the boot_rom region. Not very useful, so directly call
>  memory_region_size() instead."?

Sure. I will rephrase.

Thanks,

C. 


> 
>> Use memory_region_size() instead.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ssi/aspeed_smc.h | 1 -
>>  hw/arm/aspeed.c             | 7 ++++---
>>  hw/ssi/aspeed_smc.c         | 5 ++---
>>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash
  2021-09-07  6:58 ` [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash Cédric Le Goater
@ 2021-09-07  8:36   ` Philippe Mathieu-Daudé
  2021-09-07  9:40     ` Cédric Le Goater
  0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07  8:36 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 8:58 AM, Cédric Le Goater wrote:
> There is no use for it.

Hmmm this is not the correct justification.

This devices sits on a bus, so its state will be released when
the bus is released. There is no need to release it manually,
so we can remove the reference.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/aspeed_smc.h |  1 -
>  hw/arm/aspeed.c             | 11 +++++------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index 0ea536a44c3a..f32f66f9a838 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
>      uint32_t size;
>  
>      MemoryRegion mmio;
> -    DeviceState *flash;
>  } AspeedSMCFlash;
>  
>  #define TYPE_ASPEED_SMC "aspeed.smc"
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 95ce4b1670ac..64c3a7fb66db 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
>      int i ;
>  
>      for (i = 0; i < s->num_cs; ++i) {
> -        AspeedSMCFlash *fl = &s->flashes[i];
>          DriveInfo *dinfo = drive_get_next(IF_MTD);
>          qemu_irq cs_line;
> +        DeviceState *dev;
>  
> -        fl->flash = qdev_new(flashtype);
> +        dev = qdev_new(flashtype);
>          if (dinfo) {
> -            qdev_prop_set_drive(fl->flash, "drive",
> -                                blk_by_legacy_dinfo(dinfo));
> +            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>          }
> -        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
> +        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>  
> -        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
> +        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>          sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
>      }
>  }
> 



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash
  2021-09-07  8:36   ` Philippe Mathieu-Daudé
@ 2021-09-07  9:40     ` Cédric Le Goater
  2021-09-07 10:21       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-07  9:40 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm

On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote:
> On 9/7/21 8:58 AM, Cédric Le Goater wrote:
>> There is no use for it.
> 
> Hmmm this is not the correct justification.
> 
> This devices sits on a bus, so its state will be released when
> the bus is released. There is no need to release it manually,
> so we can remove the reference.

That's what the code is doing AFAIUI.

This is just removing the AspeedSMCFlash attribute because it is 
not used anywhere else than under aspeed_board_init_flashes().
 
Is there anything else ? I am bit lost by your comment.

Thanks,

C.

> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ssi/aspeed_smc.h |  1 -
>>  hw/arm/aspeed.c             | 11 +++++------
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 0ea536a44c3a..f32f66f9a838 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -37,7 +37,6 @@ typedef struct AspeedSMCFlash {
>>      uint32_t size;
>>  
>>      MemoryRegion mmio;
>> -    DeviceState *flash;
>>  } AspeedSMCFlash;
>>  
>>  #define TYPE_ASPEED_SMC "aspeed.smc"
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 95ce4b1670ac..64c3a7fb66db 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -274,18 +274,17 @@ static void aspeed_board_init_flashes(AspeedSMCState *s,
>>      int i ;
>>  
>>      for (i = 0; i < s->num_cs; ++i) {
>> -        AspeedSMCFlash *fl = &s->flashes[i];
>>          DriveInfo *dinfo = drive_get_next(IF_MTD);
>>          qemu_irq cs_line;
>> +        DeviceState *dev;
>>  
>> -        fl->flash = qdev_new(flashtype);
>> +        dev = qdev_new(flashtype);
>>          if (dinfo) {
>> -            qdev_prop_set_drive(fl->flash, "drive",
>> -                                blk_by_legacy_dinfo(dinfo));
>> +            qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>>          }
>> -        qdev_realize_and_unref(fl->flash, BUS(s->spi), &error_fatal);
>> +        qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>>  
>> -        cs_line = qdev_get_gpio_in_named(fl->flash, SSI_GPIO_CS, 0);
>> +        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>>          sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
>>      }
>>  }
>>
> 



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash
  2021-09-07  9:40     ` Cédric Le Goater
@ 2021-09-07 10:21       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07 10:21 UTC (permalink / raw)
  To: Cédric Le Goater, Peter Maydell, Andrew Jeffery, Joel Stanley
  Cc: Alistair Francis, qemu-devel, qemu-arm, Markus Armbruster

On 9/7/21 11:40 AM, Cédric Le Goater wrote:
> On 9/7/21 10:36 AM, Philippe Mathieu-Daudé wrote:
>> On 9/7/21 8:58 AM, Cédric Le Goater wrote:
>>> There is no use for it.
>>
>> Hmmm this is not the correct justification.
>>
>> This devices sits on a bus, so its state will be released when
>> the bus is released. There is no need to release it manually,
>> so we can remove the reference.
> 
> That's what the code is doing AFAIUI.
> 
> This is just removing the AspeedSMCFlash attribute because it is 
> not used anywhere else than under aspeed_board_init_flashes().
>  
> Is there anything else ? I am bit lost by your comment.

I was thinking of d4e1d8f57eb ("hw/arm/tosa: Encapsulate misc
GPIO handling in a device"), if the device were not created on
a bus, the we'd need to keep this reference, otherwise we'd
leak it.

Anyhow this is board code where we are not releasing anything.

Maybe "There is no need to keep a reference of the flash qdev in the
AspeedSMCFlash state: the SPI bus takes ownership and will release
its resources. Remove AspeedSMCFlash::flash."?

Anyway no big deal with the comment,
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
  2021-09-07  6:58 ` [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers Cédric Le Goater
@ 2021-09-08 18:46   ` Peter Delevoryas
  2021-09-09 12:33     ` Cédric Le Goater
  0 siblings, 1 reply; 22+ messages in thread
From: Peter Delevoryas @ 2021-09-08 18:46 UTC (permalink / raw)
  To: Cédric Le Goater
  Cc: Peter Maydell, Andrew Jeffery, Joel Stanley, Alistair Francis,
	qemu-arm, Cameron Esfahani via



> 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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers
  2021-09-08 18:46   ` Peter Delevoryas
@ 2021-09-09 12:33     ` Cédric Le Goater
  0 siblings, 0 replies; 22+ messages in thread
From: Cédric Le Goater @ 2021-09-09 12:33 UTC (permalink / raw)
  To: Peter Delevoryas
  Cc: Peter Maydell, Andrew Jeffery, Alistair Francis,
	Cameron Esfahani via, qemu-arm, Joel Stanley

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.


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-09-09 12:35 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07  6:58 [PATCH 00/10] aspeed/smc: Cleanups and QOMification Cédric Le Goater
2021-09-07  6:58 ` [PATCH 01/10] aspeed/smc: Add watchdog Control/Status Registers Cédric Le Goater
2021-09-08 18:46   ` Peter Delevoryas
2021-09-09 12:33     ` Cédric Le Goater
2021-09-07  6:58 ` [PATCH 02/10] aspeed/smc: Introduce aspeed_smc_error() helper Cédric Le Goater
2021-09-07  6:58 ` [PATCH 03/10] aspeed/smc: Stop using the model name for the memory regions Cédric Le Goater
2021-09-07  6:58 ` [PATCH 04/10] aspeed/smc: Drop AspeedSMCController structure Cédric Le Goater
2021-09-07  7:28   ` Philippe Mathieu-Daudé
2021-09-07  6:58 ` [PATCH 05/10] aspeed/smc: Remove the 'flash' attribute from AspeedSMCFlash Cédric Le Goater
2021-09-07  8:36   ` Philippe Mathieu-Daudé
2021-09-07  9:40     ` Cédric Le Goater
2021-09-07 10:21       ` Philippe Mathieu-Daudé
2021-09-07  6:58 ` [PATCH 06/10] aspeed/smc: Remove the 'size' " Cédric Le Goater
2021-09-07  7:20   ` Philippe Mathieu-Daudé
2021-09-07  7:34     ` Cédric Le Goater
2021-09-07  6:58 ` [PATCH 07/10] aspeed/smc: Rename AspeedSMCFlash 'id' to 'cs' Cédric Le Goater
2021-09-07  7:17   ` Philippe Mathieu-Daudé
2021-09-07  6:58 ` [PATCH 08/10] aspeed/smc: QOMify AspeedSMCFlash Cédric Le Goater
2021-09-07  7:24   ` Philippe Mathieu-Daudé
2021-09-07  6:58 ` [PATCH 09/10] aspeed/smc: Add default reset values Cédric Le Goater
2021-09-07  6:58 ` [PATCH 10/10] aspeed/smc: Introduce an addr_width() class handler Cédric Le Goater
2021-09-07  7:17 ` [PATCH 00/10] aspeed/smc: Cleanups and QOMification Joel Stanley

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.