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

[Peter Crosthwaite]

On Sun, Dec 6, 2015 at 10:03 PM, Andrew Baumann
<address@hidden> wrote:
> Hi Peter,
>
>> From: Peter Crosthwaite [mailto:address@hidden
>> Sent: Sunday, 6 December 2015 15:47
>> 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.
>
> Thanks for the detailed investigation of this. I now actually think you're 
> probably correct. The MMC spec is certainly clear, much more so than the SD 
> spec which I was looking at until now.
>
> I went looking for the UEFI code. It turns out that this use of CMD23 is not 
> in the mainline version of EDK2; it appears to have been added somewhere 
> along the way to building the Pi2 bootloader (I'm not sure why), so we 
> probably shouldn't place a huge amount of faith in it.

Is that in a git repo that we could run a git blame on?

Regards,
Peter

>It also doesn't check for the success/failure of the CMD12 which it issues. 
>After my original implementation, I saw a console full of "CMD12 in a wrong 
>state" printfs and immediately assumed I was doing something wrong, but I now 
>think my first guess was correct and I should have silenced the printf 
>instead. I'll dust that code off and resubmit the patch, assuming it tests ok.
>
> Andrew