Re: [PATCH] Fixes and enhancements for the MMC SPI driver - revised

[Pierre Ossman <drzeus@drzeus.cx>]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

On Mon, 26 Jan 2009 14:18:19 +0100
Wolfgang Mües <wolfgang.mues@auerswald.de> wrote:

> Hi,
> 
> thanks to all contributors who have reviewed my patch, especial to 
> Andrew Morton for pointing out some problems.
> 
> Here is a revised patch incl. changelog. I have though about putting
> some warnings in the message log with printk, Pierre. But this is low-level
> stuff and will likely flood the logfile.
> 

There are some helpers to avoid flooding dmesg, but its a wish a not a
requirement so don't sweat it.

> changelog:

Please split these things up into one patch for each item. It keeps the
logs sensible and makes bisecting work better in case of problems.

> @@ -609,6 +616,18 @@ mmc_spi_writeblock(struct mmc_spi_host *
>  	struct spi_device	*spi = host->spi;
>  	int			status, i;
>  	struct scratch		*scratch = host->data;
> +	u32			pattern;
> +	struct timeval 		tv;
> +
> +	/*
> +	 * The MMC framework does a good job of computing timeouts
> +	 * according to the mmc/sd standard. However, we found that in
> +	 * SPI mode, there are many cards which need a longer timeout
> +	 * of 1s after receiving a long stream of write data.
> +	 */
> +	tv = ktime_to_timeval(timeout);
> +	if (tv.tv_sec == 0)
> +		timeout = ktime_set(1, 0);
>  
>  	if (host->mmc->use_spi_crc)
>  		scratch->crc_val = cpu_to_be16(

This might as well be handled when we compute the timeout in the core.
Having things spread out is just confusing.

> @@ -743,7 +777,12 @@ mmc_spi_readblock(struct mmc_spi_host *h
>  		return -EIO;
>  	}
>  
> -	if (host->mmc->use_spi_crc) {
> +	/*
> +	 * Omit the CRC check for CID and CSD reads. There are some SDHC
> +	 * cards which don't supply a valid CRC after CID reads.
> +	 * Note that the CID has it's own CRC7 value inside the data block.
> +	 */
> +	if (host->mmc->use_spi_crc && (t->len == MMC_SPI_BLOCKSIZE)) {
>  		u16 crc = crc_itu_t(0, t->rx_buf, t->len);
>  
>  		be16_to_cpus(&scratch->crc_val);

Hmmm... I think this also better handled in the core. Reorder things in
the init to not turn CRC checks on until after those registers have
been read.

Other than that, the changes look fine. So split them up and I can
queue them up for the next merge window.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]