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

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> 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

I don't like the idea of making the command line more complicated.
That is not a comment on this patch though, but it would be nice if we
could head towards decreasing the complexity.

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

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


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