Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
From: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
Date: Fri, 17 Jan 2020 15:05:11 +0100
Message-ID: <20200117140511.GC26135@qmqm.qmqm.pl> (raw)
In-Reply-To: <CAPDyKFpZWnkK7UmCZ8M4UnM05wR3MQsPrpEjOJuwkKcN2gePSg@mail.gmail.com>

On Thu, Jan 16, 2020 at 03:07:22PM +0100, Ulf Hansson wrote:
> On Thu, 2 Jan 2020 at 11:54, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >
> > Currently MMC core disregards host->f_max during card initialization
> > phase. Obey upper boundary for the clock frequency and skip faster
> > speeds when they are above the limit.
> 
> Is this a hypothetical problem or a real problem?

This is a problem on noisy or broken boards or cards - so needed for
debugging such a combination. I wouldn't expect this is required for
normal devices.

> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  drivers/mmc/core/core.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index abf8f5eb0a1c..aa54d359dab7 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2330,7 +2330,13 @@ void mmc_rescan(struct work_struct *work)
> >         }
> >
> >         for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> > -               if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> > +               unsigned int freq = freqs[i];
> > +               if (freq > host->f_max) {
> > +                       if (i + 1 < ARRAY_SIZE(freqs))
> > +                               continue;
> > +                       freq = host->f_max;
> 
> This looks wrong to me. For example, what if f_max = 250KHz and f_min = 50 KHz.
> 
> Then we should try with 250KHz, then 200KHz and then 100KHz. This
> isn't what the above code does, I think.
> 
> Instead it will try with 200KHz and then 100KHz, thus skip 250KHz.
> 
> Maybe we should figure out what index of freqs[] to start the loop for
> (before actually starting the loop), depending on the value of f_max -
> rather than always start at 0.

Yes, it will skip higher frequencies. I didn't view it a problem,
because the new code guarantees at least one frequency will be tried.
The eMMC standard specifies only max init frequency (400kHz), so all we
should try is something less whatever works.

SD spec specifies minimal frequency (100kHz), but I wouldn't expect
this to be enforced nor required to be anywhere.

Best Regards
Michał Mirosław

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-02 10:54 Michał Mirosław
2020-01-16 14:07 ` Ulf Hansson
2020-01-17 14:05   ` Michał Mirosław [this message]
2020-01-17 15:26     ` Ulf Hansson
2020-01-17 16:14       ` Michał Mirosław
2020-01-20 11:25         ` Ulf Hansson

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=20200117140511.GC26135@qmqm.qmqm.pl \
    --to=mirq-linux@rere.qmqm.pl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.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

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git