[U-Boot] [PATCH v2 1/4] mmc: dw-mmc: support DesignWare MMC Controller

[Jaehoon Chung <jh80.chung@samsung.com>]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>