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

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
>

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
>>

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
> >>
>

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.

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

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
>
>