Re: [Qemu-devel] [PATCH v4 17/20] sdcard: add SD SEND_TUNING_BLOCK (CMD19)

[Peter Maydell]

On 15 February 2018 at 22:13, Philippe Mathieu-Daudé <address@hidden> wrote:
> [based on a patch from Alistair Francis <address@hidden>
>  from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> Reviewed-by: Alistair Francis <address@hidden>
> ---
>  hw/sd/sd.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 911aae6233..4b0bb7992d 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1166,6 +1166,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>          }
>          break;
>
> +    case 19:    /* CMD19: SEND_TUNING_BLOCK (SD) */
> +        if (sd->state == sd_transfer_state) {
> +            sd->state = sd_sendingdata_state;
> +            sd->data_offset = 0;
> +            return sd_r1;
> +        }
> +        break;
> +
>      case 23:    /* CMD23: SET_BLOCK_COUNT */
>          switch (sd->state) {
>          case sd_transfer_state:
> @@ -1889,6 +1897,15 @@ void sd_write_data(SDState *sd, uint8_t value)
>      }
>  }
>
> +#define SD_TUNING_BLOCK_SIZE    64
> +
> +static const uint32_t sd_tunning_data[SD_TUNING_BLOCK_SIZE / 4] = {

Typo, s/tunning/tuning/.

> +    0xFF0FFF00, 0x0FFCC3CC, 0xC33CCCFF, 0xFEFFFEEF,
> +    0xFFDFFFDD, 0xFFFBFFFB, 0XBFFF7FFF, 0X77F7BDEF,

Can we have lowercase 'x' consistently here please?

> +    0XFFF0FFF0, 0X0FFCCC3C, 0XCC33CCCF, 0XFFEFFFEE,
> +    0XFFFDFFFD, 0XDFFFBFFF, 0XBBFFF7FF, 0XF77F7BDE,
> +};

A comment about what all these magic numbers are for would
be useful.


> +
>  uint8_t sd_read_data(SDState *sd)
>  {
>      /* TODO: Append CRCs */
> @@ -1968,6 +1985,13 @@ uint8_t sd_read_data(SDState *sd)
>          }
>          break;
>
> +    case 19:    /* CMD19:  SEND_TUNING_BLOCK (SD) */
> +        if (sd->data_offset >= SD_TUNING_BLOCK_SIZE - 1) {
> +            sd->state = sd_transfer_state;
> +        }
> +        ret = ((uint8_t *)(&sd_tunning_data))[sd->data_offset++];

If you're going to treat it as a uint8_t buffer it might be
cleaner to just declare it as a uint8_t buffer rather than
uint32_t: you'd get to avoid the cast here and the / 4 in
the array definition.

> +        break;
> +
>      case 22:   /* ACMD22: SEND_NUM_WR_BLOCKS */
>          ret = sd->data[sd->data_offset ++];
>
> --
> 2.16.1
>

thanks
-- PMM