[PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Cédric Le Goater]

Hello,

This series starts with a first set of patches fixing I2C slave mode
in the Aspeed I2C controller, a test device and its associated test in
avocado.

Follow some cleanups which allow the use of block devices instead of
drives. So that, instead of specifying :

  -drive file=./flash-ast2600-evb,format=raw,if=mtd
  -drive file=./ast2600-evb.pnor,format=raw,if=mtd
  ...

and guessing from the order which bus the device is attached to, we
can use :

  -blockdev node-name=fmc0,driver=file,filename=./bmc.img
  -device mx66u51235f,bus=ssi.0,drive=fmc0
  -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
  -device mx66u51235f,bus=ssi.0,drive=fmc1 
  -blockdev node-name=pnor,driver=file,filename=./pnor
  -device mx66l1g45g,bus=ssi.1,drive=pnor
  ...

It is not perfect, the CS index still depends on the order, but it is
now possible to run a machine without -drive ...,if=mtd.

This lacks the final patch enabling the '-nodefaults' option by not
creating the default devices if specified on the command line. It
needs some more evaluation of the possible undesired effects. 
Thanks,

C.

Cédric Le Goater (6):
  m25p80: Improve error when the backend file size does not match the
    device
  tests/avocado/machine_aspeed.py: Add I2C slave tests
  aspeed/smc: Replace SysBus IRQs with GPIO lines
  aspeed/smc: Wire CS lines at reset
  aspeed: Introduce a spi_boot region under the SoC
  aspeed: Add a boot_rom overlap region in the SoC spi_boot container

Klaus Jensen (2):
  hw/i2c: only schedule pending master when bus is idle
  hw/misc: add a toy i2c echo device

 include/hw/arm/aspeed_soc.h     |   3 +
 include/hw/i2c/i2c.h            |   2 +
 hw/arm/aspeed.c                 |  60 ++++++------
 hw/arm/aspeed_ast2600.c         |  13 +++
 hw/arm/aspeed_soc.c             |  14 +++
 hw/arm/fby35.c                  |   8 +-
 hw/block/m25p80.c               |   4 +-
 hw/i2c/aspeed_i2c.c             |   2 +
 hw/i2c/core.c                   |  37 +++++---
 hw/misc/i2c-echo.c              | 156 ++++++++++++++++++++++++++++++++
 hw/ssi/aspeed_smc.c             |  29 +++++-
 hw/misc/meson.build             |   2 +
 tests/avocado/machine_aspeed.py |  10 ++
 13 files changed, 279 insertions(+), 61 deletions(-)
 create mode 100644 hw/misc/i2c-echo.c

-- 
2.39.1

[PATCH 1/8] m25p80: Improve error when the backend file size does not match the device

[Cédric Le Goater]

Currently, when a block backend is attached to a m25p80 device and the
associated file size does not match the flash model, QEMU complains
with the error message "failed to read the initial flash content".
This is confusing for the user.

Use blk_check_size_and_read_all() instead of blk_pread() to improve
the reported error.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Delevoryas <peter@pjd.dev>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20221115151000.2080833-1-clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

  breakage with commit a4b15a8b9e ("pflash: Only read non-zero parts
  of backend image") when using -snaphot.

 hw/block/m25p80.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 802d2eb021..dc5ffbc4ff 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -24,6 +24,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "sysemu/block-backend.h"
+#include "hw/block/block.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/ssi/ssi.h"
@@ -1615,8 +1616,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
         trace_m25p80_binding(s);
         s->storage = blk_blockalign(s->blk, s->size);
 
-        if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
-            error_setg(errp, "failed to read the initial flash content");
+        if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
             return;
         }
     } else {
-- 
2.39.1

Re: [PATCH 1/8] m25p80: Improve error when the backend file size does not match the device

[Peter Delevoryas]

On Tue, Feb 14, 2023 at 06:18:23PM +0100, Cédric Le Goater wrote:
> Currently, when a block backend is attached to a m25p80 device and the
> associated file size does not match the flash model, QEMU complains
> with the error message "failed to read the initial flash content".
> This is confusing for the user.
> 
> Use blk_check_size_and_read_all() instead of blk_pread() to improve
> the reported error.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Message-Id: <20221115151000.2080833-1-clg@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>   breakage with commit a4b15a8b9e ("pflash: Only read non-zero parts
>   of backend image") when using -snaphot.


I guess it's not obvious to me, what broke?

1. BlockBackend *blk = -drive file=blob.mtd,format=raw,if=mtd,snapshot=on
2. uint8_t *storage = malloc(...)
3. blk_check_size_and_read_all(blk, storage, size)
4. commit a4b15a8b9e 
    for sector in blk:
        if any(b != 0 for b in sector):
            memcpy(&storage[...], sector, sizeof(sector))
        else:
            # Skip memcpy of zeroes

But this is probably a regression for us because we actually expect the zeroes
to be copied?

- Peter

> 
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 802d2eb021..dc5ffbc4ff 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -24,6 +24,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
>  #include "sysemu/block-backend.h"
> +#include "hw/block/block.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
>  #include "hw/ssi/ssi.h"
> @@ -1615,8 +1616,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>          trace_m25p80_binding(s);
>          s->storage = blk_blockalign(s->blk, s->size);
>  
> -        if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
> -            error_setg(errp, "failed to read the initial flash content");
> +        if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
>              return;
>          }
>      } else {
> -- 
> 2.39.1
> 

Re: [PATCH 1/8] m25p80: Improve error when the backend file size does not match the device

[Philippe Mathieu-Daudé]

On 15/2/23 20:52, Peter Delevoryas wrote:
> On Tue, Feb 14, 2023 at 06:18:23PM +0100, Cédric Le Goater wrote:
>> Currently, when a block backend is attached to a m25p80 device and the
>> associated file size does not match the flash model, QEMU complains
>> with the error message "failed to read the initial flash content".
>> This is confusing for the user.
>>
>> Use blk_check_size_and_read_all() instead of blk_pread() to improve
>> the reported error.

Could you reference commit 06f1521795?

>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Peter Delevoryas <peter@pjd.dev>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Message-Id: <20221115151000.2080833-1-clg@kaod.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>    breakage with commit a4b15a8b9e ("pflash: Only read non-zero parts
>>    of backend image") when using -snaphot.
> 
> 
> I guess it's not obvious to me, what broke?
> 
> 1. BlockBackend *blk = -drive file=blob.mtd,format=raw,if=mtd,snapshot=on
> 2. uint8_t *storage = malloc(...)
> 3. blk_check_size_and_read_all(blk, storage, size)
> 4. commit a4b15a8b9e
>      for sector in blk:
>          if any(b != 0 for b in sector):
>              memcpy(&storage[...], sector, sizeof(sector))
>          else:
>              # Skip memcpy of zeroes
> 
> But this is probably a regression for us because we actually expect the zeroes
> to be copied?

Yes... Commit a4b15a8b9e mostly considered cloud payload optimization.

Since EEPROMs are usually "small", this could be kludged as:

-- >8 --
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1615,6 +1615,7 @@ static void m25p80_realize(SSIPeripheral *ss, 
Error **errp)
          trace_m25p80_binding(s);
          s->storage = blk_blockalign(s->blk, s->size);

+        memset(s->storage, ERASED_BYTE, s->size);
          if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
              error_setg(errp, "failed to read the initial flash content");
              return;
---

On emulation, it is simpler to ask the user to provide a block image
with the same size of the emulated device. See commit 06f1521795
("pflash: Require backend size to match device, improve errors") for
more information.

>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 802d2eb021..dc5ffbc4ff 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -24,6 +24,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/units.h"
>>   #include "sysemu/block-backend.h"
>> +#include "hw/block/block.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-properties-system.h"
>>   #include "hw/ssi/ssi.h"
>> @@ -1615,8 +1616,7 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>>           trace_m25p80_binding(s);
>>           s->storage = blk_blockalign(s->blk, s->size);
>>   
>> -        if (blk_pread(s->blk, 0, s->size, s->storage, 0) < 0) {
>> -            error_setg(errp, "failed to read the initial flash content");
>> +        if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
>>               return;
>>           }
>>       } else {
>> -- 
>> 2.39.1
>>

[PATCH 2/8] hw/i2c: only schedule pending master when bus is idle

[Cédric Le Goater]

From: Klaus Jensen <k.jensen@samsung.com>

It is not given that the current master will release the bus after a
transfer ends. Only schedule a pending master if the bus is idle.

Fixes: 37fa5ca42623 ("hw/i2c: support multiple masters")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Acked-by: Corey Minyard <cminyard@mvista.com>
Message-Id: <20221116084312.35808-2-its@irrelevant.dk>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/i2c/i2c.h |  2 ++
 hw/i2c/aspeed_i2c.c  |  2 ++
 hw/i2c/core.c        | 37 ++++++++++++++++++++++---------------
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b9581d230..2a3abacd1b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -141,6 +141,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
  */
 int i2c_start_send_async(I2CBus *bus, uint8_t address);
 
+void i2c_schedule_pending_master(I2CBus *bus);
+
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
 void i2c_ack(I2CBus *bus);
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c166fd20fa..1f071a3811 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -550,6 +550,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         }
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_STOP_CMD, 0);
         aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+        i2c_schedule_pending_master(bus->bus);
     }
 
     if (aspeed_i2c_bus_pkt_mode_en(bus)) {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d4ba8146bf..bed594fe59 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -185,22 +185,39 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
 
 void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
 {
-    if (i2c_bus_busy(bus)) {
-        I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
-        node->bh = bh;
+    I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+    node->bh = bh;
+
+    QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+}
+
+void i2c_schedule_pending_master(I2CBus *bus)
+{
+    I2CPendingMaster *node;
 
-        QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+    if (i2c_bus_busy(bus)) {
+        /* someone is already controlling the bus; wait for it to release it */
+        return;
+    }
 
+    if (QSIMPLEQ_EMPTY(&bus->pending_masters)) {
         return;
     }
 
-    bus->bh = bh;
+    node = QSIMPLEQ_FIRST(&bus->pending_masters);
+    bus->bh = node->bh;
+
+    QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
+    g_free(node);
+
     qemu_bh_schedule(bus->bh);
 }
 
 void i2c_bus_release(I2CBus *bus)
 {
     bus->bh = NULL;
+
+    i2c_schedule_pending_master(bus);
 }
 
 int i2c_start_recv(I2CBus *bus, uint8_t address)
@@ -234,16 +251,6 @@ void i2c_end_transfer(I2CBus *bus)
         g_free(node);
     }
     bus->broadcast = false;
-
-    if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
-        I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
-        bus->bh = node->bh;
-
-        QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
-        g_free(node);
-
-        qemu_bh_schedule(bus->bh);
-    }
 }
 
 int i2c_send(I2CBus *bus, uint8_t data)
-- 
2.39.1

[PATCH 3/8] hw/misc: add a toy i2c echo device

[Cédric Le Goater]

From: Klaus Jensen <k.jensen@samsung.com>

Add an example I2C device to demonstrate how a slave may master the bus
and send data asynchronously to another slave.

The device will echo whatever it is sent to the device identified by the
first byte received.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
[ clg: - Changed to build to use CONFIG_ASPEED_SOC since only supported
         on such SoCs
       - folded in these fixes :
       	 https://lore.kernel.org/qemu-devel/Y3yMKAhOkYGtnkOp@cormorant.local/
]
Message-Id: <20220601210831.67259-7-its@irrelevant.dk>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/i2c-echo.c  | 156 ++++++++++++++++++++++++++++++++++++++++++++
 hw/misc/meson.build |   2 +
 2 files changed, 158 insertions(+)
 create mode 100644 hw/misc/i2c-echo.c

diff --git a/hw/misc/i2c-echo.c b/hw/misc/i2c-echo.c
new file mode 100644
index 0000000000..5705ab5d73
--- /dev/null
+++ b/hw/misc/i2c-echo.c
@@ -0,0 +1,156 @@
+#include "qemu/osdep.h"
+#include "qemu/timer.h"
+#include "qemu/main-loop.h"
+#include "block/aio.h"
+#include "hw/i2c/i2c.h"
+
+#define TYPE_I2C_ECHO "i2c-echo"
+OBJECT_DECLARE_SIMPLE_TYPE(I2CEchoState, I2C_ECHO)
+
+enum i2c_echo_state {
+    I2C_ECHO_STATE_IDLE,
+    I2C_ECHO_STATE_START_SEND,
+    I2C_ECHO_STATE_ACK,
+};
+
+typedef struct I2CEchoState {
+    I2CSlave parent_obj;
+
+    I2CBus *bus;
+
+    enum i2c_echo_state state;
+    QEMUBH *bh;
+
+    unsigned int pos;
+    uint8_t data[3];
+} I2CEchoState;
+
+static void i2c_echo_bh(void *opaque)
+{
+    I2CEchoState *state = opaque;
+
+    switch (state->state) {
+    case I2C_ECHO_STATE_IDLE:
+        return;
+
+    case I2C_ECHO_STATE_START_SEND:
+        if (i2c_start_send_async(state->bus, state->data[0])) {
+            goto release_bus;
+        }
+
+        state->pos++;
+        state->state = I2C_ECHO_STATE_ACK;
+        return;
+
+    case I2C_ECHO_STATE_ACK:
+        if (state->pos > 2) {
+            break;
+        }
+
+        if (i2c_send_async(state->bus, state->data[state->pos++])) {
+            break;
+        }
+
+        return;
+    }
+
+
+    i2c_end_transfer(state->bus);
+release_bus:
+    i2c_bus_release(state->bus);
+
+    state->state = I2C_ECHO_STATE_IDLE;
+}
+
+static int i2c_echo_event(I2CSlave *s, enum i2c_event event)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    switch (event) {
+    case I2C_START_RECV:
+        state->pos = 0;
+
+        break;
+
+    case I2C_START_SEND:
+        state->pos = 0;
+
+        break;
+
+    case I2C_FINISH:
+        state->pos = 0;
+        state->state = I2C_ECHO_STATE_START_SEND;
+        i2c_bus_master(state->bus, state->bh);
+
+        break;
+
+    case I2C_NACK:
+        break;
+
+    default:
+        return -1;
+    }
+
+    return 0;
+}
+
+static uint8_t i2c_echo_recv(I2CSlave *s)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    if (state->pos > 2) {
+        return 0xff;
+    }
+
+    return state->data[state->pos++];
+}
+
+static int i2c_echo_send(I2CSlave *s, uint8_t data)
+{
+    I2CEchoState *state = I2C_ECHO(s);
+
+    if (state->pos > 2) {
+        return -1;
+    }
+
+    state->data[state->pos++] = data;
+
+    return 0;
+}
+
+static void i2c_echo_realize(DeviceState *dev, Error **errp)
+{
+    I2CEchoState *state = I2C_ECHO(dev);
+    BusState *bus = qdev_get_parent_bus(dev);
+
+    state->bus = I2C_BUS(bus);
+    state->bh = qemu_bh_new(i2c_echo_bh, state);
+
+    return;
+}
+
+static void i2c_echo_class_init(ObjectClass *oc, void *data)
+{
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = i2c_echo_realize;
+
+    sc->event = i2c_echo_event;
+    sc->recv = i2c_echo_recv;
+    sc->send = i2c_echo_send;
+}
+
+static const TypeInfo i2c_echo = {
+    .name = TYPE_I2C_ECHO,
+    .parent = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(I2CEchoState),
+    .class_init = i2c_echo_class_init,
+};
+
+static void register_types(void)
+{
+    type_register_static(&i2c_echo);
+}
+
+type_init(register_types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 448e14b531..3eb1bda710 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -129,6 +129,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
 
 softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
 
+softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('i2c-echo.c'))
+
 specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
 
 specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c'))
-- 
2.39.1

Re: [PATCH 3/8] hw/misc: add a toy i2c echo device

[Philippe Mathieu-Daudé]

On 14/2/23 18:18, Cédric Le Goater wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add an example I2C device to demonstrate how a slave may master the bus
> and send data asynchronously to another slave.

What a rebellion...

> The device will echo whatever it is sent to the device identified by the
> first byte received.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> [ clg: - Changed to build to use CONFIG_ASPEED_SOC since only supported
>           on such SoCs
>         - folded in these fixes :
>         	 https://lore.kernel.org/qemu-devel/Y3yMKAhOkYGtnkOp@cormorant.local/
> ]
> Message-Id: <20220601210831.67259-7-its@irrelevant.dk>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/misc/i2c-echo.c  | 156 ++++++++++++++++++++++++++++++++++++++++++++
>   hw/misc/meson.build |   2 +
>   2 files changed, 158 insertions(+)
>   create mode 100644 hw/misc/i2c-echo.c


> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 448e14b531..3eb1bda710 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -129,6 +129,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>   
>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>   
> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('i2c-echo.c'))

s/CONFIG_ASPEED_SOC/CONFIG_I2C/ since this is a generic device.

>   specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
>   
>   specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c'))

Re: [PATCH 3/8] hw/misc: add a toy i2c echo device

[Cédric Le Goater]

On 2/15/23 11:55, Philippe Mathieu-Daudé wrote:
> On 14/2/23 18:18, Cédric Le Goater wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> Add an example I2C device to demonstrate how a slave may master the bus
>> and send data asynchronously to another slave.
> 
> What a rebellion...
> 
>> The device will echo whatever it is sent to the device identified by the
>> first byte received.
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> [ clg: - Changed to build to use CONFIG_ASPEED_SOC since only supported
>>           on such SoCs
>>         - folded in these fixes :
>>              https://lore.kernel.org/qemu-devel/Y3yMKAhOkYGtnkOp@cormorant.local/
>> ]
>> Message-Id: <20220601210831.67259-7-its@irrelevant.dk>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/misc/i2c-echo.c  | 156 ++++++++++++++++++++++++++++++++++++++++++++
>>   hw/misc/meson.build |   2 +
>>   2 files changed, 158 insertions(+)
>>   create mode 100644 hw/misc/i2c-echo.c
> 
> 
>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>> index 448e14b531..3eb1bda710 100644
>> --- a/hw/misc/meson.build
>> +++ b/hw/misc/meson.build
>> @@ -129,6 +129,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('i2c-echo.c'))
> 
> s/CONFIG_ASPEED_SOC/CONFIG_I2C/ since this is a generic device.

even if only supported by the Aspeed SoC ? I am OK with both.


> 
>>   specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
>>   specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c'))
> 

Re: [PATCH 3/8] hw/misc: add a toy i2c echo device

[Philippe Mathieu-Daudé]

On 15/2/23 12:09, Cédric Le Goater wrote:
> On 2/15/23 11:55, Philippe Mathieu-Daudé wrote:
>> On 14/2/23 18:18, Cédric Le Goater wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Add an example I2C device to demonstrate how a slave may master the bus
>>> and send data asynchronously to another slave.
>>
>> What a rebellion...
>>
>>> The device will echo whatever it is sent to the device identified by the
>>> first byte received.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> [ clg: - Changed to build to use CONFIG_ASPEED_SOC since only supported
>>>           on such SoCs
>>>         - folded in these fixes :
>>>              
>>> https://lore.kernel.org/qemu-devel/Y3yMKAhOkYGtnkOp@cormorant.local/
>>> ]
>>> Message-Id: <20220601210831.67259-7-its@irrelevant.dk>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   hw/misc/i2c-echo.c  | 156 ++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/misc/meson.build |   2 +
>>>   2 files changed, 158 insertions(+)
>>>   create mode 100644 hw/misc/i2c-echo.c
>>
>>
>>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>>> index 448e14b531..3eb1bda710 100644
>>> --- a/hw/misc/meson.build
>>> +++ b/hw/misc/meson.build
>>> @@ -129,6 +129,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: 
>>> files('nrf51_rng.c'))
>>>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: 
>>> files('grlib_ahb_apb_pnp.c'))
>>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('i2c-echo.c'))
>>
>> s/CONFIG_ASPEED_SOC/CONFIG_I2C/ since this is a generic device.
> 
> even if only supported by the Aspeed SoC ? I am OK with both.

Any machine exposing an i2c bus can use this device, isn't it?

   -device i2c-echo,bus=bus69,address=0x42 ...

Re: [PATCH 3/8] hw/misc: add a toy i2c echo device

[Cédric Le Goater]

On 2/15/23 13:26, Philippe Mathieu-Daudé wrote:
> On 15/2/23 12:09, Cédric Le Goater wrote:
>> On 2/15/23 11:55, Philippe Mathieu-Daudé wrote:
>>> On 14/2/23 18:18, Cédric Le Goater wrote:
>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>
>>>> Add an example I2C device to demonstrate how a slave may master the bus
>>>> and send data asynchronously to another slave.
>>>
>>> What a rebellion...
>>>
>>>> The device will echo whatever it is sent to the device identified by the
>>>> first byte received.
>>>>
>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>>> [ clg: - Changed to build to use CONFIG_ASPEED_SOC since only supported
>>>>           on such SoCs
>>>>         - folded in these fixes :
>>>> https://lore.kernel.org/qemu-devel/Y3yMKAhOkYGtnkOp@cormorant.local/
>>>> ]
>>>> Message-Id: <20220601210831.67259-7-its@irrelevant.dk>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>   hw/misc/i2c-echo.c  | 156 ++++++++++++++++++++++++++++++++++++++++++++
>>>>   hw/misc/meson.build |   2 +
>>>>   2 files changed, 158 insertions(+)
>>>>   create mode 100644 hw/misc/i2c-echo.c
>>>
>>>
>>>> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
>>>> index 448e14b531..3eb1bda710 100644
>>>> --- a/hw/misc/meson.build
>>>> +++ b/hw/misc/meson.build
>>>> @@ -129,6 +129,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
>>>>   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
>>>> +softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('i2c-echo.c'))
>>>
>>> s/CONFIG_ASPEED_SOC/CONFIG_I2C/ since this is a generic device.
>>
>> even if only supported by the Aspeed SoC ? I am OK with both.
> 
> Any machine exposing an i2c bus can use this device, isn't it?
> 
>    -device i2c-echo,bus=bus69,address=0x42 ...

Would you have a machine with I2C buses and image to try that on ?
Not an aspeed one obvioulsy

Thanks,

C.

[PATCH 4/8] tests/avocado/machine_aspeed.py: Add I2C slave tests

[Cédric Le Goater]

Test extracted from :

  https://lists.nongnu.org/archive/html/qemu-devel/2022-06/msg00183.html

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 tests/avocado/machine_aspeed.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index ddf05b3617..d2c57ccb7e 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -199,6 +199,8 @@ def test_arm_ast2600_evb_buildroot(self):
                          'tmp105,bus=aspeed.i2c.bus.3,address=0x4d,id=tmp-test');
         self.vm.add_args('-device',
                          'ds1338,bus=aspeed.i2c.bus.3,address=0x32');
+        self.vm.add_args('-device',
+                         'i2c-echo,bus=aspeed.i2c.bus.3,address=0x42');
         self.do_test_arm_aspeed_buildroot_start(image_path, '0xf00')
 
         exec_command_and_wait_for_pattern(self,
@@ -217,6 +219,14 @@ def test_arm_ast2600_evb_buildroot(self):
         year = time.strftime("%Y")
         exec_command_and_wait_for_pattern(self, 'hwclock -f /dev/rtc1', year);
 
+        exec_command_and_wait_for_pattern(self,
+             'echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-3/new_device',
+             'i2c i2c-3: new_device: Instantiated device slave-24c02 at 0x64');
+        exec_command(self, 'i2cset -y 3 0x42 0x64 0x00 0xaa i');
+        time.sleep(0.1)
+        exec_command_and_wait_for_pattern(self,
+             'hexdump /sys/bus/i2c/devices/3-1064/slave-eeprom',
+             '0000000 ffaa ffff ffff ffff ffff ffff ffff ffff');
         self.do_test_arm_aspeed_buildroot_poweroff()
 
 
-- 
2.39.1

[PATCH 5/8] aspeed/smc: Replace SysBus IRQs with GPIO lines

[Cédric Le Goater]

It's cleaner and removes the curious '+ 1' required to skip the DMA
IRQ line of the controller.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 27dda58338..7c28546d7f 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -293,7 +293,7 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
         qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
-        sysbus_connect_irq(SYS_BUS_DEVICE(s), i + 1, cs_line);
+        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 22df4be528..7281169322 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1134,10 +1134,7 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
     /* Setup cs_lines for peripherals */
     s->cs_lines = g_new0(qemu_irq, asc->cs_num_max);
-
-    for (i = 0; i < asc->cs_num_max; ++i) {
-        sysbus_init_irq(sbd, &s->cs_lines[i]);
-    }
+    qdev_init_gpio_out_named(DEVICE(s), s->cs_lines, "cs", asc->cs_num_max);
 
     /* The memory region for the controller registers */
     memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
-- 
2.39.1

Re: [PATCH 5/8] aspeed/smc: Replace SysBus IRQs with GPIO lines

[Philippe Mathieu-Daudé]

On 14/2/23 18:18, Cédric Le Goater wrote:
> It's cleaner and removes the curious '+ 1' required to skip the DMA
> IRQ line of the controller.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c     | 2 +-
>   hw/ssi/aspeed_smc.c | 5 +----
>   2 files changed, 2 insertions(+), 5 deletions(-)

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

[PATCH 6/8] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

It has become difficult to define on the command line the flash
devices of the Aspeed machines and their file backend. Currently, a
set of default flash devices is created at machine init and drives are
associated to the FMC and SPI controller devices 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.

On real systems, these flash devices are sometime soldered to the
board but the models can be different or a socket is provided to
replace the flash device. So, it is legitimate to not consider them as
always available by default. Some machine options were provided to
specify different models, but this has its limits and the best
approach would be to allow the use of block devices, such as :

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

The first step in that direction is to wire the CS lines of all
available devices on a bus at reset time. Let's do that and check the
maximum number of devices supported by the bus while at it. The bus
parent can now be explicitly defined but the device order still
depends on the command line definitions.

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 7c28546d7f..21184f3ad4 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -283,7 +283,6 @@ 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);
@@ -291,9 +290,6 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
             qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
         }
         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..412cf125d9 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -680,6 +680,28 @@ static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value)
     aspeed_smc_flash_do_select(fl, unselect);
 }
 
+/*
+ * TODO: assumption is made on the order of creation of devices, the
+ * ones on the command line or the default devices created at machine
+ * init.
+ */
+static void aspeed_smc_wire_cs_lines(AspeedSMCState *s, int cs_max)
+{
+    BusState *b = BUS(s->spi);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &b->children, sibling) {
+        qemu_irq cs_line = qdev_get_gpio_in_named(kid->child, SSI_GPIO_CS, 0);
+        if (kid->index < cs_max) {
+            qdev_connect_gpio_out_named(DEVICE(s), "cs", kid->index, cs_line);
+        } else {
+            warn_report("Too many devices for SSI bus %s",
+                        object_class_get_name(object_get_class(OBJECT(s))));
+            return;
+        }
+    }
+}
+
 static void aspeed_smc_reset(DeviceState *d)
 {
     AspeedSMCState *s = ASPEED_SMC(d);
@@ -692,6 +714,8 @@ static void aspeed_smc_reset(DeviceState *d)
         memset(s->regs, 0, sizeof s->regs);
     }
 
+    aspeed_smc_wire_cs_lines(s, asc->cs_num_max);
+
     /* Unselect all peripherals */
     for (i = 0; i < asc->cs_num_max; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
-- 
2.39.1

[PATCH 7/8] aspeed: Introduce a spi_boot region under the SoC

[Cédric Le Goater]

The default boot of the Aspeed SoCs is address 0x0. For this reason,
the FMC flash device contents are remapped by HW on the first 256MB of
the address space. In QEMU, this is currently done in the machine init
with the setup of a region alias.

Move this code to the SoC and introduce an extra container to prepare
ground for the boot ROM region which will overlap the FMC flash
remapping.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed_soc.h |  3 +++
 hw/arm/aspeed.c             | 13 +------------
 hw/arm/aspeed_ast2600.c     | 13 +++++++++++++
 hw/arm/aspeed_soc.c         | 14 ++++++++++++++
 hw/arm/fby35.c              |  8 +-------
 5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index bd1e03e78a..db55505d14 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -58,6 +58,8 @@ struct AspeedSoCState {
     MemoryRegion *dram_mr;
     MemoryRegion dram_container;
     MemoryRegion sram;
+    MemoryRegion spi_boot_container;
+    MemoryRegion spi_boot;
     AspeedVICState vic;
     AspeedRtcState rtc;
     AspeedTimerCtrlState timerctrl;
@@ -120,6 +122,7 @@ struct AspeedSoCClass {
 
 
 enum {
+    ASPEED_DEV_SPI_BOOT,
     ASPEED_DEV_IOMEM,
     ASPEED_DEV_UART1,
     ASPEED_DEV_UART2,
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 21184f3ad4..998dc57969 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -384,18 +384,7 @@ static void aspeed_machine_init(MachineState *machine)
         MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
         uint64_t size = memory_region_size(&fl->mmio);
 
-        /*
-         * create a ROM region using the default mapping window size of
-         * the flash module. The window size is 64MB for the AST2400
-         * SoC and 128MB for the AST2500 SoC, which is twice as big as
-         * needed by the flash modules of the Aspeed machines.
-         */
-        if (ASPEED_MACHINE(machine)->mmio_exec) {
-            memory_region_init_alias(boot_rom, NULL, "aspeed.boot_rom",
-                                     &fl->mmio, 0, size);
-            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
-                                        boot_rom);
-        } else {
+        if (!ASPEED_MACHINE(machine)->mmio_exec) {
             memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
                                    size, &error_abort);
             memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bb2769df04..20f0b772d6 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -21,6 +21,7 @@
 #define ASPEED_SOC_DPMCU_SIZE       0x00040000
 
 static const hwaddr aspeed_soc_ast2600_memmap[] = {
+    [ASPEED_DEV_SPI_BOOT]  = 0x0,
     [ASPEED_DEV_SRAM]      = 0x10000000,
     [ASPEED_DEV_DPMCU]     = 0x18000000,
     /* 0x16000000     0x17FFFFFF : AHB BUS do LPC Bus bridge */
@@ -282,6 +283,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     qemu_irq irq;
     g_autofree char *sram_name = NULL;
 
+    /* Default boot region (SPI memory or ROMs) */
+    memory_region_init(&s->spi_boot_container, OBJECT(s),
+                       "aspeed.spi_boot_container", 0x10000000);
+    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SPI_BOOT],
+                                &s->spi_boot_container);
+
     /* IO space */
     aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->iomem), "aspeed.io",
                                   sc->memmap[ASPEED_DEV_IOMEM],
@@ -431,6 +438,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_FMC));
 
+    /* Set up an alias on the FMC CE0 region (boot default) */
+    MemoryRegion *fmc0_mmio = &s->fmc.flashes[0].mmio;
+    memory_region_init_alias(&s->spi_boot, OBJECT(s), "aspeed.spi_boot",
+                             fmc0_mmio, 0, memory_region_size(fmc0_mmio));
+    memory_region_add_subregion(&s->spi_boot_container, 0x0, &s->spi_boot);
+
     /* SPI */
     for (i = 0; i < sc->spis_num; i++) {
         object_property_set_link(OBJECT(&s->spi[i]), "dram",
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index e884d6badc..3507ea5818 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -25,6 +25,7 @@
 #define ASPEED_SOC_IOMEM_SIZE       0x00200000
 
 static const hwaddr aspeed_soc_ast2400_memmap[] = {
+    [ASPEED_DEV_SPI_BOOT]  = 0x0,
     [ASPEED_DEV_IOMEM]  = 0x1E600000,
     [ASPEED_DEV_FMC]    = 0x1E620000,
     [ASPEED_DEV_SPI1]   = 0x1E630000,
@@ -59,6 +60,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
 };
 
 static const hwaddr aspeed_soc_ast2500_memmap[] = {
+    [ASPEED_DEV_SPI_BOOT]  = 0x0,
     [ASPEED_DEV_IOMEM]  = 0x1E600000,
     [ASPEED_DEV_FMC]    = 0x1E620000,
     [ASPEED_DEV_SPI1]   = 0x1E630000,
@@ -245,6 +247,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     Error *err = NULL;
     g_autofree char *sram_name = NULL;
 
+    /* Default boot region (SPI memory or ROMs) */
+    memory_region_init(&s->spi_boot_container, OBJECT(s),
+                       "aspeed.spi_boot_container", 0x10000000);
+    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SPI_BOOT],
+                                &s->spi_boot_container);
+
     /* IO space */
     aspeed_mmio_map_unimplemented(s, SYS_BUS_DEVICE(&s->iomem), "aspeed.io",
                                   sc->memmap[ASPEED_DEV_IOMEM],
@@ -354,6 +362,12 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->fmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_FMC));
 
+    /* Set up an alias on the FMC CE0 region (boot default) */
+    MemoryRegion *fmc0_mmio = &s->fmc.flashes[0].mmio;
+    memory_region_init_alias(&s->spi_boot, OBJECT(s), "aspeed.spi_boot",
+                             fmc0_mmio, 0, memory_region_size(fmc0_mmio));
+    memory_region_add_subregion(&s->spi_boot_container, 0x0, &s->spi_boot);
+
     /* SPI */
     for (i = 0; i < sc->spis_num; i++) {
         if (!sysbus_realize(SYS_BUS_DEVICE(&s->spi[i]), errp)) {
diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index 90c04bbc33..f4600c290b 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -100,13 +100,7 @@ static void fby35_bmc_init(Fby35State *s)
         MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
         uint64_t size = memory_region_size(&fl->mmio);
 
-        if (s->mmio_exec) {
-            memory_region_init_alias(boot_rom, NULL, "aspeed.boot_rom",
-                                     &fl->mmio, 0, size);
-            memory_region_add_subregion(&s->bmc_memory, FBY35_BMC_FIRMWARE_ADDR,
-                                        boot_rom);
-        } else {
-
+        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,
-- 
2.39.1

Re: [PATCH 7/8] aspeed: Introduce a spi_boot region under the SoC

[Philippe Mathieu-Daudé]

On 14/2/23 18:18, Cédric Le Goater wrote:
> The default boot of the Aspeed SoCs is address 0x0. For this reason,
> the FMC flash device contents are remapped by HW on the first 256MB of
> the address space. In QEMU, this is currently done in the machine init
> with the setup of a region alias.
> 
> Move this code to the SoC and introduce an extra container to prepare
> ground for the boot ROM region which will overlap the FMC flash
> remapping.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/arm/aspeed_soc.h |  3 +++
>   hw/arm/aspeed.c             | 13 +------------
>   hw/arm/aspeed_ast2600.c     | 13 +++++++++++++
>   hw/arm/aspeed_soc.c         | 14 ++++++++++++++
>   hw/arm/fby35.c              |  8 +-------
>   5 files changed, 32 insertions(+), 19 deletions(-)

>   enum {
> +    ASPEED_DEV_SPI_BOOT,
>       ASPEED_DEV_IOMEM,
>       ASPEED_DEV_UART1,
>       ASPEED_DEV_UART2,


>   #define ASPEED_SOC_DPMCU_SIZE       0x00040000
>   
>   static const hwaddr aspeed_soc_ast2600_memmap[] = {
> +    [ASPEED_DEV_SPI_BOOT]  = 0x0,

Isn't this a constant address for this Soc family?
If so, we can define ASPEED_SOC_RESET_ADDR once ...

>       [ASPEED_DEV_SRAM]      = 0x10000000,
>       [ASPEED_DEV_DPMCU]     = 0x18000000,
>       /* 0x16000000     0x17FFFFFF : AHB BUS do LPC Bus bridge */
> @@ -282,6 +283,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>       qemu_irq irq;
>       g_autofree char *sram_name = NULL;
>   
> +    /* Default boot region (SPI memory or ROMs) */
> +    memory_region_init(&s->spi_boot_container, OBJECT(s),
> +                       "aspeed.spi_boot_container", 0x10000000);
> +    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SPI_BOOT],

... and use it here.

> +                                &s->spi_boot_container);

Re: [PATCH 7/8] aspeed: Introduce a spi_boot region under the SoC

[Cédric Le Goater]

On 2/15/23 12:02, Philippe Mathieu-Daudé wrote:
> On 14/2/23 18:18, Cédric Le Goater wrote:
>> The default boot of the Aspeed SoCs is address 0x0. For this reason,
>> the FMC flash device contents are remapped by HW on the first 256MB of
>> the address space. In QEMU, this is currently done in the machine init
>> with the setup of a region alias.
>>
>> Move this code to the SoC and introduce an extra container to prepare
>> ground for the boot ROM region which will overlap the FMC flash
>> remapping.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/arm/aspeed_soc.h |  3 +++
>>   hw/arm/aspeed.c             | 13 +------------
>>   hw/arm/aspeed_ast2600.c     | 13 +++++++++++++
>>   hw/arm/aspeed_soc.c         | 14 ++++++++++++++
>>   hw/arm/fby35.c              |  8 +-------
>>   5 files changed, 32 insertions(+), 19 deletions(-)
> 
>>   enum {
>> +    ASPEED_DEV_SPI_BOOT,
>>       ASPEED_DEV_IOMEM,
>>       ASPEED_DEV_UART1,
>>       ASPEED_DEV_UART2,
> 
> 
>>   #define ASPEED_SOC_DPMCU_SIZE       0x00040000
>>   static const hwaddr aspeed_soc_ast2600_memmap[] = {
>> +    [ASPEED_DEV_SPI_BOOT]  = 0x0,
> 
> Isn't this a constant address for this Soc family?
> If so, we can define ASPEED_SOC_RESET_ADDR once ...

I will introduce :

   #define ASPEED_SPI_BOOT_ADDR 0x0

and use it every where it makes sense. This should replace FIRMWARE_ADDR
in aspeed.c also.

Thanks,

C.

> 
>>       [ASPEED_DEV_SRAM]      = 0x10000000,
>>       [ASPEED_DEV_DPMCU]     = 0x18000000,
>>       /* 0x16000000     0x17FFFFFF : AHB BUS do LPC Bus bridge */
>> @@ -282,6 +283,12 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>       qemu_irq irq;
>>       g_autofree char *sram_name = NULL;
>> +    /* Default boot region (SPI memory or ROMs) */
>> +    memory_region_init(&s->spi_boot_container, OBJECT(s),
>> +                       "aspeed.spi_boot_container", 0x10000000);
>> +    memory_region_add_subregion(s->memory, sc->memmap[ASPEED_DEV_SPI_BOOT],
> 
> ... and use it here.
> 
>> +                                &s->spi_boot_container);
> 

[PATCH 8/8] aspeed: Add a boot_rom overlap region in the SoC spi_boot container

[Cédric Le Goater]

To avoid the SPI transactions fetching instructions from the FMC CE0
flash device and speed up boot, a ROM can be created if a drive is
available.

Reverse a bit the logic to allow a machine to boot without a drive,
using a block device instead :

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

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

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 998dc57969..13e719bae7 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -243,10 +243,9 @@ static void aspeed_reset_secondary(ARMCPU *cpu,
 
 #define FIRMWARE_ADDR 0x0
 
-static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
+static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
                            Error **errp)
 {
-    BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
     g_autofree void *storage = NULL;
     int64_t size;
 
@@ -272,6 +271,22 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
     rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
 }
 
+/*
+ * 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,
+                                    uint64_t rom_size)
+{
+    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+
+    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
+                           &error_abort);
+    memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
+                                        boot_rom, 1);
+    write_boot_rom(blk, FIRMWARE_ADDR, rom_size, &error_abort);
+}
+
 void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
                                       unsigned int count, int unit0)
 {
@@ -328,7 +343,6 @@ static void aspeed_machine_init(MachineState *machine)
     AspeedMachineState *bmc = ASPEED_MACHINE(machine);
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
     AspeedSoCClass *sc;
-    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
     int i;
     NICInfo *nd = &nd_table[0];
 
@@ -378,21 +392,6 @@ static void aspeed_machine_init(MachineState *machine)
                               bmc->spi_model ? bmc->spi_model : amc->spi_model,
                               1, amc->num_cs);
 
-    /* Install first FMC flash content as a boot rom. */
-    if (drive0) {
-        AspeedSMCFlash *fl = &bmc->soc.fmc.flashes[0];
-        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
-        uint64_t size = memory_region_size(&fl->mmio);
-
-        if (!ASPEED_MACHINE(machine)->mmio_exec) {
-            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
-                                   size, &error_abort);
-            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
-                                        boot_rom);
-            write_boot_rom(drive0, FIRMWARE_ADDR, size, &error_abort);
-        }
-    }
-
     if (machine->kernel_filename && sc->num_cpus > 1) {
         /* With no u-boot we must set up a boot stub for the secondary CPU */
         MemoryRegion *smpboot = g_new(MemoryRegion, 1);
@@ -423,6 +422,16 @@ static void aspeed_machine_init(MachineState *machine)
                            drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
     }
 
+    if (!bmc->mmio_exec) {
+        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
+
+        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);
+        }
+    }
+
     arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
 }
 
-- 
2.39.1

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> Hello,
>
> This series starts with a first set of patches fixing I2C slave mode
> in the Aspeed I2C controller, a test device and its associated test in
> avocado.
>
> Follow some cleanups which allow the use of block devices instead of
> drives. So that, instead of specifying :
>
>   -drive file=./flash-ast2600-evb,format=raw,if=mtd
>   -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>   ...
>
> and guessing from the order which bus the device is attached to, we
> can use :
>
>   -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>   -device mx66u51235f,bus=ssi.0,drive=fmc0
>   -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>   -device mx66u51235f,bus=ssi.0,drive=fmc1 
>   -blockdev node-name=pnor,driver=file,filename=./pnor
>   -device mx66l1g45g,bus=ssi.1,drive=pnor
>   ...
>
> It is not perfect, the CS index still depends on the order, but it is
> now possible to run a machine without -drive ...,if=mtd.

Lovely!

Does this cover all uses of IF_MTD, or only some?

> This lacks the final patch enabling the '-nodefaults' option by not
> creating the default devices if specified on the command line. It
> needs some more evaluation of the possible undesired effects. 

Are you thinking of something similar to the default CD-ROM, i.e. use
default_list to have -device suppress a certain kind of default devices,
and also have -nodefaults suppress them all?

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Cédric Le Goater]

On 2/15/23 07:38, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> Hello,
>>
>> This series starts with a first set of patches fixing I2C slave mode
>> in the Aspeed I2C controller, a test device and its associated test in
>> avocado.
>>
>> Follow some cleanups which allow the use of block devices instead of
>> drives. So that, instead of specifying :
>>
>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>    ...
>>
>> and guessing from the order which bus the device is attached to, we
>> can use :
>>
>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>    ...
>>
>> It is not perfect, the CS index still depends on the order, but it is
>> now possible to run a machine without -drive ...,if=mtd.
> 
> Lovely!
> 
> Does this cover all uses of IF_MTD, or only some?

All use for default devices in the aspeed machines. I think most
machines use IF_MTD in a similar way.

Yet, one extra use of IF_MTD is to fill a ROM region with the first
drive contents. It avoids fetching instructions from the SPI flash
mapping and speeds up boot quite significantly.

Once the default flash devices are created and attached to their
drive, it is possible to query the block backend without the
drive_get() API. I have a couple of patches for it and it shouldn't
be a problem.
  
>> This lacks the final patch enabling the '-nodefaults' option by not
>> creating the default devices if specified on the command line. It
>> needs some more evaluation of the possible undesired effects.
> 
> Are you thinking of something similar to the default CD-ROM, i.e. use
> default_list to have -device suppress a certain kind of default devices,
> and also have -nodefaults suppress them all?

I would have -nodefaults suppress all flash devices. There are also
I2C devices but they are less problematic for the machine definition.
(Well, eeprom contents can be complex to handle)

The next step would be to get rid of the drive_get(IF_MTD) internal
API, which means finding a way to attach block backend devices from
the command line to the default flash devices. This should be done
at machine init time and the blockdev should have some 'bus@addr'
identifier. I lack the knowledge on how this could be done.

Thanks,

C.

  

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> On 2/15/23 07:38, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>>> Hello,
>>>
>>> This series starts with a first set of patches fixing I2C slave mode
>>> in the Aspeed I2C controller, a test device and its associated test in
>>> avocado.
>>>
>>> Follow some cleanups which allow the use of block devices instead of
>>> drives. So that, instead of specifying :
>>>
>>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>>    ...
>>>
>>> and guessing from the order which bus the device is attached to, we
>>> can use :
>>>
>>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>>    ...
>>>
>>> It is not perfect, the CS index still depends on the order, but it is
>>> now possible to run a machine without -drive ...,if=mtd.
>>
>> Lovely!
>>
>> Does this cover all uses of IF_MTD, or only some?
>
> All use for default devices in the aspeed machines. I think most
> machines use IF_MTD in a similar way.
>
> Yet, one extra use of IF_MTD is to fill a ROM region with the first
> drive contents. It avoids fetching instructions from the SPI flash
> mapping and speeds up boot quite significantly.
>
> Once the default flash devices are created and attached to their
> drive, it is possible to query the block backend without the
> drive_get() API. I have a couple of patches for it and it shouldn't
> be a problem.
>>
>>> This lacks the final patch enabling the '-nodefaults' option by not
>>> creating the default devices if specified on the command line. It
>>> needs some more evaluation of the possible undesired effects.
>>
>> Are you thinking of something similar to the default CD-ROM, i.e. use
>> default_list to have -device suppress a certain kind of default devices,
>> and also have -nodefaults suppress them all?
>
> I would have -nodefaults suppress all flash devices. There are also
> I2C devices but they are less problematic for the machine definition.
> (Well, eeprom contents can be complex to handle)
>
> The next step would be to get rid of the drive_get(IF_MTD) internal
> API, which means finding a way to attach block backend devices from
> the command line to the default flash devices. This should be done
> at machine init time and the blockdev should have some 'bus@addr'
> identifier. I lack the knowledge on how this could be done.

Possibly interesting for you: commit e0561e60f17 "hw/arm/virt: Support
firmware configuration with -blockdev".

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Cédric Le Goater]

>> The next step would be to get rid of the drive_get(IF_MTD) internal
>> API, which means finding a way to attach block backend devices from
>> the command line to the default flash devices. This should be done
>> at machine init time and the blockdev should have some 'bus@addr'
>> identifier. I lack the knowledge on how this could be done.
> 
> Possibly interesting for you: commit e0561e60f17 "hw/arm/virt: Support
> firmware configuration with -blockdev".

I see. The mapping device<->blk is moved at the machine level with
an option. The same could be done for the Aspeed machines with "fmc0",
"spi0", identifiers.

I think we should deprecate the "fmc-model" and "spi-model" machine
options. They become useless with -nodefaults correctly implemented.

Thanks,
  

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Philippe Mathieu-Daudé]

On 14/2/23 18:18, Cédric Le Goater wrote:
> Hello,
> 
> This series starts with a first set of patches fixing I2C slave mode
> in the Aspeed I2C controller, a test device and its associated test in
> avocado.
> 
> Follow some cleanups which allow the use of block devices instead of
> drives. So that, instead of specifying :
> 
>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>    ...
> 
> and guessing from the order which bus the device is attached to, we
> can use :
> 
>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>    -blockdev node-name=pnor,driver=file,filename=./pnor
>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>    ...
> 
> It is not perfect, the CS index still depends on the order

Quick thoughts here:

TYPE_SSI_PERIPHERAL devices have one input SSI_GPIO_CS.

TYPE_SSI_BUS could have a "cs-num" property (how many
CS line associated with this bus) and create an array of
#cs-num output SSI_GPIO_CS.

TYPE_SSI_PERIPHERAL could have a "cs" (index) property;
if set, upon ssi_peripheral_realize() when the device is
plugged on the bus, the GPIO line is wired.

So we could set the 'cs=' property from CLI:

   -blockdev node-name=fmc0,driver=file,filename=./bmc.img
   -device mx66u51235f,bus=ssi.0,cs=1,drive=fmc0
   -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
   -device mx66u51235f,bus=ssi.0,cs=0,drive=fmc1

> but it is
> now possible to run a machine without -drive ...,if=mtd.
> 
> This lacks the final patch enabling the '-nodefaults' option by not
> creating the default devices if specified on the command line. It
> needs some more evaluation of the possible undesired effects.
> Thanks,
> 
> C.

Re: [PATCH 0/8] aspeed: I2C fixes, -drive removal (first step)

[Cédric Le Goater]

On 2/15/23 11:45, Philippe Mathieu-Daudé wrote:
> On 14/2/23 18:18, Cédric Le Goater wrote:
>> Hello,
>>
>> This series starts with a first set of patches fixing I2C slave mode
>> in the Aspeed I2C controller, a test device and its associated test in
>> avocado.
>>
>> Follow some cleanups which allow the use of block devices instead of
>> drives. So that, instead of specifying :
>>
>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>    ...
>>
>> and guessing from the order which bus the device is attached to, we
>> can use :
>>
>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>    ...
>>
>> It is not perfect, the CS index still depends on the order
> 
> Quick thoughts here:
> 
> TYPE_SSI_PERIPHERAL devices have one input SSI_GPIO_CS.
> 
> TYPE_SSI_BUS could have a "cs-num" property (how many
> CS line associated with this bus) and create an array of
> #cs-num output SSI_GPIO_CS.
> 
> TYPE_SSI_PERIPHERAL could have a "cs" (index) property;
> if set, upon ssi_peripheral_realize() when the device is
> plugged on the bus, the GPIO line is wired.

yes. I would like to check first the impact on migration compatibility.

Thanks,

C.

> So we could set the 'cs=' property from CLI:
> 
>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>    -device mx66u51235f,bus=ssi.0,cs=1,drive=fmc0
>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>    -device mx66u51235f,bus=ssi.0,cs=0,drive=fmc1
> 
>> but it is
>> now possible to run a machine without -drive ...,if=mtd.
>>
>> This lacks the final patch enabling the '-nodefaults' option by not
>> creating the default devices if specified on the command line. It
>> needs some more evaluation of the possible undesired effects.
>> Thanks,
>>
>> C.
>