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

[Philippe Mathieu-Daudé]

On 05/22/2018 02:11 AM, Philippe Mathieu-Daudé wrote:
> 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.

Select:
Current Limit: 400mA (function name 1 to group No 4, arg slice [15:12]),
Command system: Vendor specific (function name 0xF to group No 2, arg
slice [7:4])

(gdb) p/x (1 << 31) | (1 << 12) | (0xf << 4)
0x800010f0
>>> do_cmd(6, 0x800010f0)
SWITCH_FUNC CMD06(0x800010f0) crc7:0xb7
00008001800180018001c001800100f00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008120

0000 // 0mA
8001 //6
8001 //5
8001 //4
8001 //3
c001 //2
8001 //1
0 //6
0 //5
f //function group 4 "0xF shows function set error with the argument."
0 //3
0 //function group 2 "The function which is result of the switch command"
0 //1
00 // v0

So the function 0xF of group No 2 was not selected.

--

Now let's try with 2 invalid functions (3 for group 3, 9 for group 2).

(gdb) p/x (1 << 31) | (0 << 12) | (0x3 << 8) | (0x9 << 4)
0x80000390

>>> do_cmd(6, 0x80000390)
SWITCH_FUNC CMD06(0x80000390) crc7:0x53
0000
8001
8001
8001
8001
c001
8001
0 //6
0 //5
0 //4
f //function group 3 "0xF shows function set error with the argument"
f //function group 2 "0xF shows function set error with the argument"
0 //1
00 // v0

--

Group 1 & 2 with valid selection, group 3 invalid (0x3):

(gdb) p/x (1 << 31) | (0x3 << 8) | (0xE << 4) | (0xf << 0)
0x800003ef

>>> do_cmd(6, 0x800003ef)
SWITCH_FUNC CMD06(0x800003ef) crc7:0x23
00008001800180018001c0018001000f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001431

0000
8001
8001
8001
8001
c001
8001
0 //6
0 //5
0 //4
f //function group 3 "0xF shows function set error with the argument"
0 //2
0 //1
00 // v0

> 
>>
>> 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.