[PATCH 0/2] hw/sd: Remove a pair of legacy methods

[PATCH 0/2] hw/sd: Remove a pair of legacy methods

[Philippe Mathieu-Daudé]

The omap2_mmc device -- deprecated and about to be
removed -- was the last user of the legacy sd_set_cb()
and sd_enable() methods. Remove them too along with
the me_no_qdev_me_kill_mammoth_with_rocks kludge.

Based-on: <20240903160751.4100218-1-peter.maydell@linaro.org>

Philippe Mathieu-Daudé (2):
  hw/sd: Remove legacy sd_set_cb()
  hw/sd: Remove legacy sd_enable()

 include/hw/sd/sd.h            |  1 -
 include/hw/sd/sdcard_legacy.h | 10 -------
 hw/sd/sd.c                    | 50 +++++++----------------------------
 3 files changed, 10 insertions(+), 51 deletions(-)

-- 
2.45.2

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

(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);

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

[Guenter Roeck]

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

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

[Peter Maydell]

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

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

[Markus Armbruster]

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?

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

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

[Markus Armbruster]

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!

[PATCH 2/2] hw/sd: Remove legacy sd_enable()

[Philippe Mathieu-Daudé]

sd_enable() was only used by omap_mmc_enable() which
got recently removed. Time to remove it.

Since the SDState::enable boolean is now always %true,
we can remove it and simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/sd/sd.h            |  1 -
 include/hw/sd/sdcard_legacy.h |  9 ---------
 hw/sd/sd.c                    | 20 ++++++--------------
 3 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index d35a839f5e..aa0f229747 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -127,7 +127,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/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
index a121232560..82b62e87d1 100644
--- a/include/hw/sd/sdcard_legacy.h
+++ b/include/hw/sd/sdcard_legacy.h
@@ -37,13 +37,4 @@ 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);
 
-/* 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/sd.c b/hw/sd/sd.c
index 8a30c61ce0..0c681da432 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -166,7 +166,6 @@ struct SDState {
     size_t data_size;
     uint8_t data[512];
     QEMUTimer *ocr_power_timer;
-    bool enable;
     uint8_t dat_lines;
     bool cmd_line;
 };
@@ -285,12 +284,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)
@@ -974,7 +973,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(sizeof(bool)),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (const VMStateDescription * const []) {
@@ -2177,7 +2176,7 @@ 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;
     }
 
@@ -2328,7 +2327,7 @@ 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) {
@@ -2460,7 +2459,7 @@ 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) {
@@ -2536,11 +2535,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 = {
@@ -2700,7 +2694,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);
 }
 
@@ -2809,7 +2802,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.45.2

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

[Peter Maydell]

On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> sd_enable() was only used by omap_mmc_enable() which
> got recently removed. Time to remove it.
>
> Since the SDState::enable boolean is now always %true,
> we can remove it and simplify.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> @@ -2328,7 +2327,7 @@ 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) {
> @@ -2460,7 +2459,7 @@ 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) {

Maybe add the { } that coding style wants to these if()s
since we're editing them anyway?

Either way,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 9/9/24 16:26, Peter Maydell wrote:
> On Tue, 3 Sept 2024 at 21:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> sd_enable() was only used by omap_mmc_enable() which
>> got recently removed. Time to remove it.
>>
>> Since the SDState::enable boolean is now always %true,
>> we can remove it and simplify.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> @@ -2328,7 +2327,7 @@ 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) {
>> @@ -2460,7 +2459,7 @@ 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) {
> 
> Maybe add the { } that coding style wants to these if()s
> since we're editing them anyway?

Sure, I didn't notice.

> Either way,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, patch queued.