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

[Andrew Baumann]

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