On Mon, 24 Jan 2022 at 06:09, <frank.ch...@sifive.com> wrote: > > From: Frank Chang <frank.ch...@sifive.com> > > In SPI-mode, unlike SD-mode, card status bits: ILLEGAL_COMMAND and > COM_CRC_ERROR have type C ("cleared by read") clear conditions. > Also, type B ("cleared on valid command") clear condition is not > supported in SPI-mode. As the "In idle state" bit in SPI-mode has type A > ("according to current state") clear condition, the CURRENT_STATE bits > in an SPI-mode response should be the SD card's state after the command > is executed, instead of the state when it received the preceding > command. > > This patch redefines the card status clear conditions used in SD-mode > and SPI-mode according to SD spec. > > Signed-off-by: Frank Chang <frank.ch...@sifive.com>
This looks mostly OK to me, but it does show up that we have a rather odd way of implementing SPI mode. SPI mode's response word is a different format to SD mode (it's 16 bits, not 32), but we only report SD mode format status and require the device model that's called us to do the conversion (hw/sd/ssi-sd.c does this, for example). > +static uint32_t sd_card_status_c(SDState *sd) { > + uint32_t sd_mask = R_CSR_AKE_SEQ_ERROR_MASK | > + R_CSR_APP_CMD_MASK | > + R_CSR_ERASE_RESET_MASK | > + R_CSR_WP_ERASE_SKIP_MASK | > + R_CSR_CSD_OVERWRITE_MASK | > + R_CSR_ERROR_MASK | > + R_CSR_CC_ERROR_MASK | > + R_CSR_CARD_ECC_FAILED_MASK | > + R_CSR_LOCK_UNLOCK_FAILED_MASK | > + R_CSR_WP_VIOLATION_MASK | > + R_CSR_ERASE_PARAM_MASK | > + R_CSR_ERASE_SEQ_ERROR_MASK | > + R_CSR_BLOCK_LEN_ERROR_MASK | > + R_CSR_ADDRESS_ERROR_MASK | > + R_CSR_OUT_OF_RANGE_MASK; > + uint32_t spi_mask = R_CSR_ERASE_RESET_MASK | > + R_CSR_LOCK_UNLOCK_FAILED_MASK | > + R_CSR_WP_ERASE_SKIP_MASK | > + R_CSR_CSD_OVERWRITE_MASK | > + R_CSR_ERROR_MASK | > + R_CSR_CC_ERROR_MASK | > + R_CSR_CARD_ECC_FAILED_MASK | > + R_CSR_ILLEGAL_COMMAND_MASK | > + R_CSR_COM_CRC_ERROR_MASK| > + R_CSR_WP_VIOLATION_MASK | > + R_CSR_ERASE_PARAM_MASK | > + R_CSR_ERASE_SEQ_ERROR_MASK | > + R_CSR_ADDRESS_ERROR_MASK | > + R_CSR_OUT_OF_RANGE_MASK; > + > + return !sd->spi ? sd_mask : spi_mask; > +} I feel like it ought to be possible to write this something like sd_mask = CARD_STATUS_C; if (sd->spi) { sd_mask |= CARD_STATUS_B; } (ie all the SD mode status C bits are either not visible in SPI mode or are status C, and all the status B bits in SD mode should be status C.) > static void sd_set_cardstatus(SDState *sd) > { > @@ -522,7 +548,7 @@ static void sd_response_r1_make(SDState *sd, uint8_t > *response) > stl_be_p(response, sd->card_status); > > /* Clear the "clear on read" status bits */ > - sd->card_status &= ~CARD_STATUS_C; > + sd->card_status &= ~sd_card_status_c(sd); > } > > static void sd_response_r3_make(SDState *sd, uint8_t *response) > @@ -537,7 +563,7 @@ static void sd_response_r6_make(SDState *sd, uint8_t > *response) > status = ((sd->card_status >> 8) & 0xc000) | > ((sd->card_status >> 6) & 0x2000) | > (sd->card_status & 0x1fff); > - sd->card_status &= ~(CARD_STATUS_C & 0xc81fff); > + sd->card_status &= ~(sd_card_status_c(sd) & 0xc81fff); > stw_be_p(response + 0, sd->rca); > stw_be_p(response + 2, status); > } > @@ -1757,12 +1783,20 @@ int sd_do_command(SDState *sd, SDRequest *req, > if (rtype == sd_illegal) { > sd->card_status |= ILLEGAL_COMMAND; > } else { > - /* Valid command, we can update the 'state before command' bits. > - * (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); > + > + if (!sd->spi) { > + /* Valid command, we can update the 'state before command' bits. > + * (Do this now so they appear in r1 responses.) > + */ > + sd->card_status |= (last_state << 9); > + } else { > + /* Type B ("clear on valid command") is not supported > + * in SPI-mode. > + */ > + sd->card_status |= (sd->state << 9); > + } I think this is right, in that for SD mode we want these bits to be "relating to previous command", and for SPI mode they are going to end up being used by the caller to calculate the Idle bit. But shouldn't the other bits that are type B for SD mode also work this way? Either we're currently getting those wrong in SD mode (returning the CRC-error/illegal-command state for this command when we should be returning it for the previous command), or we're getting it wrong still in SPI mode (returning it for the previous command when it should be for this command)... > } > > send_response: > @@ -1811,7 +1845,7 @@ send_response: > /* Clear the "clear on valid command" status bits now we've > * sent any response > */ > - sd->card_status &= ~CARD_STATUS_B; > + sd->card_status &= ~sd_card_status_b(sd); > } > > #ifdef DEBUG_SD > -- > 2.31.1 thanks -- PMM