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

Philippe Mathieu-Daudé posted 2 patches 2 months, 3 weeks ago
[PATCH 2/2] hw/sd: Remove legacy sd_enable()
Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago
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()
Posted by Peter Maydell 2 months, 2 weeks ago
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()
Posted by Philippe Mathieu-Daudé 2 weeks, 3 days ago
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.