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@xxxxxxxxxxxxxx >> <mailto:shawn.lin@xxxxxxxxxxxxxx>> 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@xxxxxxxxxx >> <mailto:guodong.xu@xxxxxxxxxx>> >> Signed-off-by: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx >> <mailto:kong.kongxinwei@xxxxxxxxxxxxx>> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx >> <mailto:zhangfei.gao@xxxxxxxxxx>> >> >> >> 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? 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 >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
