From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaehoon Chung Subject: Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc Date: Tue, 29 Mar 2016 14:56:58 +0900 Message-ID: <56FA192A.9030209@samsung.com> 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> <56F9E6E5.9000100@kernel-upstream.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mailout2.samsung.com ([203.254.224.25]:60729 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752447AbcC2F5C convert rfc822-to-8bit (ORCPT ); Tue, 29 Mar 2016 01:57:02 -0400 In-reply-to: <56F9E6E5.9000100@kernel-upstream.org> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Shawn Lin , Guodong Xu , Shawn Lin Cc: robh+dt@kernel.org, =?UTF-8?B?UGF3ZcWCIE1vbGw=?= , Mark Rutland , ijc+devicetree@hellion.org.uk, Kumar Gala , Ulf Hansson , devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , linux-mmc@vger.kernel.org, Xinwei Kong , Zhangfei Gao On 03/29/2016 11:22 AM, Shawn Lin wrote: > =E5=9C=A8 2016/3/25 13:35, Guodong Xu =E5=86=99=E9=81=93: >> Hi, Shawn >> >> Sorry I replied late. I added comments below. >> >> On 6 March 2016 at 22:16, Shawn Lin > > wrote: >> >> On 2016/3/6 16:47, Guodong Xu wrote: >> >> mmc registers may in abnormal state if mmc is used in bootlo= ader, >> eg. to support booting from eMMC. So we need reset mmc regis= ters >> when kernel boots up, instead of assuming mmc is in clean st= ate. >> >> With this patch, user can add a 'resets' property into dw_mm= c dts >> node. When driver parse_dt and probe, it calls reset API to >> deassert the 'reset' of dw_mmc host controller. When probe e= rror or >> remove, it calls reset API to assert it. >> >> Please also refer to >> Documentation/devicetree/bindings/reset/reset.txt >> >> Signed-off-by: Guodong Xu > > >> Signed-off-by: Xinwei Kong > > >> Signed-off-by: Zhangfei Gao > > >> >> >> 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 =3D devm_reset_control_get_optional(dev,= NULL); >> + if (IS_ERR(pdata->rstc)) { >> + if (PTR_ERR(pdata->rstc) =3D=3D -EPROBE_DEFE= R) >> + return ERR_PTR(-EPROBE_DEFER); >> + pdata->rstc =3D NULL; >> >> >> maybe we can remove "pdata->rstc =3D NULL", and directly >> use IS_ERR(..) for the following "if (host->pdata->rstc !=3D NUL= L)" >> 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_s= lots); >> >> @@ -2949,7 +2957,9 @@ int dw_mci_probe(struct dw_mci *host) >> >> if (!host->pdata) { >> host->pdata =3D dw_mci_parse_dt(host); >> - if (IS_ERR(host->pdata)) { >> + if (PTR_ERR(host->pdata) =3D=3D -EPROBE_DEFE= R) >> + 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) =3D=3D -EPROBE_DEFER) { >> + return -EPROBE_DEFER; >> >> + } >> >> I will add {}. >> >> >> >> + else if (IS_ERR(host->pdata)) { >> dev_err(host->dev, "platform data n= ot >> available\n"); >> return -EINVAL; >> } >> @@ -3012,6 +3022,9 @@ int dw_mci_probe(struct dw_mci *host) >> } >> } >> >> + if (host->pdata->rstc !=3D NULL) >> + reset_control_deassert(host->pdata->rstc); >> + >> >> >> sorry, I can't follow your intention here. Shouldn't it be somet= hing >> like "assert mmc -> may need delay -> deassert mmc". As your cur= rent >> code, nothing happend right? >> >> >> The chip exits from bootloader with this bit asserted. And when ente= ring >> kernel, we only need to deassert. >> >> In my current code, the driver deassert mmc in _probe(), and assert = mmc >> in _remove(). >=20 > I catch your point. From the previous discussion, we add it to make s= ure > 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 wi= th MMC_CAP_HW_RESET. Could you check this? Best Regards, Jaehoon Chung >=20 > 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. >=20 >=20 >> >> >> >> 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 !=3D 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 *ho= st) >> if (host->use_dma && host->dma_ops->exit) >> host->dma_ops->exit(host); >> >> + if (host->pdata->rstc !=3D 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 >> >> >=20 >=20 >=20