| Message ID | 20201231113010.27108-6-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv: sifive_u: Add missing SPI support | expand |
On Thu, Dec 31, 2020 at 5:04 PM Bin Meng <bmeng.cn@gmail.com> 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 exisitng 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 > transfered outside the sd core, as in sd_read_byte() we see: > > if (sd->data_offset >= 64) > sd->state = sd_transfer_state; > > Given above reaons, let's drop it. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > hw/sd/sd.c | 18 ------------------ > 1 file changed, 18 deletions(-) Reviewed-by: Pragnesh Patel <pragnesh.patel@sifive.com> Tested-by: Pragnesh Patel <pragnesh.patel@sifive.com>
On 12/31/20 12:29 PM, 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. Yeah: https://www.mail-archive.com/qemu-devel@nongnu.org/msg531790.html > 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 exisitng 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 > transfered outside the sd core, Typos "existing", "transferred". Well, I tried by adding tests, see: https://www.mail-archive.com/qemu-devel@nongnu.org/msg531777.html (Series:) https://www.mail-archive.com/qemu-devel@nongnu.org/msg531768.html > as in sd_read_byte() we see: > > if (sd->data_offset >= 64) > sd->state = sd_transfer_state; > > Given above reaons, let's drop it. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > hw/sd/sd.c | 18 ------------------ > 1 file changed, 18 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 2036734da1..52c7217fe1 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -270,23 +270,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) @@ -842,7 +825,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)