| Message ID | 87bnr5ic6h.wl%kuninori.morimoto.gx@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 28 August 2014 04:24, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > This patch ensures that the clock has been stopped before > it calls tmio_mmc_set_clock(). > The clock settings might be failed without this patch > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > v3 -> v4 > > - based on latest git://git.linaro.org/people/ulf.hansson/mmc.git#next > > drivers/mmc/host/tmio_mmc_pio.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index ba45413..7289331 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -924,12 +924,14 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > tmio_mmc_clk_stop(host); > break; > case MMC_POWER_UP: > + tmio_mmc_clk_stop(host); I don't think this is needed. Let me elaborate on why. There two scenarios for when you may come down this path. Either after ->probe() or after a MMC_POWER_OFF has been handled. In both scenarios, tmio_mmc_clk_stop(host) has already been invoked. Well, actually in the ->probe() we invoke tmio_mmc_clk_stop() and then tmio_mmc_reset(), should those maybe be done in reverse order to make sure the clock is stopped? > tmio_mmc_set_clock(host, ios->clock); > tmio_mmc_power_on(host, ios->vdd); > tmio_mmc_clk_start(host); > tmio_mmc_set_bus_width(host, ios->bus_width); > break; > case MMC_POWER_ON: > + tmio_mmc_clk_stop(host); > tmio_mmc_set_clock(host, ios->clock); > tmio_mmc_clk_start(host); > tmio_mmc_set_bus_width(host, ios->bus_width); > @@ -1199,6 +1201,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev) > tmio_mmc_clk_update(host); > > if (host->clk_cache) { > + tmio_mmc_clk_stop(host); This should also not be needed. To get runtime PM resumed, you must first be runtime PM suspended. In the runtime PM suspend callback we have already invoked tmio_mmc_set_clock(). Well, again it may depend on if tmio_mmc_reset() should be followed by a tmio_mmc_clk_stop(), which isn't the case right now. > tmio_mmc_set_clock(host, host->clk_cache); > tmio_mmc_clk_start(host); > } > -- > 1.7.9.5 > Finally, another question - somewhat related to clocks but not so much to this patch. Shouldn't tmio_mmc_set_clock() when requested rate is 0, actually invoke tmio_mmc_clk_stop()? How will otherwise the clock be stopped? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 August 2014 09:11, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 28 August 2014 04:24, Kuninori Morimoto > <kuninori.morimoto.gx@gmail.com> wrote: >> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> >> This patch ensures that the clock has been stopped before >> it calls tmio_mmc_set_clock(). >> The clock settings might be failed without this patch >> >> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> --- >> v3 -> v4 >> >> - based on latest git://git.linaro.org/people/ulf.hansson/mmc.git#next >> >> drivers/mmc/host/tmio_mmc_pio.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c >> index ba45413..7289331 100644 >> --- a/drivers/mmc/host/tmio_mmc_pio.c >> +++ b/drivers/mmc/host/tmio_mmc_pio.c >> @@ -924,12 +924,14 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) >> tmio_mmc_clk_stop(host); >> break; >> case MMC_POWER_UP: >> + tmio_mmc_clk_stop(host); > > I don't think this is needed. Let me elaborate on why. > > There two scenarios for when you may come down this path. > > Either after ->probe() or after a MMC_POWER_OFF has been handled. In > both scenarios, tmio_mmc_clk_stop(host) has already been invoked. > > Well, actually in the ->probe() we invoke tmio_mmc_clk_stop() and then > tmio_mmc_reset(), should those maybe be done in reverse order to make > sure the clock is stopped? > >> tmio_mmc_set_clock(host, ios->clock); >> tmio_mmc_power_on(host, ios->vdd); >> tmio_mmc_clk_start(host); >> tmio_mmc_set_bus_width(host, ios->bus_width); >> break; >> case MMC_POWER_ON: >> + tmio_mmc_clk_stop(host); >> tmio_mmc_set_clock(host, ios->clock); >> tmio_mmc_clk_start(host); >> tmio_mmc_set_bus_width(host, ios->bus_width); >> @@ -1199,6 +1201,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev) >> tmio_mmc_clk_update(host); >> >> if (host->clk_cache) { >> + tmio_mmc_clk_stop(host); > > This should also not be needed. > > To get runtime PM resumed, you must first be runtime PM suspended. In > the runtime PM suspend callback we have already invoked > tmio_mmc_set_clock(). /s /tmio_mmc_set_clock() / tmio_mmc_clk_stop() > > Well, again it may depend on if tmio_mmc_reset() should be followed by > a tmio_mmc_clk_stop(), which isn't the case right now. > >> tmio_mmc_set_clock(host, host->clk_cache); >> tmio_mmc_clk_start(host); >> } >> -- >> 1.7.9.5 >> > > Finally, another question - somewhat related to clocks but not so much > to this patch. > > Shouldn't tmio_mmc_set_clock() when requested rate is 0, actually > invoke tmio_mmc_clk_stop()? How will otherwise the clock be stopped? > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf > > I don't think this is needed. Let me elaborate on why. > > > > There two scenarios for when you may come down this path. > > > > Either after ->probe() or after a MMC_POWER_OFF has been handled. In > > both scenarios, tmio_mmc_clk_stop(host) has already been invoked. > > > > Well, actually in the ->probe() we invoke tmio_mmc_clk_stop() and then > > tmio_mmc_reset(), should those maybe be done in reverse order to make > > sure the clock is stopped? (snip) > > To get runtime PM resumed, you must first be runtime PM suspended. In > > the runtime PM suspend callback we have already invoked > > tmio_mmc_set_clock(). I see Thank you for you detail explain about that Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ba45413..7289331 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -924,12 +924,14 @@ static void tmio_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) tmio_mmc_clk_stop(host); break; case MMC_POWER_UP: + tmio_mmc_clk_stop(host); tmio_mmc_set_clock(host, ios->clock); tmio_mmc_power_on(host, ios->vdd); tmio_mmc_clk_start(host); tmio_mmc_set_bus_width(host, ios->bus_width); break; case MMC_POWER_ON: + tmio_mmc_clk_stop(host); tmio_mmc_set_clock(host, ios->clock); tmio_mmc_clk_start(host); tmio_mmc_set_bus_width(host, ios->bus_width); @@ -1199,6 +1201,7 @@ int tmio_mmc_host_runtime_resume(struct device *dev) tmio_mmc_clk_update(host); if (host->clk_cache) { + tmio_mmc_clk_stop(host); tmio_mmc_set_clock(host, host->clk_cache); tmio_mmc_clk_start(host); }