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

[Cédric Le Goater]

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


An alternative for the wiring would be to connect the GPIO lines in the
m25p80 realize routine. See below for a draft. Assumption is made on the
availability of the CS lines at the bus parent level, which should be
the controller.
 
Thanks,

C.


diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index aa0bfa57bb26..ae39a1a24b1b 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -154,6 +154,18 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char 
*name)
     return SSI_BUS(bus);
 }
 
+void ssi_attach(SSIPeripheral *s)
+{
+    BusState *bus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    qemu_irq cs_line = qdev_get_gpio_in_named(DEVICE(s), SSI_GPIO_CS, 0);
+
+    /*
+     * TODO: Will abort if "cs" GPIOs are not defined at the
+     * controller level
+     */
+    qdev_connect_gpio_out_named(DEVICE(bus->parent), "cs", s->addr, cs_line);
+}
+
 uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
 {
     BusState *b = BUS(bus);


diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index afc3fdf4d60b..89add100aefd 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1628,6 +1628,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error 
**errp)
 
     qdev_init_gpio_in_named(DEVICE(s),
                             m25p80_write_protect_pin_irq_handler, "WP#", 1);
+
+    ssi_attach(ss);
 }
 
 static void m25p80_reset(DeviceState *d)