[PATCH 00/12] aspeed: fixes and extensions

[PATCH 00/12] aspeed: fixes and extensions

[Cédric Le Goater]

Hello,

This series fixes issues spotted by Coverity and adds a couple of
improvements for the machine definition.

The first is to offer the capability to define all CS of all SPI
controllers without introducing new machine types, using a blockdev on
the command line :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0

instead of using drives which relies on the command line order.
Ultimately, we will get rid of drive_get(IF_MTD, ...) but we are not
there yet. For this, SSIPeripheral is extended with an "addr"
property.

A second extension is the introduction of a "uart" machine option to
let the user define the default UART device of the machine from the
QEMU command line :

    -M ast2500-evb,uart=uart3

Last, a new "vfp-d32" CPU property is added to ARM CPUs to model FPUs
implementing VFPv4 without NEON support and with 16 64-bit FPU
registers (and not 32 registers). This is the case for the Cortex A7
of the Aspeed AST2600 SoC. I hope I got it right this time.

Thanks,

C.

Cédric Le Goater (12):
  aspeed/hace: Initialize g_autofree pointer
  aspeed: Introduce a boot_rom region at the machine level
  aspeed: Use the boot_rom region of the fby35 machine
  hw/ssi: Add an "addr" property to SSIPeripheral
  hw/ssi: Introduce a ssi_get_cs() helper
  aspeed/smc: Wire CS lines at reset
  hw/ssi: Check for duplicate addresses
  aspeed: Create flash devices only when defaults are enabled
  m25p80: Introduce an helper to retrieve the BlockBackend of a device
  aspeed: Get the BlockBackend of FMC0 from the flash device
  aspeed: Introduce a "uart" machine option
  target/arm: Allow users to set the number of VFP registers

 docs/system/arm/aspeed.rst          | 10 +++++
 include/hw/block/flash.h            |  4 ++
 include/hw/ssi/ssi.h                |  5 +++
 target/arm/cpu.h                    |  2 +
 hw/arm/aspeed.c                     | 68 ++++++++++++++++++++++-------
 hw/arm/aspeed_ast2600.c             |  2 +
 hw/arm/fby35.c                      | 29 ++++++------
 hw/arm/stellaris.c                  |  4 +-
 hw/arm/xilinx_zynq.c                |  1 +
 hw/arm/xlnx-versal-virt.c           |  1 +
 hw/arm/xlnx-zcu102.c                |  2 +
 hw/block/m25p80.c                   |  6 +++
 hw/microblaze/petalogix_ml605_mmu.c |  1 +
 hw/misc/aspeed_hace.c               |  2 +-
 hw/ssi/aspeed_smc.c                 |  8 ++++
 hw/ssi/ssi.c                        | 42 ++++++++++++++++++
 target/arm/cpu.c                    | 32 ++++++++++++++
 17 files changed, 187 insertions(+), 32 deletions(-)

-- 
2.40.0

[PATCH 01/12] aspeed/hace: Initialize g_autofree pointer

[Cédric Le Goater]

As mentioned in docs/devel/style.rst "Automatic memory deallocation":

* Variables declared with g_auto* MUST always be initialized,
  otherwise the cleanup function will use uninitialized stack memory

This avoids QEMU to coredump when running the "hash test" command
under Zephyr.

Cc: Steven Lee <steven_lee@aspeedtech.com>
Cc: Joel Stanley <joel@jms.id.au>
Fixes: c5475b3f9a ("hw: Model ASPEED's Hash and Crypto Engine")
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20230421131547.2177449-1-clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_hace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 12a761f1f5..b07506ec04 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -189,7 +189,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
                               bool acc_mode)
 {
     struct iovec iov[ASPEED_HACE_MAX_SG];
-    g_autofree uint8_t *digest_buf;
+    g_autofree uint8_t *digest_buf = NULL;
     size_t digest_len = 0;
     int niov = 0;
     int i;
-- 
2.40.0

[PATCH 02/12] aspeed: Introduce a boot_rom region at the machine level

[Cédric Le Goater]

This should also avoid Coverity to report a memory leak warning when
the QEMU process exits. See CID 1508061.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0b29028fe1..b654513f35 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -40,6 +40,7 @@ struct AspeedMachineState {
     /* Public */
 
     AspeedSoCState soc;
+    MemoryRegion boot_rom;
     bool mmio_exec;
     char *fmc_model;
     char *spi_model;
@@ -275,15 +276,15 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
  * Create a ROM and copy the flash contents at the expected address
  * (0x0). Boots faster than execute-in-place.
  */
-static void aspeed_install_boot_rom(AspeedSoCState *soc, BlockBackend *blk,
+static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
                                     uint64_t rom_size)
 {
-    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+    AspeedSoCState *soc = &bmc->soc;
 
-    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
+    memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
                            &error_abort);
     memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
-                                        boot_rom, 1);
+                                        &bmc->boot_rom, 1);
     write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
 }
 
@@ -431,8 +432,7 @@ static void aspeed_machine_init(MachineState *machine)
 
         if (mtd0) {
             uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
-            aspeed_install_boot_rom(&bmc->soc, blk_by_legacy_dinfo(mtd0),
-                                    rom_size);
+            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
         }
     }
 
-- 
2.40.0

[PATCH 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Cédric Le Goater]

This change completes commits 5aa281d757 ("aspeed: Introduce a
spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
boot_rom overlap region in the SoC spi_boot container") which
introduced a spi_boot container at the SoC level to map the boot rom
region as an overlap.

It also fixes a Coverity report (CID 1508061) for a memory leak
warning when the QEMU process exits by using an bmc_boot_rom
MemoryRegion available at the machine level.

Cc: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/fby35.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index f4600c290b..f2ff6c1abf 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -70,8 +70,6 @@ static void fby35_bmc_write_boot_rom(DriveInfo *dinfo, MemoryRegion *mr,
 
 static void fby35_bmc_init(Fby35State *s)
 {
-    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
-
     object_initialize_child(OBJECT(s), "bmc", &s->bmc, "ast2600-a3");
 
     memory_region_init(&s->bmc_memory, OBJECT(&s->bmc), "bmc-memory",
@@ -95,18 +93,21 @@ static void fby35_bmc_init(Fby35State *s)
     aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
 
     /* Install first FMC flash content as a boot rom. */
-    if (drive0) {
-        AspeedSMCFlash *fl = &s->bmc.fmc.flashes[0];
-        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
-        uint64_t size = memory_region_size(&fl->mmio);
-
-        if (!s->mmio_exec) {
-            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
-                                   size, &error_abort);
-            memory_region_add_subregion(&s->bmc_memory, FBY35_BMC_FIRMWARE_ADDR,
-                                        boot_rom);
-            fby35_bmc_write_boot_rom(drive0, boot_rom, FBY35_BMC_FIRMWARE_ADDR,
-                                     size, &error_abort);
+    if (!s->mmio_exec) {
+        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
+
+        if (mtd0) {
+            AspeedSoCState *bmc = &s->bmc;
+            uint64_t rom_size = memory_region_size(&bmc->spi_boot);
+
+            memory_region_init_rom(&s->bmc_boot_rom, NULL, "aspeed.boot_rom",
+                                   rom_size, &error_abort);
+            memory_region_add_subregion_overlap(&bmc->spi_boot_container, 0,
+                                                &s->bmc_boot_rom, 1);
+
+            fby35_bmc_write_boot_rom(mtd0, &s->bmc_boot_rom,
+                                     FBY35_BMC_FIRMWARE_ADDR,
+                                     rom_size, &error_abort);
         }
     }
 }
-- 
2.40.0

[PATCH 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Cédric Le Goater]

Boards will use this new property to identify the device CS line and
wire the SPI controllers accordingly.

Cc: Alistair Francis <alistair@alistair23.me>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/ssi.h | 3 +++
 hw/ssi/ssi.c         | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 6950f86810..ffd3a34ba4 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -64,6 +64,9 @@ struct SSIPeripheral {
 
     /* Chip select state */
     bool cs;
+
+    /* Chip select address/index */
+    uint32_t addr;
 };
 
 extern const VMStateDescription vmstate_ssi_peripheral;
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index d54a109bee..9fffe4f27a 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
 #include "hw/ssi/ssi.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
@@ -71,6 +72,11 @@ static void ssi_peripheral_realize(DeviceState *dev, Error **errp)
     ssc->realize(s, errp);
 }
 
+static Property ssi_peripheral_properties[] = {
+    DEFINE_PROP_UINT32("addr", SSIPeripheral, addr, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
 {
     SSIPeripheralClass *ssc = SSI_PERIPHERAL_CLASS(klass);
@@ -81,6 +87,7 @@ static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
     if (!ssc->transfer_raw) {
         ssc->transfer_raw = ssi_transfer_raw_default;
     }
+    device_class_set_props(dc, ssi_peripheral_properties);
 }
 
 static const TypeInfo ssi_peripheral_info = {
-- 
2.40.0

[PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

Simple routine to retrieve a DeviceState object on a SPI bus using its
address/cs. It will be useful for the board to wire the CS lines.

Cc: Alistair Francis <alistair@alistair23.me>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/ssi.h |  2 ++
 hw/ssi/ssi.c         | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index ffd3a34ba4..c7beabdb09 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
 
 uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
 
+DeviceState *ssi_get_cs(SSIBus *bus, int addr);
+
 #endif
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 9fffe4f27a..a25e064417 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -27,6 +27,21 @@ struct SSIBus {
 #define TYPE_SSI_BUS "SSI"
 OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
 
+DeviceState *ssi_get_cs(SSIBus *bus, int addr)
+{
+    BusState *b = BUS(bus);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &b->children, sibling) {
+        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
+        if (kid_ssi->addr == addr) {
+            return kid->child;
+        }
+    }
+
+    return NULL;
+}
+
 static const TypeInfo ssi_bus_info = {
     .name = TYPE_SSI_BUS,
     .parent = TYPE_BUS,
-- 
2.40.0

[PATCH 06/12] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

Currently, a set of default flash devices is created at machine init
and drives defined on the QEMU command line are associated to the FMC
and SPI controllers in sequence :

   -drive file<file>,format=raw,if=mtd
   -drive file<file1>,format=raw,if=mtd

The CS lines are wired in the same creation loop. This makes a strong
assumption on the ordering and is not very flexible since only a
limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
which is less than what the SoC really supports.

A better alternative would be to define the flash devices on the
command line using a blockdev attached to a CS line of a SSI bus :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0

However, user created flash devices are not correctly wired to their
SPI controller and consequently can not be used by the machine. Fix
that and wire the CS lines of all available devices when the SSI bus
is reset.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index b654513f35..9a15899cbc 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
 
     for (i = 0; i < count; ++i) {
         DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
-        qemu_irq cs_line;
         DeviceState *dev;
 
         dev = qdev_new(flashtype);
         if (dinfo) {
             qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
         }
+        qdev_prop_set_uint32(dev, "addr", i);
         qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
-
-        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
-        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
     }
 }
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 7281169322..2a4001b774 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
         memset(s->regs, 0, sizeof s->regs);
     }
 
+    for (i = 0; i < asc->cs_num_max; i++) {
+        DeviceState *dev = ssi_get_cs(s->spi, i);
+        if (dev) {
+            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
+            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
+        }
+    }
+
     /* Unselect all peripherals */
     for (i = 0; i < asc->cs_num_max; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
-- 
2.40.0

[PATCH 07/12] hw/ssi: Check for duplicate addresses

[Cédric Le Goater]

This to avoid address conflicts on the same SSI bus. Adapt machines
using multiple devices on the same bus to avoid breakage.

Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/stellaris.c                  |  4 +++-
 hw/arm/xilinx_zynq.c                |  1 +
 hw/arm/xlnx-versal-virt.c           |  1 +
 hw/arm/xlnx-zcu102.c                |  2 ++
 hw/microblaze/petalogix_ml605_mmu.c |  1 +
 hw/ssi/ssi.c                        | 20 ++++++++++++++++++++
 6 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index f7e99baf62..ffa5999a1d 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1242,7 +1242,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
                                    qdev_get_child_bus(sddev, "sd-bus"),
                                    &error_fatal);
 
-            ssddev = ssi_create_peripheral(bus, "ssd0323");
+            ssddev = qdev_new("ssd0323");
+            qdev_prop_set_uint32(ssddev, "addr", 1);
+            qdev_realize_and_unref(ssddev, bus, &error_fatal);
 
             gpio_d_splitter = qdev_new(TYPE_SPLIT_IRQ);
             qdev_prop_set_uint32(gpio_d_splitter, "num-lines", 2);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8d..91718c5267 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -164,6 +164,7 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
                                         blk_by_legacy_dinfo(dinfo),
                                         &error_fatal);
             }
+            qdev_prop_set_uint32(flash_dev, "addr", j);
             qdev_realize_and_unref(flash_dev, BUS(spi), &error_fatal);
 
             cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 668a9d65a4..ac2ad3fd0d 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -701,6 +701,7 @@ static void versal_virt_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint32(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 4c84bb932a..70b4e4b320 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -201,6 +201,7 @@ static void xlnx_zcu102_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint32(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
@@ -224,6 +225,7 @@ static void xlnx_zcu102_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint32(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index a24fadddca..0ef5b3a02b 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -192,6 +192,7 @@ petalogix_ml605_init(MachineState *machine)
                                         blk_by_legacy_dinfo(dinfo),
                                         &error_fatal);
             }
+            qdev_prop_set_uint32(dev, "addr", i);
             qdev_realize_and_unref(dev, BUS(spi), &error_fatal);
 
             cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index a25e064417..685b7678e0 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -42,10 +42,30 @@ DeviceState *ssi_get_cs(SSIBus *bus, int addr)
     return NULL;
 }
 
+static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error **errp)
+{
+    SSIPeripheral *s = SSI_PERIPHERAL(dev);
+
+    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
+        error_setg(errp, "addr '0x%x' already in use", s->addr);
+        return false;
+    }
+
+    return true;
+}
+
+static void ssi_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+
+    k->check_address = ssi_bus_check_address;
+}
+
 static const TypeInfo ssi_bus_info = {
     .name = TYPE_SSI_BUS,
     .parent = TYPE_BUS,
     .instance_size = sizeof(SSIBus),
+    .class_init = ssi_bus_class_init,
 };
 
 static void ssi_cs_default(void *opaque, int n, int level)
-- 
2.40.0

[PATCH 08/12] aspeed: Create flash devices only when defaults are enabled

[Cédric Le Goater]

When the -nodefaults option is set, flash devices should be created
with :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img \
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 \

To be noted that in this case, the ROM will not be installed and the
initial boot sequence (U-Boot loading) will fetch instructions using
SPI transactions which is significantly slower. That's exactly how HW
operates though.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 9a15899cbc..a8832c0072 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -387,12 +387,14 @@ static void aspeed_machine_init(MachineState *machine)
     connect_serial_hds_to_uarts(bmc);
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
-    aspeed_board_init_flashes(&bmc->soc.fmc,
+    if (defaults_enabled()) {
+        aspeed_board_init_flashes(&bmc->soc.fmc,
                               bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
                               amc->num_cs, 0);
-    aspeed_board_init_flashes(&bmc->soc.spi[0],
+        aspeed_board_init_flashes(&bmc->soc.spi[0],
                               bmc->spi_model ? bmc->spi_model : amc->spi_model,
                               1, amc->num_cs);
+    }
 
     if (machine->kernel_filename && sc->num_cpus > 1) {
         /* With no u-boot we must set up a boot stub for the secondary CPU */
-- 
2.40.0

[PATCH 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Cédric Le Goater]

It will help in getting rid of some drive_get(IF_MTD) calls by
retrieving the BlockBackend directly from the m25p80 device.

Cc: Alistair Francis <alistair@alistair23.me>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/block/flash.h | 4 ++++
 hw/block/m25p80.c        | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 7198953702..de93756cbe 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
 void ecc_reset(ECCState *s);
 extern const VMStateDescription vmstate_ecc_state;
 
+/* m25p80.c */
+
+BlockBackend *m25p80_get_blk(DeviceState *dev);
+
 #endif
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index dc5ffbc4ff..afc3fdf4d6 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -25,6 +25,7 @@
 #include "qemu/units.h"
 #include "sysemu/block-backend.h"
 #include "hw/block/block.h"
+#include "hw/block/flash.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/ssi/ssi.h"
@@ -1830,3 +1831,8 @@ static void m25p80_register_types(void)
 }
 
 type_init(m25p80_register_types)
+
+BlockBackend *m25p80_get_blk(DeviceState *dev)
+{
+    return M25P80(dev)->blk;
+}
-- 
2.40.0

[PATCH 10/12] aspeed: Get the BlockBackend of FMC0 from the flash device

[Cédric Le Goater]

and get rid of an unnecessary drive_get(IF_MTD) call.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a8832c0072..3d5488faf7 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -15,6 +15,7 @@
 #include "hw/arm/aspeed.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/arm/aspeed_eeprom.h"
+#include "hw/block/flash.h"
 #include "hw/i2c/i2c_mux_pca954x.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/misc/pca9552.h"
@@ -427,11 +428,12 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     if (!bmc->mmio_exec) {
-        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
+        DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
+        BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
 
-        if (mtd0) {
+        if (fmc0) {
             uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
-            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
+            aspeed_install_boot_rom(bmc, fmc0, rom_size);
         }
     }
 
-- 
2.40.0

[PATCH 11/12] aspeed: Introduce a "uart" machine option

[Cédric Le Goater]

Most of the Aspeed machines use the UART5 device for the boot console,
and QEMU connects the first serial Chardev to this SoC device for this
purpose. See routine connect_serial_hds_to_uarts().

Nevertheless, some machines use another boot console, such as the fuji,
and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
default") introduced a SoC class attribute 'uart_default' and property
to be able to change the boot console device. It was later changed by
commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").

The "uart" machine option goes a step further and lets the user define
the UART device from the QEMU command line without introducing a new
machine definition. For instance, to use device UART3 (mapped on
/dev/ttyS2 under Linux) instead of the default UART5, one would use :

  -M ast2500-evb,uart=uart3

Cc: Abhishek Singh Dagur <abhishek@drut.io>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 docs/system/arm/aspeed.rst | 10 ++++++++++
 hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index d4e293e7f9..e70f0aeea3 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
 
  * ``spi-model`` to change the SPI Flash model.
 
+ * ``uart`` to change the default console device. Most of the machines
+   use the ``UART5`` device for a boot console, which is mapped on
+   ``/dev/ttyS4`` under Linux, but it is not always the case.
+
 For instance, to start the ``ast2500-evb`` machine with a different
 FMC chip and a bigger (64M) SPI chip, use :
 
@@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
 
   -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
 
+To change the boot console and use device ``UART3`` (``/dev/ttyS2``
+under Linux), use :
+
+.. code-block:: bash
+
+  -M ast2500-evb,uart=uart3
 
 Aspeed minibmc family boards (``ast1030-evb``)
 ==================================================================
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 3d5488faf7..6c32f674b9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -43,6 +43,7 @@ struct AspeedMachineState {
     AspeedSoCState soc;
     MemoryRegion boot_rom;
     bool mmio_exec;
+    uint32_t uart_chosen;
     char *fmc_model;
     char *spi_model;
 };
@@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
     AspeedSoCState *s = &bmc->soc;
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
 
-    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
+    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
     for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
-        if (uart == amc->uart_default) {
+        if (uart == uart_chosen) {
             continue;
         }
         aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
@@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
     bmc->spi_model = g_strdup(value);
 }
 
+static char *aspeed_get_uart(Object *obj, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
+
+    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
+}
+
+static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
+    int val;
+
+    if (sscanf(value, "uart%u", &val) != 1) {
+        error_setg(errp, "Bad value for \"uart\" property");
+        return;
+    }
+
+    /* The number of UART depends on the SoC */
+    if (val < 1 || val > sc->uarts_num) {
+        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
+        return;
+    }
+    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
+}
+
 static void aspeed_machine_class_props_init(ObjectClass *oc)
 {
     object_class_property_add_bool(oc, "execute-in-place",
@@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
     object_class_property_set_description(oc, "execute-in-place",
                            "boot directly from CE0 flash device");
 
+    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
+    object_class_property_set_description(oc, "uart",
+                           "Change the default UART to \"uartX\"");
+
     object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
                                    aspeed_set_fmc_model);
     object_class_property_set_description(oc, "fmc-model",
-- 
2.40.0

[PATCH 12/12] target/arm: Allow users to set the number of VFP registers

[Cédric Le Goater]

Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
have 16 64-bit FPU registers and not 32 registers. Let users set the
number of VFP registers with a CPU property.

The primary use case of this property is for the Cortex A7 of the
Aspeed AST2600 SoC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/arm/cpu.h        |  2 ++
 hw/arm/aspeed_ast2600.c |  2 ++
 target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d469a2637b..79f1a96ddf 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -916,6 +916,8 @@ struct ArchCPU {
     bool has_pmu;
     /* CPU has VFP */
     bool has_vfp;
+    /* CPU has 32 VFP registers */
+    bool has_vfp_d32;
     /* CPU has Neon */
     bool has_neon;
     /* CPU has M-profile DSP extension */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1bf1246148..a8b3a8065a 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                                 &error_abort);
         object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
                                 &error_abort);
+        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
+                                &error_abort);
         object_property_set_link(OBJECT(&s->cpu[i]), "memory",
                                  OBJECT(s->memory), &error_abort);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5182ed0c91..74fe6ae781 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
 static Property arm_cpu_has_vfp_property =
             DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
 
+static Property arm_cpu_has_vfp_d32_property =
+            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
+
 static Property arm_cpu_has_neon_property =
             DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
 
@@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
         }
     }
 
+    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
+        cpu->has_vfp_d32 = true;
+        if (!kvm_enabled()) {
+            /*
+             * The permitted values of the SIMDReg bits [3:0] on
+             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
+             * make sure that has_vfp_d32 can not be set to false.
+             */
+            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
+                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
+                qdev_property_add_static(DEVICE(obj),
+                                         &arm_cpu_has_vfp_d32_property);
+            }
+        }
+    }
+
     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
         cpu->has_neon = true;
         if (!kvm_enabled()) {
@@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (cpu->has_vfp_d32 != cpu->has_neon) {
+        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
+        return;
+    }
+
+   if (!cpu->has_vfp_d32) {
+        uint32_t u;
+
+        u = cpu->isar.mvfr0;
+        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
+        cpu->isar.mvfr0 = u;
+    }
+
     if (!cpu->has_vfp) {
         uint64_t t;
         uint32_t u;
-- 
2.40.0

Re: [PATCH 02/12] aspeed: Introduce a boot_rom region at the machine level

[Francisco Iglesias]

On [2023 May 08] Mon 09:58:49, Cédric Le Goater wrote:
> This should also avoid Coverity to report a memory leak warning when
> the QEMU process exits. See CID 1508061.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/arm/aspeed.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 0b29028fe1..b654513f35 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -40,6 +40,7 @@ struct AspeedMachineState {
>      /* Public */
>  
>      AspeedSoCState soc;
> +    MemoryRegion boot_rom;
>      bool mmio_exec;
>      char *fmc_model;
>      char *spi_model;
> @@ -275,15 +276,15 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
>   * Create a ROM and copy the flash contents at the expected address
>   * (0x0). Boots faster than execute-in-place.
>   */
> -static void aspeed_install_boot_rom(AspeedSoCState *soc, BlockBackend *blk,
> +static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
>                                      uint64_t rom_size)
>  {
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    AspeedSoCState *soc = &bmc->soc;
>  
> -    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
> +    memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
>                             &error_abort);
>      memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
> -                                        boot_rom, 1);
> +                                        &bmc->boot_rom, 1);
>      write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
>  }
>  
> @@ -431,8 +432,7 @@ static void aspeed_machine_init(MachineState *machine)
>  
>          if (mtd0) {
>              uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
> -            aspeed_install_boot_rom(&bmc->soc, blk_by_legacy_dinfo(mtd0),
> -                                    rom_size);
> +            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
>          }
>      }
>  
> -- 
> 2.40.0
> 
> 

Re: [PATCH 01/12] aspeed/hace: Initialize g_autofree pointer

[Francisco Iglesias]

On [2023 May 08] Mon 09:58:48, Cédric Le Goater wrote:
> As mentioned in docs/devel/style.rst "Automatic memory deallocation":
> 
> * Variables declared with g_auto* MUST always be initialized,
>   otherwise the cleanup function will use uninitialized stack memory
> 
> This avoids QEMU to coredump when running the "hash test" command
> under Zephyr.
> 
> Cc: Steven Lee <steven_lee@aspeedtech.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Fixes: c5475b3f9a ("hw: Model ASPEED's Hash and Crypto Engine")
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20230421131547.2177449-1-clg@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/misc/aspeed_hace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 12a761f1f5..b07506ec04 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -189,7 +189,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                                bool acc_mode)
>  {
>      struct iovec iov[ASPEED_HACE_MAX_SG];
> -    g_autofree uint8_t *digest_buf;
> +    g_autofree uint8_t *digest_buf = NULL;
>      size_t digest_len = 0;
>      int niov = 0;
>      int i;
> -- 
> 2.40.0
> 
> 

Re: [PATCH 00/12] aspeed: fixes and extensions

[Cédric Le Goater]

Hello,

On 5/8/23 09:58, Cédric Le Goater wrote:
> Hello,
> 
> This series fixes issues spotted by Coverity and adds a couple of
> improvements for the machine definition.
> 
> The first is to offer the capability to define all CS of all SPI
> controllers without introducing new machine types, using a blockdev on
> the command line :
> 
>      -blockdev node-name=fmc0,driver=file,filename=./flash.img
>      -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0
> 
> instead of using drives which relies on the command line order.
> Ultimately, we will get rid of drive_get(IF_MTD, ...) but we are not
> there yet. For this, SSIPeripheral is extended with an "addr"
> property.
> 
> A second extension is the introduction of a "uart" machine option to
> let the user define the default UART device of the machine from the
> QEMU command line :
> 
>      -M ast2500-evb,uart=uart3
> 
> Last, a new "vfp-d32" CPU property is added to ARM CPUs to model FPUs
> implementing VFPv4 without NEON support and with 16 64-bit FPU
> registers (and not 32 registers). This is the case for the Cortex A7
> of the Aspeed AST2600 SoC. I hope I got it right this time.

I should include these changes in the next aspeed PR. I would have
preferred to have R-b tags on some of them, SSI for instance, and
also the last patch adding the "vfp-d32" CPU property.

Thanks,

C.

Re: [PATCH 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> Boards will use this new property to identify the device CS line and
> wire the SPI controllers accordingly.
> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/ssi/ssi.h | 3 +++
>   hw/ssi/ssi.c         | 7 +++++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 6950f86810..ffd3a34ba4 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -64,6 +64,9 @@ struct SSIPeripheral {
>   
>       /* Chip select state */
>       bool cs;
> +
> +    /* Chip select address/index */
> +    uint32_t addr;

uint8_t is probably enough.

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   };

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> Simple routine to retrieve a DeviceState object on a SPI bus using its
> address/cs. It will be useful for the board to wire the CS lines.
> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/ssi/ssi.h |  2 ++
>   hw/ssi/ssi.c         | 15 +++++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index ffd3a34ba4..c7beabdb09 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>   
>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>   
> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);

Previous patch use uint32_t. uint8_t is probably enough,
otherwise 'unsigned'? Otherwise

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/12] aspeed/smc: Wire CS lines at reset

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> Currently, a set of default flash devices is created at machine init
> and drives defined on the QEMU command line are associated to the FMC
> and SPI controllers in sequence :
> 
>     -drive file<file>,format=raw,if=mtd
>     -drive file<file1>,format=raw,if=mtd
> 
> The CS lines are wired in the same creation loop. This makes a strong
> assumption on the ordering and is not very flexible since only a
> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
> which is less than what the SoC really supports.
> 
> A better alternative would be to define the flash devices on the
> command line using a blockdev attached to a CS line of a SSI bus :
> 
>      -blockdev node-name=fmc0,driver=file,filename=./flash.img
>      -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0
> 
> However, user created flash devices are not correctly wired to their
> SPI controller and consequently can not be used by the machine. Fix
> that and wire the CS lines of all available devices when the SSI bus
> is reset.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c     | 5 +----
>   hw/ssi/aspeed_smc.c | 8 ++++++++
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index b654513f35..9a15899cbc 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>   
>       for (i = 0; i < count; ++i) {
>           DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
> -        qemu_irq cs_line;
>           DeviceState *dev;
>   
>           dev = qdev_new(flashtype);
>           if (dinfo) {
>               qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>           }
> +        qdev_prop_set_uint32(dev, "addr", i);
>           qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
> -
> -        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> -        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>       }
>   }
>   
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 7281169322..2a4001b774 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
>           memset(s->regs, 0, sizeof s->regs);
>       }
>   
> +    for (i = 0; i < asc->cs_num_max; i++) {
> +        DeviceState *dev = ssi_get_cs(s->spi, i);
> +        if (dev) {
> +            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> +            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
> +        }
> +    }

Why did you chose the DeviceReset handler?
Shouldn't this be done in aspeed_smc_realize(), the DeviceRealize handler?

>       /* Unselect all peripherals */
>       for (i = 0; i < asc->cs_num_max; ++i) {
>           s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;

Re: [PATCH 07/12] hw/ssi: Check for duplicate addresses

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> This to avoid address conflicts on the same SSI bus. Adapt machines
> using multiple devices on the same bus to avoid breakage.
> 
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/stellaris.c                  |  4 +++-
>   hw/arm/xilinx_zynq.c                |  1 +
>   hw/arm/xlnx-versal-virt.c           |  1 +
>   hw/arm/xlnx-zcu102.c                |  2 ++
>   hw/microblaze/petalogix_ml605_mmu.c |  1 +
>   hw/ssi/ssi.c                        | 20 ++++++++++++++++++++
>   6 files changed, 28 insertions(+), 1 deletion(-)


> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index a25e064417..685b7678e0 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -42,10 +42,30 @@ DeviceState *ssi_get_cs(SSIBus *bus, int addr)
>       return NULL;
>   }
>   
> +static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error **errp)
> +{
> +    SSIPeripheral *s = SSI_PERIPHERAL(dev);
> +
> +    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
> +        error_setg(errp, "addr '0x%x' already in use", s->addr);

We could return "... in use by a $MODEL device".

   DeviceState *d = ssi_get_cs(SSI_BUS(b), s->addr);
   if (d) {
       "... in use by a %s device", ..., object_get_typename(OBJECT(d)));
   }

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        return false;
> +    }
> +
> +    return true;
> +}

Re: [PATCH 10/12] aspeed: Get the BlockBackend of FMC0 from the flash device

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> and get rid of an unnecessary drive_get(IF_MTD) call.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> It will help in getting rid of some drive_get(IF_MTD) calls by
> retrieving the BlockBackend directly from the m25p80 device.
> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/block/flash.h | 4 ++++
>   hw/block/m25p80.c        | 6 ++++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> index 7198953702..de93756cbe 100644
> --- a/include/hw/block/flash.h
> +++ b/include/hw/block/flash.h
> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>   void ecc_reset(ECCState *s);
>   extern const VMStateDescription vmstate_ecc_state;
>   
> +/* m25p80.c */
> +
> +BlockBackend *m25p80_get_blk(DeviceState *dev);

- Option 1, declare QOM typedef and use proper type:

   #define TYPE_M25P80 "m25p80-generic"
   OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80)

   BlockBackend *m25p80_get_blk(Flash *dev);

- Option 2, preliminary patch renaming 'Flash' type to
'M25P80' then option 1 again

- Option 3: no change.

With the QOM style we try to enforce, I'd go for #2.

Still,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   #endif
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index dc5ffbc4ff..afc3fdf4d6 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -25,6 +25,7 @@
>   #include "qemu/units.h"
>   #include "sysemu/block-backend.h"
>   #include "hw/block/block.h"
> +#include "hw/block/flash.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-properties-system.h"
>   #include "hw/ssi/ssi.h"
> @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void)
>   }
>   
>   type_init(m25p80_register_types)
> +
> +BlockBackend *m25p80_get_blk(DeviceState *dev)
> +{
> +    return M25P80(dev)->blk;
> +}

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Philippe Mathieu-Daudé]

On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>> address/cs. It will be useful for the board to wire the CS lines.
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/ssi/ssi.h |  2 ++
>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>> index ffd3a34ba4..c7beabdb09 100644
>> --- a/include/hw/ssi/ssi.h
>> +++ b/include/hw/ssi/ssi.h
>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const 
>> char *name);
>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);

Also, this helper should (preferably) return a SSIPeripheral type.

> Previous patch use uint32_t. uint8_t is probably enough,
> otherwise 'unsigned'? Otherwise
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [PATCH 11/12] aspeed: Introduce a "uart" machine option

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> Most of the Aspeed machines use the UART5 device for the boot console,
> and QEMU connects the first serial Chardev to this SoC device for this
> purpose. See routine connect_serial_hds_to_uarts().
> 
> Nevertheless, some machines use another boot console, such as the fuji,
> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
> default") introduced a SoC class attribute 'uart_default' and property
> to be able to change the boot console device. It was later changed by
> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
> 
> The "uart" machine option goes a step further and lets the user define
> the UART device from the QEMU command line without introducing a new
> machine definition. For instance, to use device UART3 (mapped on
> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
> 
>    -M ast2500-evb,uart=uart3
> 
> Cc: Abhishek Singh Dagur <abhishek@drut.io>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   docs/system/arm/aspeed.rst | 10 ++++++++++
>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>   2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index d4e293e7f9..e70f0aeea3 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>   
>    * ``spi-model`` to change the SPI Flash model.
>   
> + * ``uart`` to change the default console device. Most of the machines
> +   use the ``UART5`` device for a boot console, which is mapped on
> +   ``/dev/ttyS4`` under Linux, but it is not always the case.

This comment ...

>   For instance, to start the ``ast2500-evb`` machine with a different
>   FMC chip and a bigger (64M) SPI chip, use :
>   
> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>   
>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>   
> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
> +under Linux), use :

... and this one suggest 'boot-console' could be a better name.

Or 'boot-console-index'.

> +.. code-block:: bash
> +
> +  -M ast2500-evb,uart=uart3
>   
>   Aspeed minibmc family boards (``ast1030-evb``)
>   ==================================================================
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 3d5488faf7..6c32f674b9 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>       AspeedSoCState soc;
>       MemoryRegion boot_rom;
>       bool mmio_exec;
> +    uint32_t uart_chosen;
>       char *fmc_model;
>       char *spi_model;
>   };
> @@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>       AspeedSoCState *s = &bmc->soc;
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
> +    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
>       for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> -        if (uart == amc->uart_default) {
> +        if (uart == uart_chosen) {
>               continue;
>           }
>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
>       bmc->spi_model = g_strdup(value);
>   }
>   
> +static char *aspeed_get_uart(Object *obj, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
> +
> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
> +}
> +
> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
> +    int val;
> +
> +    if (sscanf(value, "uart%u", &val) != 1) {
> +        error_setg(errp, "Bad value for \"uart\" property");

Why are you asking for a string and not an index?

> +        return;
> +    }
> +
> +    /* The number of UART depends on the SoC */
> +    if (val < 1 || val > sc->uarts_num) {
> +        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
> +        return;
> +    }
> +    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
> +}
> +
>   static void aspeed_machine_class_props_init(ObjectClass *oc)
>   {
>       object_class_property_add_bool(oc, "execute-in-place",
> @@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
>       object_class_property_set_description(oc, "execute-in-place",
>                              "boot directly from CE0 flash device");
>   
> +    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
> +    object_class_property_set_description(oc, "uart",
> +                           "Change the default UART to \"uartX\"");
> +
>       object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
>                                      aspeed_set_fmc_model);
>       object_class_property_set_description(oc, "fmc-model",

Re: [PATCH 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> This change completes commits 5aa281d757 ("aspeed: Introduce a
> spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
> boot_rom overlap region in the SoC spi_boot container") which
> introduced a spi_boot container at the SoC level to map the boot rom
> region as an overlap.

2nd patch,

> It also fixes a Coverity report (CID 1508061) for a memory leak
> warning when the QEMU process exits by using an bmc_boot_rom
> MemoryRegion available at the machine level.

1st patch?

> Cc: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/fby35.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)

Re: [PATCH 02/12] aspeed: Introduce a boot_rom region at the machine level

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> This should also avoid Coverity to report a memory leak warning when
> the QEMU process exits. See CID 1508061.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 01/12] aspeed/hace: Initialize g_autofree pointer

[Philippe Mathieu-Daudé]

On 8/5/23 09:58, Cédric Le Goater wrote:
> As mentioned in docs/devel/style.rst "Automatic memory deallocation":
> 
> * Variables declared with g_auto* MUST always be initialized,
>    otherwise the cleanup function will use uninitialized stack memory
> 
> This avoids QEMU to coredump when running the "hash test" command
> under Zephyr.
> 
> Cc: Steven Lee <steven_lee@aspeedtech.com>
> Cc: Joel Stanley <joel@jms.id.au>

Cc: qemu-stable@nongnu.org

> Fixes: c5475b3f9a ("hw: Model ASPEED's Hash and Crypto Engine")
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20230421131547.2177449-1-clg@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/misc/aspeed_hace.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 12a761f1f5..b07506ec04 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -189,7 +189,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                                 bool acc_mode)
>   {
>       struct iovec iov[ASPEED_HACE_MAX_SG];
> -    g_autofree uint8_t *digest_buf;
> +    g_autofree uint8_t *digest_buf = NULL;
>       size_t digest_len = 0;
>       int niov = 0;
>       int i;

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Cédric Le Goater]

On 5/30/23 23:27, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> This change completes commits 5aa281d757 ("aspeed: Introduce a
>> spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
>> boot_rom overlap region in the SoC spi_boot container") which
>> introduced a spi_boot container at the SoC level to map the boot rom
>> region as an overlap.
> 
> 2nd patch,
> 
>> It also fixes a Coverity report (CID 1508061) for a memory leak
>> warning when the QEMU process exits by using an bmc_boot_rom
>> MemoryRegion available at the machine level.
> 
> 1st patch?

That would be superfluous.

> 
>> Cc: Peter Delevoryas <peter@pjd.dev>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/arm/fby35.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
> 

Re: [PATCH 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Cédric Le Goater]

On 5/30/23 22:33, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Boards will use this new property to identify the device CS line and
>> wire the SPI controllers accordingly.
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/ssi/ssi.h | 3 +++
>>   hw/ssi/ssi.c         | 7 +++++++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>> index 6950f86810..ffd3a34ba4 100644
>> --- a/include/hw/ssi/ssi.h
>> +++ b/include/hw/ssi/ssi.h
>> @@ -64,6 +64,9 @@ struct SSIPeripheral {
>>       /* Chip select state */
>>       bool cs;
>> +
>> +    /* Chip select address/index */
>> +    uint32_t addr;
> 
> uint8_t is probably enough.

Yes. A uint8_t is more than enough.

Thanks,

C.

  
> 
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>>   };
> 

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 5/30/23 22:34, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>> address/cs. It will be useful for the board to wire the CS lines.
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/ssi/ssi.h |  2 ++
>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>> index ffd3a34ba4..c7beabdb09 100644
>> --- a/include/hw/ssi/ssi.h
>> +++ b/include/hw/ssi/ssi.h
>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
> 
> Previous patch use uint32_t. uint8_t is probably enough,
> otherwise 'unsigned'? Otherwise

uint8_t is fine.

Thanks,

C.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>>> address/cs. It will be useful for the board to wire the CS lines.
>>>
>>> Cc: Alistair Francis <alistair@alistair23.me>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   include/hw/ssi/ssi.h |  2 ++
>>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>>   2 files changed, 17 insertions(+)
>>>
>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>> index ffd3a34ba4..c7beabdb09 100644
>>> --- a/include/hw/ssi/ssi.h
>>> +++ b/include/hw/ssi/ssi.h
>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
> 
> Also, this helper should (preferably) return a SSIPeripheral type.

Well, having a DeviceState is handy for the callers (today) and
ssi_create_peripheral() returns a DeviceState. Let's keep it that
way.

Thanks,

C.

> 
>> Previous patch use uint32_t. uint8_t is probably enough,
>> otherwise 'unsigned'? Otherwise
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
> 

Re: [PATCH 06/12] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

On 5/30/23 22:56, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Currently, a set of default flash devices is created at machine init
>> and drives defined on the QEMU command line are associated to the FMC
>> and SPI controllers in sequence :
>>
>>     -drive file<file>,format=raw,if=mtd
>>     -drive file<file1>,format=raw,if=mtd
>>
>> The CS lines are wired in the same creation loop. This makes a strong
>> assumption on the ordering and is not very flexible since only a
>> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
>> which is less than what the SoC really supports.
>>
>> A better alternative would be to define the flash devices on the
>> command line using a blockdev attached to a CS line of a SSI bus :
>>
>>      -blockdev node-name=fmc0,driver=file,filename=./flash.img
>>      -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0
>>
>> However, user created flash devices are not correctly wired to their
>> SPI controller and consequently can not be used by the machine. Fix
>> that and wire the CS lines of all available devices when the SSI bus
>> is reset.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/arm/aspeed.c     | 5 +----
>>   hw/ssi/aspeed_smc.c | 8 ++++++++
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index b654513f35..9a15899cbc 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>       for (i = 0; i < count; ++i) {
>>           DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
>> -        qemu_irq cs_line;
>>           DeviceState *dev;
>>           dev = qdev_new(flashtype);
>>           if (dinfo) {
>>               qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>>           }
>> +        qdev_prop_set_uint32(dev, "addr", i);
>>           qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>> -
>> -        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>> -        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>>       }
>>   }
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 7281169322..2a4001b774 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
>>           memset(s->regs, 0, sizeof s->regs);
>>       }
>> +    for (i = 0; i < asc->cs_num_max; i++) {
>> +        DeviceState *dev = ssi_get_cs(s->spi, i);
>> +        if (dev) {
>> +            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>> +            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>> +        }
>> +    }
> 
> Why did you chose the DeviceReset handler?
> Shouldn't this be done in aspeed_smc_realize(), the DeviceRealize handler?

This looks weird, I agree, but the wiring needs to be done at reset time
to wire the user-created devices. I don't think there is another way.

Thanks,

C.
  
  
> 
>>       /* Unselect all peripherals */
>>       for (i = 0; i < asc->cs_num_max; ++i) {
>>           s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
> 

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Philippe Mathieu-Daudé]

+QOM tinkerers

On 31/5/23 07:59, Cédric Le Goater wrote:
> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>>>> address/cs. It will be useful for the board to wire the CS lines.
>>>>
>>>> Cc: Alistair Francis <alistair@alistair23.me>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>   include/hw/ssi/ssi.h |  2 ++
>>>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>>>   2 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>>> index ffd3a34ba4..c7beabdb09 100644
>>>> --- a/include/hw/ssi/ssi.h
>>>> +++ b/include/hw/ssi/ssi.h
>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, 
>>>> const char *name);
>>>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
>>
>> Also, this helper should (preferably) return a SSIPeripheral type.
> 
> Well, having a DeviceState is handy for the callers (today) and
> ssi_create_peripheral() returns a DeviceState. Let's keep it that
> way.

Yes I know it is handy :) I'm not against your patch; besides other
APIs do that. I'm wondering about QOM design here. Having Foo device,
should FOO API return the common qdev abstract type (DeviceState) or
a Foo type? Either ways we keep QOM-casting around, but I still tend
to consider FOO API returning Foo pointer provides some type check
safety, and also provides the API user hints about what is used.
Need more coffee.

Re: [PATCH 07/12] hw/ssi: Check for duplicate addresses

[Cédric Le Goater]

On 5/30/23 23:05, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> This to avoid address conflicts on the same SSI bus. Adapt machines
>> using multiple devices on the same bus to avoid breakage.
>>
>> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/arm/stellaris.c                  |  4 +++-
>>   hw/arm/xilinx_zynq.c                |  1 +
>>   hw/arm/xlnx-versal-virt.c           |  1 +
>>   hw/arm/xlnx-zcu102.c                |  2 ++
>>   hw/microblaze/petalogix_ml605_mmu.c |  1 +
>>   hw/ssi/ssi.c                        | 20 ++++++++++++++++++++
>>   6 files changed, 28 insertions(+), 1 deletion(-)
> 
> 
>> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
>> index a25e064417..685b7678e0 100644
>> --- a/hw/ssi/ssi.c
>> +++ b/hw/ssi/ssi.c
>> @@ -42,10 +42,30 @@ DeviceState *ssi_get_cs(SSIBus *bus, int addr)
>>       return NULL;
>>   }
>> +static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error **errp)
>> +{
>> +    SSIPeripheral *s = SSI_PERIPHERAL(dev);
>> +
>> +    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
>> +        error_setg(errp, "addr '0x%x' already in use", s->addr);
> 
> We could return "... in use by a $MODEL device".
> 
>    DeviceState *d = ssi_get_cs(SSI_BUS(b), s->addr);
>    if (d) {
>        "... in use by a %s device", ..., object_get_typename(OBJECT(d)));
>    }

Yes. I will change the error message.

Thanks,

C.


> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
> 
> 

Re: [PATCH 11/12] aspeed: Introduce a "uart" machine option

[Cédric Le Goater]

On 5/30/23 23:22, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Most of the Aspeed machines use the UART5 device for the boot console,
>> and QEMU connects the first serial Chardev to this SoC device for this
>> purpose. See routine connect_serial_hds_to_uarts().
>>
>> Nevertheless, some machines use another boot console, such as the fuji,
>> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
>> default") introduced a SoC class attribute 'uart_default' and property
>> to be able to change the boot console device. It was later changed by
>> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
>>
>> The "uart" machine option goes a step further and lets the user define
>> the UART device from the QEMU command line without introducing a new
>> machine definition. For instance, to use device UART3 (mapped on
>> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
>>
>>    -M ast2500-evb,uart=uart3
>>
>> Cc: Abhishek Singh Dagur <abhishek@drut.io>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   docs/system/arm/aspeed.rst | 10 ++++++++++
>>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
>> index d4e293e7f9..e70f0aeea3 100644
>> --- a/docs/system/arm/aspeed.rst
>> +++ b/docs/system/arm/aspeed.rst
>> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>>    * ``spi-model`` to change the SPI Flash model.
>> + * ``uart`` to change the default console device. Most of the machines
>> +   use the ``UART5`` device for a boot console, which is mapped on
>> +   ``/dev/ttyS4`` under Linux, but it is not always the case.
> 
> This comment ...
> 
>>   For instance, to start the ``ast2500-evb`` machine with a different
>>   FMC chip and a bigger (64M) SPI chip, use :
>> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
>> +under Linux), use :
> 
> ... and this one suggest 'boot-console' could be a better name.
> 
> Or 'boot-console-index'.

you might be right. people expect to find the console messages on
this device. I will think about it.

> 
>> +.. code-block:: bash
>> +
>> +  -M ast2500-evb,uart=uart3
>>   Aspeed minibmc family boards (``ast1030-evb``)
>>   ==================================================================
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 3d5488faf7..6c32f674b9 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>>       AspeedSoCState soc;
>>       MemoryRegion boot_rom;
>>       bool mmio_exec;
>> +    uint32_t uart_chosen;
>>       char *fmc_model;
>>       char *spi_model;
>>   };
>> @@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>       AspeedSoCState *s = &bmc->soc;
>>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
>> +    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
>>       for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
>> -        if (uart == amc->uart_default) {
>> +        if (uart == uart_chosen) {
>>               continue;
>>           }
>>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
>> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
>>       bmc->spi_model = g_strdup(value);
>>   }
>> +static char *aspeed_get_uart(Object *obj, Error **errp)
>> +{
>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>> +
>> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
>> +}
>> +
>> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
>> +{
>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>> +    int val;
>> +
>> +    if (sscanf(value, "uart%u", &val) != 1) {
>> +        error_setg(errp, "Bad value for \"uart\" property");
> 
> Why are you asking for a string and not an index?

because the literal name is what people find in the DT. See files
arch/arm/boot/dts/aspeed-bmc-* under Linux.

Thanks,

C.

> 
>> +        return;
>> +    }
>> +
>> +    /* The number of UART depends on the SoC */
>> +    if (val < 1 || val > sc->uarts_num) {
>> +        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
>> +        return;
>> +    }
>> +    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
>> +}
>> +
>>   static void aspeed_machine_class_props_init(ObjectClass *oc)
>>   {
>>       object_class_property_add_bool(oc, "execute-in-place",
>> @@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
>>       object_class_property_set_description(oc, "execute-in-place",
>>                              "boot directly from CE0 flash device");
>> +    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
>> +    object_class_property_set_description(oc, "uart",
>> +                           "Change the default UART to \"uartX\"");
>> +
>>       object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
>>                                      aspeed_set_fmc_model);
>>       object_class_property_set_description(oc, "fmc-model",
> 

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 5/31/23 08:17, Philippe Mathieu-Daudé wrote:
> +QOM tinkerers
> 
> On 31/5/23 07:59, Cédric Le Goater wrote:
>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>>>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>>>>> address/cs. It will be useful for the board to wire the CS lines.
>>>>>
>>>>> Cc: Alistair Francis <alistair@alistair23.me>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>> ---
>>>>>   include/hw/ssi/ssi.h |  2 ++
>>>>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>>>>   2 files changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>>>> index ffd3a34ba4..c7beabdb09 100644
>>>>> --- a/include/hw/ssi/ssi.h
>>>>> +++ b/include/hw/ssi/ssi.h
>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>>>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
>>>
>>> Also, this helper should (preferably) return a SSIPeripheral type.
>>
>> Well, having a DeviceState is handy for the callers (today) and
>> ssi_create_peripheral() returns a DeviceState. Let's keep it that
>> way.
> 
> Yes I know it is handy :) I'm not against your patch; besides other
> APIs do that. I'm wondering about QOM design here. Having Foo device,
> should FOO API return the common qdev abstract type (DeviceState) or
> a Foo type? Either ways we keep QOM-casting around, but I still tend
> to consider FOO API returning Foo pointer provides some type check
> safety, and also provides the API user hints about what is used.
> Need more coffee.

It is used in two code paths today:

	...
         DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
         BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
	...
and
	...
         DeviceState *dev = ssi_get_cs(s->spi, i);
         if (dev) {
             qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
	...


Changing the interface is not a radical change, it will add two QOM-casts.
I can give it a try in v2.

Thanks,

C.

Re: [PATCH 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Cédric Le Goater]

On 5/30/23 23:14, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> It will help in getting rid of some drive_get(IF_MTD) calls by
>> retrieving the BlockBackend directly from the m25p80 device.
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/block/flash.h | 4 ++++
>>   hw/block/m25p80.c        | 6 ++++++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>> index 7198953702..de93756cbe 100644
>> --- a/include/hw/block/flash.h
>> +++ b/include/hw/block/flash.h
>> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>   void ecc_reset(ECCState *s);
>>   extern const VMStateDescription vmstate_ecc_state;
>> +/* m25p80.c */
>> +
>> +BlockBackend *m25p80_get_blk(DeviceState *dev);
> 
> - Option 1, declare QOM typedef and use proper type:
> 
>    #define TYPE_M25P80 "m25p80-generic"
>    OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80)
> 
>    BlockBackend *m25p80_get_blk(Flash *dev);
> 
> - Option 2, preliminary patch renaming 'Flash' type to
> 'M25P80' then option 1 again

That's a large change and we would need to introduce a m25p80.h with
these definitions. Given that the caller needs a DeviceState in the
end, may be not worth the extra hassle.

How would you rename 'Flash' ? 'SpiFlash' ?

Thanks,

C.

> 
> - Option 3: no change.
> 
> With the QOM style we try to enforce, I'd go for #2.
> 
> Still,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>>   #endif
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index dc5ffbc4ff..afc3fdf4d6 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -25,6 +25,7 @@
>>   #include "qemu/units.h"
>>   #include "sysemu/block-backend.h"
>>   #include "hw/block/block.h"
>> +#include "hw/block/flash.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-properties-system.h"
>>   #include "hw/ssi/ssi.h"
>> @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void)
>>   }
>>   type_init(m25p80_register_types)
>> +
>> +BlockBackend *m25p80_get_blk(DeviceState *dev)
>> +{
>> +    return M25P80(dev)->blk;
>> +}
> 

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Philippe Mathieu-Daudé]

On 31/5/23 08:36, Cédric Le Goater wrote:
> On 5/31/23 08:17, Philippe Mathieu-Daudé wrote:
>> +QOM tinkerers
>>
>> On 31/5/23 07:59, Cédric Le Goater wrote:
>>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
>>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>>>>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using 
>>>>>> its
>>>>>> address/cs. It will be useful for the board to wire the CS lines.
>>>>>>
>>>>>> Cc: Alistair Francis <alistair@alistair23.me>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>   include/hw/ssi/ssi.h |  2 ++
>>>>>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>>>>>   2 files changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>>>>> index ffd3a34ba4..c7beabdb09 100644
>>>>>> --- a/include/hw/ssi/ssi.h
>>>>>> +++ b/include/hw/ssi/ssi.h
>>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, 
>>>>>> const char *name);
>>>>>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
>>>>
>>>> Also, this helper should (preferably) return a SSIPeripheral type.
>>>
>>> Well, having a DeviceState is handy for the callers (today) and
>>> ssi_create_peripheral() returns a DeviceState. Let's keep it that
>>> way.
>>
>> Yes I know it is handy :) I'm not against your patch; besides other
>> APIs do that. I'm wondering about QOM design here. Having Foo device,
>> should FOO API return the common qdev abstract type (DeviceState) or
>> a Foo type? Either ways we keep QOM-casting around, but I still tend
>> to consider FOO API returning Foo pointer provides some type check
>> safety, and also provides the API user hints about what is used.
>> Need more coffee.
> 
> It is used in two code paths today:
> 
>      ...
>          DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
>          BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
>      ...
> and
>      ...
>          DeviceState *dev = ssi_get_cs(s->spi, i);
>          if (dev) {
>              qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 
> 0);
>      ...
> 
> 
> Changing the interface is not a radical change, it will add two QOM-casts.
> I can give it a try in v2.

Hold on, lets see what others think first.

Re: [PATCH 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Philippe Mathieu-Daudé]

On 31/5/23 08:48, Cédric Le Goater wrote:
> On 5/30/23 23:14, Philippe Mathieu-Daudé wrote:
>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>> It will help in getting rid of some drive_get(IF_MTD) calls by
>>> retrieving the BlockBackend directly from the m25p80 device.
>>>
>>> Cc: Alistair Francis <alistair@alistair23.me>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   include/hw/block/flash.h | 4 ++++
>>>   hw/block/m25p80.c        | 6 ++++++
>>>   2 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>>> index 7198953702..de93756cbe 100644
>>> --- a/include/hw/block/flash.h
>>> +++ b/include/hw/block/flash.h
>>> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>>>   void ecc_reset(ECCState *s);
>>>   extern const VMStateDescription vmstate_ecc_state;
>>> +/* m25p80.c */
>>> +
>>> +BlockBackend *m25p80_get_blk(DeviceState *dev);
>>
>> - Option 1, declare QOM typedef and use proper type:
>>
>>    #define TYPE_M25P80 "m25p80-generic"
>>    OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80)
>>
>>    BlockBackend *m25p80_get_blk(Flash *dev);
>>
>> - Option 2, preliminary patch renaming 'Flash' type to
>> 'M25P80' then option 1 again
> 
> That's a large change

Yes, it can be done later if you rather.

> and we would need to introduce a m25p80.h with
> these definitions.

FlashPartInfo is used as pointer so can be forward-declared.
Then we only need to move M25P80_INTERNAL_DATA_BUFFER_SZ.
For 5 lines, "hw/block/flash.h" is good enough IMO, keeping
all the rest to m25p80.c.

> Given that the caller needs a DeviceState in the
> end, may be not worth the extra hassle.
> 
> How would you rename 'Flash' ? 'SpiFlash' ?

Since you ask, my preference is SpiNorFlash :)

But again, this can be done later on top.

Thanks,

Phil.

Re: [PATCH 11/12] aspeed: Introduce a "uart" machine option

[Philippe Mathieu-Daudé]

On 31/5/23 08:28, Cédric Le Goater wrote:
> On 5/30/23 23:22, Philippe Mathieu-Daudé wrote:
>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>> Most of the Aspeed machines use the UART5 device for the boot console,
>>> and QEMU connects the first serial Chardev to this SoC device for this
>>> purpose. See routine connect_serial_hds_to_uarts().
>>>
>>> Nevertheless, some machines use another boot console, such as the fuji,
>>> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
>>> default") introduced a SoC class attribute 'uart_default' and property
>>> to be able to change the boot console device. It was later changed by
>>> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
>>>
>>> The "uart" machine option goes a step further and lets the user define
>>> the UART device from the QEMU command line without introducing a new
>>> machine definition. For instance, to use device UART3 (mapped on
>>> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
>>>
>>>    -M ast2500-evb,uart=uart3
>>>
>>> Cc: Abhishek Singh Dagur <abhishek@drut.io>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   docs/system/arm/aspeed.rst | 10 ++++++++++
>>>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>>>   2 files changed, 47 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
>>> index d4e293e7f9..e70f0aeea3 100644
>>> --- a/docs/system/arm/aspeed.rst
>>> +++ b/docs/system/arm/aspeed.rst
>>> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>>>    * ``spi-model`` to change the SPI Flash model.
>>> + * ``uart`` to change the default console device. Most of the machines
>>> +   use the ``UART5`` device for a boot console, which is mapped on
>>> +   ``/dev/ttyS4`` under Linux, but it is not always the case.
>>
>> This comment ...
>>
>>>   For instance, to start the ``ast2500-evb`` machine with a different
>>>   FMC chip and a bigger (64M) SPI chip, use :
>>> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>>>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>>> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
>>> +under Linux), use :
>>
>> ... and this one suggest 'boot-console' could be a better name.
>>
>> Or 'boot-console-index'.
> 
> you might be right. people expect to find the console messages on
> this device. I will think about it.
> 
>>
>>> +.. code-block:: bash
>>> +
>>> +  -M ast2500-evb,uart=uart3
>>>   Aspeed minibmc family boards (``ast1030-evb``)
>>>   ==================================================================
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 3d5488faf7..6c32f674b9 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>>>       AspeedSoCState soc;
>>>       MemoryRegion boot_rom;
>>>       bool mmio_exec;
>>> +    uint32_t uart_chosen;
>>>       char *fmc_model;
>>>       char *spi_model;
>>>   };
>>> @@ -331,10 +332,11 @@ static void 
>>> connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>>       AspeedSoCState *s = &bmc->soc;
>>>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : 
>>> amc->uart_default;
>>> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
>>> +    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
>>>       for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; 
>>> i++, uart++) {
>>> -        if (uart == amc->uart_default) {
>>> +        if (uart == uart_chosen) {
>>>               continue;
>>>           }
>>>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
>>> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, 
>>> const char *value, Error **errp)
>>>       bmc->spi_model = g_strdup(value);
>>>   }
>>> +static char *aspeed_get_uart(Object *obj, Error **errp)
>>> +{
>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : 
>>> amc->uart_default;
>>> +
>>> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 
>>> + 1);
>>> +}
>>> +
>>> +static void aspeed_set_uart(Object *obj, const char *value, Error 
>>> **errp)
>>> +{
>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>> +    AspeedSoCClass *sc = 
>>> ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>>> +    int val;
>>> +
>>> +    if (sscanf(value, "uart%u", &val) != 1) {
>>> +        error_setg(errp, "Bad value for \"uart\" property");
>>
>> Why are you asking for a string and not an index?
> 
> because the literal name is what people find in the DT. See files
> arch/arm/boot/dts/aspeed-bmc-* under Linux.

OK. After looking at this file, I suppose people would expect
a "bmc-console" property name:

   -M ast2500-evb,bmc-console=uart3

Re: [PATCH 11/12] aspeed: Introduce a "uart" machine option

[Cédric Le Goater]

>>>> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
>>>> +{
>>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>>> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>>>> +    int val;
>>>> +
>>>> +    if (sscanf(value, "uart%u", &val) != 1) {
>>>> +        error_setg(errp, "Bad value for \"uart\" property");
>>>
>>> Why are you asking for a string and not an index?
>>
>> because the literal name is what people find in the DT. See files
>> arch/arm/boot/dts/aspeed-bmc-* under Linux.
> 
> OK. After looking at this file, I suppose people would expect
> a "bmc-console" property name:
> 
>    -M ast2500-evb,bmc-console=uart3
> 

yes. This is better naming for the user. I will keep the internal
'uart_chosen' though.

Thanks,

C.

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Bernhard Beschow]

Am 31. Mai 2023 07:39:32 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 31/5/23 08:36, Cédric Le Goater wrote:
>> On 5/31/23 08:17, Philippe Mathieu-Daudé wrote:
>>> +QOM tinkerers
>>> 
>>> On 31/5/23 07:59, Cédric Le Goater wrote:
>>>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
>>>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>>>>>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>>>>>>> address/cs. It will be useful for the board to wire the CS lines.
>>>>>>> 
>>>>>>> Cc: Alistair Francis <alistair@alistair23.me>
>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>>> ---
>>>>>>>   include/hw/ssi/ssi.h |  2 ++
>>>>>>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>>>>>>   2 files changed, 17 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>>>>>> index ffd3a34ba4..c7beabdb09 100644
>>>>>>> --- a/include/hw/ssi/ssi.h
>>>>>>> +++ b/include/hw/ssi/ssi.h
>>>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>>>>>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
>>>>> 
>>>>> Also, this helper should (preferably) return a SSIPeripheral type.
>>>> 
>>>> Well, having a DeviceState is handy for the callers (today) and
>>>> ssi_create_peripheral() returns a DeviceState. Let's keep it that
>>>> way.
>>> 
>>> Yes I know it is handy :) I'm not against your patch; besides other
>>> APIs do that. I'm wondering about QOM design here. Having Foo device,
>>> should FOO API return the common qdev abstract type (DeviceState) or
>>> a Foo type? Either ways we keep QOM-casting around, but I still tend
>>> to consider FOO API returning Foo pointer provides some type check
>>> safety, and also provides the API user hints about what is used.
>>> Need more coffee.
>> 
>> It is used in two code paths today:
>> 
>>      ...
>>          DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
>>          BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;

Looks like m25p80_get_blk() should take a more specific argument as well, like Phil suggested already. Wouldn't that avoid the QOM cast here?

Best regards,
Bernhard

>>      ...
>> and
>>      ...
>>          DeviceState *dev = ssi_get_cs(s->spi, i);
>>          if (dev) {
>>              qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>>      ...
>> 
>> 
>> Changing the interface is not a radical change, it will add two QOM-casts.
>> I can give it a try in v2.
>
>Hold on, lets see what others think first.

Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 6/5/23 07:57, Bernhard Beschow wrote:
> 
> 
> Am 31. Mai 2023 07:39:32 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 31/5/23 08:36, Cédric Le Goater wrote:
>>> On 5/31/23 08:17, Philippe Mathieu-Daudé wrote:
>>>> +QOM tinkerers
>>>>
>>>> On 31/5/23 07:59, Cédric Le Goater wrote:
>>>>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
>>>>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
>>>>>>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>>>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>>>>>>>> address/cs. It will be useful for the board to wire the CS lines.
>>>>>>>>
>>>>>>>> Cc: Alistair Francis <alistair@alistair23.me>
>>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>>>> ---
>>>>>>>>    include/hw/ssi/ssi.h |  2 ++
>>>>>>>>    hw/ssi/ssi.c         | 15 +++++++++++++++
>>>>>>>>    2 files changed, 17 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>>>>>>>> index ffd3a34ba4..c7beabdb09 100644
>>>>>>>> --- a/include/hw/ssi/ssi.h
>>>>>>>> +++ b/include/hw/ssi/ssi.h
>>>>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>>>>>>>    uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>>>>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
>>>>>>
>>>>>> Also, this helper should (preferably) return a SSIPeripheral type.
>>>>>
>>>>> Well, having a DeviceState is handy for the callers (today) and
>>>>> ssi_create_peripheral() returns a DeviceState. Let's keep it that
>>>>> way.
>>>>
>>>> Yes I know it is handy :) I'm not against your patch; besides other
>>>> APIs do that. I'm wondering about QOM design here. Having Foo device,
>>>> should FOO API return the common qdev abstract type (DeviceState) or
>>>> a Foo type? Either ways we keep QOM-casting around, but I still tend
>>>> to consider FOO API returning Foo pointer provides some type check
>>>> safety, and also provides the API user hints about what is used.
>>>> Need more coffee.
>>>
>>> It is used in two code paths today:
>>>
>>>       ...
>>>           DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
>>>           BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
> 
> Looks like m25p80_get_blk() should take a more specific argument as well, like Phil suggested already. Wouldn't that avoid the QOM cast here?

Exposing the m25p80 internals could be painful. I'd rather keep the
definitions where they are under m25p80.c.

Thanks,

C.


> 
> Best regards,
> Bernhard
> 
>>>       ...
>>> and
>>>       ...
>>>           DeviceState *dev = ssi_get_cs(s->spi, i);
>>>           if (dev) {
>>>               qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>>>       ...
>>>
>>>
>>> Changing the interface is not a radical change, it will add two QOM-casts.
>>> I can give it a try in v2.
>>
>> Hold on, lets see what others think first.