Skip to contentSkip to conversation

mmc: dw-mmc: support DesignWare MMC Controller

PATCH Superseded Jaehoon Chung 3 participants 5 messages Jul 3, 2012 – Oct 15, 2012 MMC, SD and SDIO

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller

This patch is supported DesginWare MMC Controller.

---
 drivers/mmc/dw_mmc.c |  400 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/dwmmc.h      |  186 +++++++++++++++++++++++
 2 files changed, 586 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mmc/dw_mmc.c
 create mode 100644 include/dwmmc.h
drivers/mmc/dw_mmc.c new file +400 -0
new file mode 100644
index 0000000..e634e1d
@@ -0,0 +1,400 @@
+/*
+ * (C) Copyright 2012 SAMSUNG Electronics
+ * Jaehoon Chung <[email protected]>
+ * Rajeshawari Shinde <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,  MA 02111-1307 USA
+ *
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <mmc.h>
+#include <dwmmc.h>
+#include <asm/arch/clk.h>
+#include <asm-generic/errno.h>
+
+#define PAGE_SIZE 4096
+
+static int dwmci_wait_reset(struct dwmci_host *host, u32 value)
+{
+	unsigned long timeout = 1000;
+	u32 ctrl;
+
+	dwmci_writel(host, DWMCI_CTRL, value);
+
+	while (timeout--) {
+		ctrl = dwmci_readl(host, DWMCI_CTRL);
+		if (!(ctrl & DWMCI_RESET_ALL))
+			return 1;
+		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;
+
+	desc->des0 = des0;
+	desc->des1 = des1;
+	desc->des2 = des2;
+	desc->des3 = (unsigned int)desc + sizeof(struct dwmci_idmac);
+}
+
+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);
+
+
+	p = idmac;
+	blk_cnt = data->blocks;
+
+	dwmci_wait_reset(host, DWMCI_CTRL_FIFO_RESET);
+
+	if (data->flags == MMC_DATA_READ)
+		start_addr = (unsigned int)data->dest;
+	else
+		start_addr = (unsigned int)data->src;
+
+	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;
+			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);
+
+	data_start = (ulong)idmac;
+	data_end = (ulong)p;
+	flush_dcache_range(data_start, data_end + ARCH_DMA_MINALIGN);
+
+	dwmci_writel(host, DWMCI_DBADDR, (unsigned int)(idmac));
+
+	ctrl = dwmci_readl(host, DWMCI_CTRL);
+	ctrl |= DWMCI_IDMAC_EN | DWMCI_DMA_EN;
+	dwmci_writel(host, DWMCI_CTRL, ctrl);
+
+	ctrl = dwmci_readl(host, DWMCI_BMOD);
+	ctrl |= DWMCI_BMOD_IDMAC_FB | DWMCI_BMOD_IDMAC_EN;
+	dwmci_writel(host, DWMCI_BMOD, ctrl);
+
+	dwmci_writel(host, DWMCI_BLKSIZ, data->blocksize);
+	dwmci_writel(host, DWMCI_BYTCNT, data->blocksize * data->blocks);
+}
+
+static int dwmci_set_transfer_mode(struct dwmci_host *host,
+		struct mmc_data *data)
+{
+	unsigned long mode;
+
+	mode = DWMCI_CMD_DATA_EXP;
+	if (data->flags & MMC_DATA_WRITE)
+		mode |= DWMCI_CMD_RW;
+
+	return mode;
+}
+
+static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
+		struct mmc_data *data)
+{
+	struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
+	int flags = 0, i;
+	unsigned int timeout = 100000;
+	u32 retry = 10000;
+	u32 mask, ctrl;
+
+	while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
+		if (timeout == 0) {
+			printf("Timeout on data busy\n");
+			return TIMEOUT;
+		}
+		timeout--;
+	}
+
+	dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_ALL);
+
+	if (data)
+		dwmci_prepare_data(host, data);
+
+
+	dwmci_writel(host, DWMCI_CMDARG, cmd->cmdarg);
+
+	if (data) {
+		flags = dwmci_set_transfer_mode(host, data);
+	}
+
+	if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
+		return -1;
+
+	if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
+		flags |= DWMCI_CMD_ABORT_STOP;
+	else
+		flags |= DWMCI_CMD_PRV_DAT_WAIT;
+
+	if (cmd->resp_type & MMC_RSP_PRESENT) {
+		flags |= DWMCI_CMD_RESP_EXP;
+		if (cmd->resp_type & MMC_RSP_136)
+			flags |= DWMCI_CMD_RESP_LENGTH;
+	}
+
+	if (cmd->resp_type & MMC_RSP_CRC)
+		flags |= DWMCI_CMD_CHECK_CRC;
+
+	flags |= (cmd->cmdidx | DWMCI_CMD_START | DWMCI_CMD_USE_HOLD_REG);
+
+	debug("Sending CMD%d\n",cmd->cmdidx);
+
+	dwmci_writel(host, DWMCI_CMD, flags);
+
+	for (i = 0; i < retry; i++) {
+		mask = dwmci_readl(host, DWMCI_RINTSTS);
+		if (mask & DWMCI_INTMSK_CDONE) {
+			if (!data)
+				dwmci_writel(host, DWMCI_RINTSTS, mask);
+			break;
+		}
+	}
+
+	if (i == retry)
+		return TIMEOUT;
+
+	if (mask & DWMCI_INTMSK_RTO) {
+		debug("Response Timeout..\n");
+		return TIMEOUT;
+	} else if (mask & DWMCI_INTMSK_RE) {
+		debug("Response Error..\n");
+		return -1;
+	}
+
+
+	if (cmd->resp_type & MMC_RSP_PRESENT) {
+		if (cmd->resp_type & MMC_RSP_136) {
+			cmd->response[0] = dwmci_readl(host, DWMCI_RESP3);
+			cmd->response[1] = dwmci_readl(host, DWMCI_RESP2);
+			cmd->response[2] = dwmci_readl(host, DWMCI_RESP1);
+			cmd->response[3] = dwmci_readl(host, DWMCI_RESP0);
+		} else {
+			cmd->response[0] = dwmci_readl(host, DWMCI_RESP0);
+		}
+	}
+
+	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;
+		}
+
+		dwmci_writel(host, DWMCI_RINTSTS, mask);
+
+		ctrl = dwmci_readl(host, DWMCI_CTRL);
+		ctrl &= ~(DWMCI_DMA_EN);
+		dwmci_writel(host, DWMCI_CTRL, ctrl);
+	}
+
+	udelay(100);
+
+	return 0;
+}
+
+static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
+{
+	u32 div, status;
+	int timeout = 10000;
+	unsigned long sclk;
+
+	if (freq == host->clock)
+		return 0;
+
+	/*
+	 * If host->mmc_clk didn't define,
+	 * then assume that host->bus_hz is source clock value.
+	 * host->bus_hz should be set from user.
+	 */
+	if (host->mmc_clk)
+		sclk = host->mmc_clk(host->dev_index);
+	else if (host->bus_hz)
+		sclk = host->bus_hz;
+	else {
+		printf("Didn't get source clock value..\n");
+		return -EINVAL;
+	}
+
+	for (div = 1; div < 255; div++) {
+		if ((sclk / (2 * div)) <= freq) {
+			break;
+		}
+	}
+
+	dwmci_writel(host, DWMCI_CLKENA, 0);
+	dwmci_writel(host, DWMCI_CLKSRC, 0);
+
+	dwmci_writel(host, DWMCI_CLKDIV, div);
+	dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT |
+			DWMCI_CMD_UPD_CLK | DWMCI_CMD_START);
+
+	do {
+		status = dwmci_readl(host, DWMCI_CMD);
+		if (timeout == 0) {
+			printf("TIMEOUT error!!\n");
+			return -ETIMEDOUT;
+		}
+	} while ((status & DWMCI_CMD_START) && timeout--);
+
+	dwmci_writel(host, DWMCI_CLKENA, DWMCI_CLKEN_ENABLE |
+			DWMCI_CLKEN_LOW_PWR);
+
+	dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT |
+			DWMCI_CMD_UPD_CLK | DWMCI_CMD_START);
+
+	timeout = 10000;
+	do {
+		status = dwmci_readl(host, DWMCI_CMD);
+		if (timeout == 0) {
+			printf("TIMEOUT error!!\n");
+			return -ETIMEDOUT;
+		}
+	} while ((status & DWMCI_CMD_START) && timeout--);
+
+	host->clock = freq;
+
+	return 0;
+}
+
+static void dwmci_set_ios(struct mmc *mmc)
+{
+	struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
+	u32 ctype;
+
+	debug("Buswidth = %d, clock: %d\n",mmc->bus_width, mmc->clock);
+
+	dwmci_setup_bus(host, mmc->clock);
+	switch (mmc->bus_width) {
+	case 8:
+		ctype = DWMCI_CTYPE_8BIT;
+		break;
+	case 4:
+		ctype = DWMCI_CTYPE_4BIT;
+		break;
+	default:
+		ctype = DWMCI_CTYPE_1BIT;
+		break;
+	}
+
+	dwmci_writel(host, DWMCI_CTYPE, ctype);
+
+	if (host->clksel)
+		host->clksel(host);
+}
+
+static int dwmci_init(struct mmc *mmc)
+{
+	struct dwmci_host *host = (struct dwmci_host *)mmc->priv;
+	u32 fifo_size, fifoth_val;
+
+	dwmci_writel(host, DWMCI_PWREN, 1);
+
+	if (!dwmci_wait_reset(host, DWMCI_RESET_ALL)) {
+		debug("%s[%d] Fail-reset!!\n",__func__,__LINE__);
+		return -1;
+	}
+
+	dwmci_writel(host, DWMCI_RINTSTS, 0xFFFFFFFF);
+	dwmci_writel(host, DWMCI_INTMASK, 0);
+
+	dwmci_writel(host, DWMCI_TMOUT, 0xFFFFFFFF);
+
+	dwmci_writel(host, DWMCI_IDINTEN, 0);
+	dwmci_writel(host, DWMCI_BMOD, 1);
+
+	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);
+	dwmci_writel(host, DWMCI_FIFOTH, fifoth_val);
+
+	dwmci_writel(host, DWMCI_CLKENA, 0);
+	dwmci_writel(host, DWMCI_CLKSRC, 0);
+
+	return 0;
+}
+
+int add_dwmci(struct dwmci_host *host, u32 max_clk, u32 min_clk, int index)
+{
+	struct mmc *mmc;
+	int err = 0;
+
+	mmc = malloc(sizeof(struct mmc));
+	if (!mmc) {
+		printf("mmc malloc fail!\n");
+		return -1;
+	}
+
+	mmc->priv = host;
+	host->mmc = mmc;
+
+	sprintf(mmc->name, "%s", host->name);
+	mmc->send_cmd = dwmci_send_cmd;
+	mmc->set_ios = dwmci_set_ios;
+	mmc->init = dwmci_init;
+	mmc->f_min = min_clk;
+	mmc->f_max = max_clk;
+
+	mmc->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 | MMC_VDD_165_195;
+
+	if (host->caps)
+		mmc->host_caps = host->caps;
+	else
+		mmc->host_caps = 0;
+
+	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;
+
+	err = mmc_register(mmc);
+
+	return err;
+}
include/dwmmc.h new file +186 -0

186 lines not on this page · raw message

#1 lore raw

PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller

Quoted 8 lines · Jaehoon Chung

This patch is supported DesginWare MMC Controller.

drivers/mmc/dw_mmc.c line 43

Quoted 7 lines
drivers/mmc/dw_mmc.c +6 -0
 
+       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;"

drivers/mmc/dw_mmc.c line 51

Quoted 8 lines
drivers/mmc/dw_mmc.c +8 -0
+       }
+       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 * ?

drivers/mmc/dw_mmc.c line 56

Quoted 5 lines
drivers/mmc/dw_mmc.c +5 -0
+
+       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?

drivers/mmc/dw_mmc.c line 66

Quoted 10 lines
drivers/mmc/dw_mmc.c +10 -0
+}
+
+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...

drivers/mmc/dw_mmc.c line 86

Quoted 9 lines
drivers/mmc/dw_mmc.c +9 -0
+
+       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).

drivers/mmc/dw_mmc.c line 98

Quoted 12 lines
drivers/mmc/dw_mmc.c +12 -0
+                       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.

drivers/mmc/dw_mmc.c line 101

Quoted 3 lines
drivers/mmc/dw_mmc.c +3 -0
+
+       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.

drivers/mmc/dw_mmc.c

Quoted 2 lines
drivers/mmc/dw_mmc.c +2 -0
+       flush_dcache_range(data_start, data_end + ARCH_DMA_MINALIGN);
+

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

drivers/mmc/dw_mmc.c line 157

Quoted 4 lines
drivers/mmc/dw_mmc.c +4 -0
+
+       if (data) {
+               flags = dwmci_set_transfer_mode(host, data);
+       }

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

[...]

drivers/mmc/dw_mmc.c line 222

Quoted 9 lines
drivers/mmc/dw_mmc.c +9 -0
+       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))

[...]

drivers/mmc/dw_mmc.c line 263

Quoted 6 lines
drivers/mmc/dw_mmc.c +6 -0
+
+       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.

drivers/mmc/dw_mmc.c line 278

Quoted 7 lines
drivers/mmc/dw_mmc.c +7 -0
+       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.

drivers/mmc/dw_mmc.c line 293

Quoted 8 lines
drivers/mmc/dw_mmc.c +8 -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.

[...]

drivers/mmc/dw_mmc.c line 351

Quoted 6 lines
drivers/mmc/dw_mmc.c +6 -0
+       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.

drivers/mmc/dw_mmc.c line 386

Quoted 8 lines
drivers/mmc/dw_mmc.c +6 -0
+
+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;

drivers/mmc/dw_mmc.c line 395

Quoted 9 lines
drivers/mmc/dw_mmc.c +9 -0
+
+       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?

Quoted 5 lines
include/dwmmc.h new file +0 -0
new file mode 100644
index 0000000..9648586

[...]

include/dwmmc.h line 114

Quoted 5 lines
include/dwmmc.h new file +5 -0
+/* CLKENA register */
+#define DWMCI_CLKEN_ENABLE     (1 << 0)
+#define DWMCI_CLKEN_LOW_PWR    (1 << 16)
+
+/* Card-type registe */

register

[...]

include/dwmmc.h line 154

Quoted 6 lines
include/dwmmc.h new file +6 -0
+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

#2 lore raw

PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller

Hi Andy

Quoted 18 lines · Andy Fleming
Quoted 8 lines · Jaehoon Chung

This patch is supported DesginWare MMC Controller.

drivers/mmc/dw_mmc.c line 43

Quoted 7 lines
drivers/mmc/dw_mmc.c +6 -0
 
+       while (timeout--) {
+               ctrl = dwmci_readl(host, DWMCI_CTRL);
+               if (!(ctrl & DWMCI_RESET_ALL))
+                       return 1;
+               if (timeout == 0)
+                       break;
Quoted 2 lines

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

Quoted 14 lines

drivers/mmc/dw_mmc.c line 51

Quoted 10 lines
drivers/mmc/dw_mmc.c +8 -0
+       }
+       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.

Quoted 9 lines

drivers/mmc/dw_mmc.c line 56

Quoted 7 lines
drivers/mmc/dw_mmc.c +5 -0
+
+       desc->des0 = des0;
+       desc->des1 = des1;
+       desc->des2 = des2;
+       desc->des3 = (unsigned int)desc + sizeof(struct dwmci_idmac);
Quoted 3 lines

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.

Quoted 15 lines

drivers/mmc/dw_mmc.c line 66

Quoted 12 lines
drivers/mmc/dw_mmc.c +10 -0
+}
+
+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)

Quoted 14 lines

drivers/mmc/dw_mmc.c line 86

Quoted 12 lines
drivers/mmc/dw_mmc.c +9 -0
+
+       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;
Quoted 2 lines

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

Remove this.

Quoted 18 lines
Quoted 4 lines
drivers/mmc/dw_mmc.c +2 -0
+                       cnt = data->blocksize * 8;
+               }

drivers/mmc/dw_mmc.c line 98

Quoted 10 lines
drivers/mmc/dw_mmc.c +10 -0
+
+               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

Quoted 4 lines

} 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.

Quoted 7 lines

drivers/mmc/dw_mmc.c line 101

Quoted 5 lines
drivers/mmc/dw_mmc.c +3 -0
+
+       data_start = (ulong)idmac;
+       data_end = (ulong)p;
Quoted 5 lines

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.

Quoted 9 lines

drivers/mmc/dw_mmc.c

Quoted 4 lines
drivers/mmc/dw_mmc.c +2 -0
+       flush_dcache_range(data_start, data_end + ARCH_DMA_MINALIGN);
+

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

Quoted 7 lines

drivers/mmc/dw_mmc.c line 157

Quoted 4 lines
drivers/mmc/dw_mmc.c +4 -0
+
+       if (data) {
+               flags = dwmci_set_transfer_mode(host, data);
+       }

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

Will remove.

Quoted 15 lines

[...]

drivers/mmc/dw_mmc.c line 222

Quoted 9 lines
drivers/mmc/dw_mmc.c +9 -0
+       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;
+               }
Quoted 3 lines
do {
   mask = ...
} while (!(mask & DWMCI_INTMSK_DTO))

Will modify.

Quoted 17 lines

[...]

drivers/mmc/dw_mmc.c line 263

Quoted 6 lines
drivers/mmc/dw_mmc.c +6 -0
+
+       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.

Quoted 7 lines

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

Quoted 11 lines

drivers/mmc/dw_mmc.c line 278

Quoted 9 lines
drivers/mmc/dw_mmc.c +7 -0
+       do {
+               status = dwmci_readl(host, DWMCI_CMD);
+               if (timeout == 0) {
+                       printf("TIMEOUT error!!\n");
+                       return -ETIMEDOUT;
+               }
+       } while ((status & DWMCI_CMD_START) && timeout--);
Quoted 5 lines

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.

Quoted 26 lines

drivers/mmc/dw_mmc.c line 293

Quoted 11 lines
drivers/mmc/dw_mmc.c +8 -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.

[...]

drivers/mmc/dw_mmc.c line 351

Quoted 6 lines
drivers/mmc/dw_mmc.c +6 -0
+       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);
Quoted 2 lines

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

Will the macro and remove "<< 0".

Quoted 12 lines

drivers/mmc/dw_mmc.c line 386

Quoted 10 lines
drivers/mmc/dw_mmc.c +6 -0
+
+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;
Quoted 3 lines

This can be replaced with:

mmc->host_caps = host->caps;

Will fix.

Quoted 13 lines

drivers/mmc/dw_mmc.c line 395

Quoted 11 lines
drivers/mmc/dw_mmc.c +9 -0
+
+       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;
Quoted 2 lines

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)

Quoted 11 lines
Quoted 8 lines
include/dwmmc.h new file +0 -0
new file mode 100644
index 0000000..9648586

[...]

Quoted 7 lines

include/dwmmc.h line 114

Quoted 5 lines
include/dwmmc.h new file +5 -0
+/* CLKENA register */
+#define DWMCI_CLKEN_ENABLE     (1 << 0)
+#define DWMCI_CLKEN_LOW_PWR    (1 << 16)
+
+/* Card-type registe */

register

Will fix

Quoted 4 lines

[...]

Quoted 10 lines

include/dwmmc.h line 154

Quoted 6 lines
include/dwmmc.h new file +6 -0
+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

Quoted 7 lines

Andy
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

#3 lore raw

PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller

Hi Jaehoon,

Quoted 10 lines
Quoted 9 lines

drivers/mmc/dw_mmc.c line 56

Quoted 4 lines
drivers/mmc/dw_mmc.c new file +4 -0
+       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.

Makes the code pretty cryptic. Better name the struct membder by their function and comment with their IP spec designations, or maybe name them by both their IP name and function, e.g. desc->des0_flags, desc->des1_count etc.

But in any case, local variables (des0, des1...) have zero reason to be named after the IP spec. These must be renamed according to their function.

Amicalement,

Signature · 1 line
Albert.
#4 lore raw

PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller

Hi Albert,

Quoted 13 lines · Albert ARIBAUD

Hi Jaehoon,

Quoted 10 lines
Quoted 9 lines

drivers/mmc/dw_mmc.c line 56

Quoted 4 lines
drivers/mmc/dw_mmc.c new file +4 -0
+       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.

Quoted 8 lines

Makes the code pretty cryptic. Better name the struct membder by their function and comment with their IP spec designations, or maybe name them by both their IP name and function, e.g. desc->des0_flags, desc->des1_count etc.

But in any case, local variables (des0, des1...) have zero reason to be named after the IP spec. These must be renamed according to their function.

Thanks for your comment.
I will modify that...e.g. desc->flags/cnt/cur_addr/next_addr or others.

Best Regards,
Jaehoon Chung

Quoted 3 lines

Amicalement,