Re: [PATCH 1/2] hw/sd: Remove legacy sd_set_cb()

[Peter Maydell]

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