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

[Andy Fleming]

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