Hi Andy,
Thanks for you review.
On 04/28/2010 11:21 PM, Andy Fleming wrote:
The crc7 lib func is merged from linux and used to compute mmc command checksum.
This should probably be a separate patch.
OK.
do {mmc = find_mmc_device(++dev_num);} while (mmc&& strcmp(mmc->name, "MMC_SPI"));if (!mmc) {printf("Create MMC Device\n");mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,CONFIG_MMC_SPI_CS,CONFIG_MMC_SPI_SPEED,CONFIG_MMC_SPI_MODE);if (!mmc) {printf("Failed to create MMC Device\n");return 1;}dev_num = mmc->block_dev.dev;}I'm not sure I understand the logic behind this code. The arguments to the command should be used to either find the already-existing bus, or to create a new one. Unless I'm misunderstanding, this searches for the first MMC_SPI bus, and if it finds it, uses that, otherwise it creates a new one with the values specified in the config file. Then it parses the command and overwrites the old parameters for the bus with new ones? Why? My instinct would be to create a separate instance of an MMC_SPI bus for each bus and chip select. My SPI is rusty, so maybe chip-select should be configurable on a use-by-use basis.
Certainly the current code will only use at most one MMC_SPI bus even if more are created, which seems wrong.
Though more than one MMC_SPI device can be created for specific bus and cs by calling mmc_spi_init() within board_mmc_init() or cpu_mmc_init(). This command is used to change the spi bus and chip select on a use-by-use basis at run time, so one changeable mmc_spi device (the first one if we found) should be enough.
cmdo[1] = 0x40 + cmdidx;Use a #define'd constant for 0x40. Maybe do this:
/* MMC SPI commands start with a start bit "0" and a transmit bit "1" */ #define MMC_SPI_CMD(x) (0x40 | (x& 0x3f))
cmdo[1] = MMC_SPI_CMD(cmdidx);
OK. Will define macros for all the cmd, token and response.
Why not check the CRC to make sure your data is correct?
OK. Will check CRC on data packet.
+static inline uint rswab(u8 *d)
Did you mean "swap", here?
It should be swap.
Hmmm.... I'm not entirely sure this is the way to go. If I'm reading this correctly, this function is converting between the standard SD protocol, and the SPI version of the protocol. It might be better to make mmc.c aware of which protocol it is using, so it a) doesn't issue commands that the SPI card doesn't understand, and b) can properly interpret responses.
I have avoided to touch the core mmc.c, so that I won't break other SD hosts by accident. Only minor translation of initialization commands and status mapping is enough to support SPI protocol.
}if (data) {debug("%s:data %x %x %x\n", __func__,data->flags, data->blocks, data->blocksize);if (data->flags == MMC_DATA_READ) {r1 = mmc_spi_readdata(mmc, data->dest,data->blocks, data->blocksize);} else if (data->flags == MMC_DATA_WRITE) {if (cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK)r1 = mmc_spi_writeblock(mmc, data->src,data->blocks, data->blocksize);elser1 = mmc_spi_writedata(mmc, data->src,data->blocks, data->blocksize);}Why not check r1 to make sure your writes actually succeeded?
OK. Will check data response.
mmc->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;Is this part of the SPI spec? If so, this is fine, otherwise, we need to get the voltage information from the SPI bus, somehow.
There is no voltage capability from SPI bus. This assumes 3.3V interface. Should I include other voltage?
mmc->f_max = speed;mmc->f_min = mmc->f_max>> 9;What's the logic behind f_min being f_max/512?
It yields f_min lower than 400KHz to meet the requirement for MMC.
Best regards, Thomas