| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 29 Mar 2016 14:09:32 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Jaehoon Chung <jh80.chung@...sung.com>,
Guodong Xu <guodong.xu@...aro.org>
Cc: shawn.lin@...k-chips.com, robh+dt@...nel.org,
Paweł Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
ijc+devicetree@...lion.org.uk, Kumar Gala <galak@...eaurora.org>,
Ulf Hansson <ulf.hansson@...aro.org>,
devicetree@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-mmc@...r.kernel.org,
Xinwei Kong <kong.kongxinwei@...ilicon.com>,
Zhangfei Gao <zhangfei.gao@...aro.org>
Subject: Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc
在 2016/3/29 13:56, Jaehoon Chung 写道:
> On 03/29/2016 11:22 AM, Shawn Lin wrote:
>> 在 2016/3/25 13:35, Guodong Xu 写道:
>>> Hi, Shawn
>>>
>>> Sorry I replied late. I added comments below.
>>>
>>> On 6 March 2016 at 22:16, Shawn Lin <shawn.lin@...k-chips.com
>>> <mailto:shawn.lin@...k-chips.com>> wrote:
>>>
>>> On 2016/3/6 16:47, Guodong Xu wrote:
>>>
>>> mmc registers may in abnormal state if mmc is used in bootloader,
>>> eg. to support booting from eMMC. So we need reset mmc registers
>>> when kernel boots up, instead of assuming mmc is in clean state.
>>>
>>> With this patch, user can add a 'resets' property into dw_mmc dts
>>> node. When driver parse_dt and probe, it calls reset API to
>>> deassert the 'reset' of dw_mmc host controller. When probe error or
>>> remove, it calls reset API to assert it.
>>>
>>> Please also refer to
>>> Documentation/devicetree/bindings/reset/reset.txt
>>>
>>> Signed-off-by: Guodong Xu <guodong.xu@...aro.org
>>> <mailto:guodong.xu@...aro.org>>
>>> Signed-off-by: Xinwei Kong <kong.kongxinwei@...ilicon.com
>>> <mailto:kong.kongxinwei@...ilicon.com>>
>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@...aro.org
>>> <mailto:zhangfei.gao@...aro.org>>
>>>
>>>
>>> Really should V2 and add the changelog.
>>>
>>>
>>> Yes, will do. next version I sent will be labelled as V3.
>>>
>>>
>>>
>>> ---
>>> drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++++++++-
>>> 1 file changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 242f9a0..281ea9c 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -2878,6 +2878,14 @@ static struct dw_mci_board
>>> *dw_mci_parse_dt(struct dw_mci *host)
>>> if (!pdata)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> + /* find reset controller when exist */
>>> + pdata->rstc = devm_reset_control_get_optional(dev, NULL);
>>> + if (IS_ERR(pdata->rstc)) {
>>> + if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>> + return ERR_PTR(-EPROBE_DEFER);
>>> + pdata->rstc = NULL;
>>>
>>>
>>> maybe we can remove "pdata->rstc = NULL", and directly
>>> use IS_ERR(..) for the following "if (host->pdata->rstc != NULL)"
>>> statement
>>>
>>>
>>> Yes, will do.
>>> I see your point, other lines in this file are using IS_ERR(!..), I will
>>> use this style too.
>>>
>>> + }
>>> +
>>> /* find out number of slots supported */
>>> of_property_read_u32(np, "num-slots", &pdata->num_slots);
>>>
>>> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host)
>>>
>>> if (!host->pdata) {
>>> host->pdata = dw_mci_parse_dt(host);
>>> - if (IS_ERR(host->pdata)) {
>>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>>
>>>
>>> please fix the coding style here.
>>>
>>>
>>> Do you mean to add additional {} for this 'if' , like this?
>>>
>>> + if (PTR_ERR(host->pdata) == -EPROBE_DEFER) {
>>> + return -EPROBE_DEFER;
>>>
>>> + }
>>>
>>> I will add {}.
>>>
>>>
>>>
>>> + 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?
>>>
>>>
>>> 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?
>
> Doesn't dw_mci_hw_reset work fine? I think that card should be reset with MMC_CAP_HW_RESET.
> Could you check this?
>
MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to
reset the controller rather than mmc card :)
> Best Regards,
> Jaehoon Chung
>
>>
>> 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.
>>
>>
>>>
>>>
>>>
>>> setup_timer(&host->cmd11_timer,
>>> dw_mci_cmd11_timer, (unsigned long)host);
>>>
>>> @@ -3164,6 +3177,9 @@ err_dmaunmap:
>>> if (host->use_dma && host->dma_ops->exit)
>>> host->dma_ops->exit(host);
>>>
>>> + if (host->pdata->rstc != NULL)
>>> + reset_control_assert(host->pdata->rstc);
>>> +
>>> err_clk_ciu:
>>> if (!IS_ERR(host->ciu_clk))
>>> clk_disable_unprepare(host->ciu_clk);
>>> @@ -3196,11 +3212,15 @@ void dw_mci_remove(struct dw_mci *host)
>>> if (host->use_dma && host->dma_ops->exit)
>>> host->dma_ops->exit(host);
>>>
>>> + if (host->pdata->rstc != NULL)
>>> + reset_control_assert(host->pdata->rstc);
>>> +
>>> if (!IS_ERR(host->ciu_clk))
>>> clk_disable_unprepare(host->ciu_clk);
>>>
>>> if (!IS_ERR(host->biu_clk))
>>> clk_disable_unprepare(host->biu_clk);
>>> +
>>> }
>>>
>>>
>>> unnecessary new line here.
>>>
>>>
>>> Will fix.
>>>
>>> -Guodong
>>>
>>>
>>> EXPORT_SYMBOL(dw_mci_remove);
>>>
>>>
>>>
>>>
>>> --
>>> Best Regards
>>> Shawn Lin
>>>
>>>
>>
>>
>>
>
>
>
>
--
Best Regards
Shawn Lin
Powered by blists - more mailing lists