diff mbox series

[v2,05/25] hw/sd: sd: Drop sd_crc16()

Message ID 20210123104016.17485-6-bmeng.cn@gmail.com (mailing list archive)
State New, archived
Headers show
Series hw/riscv: sifive_u: Add missing SPI support | expand

Commit Message

Bin Meng Jan. 23, 2021, 10:39 a.m. UTC
From: Bin Meng <bin.meng@windriver.com>

commit f6fb1f9b319f ("sdcard: Correct CRC16 offset in sd_function_switch()")
changed the 16-bit CRC to be stored at offset 64. In fact, this CRC
calculation is completely wrong. From the original codes, it wants
to calculate the CRC16 of the first 64 bytes of sd->data[], however
passing 64 as the `width` to sd_crc16() actually counts 256 bytes
starting from the `message` for the CRC16 calculation, which is not
what we want.

Besides that, it seems existing sd_crc16() algorithm does not match
the SD spec, which says CRC16 is the CCITT one but the calculation
does not produce expected result. It turns out the CRC16 was never
transferred outside the sd core, as in sd_read_byte() we see:

    if (sd->data_offset >= 64)
        sd->state = sd_transfer_state;

Given above reasons, let's drop it.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Pragnesh Patel <pragnesh.patel@sifive.com>
Tested-by: Pragnesh Patel <pragnesh.patel@sifive.com>

---

Changes in v2:
- Fix several typos in the commit message

 hw/sd/sd.c | 18 ------------------
 1 file changed, 18 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 24, 2021, 6:14 p.m. UTC | #1
On 1/23/21 11:39 AM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> commit f6fb1f9b319f ("sdcard: Correct CRC16 offset in sd_function_switch()")
> changed the 16-bit CRC to be stored at offset 64. In fact, this CRC
> calculation is completely wrong. From the original codes, it wants
> to calculate the CRC16 of the first 64 bytes of sd->data[], however
> passing 64 as the `width` to sd_crc16() actually counts 256 bytes
> starting from the `message` for the CRC16 calculation, which is not
> what we want.
> 
> Besides that, it seems existing sd_crc16() algorithm does not match
> the SD spec, which says CRC16 is the CCITT one but the calculation
> does not produce expected result. It turns out the CRC16 was never
> transferred outside the sd core, as in sd_read_byte() we see:
> 
>     if (sd->data_offset >= 64)
>         sd->state = sd_transfer_state;
> 
> Given above reasons, let's drop it.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Pragnesh Patel <pragnesh.patel@sifive.com>
> Tested-by: Pragnesh Patel <pragnesh.patel@sifive.com>
> 
> ---
> 
> Changes in v2:
> - Fix several typos in the commit message
> 
>  hw/sd/sd.c | 18 ------------------
>  1 file changed, 18 deletions(-)

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

Patch

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bfea5547d5..b3952514fe 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -271,23 +271,6 @@  static uint8_t sd_crc7(const void *message, size_t width)
     return shift_reg;
 }
 
-static uint16_t sd_crc16(const void *message, size_t width)
-{
-    int i, bit;
-    uint16_t shift_reg = 0x0000;
-    const uint16_t *msg = (const uint16_t *)message;
-    width <<= 1;
-
-    for (i = 0; i < width; i ++, msg ++)
-        for (bit = 15; bit >= 0; bit --) {
-            shift_reg <<= 1;
-            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1))
-                shift_reg ^= 0x1011;
-        }
-
-    return shift_reg;
-}
-
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
 FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
@@ -843,7 +826,6 @@  static void sd_function_switch(SDState *sd, uint32_t arg)
         sd->data[16 - (i >> 1)] |= new_func << ((i % 2) * 4);
     }
     memset(&sd->data[17], 0, 47);
-    stw_be_p(sd->data + 64, sd_crc16(sd->data, 64));
 }
 
 static inline bool sd_wp_addr(SDState *sd, uint64_t addr)