Subject: Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc
To: zhangfei <zhangfei.gao@linaro.org>, Guodong Xu <guodong.xu@linaro.org>
References: <1457254062-22677-1-git-send-email-guodong.xu@linaro.org>
 <1457254062-22677-2-git-send-email-guodong.xu@linaro.org>
 <56DC3BAD.8020107@rock-chips.com>
 <CAFGCpxz3GfFeduaz1Dkg0WvRJWs+GHdVu8uY0-wV=RGyRfBE=A@mail.gmail.com>
 <56F9E6E5.9000100@kernel-upstream.org> <56FA3B70.2090902@linaro.org>
Cc: shawn.lin@rock-chips.com, robh+dt@kernel.org,
        =?UTF-8?Q?Pawe=c5=82_Moll?= <pawel.moll@arm.com>,
        Mark Rutland <mark.rutland@arm.com>, ijc+devicetree@hellion.org.uk,
        Kumar Gala <galak@codeaurora.org>, jh80.chung@samsung.com,
        Ulf Hansson <ulf.hansson@linaro.org>, devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-mmc@vger.kernel.org, Xinwei Kong <kong.kongxinwei@hisilicon.com>
From: Shawn Lin <shawn.lin@rock-chips.com>
Message-ID: <56FB21E8.30008@rock-chips.com>
Date: Wed, 30 Mar 2016 08:46:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: <56FA3B70.2090902@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3664
Lines: 115

在 2016/3/29 16:23, zhangfei 写道:
>
>
> On 03/29/2016 10:22 AM, Shawn Lin wrote:
>
>>>
>>>
>>>
>>>         +               else if (IS_ERR(host->pdata)) {
>>>                                  dev_err(host->dev, "platform data not
>>>         available\n");
>>>                                  return -EINVAL;
>>>                          }
>>>         @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>                          }
>>>                  }
>>>
>>>         +       if (host->pdata->rstc != NULL)
>>>         +               reset_control_deassert(host->pdata->rstc);
>>>         +
>>>
>>>
>>>     sorry, I can't follow your intention here. Shouldn't it be something
>>>     like "assert mmc -> may need delay -> deassert mmc". As your current
>>>     code, nothing happend right?
> Should be abstracted in reset driver.
>
>>>
>>>
>>> The chip exits from bootloader with this bit asserted. And when entering
>>> kernel, we only need to deassert.
>>>
>>> In my current code, the driver deassert mmc in _probe(), and assert mmc
>>> in _remove().
>>
>> I catch your point. From the previous discussion, we add it to make sure
>> dw_mmc in good state after leaving bootloader to kernel. But My real
>> question is that you can assert it in  bootloader, so you can also
>> dessert it in bootloaer to make sure dw_mmc work fine when probing
>> in kernel. In that way, we don't need this patch?
>
> uefi does not have exit point, and kernel may not assume mmc controller
> state is always correct when boot.
> If Uefi need copy Image from mmc, mmc controller is in working state.
> When jump to kernel, uefi mmc driver can not recover itself.
> If kernel assume mmc controller state is clean, mmc will be in abnormal
> state.
> Some controller will clear itself when set clock, however, hip660 does
> not, it need special register to access.
>

yep, I understand your requirement.

>
>>
>> More to think, Is it ok to match the behaviour of bootloader stage?
>> My bootloader doesn't assert the reset pin of dw_mmc, so it seams if
>> I want to fix you issue on kernel stage, I need a new round of
>> assert->delay->deassert.
>
> The process like delay (if required) should be abstracted in reset driver.
> reset framework just export reset_control_assert/reset_control_deassert
> API.
> Unfortunately not find clear description in Documentation/.
> Suppose deassert is like start, while assert is like stop.
>

yes, no docs except dt-bindings..... So seems the usage of these two
APIs depends on the implementation of reset controller driver

> drivers/reset/core.c
> reset_control_deassert - deasserts the reset line
> reset_control_assert - asserts the reset line
>
> More example:
> drivers/mmc/host/sdhci-st.c
> drivers/mmc/host/sunxi-mmc.c
> drivers/usb/host/ohci-platform.c
> drivers/i2c/busses/i2c-mv64xxx.c

But I'm afraid I have to nack here....

Looking at these files:
drivers/dma/stm32-dma.c
drivers/net/ethernet/mediatek/mtk_eth_soc.c
drivers/devfreq/tegra-devfreq.c
drivers/crypto/rockchip/rk3288_crypto.c
drivers/thermal/rockchip_thermal.c
drivers/thermal/tegra_soctherm.c
drivers/i2c/busses/i2c-tegra.c
....

they just do the way like: assert->[delay](maybe abstracted)->deassert
I think these drivers are vendor specific, so they can do
the reset operations in consistent with the implementation of
their platforms' reset controller drivers.

But, dw_mmc is shared by many Socs. So It's not good to do it in
dw_mci_probe, otherwise you force my reset controller driver to be
implemented in the same way as yours..... Right?

How about move it to your drv_data->init callback?


>
> Thanks
>
>
>
>


-- 
Best Regards
Shawn Lin