Re: [U-Boot] [PATCH v5 13/21] mmc: dw_mmc: support fifo mode in dwc mmc driver

+Pantelis (mmc maintainer)

Hi Lin,

On 10 November 2015 at 03:24, Lin Huang hl@rock-chips.com wrote:

some soc(rk3036 etc) use dw_mmc but do not have internal dma, so we implement fifo mode to read and write data.

Signed-off-by: Lin Huang hl@rock-chips.com

drivers/mmc/dw_mmc.c | 81 +++++++++++++++++++++++++++++++++++++++++++--------- include/dwmmc.h | 5 ++++ 2 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 26d34ae..18be055 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -118,6 +118,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, u32 mask, ctrl; ulong start = get_timer(0); struct bounce_buffer bbstate;

•   unsigned int fifo_len, fifo_depth, size;
  

•   unsigned int *buf = NULL;
  
    while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
            if (get_timer(start) > timeout) {
  

@@ -129,17 +131,28 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, dwmci_writel(host, DWMCI_RINTSTS, DWMCI_INTMSK_ALL);

    if (data) {

•           if (data->flags == MMC_DATA_READ) {
  

•                   bounce_buffer_start(&bbstate, (void*)data->dest,
  

•                                       data->blocksize *
  

•                                       data->blocks, GEN_BB_WRITE);
  

•           if (host->fifo_mode) {
  

•                   if (data->flags == MMC_DATA_READ)
  

•                           buf = (unsigned int *)data->dest;
  

•                   else
  

•                           buf = (unsigned int *)data->src;
  

•                   dwmci_writel(host, DWMCI_BLKSIZ, data->blocksize);
  

•                   dwmci_writel(host, DWMCI_BYTCNT,
  

•                                data->blocksize * data->blocks);
  

•                   dwmci_wait_reset(host, DWMCI_CTRL_FIFO_RESET);
            } else {
  

•                   bounce_buffer_start(&bbstate, (void*)data->src,
  

•                                       data->blocksize *
  

•                                       data->blocks, GEN_BB_READ);
  

•                   if (data->flags == MMC_DATA_READ) {
  

•                           bounce_buffer_start(&bbstate, (void*)data->dest,
  

•                                           data->blocksize *
  

•                                           data->blocks, GEN_BB_WRITE);
  

•                   } else {
  

•                           bounce_buffer_start(&bbstate, (void*)data->src,
  

•                                           data->blocksize *
  

•                                           data->blocks, GEN_BB_READ);
  

•                   }
  

•                   dwmci_prepare_data(host, data, cur_idmac,
  

•                                      bbstate.bounce_buffer);
            }
  

•           dwmci_prepare_data(host, data, cur_idmac,
  

•                              bbstate.bounce_buffer);
    }
  
    dwmci_writel(host, DWMCI_CMDARG, cmd->cmdarg);
  

@@ -215,6 +228,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, if (data) { start = get_timer(0); timeout = 240000;

•           size = data->blocksize * data->blocks / 4;
            for (;;) {
                    mask = dwmci_readl(host, DWMCI_RINTSTS);
                    /* Error during data transfer. */
  

@@ -224,6 +238,44 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, break; }

This looks too long to me. Can you first add a patch to move everything inside the 'if (data)' block to a separate static function?

•                   /* only fifo mode need it */
  

•                   if (data->flags == MMC_DATA_READ && host->fifo_mode) {
  

I think both of these blocks should be in the same 'if (host->fifo_mode)'

In fact, can it be 'if (host->fifo_mode && size)' ?

•                           if ((dwmci_readl(host, DWMCI_RINTSTS) &&
  

•                                DWMCI_INTMSK_RXDR) && size) {
  

•                                   fifo_len = dwmci_readl(host,
  

•                                                          DWMCI_STATUS);
  

•                                   fifo_len = (fifo_len >> DWMCI_FIFO_SHIFT)
  

•                                               & DWMCI_FIFO_MASK;
  

•                                   for (i = 0; i < fifo_len; i++)
  

•                                           *buf++ = dwmci_readl(host,
  

•                                                           DWMCI_DATA);
  

•                                   dwmci_writel(host, DWMCI_RINTSTS,
  

•                                                DWMCI_INTMSK_RXDR);
  

•                                   size = size > fifo_len ?
  

•                                                   (size - fifo_len) : 0;
  

Then the above line can be common at the end of the if ()/

•                           }
  

•                   } else if (data->flags == MMC_DATA_WRITE &&
  

•                              host->fifo_mode) {
  

•                           fifo_depth = (((host->fifoth_val &
  

•                                           RX_WMARK_MASK) >>
  

•                                           RX_WMARK_SHIFT) + 1) * 2;
  

•                           if ((dwmci_readl(host, DWMCI_RINTSTS) &&
  

•                                DWMCI_INTMSK_TXDR) && size) {
  

•                                   fifo_len = dwmci_readl(host,
  

•                                                   DWMCI_STATUS);
  

•                                   fifo_len = fifo_depth -
  

•                                           ((fifo_len >> DWMCI_FIFO_SHIFT)
  

•                                           & DWMCI_FIFO_MASK);
  

•                                   for (i = 0; i < fifo_len; i++)
  

•                                           dwmci_writel(host, DWMCI_DATA,
  

•                                                        *buf++);
  

•                                   dwmci_writel(host, DWMCI_RINTSTS,
  

•                                                DWMCI_INTMSK_TXDR);
  

•                                   size = size > fifo_len ?
  

•                                                   (size - fifo_len) : 0;
  

•                           }
  

•                   }
  

•                   /* Data arrived correctly. */
                    if (mask & DWMCI_INTMSK_DTO) {
                            ret = 0;
  

@@ -241,11 +293,12 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,

            dwmci_writel(host, DWMCI_RINTSTS, mask);

•           ctrl = dwmci_readl(host, DWMCI_CTRL);
  

•           ctrl &= ~(DWMCI_DMA_EN);
  

•           dwmci_writel(host, DWMCI_CTRL, ctrl);
  

•           bounce_buffer_stop(&bbstate);
  

•           if (!host->fifo_mode) {
  

•                   ctrl = dwmci_readl(host, DWMCI_CTRL);
  

•                   ctrl &= ~(DWMCI_DMA_EN);
  

•                   dwmci_writel(host, DWMCI_CTRL, ctrl);
  

•                   bounce_buffer_stop(&bbstate);
  

•           }
    }
  
    udelay(100);
  

diff --git a/include/dwmmc.h b/include/dwmmc.h index 25cf42c..8d26e5e 100644 --- a/include/dwmmc.h +++ b/include/dwmmc.h @@ -105,6 +105,8 @@

/* Status Register */ #define DWMCI_BUSY (1 << 9) +#define DWMCI_FIFO_MASK 0x1ff +#define DWMCI_FIFO_SHIFT 17

/* FIFOTH Register */ #define MSIZE(x) ((x) << 28) @@ -180,6 +182,9 @@ struct dwmci_host { unsigned int (*get_mmc_clk)(struct dwmci_host *host, uint freq);

    struct mmc_config cfg;

•   /* use fifo mode to read and write data */
  

•   u32 fifo_mode;
  

Can this be bool?

};

struct dwmci_idmac {

1.9.1

Regards, Simon