On 2 April 2016 at 22:03, Heiko Stuebner [off-list ref] wrote:

Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu:

quoted

On 2 April 2016 at 02:42, Heiko Stuebner [off-list ref] wrote:

quoted

Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu:

[...]

quoted

@@ -2949,7 +2956,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;
+             } else if (IS_ERR(host->pdata)) {

how is this related to adding the reset handling?

I added this into dw_mci_parse_dt(host), and that's the first time it may
return -EPROBE_DEFER

  /* find reset controller when exist */
  pdata->rstc = devm_reset_control_get_optional(dev, NULL);

So, I added processing to this error in this patch.

ah, you're right of course

quoted

quoted

Making the driver handle probe deferral better should be a separate
patch.>

quoted

                      dev_err(host->dev, "platform data not

available\n");

quoted

                      return -EINVAL;

              }

@@ -3012,6 +3021,9 @@ int dw_mci_probe(struct dw_mci *host)

              }

      }

+     if (!IS_ERR(host->pdata->rstc))
+             reset_control_deassert(host->pdata->rstc);
+

Wouldn't reset_control_reset be better? The way it is now it would
expect
the reset to be asserted somewhere else before dw_mmc probes?

It relates to how the SoC's reset logic is like. One bit set can clear all
dw_mmc host controller registers. It doesn't need do assert then
deassert.

That's what I saw in hi6220 (it integrates three dw_mmc host controller),
drivers/reset/hisilicon/hi6220_reset.c
, which I wrote this patch for.

I just realized again that reset_control_reset is a completely separate
operation (not related to assert / deassert).

What I was originally getting at is that I don't see any assert-counterpart.
So if the reset-control is already deasserted, nothing will happen on some
designs.

For example on Rockchip SoCs the reset controller needs the signal to be
high to assert the reset and the dw_mmc part of the manual explicitly says
that the "reset_n should be asserted(active-low) for at least two clocks of
clk or cclk_in".

So I would expect something like

        reset_control_assert(reset);
        usleep_range(x, y);
        reset_control_deassert(reset);

instead of only trying to deassert the reset.

After confirmation with SoC hardware engineer, yeah, a correct _assert
action is expected. I will modify it as the above. Regarding
usleep_range(x, y) values, here is suggestion:

+               usleep_range(10, 50); /* 1/400kHz = 2.5us */

400kHz is the minimal bus speed for MMC. It stands for 2.5us per cycle.
10us is 4 cycles, and 50us is 20 cycles.

Does this setting make sense to you?

quoted

quoted

quoted

      setup_timer(&host->cmd11_timer,

                  dw_mci_cmd11_timer, (unsigned long)host);

[...]

quoted

diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 7b41c6d..b95cd84 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h

@@ -14,9 +14,10 @@

 #ifndef LINUX_MMC_DW_MMC_H
 #define LINUX_MMC_DW_MMC_H

-#include <linux/scatterlist.h>
-#include <linux/mmc/core.h>

 #include <linux/dmaengine.h>

+#include <linux/mmc/core.h>
+#include <linux/reset.h>
+#include <linux/scatterlist.h>

unrelated changed regarding the reordering of includes.

Making them in the order of alphabetic. If you dislike, I will not add.

It's Jaehoon's call and that change above is pretty small, but generally
introducing things unrelated to the change you actually want to make is not
that nice - that's what separate patches are for :-) .

Got your point. I will remove this. Make it simple.

-Guodong

Heiko