Re: [RFC] mmc: add OMAP driver

[Russell King <rmk+lkml@arm.linux.org.uk>]

On Fri, Feb 24, 2006 at 12:38:24PM -0400, Carlos Aguiar wrote:
> All suggestions and comments are welcome.

Okay, further comments.  Almost there!

> +/* PIO only */
> +static void
> +mmc_omap_xfer_data(struct mmc_omap_host *host, int write)
> +{
> +	int n;
> +	void __iomem *reg;
> +	u16 *p;
> +
> +	if (host->buffer_bytes_left == 0) {
> +		host->sg_idx++;
> +		BUG_ON(host->sg_idx == host->sg_len);
> +		mmc_omap_sg_to_buf(host);
> +	}
> +	n = 64;
> +	if (n > host->buffer_bytes_left)
> +		n = host->buffer_bytes_left;
> +	host->buffer_bytes_left -= n;
> +	host->total_bytes_left -= n;
> +	host->data->bytes_xfered += n;
> +
> +	/* Optimize the loop a bit by calculating the register only
> +	 * once */
> +	reg = host->base + OMAP_MMC_REG_DATA;
> +	p = host->buffer;
> +	n /= 2;
> +	if (write) {
> +		while (n--)
> +			__raw_writew(*p++, reg);
> +	} else {
> +		while (n-- > 0)
> +			*p++ = __raw_readw(reg);
> +	}

I thought I made a comment about readsw/writesw being more efficient
versions of these?

> +	host->buffer = p;
> +}

> +static irqreturn_t mmc_omap_irq(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	struct mmc_omap_host * host = (struct mmc_omap_host *)dev_id;
> +	u16 status;
> +	int end_command;
> +	int end_transfer;
> +	int transfer_error;
> +
> +	if (host->cmd == NULL && host->data == NULL) {
> +		status = OMAP_MMC_READ(host->base, STAT);
> +		printk(KERN_INFO "MMC%d: Spurious interrupt 0x%04x\n",
> +				host->id, status);

		dev_info(mmc_dev(host->mmc), "spurious irq 0x%04x\n", status);

There's no need to print host->id - mmc_dev(host->mmc) will print the
platform device bus_id, which is generated from the platform device
name and the platform device id.

> +		if (status != 0) {
> +			OMAP_MMC_WRITE(host->base, STAT, status);
> +			OMAP_MMC_WRITE(host->base, IE, 0);
> +		}
> +		return IRQ_HANDLED;
> +	}
> +
> +	end_command = 0;
> +	end_transfer = 0;
> +	transfer_error = 0;
> +
> +	while ((status = OMAP_MMC_READ(host->base, STAT)) != 0) {
> +		OMAP_MMC_WRITE(host->base, STAT, status);
> +#ifdef CONFIG_MMC_DEBUG
> +		printk(KERN_DEBUG "MMC IRQ %04x (CMD %d): ", status,
> +		       host->cmd != NULL ? host->cmd->opcode : -1);

		dev_dbg(mmc_dev(host->mmc), "MMC IRQ %04x (CMD %d): ",
			status, host->cmd != NULL ? host->cmd->opcode : -1);

> +		mmc_omap_report_irq(status);
> +		printk("\n");
> +#endif
> +		if (host->total_bytes_left) {
> +			if ((status & OMAP_MMC_STAT_A_FULL) ||
> +			    (status & OMAP_MMC_STAT_END_OF_DATA))
> +				mmc_omap_xfer_data(host, 0);
> +			if (status & OMAP_MMC_STAT_A_EMPTY)
> +				mmc_omap_xfer_data(host, 1);
> +		}
> +
> +		if (status & OMAP_MMC_STAT_END_OF_DATA) {
> +			end_transfer = 1;
> +		}
> +
> +		if (status & OMAP_MMC_STAT_DATA_TOUT) {
> +			dev_dbg(mmc_dev(host->mmc), "MMC%d: Data timeout\n",
> +					host->id);

			dev_dbg(mmc_dev(host->mmc), "data timeout\n");

> +			if (host->data) {
> +				host->data->error |= MMC_ERR_TIMEOUT;
> +				transfer_error = 1;
> +			}
> +		}
> +
> +		if (status & OMAP_MMC_STAT_DATA_CRC) {
> +			if (host->data) {
> +				host->data->error |= MMC_ERR_BADCRC;
> +				dev_dbg(mmc_dev(host->mmc), "MMC%d: Data CRC
> +						error, bytes left %d\n",
> +						host->id,
> +						host->total_bytes_left);

				dev_dbg(mmc_dev(host->mmc),
					 "data CRC error, bytes left %d\n",
					host->total_bytes_left);

> +				transfer_error = 1;
> +			} else {
> +				dev_dbg(mmc_dev(host->mmc), "MMC%d: Data CRC
> +						error\n", host->id);

				dev_dbg(mmc_dev(host->mmc), "data CRC error\n");

> +			}
> +		}
> +
> +		if (status & OMAP_MMC_STAT_CMD_TOUT) {
> +			/* Timeouts are routine with some commands */
> +			if (host->cmd) {
> +				if (host->cmd->opcode != MMC_ALL_SEND_CID &&
> +						host->cmd->opcode !=
> +						MMC_SEND_OP_COND &&
> +						host->cmd->opcode !=
> +						MMC_APP_CMD &&
> +						!mmc_omap_cover_is_open(host))
> +					dev_err(mmc_dev(host->mmc), "MMC%d:
> +							Command timeout,
> +							CMD%d\n", host->id,
> +							host->cmd->opcode);

					dev_err(mmc_dev(host->mmc),
						"command timeout, CMD %d\n",
						host->cmd->opcode);

> +				host->cmd->error = MMC_ERR_TIMEOUT;
> +				end_command = 1;
> +			}
> +		}
> +
> +		if (status & OMAP_MMC_STAT_CMD_CRC) {
> +			if (host->cmd) {
> +				dev_err(mmc_dev(host->mmc), "MMC%d: Command CRC
> +						error (CMD%d, arg 0x%08x)\n",
> +						host->id, host->cmd->opcode,
> +						host->cmd->arg);

				dev_err(mmc_dev(host->mmc),
					"command CRC error (CMD%d, arg 0x%08x)\n",
					host->cmd->opcode, host->cmd->arg);

> +				host->cmd->error = MMC_ERR_BADCRC;
> +				end_command = 1;
> +			} else
> +				dev_err(mmc_dev(host->mmc), "MMC%d: Command CRC
> +						error without cmd?\n",
> +						host->id);

				dev_err(mmc_dev(host->mmc),
					"command CRC error without cmd?\n");

> +		}
> +
> +		if (status & OMAP_MMC_STAT_CARD_ERR) {
> +			if (host->cmd && host->cmd->opcode == MMC_STOP_TRANSMISSION) {
> +				u32 response = OMAP_MMC_READ(host->base, RSP6)
> +					| (OMAP_MMC_READ(host->base, RSP7) << 16);
> +				/* STOP sometimes sets must-ignore bits */
> +				if (!(response & (R1_CC_ERROR
> +								| R1_ILLEGAL_COMMAND
> +								| R1_COM_CRC_ERROR))) {
> +					end_command = 1;
> +					continue;
> +				}
> +			}
> +
> +			dev_dbg(mmc_dev(host->mmc), "MMC%d: Card status error (CMD%d)\n",
> +			       host->id, host->cmd->opcode);

			dev_dbg(mmc_dev(host->mmc), "card status error (CMD%d)\n",
				host->cmd->opcode);

> +			if (host->cmd) {
> +				host->cmd->error = MMC_ERR_FAILED;
> +				end_command = 1;
> +			}
> +			if (host->data) {
> +				host->data->error = MMC_ERR_FAILED;
> +				transfer_error = 1;
> +			}
> +		}
> +
> +		/*
> +		 * NOTE: On 1610 the END_OF_CMD may come too early when
> +		 * starting a write 
> +		 */
> +		if ((status & OMAP_MMC_STAT_END_OF_CMD) &&
> +		    (!(status & OMAP_MMC_STAT_A_EMPTY))) {
> +			end_command = 1;
> +		}
> +	}
> +
> +	if (end_command) {
> +		mmc_omap_cmd_done(host, host->cmd);
> +	}
> +	if (transfer_error)
> +		mmc_omap_xfer_done(host, host->data);
> +	else if (end_transfer)
> +		mmc_omap_end_of_data(host, host->data);
> +
> +	return IRQ_HANDLED;
> +}

Same comments for the other dev_* and printk's.

> +static int __init mmc_omap_probe(struct platform_device *pdev)
> +{
> +	struct omap_mmc_conf *minfo = pdev->dev.platform_data;
> +	struct mmc_host *mmc;
> +	struct mmc_omap_host *host = NULL;
> +	int ret = 0;
> +	
> +	if (platform_get_resource(pdev, IORESOURCE_MEM, 0) ||
> +			platform_get_irq(pdev, IORESOURCE_IRQ, 0)) {
> +		dev_err(&pdev->dev, "mmc_omap_probe: invalid resource type\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!request_mem_region(pdev->resource[0].start,
> +				pdev->resource[0].end - pdev->resource[0].start + 1,
> +				pdev->name)) {
> +		dev_dbg(&pdev->dev, "request_mem_region failed\n");
> +		return -EBUSY;
> +	}
> +
> +	mmc = mmc_alloc_host(sizeof(struct mmc_omap_host), &pdev->dev);
> +	if (!mmc) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	host = mmc_priv(mmc);
> +	host->mmc = mmc;
> +
> +	spin_lock_init(&host->dma_lock);
> +	init_timer(&host->dma_timer);
> +	host->dma_timer.function = mmc_omap_dma_timer;
> +	host->dma_timer.data = (unsigned long) host;
> +
> +	host->id = pdev->id;
> +
> +	if (cpu_is_omap24xx()) {
> +		host->iclk = clk_get(&pdev->dev, "mmc_ick");
> +		if (IS_ERR(host->iclk))
> +			goto out;
> +		clk_enable(host->iclk);
> +	}
> +
> +	if (!cpu_is_omap24xx())
> +		host->fclk = clk_get(&pdev->dev, "mmc_ck");
> +	else
> +		host->fclk = clk_get(&pdev->dev, "mmc_fck");
> +
> +	if (IS_ERR(host->fclk)) {
> +		ret = PTR_ERR(host->fclk);
> +		goto out;
> +	}
> +
> +	/* REVISIT:
> +	 * Also, use minfo->cover to decide how to manage
> +	 * the card detect sensing.
> +	 */
> +	host->power_pin = minfo->power_pin;
> +	host->switch_pin = minfo->switch_pin;
> +	host->wp_pin = minfo->wp_pin;
> +	host->use_dma = 1;
> +	host->dma_ch = -1;
> +
> +	host->irq = pdev->resource[1].start;
> +	host->base = ioremap(pdev->res.start, SZ_4K);

What if ioremap fails with NULL?

> +
> +	 if (minfo->wire4)
> +		 mmc->caps |= MMC_CAP_4_BIT_DATA;
> +
> +	mmc->ops = &mmc_omap_ops;
> +	mmc->f_min = 400000;
> +	mmc->f_max = 24000000;
> +	mmc->ocr_avail = MMC_VDD_33_34;

Some cards want at least two bits set - use MMC_VDD_32_33|MMC_VDD_33_34
here please.


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core