[PATCH] mmc: core: limit probe clock frequency to configured f_max

[PATCH] mmc: core: limit probe clock frequency to configured f_max

[Michał Mirosław]

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

Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max

[Ulf Hansson]

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

Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max

[Michał Mirosław]

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

Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max

[Ulf Hansson]

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

Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max

[Michał Mirosław]

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

Re: [PATCH] mmc: core: limit probe clock frequency to configured f_max

[Ulf Hansson]

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