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

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