On 05/14/2018 12:38 PM, Peter Maydell wrote:
> On 9 May 2018 at 07:01, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
>> [based on a patch from Alistair Francis <alistair.fran...@xilinx.com>
>> from qemu/xilinx tag xilinx-v2015.2]
>> Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
>> [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é <f4...@amsat.org>
>> ---
>
>> + /* 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.
