On Mon, 18 Sept 2023 at 11:17, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> We shouldn't call QDev DeviceReset() before DeviceRealize().
>
> Since the OMAP MMC model is not QDev'ified, it has to manually
> call the SDCard reset() handler. This breaks QDev assumptions
> that DeviceReset() is never called before a device is fully
> realized. In order to avoid that, pass a 'realized' argument
> to omap_mmc_reset(). All cases are explicit manual resets,
> except in omap_mmc_write() where we expect the sdcard to be
> realized.
>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> ---
> include/hw/arm/omap.h | 2 +-
> hw/arm/omap1.c | 2 +-
> hw/arm/omap2.c | 2 +-
> hw/sd/omap_mmc.c | 21 ++++++++++++---------
> 4 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index 067e9419f7..d331467946 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -808,7 +808,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base,
> struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
> BlockBackend *blk, qemu_irq irq, qemu_irq dma[],
> omap_clk fclk, omap_clk iclk);
> -void omap_mmc_reset(struct omap_mmc_s *s);
> +void omap_mmc_reset(struct omap_mmc_s *s, bool realized);
> void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover);
> void omap_mmc_enable(struct omap_mmc_s *s, int enable);
>
> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
> index d5438156ee..3afeba6f86 100644
> --- a/hw/arm/omap1.c
> +++ b/hw/arm/omap1.c
> @@ -3728,7 +3728,7 @@ static void omap1_mpu_reset(void *opaque)
> omap_uart_reset(mpu->uart[0]);
> omap_uart_reset(mpu->uart[1]);
> omap_uart_reset(mpu->uart[2]);
> - omap_mmc_reset(mpu->mmc);
> + omap_mmc_reset(mpu->mmc, false);
> omap_mpuio_reset(mpu->mpuio);
> omap_uwire_reset(mpu->microwire);
> omap_pwl_reset(mpu->pwl);
> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
> index d5a2ae7af6..ef9b0dd60a 100644
> --- a/hw/arm/omap2.c
> +++ b/hw/arm/omap2.c
> @@ -2253,7 +2253,7 @@ static void omap2_mpu_reset(void *opaque)
> omap_uart_reset(mpu->uart[0]);
> omap_uart_reset(mpu->uart[1]);
> omap_uart_reset(mpu->uart[2]);
> - omap_mmc_reset(mpu->mmc);
> + omap_mmc_reset(mpu->mmc, false);
> omap_mcspi_reset(mpu->mcspi[0]);
> omap_mcspi_reset(mpu->mcspi[1]);
> cpu_reset(CPU(mpu->cpu));
These are reset functions registered via qemu_register_reset().
They should be OK to assume the SD card is realized.
This matters, because this is the only place that the SD
card will get reset -- as the comment in omap_mmc_reset() notes,
the SD card object isn't going to be plugged into any bus, so
it won't get auto-reset when the simulation starts, and these
reset function are the place that does the manual reset.
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index edd3cf2a1e..3c906993eb 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -287,7 +287,7 @@ static void omap_mmc_pseudo_reset(struct omap_mmc_s *host)
> host->fifo_len = 0;
> }
>
> -void omap_mmc_reset(struct omap_mmc_s *host)
> +void omap_mmc_reset(struct omap_mmc_s *host, bool realized)
> {
> host->last_cmd = 0;
> memset(host->rsp, 0, sizeof(host->rsp));
> @@ -314,11 +314,14 @@ void omap_mmc_reset(struct omap_mmc_s *host)
>
> omap_mmc_pseudo_reset(host);
>
> - /* Since we're still using the legacy SD API the card is not plugged
> - * into any bus, and we must reset it manually. When omap_mmc is
> - * QOMified this must move into the QOM reset function.
> - */
> - device_cold_reset(DEVICE(host->card));
> + if (realized) {
> + /*
> + * Since we're still using the legacy SD API the card is not plugged
> + * into any bus, and we must reset it manually. When omap_mmc is
> + * QOMified this must move into the QOM reset function.
> + */
> + device_cold_reset(DEVICE(host->card));
> + }
> }
>
> static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
> @@ -556,7 +559,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
> break;
> case 0x64: /* MMC_SYSC */
> if (value & (1 << 2)) /* SRTS */
> - omap_mmc_reset(s);
> + omap_mmc_reset(s, true);
> break;
> case 0x68: /* MMC_SYSS */
> OMAP_RO_REG(offset);
> @@ -613,7 +616,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base,
> exit(1);
> }
>
> - omap_mmc_reset(s);
> + omap_mmc_reset(s, false);
>
> return s;
> }
> @@ -643,7 +646,7 @@ struct omap_mmc_s *omap2_mmc_init(struct
> omap_target_agent_s *ta,
> s->cdet = qemu_allocate_irq(omap_mmc_cover_cb, s, 0);
> sd_set_cb(s->card, NULL, s->cdet);
>
> - omap_mmc_reset(s);
> + omap_mmc_reset(s, false);
These calls from omap_mmc_init() are probably safe to remove, but I
don't understand why they result in our resetting a non-realized
SD card object. In both cases, the call happens after we call
sd_init(). sd_init() both creates and realizes the SD card, so
it should be realized at the point when it gets reset.
In a truly ideal world we would QOMify the omap-mmc device: it
is the only remaining user of the legacy sd_init function...
thanks
-- PMM
