[U-Boot] [PATCH v2 1/4] mmc: dw-mmc: support DesignWare MMC Controller
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 --- 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
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c new file mode 100644 index 0000000..e634e1d --- /dev/null +++ b/drivers/mmc/dw_mmc.c @@ -0,0 +1,400 @@ +/* + * (C) Copyright 2012 SAMSUNG Electronics + * Jaehoon Chung jh80.chung@samsung.com + * Rajeshawari Shinde rajeshwari.s@samsung.com + * + * 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; +} diff --git a/include/dwmmc.h b/include/dwmmc.h new file mode 100644 index 0000000..9648586 --- /dev/null +++ b/include/dwmmc.h @@ -0,0 +1,186 @@ +/* + * (C) Copyright 2012 SAMSUNG Electronics + * Jaehoon Chung jh80.chung@samsung.com + * + * 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 + * + */ + +#ifndef __DWMMC_HW_H +#define __DWMMC_HW_H + +#include <asm/io.h> +#include <mmc.h> + +#define DWMCI_CTRL 0x000 +#define DWMCI_PWREN 0x004 +#define DWMCI_CLKDIV 0x008 +#define DWMCI_CLKSRC 0x00C +#define DWMCI_CLKENA 0x010 +#define DWMCI_TMOUT 0x014 +#define DWMCI_CTYPE 0x018 +#define DWMCI_BLKSIZ 0x01C +#define DWMCI_BYTCNT 0x020 +#define DWMCI_INTMASK 0x024 +#define DWMCI_CMDARG 0x028 +#define DWMCI_CMD 0x02C +#define DWMCI_RESP0 0x030 +#define DWMCI_RESP1 0x034 +#define DWMCI_RESP2 0x038 +#define DWMCI_RESP3 0x03C +#define DWMCI_MINTSTS 0x040 +#define DWMCI_RINTSTS 0x044 +#define DWMCI_STATUS 0x048 +#define DWMCI_FIFOTH 0x04C +#define DWMCI_CDETECT 0x050 +#define DWMCI_WRTPRT 0x054 +#define DWMCI_GPIO 0x058 +#define DWMCI_TCMCNT 0x05C +#define DWMCI_TBBCNT 0x060 +#define DWMCI_DEBNCE 0x064 +#define DWMCI_USRID 0x068 +#define DWMCI_VERID 0x06C +#define DWMCI_HCON 0x070 +#define DWMCI_UHS_REG 0x074 +#define DWMCI_BMOD 0x080 +#define DWMCI_PLDMND 0x084 +#define DWMCI_DBADDR 0x088 +#define DWMCI_IDSTS 0x08C +#define DWMCI_IDINTEN 0x090 +#define DWMCI_DSCADDR 0x094 +#define DWMCI_BUFADDR 0x098 +#define DWMCI_DATA 0x200 + +/* Interrupt Mask register */ +#define DWMCI_INTMSK_ALL 0xffffffff +#define DWMCI_INTMSK_RE (1 << 1) +#define DWMCI_INTMSK_CDONE (1 << 2) +#define DWMCI_INTMSK_DTO (1 << 3) +#define DWMCI_INTMSK_TXDR (1 << 4) +#define DWMCI_INTMSK_RXDR (1 << 5) +#define DWMCI_INTMSK_DCRC (1 << 7) +#define DWMCI_INTMSK_RTO (1 << 8) +#define DWMCI_INTMSK_DRTO (1 << 9) +#define DWMCI_INTMSK_HTO (1 << 10) +#define DWMCI_INTMSK_FRUN (1 << 11) +#define DWMCI_INTMSK_HLE (1 << 12) +#define DWMCI_INTMSK_SBE (1 << 13) +#define DWMCI_INTMSK_ACD (1 << 14) +#define DWMCI_INTMSK_EBE (1 << 15) + +/* Raw interrupt Regsiter */ +#define DWMCI_DATA_ERR (DWMCI_INTMSK_EBE | DWMCI_INTMSK_SBE | DWMCI_INTMSK_HLE |\ + DWMCI_INTMSK_FRUN | DWMCI_INTMSK_EBE | DWMCI_INTMSK_DCRC) +#define DWMCI_DATA_TOUT (DWMCI_INTMSK_HTO | DWMCI_INTMSK_DRTO) +/* CTRL register */ +#define DWMCI_CTRL_RESET (1 << 0) +#define DWMCI_CTRL_FIFO_RESET (1 << 1) +#define DWMCI_CTRL_DMA_RESET (1 << 2) +#define DWMCI_DMA_EN (1 << 5) +#define DWMCI_CTRL_SEND_AS_CCSD (1 << 10) +#define DWMCI_IDMAC_EN (1 << 25) +#define DWMCI_RESET_ALL (DWMCI_CTRL_RESET | DWMCI_CTRL_FIFO_RESET |\ + DWMCI_CTRL_DMA_RESET) + +/* CMD register */ +#define DWMCI_CMD_RESP_EXP (1 << 6) +#define DWMCI_CMD_RESP_LENGTH (1 << 7) +#define DWMCI_CMD_CHECK_CRC (1 << 8) +#define DWMCI_CMD_DATA_EXP (1 << 9) +#define DWMCI_CMD_RW (1 << 10) +#define DWMCI_CMD_SEND_STOP (1 << 12) +#define DWMCI_CMD_ABORT_STOP (1 << 14) +#define DWMCI_CMD_PRV_DAT_WAIT (1 << 13) +#define DWMCI_CMD_UPD_CLK (1 << 21) +#define DWMCI_CMD_USE_HOLD_REG (1 << 29) +#define DWMCI_CMD_START (1 << 31) + +/* CLKENA register */ +#define DWMCI_CLKEN_ENABLE (1 << 0) +#define DWMCI_CLKEN_LOW_PWR (1 << 16) + +/* Card-type registe */ +#define DWMCI_CTYPE_1BIT 0 +#define DWMCI_CTYPE_4BIT (1 << 0) +#define DWMCI_CTYPE_8BIT (1 << 16) + +/* Status Register */ +#define DWMCI_BUSY (1 << 9) + +#define DWMCI_IDMAC_OWN (1 << 31) +#define DWMCI_IDMAC_CH (1 << 4) +#define DWMCI_IDMAC_FS (1 << 3) +#define DWMCI_IDMAC_LD (1 << 2) + +/* Bus Mode Register */ +#define DWMCI_BMOD_IDMAC_RESET (1 << 0) +#define DWMCI_BMOD_IDMAC_FB (1 << 1) +#define DWMCI_BMOD_IDMAC_EN (1 << 7) + +struct dwmci_host { + char *name; + void *ioaddr; + unsigned int quirks; + unsigned int caps; + unsigned int version; + unsigned int clock; + unsigned int bus_hz; + int dev_index; + int buswidth; + u32 fifoth_val; + struct mmc *mmc; + + void (*clksel)(struct dwmci_host *host); + unsigned int (*mmc_clk)(int dev_index); +}; + +struct dwmci_idmac { + u32 des0; + u32 des1; + u32 des2; + u32 des3; +}; + +static inline void dwmci_writel(struct dwmci_host *host, int reg, u32 val) +{ + writel(val, host->ioaddr + reg); +} + +static inline void dwmci_writew(struct dwmci_host *host, int reg, u16 val) +{ + writew(val, host->ioaddr + reg); +} + +static inline void dwmci_writeb(struct dwmci_host *host, int reg, u8 val) +{ + writeb(val, host->ioaddr + reg); +} +static inline u32 dwmci_readl(struct dwmci_host *host, int reg) +{ + return readl(host->ioaddr + reg); +} + +static inline u16 dwmci_readw(struct dwmci_host *host, int reg) +{ + return readw(host->ioaddr + reg); +} + +static inline u8 dwmci_readb(struct dwmci_host *host, int reg) +{ + return readb(host->ioaddr + reg); +} + +int add_dwmci(struct dwmci_host *host, u32 max_clk, u32 min_clk, int index); +#endif /* __DWMMC_HW_H */
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;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.
+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;
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
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
Hi Jaehoon,
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,
Hi Albert,
On 10/15/2012 04:01 PM, Albert ARIBAUD wrote:
Hi Jaehoon,
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.
Thanks for your comment. I will modify that...e.g. desc->flags/cnt/cur_addr/next_addr or others.
Best Regards, Jaehoon Chung
Amicalement,
participants (3)
-
Albert ARIBAUD -
Andy Fleming -
Jaehoon Chung