Message-ID: <56FB2976.60205@linaro.org>
Date: Wed, 30 Mar 2016 09:18:46 +0800
From: zhangfei <zhangfei.gao@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
MIME-Version: 1.0
To: Shawn Lin <shawn.lin@rock-chips.com>, Guodong Xu <guodong.xu@linaro.org>
CC: robh+dt@kernel.org, =?UTF-8?B?UGF3ZcWCIE1vbGw=?= <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>
Subject: Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc
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> <56FB21E8.30008@rock-chips.com>
In-Reply-To: <56FB21E8.30008@rock-chips.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2518
Lines: 74


On 03/30/2016 08:46 AM, Shawn Lin wrote:
> 在 2016/3/29 16:23, zhangfei 写道:

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

Yes, have checked drivers/i2c/busses/i2c-tegra.c
         reset_control_assert(i2c_dev->rst);
         udelay(2);
         reset_control_deassert(i2c_dev->rst);

This usage looks strange, reset does not mean gpio reset, it maybe 
register accessing.
I think all these three operation should be abstracted to deassert 
function, while cover the details for sharing.

However, not doc describing the assert/deassert behavior so causing 
confusion.

>
> 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?
Yes, we can.
But firstly we should consider put in common file for sharing, otherwise 
there maybe many assert/deassert in dw_mmc-xx.c.

Guodong may send another version and wait for Jaehoon's decision.

Thanks