Re: [Qemu-devel] [PATCH v2 3/4] sdcard: Implement the UHS-I SWITCH_FUNCTION entries (Spec v3)

[Philippe Mathieu-Daudé]

On 05/14/2018 12:38 PM, Peter Maydell wrote:
> On 9 May 2018 at 07:01, 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: Edgar E. Iglesias <address@hidden>
>> [PMD: rebased, changed magic by definitions, use stw_be_p, add tracing,
>>  check all functions in group are correct before setting the values]
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
> 
>> +    /* Bits 376-399: function (4 bits each group)
>> +     *
>> +     * Do not write the values back directly:
>> +     * Check everything first writing to 'tmpbuf'
>> +     */
>> +    data_p = tmpbuf;
> 
> You don't need a tmpbuf here, because it doesn't matter if we
> write something to the data array that it turns out we don't want
> to write; we can always rewrite it later...
> 
>> +    for (fn_grp = SD_FG_MAX; fn_grp >= SD_FG_MIN; fn_grp--) {
>> +        new_func = (arg >> ((fn_grp - 1) * 4)) & 0x0f;
>> +        if (new_func == SD_FN_NO_INFLUENCE) {
>> +            /* Guest requested no influence, so this function group
>> +             * stays the same.
>> +             */
>> +            new_func = sd->function_group[fn_grp - 1];
>> +        } else {
>> +            const sd_fn_support *def = 
>> &sd_fn_support_defs[fn_grp][new_func];
>> +            if (mode) {
>> +                if (!def->name) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %d not valid for "
>> +                                  "function group %d\n",
>> +                                  new_func, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else if (def->unimp) {
>> +                    qemu_log_mask(LOG_UNIMP,
>> +                                  "Function %s (fn grp %d) not 
>> implemented\n",
>> +                                  def->name, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else if (def->uhs_only && !sd->uhs_activated) {
>> +                    qemu_log_mask(LOG_GUEST_ERROR,
>> +                                  "Function %s (fn grp %d) only "
>> +                                  "valid in UHS mode\n",
>> +                                  def->name, fn_grp);
>> +                    invalid_function_selected = true;
>> +                    new_func = SD_FN_NO_INFLUENCE;
>> +                } else {
>> +                    sd->function_group[fn_grp - 1] = new_func;
> 
> ...but don't want to update the function_group[n] to the new value until
> we've checked that all the selected values in the command are OK,
> so you either need a temporary for the new function values, or
> you need to do one pass over the inputs to check and another one to set.
> 
>> +                }
>> +            }
>> +            trace_sdcard_function_select(def->name, sd_fn_grp_name[fn_grp],
>> +                                         mode);
>> +        }
>> +        if (!(fn_grp & 0x1)) { /* evens go in high nibble */
>> +            *data_p = new_func << 4;
>> +        } else { /* odds go in low nibble */
>> +            *(data_p++) |= new_func;
>> +        }
>> +    }
>> +    if (invalid_function_selected) {
>> +        /* Ignore all the set values */
>> +        memset(&sd->data[14], 0, SD_FN_BUFSZ);
> 
> All-zeroes doesn't seem to match the spec. The spec says "The response
> to an invalid selection of function will be 0xF", which is a bit unclear,
> but has to mean at least that we return 0xf for the function groups which
> were invalid selections. I'm not sure what we should return as status
> for the function groups which weren't invalid; possibilities include:
>  * 0xf
>  * whatever the provided argument for that function group was
>  * whatever the current status for that function group is
If selection is 0xF (No influence) to query, response is
"whatever the current status for that function group is"
per group.

> 
> I don't suppose you're in a position to find out what an actual
> hardware SD card does?

I tested some SanDisk 'Ultra' card.

Tests output posted on this thread:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04840.html

I'll now rework sd_function_switch() before to respin.

Regards,

Phil.