On 21/08/20 3:25 pm, Ulf Hansson wrote:
> [...]
>
>>>> @@ -2300,6 +2304,33 @@ void mmc_rescan(struct work_struct *work)
>>>> goto out;
>>>> }
>>>>
>>>> + if (host->caps & MMC_CAP_UHS2) {
>>>> + /*
>>>> + * Start to try UHS-II initialization from 52MHz to 26MHz
>>>> + * (RCLK range) per spec.
>>>> + */
>>>> + for (i = 0; i < ARRAY_SIZE(uhs2_freqs); i++) {
>>>> + unsigned int freq = uhs2_freqs[i];
>>>> + int err;
>>>> +
>>>> + err = mmc_uhs2_rescan_try_freq(host,
>>>> + max(freq, host->f_min));
>>>> + if (!err) {
>>>> + mmc_release_host(host);
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (err == UHS2_PHY_INIT_ERR)
>>>> + /* UHS2 IF detect or Lane Sync error.
>>>> + * Try legacy interface.
>>>> + */
>>>> + break;
>>>> +
>>>> + if (freq <= host->f_min)
>>>> + break;
>>>> + }
>>>
>>> Assuming we change the initialization order, trying to initialize the
>>> legacy SD interface first to figure out if UHS-II is supported, then I
>>> think we should be able to move the entire code above into a the
>>> UHS-II specific code/path.
>>
>> If the host tries to use the SD interface first,
>> some failure status needs to be considered.
>>
>> For example, first run in SD mode, try UHS-II interface failure,
>> and then return to SD flow again.
>> I don't know a good way to go back to SD flow again.
>
> Right, a re-try path for the legacy SD interface is a very good idea!
> However, I don't think the initial support for UHS-II needs to cover
> it. Instead we can add that on top, don't you think?
>
> As a matter of fact, we could even use something like that for
> different legacy SD speed modes. For example, if UHS-I SDR104 fails we
> could try with UHS-I SDR25.
>
>>
>>>
>>>> + }
>>>> +
>>>> for (i = 0; i < ARRAY_SIZE(freqs); i++) {
>>>> unsigned int freq = freqs[i];
>>>> if (freq > host->f_max) {
>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>> index 5a2210c25aa7..c5b071bd98b3 100644
>>>> --- a/drivers/mmc/core/sd.c
>>>> +++ b/drivers/mmc/core/sd.c
>>>> @@ -901,6 +901,20 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>>>> err = mmc_read_switch(card);
>>>> if (err)
>>>> return err;
>>>> + if (host->flags & MMC_UHS2_INITIALIZED) {
>>>
>>> Rather than using host->flags, to tweak the behavior of
>>> mmc_sd_setup_card() to support UHS-II, I would prefer to give
>>> mmc_sd_setup_card() a new in-parameter that it can look at instead.
>>
>> Do you mean that adding a new in-parameter to mmc_sd_setup_card() likes this
>> mmc_sd_setup_card(struct mmc_host *, struct mmc_card *, boot reinit,
>> boot uhsii); ?
>
> Correct.
>
> [...]
>
> Looks like we have covered most of the review for the mmc core
> changes. But please tell me, if there is anything else you want me to
> look at - at any time. Otherwise I am deferring to wait for a new
> version of the series.
>
> If I get some time, I may start to help with hacking some code.
> Perhaps I can do some preparations, so it makes it easier for you to
> add the UHS-II specific code. If so, I will let you know about it, of
> course.
>
> When it comes to the changes to SDHCI, I am relying on Adrian to give
> his opinions.
I have made some comments. The thrust of which is:
- get all the UHS-II code into sdhci-uhs2 (not sdhci.c)
- make the driver (i.e sdhci-pci-gli) set existing mmc host ops callbacks
and sdhci host ops callbacks as much as possible to provide UHS-II
functionality i.e. avoid adding new hooks if possible
- refactoring and exporting functions from sdhci.c where that can be done
logically, but otherwise writing separate code for UHS-II
