Re: [Qemu-devel] [PATCH 1/2] hw/sd: implement CMD23 (SET_BLOCK_COUNT) for MMC compatibility

[Peter Crosthwaite]

On Sun, Dec 6, 2015 at 1:20 PM, Andrew Baumann
<address@hidden> wrote:
>> From: Peter Crosthwaite [mailto:address@hidden
>> Sent: Saturday, 5 December 2015 22:27
>> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
>> <address@hidden> wrote:
>> > @@ -1563,6 +1592,11 @@ void sd_write_data(SDState *sd, uint8_t value)
>> >              sd->data_offset = 0;
>> >              sd->csd[14] |= 0x40;
>> >
>> > +            if (sd->multi_blk_active) {
>> > +                assert(sd->multi_blk_cnt > 0);
>> > +                sd->multi_blk_cnt--;
>>
>> So reading the spec, it says that this command is an alternative to a
>> host issued CMD12. Does this mean completion of the length-specified
>> read should move back to sd_transfer_state the same way CMD12 does?
>
> That was my initial assumption (and implementation) too, but no: table 4-34 
> in the spec shows that you stay in transfer mode, and UEFI issues CMD12 after 
> each multiple-block (CMD23) I/O, which doesn't work if you've already left 
> the transfer state.

So table 4-34 doesn't really help as that is showing the state
following a command itself, whereas an automatic CMD12 would happen
following the end of a data payload (not an explicit command). It
would if anything be a form of "Operation complete" (first line in
4-34) which would be a state transition. You mention that CMD12
doesn't work if you have left transfer_state, and that is accurate.
However CMD12 normally doesn't transfer out of the transfer_state,
instead it transfers from data->tran or rcv->prog (note rcv->prog is
as good as rcv->tran for us as we model prog state as instantaneous).
If your original implementation transferred to something that is not
tran that would fail.

I have both the eMMC and SD specs infront of me. Here is what I have for eMMC:

"
Multiple block read with pre-defined block count:

The card will transfer the requested number of data blocks, terminate
the transaction and return to transfer state. Stop command is not
required at the end of this type of multiple block read, unless
terminated with an error. In order to start a multiple block read with
pre-defined block count the host must use the SET_BLOCK_COUNT command
(CMD23) immediately preceding the READ_MULTIPLE_BLOCK (CMD18) command.
Otherwise the card will start an open-ended multiple block read which
can be stopped using the STOP_TRANSMISION command.
"

That to me suggests that CMD12 should not be needed at all, but does
mention CMD12 is needed in error paths.

SD spec has this:

"
CMD12 has been used to stop multiple-block Read / Write operation.
However, CMD12 is timing dependent and it is difficult to control
timing to issue CMD12 at exact timing. As UHS104 card has large delay
variation between clock and data, CMD23 is useful for the host to stop
multiple read / write operation instead of CMD12. Host is not
necessary to control timing of CMD12. This command is applicable to
always 512-byte block length read/write operation and then SDSC card
does not support this command. Support of CMD23 is mandatory for
UHS104 card.

Support of CMD23 is defined in SCR. The response type of CMD23 is R1
and busy is not indicated. CMD23 is accepted in transfer state and
effective to the multiple-block read/write command (CMD18 or CMD25)
just  behind CMD23. If another command follows CMD23, set block count
is canceled (including CMD13). If command CRC error occurs, the card
does not return R1 response for CMD23. In this case, Set block count
is not valid and retry of CMD23 is required. If multiple CMD23 are
issued, the last one is valid.

Figure 4-58 shows the definition of CMD23. If block count in the
argument is set to 0, CMD23 has no effect. The block count value set
by CMD23 is not checked by the card and then CMD23 does not indicate
any error in the response (A previous command error is indicated in
the response of CMD23). If illegal block count is set, out of range
error will be indicated during read/write operation (For example, data
transfer is stopped at user area boundary). Host needs to issue CMD12
if any error is detected in the CMD18 and CMD25 operations. If a CMD25
is aborted and the amount of data transferred is less
than the amount of data indicated by the preceding CMD23, then the
area specified by CMD23 that is unwritten may contain undefined data.
If the amount of data transferred is greater than the amount of
data indicated by the preceding CMD23, then the extra data is not written.
"

which is significantly less clear but still suggest that CMD12 is not
needed. The last bit ("If the amount of data transferred is greater
than the amount of data indicated by the preceding CMD23, then the
extra data is not written") is what you have implemented very
directly, but would also be a consequence of an automatic CMD12 at the
end of a data-phase.

Some possibilities:

1: The SD card vendors don't follow the spec (as shown in 4-34) and
just ignore a spurious CMD12 when you have already returned to
transfer state. This would explain everything.
2: UEFI is using early termination in the normal code path.

Can you give me a source code pointer to the CMD12 in UEFI by any
chance? I'd like to have a look at the driver logic.

Regards,
Peter

>
>> >      case 18:   /* CMD18:  READ_MULTIPLE_BLOCK */
>> > +        if (sd->multi_blk_active && sd->multi_blk_cnt == 0) {
>> > +            /* we've overrun the block count */
>> > +            fprintf(stderr, "sd_read_data: overrun of multi-block 
>> > transfer\n");
>>
>> So if the card stays in-state and the intention is to discard overrun,
>> this message is likely to be flood prone. From my understanding (which
>> is pretty fresh) CMD23 may be designed to gracefully handle this
>> condition from the guest, which would suggest this is not an error at
>> all and no message is needed. If you do want to keep a message it
>> should be GUEST_ERROR and have a squelching mechanism so reading one
>> block past the end of CMD23 doesn't cause a large number of messages.
>
> Point taken. These printfs were more to assure myself that they never 
> happened; they can be removed. (It did seem weird to use a bare printf, but 
> all the code around here seemed to be doing the same thing.)
>
> I'll send a revised patch and respond to the other feedback probably later 
> next week.
>
> Thanks,
> Andrew