Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] mmc: core: limit probe clock frequency to configured f_max
@ 2020-01-02 10:54 Michał Mirosław
  2020-01-16 14:07 ` Ulf Hansson
  0 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2020-01-02 10:54 UTC (permalink / raw)
  To: linux-mmc; +Cc: Ulf Hansson, linux-kernel

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.

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;
+		}
+		if (!mmc_rescan_try_freq(host, max(freq, host->f_min)))
 			break;
 		if (freqs[i] <= host->f_min)
 			break;
@@ -2344,7 +2350,7 @@ void mmc_rescan(struct work_struct *work)
 
 void mmc_start_host(struct mmc_host *host)
 {
-	host->f_init = max(freqs[0], host->f_min);
+	host->f_init = max(min(freqs[0], host->f_max), host->f_min);
 	host->rescan_disable = 0;
 	host->ios.power_mode = MMC_POWER_UNDEFINED;
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
  2020-01-02 10:54 [PATCH] mmc: core: limit probe clock frequency to configured f_max Michał Mirosław
@ 2020-01-16 14:07 ` Ulf Hansson
  2020-01-17 14:05   ` Michał Mirosław
  0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2020-01-16 14:07 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: linux-mmc, Linux Kernel Mailing List

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?

>
> 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.

> +               }
> +               if (!mmc_rescan_try_freq(host, max(freq, host->f_min)))
>                         break;
>                 if (freqs[i] <= host->f_min)
>                         break;
> @@ -2344,7 +2350,7 @@ void mmc_rescan(struct work_struct *work)
>
>  void mmc_start_host(struct mmc_host *host)
>  {
> -       host->f_init = max(freqs[0], host->f_min);
> +       host->f_init = max(min(freqs[0], host->f_max), host->f_min);
>         host->rescan_disable = 0;
>         host->ios.power_mode = MMC_POWER_UNDEFINED;
>
> --
> 2.20.1
>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
  2020-01-16 14:07 ` Ulf Hansson
@ 2020-01-17 14:05   ` Michał Mirosław
  2020-01-17 15:26     ` Ulf Hansson
  0 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2020-01-17 14:05 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
  2020-01-17 14:05   ` Michał Mirosław
@ 2020-01-17 15:26     ` Ulf Hansson
  2020-01-17 16:14       ` Michał Mirosław
  0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2020-01-17 15:26 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: linux-mmc, Linux Kernel Mailing List

On Fri, 17 Jan 2020 at 15:05, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> 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.

Alright.

>
> > > 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.

Well, my point isn't so much about the specs, rather about providing a
consistent behaviour.

We deal with f_min constraints like I described above, then I think we
should make f_max behave the similar way.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
  2020-01-17 15:26     ` Ulf Hansson
@ 2020-01-17 16:14       ` Michał Mirosław
  2020-01-20 11:25         ` Ulf Hansson
  0 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2020-01-17 16:14 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Linux Kernel Mailing List

On Fri, Jan 17, 2020 at 04:26:30PM +0100, Ulf Hansson wrote:
> On Fri, 17 Jan 2020 at 15:05, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> >
> > 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.
> 
> Alright.
> 
> >
> > > > 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.
> 
> Well, my point isn't so much about the specs, rather about providing a
> consistent behaviour.
> 
> We deal with f_min constraints like I described above, then I think we
> should make f_max behave the similar way.

Okay, this would be a second fix as trying the same freq multiple times
is not what this code is supposed to do.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max
  2020-01-17 16:14       ` Michał Mirosław
@ 2020-01-20 11:25         ` Ulf Hansson
  0 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2020-01-20 11:25 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: linux-mmc, Linux Kernel Mailing List

On Fri, 17 Jan 2020 at 17:14, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> On Fri, Jan 17, 2020 at 04:26:30PM +0100, Ulf Hansson wrote:
> > On Fri, 17 Jan 2020 at 15:05, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > >
> > > 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.
> >
> > Alright.
> >
> > >
> > > > > 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.
> >
> > Well, my point isn't so much about the specs, rather about providing a
> > consistent behaviour.
> >
> > We deal with f_min constraints like I described above, then I think we
> > should make f_max behave the similar way.
>
> Okay, this would be a second fix as trying the same freq multiple times
> is not what this code is supposed to do.

Well, I think we want to allow to run a couple retries on failures,
but I admit that it's kind of questionable to try the same freq
multiple times. Anyway, that's what the code around f_min does.

In any case, I have queued this is up for next, thanks!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-02 10:54 [PATCH] mmc: core: limit probe clock frequency to configured f_max Michał Mirosław
2020-01-16 14:07 ` Ulf Hansson
2020-01-17 14:05   ` Michał Mirosław
2020-01-17 15:26     ` Ulf Hansson
2020-01-17 16:14       ` Michał Mirosław
2020-01-20 11:25         ` Ulf Hansson

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