| Message ID | 1462361893-11897-2-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: > Re-tuning is not possible when switched to the RPMB > partition. However re-tuning should not be needed > if re-tuning is done immediately before switching, > a small set of operations is done, and then we > immediately switch back to the main partition. > > To ensure that re-tuning can't be done for a short > while, add a facility to "pause" re-tuning. > > The existing facility to hold / release re-tuning > is used but it also flags re-tuning as needed to cause > re-tuning before the next command (which will be the > switch to RPMB). > > We also need to "unpause" in the recovery path, which > is catered for by adding it to mmc_retune_disable(). > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ > include/linux/mmc/host.h | 4 ++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index e0a3ee16c0d3..302e5858755a 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) > jiffies + host->retune_period * HZ); > } > > +/* > + * Pause re-tuning for a small set of operations. The pause begins after the > + * next command and after first doing re-tuning. > + */ > +void mmc_retune_pause(struct mmc_host *host) > +{ > + if (!host->retune_paused) { > + host->retune_paused = 1; > + mmc_retune_needed(host); > + mmc_retune_hold(host); > + } > +} > + When the mmc block device driver is built as a module, this doesn't build. I will drop the series from my next branch to sort this out. Should we export these via EXPORT_SYMBOL_GPL, or implement them as inline functions? This also made me think about the SDIO/WLAN driver issue, during system PM suspend/resume, which also needed temporary to disable re-tuning. *If* we are going to export these, I want to make it works for the SDIO case well... Kind regards Uffe > +void mmc_retune_unpause(struct mmc_host *host) > +{ > + if (host->retune_paused) { > + host->retune_paused = 0; > + mmc_retune_release(host); > + } > +} > + > void mmc_retune_disable(struct mmc_host *host) > { > + mmc_retune_unpause(host); > host->can_retune = 0; > del_timer_sync(&host->retune_timer); > host->retune_now = 0; > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 85800b48241f..45cde8cd39f2 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -329,6 +329,7 @@ struct mmc_host { > unsigned int can_retune:1; /* re-tuning can be used */ > unsigned int doing_retune:1; /* re-tuning in progress */ > unsigned int retune_now:1; /* do re-tuning at next req */ > + unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ > > int rescan_disable; /* disable card detection */ > int rescan_entered; /* used with nonremovable devices */ > @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host) > host->retune_now = 1; > } > > +void mmc_retune_pause(struct mmc_host *host); > +void mmc_retune_unpause(struct mmc_host *host); > + > #endif /* LINUX_MMC_HOST_H */ > -- > 1.9.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 10/05/16 15:24, Ulf Hansson wrote: > On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Re-tuning is not possible when switched to the RPMB >> partition. However re-tuning should not be needed >> if re-tuning is done immediately before switching, >> a small set of operations is done, and then we >> immediately switch back to the main partition. >> >> To ensure that re-tuning can't be done for a short >> while, add a facility to "pause" re-tuning. >> >> The existing facility to hold / release re-tuning >> is used but it also flags re-tuning as needed to cause >> re-tuning before the next command (which will be the >> switch to RPMB). >> >> We also need to "unpause" in the recovery path, which >> is catered for by adding it to mmc_retune_disable(). >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >> include/linux/mmc/host.h | 4 ++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >> index e0a3ee16c0d3..302e5858755a 100644 >> --- a/drivers/mmc/core/host.c >> +++ b/drivers/mmc/core/host.c >> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >> jiffies + host->retune_period * HZ); >> } >> >> +/* >> + * Pause re-tuning for a small set of operations. The pause begins after the >> + * next command and after first doing re-tuning. >> + */ >> +void mmc_retune_pause(struct mmc_host *host) >> +{ >> + if (!host->retune_paused) { >> + host->retune_paused = 1; >> + mmc_retune_needed(host); >> + mmc_retune_hold(host); >> + } >> +} >> + > > When the mmc block device driver is built as a module, this doesn't > build. I will drop the series from my next branch to sort this out. Oops. Sorry! > Should we export these via EXPORT_SYMBOL_GPL, or implement them as > inline functions? They need to be exported. I tend to go with what else is in the same file i.e. host.c is exporting using EXPORT_SYMBOL() > > This also made me think about the SDIO/WLAN driver issue, during > system PM suspend/resume, which also needed temporary to disable > re-tuning. > > *If* we are going to export these, I want to make it works for the > SDIO case well... SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. > > Kind regards > Uffe > >> +void mmc_retune_unpause(struct mmc_host *host) >> +{ >> + if (host->retune_paused) { >> + host->retune_paused = 0; >> + mmc_retune_release(host); >> + } >> +} >> + >> void mmc_retune_disable(struct mmc_host *host) >> { >> + mmc_retune_unpause(host); >> host->can_retune = 0; >> del_timer_sync(&host->retune_timer); >> host->retune_now = 0; >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 85800b48241f..45cde8cd39f2 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -329,6 +329,7 @@ struct mmc_host { >> unsigned int can_retune:1; /* re-tuning can be used */ >> unsigned int doing_retune:1; /* re-tuning in progress */ >> unsigned int retune_now:1; /* do re-tuning at next req */ >> + unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ >> >> int rescan_disable; /* disable card detection */ >> int rescan_entered; /* used with nonremovable devices */ >> @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host) >> host->retune_now = 1; >> } >> >> +void mmc_retune_pause(struct mmc_host *host); >> +void mmc_retune_unpause(struct mmc_host *host); >> + >> #endif /* LINUX_MMC_HOST_H */ >> -- >> 1.9.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 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 10/05/16 15:24, Ulf Hansson wrote: >> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> Re-tuning is not possible when switched to the RPMB >>> partition. However re-tuning should not be needed >>> if re-tuning is done immediately before switching, >>> a small set of operations is done, and then we >>> immediately switch back to the main partition. >>> >>> To ensure that re-tuning can't be done for a short >>> while, add a facility to "pause" re-tuning. >>> >>> The existing facility to hold / release re-tuning >>> is used but it also flags re-tuning as needed to cause >>> re-tuning before the next command (which will be the >>> switch to RPMB). >>> >>> We also need to "unpause" in the recovery path, which >>> is catered for by adding it to mmc_retune_disable(). >>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>> --- >>> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >>> include/linux/mmc/host.h | 4 ++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >>> index e0a3ee16c0d3..302e5858755a 100644 >>> --- a/drivers/mmc/core/host.c >>> +++ b/drivers/mmc/core/host.c >>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >>> jiffies + host->retune_period * HZ); >>> } >>> >>> +/* >>> + * Pause re-tuning for a small set of operations. The pause begins after the >>> + * next command and after first doing re-tuning. >>> + */ >>> +void mmc_retune_pause(struct mmc_host *host) >>> +{ >>> + if (!host->retune_paused) { >>> + host->retune_paused = 1; >>> + mmc_retune_needed(host); >>> + mmc_retune_hold(host); >>> + } >>> +} >>> + >> >> When the mmc block device driver is built as a module, this doesn't >> build. I will drop the series from my next branch to sort this out. > > Oops. Sorry! > >> Should we export these via EXPORT_SYMBOL_GPL, or implement them as >> inline functions? > > They need to be exported. I tend to go with what else is in the same file > i.e. host.c is exporting using EXPORT_SYMBOL() Yes, okay! > >> >> This also made me think about the SDIO/WLAN driver issue, during >> system PM suspend/resume, which also needed temporary to disable >> re-tuning. >> >> *If* we are going to export these, I want to make it works for the >> SDIO case well... > > SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. I what way is it different? Regarding the header file, my point is that I want to keep the numbers of exported functions to a minimum. Do you think there is way to combine these two use cases, such only one pair of new functions would be 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
On 11/05/16 09:48, Ulf Hansson wrote: > On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 10/05/16 15:24, Ulf Hansson wrote: >>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> Re-tuning is not possible when switched to the RPMB >>>> partition. However re-tuning should not be needed >>>> if re-tuning is done immediately before switching, >>>> a small set of operations is done, and then we >>>> immediately switch back to the main partition. >>>> >>>> To ensure that re-tuning can't be done for a short >>>> while, add a facility to "pause" re-tuning. >>>> >>>> The existing facility to hold / release re-tuning >>>> is used but it also flags re-tuning as needed to cause >>>> re-tuning before the next command (which will be the >>>> switch to RPMB). >>>> >>>> We also need to "unpause" in the recovery path, which >>>> is catered for by adding it to mmc_retune_disable(). >>>> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>> --- >>>> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >>>> include/linux/mmc/host.h | 4 ++++ >>>> 2 files changed, 26 insertions(+) >>>> >>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >>>> index e0a3ee16c0d3..302e5858755a 100644 >>>> --- a/drivers/mmc/core/host.c >>>> +++ b/drivers/mmc/core/host.c >>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >>>> jiffies + host->retune_period * HZ); >>>> } >>>> >>>> +/* >>>> + * Pause re-tuning for a small set of operations. The pause begins after the >>>> + * next command and after first doing re-tuning. >>>> + */ >>>> +void mmc_retune_pause(struct mmc_host *host) >>>> +{ >>>> + if (!host->retune_paused) { >>>> + host->retune_paused = 1; >>>> + mmc_retune_needed(host); >>>> + mmc_retune_hold(host); >>>> + } >>>> +} >>>> + >>> >>> When the mmc block device driver is built as a module, this doesn't >>> build. I will drop the series from my next branch to sort this out. >> >> Oops. Sorry! >> >>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as >>> inline functions? >> >> They need to be exported. I tend to go with what else is in the same file >> i.e. host.c is exporting using EXPORT_SYMBOL() > > Yes, okay! > >> >>> >>> This also made me think about the SDIO/WLAN driver issue, during >>> system PM suspend/resume, which also needed temporary to disable >>> re-tuning. >>> >>> *If* we are going to export these, I want to make it works for the >>> SDIO case well... >> >> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. > > I what way is it different? In the RPMB case there are 3 things to do: 1. Do re-tuning at next command 2. Hold re-tuning 3. Release re-tuning In the SDIO case there are 3 things to do: 1. Prevent re-tuning at next command 2. Hold re-tuning 3. Release re-tuning So the first thing is different. > > Regarding the header file, my point is that I want to keep the numbers > of exported functions to a minimum. > > Do you think there is way to combine these two use cases, such only > one pair of new functions would be needed? To make them the same we would need to add a parameter to mmc_retune_pause() i.e. something like void mmc_retune_pause(struct mmc_host *host, bool retune_now) { if (!host->retune_paused) { host->retune_paused = 1; mmc_retune_hold(host); if (retune_now) mmc_retune_needed(host); else host->retune_now = 0; } } For SDIO we would need to put the function declarations in sdio_func.h as well as host.h. Shall I make a V3 of these patches like that? -- 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 11/05/16 12:00, Adrian Hunter wrote: > On 11/05/16 09:48, Ulf Hansson wrote: >> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 10/05/16 15:24, Ulf Hansson wrote: >>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> Re-tuning is not possible when switched to the RPMB >>>>> partition. However re-tuning should not be needed >>>>> if re-tuning is done immediately before switching, >>>>> a small set of operations is done, and then we >>>>> immediately switch back to the main partition. >>>>> >>>>> To ensure that re-tuning can't be done for a short >>>>> while, add a facility to "pause" re-tuning. >>>>> >>>>> The existing facility to hold / release re-tuning >>>>> is used but it also flags re-tuning as needed to cause >>>>> re-tuning before the next command (which will be the >>>>> switch to RPMB). >>>>> >>>>> We also need to "unpause" in the recovery path, which >>>>> is catered for by adding it to mmc_retune_disable(). >>>>> >>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>>> --- >>>>> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >>>>> include/linux/mmc/host.h | 4 ++++ >>>>> 2 files changed, 26 insertions(+) >>>>> >>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >>>>> index e0a3ee16c0d3..302e5858755a 100644 >>>>> --- a/drivers/mmc/core/host.c >>>>> +++ b/drivers/mmc/core/host.c >>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >>>>> jiffies + host->retune_period * HZ); >>>>> } >>>>> >>>>> +/* >>>>> + * Pause re-tuning for a small set of operations. The pause begins after the >>>>> + * next command and after first doing re-tuning. >>>>> + */ >>>>> +void mmc_retune_pause(struct mmc_host *host) >>>>> +{ >>>>> + if (!host->retune_paused) { >>>>> + host->retune_paused = 1; >>>>> + mmc_retune_needed(host); >>>>> + mmc_retune_hold(host); >>>>> + } >>>>> +} >>>>> + >>>> >>>> When the mmc block device driver is built as a module, this doesn't >>>> build. I will drop the series from my next branch to sort this out. >>> >>> Oops. Sorry! >>> >>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as >>>> inline functions? >>> >>> They need to be exported. I tend to go with what else is in the same file >>> i.e. host.c is exporting using EXPORT_SYMBOL() >> >> Yes, okay! >> >>> >>>> >>>> This also made me think about the SDIO/WLAN driver issue, during >>>> system PM suspend/resume, which also needed temporary to disable >>>> re-tuning. >>>> >>>> *If* we are going to export these, I want to make it works for the >>>> SDIO case well... >>> >>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. >> >> I what way is it different? > > In the RPMB case there are 3 things to do: > 1. Do re-tuning at next command > 2. Hold re-tuning > 3. Release re-tuning > > In the SDIO case there are 3 things to do: > 1. Prevent re-tuning at next command > 2. Hold re-tuning > 3. Release re-tuning > > So the first thing is different. > >> >> Regarding the header file, my point is that I want to keep the numbers >> of exported functions to a minimum. >> >> Do you think there is way to combine these two use cases, such only >> one pair of new functions would be needed? > > To make them the same we would need to add a parameter to mmc_retune_pause() > i.e. something like > > void mmc_retune_pause(struct mmc_host *host, bool retune_now) > { > if (!host->retune_paused) { > host->retune_paused = 1; > mmc_retune_hold(host); > if (retune_now) > mmc_retune_needed(host); > else > host->retune_now = 0; > } > } > > For SDIO we would need to put the function declarations in sdio_func.h as > well as host.h. > > Shall I make a V3 of these patches like that? I looked again at sdio_func.h and it seems to have its own paradigm i.e. it is a completely separate set of functions that take the SDIO function as a parameter, and that hide and encapsulate core and host functions. It would be inconsistent with that paradigm to expose mmc_retune_pause() and mmc_retune_unpause() there. Is that what you want to do? -- 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 12/05/16 16:20, Ulf Hansson wrote: > On 12 May 2016 at 08:14, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 11/05/16 12:00, Adrian Hunter wrote: >>> On 11/05/16 09:48, Ulf Hansson wrote: >>>> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> On 10/05/16 15:24, Ulf Hansson wrote: >>>>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>> Re-tuning is not possible when switched to the RPMB >>>>>>> partition. However re-tuning should not be needed >>>>>>> if re-tuning is done immediately before switching, >>>>>>> a small set of operations is done, and then we >>>>>>> immediately switch back to the main partition. >>>>>>> >>>>>>> To ensure that re-tuning can't be done for a short >>>>>>> while, add a facility to "pause" re-tuning. >>>>>>> >>>>>>> The existing facility to hold / release re-tuning >>>>>>> is used but it also flags re-tuning as needed to cause >>>>>>> re-tuning before the next command (which will be the >>>>>>> switch to RPMB). >>>>>>> >>>>>>> We also need to "unpause" in the recovery path, which >>>>>>> is catered for by adding it to mmc_retune_disable(). >>>>>>> >>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>>>>> --- >>>>>>> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >>>>>>> include/linux/mmc/host.h | 4 ++++ >>>>>>> 2 files changed, 26 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >>>>>>> index e0a3ee16c0d3..302e5858755a 100644 >>>>>>> --- a/drivers/mmc/core/host.c >>>>>>> +++ b/drivers/mmc/core/host.c >>>>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >>>>>>> jiffies + host->retune_period * HZ); >>>>>>> } >>>>>>> >>>>>>> +/* >>>>>>> + * Pause re-tuning for a small set of operations. The pause begins after the >>>>>>> + * next command and after first doing re-tuning. >>>>>>> + */ >>>>>>> +void mmc_retune_pause(struct mmc_host *host) >>>>>>> +{ >>>>>>> + if (!host->retune_paused) { >>>>>>> + host->retune_paused = 1; >>>>>>> + mmc_retune_needed(host); >>>>>>> + mmc_retune_hold(host); >>>>>>> + } >>>>>>> +} >>>>>>> + >>>>>> >>>>>> When the mmc block device driver is built as a module, this doesn't >>>>>> build. I will drop the series from my next branch to sort this out. >>>>> >>>>> Oops. Sorry! >>>>> >>>>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as >>>>>> inline functions? >>>>> >>>>> They need to be exported. I tend to go with what else is in the same file >>>>> i.e. host.c is exporting using EXPORT_SYMBOL() >>>> >>>> Yes, okay! >>>> >>>>> >>>>>> >>>>>> This also made me think about the SDIO/WLAN driver issue, during >>>>>> system PM suspend/resume, which also needed temporary to disable >>>>>> re-tuning. >>>>>> >>>>>> *If* we are going to export these, I want to make it works for the >>>>>> SDIO case well... >>>>> >>>>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. >>>> >>>> I what way is it different? >>> >>> In the RPMB case there are 3 things to do: >>> 1. Do re-tuning at next command >>> 2. Hold re-tuning >>> 3. Release re-tuning >>> >>> In the SDIO case there are 3 things to do: >>> 1. Prevent re-tuning at next command >>> 2. Hold re-tuning >>> 3. Release re-tuning >>> >>> So the first thing is different. >>> >>>> >>>> Regarding the header file, my point is that I want to keep the numbers >>>> of exported functions to a minimum. >>>> >>>> Do you think there is way to combine these two use cases, such only >>>> one pair of new functions would be needed? >>> >>> To make them the same we would need to add a parameter to mmc_retune_pause() >>> i.e. something like >>> >>> void mmc_retune_pause(struct mmc_host *host, bool retune_now) >>> { >>> if (!host->retune_paused) { >>> host->retune_paused = 1; >>> mmc_retune_hold(host); >>> if (retune_now) >>> mmc_retune_needed(host); >>> else >>> host->retune_now = 0; >>> } >>> } >>> >>> For SDIO we would need to put the function declarations in sdio_func.h as >>> well as host.h. >>> >>> Shall I make a V3 of these patches like that? > > No. > >> >> I looked again at sdio_func.h and it seems to have its own paradigm i.e. it >> is a completely separate set of functions that take the SDIO function as a >> parameter, and that hide and encapsulate core and host functions. >> >> It would be inconsistent with that paradigm to expose mmc_retune_pause() and >> mmc_retune_unpause() there. Is that what you want to do? > > I agree, we shouldn't mess up the SDIO API with these functions. > > Instead, let's keep it simple and just leave out the SDIO case for > now. So do EXPORT_SYMBOL for those APIs you added in $subject patch, > without further changes. > > Okay? Yes please :-) -- 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 12 May 2016 at 08:14, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 11/05/16 12:00, Adrian Hunter wrote: >> On 11/05/16 09:48, Ulf Hansson wrote: >>> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> On 10/05/16 15:24, Ulf Hansson wrote: >>>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>> Re-tuning is not possible when switched to the RPMB >>>>>> partition. However re-tuning should not be needed >>>>>> if re-tuning is done immediately before switching, >>>>>> a small set of operations is done, and then we >>>>>> immediately switch back to the main partition. >>>>>> >>>>>> To ensure that re-tuning can't be done for a short >>>>>> while, add a facility to "pause" re-tuning. >>>>>> >>>>>> The existing facility to hold / release re-tuning >>>>>> is used but it also flags re-tuning as needed to cause >>>>>> re-tuning before the next command (which will be the >>>>>> switch to RPMB). >>>>>> >>>>>> We also need to "unpause" in the recovery path, which >>>>>> is catered for by adding it to mmc_retune_disable(). >>>>>> >>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>>>> --- >>>>>> drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ >>>>>> include/linux/mmc/host.h | 4 ++++ >>>>>> 2 files changed, 26 insertions(+) >>>>>> >>>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >>>>>> index e0a3ee16c0d3..302e5858755a 100644 >>>>>> --- a/drivers/mmc/core/host.c >>>>>> +++ b/drivers/mmc/core/host.c >>>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) >>>>>> jiffies + host->retune_period * HZ); >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * Pause re-tuning for a small set of operations. The pause begins after the >>>>>> + * next command and after first doing re-tuning. >>>>>> + */ >>>>>> +void mmc_retune_pause(struct mmc_host *host) >>>>>> +{ >>>>>> + if (!host->retune_paused) { >>>>>> + host->retune_paused = 1; >>>>>> + mmc_retune_needed(host); >>>>>> + mmc_retune_hold(host); >>>>>> + } >>>>>> +} >>>>>> + >>>>> >>>>> When the mmc block device driver is built as a module, this doesn't >>>>> build. I will drop the series from my next branch to sort this out. >>>> >>>> Oops. Sorry! >>>> >>>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as >>>>> inline functions? >>>> >>>> They need to be exported. I tend to go with what else is in the same file >>>> i.e. host.c is exporting using EXPORT_SYMBOL() >>> >>> Yes, okay! >>> >>>> >>>>> >>>>> This also made me think about the SDIO/WLAN driver issue, during >>>>> system PM suspend/resume, which also needed temporary to disable >>>>> re-tuning. >>>>> >>>>> *If* we are going to export these, I want to make it works for the >>>>> SDIO case well... >>>> >>>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h. >>> >>> I what way is it different? >> >> In the RPMB case there are 3 things to do: >> 1. Do re-tuning at next command >> 2. Hold re-tuning >> 3. Release re-tuning >> >> In the SDIO case there are 3 things to do: >> 1. Prevent re-tuning at next command >> 2. Hold re-tuning >> 3. Release re-tuning >> >> So the first thing is different. >> >>> >>> Regarding the header file, my point is that I want to keep the numbers >>> of exported functions to a minimum. >>> >>> Do you think there is way to combine these two use cases, such only >>> one pair of new functions would be needed? >> >> To make them the same we would need to add a parameter to mmc_retune_pause() >> i.e. something like >> >> void mmc_retune_pause(struct mmc_host *host, bool retune_now) >> { >> if (!host->retune_paused) { >> host->retune_paused = 1; >> mmc_retune_hold(host); >> if (retune_now) >> mmc_retune_needed(host); >> else >> host->retune_now = 0; >> } >> } >> >> For SDIO we would need to put the function declarations in sdio_func.h as >> well as host.h. >> >> Shall I make a V3 of these patches like that? No. > > I looked again at sdio_func.h and it seems to have its own paradigm i.e. it > is a completely separate set of functions that take the SDIO function as a > parameter, and that hide and encapsulate core and host functions. > > It would be inconsistent with that paradigm to expose mmc_retune_pause() and > mmc_retune_unpause() there. Is that what you want to do? I agree, we shouldn't mess up the SDIO API with these functions. Instead, let's keep it simple and just leave out the SDIO case for now. So do EXPORT_SYMBOL for those APIs you added in $subject patch, without further changes. 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
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index e0a3ee16c0d3..302e5858755a 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host) jiffies + host->retune_period * HZ); } +/* + * Pause re-tuning for a small set of operations. The pause begins after the + * next command and after first doing re-tuning. + */ +void mmc_retune_pause(struct mmc_host *host) +{ + if (!host->retune_paused) { + host->retune_paused = 1; + mmc_retune_needed(host); + mmc_retune_hold(host); + } +} + +void mmc_retune_unpause(struct mmc_host *host) +{ + if (host->retune_paused) { + host->retune_paused = 0; + mmc_retune_release(host); + } +} + void mmc_retune_disable(struct mmc_host *host) { + mmc_retune_unpause(host); host->can_retune = 0; del_timer_sync(&host->retune_timer); host->retune_now = 0; diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 85800b48241f..45cde8cd39f2 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -329,6 +329,7 @@ struct mmc_host { unsigned int can_retune:1; /* re-tuning can be used */ unsigned int doing_retune:1; /* re-tuning in progress */ unsigned int retune_now:1; /* do re-tuning at next req */ + unsigned int retune_paused:1; /* re-tuning is temporarily disabled */ int rescan_disable; /* disable card detection */ int rescan_entered; /* used with nonremovable devices */ @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host) host->retune_now = 1; } +void mmc_retune_pause(struct mmc_host *host); +void mmc_retune_unpause(struct mmc_host *host); + #endif /* LINUX_MMC_HOST_H */
Re-tuning is not possible when switched to the RPMB partition. However re-tuning should not be needed if re-tuning is done immediately before switching, a small set of operations is done, and then we immediately switch back to the main partition. To ensure that re-tuning can't be done for a short while, add a facility to "pause" re-tuning. The existing facility to hold / release re-tuning is used but it also flags re-tuning as needed to cause re-tuning before the next command (which will be the switch to RPMB). We also need to "unpause" in the recovery path, which is catered for by adding it to mmc_retune_disable(). Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/host.c | 22 ++++++++++++++++++++++ include/linux/mmc/host.h | 4 ++++ 2 files changed, 26 insertions(+)