mmc: host: Add facility to support re-tuning

For each MMC, SD and SDIO there is code that
holds the clock, calls ops->execute_tuning, and
releases the clock. Simplify the code a bit by
providing a separate function to do that.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/core.c | 24 ++++++++++++++++++++++++
 drivers/mmc/core/core.h |  3 +++
 drivers/mmc/core/mmc.c  | 14 +-------------
 drivers/mmc/core/sd.c   | 13 ++++---------
 drivers/mmc/core/sdio.c | 14 ++++----------
 5 files changed, 36 insertions(+), 32 deletions(-)

drivers/mmc/core/core.c +24 -0

index 9584bff..b329725 100644
@@ -1077,6 +1077,30 @@ void mmc_set_ungated(struct mmc_host *host)
 }
 #endif
 
+int mmc_execute_tuning(struct mmc_card *card)
+{
+       struct mmc_host *host = card->host;
+       u32 opcode;
+       int err;
+
+       if (!host->ops->execute_tuning)
+               return 0;
+
+       if (mmc_card_mmc(card))
+               opcode = MMC_SEND_TUNING_BLOCK_HS200;
+       else
+               opcode = MMC_SEND_TUNING_BLOCK;
+
+       mmc_host_clk_hold(host);
+       err = host->ops->execute_tuning(host, opcode);
+       mmc_host_clk_release(host);
+
+       if (err)
+               pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+
+       return err;
+}
+
 /*
  * Change the bus mode (open drain/push-pull) of a host.
  */

drivers/mmc/core/core.h +3 -0

index d76597c..1b022c9 100644
@@ -82,5 +82,8 @@ void mmc_add_card_debugfs(struct mmc_card *card);
 void mmc_remove_card_debugfs(struct mmc_card *card);
 
 void mmc_init_context_info(struct mmc_host *host);
+
+int mmc_execute_tuning(struct mmc_card *card);
+
 #endif
 

drivers/mmc/core/mmc.c +1 -13

index 02ad792..c712ba7 100644
@@ -1194,7 +1194,6 @@ EXPORT_SYMBOL(tuning_blk_pattern_8bit);
 static int mmc_hs200_tuning(struct mmc_card *card)
 {
        struct mmc_host *host = card->host;
-       int err = 0;
 
        /*
         * Timing should be adjusted to the HS400 target

@@ -1205,18 +1204,7 @@ static int mmc_hs200_tuning(struct mmc_card *card)
                if (host->ops->prepare_hs400_tuning)
                        host->ops->prepare_hs400_tuning(host, &host->ios);
 
-       if (host->ops->execute_tuning) {
-               mmc_host_clk_hold(host);
-               err = host->ops->execute_tuning(host,
-                               MMC_SEND_TUNING_BLOCK_HS200);
-               mmc_host_clk_release(host);
-
-               if (err)
-                       pr_err("%s: tuning execution failed\n",
-                               mmc_hostname(host));
-       }
-
-       return err;
+       return mmc_execute_tuning(card);
 }
 
 /*

drivers/mmc/core/sd.c +4 -9

index d90a6de..7907544 100644
@@ -660,15 +660,10 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
         * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
         * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
         */
-       if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
-                       (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
-                        card->sd_bus_speed == UHS_SDR104_BUS_SPEED)) {
-               mmc_host_clk_hold(card->host);
-               err = card->host->ops->execute_tuning(card->host,
-                                                     MMC_SEND_TUNING_BLOCK);
-               mmc_host_clk_release(card->host);
-       }
-
+       if (!mmc_host_is_spi(card->host) &&
+           (card->sd_bus_speed == UHS_SDR50_BUS_SPEED ||
+            card->sd_bus_speed == UHS_SDR104_BUS_SPEED))
+               err = mmc_execute_tuning(card);
 out:
        kfree(status);
 

drivers/mmc/core/sdio.c +4 -11

index fd0750b..ce6cc47 100644
@@ -567,17 +567,11 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
         * SPI mode doesn't define CMD19 and tuning is only valid for SDR50 and
         * SDR104 mode SD-cards. Note that tuning is mandatory for SDR104.
         */
-       if (!mmc_host_is_spi(card->host) && card->host->ops->execute_tuning &&
-                       ((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
-                        (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104))) {
-               mmc_host_clk_hold(card->host);
-               err = card->host->ops->execute_tuning(card->host,
-                                                     MMC_SEND_TUNING_BLOCK);
-               mmc_host_clk_release(card->host);
-       }
-
+       if (!mmc_host_is_spi(card->host) &&
+           ((card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR50) ||
+            (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)))
+               err = mmc_execute_tuning(card);
 out:
-
        return err;
 }
 
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Currently, there is core support for tuning during initialization. There can also be a need to re-tune periodically (e.g. sdhci) or to re-tune after the host controller is powered off (e.g. after PM runtime suspend / resume) or to re-tune in response to CRC errors.

The main requirements for re-tuning are:
- ability to enable /disable re-tuning
- ability to flag that re-tuning is needed
- ability to re-tune before any request
- ability to hold off re-tuning if the card is busy
- ability to hold off re-tuning if re-tuning is in progress
- ability to run a re-tuning timer

To support those requirements 5 members are added to struct mmc_host:

  unsigned int          can_retune:1;   /* re-tuning can be used */
  unsigned int          doing_retune:1; /* re-tuning in progress */
  int                   need_retune;    /* re-tuning is needed */
  int                   hold_retune;    /* hold off re-tuning */
  struct timer_list     retune_timer;   /* for periodic re-tuning */

need_retune is an integer so it can be set without needing synchronization. hold_retune is a integer to allow nesting.

Various simple functions are provided to set / clear those variables.

Subsequent patches take those functions into use.

Signed-off-by Adrian Hunter <[email protected]>

Hi Adrian,

Thanks for working on this and apologize for my late reply!

Quoted 7 lines · Adrian Hunter

Currently, there is core support for tuning during initialization. There can also be a need to re-tune periodically (e.g. sdhci) or to re-tune after the host controller is powered off (e.g. after PM runtime suspend / resume) or to re-tune in response to CRC errors.

Quoted 8 lines

The main requirements for re-tuning are:
- ability to enable /disable re-tuning
- ability to flag that re-tuning is needed
- ability to re-tune before any request
- ability to hold off re-tuning if the card is busy
- ability to hold off re-tuning if re-tuning is in progress
- ability to run a re-tuning timer

I suggest we skip the support for the re-tuning timer in this initial step and thus remove the related functionality from this patchset. It adds complexity, but more important it's not obvious that it actually will help. I am more concerned that it randomly will cause a request latency and thus decrease performance.

The re-tuning period can't be selected "perfectly", so in this initial step let's instead just rely on re-tune from the request retry path.

If we do see a need for a doing re-tuning periodically, how about using the runtime PM suspend path (of the mmc card device). In that way, we should be able to minimize the impact on performance.

Quoted 27 lines · Ulf Hansson

Hi Adrian,

Thanks for working on this and apologize for my late reply!

Quoted 7 lines · Adrian Hunter

Currently, there is core support for tuning during initialization. There can also be a need to re-tune periodically (e.g. sdhci) or to re-tune after the host controller is powered off (e.g. after PM runtime suspend / resume) or to re-tune in response to CRC errors.

Quoted 8 lines

The main requirements for re-tuning are:
- ability to enable /disable re-tuning
- ability to flag that re-tuning is needed
- ability to re-tune before any request
- ability to hold off re-tuning if the card is busy
- ability to hold off re-tuning if re-tuning is in progress
- ability to run a re-tuning timer

I suggest we skip the support for the re-tuning timer in this initial step and thus remove the related functionality from this patchset. It adds complexity, but more important it's not obvious that it actually will help. I am more concerned that it randomly will cause a request latency and thus decrease performance.

Quoted 6 lines

The re-tuning period can't be selected "perfectly", so in this initial step let's instead just rely on re-tune from the request retry path.

If we do see a need for a doing re-tuning periodically, how about using the runtime PM suspend path (of the mmc card device). In that way, we should be able to minimize the impact on performance.

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so this is just moving it into core, where it won't be used by other drivers unless they enable it.

I am not sure what you want to leave in sdhci.c and what you want in core, if anything.

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune, and switch back.

Quoted 35 lines · Adrian Hunter

Quoted 33 lines · Ulf Hansson

Hi Adrian,

Thanks for working on this and apologize for my late reply!

Quoted 7 lines · Adrian Hunter

Currently, there is core support for tuning during initialization. There can also be a need to re-tune periodically (e.g. sdhci) or to re-tune after the host controller is powered off (e.g. after PM runtime suspend / resume) or to re-tune in response to CRC errors.

Quoted 8 lines

The main requirements for re-tuning are:
- ability to enable /disable re-tuning
- ability to flag that re-tuning is needed
- ability to re-tune before any request
- ability to hold off re-tuning if the card is busy
- ability to hold off re-tuning if re-tuning is in progress
- ability to run a re-tuning timer

I suggest we skip the support for the re-tuning timer in this initial step and thus remove the related functionality from this patchset. It adds complexity, but more important it's not obvious that it actually will help. I am more concerned that it randomly will cause a request latency and thus decrease performance.

The re-tuning period can't be selected "perfectly", so in this initial step let's instead just rely on re-tune from the request retry path.

If we do see a need for a doing re-tuning periodically, how about using the runtime PM suspend path (of the mmc card device). In that way, we should be able to minimize the impact on performance.

Quoted 5 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so this is just moving it into core, where it won't be used by other drivers unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Can sdhci rely on that the mmc core performs a re-tune from the request retry mechanism instead?

Quoted 4 lines

I am not sure what you want to leave in sdhci.c and what you want in core, if anything.

We need all the infrastructure code in the core. Much like what your patchset does. Except that I would like you to remove the option of having a timer and the corresponding complexity it adds.

Quoted 4 lines

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune, and switch back.

As stated, I am only questioning the timer, nothing else.

Quoted 7 lines

Quoted 5 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so this is just moving it into core, where it won't be used by other drivers unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

Quoted 4 lines

Can sdhci rely on that the mmc core performs a re-tune from the request retry mechanism instead?

Not according to the standard.

Quoted 9 lines

Quoted 5 lines

I am not sure what you want to leave in sdhci.c and what you want in core, if anything.

We need all the infrastructure code in the core. Much like what your patchset does. Except that I would like you to remove the option of having a timer and the corresponding complexity it adds.

If we are going to follow the standard then that doesn't seem to be an option.

Quoted 7 lines

Quoted 5 lines

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune, and switch back.

As stated, I am only questioning the timer, nothing else.

Ok so how should I proceed?

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so this is just moving it into core, where it won't be used by other drivers unless they enable it.

Quoted 5 lines

Quoted 1 line

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the "sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

Does the spec say what value the timer should have?

Quoted 6 lines

Quoted 4 lines

Can sdhci rely on that the mmc core performs a re-tune from the request retry mechanism instead?

Not according to the standard.

We don't have to implement everything that comes with the standard. We can leave things out.

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

I am not sure what you want to leave in sdhci.c and what you want in core, if anything.

We need all the infrastructure code in the core. Much like what your patchset does. Except that I would like you to remove the option of having a timer and the corresponding complexity it adds.

If we are going to follow the standard then that doesn't seem to be an option.

I am not suggestion us to violating the spec. I am suggesting to currently not support all of it.

Quoted 10 lines

Quoted 7 lines

Quoted 5 lines

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune, and switch back.

As stated, I am only questioning the timer, nothing else.

Ok so how should I proceed?

As I stated, let's try without the timer first.

If we find it's not enough to recover at the request retry path, since it happens too often - lets deal with that then.

Okay?

Kind regards
Uffe

Quoted 16 lines · Ulf Hansson

[...]

Quoted 13 lines

Quoted 11 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 8 lines

Quoted 5 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Eh - Ch Reserved
Fh Get information from other source

Quoted 45 lines · Arend van Spriel

Quoted 16 lines · Ulf Hansson

[...]

Quoted 13 lines

Quoted 11 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 8 lines

Quoted 5 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like SDR104.

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

Isn't heat one of the most important factor that could effect the need for a re-tune? Does then the controller internally dynamically update this register (since it keep track of heat or similar)?

Kind regards
Uffe

Quoted 60 lines · Ulf Hansson

Quoted 54 lines · Arend van Spriel

Quoted 28 lines · Ulf Hansson

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

Quoted 2 lines

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 6 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
SDR104.

Quoted 1 line

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in
field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Quoted 5 lines

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for Re-Tuning field in this register. The timer starts counting by loading the initial value. When the timer expires, the Host Driver marks an expiration flag. On receiving a command request, the Host driver checks the expiration flag. If the expiration flag is set, then the Host Driver should perform the re-tuning procedure before issuing a command. If the expiration flag is not set, then the Host Driver issues a command without performing the re-tuning procedure. Every time the re-tuning procedure is performed, the timer loads the new initial value and the expiration flag is cleared.

So the host controller could indeed update this register for subsequent retuning.

From the same section you quoted above:

Quoted 82 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 54 lines · Arend van Spriel

Quoted 28 lines · Ulf Hansson

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

Quoted 2 lines

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 6 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
SDR104.

Quoted 1 line

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in
field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Quoted 5 lines

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for Re-Tuning field in this register. The timer starts counting by loading the initial value. When the timer expires, the Host Driver marks an expiration flag. On receiving a command request, the Host driver checks the expiration flag. If the expiration flag is set, then the Host Driver should perform the re-tuning procedure before issuing a command. If the expiration flag is not set, then the Host Driver issues a command without performing the re-tuning procedure. Every time the re-tuning procedure is performed, the timer loads the new initial value and the expiration flag is cleared.

Quoted 2 lines

So the host controller could indeed update this register for subsequent retuning.

Arend, thanks for the link and information. So, I decided to go for a look in there.

Quoted 1 line

From the same section you quoted above:

------
(1) Re-Tuning Mode 1
The host controller does not have any internal logic to detect when the re-tuning needs to be performed. In this case, the Host Driver should maintain all re-tuning timings by using a Re-Tuning Timer. To enable inserting the re-tuning procedure during data transfers, the data length per read/write command shall be limited up to 4MB.
------

That means, we can't get _any_ help from the controller HW (in mode 1) to find a good value for the timer.

Quoted 99 lines · Ulf Hansson

Quoted 82 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 54 lines · Arend van Spriel

Quoted 28 lines · Ulf Hansson

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

Quoted 2 lines

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 6 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
SDR104.

Quoted 1 line

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in
field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Quoted 5 lines

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for Re-Tuning field in this register. The timer starts counting by loading the initial value. When the timer expires, the Host Driver marks an expiration flag. On receiving a command request, the Host driver checks the expiration flag. If the expiration flag is set, then the Host Driver should perform the re-tuning procedure before issuing a command. If the expiration flag is not set, then the Host Driver issues a command without performing the re-tuning procedure. Every time the re-tuning procedure is performed, the timer loads the new initial value and the expiration flag is cleared.

Quoted 2 lines

So the host controller could indeed update this register for subsequent retuning.

Arend, thanks for the link and information. So, I decided to go for a look in there.

Quoted 1 line

From the same section you quoted above:

------
(1) Re-Tuning Mode 1
The host controller does not have any internal logic to detect when the re-tuning needs to be performed. In this case, the Host Driver should maintain all re-tuning timings by using a Re-Tuning Timer. To enable inserting the re-tuning procedure during data transfers, the data length per read/write command shall be limited up to 4MB.
------

Quoted 2 lines

That means, we can't get _any_ help from the controller HW (in mode 1) to find a good value for the timer.

In fact the timer value *is* defined in the Capabilities Register (Offset 040h) bits 43-40 Timer Count for Re-Tuning

It has been supported since 2011, see:

commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9 "mmc: sdhci: add support for retuning mode 1"

Quoted 106 lines · Adrian Hunter

Quoted 99 lines · Ulf Hansson

Quoted 82 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 54 lines · Arend van Spriel

Quoted 28 lines · Ulf Hansson

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

Quoted 2 lines

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 6 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
SDR104.

Quoted 1 line

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in
field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Quoted 5 lines

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for Re-Tuning field in this register. The timer starts counting by loading the initial value. When the timer expires, the Host Driver marks an expiration flag. On receiving a command request, the Host driver checks the expiration flag. If the expiration flag is set, then the Host Driver should perform the re-tuning procedure before issuing a command. If the expiration flag is not set, then the Host Driver issues a command without performing the re-tuning procedure. Every time the re-tuning procedure is performed, the timer loads the new initial value and the expiration flag is cleared.

Quoted 2 lines

So the host controller could indeed update this register for subsequent retuning.

Arend, thanks for the link and information. So, I decided to go for a look in there.

Quoted 1 line

From the same section you quoted above:

------
(1) Re-Tuning Mode 1
The host controller does not have any internal logic to detect when the re-tuning needs to be performed. In this case, the Host Driver should maintain all re-tuning timings by using a Re-Tuning Timer. To enable inserting the re-tuning procedure during data transfers, the data length per read/write command shall be limited up to 4MB.
------

Quoted 2 lines

That means, we can't get _any_ help from the controller HW (in mode 1) to find a good value for the timer.

In fact the timer value *is* defined in the Capabilities Register (Offset 040h) bits 43-40 Timer Count for Re-Tuning

Quoted 5 lines

It has been supported since 2011, see:

commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9 "mmc: sdhci: add support for retuning mode 1"

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature change and the corresponding effect that has on the signal.

Quoted 8 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

[...]

Quoted 22 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature change and the corresponding effect that has on the signal.

Quoted 8 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

Quoted 4 lines

I would like to extend the model to include transparently re-tuning and re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

That perfect and in line from what I heard as recommendations from memory vendors as well.

Now, can we stop arguing about the timer and try without it?

If we do see a need for a more frequent re-tuning to happen, due to that we get lots of CRC errors to recover from, then I think we should look into using runtime PM instead of the timer. And that's because I want to minimize the impact on performance.

Quoted 29 lines · Ulf Hansson

[...]

Quoted 26 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature change and the corresponding effect that has on the signal.

Quoted 8 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Quoted 8 lines

Now, can we stop arguing about the timer and try without it?

If we do see a need for a more frequent re-tuning to happen, due to that we get lots of CRC errors to recover from, then I think we should look into using runtime PM instead of the timer. And that's because I want to minimize the impact on performance.

The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS T100TA had a timer value of 128 seconds. The timer is not a performance issue.

There is a performance question with runtime PM because that happens far more frequently (typical auto-suspend delay is 50ms) and we re-tune after that. In fact I generalized that a bit in patch 13.

[PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning

Make use of mmc core support for re-tuning instead of doing it all in the sdhci driver.

This patch also changes to flag the need for re-tuning always after runtime suspend when tuning has been used at initialization. Previously it was only done if the re-tuning timer was in use.

One option to reduce the impact of the latency would be to increase the auto-suspend delay.

Quoted 30 lines · Adrian Hunter

Quoted 29 lines · Ulf Hansson

[...]

Quoted 26 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature change and the corresponding effect that has on the signal.

Quoted 8 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

Quoted 4 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

Quoted 21 lines

Quoted 8 lines

Now, can we stop arguing about the timer and try without it?

If we do see a need for a more frequent re-tuning to happen, due to that we get lots of CRC errors to recover from, then I think we should look into using runtime PM instead of the timer. And that's because I want to minimize the impact on performance.

The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS T100TA had a timer value of 128 seconds. The timer is not a performance issue.

There is a performance question with runtime PM because that happens far more frequently (typical auto-suspend delay is 50ms) and we re-tune after that. In fact I generalized that a bit in patch 13.

[PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning

Make use of mmc core support for re-tuning instead of doing it all in the sdhci driver.

Quoted 7 lines

This patch also changes to flag the need for re-tuning always after runtime suspend when tuning has been used at initialization. Previously it was only done if the re-tuning timer was in use.

One option to reduce the impact of the latency would be to increase the auto-suspend delay.

The latency will affect the first request after a runtime PM suspend/resume cycle. So for continues data transfers the impact should be zero. Also, increasing the delay would impact power consumption, but it's a balance I guess. :-)

This is a specific issue for SDHCI (it's not clear to me if all SDHCI variants have the same behaviour). Obviously the mmc core needs to support the demand from SDHCI, such enable it to tell the core to perform a re-tune. Exactly what your patchset does.

For your reference, I know about other controllers which can restore a bunch of register values, saved from earlier re-tunings, from its runtime PM resume callbacks. Thus preventing a re-tuning from happen. I wonder if some of the SDHCI variant are capable of this as well.

Kind regards
Uffe

Quoted 53 lines · Ulf Hansson

Quoted 34 lines · Adrian Hunter

Quoted 33 lines · Ulf Hansson

[...]

Quoted 30 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

Quoted 9 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

Quoted 5 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Quoted 8 lines

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main reason being that our 11ac wifi SDIO cards do support SDR104. So the brcmfmac driver support SDIO and has retry mechanisms in place. However, it may also end-up doing an abort under certain conditions.

You also mentioned using runtime-pm, but how do you deal with func drivers not supporting runtime-pm. That is already an issue aka. bug right now. Our driver does not support runtime-pm (yet) and we have reported issues that host controller does runtime-pm basically killing communication between device and func driver.

Gr. AvS

Quoted 50 lines

Quoted 24 lines

Quoted 8 lines

Now, can we stop arguing about the timer and try without it?

If we do see a need for a more frequent re-tuning to happen, due to that we get lots of CRC errors to recover from, then I think we should look into using runtime PM instead of the timer. And that's because I want to minimize the impact on performance.

The minimum timer value is 1 second. The maximum is 1024 seconds. The ASUS
T100TA had a timer value of 128 seconds. The timer is not a performance issue.

There is a performance question with runtime PM because that happens far more frequently (typical auto-suspend delay is 50ms) and we re-tune after
that. In fact I generalized that a bit in patch 13.

[PATCH 13/13] mmc: sdhci: Change to new way of doing re-tuning

Make use of mmc core support for re-tuning instead of doing it all in the sdhci driver.

Quoted 7 lines

This patch also changes to flag the need for re-tuning always after runtime suspend when tuning has been used at initialization. Previously it was only done if the re-tuning timer was in use.

One option to reduce the impact of the latency would be to increase the auto-suspend delay.

The latency will affect the first request after a runtime PM suspend/resume cycle. So for continues data transfers the impact should be zero. Also, increasing the delay would impact power consumption, but it's a balance I guess. :-)

This is a specific issue for SDHCI (it's not clear to me if all SDHCI variants have the same behaviour). Obviously the mmc core needs to support the demand from SDHCI, such enable it to tell the core to perform a re-tune. Exactly what your patchset does.

For your reference, I know about other controllers which can restore a bunch of register values, saved from earlier re-tunings, from its runtime PM resume callbacks. Thus preventing a re-tuning from happen. I wonder if some of the SDHCI variant are capable of this as well.

Kind regards
Uffe

Quoted 77 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 38 lines · Adrian Hunter

Quoted 36 lines · Ulf Hansson

[...]

Quoted 32 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

Quoted 9 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

Quoted 6 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Quoted 8 lines

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main reason being that our 11ac wifi SDIO cards do support SDR104. So the brcmfmac driver support SDIO and has retry mechanisms in place. However, it may also end-up doing an abort under certain conditions.

Quoted 5 lines

You also mentioned using runtime-pm, but how do you deal with func drivers not supporting runtime-pm. That is already an issue aka. bug right now. Our driver does not support runtime-pm (yet) and we have reported issues that host controller does runtime-pm basically killing communication between device and func driver.

Could leave it to the function driver to call mmc_retune_needed().

Quoted 78 lines · Arend van Spriel

Quoted 77 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 38 lines · Adrian Hunter

Quoted 36 lines · Ulf Hansson

[...]

Quoted 32 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

Quoted 9 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not documented in the spec but recommended by others.

Quoted 6 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Quoted 8 lines

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main reason being that our 11ac wifi SDIO cards do support SDR104. So the brcmfmac driver support SDIO and has retry mechanisms in place. However, it may also end-up doing an abort under certain conditions.

Quoted 6 lines

Quoted 5 lines

You also mentioned using runtime-pm, but how do you deal with func drivers not supporting runtime-pm. That is already an issue aka. bug right now. Our driver does not support runtime-pm (yet) and we have reported issues that host controller does runtime-pm basically killing communication between device and func driver.

Runtime PM is implemented a bit differently between SDIO vs MMC/SD. Your are right.

For MMC/SD the mmc block device handles pm_runtime_get|put() in principle per request basis and makes use of the pm_runtime_autosuspend feature. While in the SDIO case, it's entirely up the SDIO func driver to deal with pm_runtime_get|put().

So it seems like we can use runtime PM for MMC/SD but not for SDIO. At least not using the SDIO func device.

Quoted 2 lines

Could leave it to the function driver to call mmc_retune_needed().

Hmm, the positive side from such approach would be that the SDIO func driver can decide when it's convenient to do a re-tune.

The negative side is that all SDIO func driver would need to care about this. I am not sure we want that.

Kind regards
Uffe

Quoted 115 lines · Ulf Hansson

Quoted 93 lines · Arend van Spriel

Quoted 91 lines · Arend van Spriel

Quoted 72 lines · Ulf Hansson

Quoted 47 lines · Adrian Hunter

Quoted 44 lines · Ulf Hansson

[...]

Quoted 39 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

Quoted 11 lines

Even if the above commit was merged, I don't think it was the correct
way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should
be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set
host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not
documented in the spec but recommended by others.

Quoted 7 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Quoted 8 lines

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main reason being that our 11ac wifi SDIO cards do support SDR104. So the brcmfmac driver support SDIO and has retry mechanisms in place. However, it may also end-up doing an abort under certain conditions.

Quoted 7 lines

Quoted 5 lines

You also mentioned using runtime-pm, but how do you deal with func drivers not supporting runtime-pm. That is already an issue aka. bug right now. Our driver does not support runtime-pm (yet) and we have reported issues that host controller does runtime-pm basically killing communication between device and func driver.

Runtime PM is implemented a bit differently between SDIO vs MMC/SD. Your are right.

For MMC/SD the mmc block device handles pm_runtime_get|put() in principle per request basis and makes use of the pm_runtime_autosuspend feature. While in the SDIO case, it's entirely up the SDIO func driver to deal with pm_runtime_get|put().

So it seems like we can use runtime PM for MMC/SD but not for SDIO. At least not using the SDIO func device.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Could leave it to the function driver to call mmc_retune_needed().

Hmm, the positive side from such approach would be that the SDIO func driver can decide when it's convenient to do a re-tune.

I would say "appropriate" instead of "convenient".

Quoted 2 lines

The negative side is that all SDIO func driver would need to care about this. I am not sure we want that.

The whole retry handling also seems deferred to the SDIO func driver and the same for runtime-pm. As the "retune needed" question would pops up during the retry handling it seems not a bad option.

Quoted 129 lines · Arend van Spriel

Quoted 115 lines · Ulf Hansson

Quoted 93 lines · Arend van Spriel

Quoted 91 lines · Arend van Spriel

Quoted 72 lines · Ulf Hansson

Quoted 47 lines · Adrian Hunter

Quoted 44 lines · Ulf Hansson

[...]

Quoted 39 lines

Quoted 3 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Presumably the value is chosen based on the maximum rate of temperature
change and the corresponding effect that has on the signal.

Quoted 11 lines

Even if the above commit was merged, I don't think it was the correct
way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should
be managed from the mmc core, like the approach you have taken in your
$subject patchset. Second I question whether the timer is useful at all.

The SD Host Controller Specification does not document another way to do
mode 1 re-tuning. The timer is it. Otherwise re-tuning is never done.

In the patches I sent, the driver must call mmc_retune_needed() to set
host->need_retune = 1 otherwise mmc_retune() does nothing.

I would like to extend the model to include transparently re-tuning and
re-trying when there is a CRC error, but that is a separate issue, not
documented in the spec but recommended by others.

Quoted 7 lines

Quoted 2 lines

That perfect and in line from what I heard as recommendations from memory vendors as well.

How would that work for SDIO? How do you know it is OK to retry SDIO operations?

Retries or error handling, needs to be handled from SDIO func drivers or upper level code. They certainly also need it for other errors, which are not caused by the lack of a re-tune. I believe they exist already.

For mmc core point of view, we need to act on SDIO data transfers errors and perform re-tuning for cases when it makes sense.

More importantly, using a timer won't make SDIO data transfers error free, since we can still end up needing a re-tune at any point.

Quoted 8 lines

Still, you do have point for SDIO. Minimizing the number of errors for SDIO could be important, due to that an SDIO func driver may not be able to recover from data errors as smoothly as the mmc block layer can. Thus, a timer could help to improve the situation, but I think it only makes sense in the SDIO case.

BTW, what's your experience around SDIO cards supporting SDR104. I have never used such, have you?

My primary focus in all this discussing is about SDIO cards. The main reason being that our 11ac wifi SDIO cards do support SDR104. So the brcmfmac driver support SDIO and has retry mechanisms in place. However, it may also end-up doing an abort under certain conditions.

Quoted 7 lines

Quoted 5 lines

You also mentioned using runtime-pm, but how do you deal with func drivers not supporting runtime-pm. That is already an issue aka. bug right now. Our driver does not support runtime-pm (yet) and we have reported issues that host controller does runtime-pm basically killing communication between device and func driver.

Runtime PM is implemented a bit differently between SDIO vs MMC/SD. Your are right.

For MMC/SD the mmc block device handles pm_runtime_get|put() in principle per request basis and makes use of the pm_runtime_autosuspend feature. While in the SDIO case, it's entirely up the SDIO func driver to deal with pm_runtime_get|put().

So it seems like we can use runtime PM for MMC/SD but not for SDIO. At least not using the SDIO func device.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Could leave it to the function driver to call mmc_retune_needed().

Hmm, the positive side from such approach would be that the SDIO func driver can decide when it's convenient to do a re-tune.

I would say "appropriate" instead of "convenient".

Quoted 2 lines

The negative side is that all SDIO func driver would need to care about this. I am not sure we want that.

Quoted 3 lines

The whole retry handling also seems deferred to the SDIO func driver and the same for runtime-pm. As the "retune needed" question would pops up during the retry handling it seems not a bad option.

Okay, your argument seems reasonable, let's got for this approach.

Quoted 91 lines · Adrian Hunter

Quoted 90 lines · Ulf Hansson

Quoted 82 lines · Arend van Spriel

Quoted 60 lines · Ulf Hansson

Quoted 54 lines · Arend van Spriel

Quoted 28 lines · Ulf Hansson

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 7 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so
this is just moving it into core, where it won't be used by other drivers
unless they enable it.

Quoted 2 lines

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Quoted 6 lines

It is part of the SD Host Controller Standard Specification. The timer ensures that re-tuning is done before temperature changes could affect the
"sampling point". It is needed for re-tuning mode 1 for UHS-I modes like
SDR104.

Quoted 1 line

Does the spec say what value the timer should have?

It is read from the Capabilities register in the SD host controller, ie. in
field "Timer Count for Re-Tuning" (see below).

Regards,
Arend

Timer Count for Re-Tuning
This field indicates an initial value of the Re-Tuning Timer for Re-Tuning
Mode 1 to 3. Setting to 0 disables Re-Tuning Timer.

0h      Re-Tuning Timer disabled
1h      1 seconds
2h      2 seconds
3h      4 seconds
4h      8 seconds
.....   ......................
n       2(n-1) seconds
.....   ......................
Bh      1024 seconds

Quoted 2 lines

Eh - Ch Reserved
Fh Get information from other source

Quoted 5 lines

Thanks for sharing this information, but unfortunate I don't understand much from it.

Is the host driver intended to read/poll this register to find a good value?

You can download the spec (and others) here [1]. sdhci currently implements retuning mode 1, which is decribed in the spec:

Re-Tuning Timer Control Example for Re-Tuning Mode 1
The initial value of re-tuning timer is provided by Timer Count for Re-Tuning field in this register. The timer starts counting by loading the initial value. When the timer expires, the Host Driver marks an expiration flag. On receiving a command request, the Host driver checks the expiration flag. If the expiration flag is set, then the Host Driver should perform the re-tuning procedure before issuing a command. If the expiration flag is not set, then the Host Driver issues a command without performing the re-tuning procedure. Every time the re-tuning procedure is performed, the timer loads the new initial value and the expiration flag is cleared.

Quoted 2 lines

So the host controller could indeed update this register for subsequent retuning.

Arend, thanks for the link and information. So, I decided to go for a look in there.

Quoted 1 line

From the same section you quoted above:

Quoted 8 lines

Quoted 8 lines

------
(1) Re-Tuning Mode 1
The host controller does not have any internal logic to detect when the re-tuning needs to be performed. In this case, the Host Driver should maintain all re-tuning timings by using a Re-Tuning Timer. To enable inserting the re-tuning procedure during data transfers, the data length per read/write command shall be limited up to 4MB.
------

Quoted 6 lines

Quoted 2 lines

That means, we can't get _any_ help from the controller HW (in mode 1) to find a good value for the timer.

In fact the timer value *is* defined in the Capabilities Register (Offset 040h) bits 43-40 Timer Count for Re-Tuning

Quoted 5 lines

It has been supported since 2011, see:

commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9 "mmc: sdhci: add support for retuning mode 1"

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

Kind regards
Uffe

Hi Ulf,

After sending my email I read that part as well and figured my response was incorrect.

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

That means, we can't get _any_ help from the controller HW (in mode 1) to find a good value for the timer.

In fact the timer value *is* defined in the Capabilities Register (Offset 040h) bits 43-40 Timer Count for Re-Tuning

It has been supported since 2011, see:

commit cf2b5eea1ea0ff9b3184bc6771bcb93a9fdcd1d9 "mmc: sdhci: add support for retuning mode 1"

Quoted 2 lines

The value from the register is also just randomly selected, only difference is that it's the HW that has randomly set it.

I think you can not say it like that. The value from the register is set by the manufacturer of the host controller. I would not say they would set that randomly. It is just hard-coded in their IP design. Now whether the value comes from actual hardware validation is hard to say.

Quoted 3 lines

Even if the above commit was merged, I don't think it was the correct way of dealing with re-tuning.

It seems a reasonable choice to follow the specification.

Quoted 4 lines

First of all, re-tuning this is a mmc protocol specific thing should be managed from the mmc core, like the approach you have taken in your $subject patchset. Second I question whether the timer is useful at all.

Not sure I understand what the alternative approach is here. You mentioned earlier something about "the request retry path". Does that mean you proposal is to only do a re-tuning procedure when a request fails.

That does
not seem like "the correct way of dealing with re-tuning" either as it introduces additional delay of the failed request. I would rather see some algorithm to adapt the timer value and thus keep a re-tuning timer. If you are concerned about doing unnecessary re-tuning cycles retuning could be limited to ADTC request as from what I understand about retuning is that it is only needed for requests that involve using the DAT lines.

Quoted 35 lines · Adrian Hunter

Quoted 33 lines · Ulf Hansson

Hi Adrian,

Thanks for working on this and apologize for my late reply!

Quoted 7 lines · Adrian Hunter

Currently, there is core support for tuning during initialization. There can also be a need to re-tune periodically (e.g. sdhci) or to re-tune after the host controller is powered off (e.g. after PM runtime suspend / resume) or to re-tune in response to CRC errors.

Quoted 8 lines

The main requirements for re-tuning are:
- ability to enable /disable re-tuning
- ability to flag that re-tuning is needed
- ability to re-tune before any request
- ability to hold off re-tuning if the card is busy
- ability to hold off re-tuning if re-tuning is in progress
- ability to run a re-tuning timer

I suggest we skip the support for the re-tuning timer in this initial step and thus remove the related functionality from this patchset. It adds complexity, but more important it's not obvious that it actually will help. I am more concerned that it randomly will cause a request latency and thus decrease performance.

The re-tuning period can't be selected "perfectly", so in this initial step let's instead just rely on re-tune from the request retry path.

If we do see a need for a doing re-tuning periodically, how about using the runtime PM suspend path (of the mmc card device). In that way, we should be able to minimize the impact on performance.

Quoted 5 lines

Thank you for looking at the patches.

I am not sure I know what you mean. sdhci already has a re-tuning timer, so this is just moving it into core, where it won't be used by other drivers unless they enable it.

I am kind of questioning the re-tuning timer in sdhci. What is it good for?

Can sdhci rely on that the mmc core performs a re-tune from the request retry mechanism instead?

Quoted 3 lines

I am not sure what you want to leave in sdhci.c and what you want in core, if anything.

We need all the infrastructure code in the core. Much like what your patchset does. Except that I would like you to remove the option of having a timer and the corresponding complexity it adds.

Quoted 3 lines

At a minimum I need sdhci to be able to switch from hs400 to hs200, re-tune, and switch back.

As stated, I am only questioning the timer, nothing else.

Kind regards
Uffe

Both callers of mmc_start_request() call mmc_card_removed() so move that call into mmc_start_request().

This patch is preparation for adding re-tuning support.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/core.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

drivers/mmc/core/core.c +20 -13

index fcde4da..884f35a 100644
@@ -185,13 +185,14 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 
 EXPORT_SYMBOL(mmc_request_done);
 
-static void
-mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
+static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
 {
 #ifdef CONFIG_MMC_DEBUG
        unsigned int i, sz;
        struct scatterlist *sg;
 #endif
+       if (mmc_card_removed(host->card))
+               return -ENOMEDIUM;
 
        if (mrq->sbc) {
                pr_debug("<%s: starting CMD%u arg %08x flags %08x>\n",

@@ -251,6 +252,8 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
        mmc_host_clk_hold(host);
        led_trigger_event(host->led, LED_FULL);
        host->ops->request(host, mrq);
+
+       return 0;
 }
 
 /**

@@ -345,29 +348,34 @@ static void mmc_wait_done(struct mmc_request *mrq)
  */
 static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+       int err;
+
        mrq->done = mmc_wait_data_done;
        mrq->host = host;
-       if (mmc_card_removed(host->card)) {
-               mrq->cmd->error = -ENOMEDIUM;
+
+       err = mmc_start_request(host, mrq);
+       if (err) {
+               mrq->cmd->error = err;
                mmc_wait_data_done(mrq);
-               return -ENOMEDIUM;
        }
-       mmc_start_request(host, mrq);
 
-       return 0;
+       return err;
 }
 
 static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+       int err;
+
        init_completion(&mrq->completion);
        mrq->done = mmc_wait_done;
-       if (mmc_card_removed(host->card)) {
-               mrq->cmd->error = -ENOMEDIUM;
+
+       err = mmc_start_request(host, mrq);
+       if (err) {
+               mrq->cmd->error = err;
                complete(&mrq->completion);
-               return -ENOMEDIUM;
        }
-       mmc_start_request(host, mrq);
-       return 0;
+
+       return err;
 }
 
 /*
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

There is no point making the initialization
of buf_ready_int conditional on host version.
Simplify by just doing it always. Note that
the other conditional initializations will be
removed when the new way of doing re-tuning
is taken into use.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mmc/host/sdhci.c +2 -3

index bfde165..dd4cc4c 100644
@@ -3331,9 +3331,9 @@ int sdhci_add_host(struct sdhci_host *host)
 
        setup_timer(&host->timer, sdhci_timeout_timer, (unsigned long)host);
 
-       if (host->version >= SDHCI_SPEC_300) {
-               init_waitqueue_head(&host->buf_ready_int);
+       init_waitqueue_head(&host->buf_ready_int);
 
+       if (host->version >= SDHCI_SPEC_300) {
                /* Initialize re-tuning timer */
                init_timer(&host->tuning_timer);
                host->tuning_timer.data = (unsigned long)host;
--

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi

Here are some patches to move re-tuning support out of sdhci and into the core, and add support for HS400 re-tuning.

Currently sdhci does re-tuning transparently by calling sdhci_execute_tuning() from its ->request() function.

The problem with HS400 re-tuning is that it must be done in HS200 mode. That means using switch commands and making ios changes. That means it potentially conflicts with other command sequences. The new re-tuning support accomodates that, however it is not strictly necessary because HS400 re-tuning should only be needed after runtime suspend when there would not be a conflict.

Nevertheless, this approach has more structure and potentially more flexibility.

Adrian Hunter (13):
mmc: core: Simplify by adding mmc_execute_tuning()
mmc: host: Add facility to support re-tuning
mmc: core: Disable re-tuning when card is no longer initialized
mmc: core: Move mmc_card_removed() into mmc_start_request()
mmc: core: Add support for re-tuning before each request
mmc: core: Check re-tuning before retrying
mmc: core: Hold re-tuning during switch commands
mmc: core: Hold re-tuning during erase commands
mmc: core: Hold re-tuning while bkops ongoing
mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
mmc: core: Add support for HS400 re-tuning
mmc: sdhci: Always init buf_ready_int
mmc: sdhci: Change to new way of doing re-tuning

 drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
 drivers/mmc/core/core.h    |   5 ++
 drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
 drivers/mmc/core/mmc_ops.c |  31 ++++++++----
 drivers/mmc/core/sd.c      |  13 ++---
 drivers/mmc/core/sdio.c    |  14 ++----
 drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 10 files changed, 296 insertions(+), 157 deletions(-)

Quoted 38 lines · Adrian Hunter

Hi

Here are some patches to move re-tuning support out of sdhci and into the core, and add support for HS400 re-tuning.

Currently sdhci does re-tuning transparently by calling sdhci_execute_tuning() from its ->request() function.

The problem with HS400 re-tuning is that it must be done in HS200 mode. That means using switch commands and making ios changes. That means it potentially conflicts with other command sequences. The new re-tuning support accomodates that, however it is not strictly necessary because HS400 re-tuning should only be needed after runtime suspend when there would not be a conflict.

Nevertheless, this approach has more structure and potentially more flexibility.

Adrian Hunter (13):
mmc: core: Simplify by adding mmc_execute_tuning()
mmc: host: Add facility to support re-tuning
mmc: core: Disable re-tuning when card is no longer initialized
mmc: core: Move mmc_card_removed() into mmc_start_request()
mmc: core: Add support for re-tuning before each request
mmc: core: Check re-tuning before retrying
mmc: core: Hold re-tuning during switch commands
mmc: core: Hold re-tuning during erase commands
mmc: core: Hold re-tuning while bkops ongoing
mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
mmc: core: Add support for HS400 re-tuning
mmc: sdhci: Always init buf_ready_int
mmc: sdhci: Change to new way of doing re-tuning

Quoted 11 lines

 drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
 drivers/mmc/core/core.h    |   5 ++
 drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
 drivers/mmc/core/mmc_ops.c |  31 ++++++++----
 drivers/mmc/core/sd.c      |  13 ++---
 drivers/mmc/core/sdio.c    |  14 ++----
 drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 10 files changed, 296 insertions(+), 157 deletions(-)

Ulf, do you have any comments?

Quoted 38 lines · Adrian Hunter

Hi

Here are some patches to move re-tuning support out of sdhci and into the core, and add support for HS400 re-tuning.

Currently sdhci does re-tuning transparently by calling sdhci_execute_tuning() from its ->request() function.

The problem with HS400 re-tuning is that it must be done in HS200 mode. That means using switch commands and making ios changes. That means it potentially conflicts with other command sequences. The new re-tuning support accomodates that, however it is not strictly necessary because HS400 re-tuning should only be needed after runtime suspend when there would not be a conflict.

Nevertheless, this approach has more structure and potentially more flexibility.

Adrian Hunter (13):
mmc: core: Simplify by adding mmc_execute_tuning()
mmc: host: Add facility to support re-tuning
mmc: core: Disable re-tuning when card is no longer initialized
mmc: core: Move mmc_card_removed() into mmc_start_request()
mmc: core: Add support for re-tuning before each request
mmc: core: Check re-tuning before retrying
mmc: core: Hold re-tuning during switch commands
mmc: core: Hold re-tuning during erase commands
mmc: core: Hold re-tuning while bkops ongoing
mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
mmc: core: Add support for HS400 re-tuning
mmc: sdhci: Always init buf_ready_int
mmc: sdhci: Change to new way of doing re-tuning

Quoted 11 lines

 drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
 drivers/mmc/core/core.h    |   5 ++
 drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
 drivers/mmc/core/mmc_ops.c |  31 ++++++++----
 drivers/mmc/core/sd.c      |  13 ++---
 drivers/mmc/core/sdio.c    |  14 ++----
 drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 10 files changed, 296 insertions(+), 157 deletions(-)

Ulf, do you have any comments?

Hi

Here are some patches to move re-tuning support out of sdhci and into the core, and add support for HS400 re-tuning.

Currently sdhci does re-tuning transparently by calling sdhci_execute_tuning() from its ->request() function.

The problem with HS400 re-tuning is that it must be done in HS200 mode. That means using switch commands and making ios changes. That means it potentially conflicts with other command sequences. The new re-tuning support accomodates that, however it is not strictly necessary because HS400 re-tuning should only be needed after runtime suspend when there would not be a conflict.

Nevertheless, this approach has more structure and potentially more flexibility.

Adrian Hunter (13):
mmc: core: Simplify by adding mmc_execute_tuning()
mmc: host: Add facility to support re-tuning
mmc: core: Disable re-tuning when card is no longer initialized
mmc: core: Move mmc_card_removed() into mmc_start_request()
mmc: core: Add support for re-tuning before each request
mmc: core: Check re-tuning before retrying
mmc: core: Hold re-tuning during switch commands
mmc: core: Hold re-tuning during erase commands
mmc: core: Hold re-tuning while bkops ongoing
mmc: mmc: Comment that callers need to hold re-tuning if the card is put to sleep
mmc: core: Add support for HS400 re-tuning
mmc: sdhci: Always init buf_ready_int
mmc: sdhci: Change to new way of doing re-tuning

 drivers/mmc/core/core.c    |  93 +++++++++++++++++++++++++++++++-----
 drivers/mmc/core/core.h    |   5 ++
 drivers/mmc/core/host.c    |  63 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c     |  58 ++++++++++++++++++-----
 drivers/mmc/core/mmc_ops.c |  31 ++++++++----
 drivers/mmc/core/sd.c      |  13 ++---
 drivers/mmc/core/sdio.c    |  14 ++----
 drivers/mmc/host/sdhci.c   | 115 ++++++---------------------------------------
 include/linux/mmc/host.h   |  58 +++++++++++++++++++++++
 include/linux/mmc/sdhci.h  |   3 --
 10 files changed, 296 insertions(+), 157 deletions(-)

Regards
Adrian

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html