[PATCH 00/11] hw/sd: QOMify omap-mmc, remove legacy APIs

[PATCH 00/11] hw/sd: QOMify omap-mmc, remove legacy APIs

[Peter Maydell]

This series QOMifies the omap-mmc device. The main reason for
this is that this device is now the only remaining in-tree
user of the legacy SD APIs defined in sdcard_legacy.h.
The first 8 patches QOMify the device and do some minor
cleanup on it. Patches 9 to 11 then remove the unused
legacy APIs, deleting sdcard_legacy.h entirely. This includes
letting us get rid of the me_no_qdev_me_kill_mammoth_with_rocks
codepaths in sd.c.

thanks
-- PMM

Peter Maydell (11):
  hw/sd/omap_mmc: Do a minimal conversion to QDev
  hw/sd/omap_mmc: Convert remaining 'struct omap_mmc_s' uses to
    OMAPMMCState
  hw/sd/omap_mmc: Convert output qemu_irqs to gpio and sysbus IRQ APIs
  hw/sd/omap_mmc: Convert to SDBus API
  hw/sd/omap_mmc: Use similar API for "wire up omap_clk" to other OMAP
    devices
  hw/arm/omap1: Inline creation of MMC
  hw/sd/omap_mmc: Remove unused coverswitch qemu_irq
  hw/sd/omap_mmc: Untabify
  hw/sd: Remove unused 'enable' method from SDCardClass
  hw/sd: Remove unused legacy functions, stop killing mammoths
  hw/sd: Remove unused SDState::enable

 include/hw/arm/omap.h         |  17 ++-
 include/hw/sd/sd.h            |   1 -
 include/hw/sd/sdcard_legacy.h |  50 -------
 hw/arm/omap1.c                |  26 +++-
 hw/sd/omap_mmc.c              | 256 +++++++++++++++++++---------------
 hw/sd/sd.c                    |  94 +++----------
 6 files changed, 190 insertions(+), 254 deletions(-)
 delete mode 100644 include/hw/sd/sdcard_legacy.h

-- 
2.34.1

[PATCH 01/11] hw/sd/omap_mmc: Do a minimal conversion to QDev

[Peter Maydell]

Do a minimal conversion of the omap_mmc device model to QDev.

In this commit we do the bare minimum to produce a working device:
 * add the SysBusDevice parent_obj and the usual type boilerplate
 * omap_mmc_init() now returns a DeviceState*
 * reset is handled by sysbus reset, so the SoC reset function
   doesn't need to call omap_mmc_reset() any more
 * code that should obviously be in init/realize is moved there
   from omap_mmc_init()

We leave various pieces of cleanup to later commits:
 * rationalizing 'struct omap_mmc_s *' to 'OMAPMMCState *'
 * using gpio lines rather than having omap_mmc_init() directly
   set s->irq, s->dma
 * switching away from the legacy SD API and instead having
   the SD card plugged into a bus

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/omap.h | 15 ++++----
 hw/arm/omap1.c        |  1 -
 hw/sd/omap_mmc.c      | 83 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 420ed1d5735..6339c5a581e 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -529,12 +529,13 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
                                         omap_clk clk);
 
 /* omap_mmc.c */
-struct omap_mmc_s;
-struct omap_mmc_s *omap_mmc_init(hwaddr base,
-                MemoryRegion *sysmem,
-                BlockBackend *blk,
-                qemu_irq irq, qemu_irq dma[], omap_clk clk);
-void omap_mmc_reset(struct omap_mmc_s *s);
+#define TYPE_OMAP_MMC "omap-mmc"
+OBJECT_DECLARE_SIMPLE_TYPE(omap_mmc_s, OMAP_MMC)
+
+DeviceState *omap_mmc_init(hwaddr base,
+                           MemoryRegion *sysmem,
+                           BlockBackend *blk,
+                           qemu_irq irq, qemu_irq dma[], omap_clk clk);
 
 /* omap_i2c.c */
 I2CBus *omap_i2c_bus(DeviceState *omap_i2c);
@@ -601,7 +602,7 @@ struct omap_mpu_state_s {
     /* MPU public TIPB peripherals */
     struct omap_32khz_timer_s *os_timer;
 
-    struct omap_mmc_s *mmc;
+    DeviceState *mmc;
 
     struct omap_mpuio_s *mpuio;
 
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index f3a0ac40e48..ea07b9aa31e 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -3716,7 +3716,6 @@ 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_mpuio_reset(mpu->mpuio);
     omap_uwire_reset(mpu->microwire);
     omap_pwl_reset(mpu->pwl);
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 1d4e30e6b7b..9cba437f74e 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -21,11 +21,15 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "hw/irq.h"
+#include "hw/sysbus.h"
 #include "hw/arm/omap.h"
 #include "hw/sd/sdcard_legacy.h"
 
-struct omap_mmc_s {
+typedef struct omap_mmc_s {
+    SysBusDevice parent_obj;
+
     qemu_irq irq;
     qemu_irq *dma;
     qemu_irq coverswitch;
@@ -66,7 +70,7 @@ struct omap_mmc_s {
     int cdet_enable;
     int cdet_state;
     qemu_irq cdet;
-};
+} OMAPMMCState;
 
 static void omap_mmc_interrupts_update(struct omap_mmc_s *s)
 {
@@ -297,7 +301,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)
+static void omap_mmc_reset(struct omap_mmc_s *host)
 {
     host->last_cmd = 0;
     memset(host->rsp, 0, sizeof(host->rsp));
@@ -328,7 +332,9 @@ void omap_mmc_reset(struct omap_mmc_s *host)
      * 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 (host->card) {
+        device_cold_reset(DEVICE(host->card));
+    }
 }
 
 static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
@@ -583,29 +589,76 @@ static const MemoryRegionOps omap_mmc_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-struct omap_mmc_s *omap_mmc_init(hwaddr base,
-                MemoryRegion *sysmem,
-                BlockBackend *blk,
-                qemu_irq irq, qemu_irq dma[], omap_clk clk)
+DeviceState *omap_mmc_init(hwaddr base,
+                           MemoryRegion *sysmem,
+                           BlockBackend *blk,
+                           qemu_irq irq, qemu_irq dma[], omap_clk clk)
 {
-    struct omap_mmc_s *s = g_new0(struct omap_mmc_s, 1);
+    DeviceState *dev;
+    OMAPMMCState *s;
+
+    dev = qdev_new(TYPE_OMAP_MMC);
+    s = OMAP_MMC(dev);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
 
     s->irq = irq;
     s->dma = dma;
     s->clk = clk;
-    s->lines = 1;	/* TODO: needs to be settable per-board */
-    s->rev = 1;
 
-    memory_region_init_io(&s->iomem, NULL, &omap_mmc_ops, s, "omap.mmc", 0x800);
-    memory_region_add_subregion(sysmem, base, &s->iomem);
+    memory_region_add_subregion(sysmem, base,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(s), 0));
 
     /* Instantiate the storage */
     s->card = sd_init(blk, false);
     if (s->card == NULL) {
         exit(1);
     }
+    return dev;
+}
+
+static void omap_mmc_reset_hold(Object *obj, ResetType type)
+{
+    OMAPMMCState *s = OMAP_MMC(obj);
 
     omap_mmc_reset(s);
-
-    return s;
 }
+
+static void omap_mmc_initfn(Object *obj)
+{
+    OMAPMMCState *s = OMAP_MMC(obj);
+
+    /* In theory these could be settable per-board */
+    s->lines = 1;
+    s->rev = 1;
+
+    memory_region_init_io(&s->iomem, obj, &omap_mmc_ops, s, "omap.mmc", 0x800);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
+}
+
+static void omap_mmc_realize(DeviceState *dev, Error **errp)
+{
+}
+
+static void omap_mmc_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    ResettableClass *rc = RESETTABLE_CLASS(oc);
+
+    rc->phases.hold = omap_mmc_reset_hold;
+    dc->realize = omap_mmc_realize;
+}
+
+static const TypeInfo omap_mmc_info = {
+    .name = TYPE_OMAP_MMC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(OMAPMMCState),
+    .instance_init = omap_mmc_initfn,
+    .class_init = omap_mmc_class_init,
+};
+
+static void omap_mmc_register_types(void)
+{
+    type_register_static(&omap_mmc_info);
+}
+
+type_init(omap_mmc_register_types)
-- 
2.34.1

Re: [PATCH 01/11] hw/sd/omap_mmc: Do a minimal conversion to QDev

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> +static const TypeInfo omap_mmc_info = {
> +    .name = TYPE_OMAP_MMC,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(OMAPMMCState),
> +    .instance_init = omap_mmc_initfn,
> +    .class_init = omap_mmc_class_init,
> +};
> +
> +static void omap_mmc_register_types(void)
> +{
> +    type_register_static(&omap_mmc_info);
> +}
> +
> +type_init(omap_mmc_register_types)

Phil seems to be moving everything to DEFINE_TYPES now.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 01/11] hw/sd/omap_mmc: Do a minimal conversion to QDev

[Peter Maydell]

On Tue, 28 Jan 2025 at 18:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/28/25 02:45, Peter Maydell wrote:
> > +static const TypeInfo omap_mmc_info = {
> > +    .name = TYPE_OMAP_MMC,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(OMAPMMCState),
> > +    .instance_init = omap_mmc_initfn,
> > +    .class_init = omap_mmc_class_init,
> > +};
> > +
> > +static void omap_mmc_register_types(void)
> > +{
> > +    type_register_static(&omap_mmc_info);
> > +}
> > +
> > +type_init(omap_mmc_register_types)
>
> Phil seems to be moving everything to DEFINE_TYPES now.

I looked at that, but it wants an array of TypeInfo,
and this is just one. So I did it this way instead.

-- PMM

Re: [PATCH 01/11] hw/sd/omap_mmc: Do a minimal conversion to QDev

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> Do a minimal conversion of the omap_mmc device model to QDev.
> 
> In this commit we do the bare minimum to produce a working device:
>   * add the SysBusDevice parent_obj and the usual type boilerplate
>   * omap_mmc_init() now returns a DeviceState*
>   * reset is handled by sysbus reset, so the SoC reset function
>     doesn't need to call omap_mmc_reset() any more
>   * code that should obviously be in init/realize is moved there
>     from omap_mmc_init()
> 
> We leave various pieces of cleanup to later commits:
>   * rationalizing 'struct omap_mmc_s *' to 'OMAPMMCState *'
>   * using gpio lines rather than having omap_mmc_init() directly
>     set s->irq, s->dma
>   * switching away from the legacy SD API and instead having
>     the SD card plugged into a bus
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/omap.h | 15 ++++----
>   hw/arm/omap1.c        |  1 -
>   hw/sd/omap_mmc.c      | 83 +++++++++++++++++++++++++++++++++++--------
>   3 files changed, 76 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 02/11] hw/sd/omap_mmc: Convert remaining 'struct omap_mmc_s' uses to OMAPMMCState

[Peter Maydell]

Mechanically convert the remaining uses of 'struct omap_mmc_s' to
'OMAPMMCState'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/omap.h |  2 +-
 hw/sd/omap_mmc.c      | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 6339c5a581e..7d1a1afc4f8 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -530,7 +530,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
 
 /* omap_mmc.c */
 #define TYPE_OMAP_MMC "omap-mmc"
-OBJECT_DECLARE_SIMPLE_TYPE(omap_mmc_s, OMAP_MMC)
+OBJECT_DECLARE_SIMPLE_TYPE(OMAPMMCState, OMAP_MMC)
 
 DeviceState *omap_mmc_init(hwaddr base,
                            MemoryRegion *sysmem,
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 9cba437f74e..dd45abc075e 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -27,7 +27,7 @@
 #include "hw/arm/omap.h"
 #include "hw/sd/sdcard_legacy.h"
 
-typedef struct omap_mmc_s {
+typedef struct OMAPMMCState {
     SysBusDevice parent_obj;
 
     qemu_irq irq;
@@ -72,12 +72,12 @@ typedef struct omap_mmc_s {
     qemu_irq cdet;
 } OMAPMMCState;
 
-static void omap_mmc_interrupts_update(struct omap_mmc_s *s)
+static void omap_mmc_interrupts_update(OMAPMMCState *s)
 {
     qemu_set_irq(s->irq, !!(s->status & s->mask));
 }
 
-static void omap_mmc_fifolevel_update(struct omap_mmc_s *host)
+static void omap_mmc_fifolevel_update(OMAPMMCState *host)
 {
     if (!host->transfer && !host->fifo_len) {
         host->status &= 0xf3ff;
@@ -125,7 +125,7 @@ typedef enum {
     SD_TYPE_ADTC = 3,   /* addressed with data transfer */
 } MMCCmdType;
 
-static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
+static void omap_mmc_command(OMAPMMCState *host, int cmd, int dir,
                              MMCCmdType type, int busy,
                              sd_rsp_type_t resptype, int init)
 {
@@ -234,7 +234,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
         host->status |= 0x0001;
 }
 
-static void omap_mmc_transfer(struct omap_mmc_s *host)
+static void omap_mmc_transfer(OMAPMMCState *host)
 {
     uint8_t value;
 
@@ -289,19 +289,19 @@ static void omap_mmc_transfer(struct omap_mmc_s *host)
 
 static void omap_mmc_update(void *opaque)
 {
-    struct omap_mmc_s *s = opaque;
+    OMAPMMCState *s = opaque;
     omap_mmc_transfer(s);
     omap_mmc_fifolevel_update(s);
     omap_mmc_interrupts_update(s);
 }
 
-static void omap_mmc_pseudo_reset(struct omap_mmc_s *host)
+static void omap_mmc_pseudo_reset(OMAPMMCState *host)
 {
     host->status = 0;
     host->fifo_len = 0;
 }
 
-static void omap_mmc_reset(struct omap_mmc_s *host)
+static void omap_mmc_reset(OMAPMMCState *host)
 {
     host->last_cmd = 0;
     memset(host->rsp, 0, sizeof(host->rsp));
@@ -340,7 +340,7 @@ static void omap_mmc_reset(struct omap_mmc_s *host)
 static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
 {
     uint16_t i;
-    struct omap_mmc_s *s = opaque;
+    OMAPMMCState *s = opaque;
 
     if (size != 2) {
         return omap_badwidth_read16(opaque, offset);
@@ -433,7 +433,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
                            uint64_t value, unsigned size)
 {
     int i;
-    struct omap_mmc_s *s = opaque;
+    OMAPMMCState *s = opaque;
 
     if (size != 2) {
         omap_badwidth_write16(opaque, offset, value);
-- 
2.34.1

Re: [PATCH 02/11] hw/sd/omap_mmc: Convert remaining 'struct omap_mmc_s' uses to OMAPMMCState

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> Mechanically convert the remaining uses of 'struct omap_mmc_s' to
> 'OMAPMMCState'.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/arm/omap.h |  2 +-
>   hw/sd/omap_mmc.c      | 20 ++++++++++----------
>   2 files changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/11] hw/sd/omap_mmc: Convert remaining 'struct omap_mmc_s' uses to OMAPMMCState

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> Mechanically convert the remaining uses of 'struct omap_mmc_s' to
> 'OMAPMMCState'.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/omap.h |  2 +-
>   hw/sd/omap_mmc.c      | 20 ++++++++++----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index 6339c5a581e..7d1a1afc4f8 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -530,7 +530,7 @@ struct omap_lcd_panel_s *omap_lcdc_init(MemoryRegion *sysmem,
>   
>   /* omap_mmc.c */
>   #define TYPE_OMAP_MMC "omap-mmc"
> -OBJECT_DECLARE_SIMPLE_TYPE(omap_mmc_s, OMAP_MMC)
> +OBJECT_DECLARE_SIMPLE_TYPE(OMAPMMCState, OMAP_MMC)

I was expecting this in CamelCase to be OmapMmcState but then
looking in history I figured both OMAP and MMC are acronyms
(Open Multimedia Applications Platform and MultiMediaCard).

commit b56d351e25065d46fb959081fe13e8d031df35f3
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Thu Jan 28 11:41:26 2021 +0000

     hw/timer/cmsdk-apb-timer: Rename CMSDKAPBTIMER struct to CMSDKAPBTimer

     The state struct for the CMSDK APB timer device doesn't follow our
     usual naming convention of camelcase -- "CMSDK" and "APB" are both
     acronyms, but "TIMER" is not so should not be all-uppercase.
     Globally rename the struct to "CMSDKAPBTimer" (bringing it into line
     with CMSDKAPBWatchdog and CMSDKAPBDualTimer; CMSDKAPBUART remains
     as-is because "UART" is an acronym).

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 03/11] hw/sd/omap_mmc: Convert output qemu_irqs to gpio and sysbus IRQ APIs

[Peter Maydell]

The omap_mmc device has three outbound qemu_irq lines:
 * one actual interrupt line
 * two which connect to the DMA controller and are signalled for
   TX and RX DMA

Convert these to a sysbus IRQ and two named GPIO outputs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/omap_mmc.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index dd45abc075e..d497e31a6c5 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -31,7 +31,8 @@ typedef struct OMAPMMCState {
     SysBusDevice parent_obj;
 
     qemu_irq irq;
-    qemu_irq *dma;
+    qemu_irq dma_tx_gpio;
+    qemu_irq dma_rx_gpio;
     qemu_irq coverswitch;
     MemoryRegion iomem;
     omap_clk clk;
@@ -87,22 +88,22 @@ static void omap_mmc_fifolevel_update(OMAPMMCState *host)
     if (host->fifo_len > host->af_level && host->ddir) {
         if (host->rx_dma) {
             host->status &= 0xfbff;
-            qemu_irq_raise(host->dma[1]);
+            qemu_irq_raise(host->dma_rx_gpio);
         } else
             host->status |= 0x0400;
     } else {
         host->status &= 0xfbff;
-        qemu_irq_lower(host->dma[1]);
+        qemu_irq_lower(host->dma_rx_gpio);
     }
 
     if (host->fifo_len < host->ae_level && !host->ddir) {
         if (host->tx_dma) {
             host->status &= 0xf7ff;
-            qemu_irq_raise(host->dma[0]);
+            qemu_irq_raise(host->dma_tx_gpio);
         } else
             host->status |= 0x0800;
     } else {
-        qemu_irq_lower(host->dma[0]);
+        qemu_irq_lower(host->dma_tx_gpio);
         host->status &= 0xf7ff;
     }
 }
@@ -601,12 +602,13 @@ DeviceState *omap_mmc_init(hwaddr base,
     s = OMAP_MMC(dev);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
 
-    s->irq = irq;
-    s->dma = dma;
     s->clk = clk;
 
     memory_region_add_subregion(sysmem, base,
                                 sysbus_mmio_get_region(SYS_BUS_DEVICE(s), 0));
+    qdev_connect_gpio_out_named(dev, "dma-tx", 0, dma[0]);
+    qdev_connect_gpio_out_named(dev, "dma-rx", 0, dma[1]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
 
     /* Instantiate the storage */
     s->card = sd_init(blk, false);
@@ -633,6 +635,10 @@ static void omap_mmc_initfn(Object *obj)
 
     memory_region_init_io(&s->iomem, obj, &omap_mmc_ops, s, "omap.mmc", 0x800);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
+
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->dma_tx_gpio, "dma-tx", 1);
+    qdev_init_gpio_out_named(DEVICE(obj), &s->dma_rx_gpio, "dma-rx", 1);
 }
 
 static void omap_mmc_realize(DeviceState *dev, Error **errp)
-- 
2.34.1

Re: [PATCH 03/11] hw/sd/omap_mmc: Convert output qemu_irqs to gpio and sysbus IRQ APIs

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> The omap_mmc device has three outbound qemu_irq lines:
>   * one actual interrupt line
>   * two which connect to the DMA controller and are signalled for
>     TX and RX DMA
> 
> Convert these to a sysbus IRQ and two named GPIO outputs.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/sd/omap_mmc.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/11] hw/sd/omap_mmc: Convert output qemu_irqs to gpio and sysbus IRQ APIs

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> The omap_mmc device has three outbound qemu_irq lines:
>   * one actual interrupt line
>   * two which connect to the DMA controller and are signalled for
>     TX and RX DMA
> 
> Convert these to a sysbus IRQ and two named GPIO outputs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/omap_mmc.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 04/11] hw/sd/omap_mmc: Convert to SDBus API

[Peter Maydell]

Convert the OMAP MMC controller to the new SDBus API:
 * the controller creates an SDBus bus
 * instead of sd_foo functions on the SDState object, call
   sdbus_foo functions on the SDBus
 * the board code creates a proper TYPE_SD_CARD object and attaches
   it to the controller's SDBus, instead of the controller creating
   a card directly via sd_init() that never gets attached to any bus
 * because the SD card object is on a bus, it gets reset automatically
   by the "traverse the qbus tree resetting things" code, and we don't
   need to manually reset the card from the controller reset function

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/omap.h |  1 -
 hw/arm/omap1.c        | 10 +++++++++-
 hw/sd/omap_mmc.c      | 30 ++++++++++--------------------
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 7d1a1afc4f8..d20c55a8957 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -534,7 +534,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(OMAPMMCState, OMAP_MMC)
 
 DeviceState *omap_mmc_init(hwaddr base,
                            MemoryRegion *sysmem,
-                           BlockBackend *blk,
                            qemu_irq irq, qemu_irq dma[], omap_clk clk);
 
 /* omap_i2c.c */
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index ea07b9aa31e..15ba0a0d0c4 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -29,6 +29,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/arm/boot.h"
 #include "hw/arm/omap.h"
+#include "hw/sd/sd.h"
 #include "system/blockdev.h"
 #include "system/system.h"
 #include "hw/arm/soc_dma.h"
@@ -3981,11 +3982,18 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *dram,
         warn_report("missing SecureDigital device");
     }
     s->mmc = omap_mmc_init(0xfffb7800, system_memory,
-                           dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                            qdev_get_gpio_in(s->ih[1], OMAP_INT_OQN),
                            &s->drq[OMAP_DMA_MMC_TX],
                     omap_findclk(s, "mmc_ck"));
 
+    if (dinfo) {
+        DeviceState *card = qdev_new(TYPE_SD_CARD);
+        qdev_prop_set_drive_err(card, "drive", blk_by_legacy_dinfo(dinfo),
+                                &error_fatal);
+        qdev_realize_and_unref(card, qdev_get_child_bus(s->mmc, "sd-bus"),
+                               &error_fatal);
+    }
+
     s->mpuio = omap_mpuio_init(system_memory, 0xfffb5000,
                                qdev_get_gpio_in(s->ih[1], OMAP_INT_KEYBOARD),
                                qdev_get_gpio_in(s->ih[1], OMAP_INT_MPUIO),
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index d497e31a6c5..46fdeac1121 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -30,13 +30,14 @@
 typedef struct OMAPMMCState {
     SysBusDevice parent_obj;
 
+    SDBus sdbus;
+
     qemu_irq irq;
     qemu_irq dma_tx_gpio;
     qemu_irq dma_rx_gpio;
     qemu_irq coverswitch;
     MemoryRegion iomem;
     omap_clk clk;
-    SDState *card;
     uint16_t last_cmd;
     uint16_t sdio;
     uint16_t rsp[8];
@@ -158,7 +159,7 @@ static void omap_mmc_command(OMAPMMCState *host, int cmd, int dir,
     request.arg = host->arg;
     request.crc = 0; /* FIXME */
 
-    rsplen = sd_do_command(host->card, &request, response);
+    rsplen = sdbus_do_command(&host->sdbus, &request, response);
 
     /* TODO: validate CRCs */
     switch (resptype) {
@@ -247,10 +248,10 @@ static void omap_mmc_transfer(OMAPMMCState *host)
             if (host->fifo_len > host->af_level)
                 break;
 
-            value = sd_read_byte(host->card);
+            value = sdbus_read_byte(&host->sdbus);
             host->fifo[(host->fifo_start + host->fifo_len) & 31] = value;
             if (-- host->blen_counter) {
-                value = sd_read_byte(host->card);
+                value = sdbus_read_byte(&host->sdbus);
                 host->fifo[(host->fifo_start + host->fifo_len) & 31] |=
                         value << 8;
                 host->blen_counter --;
@@ -262,10 +263,10 @@ static void omap_mmc_transfer(OMAPMMCState *host)
                 break;
 
             value = host->fifo[host->fifo_start] & 0xff;
-            sd_write_byte(host->card, value);
+            sdbus_write_byte(&host->sdbus, value);
             if (-- host->blen_counter) {
                 value = host->fifo[host->fifo_start] >> 8;
-                sd_write_byte(host->card, value);
+                sdbus_write_byte(&host->sdbus, value);
                 host->blen_counter --;
             }
 
@@ -328,14 +329,6 @@ static void omap_mmc_reset(OMAPMMCState *host)
     host->clkdiv = 0;
 
     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.
-     */
-    if (host->card) {
-        device_cold_reset(DEVICE(host->card));
-    }
 }
 
 static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
@@ -592,7 +585,6 @@ static const MemoryRegionOps omap_mmc_ops = {
 
 DeviceState *omap_mmc_init(hwaddr base,
                            MemoryRegion *sysmem,
-                           BlockBackend *blk,
                            qemu_irq irq, qemu_irq dma[], omap_clk clk)
 {
     DeviceState *dev;
@@ -610,11 +602,6 @@ DeviceState *omap_mmc_init(hwaddr base,
     qdev_connect_gpio_out_named(dev, "dma-rx", 0, dma[1]);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
 
-    /* Instantiate the storage */
-    s->card = sd_init(blk, false);
-    if (s->card == NULL) {
-        exit(1);
-    }
     return dev;
 }
 
@@ -643,6 +630,9 @@ static void omap_mmc_initfn(Object *obj)
 
 static void omap_mmc_realize(DeviceState *dev, Error **errp)
 {
+    OMAPMMCState *s = OMAP_MMC(dev);
+
+    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, dev, "sd-bus");
 }
 
 static void omap_mmc_class_init(ObjectClass *oc, void *data)
-- 
2.34.1

Re: [PATCH 04/11] hw/sd/omap_mmc: Convert to SDBus API

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> Convert the OMAP MMC controller to the new SDBus API:
>   * the controller creates an SDBus bus
>   * instead of sd_foo functions on the SDState object, call
>     sdbus_foo functions on the SDBus
>   * the board code creates a proper TYPE_SD_CARD object and attaches
>     it to the controller's SDBus, instead of the controller creating
>     a card directly via sd_init() that never gets attached to any bus
>   * because the SD card object is on a bus, it gets reset automatically
>     by the "traverse the qbus tree resetting things" code, and we don't
>     need to manually reset the card from the controller reset function
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/omap.h |  1 -
>   hw/arm/omap1.c        | 10 +++++++++-
>   hw/sd/omap_mmc.c      | 30 ++++++++++--------------------
>   3 files changed, 19 insertions(+), 22 deletions(-)


> @@ -643,6 +630,9 @@ static void omap_mmc_initfn(Object *obj)
>   
>   static void omap_mmc_realize(DeviceState *dev, Error **errp)
>   {
> +    OMAPMMCState *s = OMAP_MMC(dev);
> +
> +    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, dev, "sd-bus");

Why you don't create the bus in omap_mmc_initfn()?

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   }

Re: [PATCH 04/11] hw/sd/omap_mmc: Convert to SDBus API

[Peter Maydell]

On Thu, 30 Jan 2025 at 22:26, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 28/1/25 11:45, Peter Maydell wrote:
> > Convert the OMAP MMC controller to the new SDBus API:
> >   * the controller creates an SDBus bus
> >   * instead of sd_foo functions on the SDState object, call
> >     sdbus_foo functions on the SDBus
> >   * the board code creates a proper TYPE_SD_CARD object and attaches
> >     it to the controller's SDBus, instead of the controller creating
> >     a card directly via sd_init() that never gets attached to any bus
> >   * because the SD card object is on a bus, it gets reset automatically
> >     by the "traverse the qbus tree resetting things" code, and we don't
> >     need to manually reset the card from the controller reset function
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   include/hw/arm/omap.h |  1 -
> >   hw/arm/omap1.c        | 10 +++++++++-
> >   hw/sd/omap_mmc.c      | 30 ++++++++++--------------------
> >   3 files changed, 19 insertions(+), 22 deletions(-)
>
>
> > @@ -643,6 +630,9 @@ static void omap_mmc_initfn(Object *obj)
> >
> >   static void omap_mmc_realize(DeviceState *dev, Error **errp)
> >   {
> > +    OMAPMMCState *s = OMAP_MMC(dev);
> > +
> > +    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, dev, "sd-bus");
>
> Why you don't create the bus in omap_mmc_initfn()?

Good question. I think I was using hw/sd/ssi-sd.c as a guide
(since it's the only other device that directly creates a
TYPE_SD_BUS rather than needing to subclass the bus), and
that put it in realize. But it's probably better in init.

> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

thanks
-- PMM

[PATCH 05/11] hw/sd/omap_mmc: Use similar API for "wire up omap_clk" to other OMAP devices

[Peter Maydell]

The approach we've settled on for handling the omap_clk wiring for
OMAP devices converted to QDev is to have a function omap_foo_set_clk()
whose implementation just sets the field directly in the device's
state struct. (See the "TODO" comment near the top of omap.h.)
Make omap_mmc do the same.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/omap.h | 3 +++
 hw/sd/omap_mmc.c      | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index d20c55a8957..7cb87ea89cd 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -535,6 +535,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(OMAPMMCState, OMAP_MMC)
 DeviceState *omap_mmc_init(hwaddr base,
                            MemoryRegion *sysmem,
                            qemu_irq irq, qemu_irq dma[], omap_clk clk);
+/* TODO: clock framework (see above) */
+void omap_mmc_set_clk(DeviceState *dev, omap_clk clk);
+
 
 /* omap_i2c.c */
 I2CBus *omap_i2c_bus(DeviceState *omap_i2c);
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 46fdeac1121..fcec2899afb 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -583,6 +583,13 @@ static const MemoryRegionOps omap_mmc_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+void omap_mmc_set_clk(DeviceState *dev, omap_clk clk)
+{
+    OMAPMMCState *s = OMAP_MMC(dev);
+
+    s->clk = clk;
+}
+
 DeviceState *omap_mmc_init(hwaddr base,
                            MemoryRegion *sysmem,
                            qemu_irq irq, qemu_irq dma[], omap_clk clk)
@@ -594,7 +601,7 @@ DeviceState *omap_mmc_init(hwaddr base,
     s = OMAP_MMC(dev);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
 
-    s->clk = clk;
+    omap_mmc_set_clk(dev, clk);
 
     memory_region_add_subregion(sysmem, base,
                                 sysbus_mmio_get_region(SYS_BUS_DEVICE(s), 0));
-- 
2.34.1

Re: [PATCH 05/11] hw/sd/omap_mmc: Use similar API for "wire up omap_clk" to other OMAP devices

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> The approach we've settled on for handling the omap_clk wiring for
> OMAP devices converted to QDev is to have a function omap_foo_set_clk()
> whose implementation just sets the field directly in the device's
> state struct. (See the "TODO" comment near the top of omap.h.)
> Make omap_mmc do the same.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/omap.h | 3 +++
>   hw/sd/omap_mmc.c      | 9 ++++++++-
>   2 files changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 06/11] hw/arm/omap1: Inline creation of MMC

[Peter Maydell]

Our style for other conversions of OMAP devices to qdev has been to
inline the creation and wiring into omap310_mpu_init() -- see for
instance the handling of omap-intc, omap-gpio and omap_i2c. Do
the same for omap-mmc.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/omap1.c   | 15 +++++++++++----
 hw/sd/omap_mmc.c | 22 ----------------------
 2 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 15ba0a0d0c4..ca2eb0d1576 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -3981,10 +3981,17 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *dram,
     if (!dinfo && !qtest_enabled()) {
         warn_report("missing SecureDigital device");
     }
-    s->mmc = omap_mmc_init(0xfffb7800, system_memory,
-                           qdev_get_gpio_in(s->ih[1], OMAP_INT_OQN),
-                           &s->drq[OMAP_DMA_MMC_TX],
-                    omap_findclk(s, "mmc_ck"));
+
+    s->mmc = qdev_new(TYPE_OMAP_MMC);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(s->mmc), &error_fatal);
+    omap_mmc_set_clk(s->mmc, omap_findclk(s, "mmc_ck"));
+
+    memory_region_add_subregion(system_memory, 0xfffb7800,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(s->mmc), 0));
+    qdev_connect_gpio_out_named(s->mmc, "dma-tx", 0, s->drq[OMAP_DMA_MMC_TX]);
+    qdev_connect_gpio_out_named(s->mmc, "dma-rx", 0, s->drq[OMAP_DMA_MMC_RX]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(s->mmc), 0,
+                       qdev_get_gpio_in(s->ih[1], OMAP_INT_OQN));
 
     if (dinfo) {
         DeviceState *card = qdev_new(TYPE_SD_CARD);
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index fcec2899afb..dacbea13aad 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -590,28 +590,6 @@ void omap_mmc_set_clk(DeviceState *dev, omap_clk clk)
     s->clk = clk;
 }
 
-DeviceState *omap_mmc_init(hwaddr base,
-                           MemoryRegion *sysmem,
-                           qemu_irq irq, qemu_irq dma[], omap_clk clk)
-{
-    DeviceState *dev;
-    OMAPMMCState *s;
-
-    dev = qdev_new(TYPE_OMAP_MMC);
-    s = OMAP_MMC(dev);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
-
-    omap_mmc_set_clk(dev, clk);
-
-    memory_region_add_subregion(sysmem, base,
-                                sysbus_mmio_get_region(SYS_BUS_DEVICE(s), 0));
-    qdev_connect_gpio_out_named(dev, "dma-tx", 0, dma[0]);
-    qdev_connect_gpio_out_named(dev, "dma-rx", 0, dma[1]);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
-
-    return dev;
-}
-
 static void omap_mmc_reset_hold(Object *obj, ResetType type)
 {
     OMAPMMCState *s = OMAP_MMC(obj);
-- 
2.34.1

Re: [PATCH 06/11] hw/arm/omap1: Inline creation of MMC

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> Our style for other conversions of OMAP devices to qdev has been to
> inline the creation and wiring into omap310_mpu_init() -- see for
> instance the handling of omap-intc, omap-gpio and omap_i2c. Do
> the same for omap-mmc.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/omap1.c   | 15 +++++++++++----
>   hw/sd/omap_mmc.c | 22 ----------------------
>   2 files changed, 11 insertions(+), 26 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/11] hw/arm/omap1: Inline creation of MMC

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> Our style for other conversions of OMAP devices to qdev has been to
> inline the creation and wiring into omap310_mpu_init() -- see for
> instance the handling of omap-intc, omap-gpio and omap_i2c. Do
> the same for omap-mmc.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/omap1.c   | 15 +++++++++++----
>   hw/sd/omap_mmc.c | 22 ----------------------
>   2 files changed, 11 insertions(+), 26 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 07/11] hw/sd/omap_mmc: Remove unused coverswitch qemu_irq

[Peter Maydell]

The coverswitch qemu_irq is never connected to anything, and the only thing
we do with it is set it in omap_mmc_reset(). Remove it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/omap_mmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index dacbea13aad..07d47421bc8 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -35,7 +35,6 @@ typedef struct OMAPMMCState {
     qemu_irq irq;
     qemu_irq dma_tx_gpio;
     qemu_irq dma_rx_gpio;
-    qemu_irq coverswitch;
     MemoryRegion iomem;
     omap_clk clk;
     uint16_t last_cmd;
@@ -325,7 +324,6 @@ static void omap_mmc_reset(OMAPMMCState *host)
     host->transfer = 0;
     host->cdet_wakeup = 0;
     host->cdet_enable = 0;
-    qemu_set_irq(host->coverswitch, host->cdet_state);
     host->clkdiv = 0;
 
     omap_mmc_pseudo_reset(host);
-- 
2.34.1

Re: [PATCH 07/11] hw/sd/omap_mmc: Remove unused coverswitch qemu_irq

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> The coverswitch qemu_irq is never connected to anything, and the only thing
> we do with it is set it in omap_mmc_reset(). Remove it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/omap_mmc.c | 2 --
>   1 file changed, 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 07/11] hw/sd/omap_mmc: Remove unused coverswitch qemu_irq

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> The coverswitch qemu_irq is never connected to anything, and the only thing
> we do with it is set it in omap_mmc_reset(). Remove it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/omap_mmc.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index dacbea13aad..07d47421bc8 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -35,7 +35,6 @@ typedef struct OMAPMMCState {
>       qemu_irq irq;
>       qemu_irq dma_tx_gpio;
>       qemu_irq dma_rx_gpio;
> -    qemu_irq coverswitch;
>       MemoryRegion iomem;
>       omap_clk clk;
>       uint16_t last_cmd;
> @@ -325,7 +324,6 @@ static void omap_mmc_reset(OMAPMMCState *host)
>       host->transfer = 0;
>       host->cdet_wakeup = 0;
>       host->cdet_enable = 0;
> -    qemu_set_irq(host->coverswitch, host->cdet_state);

OMAPMMCState::cdet_state is now unused:

-- >8 --
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index d31456ad236..031a6f0d333 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -71,3 +71,2 @@ typedef struct OMAPMMCState {
      int cdet_enable;
-    int cdet_state;
      qemu_irq cdet;
---

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 08/11] hw/sd/omap_mmc: Untabify

[Peter Maydell]

This is a very old source file, and still has some lingering
hard-coded tabs; untabify it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I don't propose to try to bring the whole file up to modern
coding style, but hard-coded tabs are a particular wart.
---
 hw/sd/omap_mmc.c | 124 +++++++++++++++++++++++------------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 07d47421bc8..c1598135f44 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -110,11 +110,11 @@ static void omap_mmc_fifolevel_update(OMAPMMCState *host)
 
 /* These must match the encoding of the MMC_CMD Response field */
 typedef enum {
-    sd_nore = 0,	/* no response */
-    sd_r1,		/* normal response command */
-    sd_r2,		/* CID, CSD registers */
-    sd_r3,		/* OCR register */
-    sd_r6 = 6,		/* Published RCA response */
+    sd_nore = 0,        /* no response */
+    sd_r1,              /* normal response command */
+    sd_r2,              /* CID, CSD registers */
+    sd_r3,              /* OCR register */
+    sd_r6 = 6,          /* Published RCA response */
     sd_r1b = -1,
 } sd_rsp_type_t;
 
@@ -230,7 +230,7 @@ static void omap_mmc_command(OMAPMMCState *host, int cmd, int dir,
     if (timeout)
         host->status |= 0x0080;
     else if (cmd == 12)
-        host->status |= 0x0005;	/* Makes it more real */
+        host->status |= 0x0005;         /* Makes it more real */
     else
         host->status |= 0x0001;
 }
@@ -339,32 +339,32 @@ static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
     }
 
     switch (offset) {
-    case 0x00:	/* MMC_CMD */
+    case 0x00:  /* MMC_CMD */
         return s->last_cmd;
 
-    case 0x04:	/* MMC_ARGL */
+    case 0x04:  /* MMC_ARGL */
         return s->arg & 0x0000ffff;
 
-    case 0x08:	/* MMC_ARGH */
+    case 0x08:  /* MMC_ARGH */
         return s->arg >> 16;
 
-    case 0x0c:	/* MMC_CON */
+    case 0x0c:  /* MMC_CON */
         return (s->dw << 15) | (s->mode << 12) | (s->enable << 11) | 
                 (s->be << 10) | s->clkdiv;
 
-    case 0x10:	/* MMC_STAT */
+    case 0x10:  /* MMC_STAT */
         return s->status;
 
-    case 0x14:	/* MMC_IE */
+    case 0x14:  /* MMC_IE */
         return s->mask;
 
-    case 0x18:	/* MMC_CTO */
+    case 0x18:  /* MMC_CTO */
         return s->cto;
 
-    case 0x1c:	/* MMC_DTO */
+    case 0x1c:  /* MMC_DTO */
         return s->dto;
 
-    case 0x20:	/* MMC_DATA */
+    case 0x20:  /* MMC_DATA */
         /* TODO: support 8-bit access */
         i = s->fifo[s->fifo_start];
         if (s->fifo_len == 0) {
@@ -379,42 +379,42 @@ static uint64_t omap_mmc_read(void *opaque, hwaddr offset, unsigned size)
         omap_mmc_interrupts_update(s);
         return i;
 
-    case 0x24:	/* MMC_BLEN */
+    case 0x24:  /* MMC_BLEN */
         return s->blen_counter;
 
-    case 0x28:	/* MMC_NBLK */
+    case 0x28:  /* MMC_NBLK */
         return s->nblk_counter;
 
-    case 0x2c:	/* MMC_BUF */
+    case 0x2c:  /* MMC_BUF */
         return (s->rx_dma << 15) | (s->af_level << 8) |
             (s->tx_dma << 7) | s->ae_level;
 
-    case 0x30:	/* MMC_SPI */
+    case 0x30:  /* MMC_SPI */
         return 0x0000;
-    case 0x34:	/* MMC_SDIO */
+    case 0x34:  /* MMC_SDIO */
         return (s->cdet_wakeup << 2) | (s->cdet_enable) | s->sdio;
-    case 0x38:	/* MMC_SYST */
+    case 0x38:  /* MMC_SYST */
         return 0x0000;
 
-    case 0x3c:	/* MMC_REV */
+    case 0x3c:  /* MMC_REV */
         return s->rev;
 
-    case 0x40:	/* MMC_RSP0 */
-    case 0x44:	/* MMC_RSP1 */
-    case 0x48:	/* MMC_RSP2 */
-    case 0x4c:	/* MMC_RSP3 */
-    case 0x50:	/* MMC_RSP4 */
-    case 0x54:	/* MMC_RSP5 */
-    case 0x58:	/* MMC_RSP6 */
-    case 0x5c:	/* MMC_RSP7 */
+    case 0x40:  /* MMC_RSP0 */
+    case 0x44:  /* MMC_RSP1 */
+    case 0x48:  /* MMC_RSP2 */
+    case 0x4c:  /* MMC_RSP3 */
+    case 0x50:  /* MMC_RSP4 */
+    case 0x54:  /* MMC_RSP5 */
+    case 0x58:  /* MMC_RSP6 */
+    case 0x5c:  /* MMC_RSP7 */
         return s->rsp[(offset - 0x40) >> 2];
 
     /* OMAP2-specific */
-    case 0x60:	/* MMC_IOSR */
-    case 0x64:	/* MMC_SYSC */
+    case 0x60:  /* MMC_IOSR */
+    case 0x64:  /* MMC_SYSC */
         return 0;
-    case 0x68:	/* MMC_SYSS */
-        return 1;						/* RSTD */
+    case 0x68:  /* MMC_SYSS */
+        return 1;                                               /* RSTD */
     }
 
     OMAP_BAD_REG(offset);
@@ -433,7 +433,7 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
     }
 
     switch (offset) {
-    case 0x00:	/* MMC_CMD */
+    case 0x00:  /* MMC_CMD */
         if (!s->enable)
             break;
 
@@ -448,17 +448,17 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
         omap_mmc_update(s);
         break;
 
-    case 0x04:	/* MMC_ARGL */
+    case 0x04:  /* MMC_ARGL */
         s->arg &= 0xffff0000;
         s->arg |= 0x0000ffff & value;
         break;
 
-    case 0x08:	/* MMC_ARGH */
+    case 0x08:  /* MMC_ARGH */
         s->arg &= 0x0000ffff;
         s->arg |= value << 16;
         break;
 
-    case 0x0c:	/* MMC_CON */
+    case 0x0c:  /* MMC_CON */
         s->dw = (value >> 15) & 1;
         s->mode = (value >> 12) & 3;
         s->enable = (value >> 11) & 1;
@@ -478,27 +478,27 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
             omap_mmc_pseudo_reset(s);
         break;
 
-    case 0x10:	/* MMC_STAT */
+    case 0x10:  /* MMC_STAT */
         s->status &= ~value;
         omap_mmc_interrupts_update(s);
         break;
 
-    case 0x14:	/* MMC_IE */
+    case 0x14:  /* MMC_IE */
         s->mask = value & 0x7fff;
         omap_mmc_interrupts_update(s);
         break;
 
-    case 0x18:	/* MMC_CTO */
+    case 0x18:  /* MMC_CTO */
         s->cto = value & 0xff;
         if (s->cto > 0xfd && s->rev <= 1)
             printf("MMC: CTO of 0xff and 0xfe cannot be used!\n");
         break;
 
-    case 0x1c:	/* MMC_DTO */
+    case 0x1c:  /* MMC_DTO */
         s->dto = value & 0xffff;
         break;
 
-    case 0x20:	/* MMC_DATA */
+    case 0x20:  /* MMC_DATA */
         /* TODO: support 8-bit access */
         if (s->fifo_len == 32)
             break;
@@ -509,18 +509,18 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
         omap_mmc_interrupts_update(s);
         break;
 
-    case 0x24:	/* MMC_BLEN */
+    case 0x24:  /* MMC_BLEN */
         s->blen = (value & 0x07ff) + 1;
         s->blen_counter = s->blen;
         break;
 
-    case 0x28:	/* MMC_NBLK */
+    case 0x28:  /* MMC_NBLK */
         s->nblk = (value & 0x07ff) + 1;
         s->nblk_counter = s->nblk;
         s->blen_counter = s->blen;
         break;
 
-    case 0x2c:	/* MMC_BUF */
+    case 0x2c:  /* MMC_BUF */
         s->rx_dma = (value >> 15) & 1;
         s->af_level = (value >> 8) & 0x1f;
         s->tx_dma = (value >> 7) & 1;
@@ -535,38 +535,38 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
         break;
 
     /* SPI, SDIO and TEST modes unimplemented */
-    case 0x30:	/* MMC_SPI (OMAP1 only) */
+    case 0x30:  /* MMC_SPI (OMAP1 only) */
         break;
-    case 0x34:	/* MMC_SDIO */
+    case 0x34:  /* MMC_SDIO */
         s->sdio = value & (s->rev >= 2 ? 0xfbf3 : 0x2020);
         s->cdet_wakeup = (value >> 9) & 1;
         s->cdet_enable = (value >> 2) & 1;
         break;
-    case 0x38:	/* MMC_SYST */
+    case 0x38:  /* MMC_SYST */
         break;
 
-    case 0x3c:	/* MMC_REV */
-    case 0x40:	/* MMC_RSP0 */
-    case 0x44:	/* MMC_RSP1 */
-    case 0x48:	/* MMC_RSP2 */
-    case 0x4c:	/* MMC_RSP3 */
-    case 0x50:	/* MMC_RSP4 */
-    case 0x54:	/* MMC_RSP5 */
-    case 0x58:	/* MMC_RSP6 */
-    case 0x5c:	/* MMC_RSP7 */
+    case 0x3c:  /* MMC_REV */
+    case 0x40:  /* MMC_RSP0 */
+    case 0x44:  /* MMC_RSP1 */
+    case 0x48:  /* MMC_RSP2 */
+    case 0x4c:  /* MMC_RSP3 */
+    case 0x50:  /* MMC_RSP4 */
+    case 0x54:  /* MMC_RSP5 */
+    case 0x58:  /* MMC_RSP6 */
+    case 0x5c:  /* MMC_RSP7 */
         OMAP_RO_REG(offset);
         break;
 
     /* OMAP2-specific */
-    case 0x60:	/* MMC_IOSR */
+    case 0x60:  /* MMC_IOSR */
         if (value & 0xf)
             printf("MMC: SDIO bits used!\n");
         break;
-    case 0x64:	/* MMC_SYSC */
-        if (value & (1 << 2))					/* SRTS */
+    case 0x64:  /* MMC_SYSC */
+        if (value & (1 << 2))                                   /* SRTS */
             omap_mmc_reset(s);
         break;
-    case 0x68:	/* MMC_SYSS */
+    case 0x68:  /* MMC_SYSS */
         OMAP_RO_REG(offset);
         break;
 
-- 
2.34.1

Re: [PATCH 08/11] hw/sd/omap_mmc: Untabify

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> This is a very old source file, and still has some lingering
> hard-coded tabs; untabify it.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I don't propose to try to bring the whole file up to modern
> coding style, but hard-coded tabs are a particular wart.
> ---
>   hw/sd/omap_mmc.c | 124 +++++++++++++++++++++++------------------------
>   1 file changed, 62 insertions(+), 62 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/11] hw/sd/omap_mmc: Untabify

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> This is a very old source file, and still has some lingering
> hard-coded tabs; untabify it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I don't propose to try to bring the whole file up to modern
> coding style, but hard-coded tabs are a particular wart.
> ---
>   hw/sd/omap_mmc.c | 124 +++++++++++++++++++++++------------------------
>   1 file changed, 62 insertions(+), 62 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 09/11] hw/sd: Remove unused 'enable' method from SDCardClass

[Peter Maydell]

The SDCardClass has an 'enable' method, but nothing actually invokes it.
The underlying implementation is sd_enable(), which is documented
in sdcard_legacy.h as something that should not be used and was only
present for the benefit of the now-removed nseries boards. Unlike
all the other method pointers in SDCardClass, this one doesn't have
an sdbus_foo() function wrapper in hw/sd/core.c.

Remove the unused method.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sd/sd.h | 1 -
 hw/sd/sd.c         | 1 -
 2 files changed, 2 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index f2458f37b3c..d6bad175131 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -119,7 +119,6 @@ struct SDCardClass {
     void (*set_voltage)(SDState *sd, uint16_t millivolts);
     uint8_t (*get_dat_lines)(SDState *sd);
     bool (*get_cmd_line)(SDState *sd);
-    void (*enable)(SDState *sd, bool enable);
     bool (*get_inserted)(SDState *sd);
     bool (*get_readonly)(SDState *sd);
     void (*set_cid)(SDState *sd);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0330d432fd0..f781fd1641d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2831,7 +2831,6 @@ static void sdmmc_common_class_init(ObjectClass *klass, void *data)
     sc->read_byte = sd_read_byte;
     sc->receive_ready = sd_receive_ready;
     sc->data_ready = sd_data_ready;
-    sc->enable = sd_enable;
     sc->get_inserted = sd_get_inserted;
     sc->get_readonly = sd_get_readonly;
 }
-- 
2.34.1

Re: [PATCH 09/11] hw/sd: Remove unused 'enable' method from SDCardClass

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> The SDCardClass has an 'enable' method, but nothing actually invokes it.
> The underlying implementation is sd_enable(), which is documented
> in sdcard_legacy.h as something that should not be used and was only
> present for the benefit of the now-removed nseries boards. Unlike
> all the other method pointers in SDCardClass, this one doesn't have
> an sdbus_foo() function wrapper in hw/sd/core.c.
> 
> Remove the unused method.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/sd/sd.h | 1 -
>   hw/sd/sd.c         | 1 -
>   2 files changed, 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 09/11] hw/sd: Remove unused 'enable' method from SDCardClass

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> The SDCardClass has an 'enable' method, but nothing actually invokes it.
> The underlying implementation is sd_enable(), which is documented
> in sdcard_legacy.h as something that should not be used and was only
> present for the benefit of the now-removed nseries boards. Unlike
> all the other method pointers in SDCardClass, this one doesn't have
> an sdbus_foo() function wrapper in hw/sd/core.c.

See commit c759a790b67 :)

commit c759a790b672b0c5bfc50520dcc93565b55732b3
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Thu Feb 18 14:16:18 2016 +0000

     hw/sd: Add QOM bus which SD cards plug in to

     Add a QOM bus for SD cards to plug in to.

     Note that since sd_enable() is used only by one board and there
     only as part of a broken implementation, we do not provide it in
     the SDBus API (but instead add a warning comment about the old
     function). Whoever converts OMAP and the nseries boards to QOM
     will need to either implement the card switch properly or move
     the enable hack into the OMAP MMC controller model.

> 
> Remove the unused method.

Never used, indeed.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sd/sd.h | 1 -
>   hw/sd/sd.c         | 1 -
>   2 files changed, 2 deletions(-)

[PATCH 10/11] hw/sd: Remove unused legacy functions, stop killing mammoths

[Peter Maydell]

The sdcard_legacy.h header defines function prototypes for the "legacy"
SD card API, which was used by non-qdevified SD controller models.
We've now converted the only remaining non-qdev SD controller, so
we can drop the legacy API.

Entirely unused functions:
  sd_init(), sd_set_cb(), sd_enable()

Functions which now become static inside sd.c (they are the
underlying implementations of methods on SDCardClass):
  sd_do_command(), sd_write_byte(), sd_read_byte()

Removal of sd_init() means that we can also remove the
me_no_qdev_me_kill_mammoth_with_rocks flag, the codepaths that were
only reachable when it was set, and the inserted_cb and readonly_cb
qemu_irq lines that went with that.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sd/sdcard_legacy.h | 50 -----------------------
 hw/sd/omap_mmc.c              |  2 +-
 hw/sd/sd.c                    | 77 ++++-------------------------------
 3 files changed, 9 insertions(+), 120 deletions(-)
 delete mode 100644 include/hw/sd/sdcard_legacy.h

diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
deleted file mode 100644
index 0dc38895551..00000000000
--- a/include/hw/sd/sdcard_legacy.h
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * SD Memory Card emulation (deprecated legacy API)
- *
- * Copyright (c) 2006 Andrzej Zaborowski  <balrog@zabor.org>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in
- *    the documentation and/or other materials provided with the
- *    distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
- * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
- * PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
- * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-#ifndef HW_SDCARD_LEGACY_H
-#define HW_SDCARD_LEGACY_H
-
-#include "hw/sd/sd.h"
-
-/* Legacy functions to be used only by non-qdevified callers */
-SDState *sd_init(BlockBackend *blk, bool is_spi);
-int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
-void sd_write_byte(SDState *card, uint8_t value);
-uint8_t sd_read_byte(SDState *card);
-void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
-
-/* sd_enable should not be used -- it is only used on the nseries boards,
- * where it is part of a broken implementation of the MMC card slot switch
- * (there should be two card slots which are multiplexed to a single MMC
- * controller, but instead we model it with one card and controller and
- * disable the card when the second slot is selected, so it looks like the
- * second slot is always empty).
- */
-void sd_enable(SDState *card, bool enable);
-
-#endif /* HW_SDCARD_LEGACY_H */
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index c1598135f44..d31456ad236 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -25,7 +25,7 @@
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "hw/arm/omap.h"
-#include "hw/sd/sdcard_legacy.h"
+#include "hw/sd/sd.h"
 
 typedef struct OMAPMMCState {
     SysBusDevice parent_obj;
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f781fd1641d..74bb7f39bbf 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -39,7 +39,6 @@
 #include "hw/registerfields.h"
 #include "system/block-backend.h"
 #include "hw/sd/sd.h"
-#include "hw/sd/sdcard_legacy.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/bitmap.h"
@@ -120,10 +119,6 @@ typedef struct SDProto {
 struct SDState {
     DeviceState parent_obj;
 
-    /* If true, created by sd_init() for a non-qdevified caller */
-    /* TODO purge them with fire */
-    bool me_no_qdev_me_kill_mammoth_with_rocks;
-
     /* SD Memory Card Registers */
     uint32_t ocr;
     uint8_t scr[8];
@@ -177,8 +172,6 @@ struct SDState {
     uint32_t data_offset;
     size_t data_size;
     uint8_t data[512];
-    qemu_irq readonly_cb;
-    qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
     bool enable;
     uint8_t dat_lines;
@@ -892,17 +885,10 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
         trace_sdcard_ejected();
     }
 
-    if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
-        qemu_set_irq(sd->inserted_cb, inserted);
-        if (inserted) {
-            qemu_set_irq(sd->readonly_cb, readonly);
-        }
-    } else {
-        sdbus = SD_BUS(qdev_get_parent_bus(dev));
-        sdbus_set_inserted(sdbus, inserted);
-        if (inserted) {
-            sdbus_set_readonly(sdbus, readonly);
-        }
+    sdbus = SD_BUS(qdev_get_parent_bus(dev));
+    sdbus_set_inserted(sdbus, inserted);
+    if (inserted) {
+        sdbus_set_readonly(sdbus, readonly);
     }
 }
 
@@ -1000,48 +986,6 @@ static const VMStateDescription sd_vmstate = {
     },
 };
 
-/* Legacy initialization function for use by non-qdevified callers */
-SDState *sd_init(BlockBackend *blk, bool is_spi)
-{
-    Object *obj;
-    DeviceState *dev;
-    SDState *sd;
-    Error *err = NULL;
-
-    obj = object_new(is_spi ? TYPE_SD_CARD_SPI : TYPE_SD_CARD);
-    dev = DEVICE(obj);
-    if (!qdev_prop_set_drive_err(dev, "drive", blk, &err)) {
-        error_reportf_err(err, "sd_init failed: ");
-        return NULL;
-    }
-
-    /*
-     * Realizing the device properly would put it into the QOM
-     * composition tree even though it is not plugged into an
-     * appropriate bus.  That's a no-no.  Hide the device from
-     * QOM/qdev, and call its qdev realize callback directly.
-     */
-    object_ref(obj);
-    object_unparent(obj);
-    sd_realize(dev, &err);
-    if (err) {
-        error_reportf_err(err, "sd_init failed: ");
-        return NULL;
-    }
-
-    sd = SD_CARD(dev);
-    sd->me_no_qdev_me_kill_mammoth_with_rocks = true;
-    return sd;
-}
-
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
-{
-    sd->readonly_cb = readonly;
-    sd->inserted_cb = insert;
-    qemu_set_irq(readonly, sd->blk ? !blk_is_writable(sd->blk) : 0);
-    qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
-}
-
 static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
     trace_sdcard_read_block(addr, len);
@@ -2196,8 +2140,8 @@ static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
     return cmd_class == 0 || cmd_class == 7;
 }
 
-int sd_do_command(SDState *sd, SDRequest *req,
-                  uint8_t *response) {
+static int sd_do_command(SDState *sd, SDRequest *req,
+                         uint8_t *response) {
     int last_state;
     sd_rsp_type_t rtype;
     int rsplen;
@@ -2349,7 +2293,7 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
     return false;
 }
 
-void sd_write_byte(SDState *sd, uint8_t value)
+static void sd_write_byte(SDState *sd, uint8_t value)
 {
     int i;
 
@@ -2478,7 +2422,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
     }
 }
 
-uint8_t sd_read_byte(SDState *sd)
+static uint8_t sd_read_byte(SDState *sd)
 {
     /* TODO: Append CRCs */
     const uint8_t dummy_byte = 0x00;
@@ -2561,11 +2505,6 @@ static bool sd_data_ready(SDState *sd)
     return sd->state == sd_sendingdata_state;
 }
 
-void sd_enable(SDState *sd, bool enable)
-{
-    sd->enable = enable;
-}
-
 static const SDProto sd_proto_spi = {
     .name = "SPI",
     .cmd = {
-- 
2.34.1

Re: [PATCH 10/11] hw/sd: Remove unused legacy functions, stop killing mammoths

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> The sdcard_legacy.h header defines function prototypes for the "legacy"
> SD card API, which was used by non-qdevified SD controller models.
> We've now converted the only remaining non-qdev SD controller, so
> we can drop the legacy API.
> 
> Entirely unused functions:
>    sd_init(), sd_set_cb(), sd_enable()
> 
> Functions which now become static inside sd.c (they are the
> underlying implementations of methods on SDCardClass):
>    sd_do_command(), sd_write_byte(), sd_read_byte()
> 
> Removal of sd_init() means that we can also remove the
> me_no_qdev_me_kill_mammoth_with_rocks flag, the codepaths that were
> only reachable when it was set, and the inserted_cb and readonly_cb
> qemu_irq lines that went with that.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/sd/sdcard_legacy.h | 50 -----------------------
>   hw/sd/omap_mmc.c              |  2 +-
>   hw/sd/sd.c                    | 77 ++++-------------------------------
>   3 files changed, 9 insertions(+), 120 deletions(-)
>   delete mode 100644 include/hw/sd/sdcard_legacy.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 10/11] hw/sd: Remove unused legacy functions, stop killing mammoths

[Philippe Mathieu-Daudé]

+Markus for commit 007d1dbf725 ("sd: Hide the qdev-but-not-quite thing
created by sd_init()").

On 28/1/25 11:45, Peter Maydell wrote:
> The sdcard_legacy.h header defines function prototypes for the "legacy"
> SD card API, which was used by non-qdevified SD controller models.
> We've now converted the only remaining non-qdev SD controller, so
> we can drop the legacy API.
> 
> Entirely unused functions:
>    sd_init(), sd_set_cb(), sd_enable()
> 
> Functions which now become static inside sd.c (they are the
> underlying implementations of methods on SDCardClass):
>    sd_do_command(), sd_write_byte(), sd_read_byte()
> 
> Removal of sd_init() means that we can also remove the
> me_no_qdev_me_kill_mammoth_with_rocks flag, the codepaths that were
> only reachable when it was set, and the inserted_cb and readonly_cb
> qemu_irq lines that went with that.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sd/sdcard_legacy.h | 50 -----------------------
>   hw/sd/omap_mmc.c              |  2 +-
>   hw/sd/sd.c                    | 77 ++++-------------------------------
>   3 files changed, 9 insertions(+), 120 deletions(-)
>   delete mode 100644 include/hw/sd/sdcard_legacy.h
> 
> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
> deleted file mode 100644
> index 0dc38895551..00000000000
> --- a/include/hw/sd/sdcard_legacy.h
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/*
> - * SD Memory Card emulation (deprecated legacy API)
> - *
> - * Copyright (c) 2006 Andrzej Zaborowski  <balrog@zabor.org>
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - *
> - * 1. Redistributions of source code must retain the above copyright
> - *    notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - *    notice, this list of conditions and the following disclaimer in
> - *    the documentation and/or other materials provided with the
> - *    distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
> - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> - * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> - * PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
> - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> - * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> - * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> - * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> - * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> - */
> -#ifndef HW_SDCARD_LEGACY_H
> -#define HW_SDCARD_LEGACY_H
> -
> -#include "hw/sd/sd.h"
> -
> -/* Legacy functions to be used only by non-qdevified callers */
> -SDState *sd_init(BlockBackend *blk, bool is_spi);
> -int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
> -void sd_write_byte(SDState *card, uint8_t value);
> -uint8_t sd_read_byte(SDState *card);
> -void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
> -
> -/* sd_enable should not be used -- it is only used on the nseries boards,
> - * where it is part of a broken implementation of the MMC card slot switch
> - * (there should be two card slots which are multiplexed to a single MMC
> - * controller, but instead we model it with one card and controller and
> - * disable the card when the second slot is selected, so it looks like the
> - * second slot is always empty).
> - */
> -void sd_enable(SDState *card, bool enable);
> -
> -#endif /* HW_SDCARD_LEGACY_H */
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index c1598135f44..d31456ad236 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -25,7 +25,7 @@
>   #include "hw/irq.h"
>   #include "hw/sysbus.h"
>   #include "hw/arm/omap.h"
> -#include "hw/sd/sdcard_legacy.h"
> +#include "hw/sd/sd.h"

This hunk belongs to patch #4 "hw/sd/omap_mmc: Convert to SDBus API",
otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   
>   typedef struct OMAPMMCState {
>       SysBusDevice parent_obj;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index f781fd1641d..74bb7f39bbf 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -39,7 +39,6 @@
>   #include "hw/registerfields.h"
>   #include "system/block-backend.h"
>   #include "hw/sd/sd.h"
> -#include "hw/sd/sdcard_legacy.h"
>   #include "migration/vmstate.h"
>   #include "qapi/error.h"
>   #include "qemu/bitmap.h"
> @@ -120,10 +119,6 @@ typedef struct SDProto {
>   struct SDState {
>       DeviceState parent_obj;
>   
> -    /* If true, created by sd_init() for a non-qdevified caller */
> -    /* TODO purge them with fire */
> -    bool me_no_qdev_me_kill_mammoth_with_rocks;
> -
>       /* SD Memory Card Registers */
>       uint32_t ocr;
>       uint8_t scr[8];
> @@ -177,8 +172,6 @@ struct SDState {
>       uint32_t data_offset;
>       size_t data_size;
>       uint8_t data[512];
> -    qemu_irq readonly_cb;
> -    qemu_irq inserted_cb;
>       QEMUTimer *ocr_power_timer;
>       bool enable;
>       uint8_t dat_lines;
> @@ -892,17 +885,10 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>           trace_sdcard_ejected();
>       }
>   
> -    if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
> -        qemu_set_irq(sd->inserted_cb, inserted);
> -        if (inserted) {
> -            qemu_set_irq(sd->readonly_cb, readonly);
> -        }
> -    } else {
> -        sdbus = SD_BUS(qdev_get_parent_bus(dev));
> -        sdbus_set_inserted(sdbus, inserted);
> -        if (inserted) {
> -            sdbus_set_readonly(sdbus, readonly);
> -        }
> +    sdbus = SD_BUS(qdev_get_parent_bus(dev));
> +    sdbus_set_inserted(sdbus, inserted);
> +    if (inserted) {
> +        sdbus_set_readonly(sdbus, readonly);
>       }
>   }
>   
> @@ -1000,48 +986,6 @@ static const VMStateDescription sd_vmstate = {
>       },
>   };
>   
> -/* Legacy initialization function for use by non-qdevified callers */
> -SDState *sd_init(BlockBackend *blk, bool is_spi)
> -{
> -    Object *obj;
> -    DeviceState *dev;
> -    SDState *sd;
> -    Error *err = NULL;
> -
> -    obj = object_new(is_spi ? TYPE_SD_CARD_SPI : TYPE_SD_CARD);
> -    dev = DEVICE(obj);
> -    if (!qdev_prop_set_drive_err(dev, "drive", blk, &err)) {
> -        error_reportf_err(err, "sd_init failed: ");
> -        return NULL;
> -    }
> -
> -    /*
> -     * Realizing the device properly would put it into the QOM
> -     * composition tree even though it is not plugged into an
> -     * appropriate bus.  That's a no-no.  Hide the device from
> -     * QOM/qdev, and call its qdev realize callback directly.
> -     */
> -    object_ref(obj);
> -    object_unparent(obj);
> -    sd_realize(dev, &err);
> -    if (err) {
> -        error_reportf_err(err, "sd_init failed: ");
> -        return NULL;
> -    }
> -
> -    sd = SD_CARD(dev);
> -    sd->me_no_qdev_me_kill_mammoth_with_rocks = true;
> -    return sd;
> -}
> -
> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> -{
> -    sd->readonly_cb = readonly;
> -    sd->inserted_cb = insert;
> -    qemu_set_irq(readonly, sd->blk ? !blk_is_writable(sd->blk) : 0);
> -    qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
> -}
> -
>   static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>   {
>       trace_sdcard_read_block(addr, len);
> @@ -2196,8 +2140,8 @@ static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
>       return cmd_class == 0 || cmd_class == 7;
>   }
>   
> -int sd_do_command(SDState *sd, SDRequest *req,
> -                  uint8_t *response) {
> +static int sd_do_command(SDState *sd, SDRequest *req,
> +                         uint8_t *response) {
>       int last_state;
>       sd_rsp_type_t rtype;
>       int rsplen;
> @@ -2349,7 +2293,7 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
>       return false;
>   }
>   
> -void sd_write_byte(SDState *sd, uint8_t value)
> +static void sd_write_byte(SDState *sd, uint8_t value)
>   {
>       int i;
>   
> @@ -2478,7 +2422,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>       }
>   }
>   
> -uint8_t sd_read_byte(SDState *sd)
> +static uint8_t sd_read_byte(SDState *sd)
>   {
>       /* TODO: Append CRCs */
>       const uint8_t dummy_byte = 0x00;
> @@ -2561,11 +2505,6 @@ static bool sd_data_ready(SDState *sd)
>       return sd->state == sd_sendingdata_state;
>   }
>   
> -void sd_enable(SDState *sd, bool enable)
> -{
> -    sd->enable = enable;
> -}
> -
>   static const SDProto sd_proto_spi = {
>       .name = "SPI",
>       .cmd = {

Re: [PATCH 10/11] hw/sd: Remove unused legacy functions, stop killing mammoths

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> The sdcard_legacy.h header defines function prototypes for the "legacy"
> SD card API, which was used by non-qdevified SD controller models.
> We've now converted the only remaining non-qdev SD controller, so
> we can drop the legacy API.
>
> Entirely unused functions:
>   sd_init(), sd_set_cb(), sd_enable()
>
> Functions which now become static inside sd.c (they are the
> underlying implementations of methods on SDCardClass):
>   sd_do_command(), sd_write_byte(), sd_read_byte()
>
> Removal of sd_init() means that we can also remove the
> me_no_qdev_me_kill_mammoth_with_rocks flag, the codepaths that were
> only reachable when it was set, and the inserted_cb and readonly_cb
> qemu_irq lines that went with that.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Very much appreciated!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 11/11] hw/sd: Remove unused SDState::enable

[Peter Maydell]

Now that sd_enable() has been removed, SD::enable is set to true in
sd_instance_init() and then never changed. So we can remove it.

Note that the VMSTATE_UNUSED() size argument should be '1', not
'sizeof(bool)', as noted in the CAUTION comment in vmstate.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sd.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 74bb7f39bbf..e541c57f8c3 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -173,7 +173,6 @@ struct SDState {
     size_t data_size;
     uint8_t data[512];
     QEMUTimer *ocr_power_timer;
-    bool enable;
     uint8_t dat_lines;
     bool cmd_line;
 };
@@ -292,12 +291,12 @@ static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
 
 static uint8_t sd_get_dat_lines(SDState *sd)
 {
-    return sd->enable ? sd->dat_lines : 0;
+    return sd->dat_lines;
 }
 
 static bool sd_get_cmd_line(SDState *sd)
 {
-    return sd->enable ? sd->cmd_line : false;
+    return sd->cmd_line;
 }
 
 static void sd_set_voltage(SDState *sd, uint16_t millivolts)
@@ -976,7 +975,7 @@ static const VMStateDescription sd_vmstate = {
         VMSTATE_UINT32(data_offset, SDState),
         VMSTATE_UINT8_ARRAY(data, SDState, 512),
         VMSTATE_UNUSED_V(1, 512),
-        VMSTATE_BOOL(enable, SDState),
+        VMSTATE_UNUSED(1),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * const []) {
@@ -2146,7 +2145,7 @@ static int sd_do_command(SDState *sd, SDRequest *req,
     sd_rsp_type_t rtype;
     int rsplen;
 
-    if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) {
+    if (!sd->blk || !blk_is_inserted(sd->blk)) {
         return 0;
     }
 
@@ -2297,8 +2296,9 @@ static void sd_write_byte(SDState *sd, uint8_t value)
 {
     int i;
 
-    if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable)
+    if (!sd->blk || !blk_is_inserted(sd->blk)) {
         return;
+    }
 
     if (sd->state != sd_receivingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -2429,8 +2429,9 @@ static uint8_t sd_read_byte(SDState *sd)
     uint8_t ret;
     uint32_t io_len;
 
-    if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable)
+    if (!sd->blk || !blk_is_inserted(sd->blk)) {
         return dummy_byte;
+    }
 
     if (sd->state != sd_sendingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
@@ -2664,7 +2665,6 @@ static void sd_instance_init(Object *obj)
 
     sd->proto = sc->proto;
     sd->last_cmd_name = "UNSET";
-    sd->enable = true;
     sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
 }
 
-- 
2.34.1

Re: [PATCH 11/11] hw/sd: Remove unused SDState::enable

[Richard Henderson]

On 1/28/25 02:45, Peter Maydell wrote:
> Now that sd_enable() has been removed,SD::enable is set to true in
> sd_instance_init() and then never changed. So we can remove it.
> 
> Note that the VMSTATE_UNUSED() size argument should be '1', not
> 'sizeof(bool)', as noted in the CAUTION comment in vmstate.h.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/sd/sd.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 11/11] hw/sd: Remove unused SDState::enable

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> Now that sd_enable() has been removed, SD::enable is set to true in
> sd_instance_init() and then never changed. So we can remove it.
> 
> Note that the VMSTATE_UNUSED() size argument should be '1', not
> 'sizeof(bool)', as noted in the CAUTION comment in vmstate.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/sd/sd.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 00/11] hw/sd: QOMify omap-mmc, remove legacy APIs

[Philippe Mathieu-Daudé]

On 28/1/25 11:45, Peter Maydell wrote:
> This series QOMifies the omap-mmc device. The main reason for
> this is that this device is now the only remaining in-tree
> user of the legacy SD APIs defined in sdcard_legacy.h.
> The first 8 patches QOMify the device and do some minor
> cleanup on it. Patches 9 to 11 then remove the unused
> legacy APIs, deleting sdcard_legacy.h entirely. This includes
> letting us get rid of the me_no_qdev_me_kill_mammoth_with_rocks
> codepaths in sd.c.

Thanks for this long due cleanup!

Series queued squashing:

-- >8 --
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index d31456ad236..bbe7b971bbe 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -71,3 +71,2 @@ typedef struct OMAPMMCState {
      int cdet_enable;
-    int cdet_state;
      qemu_irq cdet;
@@ -611,9 +610,4 @@ static void omap_mmc_initfn(Object *obj)
      qdev_init_gpio_out_named(DEVICE(obj), &s->dma_rx_gpio, "dma-rx", 1);
-}

-static void omap_mmc_realize(DeviceState *dev, Error **errp)
-{
-    OMAPMMCState *s = OMAP_MMC(dev);
-
-    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, dev, "sd-bus");
+    qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, DEVICE(obj), 
"sd-bus");
  }
@@ -622,3 +616,2 @@ static void omap_mmc_class_init(ObjectClass *oc, 
void *data)
  {
-    DeviceClass *dc = DEVICE_CLASS(oc);
      ResettableClass *rc = RESETTABLE_CLASS(oc);
@@ -626,3 +619,2 @@ static void omap_mmc_class_init(ObjectClass *oc, 
void *data)
      rc->phases.hold = omap_mmc_reset_hold;
-    dc->realize = omap_mmc_realize;
  }
---