From: Adrian Hunter <adrian.hunter@intel.com>
To: Ulf Hansson <ulf.hansson@linaro.org>,
Ben Chuang <benchuanggli@gmail.com>
Cc: "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Ben Chuang <ben.chuang@genesyslogic.com.tw>,
Takahiro Akashi <takahiro.akashi@linaro.org>,
greg.tu@genesyslogic.com.tw
Subject: Re: [RFC PATCH V3 04/21] mmc: core: UHS-II support, try to select UHS-II interface
Date: Fri, 21 Aug 2020 17:28:08 +0300 [thread overview]
Message-ID: <a5ee1d39-01c7-acb4-fe84-64a416ced9de@intel.com> (raw)
In-Reply-To: <CAPDyKFoL8Yh_SJSw56kQsFBRE64oRGTnyzgvOm__-1f+Cvxpzw@mail.gmail.com>
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
prev parent reply other threads:[~2020-08-21 14:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 11:08 [RFC PATCH V3 04/21] mmc: core: UHS-II support, try to select UHS-II interface Ben Chuang
2020-08-18 11:41 ` Ulf Hansson
2020-08-21 11:33 ` Ben Chuang
2020-08-21 12:25 ` Ulf Hansson
2020-08-21 14:28 ` Adrian Hunter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a5ee1d39-01c7-acb4-fe84-64a416ced9de@intel.com \
--to=adrian.hunter@intel.com \
--cc=ben.chuang@genesyslogic.com.tw \
--cc=benchuanggli@gmail.com \
--cc=greg.tu@genesyslogic.com.tw \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=takahiro.akashi@linaro.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).