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);
>> + else
>> + r1 = 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
