[PATCH v2 0/8] hw/sd: sd: Erase operation and other fixes

[PATCH v2 0/8] hw/sd: sd: Erase operation and other fixes

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

This includes several fixes related to erase operation of a SD card.

Currently sd_erase() does not perform the actual erase operation to
reset the requested block contents to 0xFFs. When trying to add such
functionality, one issue was spotted that the write protection is
only supported by SDSC cards. This seems to be missed when adding
high capability card support to the SD model.

The write groups check is now bypassed in the erase and block write
handling codes for high capacity cards.

The last patch was previously sent as a standalone patch [1], and is
now included in this v2 series. It was about SD card in SPI mode that
CMD13 argument should be ignored.

[1] http://patchwork.ozlabs.org/project/qemu-devel/patch/20210129085124.21525-1-bmeng.cn@gmail.com/

Based-on:
http://patchwork.ozlabs.org/project/qemu-devel/list/?series=226785

Changes in v2:
- new patch: sd: Only SDSC cards support CMD28/29/30
- new patch: sd: Fix CMD30 response type
- new patch: sd: Skip write protect groups check in sd_erase() for high capacity card
- honor the write protection bits for SDSC cards
- new patch: sd: Skip write protect groups check in CMD24/25 for high capacity cards
- update commit message to include the reference in the spec

Bin Meng (8):
  hw/sd: sd: Fix address check in sd_erase()
  hw/sd: sd: Only SDSC cards support CMD28/29/30
  hw/sd: sd: Fix CMD30 response type
  hw/sd: sd: Move the sd_block_{read,write} and macros ahead
  hw/sd: sd: Skip write protect groups check in sd_erase() for high
    capacity cards
  hw/sd: sd: Actually perform the erase operation
  hw/sd: sd: Skip write protect groups check in CMD24/25 for high
    capacity cards
  hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode

 hw/sd/sd.c | 99 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH v2 1/8] hw/sd: sd: Fix address check in sd_erase()

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

For high capacity memory cards, the erase start address and end
address are multiplied by 512, but the address check is still
based on the original block number in sd->erase_{start, end}.

Fixes: 1bd6fd8ed593 ("hw/sd/sdcard: Do not attempt to erase out of range addresses")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 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 c99c0e93bb..a6a0b3dcc6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -760,7 +760,7 @@ static void sd_erase(SDState *sd)
         erase_end *= 512;
     }
 
-    if (sd->erase_start > sd->size || sd->erase_end > sd->size) {
+    if (erase_start > sd->size || erase_end > sd->size) {
         sd->card_status |= OUT_OF_RANGE;
         sd->erase_start = INVALID_ADDRESS;
         sd->erase_end = INVALID_ADDRESS;
-- 
2.25.1

[PATCH v2 2/8] hw/sd: sd: Only SDSC cards support CMD28/29/30

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Per the "Physical Layer Specification Version 8.00", table 4-26
(SD mode) and table 7-3 (SPI mode) command descriptions, the
following commands:

- CMD28 (SET_WRITE_PROT)
- CMD29 (CLR_WRITE_PROT)
- CMD30 (SEND_WRITE_PROT)

are only supported by SDSC cards.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- new patch: sd: Only SDSC cards support CMD28/29/30

 hw/sd/sd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a6a0b3dcc6..273bae0a9a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1284,6 +1284,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     /* Write protection (Class 6) */
     case 28:	/* CMD28:  SET_WRITE_PROT */
+        if (sd->size > SDSC_MAX_CAPACITY) {
+            return sd_illegal;
+        }
+
         switch (sd->state) {
         case sd_transfer_state:
             if (addr >= sd->size) {
@@ -1303,6 +1307,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 29:	/* CMD29:  CLR_WRITE_PROT */
+        if (sd->size > SDSC_MAX_CAPACITY) {
+            return sd_illegal;
+        }
+
         switch (sd->state) {
         case sd_transfer_state:
             if (addr >= sd->size) {
@@ -1322,6 +1330,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 30:	/* CMD30:  SEND_WRITE_PROT */
+        if (sd->size > SDSC_MAX_CAPACITY) {
+            return sd_illegal;
+        }
+
         switch (sd->state) {
         case sd_transfer_state:
             sd->state = sd_sendingdata_state;
-- 
2.25.1

[PATCH v2 3/8] hw/sd: sd: Fix CMD30 response type

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Per the "Physical Layer Specification Version 8.00", table 4-26
(SD mode) and table 7-3 (SPI mode) command descriptions, CMD30
response type is R1, not R1b.

Fixes: a1bb27b1e98a ("SD card emulation initial implementation")
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- new patch: sd: Fix CMD30 response type

 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 273bae0a9a..6af821b75b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1340,7 +1340,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
             sd->data_start = addr;
             sd->data_offset = 0;
-            return sd_r1b;
+            return sd_r1;
 
         default:
             break;
-- 
2.25.1

[PATCH v2 4/8] hw/sd: sd: Move the sd_block_{read, write} and macros ahead

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

These APIs and macros may be referenced by functions that are
currently before them. Move them ahead a little bit.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/sd.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6af821b75b..5bd95304f1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -739,6 +739,27 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
     qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
 }
 
+static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+    trace_sdcard_read_block(addr, len);
+    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
+        fprintf(stderr, "sd_blk_read: read error on host side\n");
+    }
+}
+
+static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+    trace_sdcard_write_block(addr, len);
+    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
+        fprintf(stderr, "sd_blk_write: write error on host side\n");
+    }
+}
+
+#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)
+
 static void sd_erase(SDState *sd)
 {
     int i;
@@ -1754,27 +1775,6 @@ send_response:
     return rsplen;
 }
 
-static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
-{
-    trace_sdcard_read_block(addr, len);
-    if (!sd->blk || blk_pread(sd->blk, addr, sd->data, len) < 0) {
-        fprintf(stderr, "sd_blk_read: read error on host side\n");
-    }
-}
-
-static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
-{
-    trace_sdcard_write_block(addr, len);
-    if (!sd->blk || blk_pwrite(sd->blk, addr, sd->data, len, 0) < 0) {
-        fprintf(stderr, "sd_blk_write: write error on host side\n");
-    }
-}
-
-#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)
-
 void sd_write_byte(SDState *sd, uint8_t value)
 {
     int i;
-- 
2.25.1

[PATCH v2 5/8] hw/sd: sd: Skip write protect groups check in sd_erase() for high capacity cards

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

High capacity cards don't support write protection hence we should
not preform the write protect groups check in sd_erase() for them.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- new patch: sd: Skip write protect groups check in sd_erase() for high capacity card

 hw/sd/sd.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5bd95304f1..f1f98bdec3 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -765,6 +765,7 @@ static void sd_erase(SDState *sd)
     int i;
     uint64_t erase_start = sd->erase_start;
     uint64_t erase_end = sd->erase_end;
+    bool sdsc = true;
 
     trace_sdcard_erase(sd->erase_start, sd->erase_end);
     if (sd->erase_start == INVALID_ADDRESS
@@ -779,6 +780,7 @@ static void sd_erase(SDState *sd)
         /* High capacity memory card: erase units are 512 byte blocks */
         erase_start *= 512;
         erase_end *= 512;
+        sdsc = false;
     }
 
     if (erase_start > sd->size || erase_end > sd->size) {
@@ -788,16 +790,20 @@ static void sd_erase(SDState *sd)
         return;
     }
 
-    erase_start = sd_addr_to_wpnum(erase_start);
-    erase_end = sd_addr_to_wpnum(erase_end);
     sd->erase_start = INVALID_ADDRESS;
     sd->erase_end = INVALID_ADDRESS;
     sd->csd[14] |= 0x40;
 
-    for (i = erase_start; i <= erase_end; i++) {
-        assert(i < sd->wpgrps_size);
-        if (test_bit(i, sd->wp_groups)) {
-            sd->card_status |= WP_ERASE_SKIP;
+    /* Only SDSC cards support write protect groups */
+    if (sdsc) {
+        erase_start = sd_addr_to_wpnum(erase_start);
+        erase_end = sd_addr_to_wpnum(erase_end);
+
+        for (i = erase_start; i <= erase_end; i++) {
+            assert(i < sd->wpgrps_size);
+            if (test_bit(i, sd->wp_groups)) {
+                sd->card_status |= WP_ERASE_SKIP;
+            }
         }
     }
 }
-- 
2.25.1

[PATCH v2 6/8] hw/sd: sd: Actually perform the erase operation

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present the sd_erase() does not erase the requested range of card
data to 0xFFs. Let's make the erase operation actually happen.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- honor the write protection bits for SDSC cards

 hw/sd/sd.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f1f98bdec3..b386f16fcb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -766,6 +766,9 @@ static void sd_erase(SDState *sd)
     uint64_t erase_start = sd->erase_start;
     uint64_t erase_end = sd->erase_end;
     bool sdsc = true;
+    uint64_t wpnum;
+    uint64_t erase_addr;
+    int erase_len = 1 << HWBLOCK_SHIFT;
 
     trace_sdcard_erase(sd->erase_start, sd->erase_end);
     if (sd->erase_start == INVALID_ADDRESS
@@ -794,17 +797,20 @@ static void sd_erase(SDState *sd)
     sd->erase_end = INVALID_ADDRESS;
     sd->csd[14] |= 0x40;
 
-    /* Only SDSC cards support write protect groups */
-    if (sdsc) {
-        erase_start = sd_addr_to_wpnum(erase_start);
-        erase_end = sd_addr_to_wpnum(erase_end);
-
-        for (i = erase_start; i <= erase_end; i++) {
-            assert(i < sd->wpgrps_size);
-            if (test_bit(i, sd->wp_groups)) {
+    memset(sd->data, 0xff, erase_len);
+    erase_addr = erase_start;
+    for (i = 0; i <= (erase_end - erase_start) / erase_len; i++) {
+        if (sdsc) {
+            /* Only SDSC cards support write protect groups */
+            wpnum = sd_addr_to_wpnum(erase_addr);
+            assert(wpnum < sd->wpgrps_size);
+            if (test_bit(wpnum, sd->wp_groups)) {
                 sd->card_status |= WP_ERASE_SKIP;
+                continue;
             }
         }
+        BLK_WRITE_BLOCK(erase_addr, erase_len);
+        erase_addr += erase_len;
     }
 }
 
-- 
2.25.1

[PATCH v2 7/8] hw/sd: sd: Skip write protect groups check in CMD24/25 for high capacity cards

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

High capacity cards don't support write protection hence we should
not preform the write protect groups check in CMD24/25 for them.

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- new patch: sd: Skip write protect groups check in CMD24/25 for high capacity cards

 hw/sd/sd.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b386f16fcb..75dc57bf0e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1274,8 +1274,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
+            if (sd->size <= SDSC_MAX_CAPACITY) {
+                if (sd_wp_addr(sd, sd->data_start)) {
+                    sd->card_status |= WP_VIOLATION;
+                }
             }
             if (sd->csd[14] & 0x30) {
                 sd->card_status |= WP_VIOLATION;
@@ -1827,9 +1829,11 @@ void sd_write_byte(SDState *sd, uint8_t value)
                 sd->card_status |= ADDRESS_ERROR;
                 break;
             }
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-                break;
+            if (sd->size <= SDSC_MAX_CAPACITY) {
+                if (sd_wp_addr(sd, sd->data_start)) {
+                    sd->card_status |= WP_VIOLATION;
+                    break;
+                }
             }
         }
         sd->data[sd->data_offset++] = value;
-- 
2.25.1

[PATCH v2 8/8] hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Unlike SD mode, when SD card is working in SPI mode, the argument
of CMD13 is stuff bits. Hence we should bypass the RCA check.

See "Physical Layer Specification Version 8.00", chapter 7.3.1.3
Detailed Command Description (SPI mode):

  "The card shall ignore stuff bits and reserved bits in an argument"

and Table 7-3 Commands and Arguments (SPI mode):

  "CMD13 Argument [31:0] stuff bits"

Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

Changes in v2:
- update commit message to include the reference in the spec

 hw/sd/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 75dc57bf0e..6d6da83b4b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1169,8 +1169,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 13:	/* CMD13:  SEND_STATUS */
         switch (sd->mode) {
         case sd_data_transfer_mode:
-            if (sd->rca != rca)
+            if (!sd->spi && sd->rca != rca) {
                 return sd_r0;
+            }
 
             return sd_r1;
 
-- 
2.25.1

Re: [PATCH v2 8/8] hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Unlike SD mode, when SD card is working in SPI mode, the argument
> of CMD13 is stuff bits. Hence we should bypass the RCA check.
> 
> See "Physical Layer Specification Version 8.00", chapter 7.3.1.3
> Detailed Command Description (SPI mode):
> 
>   "The card shall ignore stuff bits and reserved bits in an argument"
> 
> and Table 7-3 Commands and Arguments (SPI mode):
> 
>   "CMD13 Argument [31:0] stuff bits"
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - update commit message to include the reference in the spec
> 
>  hw/sd/sd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

Re: [PATCH v2 2/8] hw/sd: sd: Only SDSC cards support CMD28/29/30

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Per the "Physical Layer Specification Version 8.00", table 4-26
> (SD mode) and table 7-3 (SPI mode) command descriptions, the
> following commands:
> 
> - CMD28 (SET_WRITE_PROT)
> - CMD29 (CLR_WRITE_PROT)
> - CMD30 (SEND_WRITE_PROT)
> 
> are only supported by SDSC cards.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Only SDSC cards support CMD28/29/30
> 
>  hw/sd/sd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

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

Re: [PATCH v2 3/8] hw/sd: sd: Fix CMD30 response type

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Per the "Physical Layer Specification Version 8.00", table 4-26
> (SD mode) and table 7-3 (SPI mode) command descriptions, CMD30
> response type is R1, not R1b.
> 
> Fixes: a1bb27b1e98a ("SD card emulation initial implementation")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Fix CMD30 response type
> 
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH v2 7/8] hw/sd: sd: Skip write protect groups check in CMD24/25 for high capacity cards

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> High capacity cards don't support write protection hence we should
> not preform the write protect groups check in CMD24/25 for them.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Skip write protect groups check in CMD24/25 for high capacity cards
> 
>  hw/sd/sd.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

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

Re: [PATCH v2 6/8] hw/sd: sd: Actually perform the erase operation

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present the sd_erase() does not erase the requested range of card
> data to 0xFFs. Let's make the erase operation actually happen.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - honor the write protection bits for SDSC cards
> 
>  hw/sd/sd.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index f1f98bdec3..b386f16fcb 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -766,6 +766,9 @@ static void sd_erase(SDState *sd)
>      uint64_t erase_start = sd->erase_start;
>      uint64_t erase_end = sd->erase_end;
>      bool sdsc = true;
> +    uint64_t wpnum;
> +    uint64_t erase_addr;
> +    int erase_len = 1 << HWBLOCK_SHIFT;
>  
>      trace_sdcard_erase(sd->erase_start, sd->erase_end);
>      if (sd->erase_start == INVALID_ADDRESS
> @@ -794,17 +797,20 @@ static void sd_erase(SDState *sd)
>      sd->erase_end = INVALID_ADDRESS;
>      sd->csd[14] |= 0x40;
>  
> -    /* Only SDSC cards support write protect groups */
> -    if (sdsc) {
> -        erase_start = sd_addr_to_wpnum(erase_start);
> -        erase_end = sd_addr_to_wpnum(erase_end);
> -
> -        for (i = erase_start; i <= erase_end; i++) {
> -            assert(i < sd->wpgrps_size);
> -            if (test_bit(i, sd->wp_groups)) {
> +    memset(sd->data, 0xff, erase_len);
> +    erase_addr = erase_start;
> +    for (i = 0; i <= (erase_end - erase_start) / erase_len; i++) {
> +        if (sdsc) {
> +            /* Only SDSC cards support write protect groups */
> +            wpnum = sd_addr_to_wpnum(erase_addr);
> +            assert(wpnum < sd->wpgrps_size);
> +            if (test_bit(wpnum, sd->wp_groups)) {
>                  sd->card_status |= WP_ERASE_SKIP;
> +                continue;

So if a group is protected, you skip it but don't increase erase_addr.
If G#4 is protected and G#5 isn't, when you check G#5 you end erasing
G#4.

>              }
>          }
> +        BLK_WRITE_BLOCK(erase_addr, erase_len);
> +        erase_addr += erase_len;
>      }
>  }
>  
> 

Re: [PATCH v2 5/8] hw/sd: sd: Skip write protect groups check in sd_erase() for high capacity cards

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> High capacity cards don't support write protection hence we should
> not preform the write protect groups check in sd_erase() for them.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> 
> ---
> 
> Changes in v2:
> - new patch: sd: Skip write protect groups check in sd_erase() for high capacity card
> 
>  hw/sd/sd.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

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

Re: [PATCH v2 0/8] hw/sd: sd: Erase operation and other fixes

[Philippe Mathieu-Daudé]

On 2/16/21 4:02 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> This includes several fixes related to erase operation of a SD card.

> Bin Meng (8):
>   hw/sd: sd: Fix address check in sd_erase()
>   hw/sd: sd: Only SDSC cards support CMD28/29/30
>   hw/sd: sd: Fix CMD30 response type
>   hw/sd: sd: Move the sd_block_{read,write} and macros ahead
>   hw/sd: sd: Skip write protect groups check in sd_erase() for high
>     capacity cards
>   hw/sd: sd: Actually perform the erase operation
>   hw/sd: sd: Skip write protect groups check in CMD24/25 for high
>     capacity cards
>   hw/sd: sd: Bypass the RCA check for CMD13 in SPI mode
> 
>  hw/sd/sd.c | 99 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 64 insertions(+), 35 deletions(-)

Thanks, patches 1-5 and 7-8 applied to sdmmc-next tree.

Re: [PATCH v2 6/8] hw/sd: sd: Actually perform the erase operation

[Bin Meng]

On Sat, Feb 20, 2021 at 6:28 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/16/21 4:02 PM, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present the sd_erase() does not erase the requested range of card
> > data to 0xFFs. Let's make the erase operation actually happen.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >
> > ---
> >
> > Changes in v2:
> > - honor the write protection bits for SDSC cards
> >
> >  hw/sd/sd.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index f1f98bdec3..b386f16fcb 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -766,6 +766,9 @@ static void sd_erase(SDState *sd)
> >      uint64_t erase_start = sd->erase_start;
> >      uint64_t erase_end = sd->erase_end;
> >      bool sdsc = true;
> > +    uint64_t wpnum;
> > +    uint64_t erase_addr;
> > +    int erase_len = 1 << HWBLOCK_SHIFT;
> >
> >      trace_sdcard_erase(sd->erase_start, sd->erase_end);
> >      if (sd->erase_start == INVALID_ADDRESS
> > @@ -794,17 +797,20 @@ static void sd_erase(SDState *sd)
> >      sd->erase_end = INVALID_ADDRESS;
> >      sd->csd[14] |= 0x40;
> >
> > -    /* Only SDSC cards support write protect groups */
> > -    if (sdsc) {
> > -        erase_start = sd_addr_to_wpnum(erase_start);
> > -        erase_end = sd_addr_to_wpnum(erase_end);
> > -
> > -        for (i = erase_start; i <= erase_end; i++) {
> > -            assert(i < sd->wpgrps_size);
> > -            if (test_bit(i, sd->wp_groups)) {
> > +    memset(sd->data, 0xff, erase_len);
> > +    erase_addr = erase_start;
> > +    for (i = 0; i <= (erase_end - erase_start) / erase_len; i++) {
> > +        if (sdsc) {
> > +            /* Only SDSC cards support write protect groups */
> > +            wpnum = sd_addr_to_wpnum(erase_addr);
> > +            assert(wpnum < sd->wpgrps_size);
> > +            if (test_bit(wpnum, sd->wp_groups)) {
> >                  sd->card_status |= WP_ERASE_SKIP;
> > +                continue;
>
> So if a group is protected, you skip it but don't increase erase_addr.
> If G#4 is protected and G#5 isn't, when you check G#5 you end erasing
> G#4.
>

Oops, good catch!

I will send v2.

> >              }
> >          }
> > +        BLK_WRITE_BLOCK(erase_addr, erase_len);
> > +        erase_addr += erase_len;
> >      }
> >  }

Regards,
Bin