| Message ID | 1417801271-15575-3-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Adrian, Thanks for working on this and apologize for my late reply! On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote: > 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 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. Kind regards Uffe > > 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 <adrian.hunter@intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/01/15 13:25, Ulf Hansson wrote: > Hi Adrian, > > Thanks for working on this and apologize for my late reply! > > On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote: >> 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 > > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 January 2015 at 14:23, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 13/01/15 13:25, Ulf Hansson wrote: >> Hi Adrian, >> >> Thanks for working on this and apologize for my late reply! >> >> On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> 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 >> >> 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. > > 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? > > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/01/15 16:22, Ulf Hansson wrote: > On 13 January 2015 at 14:23, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 13/01/15 13:25, Ulf Hansson wrote: >>> Hi Adrian, >>> >>> Thanks for working on this and apologize for my late reply! >>> >>> On 5 December 2014 at 18:41, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> 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 >>> >>> 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. >> >> 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. > > Can sdhci rely on that the mmc core performs a re-tune from the > request retry mechanism instead? Not according to the standard. > >> >> 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. > >> >> 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? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >>> 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. Does the spec say what value the timer should have? > >> >> 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. > >> >>> >>> 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. > >> >>> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/13/15 15:22, Ulf Hansson wrote: > On 13 January 2015 at 14:23, Adrian Hunter<adrian.hunter@intel.com> wrote: >> On 13/01/15 13:25, Ulf Hansson wrote: >>> Hi Adrian, >>> >>> Thanks for working on this and apologize for my late reply! >>> >>> On 5 December 2014 at 18:41, Adrian Hunter<adrian.hunter@intel.com> wrote: >>>> 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 >>> >>> 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. >> >> 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 good for complying with the SD Host controller spec. In section "2.2.25 Capabilities Register (pg. 74) the different "Re-Tuning Modes" are described. Guess these different should be taken into account in this patch series although in sdhci only the retuning timer was supported. Regards, Arend > Can sdhci rely on that the mmc core performs a re-tune from the > request retry mechanism instead? > >> >> 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. > >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/13/15 15:56, Ulf Hansson wrote: > [...] > >>>> 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. > > 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 >> >>> >>> 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. > >> >>> >>>> >>>> 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. > >> >>> >>>> >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 January 2015 at 16:11, Arend van Spriel <arend@broadcom.com> wrote: > On 01/13/15 15:56, Ulf Hansson wrote: >> >> [...] >> >>>>> 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. >> >> >> 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/13/15 16:41, Ulf Hansson wrote: > On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >> On 01/13/15 15:56, Ulf Hansson wrote: >>> >>> [...] >>> >>>>>> 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. >>> >>> >>> 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 > > 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. Regards, Arend [1] https://www.sdcard.org/downloads/pls/ > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote: > On 01/13/15 16:41, Ulf Hansson wrote: >> >> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >>> >>> On 01/13/15 15:56, Ulf Hansson wrote: >>>> >>>> >>>> [...] >>>> >>>>>>> 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. >>>> >>>> >>>> >>>> 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 >> >> >> 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. Arend, thanks for the link and information. So, I decided to go for a look in there. 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. It simply says that it's recommended to do a periodic re-tuning at some times, which is also stated by the SD and eMMC specs. Thus, to find a decent value for the timer, the mmc core would have to collect statistics for how data is read/written to the card to anticipate the heat. I don't think that's an effort that justifies its need. That leaves us with these options: 1) Use a timer with a random selected value. 2) Perform a re-tune at runtime PM suspend or resume (of the mmc card). 3) While catching request errors (like CRC), perform a re-tune in the request retry path. Now, since we don't have any statistics available for how often a re-tuning actually would be needed, let's first try out option 3) to see if that's enough. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/01/15 11:47, Ulf Hansson wrote: > On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote: >> On 01/13/15 16:41, Ulf Hansson wrote: >>> >>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >>>> >>>> On 01/13/15 15:56, Ulf Hansson wrote: >>>>> >>>>> >>>>> [...] >>>>> >>>>>>>> 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. >>>>> >>>>> >>>>> >>>>> 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 >>> >>> >>> 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. > > Arend, thanks for the link and information. So, I decided to go for a > look in there. > >>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. 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" -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 January 2015 at 10:57, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 14/01/15 11:47, Ulf Hansson wrote: >> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote: >>> On 01/13/15 16:41, Ulf Hansson wrote: >>>> >>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >>>>> >>>>> On 01/13/15 15:56, Ulf Hansson wrote: >>>>>> >>>>>> >>>>>> [...] >>>>>> >>>>>>>>> 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. >>>>>> >>>>>> >>>>>> >>>>>> 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 >>>> >>>> >>>> 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. >> >> Arend, thanks for the link and information. So, I decided to go for a >> look in there. >> >>>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. > > 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" > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/01/15 12:13, Ulf Hansson wrote: > On 14 January 2015 at 10:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 14/01/15 11:47, Ulf Hansson wrote: >>> On 13 January 2015 at 17:02, Arend van Spriel <arend@broadcom.com> wrote: >>>> On 01/13/15 16:41, Ulf Hansson wrote: >>>>> >>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >>>>>> >>>>>> On 01/13/15 15:56, Ulf Hansson wrote: >>>>>>> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> >>>>>>> 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 >>>>> >>>>> >>>>> 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. >>> >>> Arend, thanks for the link and information. So, I decided to go for a >>> look in there. >>> >>> >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. >> >> 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" >> > > 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. > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/14/15 11:13, Ulf Hansson wrote: > On 14 January 2015 at 10:57, Adrian Hunter<adrian.hunter@intel.com> wrote: >> On 14/01/15 11:47, Ulf Hansson wrote: >>> On 13 January 2015 at 17:02, Arend van Spriel<arend@broadcom.com> wrote: >>>> On 01/13/15 16:41, Ulf Hansson wrote: >>>>> >>>>> On 13 January 2015 at 16:11, Arend van Spriel<arend@broadcom.com> wrote: >>>>>> >>>>>> On 01/13/15 15:56, Ulf Hansson wrote: >>>>>>> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> >>>>>>> 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 >>>>> >>>>> >>>>> 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. >>> >>> Arend, thanks for the link and information. So, I decided to go for a >>> look in there. >>> >>> > 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. >>> ------ Hi Ulf, After sending my email I read that part as well and figured my response was incorrect. >>> 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" >> > > 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. > 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. > 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. Regards, Arend > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > > Hi Ulf, > > After sending my email I read that part as well and figured my response was > incorrect. > >>>> 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" >>> >> >> 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. How can they pre-validate use-cases? They don't have any clue of how the card is going to be exercised on a real SOC. It can't be more than just a guess. > >> 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. > >> 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. Correct. It actually already implemented as part of $subject patchset. > 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. It will introduce a delay/latency, but only when it's actually needed to do a re-tune. With the timer, it will add a latency at a random point in time, depending on the selected value for it. More importantly, user the timer means we will potentially insert latencies, when in fact the re-tune wasn't needed. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> >> 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. > >> >> 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. > 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. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/01/15 14:59, Ulf Hansson wrote: > [...] > >>> >>> 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. >> >>> >>> 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. >> > > 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? > > 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. I have cc'ed the author of "mmc: sdhci: add support for retuning mode 1" -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 January 2015 at 11:17, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 14/01/15 14:59, Ulf Hansson wrote: >> [...] >> >>>> >>>> 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. >>> >>>> >>>> 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. >>> >> >> 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? > >> >> 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/15/15 14:39, Ulf Hansson wrote: > On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> wrote: >> On 14/01/15 14:59, Ulf Hansson wrote: >>> [...] >>> >>>>> >>>>> 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. >>>> >>>>> >>>>> 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. >>>> >>> >>> 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? 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 >> >>> >>> 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/15/15 15:07, Arend van Spriel wrote: > On 01/15/15 14:39, Ulf Hansson wrote: >> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> >> wrote: >>> On 14/01/15 14:59, Ulf Hansson wrote: >>>> [...] >>>> >>>>>> >>>>>> 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. >>>>> >>>>>> >>>>>> 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. >>>>> >>>> >>>> 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? > > 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. Could leave it to the function driver to call mmc_retune_needed(). Regards, Arend > Gr. AvS > >>> >>>> >>>> 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. >> >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 January 2015 at 15:17, Arend van Spriel <arend@broadcom.com> wrote: > On 01/15/15 15:07, Arend van Spriel wrote: >> >> On 01/15/15 14:39, Ulf Hansson wrote: >>> >>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> >>> wrote: >>>> >>>> On 14/01/15 14:59, Ulf Hansson wrote: >>>>> >>>>> [...] >>>>> >>>>>>> >>>>>>> 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. >>>>>> >>>>>>> >>>>>>> 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. >>>>>> >>>>> >>>>> 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? >> >> >> 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. > 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/15/15 15:46, Ulf Hansson wrote: > On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com> wrote: >> On 01/15/15 15:07, Arend van Spriel wrote: >>> >>> On 01/15/15 14:39, Ulf Hansson wrote: >>>> >>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> >>>> wrote: >>>>> >>>>> On 14/01/15 14:59, Ulf Hansson wrote: >>>>>> >>>>>> [...] >>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>> >>>>>> 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? >>> >>> >>> 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. >> > > 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. > >> >> 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". > 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. Regards, Arend > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 January 2015 at 15:59, Arend van Spriel <arend@broadcom.com> wrote: > On 01/15/15 15:46, Ulf Hansson wrote: >> >> On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com> wrote: >>> >>> On 01/15/15 15:07, Arend van Spriel wrote: >>>> >>>> >>>> On 01/15/15 14:39, Ulf Hansson wrote: >>>>> >>>>> >>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 14/01/15 14:59, Ulf Hansson wrote: >>>>>>> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> 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? >>>> >>>> >>>> >>>> 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. >>> >>> >> >> 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. >> >>> >>> 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". > >> 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. Okay, your argument seems reasonable, let's got for this approach. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/01/15 11:27, Ulf Hansson wrote: > On 15 January 2015 at 15:59, Arend van Spriel <arend@broadcom.com> wrote: >> On 01/15/15 15:46, Ulf Hansson wrote: >>> >>> On 15 January 2015 at 15:17, Arend van Spriel<arend@broadcom.com> wrote: >>>> >>>> On 01/15/15 15:07, Arend van Spriel wrote: >>>>> >>>>> >>>>> On 01/15/15 14:39, Ulf Hansson wrote: >>>>>> >>>>>> >>>>>> On 15 January 2015 at 11:17, Adrian Hunter<adrian.hunter@intel.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On 14/01/15 14:59, Ulf Hansson wrote: >>>>>>>> >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> >>>>>>>> >>>>>>>> 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? >>>>> >>>>> >>>>> >>>>> 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. >>>> >>>> >>> >>> 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. >>> >>>> >>>> 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". >> >>> 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. > > Okay, your argument seems reasonable, let's got for this approach. A re-tune is needed when there is a CRC error. That should be a low level decision because it is needed no matter if it is a SDIO function driver or eMMC block driver. There is a mechanism to hold-off re-tuning if it would cause a problem. Otherwise re-tuning should be done immediately. Remember we are already in an error condition, which must be rare to non-existent for the device to perform reasonably. The decision of the upper layers is when to retry. My thought for the block driver was that it would indicate that it was ok to transparently re-try if re-tuning was needed. Then the core would do the re-try. A complication is the need to retry just once not get stuck error->retry->error->retry->... -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index 270d58a..a4aa25b 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -297,6 +297,51 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host) #endif +void mmc_retune_enable(struct mmc_host *host, unsigned int period) +{ + host->can_retune = 1; + if (period) + mod_timer(&host->retune_timer, jiffies + period * HZ); +} + +void mmc_retune_disable(struct mmc_host *host) +{ + host->can_retune = 0; + del_timer_sync(&host->retune_timer); + host->need_retune = 0; +} + +void mmc_retune_timer_stop(struct mmc_host *host) +{ + del_timer_sync(&host->retune_timer); +} + +int mmc_retune(struct mmc_host *host) +{ + int err; + + if (!host->need_retune || host->doing_retune || host->hold_retune || + !host->card) + return 0; + + host->need_retune = 0; + + host->doing_retune = 1; + + err = mmc_execute_tuning(host->card); + + host->doing_retune = 0; + + return err; +} + +static void mmc_retune_timer(unsigned long data) +{ + struct mmc_host *host = (struct mmc_host *)data; + + mmc_retune_needed(host); +} + /** * mmc_of_parse() - parse host's device-tree node * @host: host whose node should be parsed. @@ -512,6 +557,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev) #ifdef CONFIG_PM host->pm_notify.notifier_call = mmc_pm_notify; #endif + setup_timer(&host->retune_timer, mmc_retune_timer, (unsigned long)host); /* * By default, hosts do not support SGIO or large requests. diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 9f32270..bfbe749 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -12,6 +12,7 @@ #include <linux/leds.h> #include <linux/mutex.h> +#include <linux/timer.h> #include <linux/sched.h> #include <linux/device.h> #include <linux/fault-inject.h> @@ -327,10 +328,16 @@ struct mmc_host { #ifdef CONFIG_MMC_DEBUG unsigned int removed:1; /* host is being removed */ #endif + unsigned int can_retune:1; /* re-tuning can be used */ + unsigned int doing_retune:1; /* re-tuning in progress */ int rescan_disable; /* disable card detection */ int rescan_entered; /* used with nonremovable devices */ + int need_retune; /* re-tuning is needed */ + int hold_retune; /* hold off re-tuning */ + struct timer_list retune_timer; /* for periodic re-tuning */ + bool trigger_card_event; /* card_event necessary */ struct mmc_card *card; /* device attached to this host */ @@ -519,4 +526,55 @@ static inline bool mmc_card_hs400(struct mmc_card *card) return card->host->ios.timing == MMC_TIMING_MMC_HS400; } +void mmc_retune_enable(struct mmc_host *host, unsigned int period); +void mmc_retune_disable(struct mmc_host *host); +void mmc_retune_timer_stop(struct mmc_host *host); +int mmc_retune(struct mmc_host *host); + +static inline void mmc_retune_needed(struct mmc_host *host) +{ + if (host->can_retune) + host->need_retune = 1; +} + +static inline void mmc_retune_not_needed(struct mmc_host *host) +{ + host->need_retune = 0; +} + +static inline void mmc_retune_hold(struct mmc_host *host) +{ + host->hold_retune += 1; +} + +static inline void mmc_retune_release(struct mmc_host *host) +{ + if (host->hold_retune) + host->hold_retune -= 1; + else + WARN_ON(1); +} + +static inline int mmc_retune_and_hold(struct mmc_host *host) +{ + int err; + + err = mmc_retune(host); + if (!err) + mmc_retune_hold(host); + + return err; +} + +static inline int mmc_retune_retry(struct mmc_host *host) +{ + int err; + + mmc_retune_release(host); + err = mmc_retune(host); + mmc_retune_hold(host); + + return err; +} + #endif /* LINUX_MMC_HOST_H */
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 <adrian.hunter@intel.com> --- drivers/mmc/core/host.c | 46 ++++++++++++++++++++++++++++++++++++++ include/linux/mmc/host.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+)