Re: [PATCH][RESEND][mmc/host]:Blackfin SD Host Controller Driver

[Pierre Ossman]

On Fri, 17 Apr 2009 01:40:41 +0800
cliffcai.sh@xxxxxxxxx wrote:

> From: Cliff Cai <cliffcai.sh@xxxxxxxxx>
> 
> Signed-off-by: Cliff Cai <cliffcai.sh@xxxxxxxxx>
> ---
>  drivers/mmc/host/Kconfig    |   19 ++
>  drivers/mmc/host/Makefile   |    1 +
>  drivers/mmc/host/bfin_sdh.c |  648 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 668 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mmc/host/bfin_sdh.c
> 

I'd also like a MAINTAINERS entry for the driver.

> +config SDH_BFIN
> +	tristate "Blackfin Secure Digital Host support"
> +	depends on MMC && ((BF54x && !BF544) || (BF51x && !BF512))

You don't need "depends on MMC" as the entire block is conditioned on
MMC already.

> +/* In term of ADSP_BF5xx_Blackfin_Processor_Peripheral_Hardware_Reference,
> + * the SDH allows software to detect a card when it is inserted into its slot.
> + * The SD_DATA3 pin powers up low due to a special pull-down resistor. When an
> + * SD Card is inserted in its slot, the resistance increases and a rising edge
> + * is detected by the SDH module.
> + * But this doesn't work sometimes. When a MMC/SD card is inserted, the voltage
> + * doesn't rise on SD_DATA3. In term of The MultiMediaCard System Specification,
> + * SD_DATA3 is used as CS pin in SPI mode. The MultiMediaCard wakes up in the
> + * MultiMediaCard mode. During the scan procedure, host will send CMD0 to reset
> + * MMC card, if CS pin is low, MMC card will enter SPI mode. Of course Secure
> + * Digital Host controller is not a SPI controller. So the Card detect function
> + * has to be disabled. After card is inserted run "echo 0 > /proc/driver/sdh"
> + * to trigger card scanning */

If the controller can only do DAT3-detection, then I think it's best if
we put it into polling mode.

> +	BUG_ON(data->blksz & (data->blksz -1));

Not a bug so you need to deal with this. Most likely fail the request
with EINVAL.

> +#if defined(CONFIG_BF54x)
> +	dma_cfg |= DMAFLOW_ARRAY | NDSIZE_5 | RESTART | WDSIZE_32 | DMAEN;
> +	for (i = 0; i < host->dma_len; i++) {
> +		host->sg_cpu[i].start_addr = sg_dma_address(&data->sg[i]);

You cannot index sg lists directly anymore. You have to iterate over
them using the iteration helpers.

> +	if (!data->error)
> +		data->bytes_xfered = data->blocks * data->blksz;
> +	else
> +		data->bytes_xfered = data->blocks * data->blksz - \
> +				     bfin_read_SDH_DATA_CNT();

This is probably wrong. You need to set bytes_xfered to the number of
bytes acked by the card, not the number of bytes sent over the wire
(for writes that is). If your hardware can provide that then fine,
otherwise set bytes_xfered to 0 on failure.

> +	mmc->ops = &sdh_ops;
> +	mmc->max_phys_segs = NR_SG;
> +	mmc->max_seg_size = 1 << 16;
> +	mmc->max_blk_size = 2 << 11;
> +	mmc->max_blk_count = 2 << 16;

You forgot max_req_size.

> +out3:
> +	free_dma(host->dma_ch);

You need a mmc_remove_host() here.

Also check Mike's comments.

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: signature.asc
Description: PGP signature