Re: [PATCH v4] mmc: tmio: ensure that the clock has been stopped before set_clock

[Ulf Hansson <ulf.hansson@linaro.org>]

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