Re: [PATCH] hw/sd: Correct card status clear conditions in SPI-mode

[Peter Maydell]

On Mon, 24 Jan 2022 at 06:09, <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@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.chang@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