From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Tue, 16 Feb 2016 13:51:38 +0100
Subject: [U-Boot] [PATCH v2 1/3] mmc: uniphier: add driver for UniPhier
 SD/MMC host controller
In-Reply-To: <CAK7LNAS_Yy3UZRZ1cdCCrOk7ZO-dR0ajC3aNqqJk+Lo-vgQ97g@mail.gmail.com>
References: <1455110911-12224-1-git-send-email-yamada.masahiro@socionext.com>	<1455110911-12224-2-git-send-email-yamada.masahiro@socionext.com>	<56BB3F0C.2040002@denx.de>
 <CAK7LNAS_Yy3UZRZ1cdCCrOk7ZO-dR0ajC3aNqqJk+Lo-vgQ97g@mail.gmail.com>
Message-ID: <56C31B5A.60902@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 02/16/2016 09:18 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi!

[...]

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

You have a point there.

I wonder if it'd make sense to extend the wait_for_bit with some
callback maybe ?

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

Understood, thanks!