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

[Jaehoon Chung]

On 03/29/2016 03:09 PM, Shawn Lin wrote:
> å 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@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?
>>
> 
> MMC_CAP_HW_RESET actually reset the mmc card, but Guodong means to
> reset the controller rather than mmc card :)

We have talked about FBE scenarios, right? :)
Now, I remembered it.

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