mmc: dw-mmc: support DesignWare MMC Controller
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
@@ -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
PATCH v2 1/4 mmc: dw-mmc: support DesignWare MMC Controller
Quoted 8 lines · Jaehoon Chung
This patch is supported DesginWare MMC Controller.
Signed-off-by Jaehoon Chung <[email protected]>Signed-off-by Kyungmin Park <[email protected]>Signed-off-by Rajeshawari Shinde <[email protected]>
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
[...]
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
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.
Signed-off-by Jaehoon Chung <[email protected]>Signed-off-by Kyungmin Park <[email protected]>Signed-off-by Rajeshawari Shinde <[email protected]>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[...]
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
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.
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_countetc.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,