Hi Andy
On 08/31/2012 06:11 AM, Andy Fleming wrote:
> On Tue, Jul 3, 2012 at 12:58 AM, Jaehoon Chung <jh80.ch...@samsung.com> wrote:
>> This patch is supported DesginWare MMC Controller.
>>
>> Signed-off-by: Jaehoon Chung <jh80.ch...@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
>> Signed-off-by: Rajeshawari Shinde <rajeshwar...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
