[PATCH 00/23] hw/sd/sdcard: Accumulation of cleanups and fixes

[PATCH 00/23] hw/sd/sdcard: Accumulation of cleanups and fixes

[Philippe Mathieu-Daudé]

Various SD card cleanups and fixes accumulated over
the years. Various have been useful to help integrating
eMMC support (which will come later).

Based-on: <20240621075607.17902-1-philmd@linaro.org> st24_be_p()

Philippe Mathieu-Daudé (23):
  hw/sd/sdcard: Correct code indentation
  hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)
  hw/sd/sdcard: Fix typo in SEND_OP_COND command name
  hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values
  hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition
  hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers
  hw/sd/sdcard: Remove ACMD6 handler for SPI mode
  hw/sd/sdcard: Remove explicit entries for illegal commands
  hw/sd/sdcard: Generate random RCA value
  hw/sd/sdcard: Track last command used to help logging
  hw/sd/sdcard: Trace update of block count (CMD23)
  hw/sd/sdcard: Trace block offset in READ/WRITE data accesses
  hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value
  hw/sd/sdcard: Factor sd_req_get_rca() method out
  hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used
  hw/sd/sdcard: Factor sd_req_get_address() method out
  hw/sd/sdcard: Only call sd_req_get_address() where address is used
  hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode
    switch
  hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros
  hw/sd/sdcard: Add comments around registers and commands
  hw/sd/sdcard: Do not store vendor data on block drive (CMD56)
  hw/sd/sdcard: Send WRITE_PROT bits MSB first (CMD30)
  hw/sd/sdcard: Send NUM_WR_BLOCKS bits MSB first (ACMD22)

 hw/sd/sd.c             | 278 +++++++++++++++++++++++------------------
 hw/sd/sdmmc-internal.c |   2 +-
 hw/sd/trace-events     |   7 +-
 3 files changed, 159 insertions(+), 128 deletions(-)

-- 
2.41.0

[PATCH 01/23] hw/sd/sdcard: Correct code indentation

[Philippe Mathieu-Daudé]

Fix mis-alignment from commits 793d04f495 and 6380cd2052
("Add sd_cmd_SEND_TUNING_BLOCK" and "Add sd_cmd_SET_BLOCK_COUNT").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 16d8d52a78..626e99ecd6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1069,33 +1069,33 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
 
 static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
 {
-        if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
-            return sd_cmd_illegal(sd, req);
-        }
+    if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
+        return sd_cmd_illegal(sd, req);
+    }
 
-        if (sd->state != sd_transfer_state) {
-            return sd_invalid_state_for_cmd(sd, req);
-        }
+    if (sd->state != sd_transfer_state) {
+        return sd_invalid_state_for_cmd(sd, req);
+    }
 
-        sd->state = sd_sendingdata_state;
-        sd->data_offset = 0;
+    sd->state = sd_sendingdata_state;
+    sd->data_offset = 0;
 
-        return sd_r1;
+    return sd_r1;
 }
 
 static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 {
-        if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
-            return sd_cmd_illegal(sd, req);
-        }
+    if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
+        return sd_cmd_illegal(sd, req);
+    }
 
-        if (sd->state != sd_transfer_state) {
-            return sd_invalid_state_for_cmd(sd, req);
-        }
+    if (sd->state != sd_transfer_state) {
+        return sd_invalid_state_for_cmd(sd, req);
+    }
 
-        sd->multi_blk_cnt = req.arg;
+    sd->multi_blk_cnt = req.arg;
 
-        return sd_r1;
+    return sd_r1;
 }
 
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
-- 
2.41.0

Re: [PATCH 01/23] hw/sd/sdcard: Correct code indentation

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Fix mis-alignment from commits 793d04f495 and 6380cd2052
> ("Add sd_cmd_SEND_TUNING_BLOCK" and "Add sd_cmd_SET_BLOCK_COUNT").
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sd/sd.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 16d8d52a78..626e99ecd6 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1069,33 +1069,33 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
>   
>   static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
>   {
> -        if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> -            return sd_cmd_illegal(sd, req);
> -        }
> +    if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> +        return sd_cmd_illegal(sd, req);
> +    }
>   
> -        if (sd->state != sd_transfer_state) {
> -            return sd_invalid_state_for_cmd(sd, req);
> -        }
> +    if (sd->state != sd_transfer_state) {
> +        return sd_invalid_state_for_cmd(sd, req);
> +    }
>   
> -        sd->state = sd_sendingdata_state;
> -        sd->data_offset = 0;
> +    sd->state = sd_sendingdata_state;
> +    sd->data_offset = 0;
>   
> -        return sd_r1;
> +    return sd_r1;
>   }
>   
>   static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   {
> -        if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> -            return sd_cmd_illegal(sd, req);
> -        }
> +    if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> +        return sd_cmd_illegal(sd, req);
> +    }
>   
> -        if (sd->state != sd_transfer_state) {
> -            return sd_invalid_state_for_cmd(sd, req);
> -        }
> +    if (sd->state != sd_transfer_state) {
> +        return sd_invalid_state_for_cmd(sd, req);
> +    }
>   
> -        sd->multi_blk_cnt = req.arg;
> +    sd->multi_blk_cnt = req.arg;
>   
> -        return sd_r1;
> +    return sd_r1;
>   }
>   
>   static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)

[PATCH 02/23] hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)

[Philippe Mathieu-Daudé]

Keep this handler style in sync with other handlers by
using a switch() case, which might become handy to
handle other states.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 626e99ecd6..addeb1940f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1044,13 +1044,13 @@ static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
 
 static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
 {
-    if (sd->state != sd_ready_state) {
+    switch (sd->state) {
+    case sd_ready_state:
+        sd->state = sd_identification_state;
+        return sd_r2_i;
+    default:
         return sd_invalid_state_for_cmd(sd, req);
     }
-
-    sd->state = sd_identification_state;
-
-    return sd_r2_i;
 }
 
 static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
-- 
2.41.0

Re: [PATCH 02/23] hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Keep this handler style in sync with other handlers by
> using a switch() case, which might become handy to
> handle other states.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 626e99ecd6..addeb1940f 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1044,13 +1044,13 @@ static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
>   
>   static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
>   {
> -    if (sd->state != sd_ready_state) {
> +    switch (sd->state) {
> +    case sd_ready_state:
> +        sd->state = sd_identification_state;
> +        return sd_r2_i;
> +    default:
>           return sd_invalid_state_for_cmd(sd, req);
>       }
> -
> -    sd->state = sd_identification_state;
> -
> -    return sd_r2_i;
>   }
>   
>   static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)

[PATCH 03/23] hw/sd/sdcard: Fix typo in SEND_OP_COND command name

[Philippe Mathieu-Daudé]

There is no SEND_OP_CMD but SEND_OP_COND.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c             | 6 +++---
 hw/sd/sdmmc-internal.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index addeb1940f..331cef5779 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1035,7 +1035,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
     return sd_is_spi(sd) ? sd_r1 : sd_r0;
 }
 
-static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
+static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
 {
     sd->state = sd_transfer_state;
 
@@ -2149,7 +2149,7 @@ static const SDProto sd_proto_spi = {
     .name = "SPI",
     .cmd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
-        [1]         = sd_cmd_SEND_OP_CMD,
+        [1]         = spi_cmd_SEND_OP_COND,
         [2 ... 4]   = sd_cmd_illegal,
         [5]         = sd_cmd_illegal,
         [7]         = sd_cmd_illegal,
@@ -2159,7 +2159,7 @@ static const SDProto sd_proto_spi = {
     },
     .acmd = {
         [6]         = sd_cmd_unimplemented,
-        [41]        = sd_cmd_SEND_OP_CMD,
+        [41]        = spi_cmd_SEND_OP_COND,
     },
 };
 
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 8648a7808d..c1d5508ae6 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -14,7 +14,7 @@
 const char *sd_cmd_name(uint8_t cmd)
 {
     static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
-         [0]    = "GO_IDLE_STATE",           [1]    = "SEND_OP_CMD",
+         [0]    = "GO_IDLE_STATE",           [1]    = "SEND_OP_COND",
          [2]    = "ALL_SEND_CID",            [3]    = "SEND_RELATIVE_ADDR",
          [4]    = "SET_DSR",                 [5]    = "IO_SEND_OP_COND",
          [6]    = "SWITCH_FUNC",             [7]    = "SELECT/DESELECT_CARD",
-- 
2.41.0

Re: [PATCH 03/23] hw/sd/sdcard: Fix typo in SEND_OP_COND command name

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> There is no SEND_OP_CMD but SEND_OP_COND.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c             | 6 +++---
>   hw/sd/sdmmc-internal.c | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index addeb1940f..331cef5779 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1035,7 +1035,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
>       return sd_is_spi(sd) ? sd_r1 : sd_r0;
>   }
>   
> -static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
> +static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
>   {
>       sd->state = sd_transfer_state;
>   
> @@ -2149,7 +2149,7 @@ static const SDProto sd_proto_spi = {
>       .name = "SPI",
>       .cmd = {
>           [0]         = sd_cmd_GO_IDLE_STATE,
> -        [1]         = sd_cmd_SEND_OP_CMD,
> +        [1]         = spi_cmd_SEND_OP_COND,
>           [2 ... 4]   = sd_cmd_illegal,
>           [5]         = sd_cmd_illegal,
>           [7]         = sd_cmd_illegal,
> @@ -2159,7 +2159,7 @@ static const SDProto sd_proto_spi = {
>       },
>       .acmd = {
>           [6]         = sd_cmd_unimplemented,
> -        [41]        = sd_cmd_SEND_OP_CMD,
> +        [41]        = spi_cmd_SEND_OP_COND,
>       },
>   };
>   
> diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
> index 8648a7808d..c1d5508ae6 100644
> --- a/hw/sd/sdmmc-internal.c
> +++ b/hw/sd/sdmmc-internal.c
> @@ -14,7 +14,7 @@
>   const char *sd_cmd_name(uint8_t cmd)
>   {
>       static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
> -         [0]    = "GO_IDLE_STATE",           [1]    = "SEND_OP_CMD",
> +         [0]    = "GO_IDLE_STATE",           [1]    = "SEND_OP_COND",
>            [2]    = "ALL_SEND_CID",            [3]    = "SEND_RELATIVE_ADDR",
>            [4]    = "SET_DSR",                 [5]    = "IO_SEND_OP_COND",
>            [6]    = "SWITCH_FUNC",             [7]    = "SELECT/DESELECT_CARD",

[PATCH 04/23] hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 331cef5779..c528c30bcf 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -596,7 +596,7 @@ static void sd_reset(DeviceState *dev)
     } else {
         sect = 0;
     }
-    size = sect << 9;
+    size = sect << HWBLOCK_SHIFT;
 
     sect = sd_addr_to_wpnum(size) + 1;
 
@@ -822,8 +822,8 @@ static void sd_erase(SDState *sd)
 
     if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
         /* High capacity memory card: erase units are 512 byte blocks */
-        erase_start *= 512;
-        erase_end *= 512;
+        erase_start <<= HWBLOCK_SHIFT;
+        erase_end <<= HWBLOCK_SHIFT;
         sdsc = false;
     }
 
-- 
2.41.0

Re: [PATCH 04/23] hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 331cef5779..c528c30bcf 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -596,7 +596,7 @@ static void sd_reset(DeviceState *dev)
>       } else {
>           sect = 0;
>       }
> -    size = sect << 9;
> +    size = sect << HWBLOCK_SHIFT;
>   
>       sect = sd_addr_to_wpnum(size) + 1;
>   
> @@ -822,8 +822,8 @@ static void sd_erase(SDState *sd)
>   
>       if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
>           /* High capacity memory card: erase units are 512 byte blocks */
> -        erase_start *= 512;
> -        erase_end *= 512;
> +        erase_start <<= HWBLOCK_SHIFT;
> +        erase_end <<= HWBLOCK_SHIFT;
>           sdsc = false;
>       }
>   

[PATCH 05/23] hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition

[Philippe Mathieu-Daudé]

Use registerfield-generated definitions to update card_status.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c528c30bcf..24415cb9f0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1788,8 +1788,8 @@ int sd_do_command(SDState *sd, SDRequest *req,
          * (Do this now so they appear in r1 responses.)
          */
         sd->current_cmd = req->cmd;
-        sd->card_status &= ~CURRENT_STATE;
-        sd->card_status |= (last_state << 9);
+        sd->card_status = FIELD_DP32(sd->card_status, CSR,
+                                     CURRENT_STATE, last_state);
     }
 
 send_response:
-- 
2.41.0

Re: [PATCH 05/23] hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Use registerfield-generated definitions to update card_status.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index c528c30bcf..24415cb9f0 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1788,8 +1788,8 @@ int sd_do_command(SDState *sd, SDRequest *req,
>            * (Do this now so they appear in r1 responses.)
>            */
>           sd->current_cmd = req->cmd;
> -        sd->card_status &= ~CURRENT_STATE;
> -        sd->card_status |= (last_state << 9);
> +        sd->card_status = FIELD_DP32(sd->card_status, CSR,
> +                                     CURRENT_STATE, last_state);
>       }
>   
>   send_response:

[PATCH 06/23] hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers

[Philippe Mathieu-Daudé]

The ld/st API helps noticing CID or CSD bytes refer
to the same field. Multi-bytes fields are stored MSB
first in CID / CSD.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 24415cb9f0..b0cd30c657 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -393,10 +393,7 @@ static void sd_set_cid(SDState *sd)
     sd->cid[6] = PNM[3];
     sd->cid[7] = PNM[4];
     sd->cid[8] = PRV;       /* Fake product revision (PRV) */
-    sd->cid[9] = 0xde;      /* Fake serial number (PSN) */
-    sd->cid[10] = 0xad;
-    sd->cid[11] = 0xbe;
-    sd->cid[12] = 0xef;
+    stl_be_p(&sd->cid[9], 0xdeadbeef); /* Fake serial number (PSN) */
     sd->cid[13] = 0x00 |    /* Manufacture date (MDT) */
         ((MDT_YR - 2000) / 10);
     sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
@@ -462,9 +459,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[4] = 0x5b;
         sd->csd[5] = 0x59;
         sd->csd[6] = 0x00;
-        sd->csd[7] = (size >> 16) & 0xff;
-        sd->csd[8] = (size >> 8) & 0xff;
-        sd->csd[9] = (size & 0xff);
+        st24_be_p(&sd->csd[7], size);
         sd->csd[10] = 0x7f;
         sd->csd[11] = 0x80;
         sd->csd[12] = 0x0a;
-- 
2.41.0

Re: [PATCH 06/23] hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> The ld/st API helps noticing CID or CSD bytes refer
> to the same field. Multi-bytes fields are stored MSB
> first in CID / CSD.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 24415cb9f0..b0cd30c657 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -393,10 +393,7 @@ static void sd_set_cid(SDState *sd)
>       sd->cid[6] = PNM[3];
>       sd->cid[7] = PNM[4];
>       sd->cid[8] = PRV;       /* Fake product revision (PRV) */
> -    sd->cid[9] = 0xde;      /* Fake serial number (PSN) */
> -    sd->cid[10] = 0xad;
> -    sd->cid[11] = 0xbe;
> -    sd->cid[12] = 0xef;
> +    stl_be_p(&sd->cid[9], 0xdeadbeef); /* Fake serial number (PSN) */
>       sd->cid[13] = 0x00 |    /* Manufacture date (MDT) */
>           ((MDT_YR - 2000) / 10);
>       sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
> @@ -462,9 +459,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>           sd->csd[4] = 0x5b;
>           sd->csd[5] = 0x59;
>           sd->csd[6] = 0x00;
> -        sd->csd[7] = (size >> 16) & 0xff;
> -        sd->csd[8] = (size >> 8) & 0xff;
> -        sd->csd[9] = (size & 0xff);
> +        st24_be_p(&sd->csd[7], size);
>           sd->csd[10] = 0x7f;
>           sd->csd[11] = 0x80;
>           sd->csd[12] = 0x0a;

[PATCH 07/23] hw/sd/sdcard: Remove ACMD6 handler for SPI mode

[Philippe Mathieu-Daudé]

There is no ACMD6 command in SPI mode, remove the pointless
handler introduced in commit 946897ce18 ("sdcard: handles
more commands in SPI mode"). Keep sd_cmd_unimplemented()
since we'll reuse it later.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b0cd30c657..e9af834a8c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1012,6 +1012,7 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
 }
 
 /* Commands that are recognised but not yet implemented. */
+__attribute__((unused))
 static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
 {
     qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
@@ -2153,7 +2154,6 @@ static const SDProto sd_proto_spi = {
         [52 ... 54] = sd_cmd_illegal,
     },
     .acmd = {
-        [6]         = sd_cmd_unimplemented,
         [41]        = spi_cmd_SEND_OP_COND,
     },
 };
-- 
2.41.0

Re: [PATCH 07/23] hw/sd/sdcard: Remove ACMD6 handler for SPI mode

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> There is no ACMD6 command in SPI mode, remove the pointless
> handler introduced in commit 946897ce18 ("sdcard: handles
> more commands in SPI mode"). Keep sd_cmd_unimplemented()
> since we'll reuse it later.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b0cd30c657..e9af834a8c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1012,6 +1012,7 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
>   }
>   
>   /* Commands that are recognised but not yet implemented. */
> +__attribute__((unused))
>   static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
>   {
>       qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
> @@ -2153,7 +2154,6 @@ static const SDProto sd_proto_spi = {
>           [52 ... 54] = sd_cmd_illegal,
>       },
>       .acmd = {
> -        [6]         = sd_cmd_unimplemented,
>           [41]        = spi_cmd_SEND_OP_COND,
>       },
>   };

[PATCH 08/23] hw/sd/sdcard: Remove explicit entries for illegal commands

[Philippe Mathieu-Daudé]

NULL handler is already handled as illegal, no need to
duplicate (that keeps this array simpler to maintain).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e9af834a8c..30239b28bc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2146,12 +2146,6 @@ static const SDProto sd_proto_spi = {
     .cmd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
         [1]         = spi_cmd_SEND_OP_COND,
-        [2 ... 4]   = sd_cmd_illegal,
-        [5]         = sd_cmd_illegal,
-        [7]         = sd_cmd_illegal,
-        [15]        = sd_cmd_illegal,
-        [26]        = sd_cmd_illegal,
-        [52 ... 54] = sd_cmd_illegal,
     },
     .acmd = {
         [41]        = spi_cmd_SEND_OP_COND,
@@ -2162,15 +2156,10 @@ static const SDProto sd_proto_sd = {
     .name = "SD",
     .cmd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
-        [1]         = sd_cmd_illegal,
         [2]         = sd_cmd_ALL_SEND_CID,
         [3]         = sd_cmd_SEND_RELATIVE_ADDR,
-        [5]         = sd_cmd_illegal,
         [19]        = sd_cmd_SEND_TUNING_BLOCK,
         [23]        = sd_cmd_SET_BLOCK_COUNT,
-        [52 ... 54] = sd_cmd_illegal,
-        [58]        = sd_cmd_illegal,
-        [59]        = sd_cmd_illegal,
     },
 };
 
-- 
2.41.0

Re: [PATCH 08/23] hw/sd/sdcard: Remove explicit entries for illegal commands

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> NULL handler is already handled as illegal, no need to
> duplicate (that keeps this array simpler to maintain).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index e9af834a8c..30239b28bc 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2146,12 +2146,6 @@ static const SDProto sd_proto_spi = {
>       .cmd = {
>           [0]         = sd_cmd_GO_IDLE_STATE,
>           [1]         = spi_cmd_SEND_OP_COND,
> -        [2 ... 4]   = sd_cmd_illegal,
> -        [5]         = sd_cmd_illegal,
> -        [7]         = sd_cmd_illegal,
> -        [15]        = sd_cmd_illegal,
> -        [26]        = sd_cmd_illegal,
> -        [52 ... 54] = sd_cmd_illegal,
>       },
>       .acmd = {
>           [41]        = spi_cmd_SEND_OP_COND,
> @@ -2162,15 +2156,10 @@ static const SDProto sd_proto_sd = {
>       .name = "SD",
>       .cmd = {
>           [0]         = sd_cmd_GO_IDLE_STATE,
> -        [1]         = sd_cmd_illegal,
>           [2]         = sd_cmd_ALL_SEND_CID,
>           [3]         = sd_cmd_SEND_RELATIVE_ADDR,
> -        [5]         = sd_cmd_illegal,
>           [19]        = sd_cmd_SEND_TUNING_BLOCK,
>           [23]        = sd_cmd_SET_BLOCK_COUNT,
> -        [52 ... 54] = sd_cmd_illegal,
> -        [58]        = sd_cmd_illegal,
> -        [59]        = sd_cmd_illegal,
>       },
>   };
>   

[PATCH 09/23] hw/sd/sdcard: Generate random RCA value

[Philippe Mathieu-Daudé]

Rather than using the obscure 0x4567 magic value,
use a real random one.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 30239b28bc..e1f13e316a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -46,6 +46,7 @@
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "qemu/log.h"
+#include "qemu/guest-random.h"
 #include "qemu/module.h"
 #include "sdmmc-internal.h"
 #include "trace.h"
@@ -469,11 +470,6 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
 }
 
-static void sd_set_rca(SDState *sd)
-{
-    sd->rca += 0x4567;
-}
-
 FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
 FIELD(CSR, APP_CMD,                     5,  1)
 FIELD(CSR, FX_EVENT,                    6,  1)
@@ -1055,7 +1051,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
     case sd_identification_state:
     case sd_standby_state:
         sd->state = sd_standby_state;
-        sd_set_rca(sd);
+        qemu_guest_getrandom_nofail(&sd->rca, sizeof(sd->rca));
         return sd_r6;
 
     default:
-- 
2.41.0

Re: [PATCH 09/23] hw/sd/sdcard: Generate random RCA value

[Philippe Mathieu-Daudé]

On 21/6/24 10:05, Philippe Mathieu-Daudé wrote:
> Rather than using the obscure 0x4567 magic value,
> use a real random one.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sd/sd.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 30239b28bc..e1f13e316a 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -46,6 +46,7 @@
>   #include "qemu/error-report.h"
>   #include "qemu/timer.h"
>   #include "qemu/log.h"
> +#include "qemu/guest-random.h"
>   #include "qemu/module.h"
>   #include "sdmmc-internal.h"
>   #include "trace.h"
> @@ -469,11 +470,6 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>       sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>   }
>   
> -static void sd_set_rca(SDState *sd)
> -{
> -    sd->rca += 0x4567;
> -}
> -
>   FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
>   FIELD(CSR, APP_CMD,                     5,  1)
>   FIELD(CSR, FX_EVENT,                    6,  1)
> @@ -1055,7 +1051,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
>       case sd_identification_state:
>       case sd_standby_state:
>           sd->state = sd_standby_state;
> -        sd_set_rca(sd);
> +        qemu_guest_getrandom_nofail(&sd->rca, sizeof(sd->rca));
>           return sd_r6;

Hmm the NPCM7xx test is failing:

▶  58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: 
assertion failed: (!memcmp(rmsg, msg, len)) ERROR

But booting various Linux / FreeBSD guests on SD cards works,
so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some
rework.

$ git grep 0x4567
tests/qtest/npcm7xx_sdhci-test.c:47:    sdhci_cmd_regs(qts, 
NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
tests/qtest/npcm7xx_sdhci-test.c-48- 
SDHC_SELECT_DESELECT_CARD);

In tests/qtest/libqos/sdhci-cmd.h:

/* CMD Reg */
#define SDHC_SEND_RELATIVE_ADDR (3 << 8)
#define SDHC_SELECT_DESELECT_CARD (7 << 8)

IIUC setup_sd_card():

card address is queried ...:

     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0,
                    SDHC_SEND_RELATIVE_ADDR);

... but then ignored and magic 0x4567 is used instead:

     sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
                    SDHC_SELECT_DESELECT_CARD);

OK, so this test need rework. I see the sdhci_read_cmd()
method but it reads the SDHC_BDATA FIFO register, not sure
this is what should be used to read the RCA from R6 command
response.

Shengtan, Nuvoton folks, what do you think?

Thanks,

Phil.

Re: [PATCH 09/23] hw/sd/sdcard: Generate random RCA value

[Philippe Mathieu-Daudé]

On 21/6/24 11:44, Philippe Mathieu-Daudé wrote:
> On 21/6/24 10:05, Philippe Mathieu-Daudé wrote:
>> Rather than using the obscure 0x4567 magic value,
>> use a real random one.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/sd/sd.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)


> Hmm the NPCM7xx test is failing:
> 
> ▶  58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: 
> assertion failed: (!memcmp(rmsg, msg, len)) ERROR
> 
> But booting various Linux / FreeBSD guests on SD cards works,
> so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some
> rework.
> 
> $ git grep 0x4567
> tests/qtest/npcm7xx_sdhci-test.c:47:    sdhci_cmd_regs(qts, 
> NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
> tests/qtest/npcm7xx_sdhci-test.c-48- SDHC_SELECT_DESELECT_CARD);
> 
> In tests/qtest/libqos/sdhci-cmd.h:
> 
> /* CMD Reg */
> #define SDHC_SEND_RELATIVE_ADDR (3 << 8)
> #define SDHC_SELECT_DESELECT_CARD (7 << 8)
> 
> IIUC setup_sd_card():
> 
> card address is queried ...:
> 
>      sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0,
>                     SDHC_SEND_RELATIVE_ADDR);
> 
> ... but then ignored and magic 0x4567 is used instead:
> 
>      sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
>                     SDHC_SELECT_DESELECT_CARD);
> 
> OK, so this test need rework. I see the sdhci_read_cmd()
> method but it reads the SDHC_BDATA FIFO register, not sure
> this is what should be used to read the RCA from R6 command
> response.
> 
> Shengtan, Nuvoton folks, what do you think?

More than 1 week passed so I'll have a look at it myself.

Regards,

Phil.

Re: [PATCH 09/23] hw/sd/sdcard: Generate random RCA value

[Philippe Mathieu-Daudé]

On 2/7/24 15:13, Philippe Mathieu-Daudé wrote:
> On 21/6/24 11:44, Philippe Mathieu-Daudé wrote:
>> On 21/6/24 10:05, Philippe Mathieu-Daudé wrote:
>>> Rather than using the obscure 0x4567 magic value,
>>> use a real random one.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/sd/sd.c | 8 ++------
>>>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> 
>> Hmm the NPCM7xx test is failing:
>>
>> ▶  58/450 ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: 
>> assertion failed: (!memcmp(rmsg, msg, len)) ERROR
>>
>> But booting various Linux / FreeBSD guests on SD cards works,
>> so I suppose the test (or TYPE_NPCM7XX_SDHCI model) need some
>> rework.
>>
>> $ git grep 0x4567
>> tests/qtest/npcm7xx_sdhci-test.c:47:    sdhci_cmd_regs(qts, 
>> NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
>> tests/qtest/npcm7xx_sdhci-test.c-48- SDHC_SELECT_DESELECT_CARD);
>>
>> In tests/qtest/libqos/sdhci-cmd.h:
>>
>> /* CMD Reg */
>> #define SDHC_SEND_RELATIVE_ADDR (3 << 8)
>> #define SDHC_SELECT_DESELECT_CARD (7 << 8)
>>
>> IIUC setup_sd_card():
>>
>> card address is queried ...:
>>
>>      sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0,
>>                     SDHC_SEND_RELATIVE_ADDR);
>>
>> ... but then ignored and magic 0x4567 is used instead:
>>
>>      sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x45670000, 0,
>>                     SDHC_SELECT_DESELECT_CARD);
>>
>> OK, so this test need rework. I see the sdhci_read_cmd()
>> method but it reads the SDHC_BDATA FIFO register, not sure
>> this is what should be used to read the RCA from R6 command
>> response.

Fix: 
https://lore.kernel.org/qemu-devel/20240702140842.54242-4-philmd@linaro.org/

[PATCH 10/23] hw/sd/sdcard: Track last command used to help logging

[Philippe Mathieu-Daudé]

The command is selected on the I/O lines, and further
processing might be done on the DAT lines via the
sd_read_byte() and sd_write_byte() handlers. Since
these methods can't distinct between normal and APP
commands, keep the name of the current command in
the SDState and use it in the DAT handlers. This
fixes a bug that all normal commands were displayed
as APP commands.

Fixes: 2ed61fb57b ("sdcard: Display command name when tracing CMD/ACMD")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e1f13e316a..4e378f7cf7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -134,6 +134,7 @@ struct SDState {
     uint32_t pwd_len;
     uint8_t function_group[6];
     uint8_t current_cmd;
+    const char *last_cmd_name;
     /* True if we will handle the next command as an ACMD. Note that this does
      * *not* track the APP_CMD status bit!
      */
@@ -1095,12 +1096,13 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     uint32_t rca = 0x0000;
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
+    sd->last_cmd_name = sd_cmd_name(req.cmd);
     /* CMD55 precedes an ACMD, so we are not interested in tracing it.
      * However there is no ACMD55, so we want to trace this particular case.
      */
     if (req.cmd != 55 || sd->expecting_acmd) {
         trace_sdcard_normal_command(sd_proto(sd)->name,
-                                    sd_cmd_name(req.cmd), req.cmd,
+                                    sd->last_cmd_name, req.cmd,
                                     req.arg, sd_state_name(sd->state));
     }
 
@@ -1571,7 +1573,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_app_command(SDState *sd,
                                     SDRequest req)
 {
-    trace_sdcard_app_command(sd_proto(sd)->name, sd_acmd_name(req.cmd),
+    sd->last_cmd_name = sd_acmd_name(req.cmd);
+    trace_sdcard_app_command(sd_proto(sd)->name, sd->last_cmd_name,
                              req.cmd, req.arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
 
@@ -1863,7 +1866,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
         return;
 
     trace_sdcard_write_data(sd_proto(sd)->name,
-                            sd_acmd_name(sd->current_cmd),
+                            sd->last_cmd_name,
                             sd->current_cmd, value);
     switch (sd->current_cmd) {
     case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
@@ -2019,7 +2022,7 @@ uint8_t sd_read_byte(SDState *sd)
     io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
 
     trace_sdcard_read_data(sd_proto(sd)->name,
-                           sd_acmd_name(sd->current_cmd),
+                           sd->last_cmd_name,
                            sd->current_cmd, io_len);
     switch (sd->current_cmd) {
     case 6:  /* CMD6:   SWITCH_FUNCTION */
@@ -2163,6 +2166,7 @@ static void sd_instance_init(Object *obj)
 {
     SDState *sd = SD_CARD(obj);
 
+    sd->last_cmd_name = "UNSET";
     sd->enable = true;
     sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
 }
-- 
2.41.0

[PATCH 11/23] hw/sd/sdcard: Trace update of block count (CMD23)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c         | 1 +
 hw/sd/trace-events | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4e378f7cf7..2586d15cbd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1087,6 +1087,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
     }
 
     sd->multi_blk_cnt = req.arg;
+    trace_sdcard_set_block_count(sd->multi_blk_cnt);
 
     return sd_r1;
 }
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 94a00557b2..724365efc3 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -43,7 +43,8 @@ sdcard_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
 sdcard_powerup(void) ""
 sdcard_inquiry_cmd41(void) ""
 sdcard_reset(void) ""
-sdcard_set_blocklen(uint16_t length) "0x%03x"
+sdcard_set_blocklen(uint16_t length) "block len 0x%03x"
+sdcard_set_block_count(uint32_t cnt) "block cnt 0x%"PRIx32
 sdcard_inserted(bool readonly) "read_only: %u"
 sdcard_ejected(void) ""
 sdcard_erase(uint32_t first, uint32_t last) "addr first 0x%" PRIx32" last 0x%" PRIx32
-- 
2.41.0

Re: [PATCH 11/23] hw/sd/sdcard: Trace update of block count (CMD23)

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c         | 1 +
>   hw/sd/trace-events | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 4e378f7cf7..2586d15cbd 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1087,6 +1087,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>       }
>   
>       sd->multi_blk_cnt = req.arg;
> +    trace_sdcard_set_block_count(sd->multi_blk_cnt);
>   
>       return sd_r1;
>   }
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 94a00557b2..724365efc3 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -43,7 +43,8 @@ sdcard_response(const char *rspdesc, int rsplen) "%s (sz:%d)"
>   sdcard_powerup(void) ""
>   sdcard_inquiry_cmd41(void) ""
>   sdcard_reset(void) ""
> -sdcard_set_blocklen(uint16_t length) "0x%03x"
> +sdcard_set_blocklen(uint16_t length) "block len 0x%03x"
> +sdcard_set_block_count(uint32_t cnt) "block cnt 0x%"PRIx32
>   sdcard_inserted(bool readonly) "read_only: %u"
>   sdcard_ejected(void) ""
>   sdcard_erase(uint32_t first, uint32_t last) "addr first 0x%" PRIx32" last 0x%" PRIx32

[PATCH 12/23] hw/sd/sdcard: Trace block offset in READ/WRITE data accesses

[Philippe Mathieu-Daudé]

Useful to detect out of bound accesses.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c         | 4 ++--
 hw/sd/trace-events | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2586d15cbd..c6cc1bab11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1868,7 +1868,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
 
     trace_sdcard_write_data(sd_proto(sd)->name,
                             sd->last_cmd_name,
-                            sd->current_cmd, value);
+                            sd->current_cmd, sd->data_offset, value);
     switch (sd->current_cmd) {
     case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
         sd->data[sd->data_offset ++] = value;
@@ -2024,7 +2024,7 @@ uint8_t sd_read_byte(SDState *sd)
 
     trace_sdcard_read_data(sd_proto(sd)->name,
                            sd->last_cmd_name,
-                           sd->current_cmd, io_len);
+                           sd->current_cmd, sd->data_offset, io_len);
     switch (sd->current_cmd) {
     case 6:  /* CMD6:   SWITCH_FUNCTION */
         ret = sd->data[sd->data_offset ++];
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 724365efc3..0eee98a646 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -52,8 +52,8 @@ sdcard_lock(void) ""
 sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
-sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
+sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint8_t value) "%s %20s/ CMD%02d ofs %"PRIu32" value 0x%02x"
+sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint32_t length) "%s %20s/ CMD%02d ofs %"PRIu32" len %" PRIu32
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # pxa2xx_mmci.c
-- 
2.41.0

Re: [PATCH 12/23] hw/sd/sdcard: Trace block offset in READ/WRITE data accesses

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Useful to detect out of bound accesses.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c         | 4 ++--
>   hw/sd/trace-events | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2586d15cbd..c6cc1bab11 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1868,7 +1868,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>   
>       trace_sdcard_write_data(sd_proto(sd)->name,
>                               sd->last_cmd_name,
> -                            sd->current_cmd, value);
> +                            sd->current_cmd, sd->data_offset, value);
>       switch (sd->current_cmd) {
>       case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
>           sd->data[sd->data_offset ++] = value;
> @@ -2024,7 +2024,7 @@ uint8_t sd_read_byte(SDState *sd)
>   
>       trace_sdcard_read_data(sd_proto(sd)->name,
>                              sd->last_cmd_name,
> -                           sd->current_cmd, io_len);
> +                           sd->current_cmd, sd->data_offset, io_len);
>       switch (sd->current_cmd) {
>       case 6:  /* CMD6:   SWITCH_FUNCTION */
>           ret = sd->data[sd->data_offset ++];
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 724365efc3..0eee98a646 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -52,8 +52,8 @@ sdcard_lock(void) ""
>   sdcard_unlock(void) ""
>   sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
>   sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
> -sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
> -sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
> +sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint8_t value) "%s %20s/ CMD%02d ofs %"PRIu32" value 0x%02x"
> +sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t offset, uint32_t length) "%s %20s/ CMD%02d ofs %"PRIu32" len %" PRIu32
>   sdcard_set_voltage(uint16_t millivolts) "%u mV"
>   
>   # pxa2xx_mmci.c

[PATCH 13/23] hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c6cc1bab11..510784fc82 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1716,7 +1716,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     return sd_illegal;
 }
 
-static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
+static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
 {
     /* Valid commands in locked state:
      * basic class (0)
@@ -1730,7 +1730,7 @@ static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
         return cmd == 41 || cmd == 42;
     }
     if (cmd == 16 || cmd == 55) {
-        return 1;
+        return true;
     }
     return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
 }
-- 
2.41.0

Re: [PATCH 13/23] hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index c6cc1bab11..510784fc82 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1716,7 +1716,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>       return sd_illegal;
>   }
>   
> -static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
> +static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
>   {
>       /* Valid commands in locked state:
>        * basic class (0)
> @@ -1730,7 +1730,7 @@ static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
>           return cmd == 41 || cmd == 42;
>       }
>       if (cmd == 16 || cmd == 55) {
> -        return 1;
> +        return true;
>       }
>       return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
>   }

[PATCH 14/23] hw/sd/sdcard: Factor sd_req_get_rca() method out

[Philippe Mathieu-Daudé]

Extract sd_req_get_rca() so we can re-use it in various
SDProto handlers. Return a 16-bit value since RCA is 16-bit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 510784fc82..bc47ae36bc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -471,6 +471,14 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
 }
 
+static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
+{
+    if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
+        return req.arg >> 16;
+    }
+    return 0;
+}
+
 FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
 FIELD(CSR, APP_CMD,                     5,  1)
 FIELD(CSR, FX_EVENT,                    6,  1)
@@ -1094,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
-    uint32_t rca = 0x0000;
+    uint16_t rca = sd_req_get_rca(sd, req);
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
     sd->last_cmd_name = sd_cmd_name(req.cmd);
@@ -1110,11 +1118,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     /* Not interpreting this as an app command */
     sd->card_status &= ~APP_CMD;
 
-    if (sd_cmd_type[req.cmd] == sd_ac
-        || sd_cmd_type[req.cmd] == sd_adtc) {
-        rca = req.arg >> 16;
-    }
-
     /* CMD23 (set block count) must be immediately followed by CMD18 or CMD25
      * if not, its effects are cancelled */
     if (sd->multi_blk_cnt != 0 && !(req.cmd == 18 || req.cmd == 25)) {
-- 
2.41.0

Re: [PATCH 14/23] hw/sd/sdcard: Factor sd_req_get_rca() method out

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Extract sd_req_get_rca() so we can re-use it in various
> SDProto handlers. Return a 16-bit value since RCA is 16-bit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 510784fc82..bc47ae36bc 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -471,6 +471,14 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>       sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>   }
>   
> +static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
> +{
> +    if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
> +        return req.arg >> 16;
> +    }
> +    return 0;
> +}
> +
>   FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
>   FIELD(CSR, APP_CMD,                     5,  1)
>   FIELD(CSR, FX_EVENT,                    6,  1)
> @@ -1094,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   
>   static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   {
> -    uint32_t rca = 0x0000;
> +    uint16_t rca = sd_req_get_rca(sd, req);
>       uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
>   
>       sd->last_cmd_name = sd_cmd_name(req.cmd);
> @@ -1110,11 +1118,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>       /* Not interpreting this as an app command */
>       sd->card_status &= ~APP_CMD;
>   
> -    if (sd_cmd_type[req.cmd] == sd_ac
> -        || sd_cmd_type[req.cmd] == sd_adtc) {
> -        rca = req.arg >> 16;
> -    }
> -
>       /* CMD23 (set block count) must be immediately followed by CMD18 or CMD25
>        * if not, its effects are cancelled */
>       if (sd->multi_blk_cnt != 0 && !(req.cmd == 18 || req.cmd == 25)) {

[PATCH 15/23] hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used

[Philippe Mathieu-Daudé]

It will be useful later to assert only AC commands
(Addressed point-to-point Commands, defined as the
'sd_ac' enum) extract the RCA value from the command
argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bc47ae36bc..cb9d85bb11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1102,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
-    uint16_t rca = sd_req_get_rca(sd, req);
+    uint16_t rca;
     uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
 
     sd->last_cmd_name = sd_cmd_name(req.cmd);
@@ -1160,6 +1160,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 7:  /* CMD7:   SELECT/DESELECT_CARD */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->state) {
         case sd_standby_state:
             if (sd->rca != rca)
@@ -1214,6 +1215,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         return sd_r7;
 
     case 9:  /* CMD9:   SEND_CSD */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->state) {
         case sd_standby_state:
             if (sd->rca != rca)
@@ -1237,6 +1239,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 10:  /* CMD10:  SEND_CID */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->state) {
         case sd_standby_state:
             if (sd->rca != rca)
@@ -1277,6 +1280,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 13:  /* CMD13:  SEND_STATUS */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->mode) {
         case sd_data_transfer_mode:
             if (!sd_is_spi(sd) && sd->rca != rca) {
@@ -1291,6 +1295,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 15:  /* CMD15:  GO_INACTIVE_STATE */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->mode) {
         case sd_data_transfer_mode:
             if (sd->rca != rca)
@@ -1523,6 +1528,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Application specific commands (Class 8) */
     case 55:  /* CMD55:  APP_CMD */
+        rca = sd_req_get_rca(sd, req);
         switch (sd->state) {
         case sd_ready_state:
         case sd_identification_state:
-- 
2.41.0

Re: [PATCH 15/23] hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> It will be useful later to assert only AC commands
> (Addressed point-to-point Commands, defined as the
> 'sd_ac' enum) extract the RCA value from the command
> argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index bc47ae36bc..cb9d85bb11 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1102,7 +1102,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   
>   static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   {
> -    uint16_t rca = sd_req_get_rca(sd, req);
> +    uint16_t rca;
>       uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
>   
>       sd->last_cmd_name = sd_cmd_name(req.cmd);
> @@ -1160,6 +1160,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           break;
>   
>       case 7:  /* CMD7:   SELECT/DESELECT_CARD */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->state) {
>           case sd_standby_state:
>               if (sd->rca != rca)
> @@ -1214,6 +1215,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           return sd_r7;
>   
>       case 9:  /* CMD9:   SEND_CSD */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->state) {
>           case sd_standby_state:
>               if (sd->rca != rca)
> @@ -1237,6 +1239,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           break;
>   
>       case 10:  /* CMD10:  SEND_CID */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->state) {
>           case sd_standby_state:
>               if (sd->rca != rca)
> @@ -1277,6 +1280,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           break;
>   
>       case 13:  /* CMD13:  SEND_STATUS */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->mode) {
>           case sd_data_transfer_mode:
>               if (!sd_is_spi(sd) && sd->rca != rca) {
> @@ -1291,6 +1295,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           break;
>   
>       case 15:  /* CMD15:  GO_INACTIVE_STATE */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->mode) {
>           case sd_data_transfer_mode:
>               if (sd->rca != rca)
> @@ -1523,6 +1528,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   
>       /* Application specific commands (Class 8) */
>       case 55:  /* CMD55:  APP_CMD */
> +        rca = sd_req_get_rca(sd, req);
>           switch (sd->state) {
>           case sd_ready_state:
>           case sd_identification_state:

[PATCH 16/23] hw/sd/sdcard: Factor sd_req_get_address() method out

[Philippe Mathieu-Daudé]

Extract sd_cmd_get_address() so we can re-use it
in various SDProto handlers. Use CARD_CAPACITY and
HWBLOCK_SHIFT definitions instead of magic values.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index cb9d85bb11..a0193a46ea 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -579,6 +579,14 @@ static void sd_response_r7_make(SDState *sd, uint8_t *response)
     stl_be_p(response, sd->vhs);
 }
 
+static uint64_t sd_req_get_address(SDState *sd, SDRequest req)
+{
+    if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
+        return (uint64_t) req.arg << HWBLOCK_SHIFT;
+    }
+    return req.arg;
+}
+
 static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
 {
     return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
@@ -1103,7 +1111,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint16_t rca;
-    uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
+    uint64_t addr = sd_req_get_address(sd, req);
 
     sd->last_cmd_name = sd_cmd_name(req.cmd);
     /* CMD55 precedes an ACMD, so we are not interested in tracing it.
-- 
2.41.0

Re: [PATCH 16/23] hw/sd/sdcard: Factor sd_req_get_address() method out

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Extract sd_cmd_get_address() so we can re-use it
> in various SDProto handlers. Use CARD_CAPACITY and
> HWBLOCK_SHIFT definitions instead of magic values.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index cb9d85bb11..a0193a46ea 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -579,6 +579,14 @@ static void sd_response_r7_make(SDState *sd, uint8_t *response)
>       stl_be_p(response, sd->vhs);
>   }
>   
> +static uint64_t sd_req_get_address(SDState *sd, SDRequest req)
> +{
> +    if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
> +        return (uint64_t) req.arg << HWBLOCK_SHIFT;
> +    }
> +    return req.arg;
> +}
> +
>   static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
>   {
>       return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
> @@ -1103,7 +1111,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   {
>       uint16_t rca;
> -    uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
> +    uint64_t addr = sd_req_get_address(sd, req);
>   
>       sd->last_cmd_name = sd_cmd_name(req.cmd);
>       /* CMD55 precedes an ACMD, so we are not interested in tracing it.

[PATCH 17/23] hw/sd/sdcard: Only call sd_req_get_address() where address is used

[Philippe Mathieu-Daudé]

It will be useful later to assert only ADTC commands
(Addressed point-to-point Data Transfer Commands, defined
as the 'sd_adtc' enum) extract the address value from the
command argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a0193a46ea..1df16ce6a2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1111,7 +1111,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint16_t rca;
-    uint64_t addr = sd_req_get_address(sd, req);
+    uint64_t addr;
 
     sd->last_cmd_name = sd_cmd_name(req.cmd);
     /* CMD55 precedes an ACMD, so we are not interested in tracing it.
@@ -1237,7 +1237,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             }
             sd->state = sd_sendingdata_state;
             memcpy(sd->data, sd->csd, 16);
-            sd->data_start = addr;
+            sd->data_start = sd_req_get_address(sd, req);
             sd->data_offset = 0;
             return sd_r1;
 
@@ -1261,7 +1261,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             }
             sd->state = sd_sendingdata_state;
             memcpy(sd->data, sd->cid, 16);
-            sd->data_start = addr;
+            sd->data_start = sd_req_get_address(sd, req);
             sd->data_offset = 0;
             return sd_r1;
 
@@ -1337,6 +1337,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     case 17:  /* CMD17:  READ_SINGLE_BLOCK */
     case 18:  /* CMD18:  READ_MULTIPLE_BLOCK */
+        addr = sd_req_get_address(sd, req);
         switch (sd->state) {
         case sd_transfer_state:
 
@@ -1357,6 +1358,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     /* Block write commands (Class 4) */
     case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
     case 25:  /* CMD25:  WRITE_MULTIPLE_BLOCK */
+        addr = sd_req_get_address(sd, req);
         switch (sd->state) {
         case sd_transfer_state:
 
@@ -1415,7 +1417,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         if (sd->size > SDSC_MAX_CAPACITY) {
             return sd_illegal;
         }
-
+        addr = sd_req_get_address(sd, req);
         switch (sd->state) {
         case sd_transfer_state:
             if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
@@ -1437,7 +1439,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         if (sd->size > SDSC_MAX_CAPACITY) {
             return sd_illegal;
         }
-
+        addr = sd_req_get_address(sd, req);
         switch (sd->state) {
         case sd_transfer_state:
             if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
@@ -1459,7 +1461,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         if (sd->size > SDSC_MAX_CAPACITY) {
             return sd_illegal;
         }
-
+        addr = sd_req_get_address(sd, req);
         switch (sd->state) {
         case sd_transfer_state:
             if (!address_in_range(sd, "SEND_WRITE_PROT",
-- 
2.41.0

Re: [PATCH 17/23] hw/sd/sdcard: Only call sd_req_get_address() where address is used

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> It will be useful later to assert only ADTC commands
> (Addressed point-to-point Data Transfer Commands, defined
> as the 'sd_adtc' enum) extract the address value from the
> command argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a0193a46ea..1df16ce6a2 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1111,7 +1111,7 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   {
>       uint16_t rca;
> -    uint64_t addr = sd_req_get_address(sd, req);
> +    uint64_t addr;
>   
>       sd->last_cmd_name = sd_cmd_name(req.cmd);
>       /* CMD55 precedes an ACMD, so we are not interested in tracing it.
> @@ -1237,7 +1237,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>               }
>               sd->state = sd_sendingdata_state;
>               memcpy(sd->data, sd->csd, 16);
> -            sd->data_start = addr;
> +            sd->data_start = sd_req_get_address(sd, req);
>               sd->data_offset = 0;
>               return sd_r1;
>   
> @@ -1261,7 +1261,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>               }
>               sd->state = sd_sendingdata_state;
>               memcpy(sd->data, sd->cid, 16);
> -            sd->data_start = addr;
> +            sd->data_start = sd_req_get_address(sd, req);
>               sd->data_offset = 0;
>               return sd_r1;
>   
> @@ -1337,6 +1337,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   
>       case 17:  /* CMD17:  READ_SINGLE_BLOCK */
>       case 18:  /* CMD18:  READ_MULTIPLE_BLOCK */
> +        addr = sd_req_get_address(sd, req);
>           switch (sd->state) {
>           case sd_transfer_state:
>   
> @@ -1357,6 +1358,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>       /* Block write commands (Class 4) */
>       case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
>       case 25:  /* CMD25:  WRITE_MULTIPLE_BLOCK */
> +        addr = sd_req_get_address(sd, req);
>           switch (sd->state) {
>           case sd_transfer_state:
>   
> @@ -1415,7 +1417,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           if (sd->size > SDSC_MAX_CAPACITY) {
>               return sd_illegal;
>           }
> -
> +        addr = sd_req_get_address(sd, req);
>           switch (sd->state) {
>           case sd_transfer_state:
>               if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
> @@ -1437,7 +1439,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           if (sd->size > SDSC_MAX_CAPACITY) {
>               return sd_illegal;
>           }
> -
> +        addr = sd_req_get_address(sd, req);
>           switch (sd->state) {
>           case sd_transfer_state:
>               if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
> @@ -1459,7 +1461,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           if (sd->size > SDSC_MAX_CAPACITY) {
>               return sd_illegal;
>           }
> -
> +        addr = sd_req_get_address(sd, req);
>           switch (sd->state) {
>           case sd_transfer_state:
>               if (!address_in_range(sd, "SEND_WRITE_PROT",

[PATCH 18/23] hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode switch

[Philippe Mathieu-Daudé]

Having the mode switch displayed help to track incomplete
command implementations.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 75 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 34 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1df16ce6a2..8d63a39a54 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -180,6 +180,17 @@ static const char *sd_version_str(enum SDPhySpecificationVersion version)
     return sdphy_version[version];
 }
 
+static const char *sd_mode_name(enum SDCardModes mode)
+{
+    static const char *mode_name[] = {
+        [sd_inactive]                   = "inactive",
+        [sd_card_identification_mode]   = "identification",
+        [sd_data_transfer_mode]         = "transfer",
+    };
+    assert(mode < ARRAY_SIZE(mode_name));
+    return mode_name[mode];
+}
+
 static const char *sd_state_name(enum SDCardStates state)
 {
     static const char *state_name[] = {
@@ -1015,6 +1026,15 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
+static sd_rsp_type_t sd_invalid_mode_for_cmd(SDState *sd, SDRequest req)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong mode: %s (spec %s)\n",
+                  sd_proto(sd)->name, req.cmd, sd_mode_name(sd->mode),
+                  sd_version_str(sd->spec_version));
+
+    return sd_illegal;
+}
+
 static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i for spec %s\n",
@@ -1154,18 +1174,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 6:  /* CMD6:   SWITCH_FUNCTION */
-        switch (sd->mode) {
-        case sd_data_transfer_mode:
-            sd_function_switch(sd, req.arg);
-            sd->state = sd_sendingdata_state;
-            sd->data_start = 0;
-            sd->data_offset = 0;
-            return sd_r1;
-
-        default:
-            break;
+        if (sd->mode != sd_data_transfer_mode) {
+            return sd_invalid_mode_for_cmd(sd, req);
         }
-        break;
+        sd_function_switch(sd, req.arg);
+        sd->state = sd_sendingdata_state;
+        sd->data_start = 0;
+        sd->data_offset = 0;
+        return sd_r1;
 
     case 7:  /* CMD7:   SELECT/DESELECT_CARD */
         rca = sd_req_get_rca(sd, req);
@@ -1289,33 +1305,24 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     case 13:  /* CMD13:  SEND_STATUS */
         rca = sd_req_get_rca(sd, req);
-        switch (sd->mode) {
-        case sd_data_transfer_mode:
-            if (!sd_is_spi(sd) && sd->rca != rca) {
-                return sd_r0;
-            }
-
-            return sd_r1;
-
-        default:
-            break;
+        if (sd->mode != sd_data_transfer_mode) {
+            return sd_invalid_mode_for_cmd(sd, req);
         }
-        break;
+        if (!sd_is_spi(sd) && sd->rca != rca) {
+            return sd_r0;
+        }
+
+        return sd_r1;
 
     case 15:  /* CMD15:  GO_INACTIVE_STATE */
-        rca = sd_req_get_rca(sd, req);
-        switch (sd->mode) {
-        case sd_data_transfer_mode:
-            if (sd->rca != rca)
-                return sd_r0;
-
-            sd->state = sd_inactive_state;
-            return sd_r0;
-
-        default:
-            break;
+        if (sd->mode != sd_data_transfer_mode) {
+            return sd_invalid_mode_for_cmd(sd, req);
         }
-        break;
+        rca = sd_req_get_rca(sd, req);
+        if (sd->rca == rca) {
+            sd->state = sd_inactive_state;
+        }
+        return sd_r0;
 
     /* Block read commands (Class 2) */
     case 16:  /* CMD16:  SET_BLOCKLEN */
-- 
2.41.0

Re: [PATCH 18/23] hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode switch

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Having the mode switch displayed help to track incomplete
> command implementations.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 75 +++++++++++++++++++++++++++++-------------------------
>   1 file changed, 41 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1df16ce6a2..8d63a39a54 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -180,6 +180,17 @@ static const char *sd_version_str(enum SDPhySpecificationVersion version)
>       return sdphy_version[version];
>   }
>   
> +static const char *sd_mode_name(enum SDCardModes mode)
> +{
> +    static const char *mode_name[] = {
> +        [sd_inactive]                   = "inactive",
> +        [sd_card_identification_mode]   = "identification",
> +        [sd_data_transfer_mode]         = "transfer",
> +    };
> +    assert(mode < ARRAY_SIZE(mode_name));
> +    return mode_name[mode];
> +}
> +
>   static const char *sd_state_name(enum SDCardStates state)
>   {
>       static const char *state_name[] = {
> @@ -1015,6 +1026,15 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
>       return sd_illegal;
>   }
>   
> +static sd_rsp_type_t sd_invalid_mode_for_cmd(SDState *sd, SDRequest req)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong mode: %s (spec %s)\n",
> +                  sd_proto(sd)->name, req.cmd, sd_mode_name(sd->mode),
> +                  sd_version_str(sd->spec_version));
> +
> +    return sd_illegal;
> +}
> +
>   static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
>   {
>       qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i for spec %s\n",
> @@ -1154,18 +1174,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>           break;
>   
>       case 6:  /* CMD6:   SWITCH_FUNCTION */
> -        switch (sd->mode) {
> -        case sd_data_transfer_mode:
> -            sd_function_switch(sd, req.arg);
> -            sd->state = sd_sendingdata_state;
> -            sd->data_start = 0;
> -            sd->data_offset = 0;
> -            return sd_r1;
> -
> -        default:
> -            break;
> +        if (sd->mode != sd_data_transfer_mode) {
> +            return sd_invalid_mode_for_cmd(sd, req);
>           }
> -        break;
> +        sd_function_switch(sd, req.arg);
> +        sd->state = sd_sendingdata_state;
> +        sd->data_start = 0;
> +        sd->data_offset = 0;
> +        return sd_r1;
>   
>       case 7:  /* CMD7:   SELECT/DESELECT_CARD */
>           rca = sd_req_get_rca(sd, req);
> @@ -1289,33 +1305,24 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>   
>       case 13:  /* CMD13:  SEND_STATUS */
>           rca = sd_req_get_rca(sd, req);
> -        switch (sd->mode) {
> -        case sd_data_transfer_mode:
> -            if (!sd_is_spi(sd) && sd->rca != rca) {
> -                return sd_r0;
> -            }
> -
> -            return sd_r1;
> -
> -        default:
> -            break;
> +        if (sd->mode != sd_data_transfer_mode) {
> +            return sd_invalid_mode_for_cmd(sd, req);
>           }
> -        break;
> +        if (!sd_is_spi(sd) && sd->rca != rca) {
> +            return sd_r0;
> +        }
> +
> +        return sd_r1;
>   
>       case 15:  /* CMD15:  GO_INACTIVE_STATE */
> -        rca = sd_req_get_rca(sd, req);
> -        switch (sd->mode) {
> -        case sd_data_transfer_mode:
> -            if (sd->rca != rca)
> -                return sd_r0;
> -
> -            sd->state = sd_inactive_state;
> -            return sd_r0;
> -
> -        default:
> -            break;
> +        if (sd->mode != sd_data_transfer_mode) {
> +            return sd_invalid_mode_for_cmd(sd, req);
>           }
> -        break;
> +        rca = sd_req_get_rca(sd, req);
> +        if (sd->rca == rca) {
> +            sd->state = sd_inactive_state;
> +        }
> +        return sd_r0;
>   
>       /* Block read commands (Class 2) */
>       case 16:  /* CMD16:  SET_BLOCKLEN */

[PATCH 19/23] hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros

[Philippe Mathieu-Daudé]

These macros only save 3 chars and make the code harder
to maintain, simply remove them.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8d63a39a54..ca2c903c5b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -816,8 +816,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
     }
 }
 
-#define BLK_READ_BLOCK(a, len)  sd_blk_read(sd, a, len)
-#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
 #define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
 #define APP_WRITE_BLOCK(a, len)
 
@@ -869,7 +867,7 @@ static void sd_erase(SDState *sd)
                 continue;
             }
         }
-        BLK_WRITE_BLOCK(erase_addr, erase_len);
+        sd_blk_write(sd, erase_addr, erase_len);
     }
 }
 
@@ -1901,7 +1899,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
         if (sd->data_offset >= sd->blk_len) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
-            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+            sd_blk_write(sd, sd->data_start, sd->data_offset);
             sd->blk_written ++;
             sd->csd[14] |= 0x40;
             /* Bzzzzzzztt .... Operation complete.  */
@@ -1927,7 +1925,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
         if (sd->data_offset >= sd->blk_len) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
-            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+            sd_blk_read(sd, sd->data_start, sd->data_offset);
             sd->blk_written++;
             sd->data_start += sd->blk_len;
             sd->data_offset = 0;
@@ -2075,8 +2073,9 @@ uint8_t sd_read_byte(SDState *sd)
         break;
 
     case 17:  /* CMD17:  READ_SINGLE_BLOCK */
-        if (sd->data_offset == 0)
-            BLK_READ_BLOCK(sd->data_start, io_len);
+        if (sd->data_offset == 0) {
+            sd_blk_read(sd, sd->data_start, io_len);
+        }
         ret = sd->data[sd->data_offset ++];
 
         if (sd->data_offset >= io_len)
@@ -2089,7 +2088,7 @@ uint8_t sd_read_byte(SDState *sd)
                                   sd->data_start, io_len)) {
                 return 0x00;
             }
-            BLK_READ_BLOCK(sd->data_start, io_len);
+            sd_blk_read(sd, sd->data_start, io_len);
         }
         ret = sd->data[sd->data_offset ++];
 
-- 
2.41.0

Re: [PATCH 19/23] hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> These macros only save 3 chars and make the code harder
> to maintain, simply remove them.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 8d63a39a54..ca2c903c5b 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -816,8 +816,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
>       }
>   }
>   
> -#define BLK_READ_BLOCK(a, len)  sd_blk_read(sd, a, len)
> -#define BLK_WRITE_BLOCK(a, len) sd_blk_write(sd, a, len)
>   #define APP_READ_BLOCK(a, len)  memset(sd->data, 0xec, len)
>   #define APP_WRITE_BLOCK(a, len)
>   
> @@ -869,7 +867,7 @@ static void sd_erase(SDState *sd)
>                   continue;
>               }
>           }
> -        BLK_WRITE_BLOCK(erase_addr, erase_len);
> +        sd_blk_write(sd, erase_addr, erase_len);
>       }
>   }
>   
> @@ -1901,7 +1899,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>           if (sd->data_offset >= sd->blk_len) {
>               /* TODO: Check CRC before committing */
>               sd->state = sd_programming_state;
> -            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
> +            sd_blk_write(sd, sd->data_start, sd->data_offset);
>               sd->blk_written ++;
>               sd->csd[14] |= 0x40;
>               /* Bzzzzzzztt .... Operation complete.  */
> @@ -1927,7 +1925,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>           if (sd->data_offset >= sd->blk_len) {
>               /* TODO: Check CRC before committing */
>               sd->state = sd_programming_state;
> -            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
> +            sd_blk_read(sd, sd->data_start, sd->data_offset);
>               sd->blk_written++;
>               sd->data_start += sd->blk_len;
>               sd->data_offset = 0;
> @@ -2075,8 +2073,9 @@ uint8_t sd_read_byte(SDState *sd)
>           break;
>   
>       case 17:  /* CMD17:  READ_SINGLE_BLOCK */
> -        if (sd->data_offset == 0)
> -            BLK_READ_BLOCK(sd->data_start, io_len);
> +        if (sd->data_offset == 0) {
> +            sd_blk_read(sd, sd->data_start, io_len);
> +        }
>           ret = sd->data[sd->data_offset ++];
>   
>           if (sd->data_offset >= io_len)
> @@ -2089,7 +2088,7 @@ uint8_t sd_read_byte(SDState *sd)
>                                     sd->data_start, io_len)) {
>                   return 0x00;
>               }
> -            BLK_READ_BLOCK(sd->data_start, io_len);
> +            sd_blk_read(sd, sd->data_start, io_len);
>           }
>           ret = sd->data[sd->data_offset ++];
>   

[PATCH 20/23] hw/sd/sdcard: Add comments around registers and commands

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ca2c903c5b..95e23abd30 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -317,6 +317,8 @@ static uint8_t sd_crc7(const void *message, size_t width)
     return shift_reg;
 }
 
+/* Operation Conditions register */
+
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
 FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
@@ -366,6 +368,8 @@ static void sd_set_ocr(SDState *sd)
     }
 }
 
+/* SD Configuration register */
+
 static void sd_set_scr(SDState *sd)
 {
     sd->scr[0] = 0 << 4;        /* SCR structure version 1.0 */
@@ -388,6 +392,8 @@ static void sd_set_scr(SDState *sd)
     sd->scr[7] = 0x00;
 }
 
+/* Card IDentification register */
+
 #define MID     0xaa
 #define OID     "XY"
 #define PNM     "QEMU!"
@@ -413,6 +419,8 @@ static void sd_set_cid(SDState *sd)
     sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
 }
 
+/* Card-Specific Data register */
+
 #define HWBLOCK_SHIFT   9        /* 512 bytes */
 #define SECTOR_SHIFT    5        /* 16 kilobytes */
 #define WPGROUP_SHIFT   7        /* 2 megs */
@@ -482,6 +490,8 @@ static void sd_set_csd(SDState *sd, uint64_t size)
     sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
 }
 
+/* Relative Card Address register */
+
 static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
 {
     if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
@@ -490,6 +500,8 @@ static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
     return 0;
 }
 
+/* Card Status register */
+
 FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
 FIELD(CSR, APP_CMD,                     5,  1)
 FIELD(CSR, FX_EVENT,                    6,  1)
@@ -620,6 +632,8 @@ static void sd_reset(DeviceState *dev)
     sect = sd_addr_to_wpnum(size) + 1;
 
     sd->state = sd_idle_state;
+
+    /* card registers */
     sd->rca = 0x0000;
     sd->size = size;
     sd_set_ocr(sd);
@@ -1052,6 +1066,7 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
+/* CMD0 */
 static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
 {
     if (sd->state != sd_inactive_state) {
@@ -1062,6 +1077,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
     return sd_is_spi(sd) ? sd_r1 : sd_r0;
 }
 
+/* CMD1 */
 static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
 {
     sd->state = sd_transfer_state;
@@ -1069,6 +1085,7 @@ static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
     return sd_r1;
 }
 
+/* CMD2 */
 static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
 {
     switch (sd->state) {
@@ -1080,6 +1097,7 @@ static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
     }
 }
 
+/* CMD3 */
 static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
 {
     switch (sd->state) {
@@ -1094,6 +1112,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
     }
 }
 
+/* CMD19 */
 static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
 {
     if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
@@ -1110,6 +1129,7 @@ static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
     return sd_r1;
 }
 
+/* CMD23 */
 static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
 {
     if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
-- 
2.41.0

Re: [PATCH 20/23] hw/sd/sdcard: Add comments around registers and commands

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/sd/sd.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ca2c903c5b..95e23abd30 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -317,6 +317,8 @@ static uint8_t sd_crc7(const void *message, size_t width)
>       return shift_reg;
>   }
>   
> +/* Operation Conditions register */
> +
>   #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>   
>   FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
> @@ -366,6 +368,8 @@ static void sd_set_ocr(SDState *sd)
>       }
>   }
>   
> +/* SD Configuration register */
> +
>   static void sd_set_scr(SDState *sd)
>   {
>       sd->scr[0] = 0 << 4;        /* SCR structure version 1.0 */
> @@ -388,6 +392,8 @@ static void sd_set_scr(SDState *sd)
>       sd->scr[7] = 0x00;
>   }
>   
> +/* Card IDentification register */
> +
>   #define MID     0xaa
>   #define OID     "XY"
>   #define PNM     "QEMU!"
> @@ -413,6 +419,8 @@ static void sd_set_cid(SDState *sd)
>       sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>   }
>   
> +/* Card-Specific Data register */
> +
>   #define HWBLOCK_SHIFT   9        /* 512 bytes */
>   #define SECTOR_SHIFT    5        /* 16 kilobytes */
>   #define WPGROUP_SHIFT   7        /* 2 megs */
> @@ -482,6 +490,8 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>       sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>   }
>   
> +/* Relative Card Address register */
> +
>   static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
>   {
>       if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc) {
> @@ -490,6 +500,8 @@ static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
>       return 0;
>   }
>   
> +/* Card Status register */
> +
>   FIELD(CSR, AKE_SEQ_ERROR,               3,  1)
>   FIELD(CSR, APP_CMD,                     5,  1)
>   FIELD(CSR, FX_EVENT,                    6,  1)
> @@ -620,6 +632,8 @@ static void sd_reset(DeviceState *dev)
>       sect = sd_addr_to_wpnum(size) + 1;
>   
>       sd->state = sd_idle_state;
> +
> +    /* card registers */
>       sd->rca = 0x0000;
>       sd->size = size;
>       sd_set_ocr(sd);
> @@ -1052,6 +1066,7 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
>       return sd_illegal;
>   }
>   
> +/* CMD0 */
>   static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
>   {
>       if (sd->state != sd_inactive_state) {
> @@ -1062,6 +1077,7 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
>       return sd_is_spi(sd) ? sd_r1 : sd_r0;
>   }
>   
> +/* CMD1 */
>   static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
>   {
>       sd->state = sd_transfer_state;
> @@ -1069,6 +1085,7 @@ static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
>       return sd_r1;
>   }
>   
> +/* CMD2 */
>   static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
>   {
>       switch (sd->state) {
> @@ -1080,6 +1097,7 @@ static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
>       }
>   }
>   
> +/* CMD3 */
>   static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
>   {
>       switch (sd->state) {
> @@ -1094,6 +1112,7 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
>       }
>   }
>   
> +/* CMD19 */
>   static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
>   {
>       if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> @@ -1110,6 +1129,7 @@ static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
>       return sd_r1;
>   }
>   
> +/* CMD23 */
>   static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
>   {
>       if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {

[PATCH 21/23] hw/sd/sdcard: Do not store vendor data on block drive (CMD56)

[Philippe Mathieu-Daudé]

"General command" (GEN_CMD, CMD56) is described as:

  GEN_CMD is the same as the single block read or write
  commands (CMD24 or CMD17). The difference is that [...]
  the data block is not a memory payload data but has a
  vendor specific format and meaning.

Thus this block must not be stored overwriting data block
on underlying storage drive. Keep it in a dedicated
'vendor_data[]' array.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Is it safe to reuse VMSTATE_UNUSED_V() (which happens
to be the same size)?

Cc: Peter Xu <peterx@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
---
 hw/sd/sd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 95e23abd30..712fbc0926 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -143,6 +143,8 @@ struct SDState {
     uint64_t data_start;
     uint32_t data_offset;
     uint8_t data[512];
+    uint8_t vendor_data[512];
+
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
@@ -647,6 +649,7 @@ static void sd_reset(DeviceState *dev)
     sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
     sd->wp_group_bits = sect;
     sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
+    memset(sd->vendor_data, 0xec, sizeof(sd->vendor_data));
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = INVALID_ADDRESS;
     sd->erase_end = INVALID_ADDRESS;
@@ -762,7 +765,7 @@ static const VMStateDescription sd_vmstate = {
         VMSTATE_UINT64(data_start, SDState),
         VMSTATE_UINT32(data_offset, SDState),
         VMSTATE_UINT8_ARRAY(data, SDState, 512),
-        VMSTATE_UNUSED_V(1, 512),
+        VMSTATE_UINT8_ARRAY(vendor_data, SDState, 512),
         VMSTATE_BOOL(enable, SDState),
         VMSTATE_END_OF_LIST()
     },
@@ -2019,9 +2022,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        sd->data[sd->data_offset ++] = value;
-        if (sd->data_offset >= sd->blk_len) {
-            APP_WRITE_BLOCK(sd->data_start, sd->data_offset);
+        sd->vendor_data[sd->data_offset ++] = value;
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
         }
         break;
@@ -2155,12 +2157,11 @@ uint8_t sd_read_byte(SDState *sd)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        if (sd->data_offset == 0)
-            APP_READ_BLOCK(sd->data_start, sd->blk_len);
-        ret = sd->data[sd->data_offset ++];
+        ret = sd->vendor_data[sd->data_offset ++];
 
-        if (sd->data_offset >= sd->blk_len)
+        if (sd->data_offset >= sizeof(sd->vendor_data)) {
             sd->state = sd_transfer_state;
+        }
         break;
 
     default:
-- 
2.41.0

[PATCH 22/23] hw/sd/sdcard: Send WRITE_PROT bits MSB first (CMD30)

[Philippe Mathieu-Daudé]

Per sections 3.6.1 (SD Bus Protocol) and 7.3.2 (Responses):

  In the CMD line the Most Significant Bit is transmitted first.

Use the stl_be_p() helper to store the value in big-endian.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC because I'm surprised this has been unnoticed for 17 years
(commit a1bb27b1e9 "initial SD card emulation", April 2007).

Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 712fbc0926..601a6908aa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1498,7 +1498,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             }
 
             sd->state = sd_sendingdata_state;
-            *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
+            stl_be_p(sd->data, sd_wpbits(sd, req.arg));
             sd->data_start = addr;
             sd->data_offset = 0;
             return sd_r1;
-- 
2.41.0

[PATCH 23/23] hw/sd/sdcard: Send NUM_WR_BLOCKS bits MSB first (ACMD22)

[Philippe Mathieu-Daudé]

Per sections 3.6.1 (SD Bus Protocol), 4.3.4 "Data Write"
and 7.3.2 (Responses):

  In the CMD line the Most Significant Bit is transmitted first.

Use the stl_be_p() helper to store the value in big-endian.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC because I'm surprised this has been unnoticed for 17 years
(commit a1bb27b1e9 "initial SD card emulation", April 2007).

Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/sd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 601a6908aa..5d572ad13c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1659,8 +1659,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     case 22:  /* ACMD22: SEND_NUM_WR_BLOCKS */
         switch (sd->state) {
         case sd_transfer_state:
-            *(uint32_t *) sd->data = sd->blk_written;
-
+            stl_be_p(sd->data, sd->blk_written);
             sd->state = sd_sendingdata_state;
             sd->data_start = 0;
             sd->data_offset = 0;
-- 
2.41.0

Re: [PATCH 00/23] hw/sd/sdcard: Accumulation of cleanups and fixes

[Cédric Le Goater]

On 6/21/24 10:05 AM, Philippe Mathieu-Daudé wrote:
> Various SD card cleanups and fixes accumulated over
> the years. Various have been useful to help integrating
> eMMC support (which will come later).
> 
> Based-on: <20240621075607.17902-1-philmd@linaro.org> st24_be_p()
> 
> Philippe Mathieu-Daudé (23):
>    hw/sd/sdcard: Correct code indentation
>    hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)
>    hw/sd/sdcard: Fix typo in SEND_OP_COND command name
>    hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values
>    hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition
>    hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers
>    hw/sd/sdcard: Remove ACMD6 handler for SPI mode
>    hw/sd/sdcard: Remove explicit entries for illegal commands
>    hw/sd/sdcard: Generate random RCA value
>    hw/sd/sdcard: Track last command used to help logging
>    hw/sd/sdcard: Trace update of block count (CMD23)
>    hw/sd/sdcard: Trace block offset in READ/WRITE data accesses
>    hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value
>    hw/sd/sdcard: Factor sd_req_get_rca() method out
>    hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used
>    hw/sd/sdcard: Factor sd_req_get_address() method out
>    hw/sd/sdcard: Only call sd_req_get_address() where address is used
>    hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode
>      switch
>    hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros
>    hw/sd/sdcard: Add comments around registers and commands
>    hw/sd/sdcard: Do not store vendor data on block drive (CMD56)
>    hw/sd/sdcard: Send WRITE_PROT bits MSB first (CMD30)
>    hw/sd/sdcard: Send NUM_WR_BLOCKS bits MSB first (ACMD22)
> 
>   hw/sd/sd.c             | 278 +++++++++++++++++++++++------------------
>   hw/sd/sdmmc-internal.c |   2 +-
>   hw/sd/trace-events     |   7 +-
>   3 files changed, 159 insertions(+), 128 deletions(-)
> 


Tested-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH 00/23] hw/sd/sdcard: Accumulation of cleanups and fixes

[Philippe Mathieu-Daudé]

On 21/6/24 10:05, Philippe Mathieu-Daudé wrote:
> Various SD card cleanups and fixes accumulated over
> the years. Various have been useful to help integrating
> eMMC support (which will come later).
> 
> Based-on: <20240621075607.17902-1-philmd@linaro.org> st24_be_p()
> 
> Philippe Mathieu-Daudé (23):
>    hw/sd/sdcard: Correct code indentation
>    hw/sd/sdcard: Rewrite sd_cmd_ALL_SEND_CID using switch case (CMD2)
>    hw/sd/sdcard: Fix typo in SEND_OP_COND command name
>    hw/sd/sdcard: Use HWBLOCK_SHIFT definition instead of magic values
>    hw/sd/sdcard: Use registerfield CSR::CURRENT_STATE definition
>    hw/sd/sdcard: Use Load/Store API to fill some CID/CSD registers
>    hw/sd/sdcard: Remove ACMD6 handler for SPI mode
>    hw/sd/sdcard: Remove explicit entries for illegal commands

>    hw/sd/sdcard: Have cmd_valid_while_locked() return a boolean value
>    hw/sd/sdcard: Factor sd_req_get_rca() method out
>    hw/sd/sdcard: Only call sd_req_get_rca() where RCA is used
>    hw/sd/sdcard: Factor sd_req_get_address() method out
>    hw/sd/sdcard: Only call sd_req_get_address() where address is used
>    hw/sd/sdcard: Add sd_invalid_mode_for_cmd to report invalid mode
>      switch
>    hw/sd/sdcard: Inline BLK_READ_BLOCK / BLK_WRITE_BLOCK macros
>    hw/sd/sdcard: Add comments around registers and commands

Patches 1-8, 13-20 queued.