[U-Boot] [PATCH v2 1/4] mmc: dw-mmc: support DesignWare MMC Controller

Andy Fleming
2012-08-30T21:11:45Z

On Tue, Jul 3, 2012 at 12:58 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> This patch is supported DesginWare MMC Controller.
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Rajeshawari Shinde <rajeshwari.s@samsung.com>

>
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c

> + while (timeout--) {
> + ctrl = dwmci_readl(host, DWMCI_CTRL);
> + if (!(ctrl & DWMCI_RESET_ALL))
> + return 1;
> + if (timeout == 0)
> + break;

Please fix this loop. "while (timeout--)" means the loop will stop
when timeout reaches 0. It's redundant with "if (timeout == 0) break;"

> + }
> + return 0;
> +}
> +
> +static void dwmci_set_idma_desc(u8 *idmac, unsigned int des0,
> + unsigned int des1, unsigned int des2)
> +{
> + struct dwmci_idmac *desc = (struct dwmci_idmac *)idmac;

I don't understand why this function takes a u8* Why not just pass a
struct dwmci_idmac * ?

> +
> + desc->des0 = des0;
> + desc->des1 = des1;
> + desc->des2 = des2;
> + desc->des3 = (unsigned int)desc + sizeof(struct dwmci_idmac);

Also, is there a reason that you've decided to label the 4 fields of
your descriptor (which appear to reflect flags, count, address,
pointer to next descriptor) as des0-3?

> +}
> +
> +static void dwmci_prepare_data(struct dwmci_host *host,
> + struct mmc_data *data)
> +{
> + unsigned long ctrl;
> + unsigned int i = 0, flag, cnt, blk_cnt;
> + struct dwmci_idmac *p;
> + ulong data_start, data_end, start_addr;
> + ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, idmac, 65565);

That's a really large buffer to allocate on the stack...

> +
> + do {
> + flag = DWMCI_IDMAC_OWN | DWMCI_IDMAC_CH ;
> + flag |= (i == 0) ? DWMCI_IDMAC_FS : 0;
> + if (blk_cnt <= 8) {
> + flag |= DWMCI_IDMAC_LD;
> + cnt = data->blocksize * blk_cnt;
> + } else {
> + flag &= ~DWMCI_IDMAC_LD;

Clearing this bit will never have an effect (flag was initialized
without it set just before).

> + cnt = data->blocksize * 8;
> + }
> +
> + dwmci_set_idma_desc((u8 *)p, flag, cnt,
> + start_addr + (i * PAGE_SIZE));
> +
> + if (blk_cnt <= 8)
> + break;
> + blk_cnt -= 8;
> + p++;
> + i++;
> + } while(1);

And, again, a loop with an internal control, as opposed to just saying

} while (blk_cnt > 8)

This one may be fine, as I see you use 'p' after. However, I think it
best if you rework this loop to be a proper loop.

> +
> + data_start = (ulong)idmac;
> + data_end = (ulong)p;

I'm not 100% sure, but I think p doesn't point to where you want it,
except by "luck". You want p to point to the last byte of the
descriptor chain, not the first byte of the last descriptor, yes? I
suspect it will always work because of the ARCH_DMA_MINALIGN, but it
makes the code fragile.

> + flush_dcache_range(data_start, data_end + ARCH_DMA_MINALIGN);
> +

This cache flush is why I think 'p' is inaccurate.

> +
> + if (data) {
> + flags = dwmci_set_transfer_mode(host, data);
> + }

Don't use braces for single-line if-clauses.

[...]

> + if (data) {
> + while (1) {
> + mask = dwmci_readl(host, DWMCI_RINTSTS);
> + if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) {
> + debug("DATA ERROR!\n");
> + return -1;
> + } else if (mask & DWMCI_INTMSK_DTO)
> + break;
> + }

do {
mask = ...
} while (!(mask & DWMCI_INTMSK_DTO))

[...]

> +
> + for (div = 1; div < 255; div++) {
> + if ((sclk / (2 * div)) <= freq) {
> + break;
> + }
> + }

1) Your braces are unnecessary.
2) This is reimplementing a basic mathematical formula in loop form.
Don't do that.

Do this:
div = DIV_ROUND_UP(sclk, 2 * freq);

This solves the formula:

freq >= sclk/(2 * div) for div, choosing a large enough number to
ensure that the inequality is satisfied.

> + do {
> + status = dwmci_readl(host, DWMCI_CMD);
> + if (timeout == 0) {
> + printf("TIMEOUT error!!\n");
> + return -ETIMEDOUT;
> + }
> + } while ((status & DWMCI_CMD_START) && timeout--);

Here, you have a loop with a "timeout" control, but it never has any
effect. Personally, I would put the timeout check *after* the loop
terminates. After all, the last read could succeed, but then you'll
timeout. You'll have to modify the if clause to look for timeout being
less than 0.

> + timeout = 10000;
> + do {
> + status = dwmci_readl(host, DWMCI_CMD);
> + if (timeout == 0) {
> + printf("TIMEOUT error!!\n");
> + return -ETIMEDOUT;
> + }
> + } while ((status & DWMCI_CMD_START) && timeout--);

And again.

[...]

> + fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> + if (host->fifoth_val)
> + fifoth_val = host->fifoth_val;
> + else
> + fifoth_val = (0x2 << 28) | ((fifo_size/2 -1) << 16) |
> + ((fifo_size/2) << 0);

Please change those magic numbers to named constants. Also, there's no
point to "<< 0" in this context.

> +
> +int add_dwmci(struct dwmci_host *host, u32 max_clk, u32 min_clk, int index)

> + if (host->caps)
> + mmc->host_caps = host->caps;
> + else
> + mmc->host_caps = 0;

This can be replaced with:

mmc->host_caps = host->caps;

> +
> + if (host->buswidth == 8) {
> + mmc->host_caps |= MMC_MODE_8BIT;
> + mmc->host_caps &= ~MMC_MODE_4BIT;
> + } else {
> + mmc->host_caps |= MMC_MODE_4BIT;
> + mmc->host_caps &= ~MMC_MODE_8BIT;
> + }
> + mmc->host_caps |= MMC_MODE_HS | MMC_MODE_HS_52MHz | MMC_MODE_HC;

I don't think it's necessary to ensure that only one MODE bit is set.
If you support 8-bit, can you not also run as 4-bit?

> diff --git a/include/dwmmc.h b/include/dwmmc.h
> new file mode 100644
> index 0000000..9648586
> --- /dev/null
> +++ b/include/dwmmc.h

[...]

> +/* CLKENA register */
> +#define DWMCI_CLKEN_ENABLE (1 << 0)
> +#define DWMCI_CLKEN_LOW_PWR (1 << 16)
> +
> +/* Card-type registe */

register

[...]

> +struct dwmci_idmac {
> + u32 des0;
> + u32 des1;
> + u32 des2;
> + u32 des3;
> +};

Just as a reminder, I think it would be better to name these fields
based on their purpose.

Andy