Hi Peter, > From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Sunday, 6 December 2015 15:47 > On Sun, Dec 6, 2015 at 1:20 PM, Andrew Baumann > <andrew.baum...@microsoft.com> wrote: > >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > >> Sent: Saturday, 5 December 2015 22:27 > >> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann > >> <andrew.baum...@microsoft.com> 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
