[U-Boot] [PATCH v5] mmc: add generic mmc spi driver

[Thomas Chou <thomas@wytron.com.tw>]

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