Hi Andy
On 08/31/2012 06:11 AM, Andy Fleming wrote:
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;"
Will fix
}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 * ?
Will use the 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?
In DesigneWare IP spec, descriptors are used to those label.
+}
+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...
I will use the data->blocks. didn't allocate always 65565 on the stack. (It's too large)
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).
Remove this.
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.
Will modify.
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.
Will check.
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.
Will remove.
[...]
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))
Will modify.
[...]
for (div = 1; div < 255; div++) {if ((sclk / (2 * div)) <= freq) {break;}}
- Your braces are unnecessary.
- 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.
Will use the DIV_ROUND_UP
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.
Will fix.
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;elsefifoth_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.
Will the macro and remove "<< 0".
+int add_dwmci(struct dwmci_host *host, u32 max_clk, u32 min_clk, int index)
if (host->caps)mmc->host_caps = host->caps;elsemmc->host_caps = 0;This can be replaced with:
mmc->host_caps = host->caps;
Will fix.
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?
Right, but in latest u-boot, if both MMC_MODE_8BIT and MMC_MODE_4BIT is set, when run mmcinfo, displayed the 12-bit buswidth. (I think this problem is bug in mmc.c)
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
Will fix
[...]
+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.
Sure, i will modify.
I will fix with your comments, and will post at this week.
Best Regards, Jaehoon Chung
Andy _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot