[U-Boot] [PATCH v2 1/3] mmc: uniphier: add driver for UniPhier SD/MMC host controller

[Masahiro Yamada]

Hi Marek,


2016-02-10 22:45 GMT+09:00 Marek Vasut <marex at denx.de>:
> On 02/10/2016 02:28 PM, Masahiro Yamada wrote:
>> Add a driver for the on-chip SD/eMMC host controller used by
>> UniPhier SoC family.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>
> 前略 山田さん,
>
> [...]
>
>> +#include <common.h>
>> +#include <clk.h>
>> +#include <fdtdec.h>
>> +#include <mapmem.h>
>> +#include <mmc.h>
>> +#include <dm/device.h>
>> +#include <linux/compat.h>
>> +#include <linux/io.h>
>> +#include <asm/unaligned.h>
>> +#include <asm/dma-mapping.h>
>> +
>> +#define pr_err   printf
>> +#define pr_warn  printf
>> +#ifdef DEBUG
>> +#define pr_debug printf
>> +#else
>> +#define pr_debug(...)
>> +#endif
>
> This should go into include/ somewhere, your driver shouldn't be a
> platform abstraction library.

Fixed in v3.



>> +static int uniphier_sd_wait_irq(struct uniphier_sd_priv *priv,
>> +                             unsigned int reg, u32 flag)
>> +{
>> +     long wait = 1000000;
>> +     int ret;
>
> Replace this with wait_for_bit() please .


It cannot check error during the loop.

I want to check the error flags in the loop
because no reason to wait for the time-out
if some error happens.


>> +     while (!(readl(priv->regbase + reg) & flag)) {
>> +             if (wait-- < 0) {
>> +                     pr_err("timeout\n");
>> +                     return -ETIMEDOUT;
>> +             }
>> +
>> +             ret = uniphier_sd_check_error(priv);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             udelay(1);
>> +     }
>> +
>> +     return 0;
>> +}
>
> [...]
>
>> +static void uniphier_sd_dma_start(struct uniphier_sd_priv *priv,
>> +                               dma_addr_t dma_addr)
>> +{
>> +     u32 tmp;
>> +
>> +     writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO1);
>> +     writel(0, priv->regbase + UNIPHIER_SD_DMA_INFO2);
>> +
>> +     /* enable DMA */
>> +     tmp = readl(priv->regbase + UNIPHIER_SD_EXTMODE);
>> +     tmp |= UNIPHIER_SD_EXTMODE_DMA_EN;
>> +     writel(tmp, priv->regbase + UNIPHIER_SD_EXTMODE);
>
> I'd say, use setbits_le32(), but could it be that this driver is kept in
> sync with Linux ?

Yes, I am developing the MMC driver
for Linux as well as U-Boot at the same time.

This is the U-Boot counter-part,
although the Linux one has not been upstreamed yet.

It can save my time to copy-paste code snippets between the two.

I want to sync as much code as possible.




-- 
Best Regards
Masahiro Yamada