On 03/29/2016 03:09 PM, Shawn Lin wrote:

在 2016/3/29 13:56, Jaehoon Chung 写道:

quoted

On 03/29/2016 11:22 AM, Shawn Lin wrote:

quoted

在 2016/3/25 13:35, Guodong Xu 写道:

quoted

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?

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.

quoted

Best Regards,
Jaehoon Chung

quoted

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.

quoted

                  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