On 2 April 2016 at 22:03, Heiko Stuebner <heiko@xxxxxxxxx> wrote: > Am Samstag, 2. April 2016, 21:39:11 schrieb Guodong Xu: >> On 2 April 2016 at 02:42, Heiko Stuebner <heiko@xxxxxxxxx> wrote: >> > Am Mittwoch, 30. März 2016, 15:24:56 schrieb Guodong Xu: >> > >> > [...] >> > >> > > @@ -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 > > >> > Making the driver handle probe deferral better should be a separate >> > patch.> >> > > dev_err(host->dev, "platform data not >> > >> > available\n"); >> > >> > > 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? > >> > > setup_timer(&host->cmd11_timer, >> > > >> > > dw_mci_cmd11_timer, (unsigned long)host); >> > >> > [...] >> > >> > > 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 -- 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