[U-Boot,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

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;
> +       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.


> +
> +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;


> +
> +       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;
>> +               }
>> +       }
> 
> 
> 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.
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;
>> +       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.
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;
>> +       else
>> +               mmc->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,
>