| Message ID | 20230607043943.1837186-7-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: fixes and extensions | expand |
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 >
On 6/7/23 12:49, Joel Stanley wrote: > 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 There are benefits to this change and patch 8 : - it is possible to define block backends out of order - it is possible to define *all* devices backends. Some machines support up to 8. - it is possible to use different flash models without adding new boards - as a consequence, the machine options "spi-model" and "fmc-model" can be deprecated. These were a clumsy interface. - with -nodefaults, the machine starts running by fetching instructions from the FMC0 device, which is what HW does. - and the machine option "execute-in-place" can be deprecated. > That is not a comment on this patch though, but it would be nice if we > could head towards decreasing the complexity. Describing the devices on various buses comes at a cost. Using -drive is still possible. It should be considered an optimization loading the FMC0 contents as a ROM to speedup boot. Thanks, C. > >> 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 >>
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)
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;
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(-)