sd_set_cb() was only used by omap2_mmc_init() which
got recently removed. Time to remove it. For historical
background on the me_no_qdev_me_kill_mammoth_with_rocks
kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
thing created by sd_init()").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/sd/sdcard_legacy.h |  1 -
 hw/sd/sd.c                    | 30 ++++--------------------------
 2 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
index 0dc3889555..a121232560 100644
--- a/include/hw/sd/sdcard_legacy.h
+++ b/include/hw/sd/sdcard_legacy.h
@@ -36,7 +36,6 @@ 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
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a140a32ccd..8a30c61ce0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -112,10 +112,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];
@@ -169,8 +165,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;
@@ -889,17 +883,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);
     }
 }
 
@@ -1027,18 +1014,9 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
     }
 
     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);
-- 
2.45.2

On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> sd_set_cb() was only used by omap2_mmc_init() which
> got recently removed. Time to remove it. For historical
> background on the me_no_qdev_me_kill_mammoth_with_rocks
> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
> thing created by sd_init()").
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/sd/sdcard_legacy.h |  1 -
>  hw/sd/sd.c                    | 30 ++++--------------------------
>  2 files changed, 4 insertions(+), 27 deletions(-)
>
> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
> index 0dc3889555..a121232560 100644
> --- a/include/hw/sd/sdcard_legacy.h
> +++ b/include/hw/sd/sdcard_legacy.h
> @@ -36,7 +36,6 @@ 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
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a140a32ccd..8a30c61ce0 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -112,10 +112,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;
> -

Should we also remove the sd_init() function in this patch
(or patchset)? It was only used by the omap-mmc, and it's
because we have no uses of it that we can get rid of this kludge.

thanks
-- PMM

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

> On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> sd_set_cb() was only used by omap2_mmc_init() which
>> got recently removed. Time to remove it. For historical
>> background on the me_no_qdev_me_kill_mammoth_with_rocks
>> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
>> thing created by sd_init()").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>  include/hw/sd/sdcard_legacy.h |  1 -
>>  hw/sd/sd.c                    | 30 ++++--------------------------
>>  2 files changed, 4 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
>> index 0dc3889555..a121232560 100644
>> --- a/include/hw/sd/sdcard_legacy.h
>> +++ b/include/hw/sd/sdcard_legacy.h
>> @@ -36,7 +36,6 @@ 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
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index a140a32ccd..8a30c61ce0 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -112,10 +112,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;
>> -
>
> Should we also remove the sd_init() function in this patch
> (or patchset)? It was only used by the omap-mmc, and it's
> because we have no uses of it that we can get rid of this kludge.

sd_init() is a legacy initialization function for use by non-qdevified
callers.  I'd *love* to finally get rid of it.  However, there seems to
be a use left in tree even after "[PATCH for-9.2 00/53] arm: Drop
deprecated boards": omap_mmc_init(), used by sx1_init() via via
omap310_mpu_init().  This is machines sx1 and sx1-v1.

Ignorant question: can we deprecate these?

On Fri, 13 Sept 2024 at 09:28, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> sd_set_cb() was only used by omap2_mmc_init() which
> >> got recently removed. Time to remove it. For historical
> >> background on the me_no_qdev_me_kill_mammoth_with_rocks
> >> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
> >> thing created by sd_init()").
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>  include/hw/sd/sdcard_legacy.h |  1 -
> >>  hw/sd/sd.c                    | 30 ++++--------------------------
> >>  2 files changed, 4 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
> >> index 0dc3889555..a121232560 100644
> >> --- a/include/hw/sd/sdcard_legacy.h
> >> +++ b/include/hw/sd/sdcard_legacy.h
> >> @@ -36,7 +36,6 @@ 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
> >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >> index a140a32ccd..8a30c61ce0 100644
> >> --- a/hw/sd/sd.c
> >> +++ b/hw/sd/sd.c
> >> @@ -112,10 +112,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;
> >> -
> >
> > Should we also remove the sd_init() function in this patch
> > (or patchset)? It was only used by the omap-mmc, and it's
> > because we have no uses of it that we can get rid of this kludge.
>
> sd_init() is a legacy initialization function for use by non-qdevified
> callers.  I'd *love* to finally get rid of it.  However, there seems to
> be a use left in tree even after "[PATCH for-9.2 00/53] arm: Drop
> deprecated boards": omap_mmc_init(), used by sx1_init() via via
> omap310_mpu_init().  This is machines sx1 and sx1-v1.

Ah, I hadn't noticed that. I'll have a re-read of this
patch based on that knowledge...

> Ignorant question: can we deprecate these?

We put them up as candidates when we were deprecating the
rest of this, but the feedback was that kernel developers
were still using sx1:
https://lore.kernel.org/qemu-devel/20240214012749.GA203324@darkstar.musicnaut.iki.fi/

It is indeed a bit of a pity from our end that we couldn't
drop all of the OMAP code entirely. We might get another
chance after the next round of kernel machine type culling
if they drop armv4t.

Once my patchset to drop all these Arm machines has got
code review and gets into git we can reassess what we
still have and look at modernising the stuff we've kept.

thanks
-- PMM

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

> On Fri, 13 Sept 2024 at 09:28, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>> >>
>> >> sd_set_cb() was only used by omap2_mmc_init() which
>> >> got recently removed. Time to remove it. For historical
>> >> background on the me_no_qdev_me_kill_mammoth_with_rocks
>> >> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
>> >> thing created by sd_init()").
>> >>
>> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[...]

>> > Should we also remove the sd_init() function in this patch
>> > (or patchset)? It was only used by the omap-mmc, and it's
>> > because we have no uses of it that we can get rid of this kludge.
>>
>> sd_init() is a legacy initialization function for use by non-qdevified
>> callers.  I'd *love* to finally get rid of it.  However, there seems to
>> be a use left in tree even after "[PATCH for-9.2 00/53] arm: Drop
>> deprecated boards": omap_mmc_init(), used by sx1_init() via via
>> omap310_mpu_init().  This is machines sx1 and sx1-v1.
>
> Ah, I hadn't noticed that. I'll have a re-read of this
> patch based on that knowledge...
>
>> Ignorant question: can we deprecate these?
>
> We put them up as candidates when we were deprecating the
> rest of this, but the feedback was that kernel developers
> were still using sx1:
> https://lore.kernel.org/qemu-devel/20240214012749.GA203324@darkstar.musicnaut.iki.fi/
>
> It is indeed a bit of a pity from our end that we couldn't
> drop all of the OMAP code entirely. We might get another
> chance after the next round of kernel machine type culling
> if they drop armv4t.
>
> Once my patchset to drop all these Arm machines has got
> code review and gets into git we can reassess what we
> still have and look at modernising the stuff we've kept.

Makes sense.  Thanks!

(Cc'ing Guenter who asked to keep the SX1 machine)

On 3/9/24 22:04, Philippe Mathieu-Daudé wrote:
> sd_set_cb() was only used by omap2_mmc_init() which
> got recently removed. Time to remove it. For historical
> background on the me_no_qdev_me_kill_mammoth_with_rocks
> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
> thing created by sd_init()").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/sd/sdcard_legacy.h |  1 -
>   hw/sd/sd.c                    | 30 ++++--------------------------
>   2 files changed, 4 insertions(+), 27 deletions(-)
> 
> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
> index 0dc3889555..a121232560 100644
> --- a/include/hw/sd/sdcard_legacy.h
> +++ b/include/hw/sd/sdcard_legacy.h
> @@ -36,7 +36,6 @@ 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
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a140a32ccd..8a30c61ce0 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -112,10 +112,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];
> @@ -169,8 +165,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;
> @@ -889,17 +883,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));

Guenter, we don't have SX1 tests. I wonder if this call could
fire an assertion. Do you have test images I can use? I'd need
to eject/reinsert a card to get there.

> +    sdbus_set_inserted(sdbus, inserted);
> +    if (inserted) {
> +        sdbus_set_readonly(sdbus, readonly);
>       }
>   }
>   
> @@ -1027,18 +1014,9 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>       }
>   
>       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);

On 9/3/24 13:10, Philippe Mathieu-Daudé wrote:
> (Cc'ing Guenter who asked to keep the SX1 machine)
> 
> On 3/9/24 22:04, Philippe Mathieu-Daudé wrote:
>> sd_set_cb() was only used by omap2_mmc_init() which
>> got recently removed. Time to remove it. For historical
>> background on the me_no_qdev_me_kill_mammoth_with_rocks
>> kludge, see commit 007d1dbf72 ("sd: Hide the qdev-but-not-quite
>> thing created by sd_init()").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/sd/sdcard_legacy.h |  1 -
>>   hw/sd/sd.c                    | 30 ++++--------------------------
>>   2 files changed, 4 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
>> index 0dc3889555..a121232560 100644
>> --- a/include/hw/sd/sdcard_legacy.h
>> +++ b/include/hw/sd/sdcard_legacy.h
>> @@ -36,7 +36,6 @@ 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
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index a140a32ccd..8a30c61ce0 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -112,10 +112,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];
>> @@ -169,8 +165,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;
>> @@ -889,17 +883,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));
> 
> Guenter, we don't have SX1 tests. I wonder if this call could
> fire an assertion. Do you have test images I can use? I'd need
> to eject/reinsert a card to get there.
> 

Have a look into
https://github.com/groeck/linux-build-test/tree/master/rootfs/arm

rootfs-armv4*are root file systems. You'd boot those with "noreboot"
command line argument to prevent them from rebooting automatically.
Let me know if you need more detailed directions; if so, I can make
a directory with everything needed.

Guenter