[PATCH] hw/sd/omap_mmc: Don't use sd_cmd_type_t

[PATCH] hw/sd/omap_mmc: Don't use sd_cmd_type_t

[Peter Maydell]

In commit 1ab08790bb75e4 we did some refactoring of the SD card
implementation, which included a rearrangement of the sd_cmd_type_t
enum values.  Unfortunately we didn't notice that this enum is not
used solely inside the SD card model itself, but is also used by the
OMAP MMC controller device.  In the OMAP MMC controller, it is used
to implement the handling of the Type field of the MMC_CMD register,
so changing the enum values so that they no longer lined up with the
bit definitions for that register field broke the controller model.
The effect is that Linux fails to boot from an SD card on the "sx1"
machine.

Give omap-mmc its own enum which we can document as needing to match
the encoding used in this device's register, so it isn't sharing
sd_cmd_type_t with the SD card model any more.  We can then move
sd_cmd_type_t's definition out of sd.h and into sd.c, which is the
only place that uses it.

Cc: qemu-stable@nongnu.org
Fixes: 1ab08790bb75 ("hw/sd/sdcard: Store command type in SDProto")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sd/sd.h |  8 --------
 hw/sd/omap_mmc.c   | 22 ++++++++++++++++------
 hw/sd/sd.c         |  8 ++++++++
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index d35a839f5ef..f2458f37b3c 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -75,14 +75,6 @@ typedef enum  {
     UHS_III             = 3,    /* currently not supported */
 } sd_uhs_mode_t;
 
-typedef enum {
-    sd_spi,
-    sd_bc,     /* broadcast -- no response */
-    sd_bcr,    /* broadcast with response */
-    sd_ac,     /* addressed -- no data transfer */
-    sd_adtc,   /* addressed with data transfer */
-} sd_cmd_type_t;
-
 typedef struct {
     uint8_t cmd;
     uint32_t arg;
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 91e9a3f1c6a..1d4e30e6b7b 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -103,6 +103,7 @@ static void omap_mmc_fifolevel_update(struct omap_mmc_s *host)
     }
 }
 
+/* These must match the encoding of the MMC_CMD Response field */
 typedef enum {
     sd_nore = 0,	/* no response */
     sd_r1,		/* normal response command */
@@ -112,8 +113,17 @@ typedef enum {
     sd_r1b = -1,
 } sd_rsp_type_t;
 
+/* These must match the encoding of the MMC_CMD Type field */
+typedef enum {
+    SD_TYPE_BC = 0,     /* broadcast -- no response */
+    SD_TYPE_BCR = 1,    /* broadcast with response */
+    SD_TYPE_AC = 2,     /* addressed -- no data transfer */
+    SD_TYPE_ADTC = 3,   /* addressed with data transfer */
+} MMCCmdType;
+
 static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
-                sd_cmd_type_t type, int busy, sd_rsp_type_t resptype, int init)
+                             MMCCmdType type, int busy,
+                             sd_rsp_type_t resptype, int init)
 {
     uint32_t rspstatus, mask;
     int rsplen, timeout;
@@ -128,7 +138,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
     if (resptype == sd_r1 && busy)
         resptype = sd_r1b;
 
-    if (type == sd_adtc) {
+    if (type == SD_TYPE_ADTC) {
         host->fifo_start = 0;
         host->fifo_len = 0;
         host->transfer = 1;
@@ -433,10 +443,10 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
         for (i = 0; i < 8; i ++)
             s->rsp[i] = 0x0000;
         omap_mmc_command(s, value & 63, (value >> 15) & 1,
-                (sd_cmd_type_t) ((value >> 12) & 3),
-                (value >> 11) & 1,
-                (sd_rsp_type_t) ((value >> 8) & 7),
-                (value >> 7) & 1);
+                         (MMCCmdType)((value >> 12) & 3),
+                         (value >> 11) & 1,
+                         (sd_rsp_type_t) ((value >> 8) & 7),
+                         (value >> 7) & 1);
         omap_mmc_update(s);
         break;
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a5d2d929a8a..b2e2d58e013 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -71,6 +71,14 @@ typedef enum {
     sd_illegal = -2,
 } sd_rsp_type_t;
 
+typedef enum {
+    sd_spi,
+    sd_bc,     /* broadcast -- no response */
+    sd_bcr,    /* broadcast with response */
+    sd_ac,     /* addressed -- no data transfer */
+    sd_adtc,   /* addressed with data transfer */
+} sd_cmd_type_t;
+
 enum SDCardModes {
     sd_inactive,
     sd_card_identification_mode,
-- 
2.34.1

Re: [PATCH] hw/sd/omap_mmc: Don't use sd_cmd_type_t

[Guenter Roeck]

On 10/17/24 09:27, Peter Maydell wrote:
> In commit 1ab08790bb75e4 we did some refactoring of the SD card
> implementation, which included a rearrangement of the sd_cmd_type_t
> enum values.  Unfortunately we didn't notice that this enum is not
> used solely inside the SD card model itself, but is also used by the
> OMAP MMC controller device.  In the OMAP MMC controller, it is used
> to implement the handling of the Type field of the MMC_CMD register,
> so changing the enum values so that they no longer lined up with the
> bit definitions for that register field broke the controller model.
> The effect is that Linux fails to boot from an SD card on the "sx1"
> machine.
> 
> Give omap-mmc its own enum which we can document as needing to match
> the encoding used in this device's register, so it isn't sharing
> sd_cmd_type_t with the SD card model any more.  We can then move
> sd_cmd_type_t's definition out of sd.h and into sd.c, which is the
> only place that uses it.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 1ab08790bb75 ("hw/sd/sdcard: Store command type in SDProto")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH] hw/sd/omap_mmc: Don't use sd_cmd_type_t

[Philippe Mathieu-Daudé]

On 17/10/24 13:27, Peter Maydell wrote:
> In commit 1ab08790bb75e4 we did some refactoring of the SD card
> implementation, which included a rearrangement of the sd_cmd_type_t
> enum values.  Unfortunately we didn't notice that this enum is not
> used solely inside the SD card model itself, but is also used by the
> OMAP MMC controller device.  In the OMAP MMC controller, it is used
> to implement the handling of the Type field of the MMC_CMD register,
> so changing the enum values so that they no longer lined up with the
> bit definitions for that register field broke the controller model.
> The effect is that Linux fails to boot from an SD card on the "sx1"
> machine.
> 
> Give omap-mmc its own enum which we can document as needing to match
> the encoding used in this device's register, so it isn't sharing
> sd_cmd_type_t with the SD card model any more.  We can then move
> sd_cmd_type_t's definition out of sd.h and into sd.c, which is the
> only place that uses it.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 1ab08790bb75 ("hw/sd/sdcard: Store command type in SDProto")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sd/sd.h |  8 --------
>   hw/sd/omap_mmc.c   | 22 ++++++++++++++++------
>   hw/sd/sd.c         |  8 ++++++++
>   3 files changed, 24 insertions(+), 14 deletions(-)

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

Thanks!