From 281c5c95b259a0d9809977c9f407d4654c3a79aa Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:39:54 +0800 Subject: hw/sd: ssi-sd: Fix incorrect card response sequence Per the "Physical Layer Specification Version 8.00" chapter 7.5.1, "Command/Response", there is a minimum 8 clock cycles (Ncr) before the card response shows up on the data out line. However current implementation jumps directly to the sending response state after all 6 bytes command is received, which is a spec violation. Add a new state PREP_RESP in the ssi-sd state machine to handle it. Fixes: 775616c3ae8c ("Partial SD card SPI mode support") Signed-off-by: Bin Meng Tested-by: Pragnesh Patel Reviewed-by: Pragnesh Patel Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-4-bmeng.cn@gmail.com> [PMD: Change VMState version id 2 -> 3] Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/ssi-sd.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 9a75e0095c..d97646795a 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -36,6 +36,7 @@ do { fprintf(stderr, "ssi_sd: error: " fmt , ## __VA_ARGS__);} while (0) typedef enum { SSI_SD_CMD = 0, SSI_SD_CMDARG, + SSI_SD_PREP_RESP, SSI_SD_RESPONSE, SSI_SD_DATA_START, SSI_SD_DATA_READ, @@ -163,12 +164,16 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) s->response[1] = status; DPRINTF("Card status 0x%02x\n", status); } - s->mode = SSI_SD_RESPONSE; + s->mode = SSI_SD_PREP_RESP; s->response_pos = 0; } else { s->cmdarg[s->arglen++] = val; } return 0xff; + case SSI_SD_PREP_RESP: + DPRINTF("Prepare card response (Ncr)\n"); + s->mode = SSI_SD_RESPONSE; + return 0xff; case SSI_SD_RESPONSE: if (s->stopping) { s->stopping = 0; @@ -224,8 +229,8 @@ static int ssi_sd_post_load(void *opaque, int version_id) static const VMStateDescription vmstate_ssi_sd = { .name = "ssi_sd", - .version_id = 2, - .minimum_version_id = 2, + .version_id = 3, + .minimum_version_id = 3, .post_load = ssi_sd_post_load, .fields = (VMStateField []) { VMSTATE_UINT32(mode, ssi_sd_state), -- cgit v1.2.3-55-g7522 From dec6d33849f3589426c5a14dce264d5c6f86e85b Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:39:55 +0800 Subject: hw/sd: sd: Support CMD59 for SPI mode After the card is put into SPI mode, CRC check for all commands including CMD0 will be done according to CMD59 setting. But this command is currently unimplemented. Simply allow the decoding of CMD59, but the CRC remains unchecked. Signed-off-by: Bin Meng Tested-by: Pragnesh Patel Reviewed-by: Pragnesh Patel Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-5-bmeng.cn@gmail.com> Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/sd.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'hw') diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 4375ed5b8b..bfea5547d5 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1517,18 +1517,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req) if (!sd->spi) { goto bad_cmd; } - goto unimplemented_spi_cmd; + return sd_r1; default: bad_cmd: qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd); return sd_illegal; - - unimplemented_spi_cmd: - /* Commands that are recognised but not yet implemented in SPI mode. */ - qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n", - req.cmd); - return sd_illegal; } qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd); -- cgit v1.2.3-55-g7522 From e9d28020d267fc76aa261537c0114d4402d678da Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:39:56 +0800 Subject: hw/sd: sd: Drop sd_crc16() 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 Tested-by: Pragnesh Patel Reviewed-by: Pragnesh Patel Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-6-bmeng.cn@gmail.com> Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/sd.c | 18 ------------------ 1 file changed, 18 deletions(-) (limited to 'hw') 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) -- cgit v1.2.3-55-g7522 From 2d174cc38bf1e86ff0cf534510c0d097e3b23680 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:39:58 +0800 Subject: hw/sd: ssi-sd: Suffix a data block with CRC16 Per the SD spec, a valid data block is suffixed with a 16-bit CRC generated by the standard CCITT polynomial x16+x12+x5+1. This part is currently missing in the ssi-sd state machine. Without it, all data block transfer fails in guest software because the expected CRC16 is missing on the data out line. Fixes: 775616c3ae8c ("Partial SD card SPI mode support") Signed-off-by: Bin Meng Acked-by: Alistair Francis Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-8-bmeng.cn@gmail.com> [PMD: Change VMState version id 3 -> 4, check s->mode validity in post_load()] Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/ssi-sd.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index d97646795a..08852dc8d4 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -17,6 +17,7 @@ #include "hw/qdev-properties.h" #include "hw/sd/sd.h" #include "qapi/error.h" +#include "qemu/crc-ccitt.h" #include "qemu/module.h" #include "qom/object.h" @@ -40,6 +41,7 @@ typedef enum { SSI_SD_RESPONSE, SSI_SD_DATA_START, SSI_SD_DATA_READ, + SSI_SD_DATA_CRC16, } ssi_sd_mode; struct ssi_sd_state { @@ -48,6 +50,7 @@ struct ssi_sd_state { int cmd; uint8_t cmdarg[4]; uint8_t response[5]; + uint16_t crc16; int32_t arglen; int32_t response_pos; int32_t stopping; @@ -194,12 +197,24 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) case SSI_SD_DATA_START: DPRINTF("Start read block\n"); s->mode = SSI_SD_DATA_READ; + s->response_pos = 0; return 0xfe; case SSI_SD_DATA_READ: val = sdbus_read_byte(&s->sdbus); + s->crc16 = crc_ccitt_false(s->crc16, (uint8_t *)&val, 1); if (!sdbus_data_ready(&s->sdbus)) { DPRINTF("Data read end\n"); + s->mode = SSI_SD_DATA_CRC16; + } + return val; + case SSI_SD_DATA_CRC16: + val = (s->crc16 & 0xff00) >> 8; + s->crc16 <<= 8; + s->response_pos++; + if (s->response_pos == 2) { + DPRINTF("CRC16 read end\n"); s->mode = SSI_SD_CMD; + s->response_pos = 0; } return val; } @@ -211,7 +226,7 @@ static int ssi_sd_post_load(void *opaque, int version_id) { ssi_sd_state *s = (ssi_sd_state *)opaque; - if (s->mode > SSI_SD_DATA_READ) { + if (s->mode > SSI_SD_DATA_CRC16) { return -EINVAL; } if (s->mode == SSI_SD_CMDARG && @@ -229,14 +244,15 @@ static int ssi_sd_post_load(void *opaque, int version_id) static const VMStateDescription vmstate_ssi_sd = { .name = "ssi_sd", - .version_id = 3, - .minimum_version_id = 3, + .version_id = 4, + .minimum_version_id = 4, .post_load = ssi_sd_post_load, .fields = (VMStateField []) { VMSTATE_UINT32(mode, ssi_sd_state), VMSTATE_INT32(cmd, ssi_sd_state), VMSTATE_UINT8_ARRAY(cmdarg, ssi_sd_state, 4), VMSTATE_UINT8_ARRAY(response, ssi_sd_state, 5), + VMSTATE_UINT16(crc16, ssi_sd_state), VMSTATE_INT32(arglen, ssi_sd_state), VMSTATE_INT32(response_pos, ssi_sd_state), VMSTATE_INT32(stopping, ssi_sd_state), @@ -288,6 +304,7 @@ static void ssi_sd_reset(DeviceState *dev) s->cmd = 0; memset(s->cmdarg, 0, sizeof(s->cmdarg)); memset(s->response, 0, sizeof(s->response)); + s->crc16 = 0; s->arglen = 0; s->response_pos = 0; s->stopping = 0; -- cgit v1.2.3-55-g7522 From 3a67cbe619179c390908bf415159290acbe96ccd Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:39:59 +0800 Subject: hw/sd: ssi-sd: Add a state representing Nac Per the "Physical Layer Specification Version 8.00" chapter 7.5.2, "Data Read", there is a minimum 8 clock cycles (Nac) after the card response and before data block shows up on the data out line. This applies to both single and multiple block read operations. Current implementation of single block read already satisfies the timing requirement as in the RESPONSE state after all responses are transferred the state remains unchanged. In the next 8 clock cycles it jumps to DATA_START state if data is ready. However we need an explicit state when expanding our support to multiple block read in the future. Let's add a new state PREP_DATA explicitly in the ssi-sd state machine to represent Nac. Note we don't change the single block read state machine to let it jump from RESPONSE state to DATA_START state as that effectively generates a 16 clock cycles Nac, which might not be safe. As the spec says the maximum Nac shall be calculated from several fields encoded in the CSD register, we don't want to bother updating CSD to ensure our Nac is within range to complicate things. Signed-off-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-9-bmeng.cn@gmail.com> [PMD: Change VMState version id 4 -> 5] Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/ssi-sd.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 08852dc8d4..1cdaf73c29 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -39,6 +39,7 @@ typedef enum { SSI_SD_CMDARG, SSI_SD_PREP_RESP, SSI_SD_RESPONSE, + SSI_SD_PREP_DATA, SSI_SD_DATA_START, SSI_SD_DATA_READ, SSI_SD_DATA_CRC16, @@ -194,6 +195,10 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) s->mode = SSI_SD_CMD; } return 0xff; + case SSI_SD_PREP_DATA: + DPRINTF("Prepare data block (Nac)\n"); + s->mode = SSI_SD_DATA_START; + return 0xff; case SSI_SD_DATA_START: DPRINTF("Start read block\n"); s->mode = SSI_SD_DATA_READ; @@ -244,8 +249,8 @@ static int ssi_sd_post_load(void *opaque, int version_id) static const VMStateDescription vmstate_ssi_sd = { .name = "ssi_sd", - .version_id = 4, - .minimum_version_id = 4, + .version_id = 5, + .minimum_version_id = 5, .post_load = ssi_sd_post_load, .fields = (VMStateField []) { VMSTATE_UINT32(mode, ssi_sd_state), -- cgit v1.2.3-55-g7522 From 1fb85c42ca47e48dd0cfe153db85bdfc1213aedb Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:40:00 +0800 Subject: hw/sd: ssi-sd: Fix the wrong command index for STOP_TRANSMISSION This fixes the wrong command index for STOP_TRANSMISSION, the required command to interrupt the multiple block read command, in the old codes. It should be CMD12 (0x4c), not CMD13 (0x4d). Fixes: 775616c3ae8c ("Partial SD card SPI mode support") Signed-off-by: Bin Meng Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-10-bmeng.cn@gmail.com> Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/ssi-sd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 1cdaf73c29..12dffb6f55 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -83,7 +83,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) ssi_sd_state *s = SSI_SD(dev); /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data. */ - if (s->mode == SSI_SD_DATA_READ && val == 0x4d) { + if (s->mode == SSI_SD_DATA_READ && val == 0x4c) { s->mode = SSI_SD_CMD; /* There must be at least one byte delay before the card responds. */ s->stopping = 1; -- cgit v1.2.3-55-g7522 From bc1edaf2041f28d99c8ab102e14b948613080e17 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Sat, 23 Jan 2021 18:40:02 +0800 Subject: hw/sd: ssi-sd: Use macros for the dummy value and tokens in the transfer At present the codes use hardcoded numbers (0xff/0xfe) for the dummy value and block start token. Replace them with macros. Signed-off-by: Bin Meng Reviewed-by: Alistair Francis Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210123104016.17485-12-bmeng.cn@gmail.com> Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/ssi-sd.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) (limited to 'hw') diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 12dffb6f55..be1bb10164 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -78,6 +78,12 @@ OBJECT_DECLARE_SIMPLE_TYPE(ssi_sd_state, SSI_SD) #define SSI_SDR_ADDRESS_ERROR 0x2000 #define SSI_SDR_PARAMETER_ERROR 0x4000 +/* single block read/write, multiple block read */ +#define SSI_TOKEN_SINGLE 0xfe + +/* dummy value - don't care */ +#define SSI_DUMMY 0xff + static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) { ssi_sd_state *s = SSI_SD(dev); @@ -91,14 +97,14 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) switch (s->mode) { case SSI_SD_CMD: - if (val == 0xff) { + if (val == SSI_DUMMY) { DPRINTF("NULL command\n"); - return 0xff; + return SSI_DUMMY; } s->cmd = val & 0x3f; s->mode = SSI_SD_CMDARG; s->arglen = 0; - return 0xff; + return SSI_DUMMY; case SSI_SD_CMDARG: if (s->arglen == 4) { SDRequest request; @@ -173,15 +179,15 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) } else { s->cmdarg[s->arglen++] = val; } - return 0xff; + return SSI_DUMMY; case SSI_SD_PREP_RESP: DPRINTF("Prepare card response (Ncr)\n"); s->mode = SSI_SD_RESPONSE; - return 0xff; + return SSI_DUMMY; case SSI_SD_RESPONSE: if (s->stopping) { s->stopping = 0; - return 0xff; + return SSI_DUMMY; } if (s->response_pos < s->arglen) { DPRINTF("Response 0x%02x\n", s->response[s->response_pos]); @@ -194,16 +200,16 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) DPRINTF("End of command\n"); s->mode = SSI_SD_CMD; } - return 0xff; + return SSI_DUMMY; case SSI_SD_PREP_DATA: DPRINTF("Prepare data block (Nac)\n"); s->mode = SSI_SD_DATA_START; - return 0xff; + return SSI_DUMMY; case SSI_SD_DATA_START: DPRINTF("Start read block\n"); s->mode = SSI_SD_DATA_READ; s->response_pos = 0; - return 0xfe; + return SSI_TOKEN_SINGLE; case SSI_SD_DATA_READ: val = sdbus_read_byte(&s->sdbus); s->crc16 = crc_ccitt_false(s->crc16, (uint8_t *)&val, 1); @@ -224,7 +230,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val) return val; } /* Should never happen. */ - return 0xff; + return SSI_DUMMY; } static int ssi_sd_post_load(void *opaque, int version_id) -- cgit v1.2.3-55-g7522