[PATCH v3 0/7] aspeed: Add blockdev support for flash device definition

[PATCH v3 0/7] aspeed: Add blockdev support for flash device definition

[Cédric Le Goater]

Hello,

This is a respin of series [1] without the patches merged in 8.1.
Since I renamed a property as suggested, I though people might want to
see the result before the next aspeed PR.

It offers the capability to define all CS of all SPI controllers
without introducing new machine types, using blockdev on the command
line :

    -blockdev node-name=fmc0,driver=file,filename=./flash-ast2600-evb \
    -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
    -blockdev node-name=fmc1,driver=file,filename=./flash-ast2600-evb-alt \
    -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
    -blockdev node-name=spi1,driver=file,filename=./ast2600-evb.pnor \
    -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \

With these changes, it is now possible :

 - to define block backends out of order instead relying on the command
   line order when using drives.
   
 - to define *all* devices backends. Some machines support up to 8.
 
 - to use different flash models without adding new boards. Machine
   options "spi-model" and "fmc-model" could be deprecated.
   
 - to start the machine with -nodefaults to let it fetch instructions
   from the FMC0 device, as HW does. Machine option "execute-in-place"
   could be deprecated.

Ultimately, we will get rid of drive_get(IF_MTD, ...) but we are not
there yet.

Thanks,

C.

[1] https://lore.kernel.org/qemu-devel/20230607043943.1837186-1-clg@kaod.org/

Changes in v3:

  - renamed "addr" property to "cs"

Changes in v2:

  - changed "addr" property to a uint8_t
  - renamed "uart" machine option to "bmc-console" 

Cédric Le Goater (7):
  hw/ssi: Add a "cs" property to SSIPeripheral
  hw/ssi: Introduce a ssi_get_cs() helper
  aspeed/smc: Wire CS lines at reset
  hw/ssi: Check for duplicate CS indexes
  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

 include/hw/block/flash.h            |  4 +++
 include/hw/ssi/ssi.h                |  5 ++++
 hw/arm/aspeed.c                     | 19 +++++++------
 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/ssi/aspeed_smc.c                 |  8 ++++++
 hw/ssi/ssi.c                        | 43 +++++++++++++++++++++++++++++
 11 files changed, 84 insertions(+), 10 deletions(-)

-- 
2.41.0

[PATCH v3 1/7] hw/ssi: Add a "cs" 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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 6950f86810d3..c5bdf1f2165f 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 index */
+    uint8_t cs_index;
 };
 
 extern const VMStateDescription vmstate_ssi_peripheral;
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index d54a109beeb5..4e33e0ea5bc2 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_UINT8("cs", SSIPeripheral, cs_index, 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.41.0

[PATCH v3 2/7] 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
CS index. It will be useful for the board to wire the CS lines.

Cc: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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 c5bdf1f2165f..3cdcbd539042 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, uint8_t cs_index);
+
 #endif
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 4e33e0ea5bc2..54ca3c34e9d0 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, uint8_t cs_index)
+{
+    BusState *b = BUS(bus);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &b->children, sibling) {
+        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
+        if (kid_ssi->cs_index == cs_index) {
+            return kid->child;
+        }
+    }
+
+    return NULL;
+}
+
 static const TypeInfo ssi_bus_info = {
     .name = TYPE_SSI_BUS,
     .parent = TYPE_BUS,
-- 
2.41.0

[PATCH v3 3/7] 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,cs=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.

Reviewed-by: Joel Stanley <joel@jms.id.au>
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 b922a2c491cc..cd92cf9ce0bb 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -307,17 +307,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_uint8(dev, "cs", 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 72811693224d..2a4001b774a2 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.41.0

[PATCH v3 4/7] hw/ssi: Check for duplicate CS indexes

[Cédric Le Goater]

This to avoid indexes 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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                        | 21 +++++++++++++++++++++
 6 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index f7e99baf6236..5a3106e00939 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_uint8(ssddev, "cs", 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 3190cc0b8dbc..8dc2ea83a93b 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_uint8(flash_dev, "cs", 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 1ee2b8697fe2..88c561ff6328 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -740,6 +740,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_uint8(flash_dev, "cs", 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 4c84bb932aa0..21483f75fd93 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_uint8(flash_dev, "cs", 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_uint8(flash_dev, "cs", 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 babb05303520..ea0fb68cf026 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_uint8(dev, "cs", 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 54ca3c34e9d0..1f3e540ab8a1 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -42,10 +42,31 @@ DeviceState *ssi_get_cs(SSIBus *bus, uint8_t cs_index)
     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->cs_index)) {
+        error_setg(errp, "CS index '0x%x' in use by a %s device", s->cs_index,
+                   object_get_typename(OBJECT(dev)));
+        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.41.0

[PATCH v3 5/7] 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,cs=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 cd92cf9ce0bb..271512ce5ced 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -396,12 +396,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.41.0

Re: [PATCH v3 5/7] aspeed: Create flash devices only when defaults are enabled

[Joel Stanley]

On Thu, 31 Aug 2023 at 12:39, Cédric Le Goater <clg@kaod.org> wrote:
>
> When the -nodefaults option is set, flash devices should be created
> with :
>
>     -blockdev node-name=fmc0,driver=file,filename=./flash.img \
>     -device mx66u51235f,cs=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>

I think this is the first foray for the aspeed machines into
nodefaults removing things that previously would have just worked. I
know we haven't had it in our recommended command lines for a long
time, so that's fine.

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

Should the content of your commit message go in the docs?

> ---
>  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 cd92cf9ce0bb..271512ce5ced 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -396,12 +396,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.41.0
>

Re: [PATCH v3 5/7] aspeed: Create flash devices only when defaults are enabled

[Cédric Le Goater]

On 8/31/23 15:00, Joel Stanley wrote:
> On Thu, 31 Aug 2023 at 12:39, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> When the -nodefaults option is set, flash devices should be created
>> with :
>>
>>      -blockdev node-name=fmc0,driver=file,filename=./flash.img \
>>      -device mx66u51235f,cs=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>
> 
> I think this is the first foray for the aspeed machines into
> nodefaults removing things that previously would have just worked.

This is true. It is change from the previous behavior.

QEMU should probably complain if no fmc0 are found to boot from.
Would that be ok ? And yes, documentation needs some update.

Thanks,

C.

> I
> know we haven't had it in our recommended command lines for a long
> time, so that's fine.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Should the content of your commit message go in the docs?
> 
>> ---
>>   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 cd92cf9ce0bb..271512ce5ced 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -396,12 +396,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.41.0
>>

Re: [PATCH v3 5/7] aspeed: Create flash devices only when defaults are enabled

[Joel Stanley]

On Thu, 31 Aug 2023 at 13:22, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/31/23 15:00, Joel Stanley wrote:
> > On Thu, 31 Aug 2023 at 12:39, Cédric Le Goater <clg@kaod.org> wrote:
> >>
> >> When the -nodefaults option is set, flash devices should be created
> >> with :
> >>
> >>      -blockdev node-name=fmc0,driver=file,filename=./flash.img \
> >>      -device mx66u51235f,cs=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>
> >
> > I think this is the first foray for the aspeed machines into
> > nodefaults removing things that previously would have just worked.
>
> This is true. It is change from the previous behavior.
>
> QEMU should probably complain if no fmc0 are found to boot from.
> Would that be ok ? And yes, documentation needs some update.

I didn't consider warning. That would help users who blindly added
-nodefaults and wondered why nothing was happening.

This is what happens if you add -nodefaults to an "old" command line
with your patch applied:

$ ./build/qemu-system-arm -M rainier-bmc -nographic -nodefaults
-serial stdio -drive
file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw
qemu-system-arm: -drive
file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw: machine
type does not support if=mtd,bus=0,unit=0

Which at least isn't sitting there spinning, as I was worried. I'll
leave it to you as to whether it needs a helpful message.

Cheers,

Joel



>
> Thanks,
>
> C.
>
> > I
> > know we haven't had it in our recommended command lines for a long
> > time, so that's fine.
> >
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> >
> > Should the content of your commit message go in the docs?
> >
> >> ---
> >>   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 cd92cf9ce0bb..271512ce5ced 100644
> >> --- a/hw/arm/aspeed.c
> >> +++ b/hw/arm/aspeed.c
> >> @@ -396,12 +396,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.41.0
> >>
>

Re: [PATCH v3 5/7] aspeed: Create flash devices only when defaults are enabled

[Cédric Le Goater]

On 8/31/23 15:42, Joel Stanley wrote:
> On Thu, 31 Aug 2023 at 13:22, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 8/31/23 15:00, Joel Stanley wrote:
>>> On Thu, 31 Aug 2023 at 12:39, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> When the -nodefaults option is set, flash devices should be created
>>>> with :
>>>>
>>>>       -blockdev node-name=fmc0,driver=file,filename=./flash.img \
>>>>       -device mx66u51235f,cs=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>
>>>
>>> I think this is the first foray for the aspeed machines into
>>> nodefaults removing things that previously would have just worked.
>>
>> This is true. It is change from the previous behavior.
>>
>> QEMU should probably complain if no fmc0 are found to boot from.
>> Would that be ok ? And yes, documentation needs some update.
> 
> I didn't consider warning. That would help users who blindly added
> -nodefaults and wondered why nothing was happening.
> 
> This is what happens if you add -nodefaults to an "old" command line
> with your patch applied:
> 
> $ ./build/qemu-system-arm -M rainier-bmc -nographic -nodefaults
> -serial stdio -drive
> file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw
> qemu-system-arm: -drive
> file=obmc-phosphor-image-rainier.static.mtd,if=mtd,format=raw: machine
> type does not support if=mtd,bus=0,unit=0

yes that's a post board init sanity check on unused drives.
  
> Which at least isn't sitting there spinning, as I was worried. I'll
> leave it to you as to whether it needs a helpful message.

It seems difficult since we could be booting the machine from a kernel also.

I will update the documentation.

Thanks,

C.

[PATCH v3.2 5/7] 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,cs=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>
---
  docs/system/arm/aspeed.rst | 35 +++++++++++++++++++++++++++++------
  hw/arm/aspeed.c            |  6 ++++--
  2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 80538422a1a4..b2dea54eedad 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -104,7 +104,7 @@ To boot a kernel directly from a Linux build tree:
          -dtb arch/arm/boot/dts/aspeed-ast2600-evb.dtb \
          -initrd rootfs.cpio
  
-The image should be attached as an MTD drive. Run :
+To boot the machine from the flash image, use an MTD drive :
  
  .. code-block:: bash
  
@@ -117,23 +117,46 @@ Options specific to Aspeed machines are :
     device by using the FMC controller to load the instructions, and
     not simply from RAM. This takes a little longer.
  
- * ``fmc-model`` to change the FMC Flash model. FW needs support for
-   the chip model to boot.
+ * ``fmc-model`` to change the default FMC Flash model. FW needs
+   support for the chip model to boot.
  
- * ``spi-model`` to change the SPI Flash model.
+ * ``spi-model`` to change the default SPI Flash model.
  
   * ``bmc-console`` 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 :
+To use other flash models, for instance a different FMC chip and a
+bigger (64M) SPI for the ``ast2500-evb`` machine, run :
  
  .. code-block:: bash
  
    -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
  
+When more flexibility is needed to define the flash devices, to use
+different flash models or define all flash devices (up to 8), the
+``-nodefaults`` QEMU option can be used to avoid creating the default
+flash devices.
+
+Flash devices should then be created from the command line and attached
+to a block device :
+
+.. code-block:: bash
+
+  $ qemu-system-arm -M ast2600-evb \
+        -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
+	-device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
+	-blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
+	-device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
+	-blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
+	-device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
+	-nographic -nodefaults
+
+In that case, the machine boots fetching instructions from the FMC0
+device. It is slower to start but closer to what HW does. Using the
+machine option ``execute-in-place`` has a similar effect.
+
  To change the boot console and use device ``UART3`` (``/dev/ttyS2``
  under Linux), use :
  
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cd92cf9ce0bb..271512ce5ced 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -396,12 +396,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.41.0

Re: [PATCH v3.2 5/7] aspeed: Create flash devices only when defaults are enabled

[Joel Stanley]

On Thu, 31 Aug 2023 at 21:13, Cédric Le Goater <clg@kaod.org> wrote:
>
> When the -nodefaults option is set, flash devices should be created
> with :
>
>      -blockdev node-name=fmc0,driver=file,filename=./flash.img \
>      -device mx66u51235f,cs=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>

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

A good addition. Thanks!

> ---
>   docs/system/arm/aspeed.rst | 35 +++++++++++++++++++++++++++++------
>   hw/arm/aspeed.c            |  6 ++++--
>   2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index 80538422a1a4..b2dea54eedad 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -104,7 +104,7 @@ To boot a kernel directly from a Linux build tree:
>           -dtb arch/arm/boot/dts/aspeed-ast2600-evb.dtb \
>           -initrd rootfs.cpio
>
> -The image should be attached as an MTD drive. Run :
> +To boot the machine from the flash image, use an MTD drive :
>
>   .. code-block:: bash
>
> @@ -117,23 +117,46 @@ Options specific to Aspeed machines are :
>      device by using the FMC controller to load the instructions, and
>      not simply from RAM. This takes a little longer.
>
> - * ``fmc-model`` to change the FMC Flash model. FW needs support for
> -   the chip model to boot.
> + * ``fmc-model`` to change the default FMC Flash model. FW needs
> +   support for the chip model to boot.
>
> - * ``spi-model`` to change the SPI Flash model.
> + * ``spi-model`` to change the default SPI Flash model.
>
>    * ``bmc-console`` 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 :
> +To use other flash models, for instance a different FMC chip and a
> +bigger (64M) SPI for the ``ast2500-evb`` machine, run :
>
>   .. code-block:: bash
>
>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>
> +When more flexibility is needed to define the flash devices, to use
> +different flash models or define all flash devices (up to 8), the
> +``-nodefaults`` QEMU option can be used to avoid creating the default
> +flash devices.
> +
> +Flash devices should then be created from the command line and attached
> +to a block device :
> +
> +.. code-block:: bash
> +
> +  $ qemu-system-arm -M ast2600-evb \
> +        -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
> +       -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
> +       -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
> +       -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
> +       -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
> +       -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
> +       -nographic -nodefaults
> +
> +In that case, the machine boots fetching instructions from the FMC0
> +device. It is slower to start but closer to what HW does. Using the
> +machine option ``execute-in-place`` has a similar effect.
> +
>   To change the boot console and use device ``UART3`` (``/dev/ttyS2``
>   under Linux), use :
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index cd92cf9ce0bb..271512ce5ced 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -396,12 +396,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.41.0
>
>

[PATCH v3 6/7] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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 7198953702b7..de93756cbe8f 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 dc5ffbc4ff52..afc3fdf4d60b 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.41.0

[PATCH v3 7/7] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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 271512ce5ced..f8ba67531a00 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"
@@ -436,11 +437,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.41.0