Re: [PATCH 2/2] mmc: dw_mmc: add resets support to dw_mmc

[Jaehoon Chung <jh80.chung@samsung.com>]

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@rock-chips.com
>> <mailto:shawn.lin@rock-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@linaro.org
>>         <mailto:guodong.xu@linaro.org>>
>>         Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com
>>         <mailto:kong.kongxinwei@hisilicon.com>>
>>         Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org
>>         <mailto:zhangfei.gao@linaro.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?

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