Re: [PATCH 1/2] hw/sd/omap_mmc: Do not reset SDCard until being fully realized

[Peter Maydell]

On Mon, 18 Sept 2023 at 11:17, Philippe Mathieu-Daudé <philmd@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é <philmd@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