[...]
>>
>> Thanks a lot for elaborating! I understand your concern now.
>>
>> In this context, I believe it's important to state that I think these
>> host driver behaves badly! _All_ host drivers shall handle their
>> "power_up" things while they get MMC_POWER_UP through the ->set_ios()
>> callback. They shouldn't be taking short-cuts and ignore the
>> MMC_POWER_UP state, that also includes those host drivers which are
>> able to handle the power sequence by help from it's controller
>> hardware.
>
> You can't solve this in this way. Let's say that you decide to force
> the inteligent controllers to power up and enable clocks in the
> POWER_UP stage:
>
> pwrseq power sequence host action
>
> power_up no power applied
> POWER_UP host applies power to the card, waits,
> and applies the clock
>
> power_on power and clock is applied
> POWER_ON host does nothing
>
> So now, you have a variability for the power_on() callback, where with
> dumb hosts, the clock is not enabled, but for more inteligent hosts,
> the clock has already enabled, and likely we have already waited the
> 74 clocks necessary in the MMC protocol.
I understand, so I was not thinking of fixing the actual problem but
more to make the behaviour consistent.
>
>> I did quick research and found that the following host's ->set_ios()
>> callbacks need to be fixed in this regards.
>> au1xmmc_set_ios()
>> cb710_mmc_set_ios()
>> omap_hsmmc_set_ios()
>> sdricoh_set_ios()
>> __toshsd_set_ios()
>
> Rather than fixing them, how about introducing a new method for host
> controllers:
>
> power_up()
>
> which can be pointed to a library function which makes the ->set_ios
> calls as the current mmc_power_up() function does for those dumb
> controllers, but for the more inteligent controllers, just invokes
> their power up sequence and returns when that is complete.
Seems like a good idea!
>
>> > An alternative way to get consistency is to eliminate the pwrseq
>> > "power_on" callback altogether; the "power_up" callback will always
>> > be made before the host interface power sequence is started - and
>> > that is about the only thing we can guarantee given the variability
>> > in host interface designs.
>>
>> Eliminating is likely not an option, since it would mean we will have
>> a hard time to cope with requirements for the pwrseq sequences.
>>
>> An option would be to move the call to the pwrseq ->power_on()
>> callback to the end of mmc_power_up(). That would make it as
>> consistent as we can from mmc core point of view. Right?
>
> Yes, and you might as well call the pwrseq callbacks "->pre_power_on()"
> and "->post_power_on()" - maybe even "->post_pwrclk_on()" to make it
> clear that it happens after the power and clocks have been applied
> according to the MMC/SD protocol.
>
> What I'm envisioning mmc_power_up() becoming is something like this:
>
> void mmc_power_up(struct mmc_host *host, u32 ocr)
> {
> if (host->ios.power_mode == MMC_POWER_ON)
> return;
>
> mmc_host_clk_hold(host);
>
> if (pwrseq)
> pwrseq->pre_power_on(host);
>
> host->ios.vdd = fls(ocr) - 1;
> host->ios.clock = host->f_init;
> host->ios.power_mode = MMC_POWER_ON;
> /* and set remainder of initial state of host->ios */
>
> host->ops->power_up(host, &host->ios);
>
> if (pwrseq)
> pwrseq->post_power_on(host);
>
> mmc_host_clk_release(host);
> }
>
> The legacy power_up host method can save a copy host->ios, and manipulate
> the saved copy to call mmc_set_ios() as we currently do, with all the
> delays we have there.
>
Looks good!
I will rename the pwrseq callbacks to your proposal and send a v2.
Kind regards
Uffe
--
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
