| Message ID | 20220125055010.1866563-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v6,1/4] mmc: rtsx: Use pm_runtime_{get,put}() to handle runtime PM | expand |
> -----Original Message----- > From: Kai-Heng Feng <kai.heng.feng@canonical.com> > Sent: Tuesday, January 25, 2022 1:50 PM > To: arnd@arndb.de; gregkh@linuxfoundation.org; ulf.hansson@linaro.org > Cc: linux-pm@vger.kernel.org; Kai-Heng Feng <kai.heng.feng@canonical.com>; > Ricky WU <ricky_wu@realtek.com>; Thomas Hebb <tommyhebb@gmail.com>; > linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH v6 1/4] mmc: rtsx: Use pm_runtime_{get,put}() to handle > runtime PM > > Commit 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") doesn't use > pm_runtime_{get,put}() helpers when it should, so the RPM refcount keeps at > zero, hence its parent driver, rtsx_pci, has to do lots of weird tricks to keep it > from runtime suspending. > > So use those helpers at right places to properly manage runtime PM. > > Fixes: 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") > Cc: Ricky WU <ricky_wu@realtek.com> Tested-by: Ricky WU <ricky_wu@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > v6: > - Change the auto suspend delay to 200ms as Realtek suggested. > > v5: > - Revert back to v3 as Realtek suggested. > > v4: > - Decrease the autosuspend delay to 200ms for more power saving. > > v3: > v2: > - No change. > > drivers/mmc/host/rtsx_pci_sdmmc.c | 44 +++++++++++++++++++++++-------- > 1 file changed, 33 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > b/drivers/mmc/host/rtsx_pci_sdmmc.c > index 58cfaffa3c2d8..2a3f14afe9f83 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -806,6 +806,7 @@ static void sd_request(struct work_struct *work) > struct mmc_request *mrq = host->mrq; > struct mmc_command *cmd = mrq->cmd; > struct mmc_data *data = mrq->data; > + struct device *dev = &host->pdev->dev; > > unsigned int data_size = 0; > int err; > @@ -822,6 +823,7 @@ static void sd_request(struct work_struct *work) > } > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -858,6 +860,8 @@ static void sd_request(struct work_struct *work) > data->bytes_xfered = data->blocks * data->blksz; > } > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > > finish: > @@ -1080,6 +1084,7 @@ static void sdmmc_set_ios(struct mmc_host *mmc, > struct mmc_ios *ios) { > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > + struct device *dev = &host->pdev->dev; > > if (host->eject) > return; > @@ -1088,6 +1093,7 @@ static void sdmmc_set_ios(struct mmc_host *mmc, > struct mmc_ios *ios) > return; > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -1121,6 +1127,8 @@ static void sdmmc_set_ios(struct mmc_host *mmc, > struct mmc_ios *ios) > rtsx_pci_switch_clock(pcr, ios->clock, host->ssc_depth, > host->initial_mode, host->double_clk, host->vpclk); > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > } > > @@ -1128,6 +1136,7 @@ static int sdmmc_get_ro(struct mmc_host *mmc) > { > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > + struct device *dev = &host->pdev->dev; > int ro = 0; > u32 val; > > @@ -1135,6 +1144,7 @@ static int sdmmc_get_ro(struct mmc_host *mmc) > return -ENOMEDIUM; > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -1144,6 +1154,8 @@ static int sdmmc_get_ro(struct mmc_host *mmc) > if (val & SD_WRITE_PROTECT) > ro = 1; > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > > return ro; > @@ -1153,6 +1165,7 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > { > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > + struct device *dev = &host->pdev->dev; > int cd = 0; > u32 val; > > @@ -1160,6 +1173,7 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > return cd; > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -1169,6 +1183,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) > if (val & SD_EXIST) > cd = 1; > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > > return cd; > @@ -1251,6 +1267,7 @@ static int sdmmc_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) { > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > + struct device *dev = &host->pdev->dev; > int err = 0; > u8 voltage; > > @@ -1265,6 +1282,7 @@ static int sdmmc_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) > return err; > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -1294,6 +1312,8 @@ static int sdmmc_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) > err = rtsx_pci_write_register(pcr, SD_BUS_STAT, > SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0); > > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > > return err; > @@ -1303,6 +1323,7 @@ static int sdmmc_execute_tuning(struct mmc_host > *mmc, u32 opcode) { > struct realtek_pci_sdmmc *host = mmc_priv(mmc); > struct rtsx_pcr *pcr = host->pcr; > + struct device *dev = &host->pdev->dev; > int err = 0; > > if (host->eject) > @@ -1313,6 +1334,7 @@ static int sdmmc_execute_tuning(struct mmc_host > *mmc, u32 opcode) > return err; > > mutex_lock(&pcr->pcr_mutex); > + pm_runtime_get_sync(dev); > > rtsx_pci_start_run(pcr); > > @@ -1345,6 +1367,8 @@ static int sdmmc_execute_tuning(struct mmc_host > *mmc, u32 opcode) > err = sd_change_phase(host, DDR50_RX_PHASE(pcr), true); > > out: > + pm_runtime_mark_last_busy(dev); > + pm_runtime_put_autosuspend(dev); > mutex_unlock(&pcr->pcr_mutex); > > return err; > @@ -1495,12 +1519,12 @@ static int rtsx_pci_sdmmc_drv_probe(struct > platform_device *pdev) > > realtek_init_host(host); > > - if (pcr->rtd3_en) { > - pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); > - pm_runtime_use_autosuspend(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > - } > - > + pm_runtime_no_callbacks(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 200); > + pm_runtime_mark_last_busy(&pdev->dev); > + pm_runtime_use_autosuspend(&pdev->dev); > > mmc_add_host(mmc); > > @@ -1521,11 +1545,6 @@ static int rtsx_pci_sdmmc_drv_remove(struct > platform_device *pdev) > pcr->slots[RTSX_SD_CARD].card_event = NULL; > mmc = host->mmc; > > - if (pcr->rtd3_en) { > - pm_runtime_dont_use_autosuspend(&pdev->dev); > - pm_runtime_disable(&pdev->dev); > - } > - > cancel_work_sync(&host->work); > > mutex_lock(&host->host_mutex); > @@ -1548,6 +1567,9 @@ static int rtsx_pci_sdmmc_drv_remove(struct > platform_device *pdev) > > flush_work(&host->work); > > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + > mmc_free_host(mmc); > > dev_dbg(&(pdev->dev), > -- > 2.33.1
On Tue, 25 Jan 2022 at 06:50, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Commit 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") doesn't > use pm_runtime_{get,put}() helpers when it should, so the RPM refcount > keeps at zero, hence its parent driver, rtsx_pci, has to do lots of > weird tricks to keep it from runtime suspending. > > So use those helpers at right places to properly manage runtime PM. > > Fixes: 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") > Cc: Ricky WU <ricky_wu@realtek.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> The runtime PM reference counting for the mmc host device is managed by the mmc core. Have a look at __mmc_claim_host() and mmc_release_host(). In other words, the runtime PM reference counting should not be needed in the mmc host driver, unless there are some specific cases, like for example during ->probe|remove(). So perhaps it's only the changes in the ->probe|remove() functions that you need to fix the problems? No? [...] Kind regards Uffe
On Fri, Feb 4, 2022 at 8:28 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 25 Jan 2022 at 06:50, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > > > Commit 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") doesn't > > use pm_runtime_{get,put}() helpers when it should, so the RPM refcount > > keeps at zero, hence its parent driver, rtsx_pci, has to do lots of > > weird tricks to keep it from runtime suspending. > > > > So use those helpers at right places to properly manage runtime PM. > > > > Fixes: 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") > > Cc: Ricky WU <ricky_wu@realtek.com> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > The runtime PM reference counting for the mmc host device is managed > by the mmc core. Have a look at __mmc_claim_host() and > mmc_release_host(). > > In other words, the runtime PM reference counting should not be needed > in the mmc host driver, unless there are some specific cases, like for > example during ->probe|remove(). > > So perhaps it's only the changes in the ->probe|remove() functions > that you need to fix the problems? No? Yes you are right. Let me send a patch to remove redundant PM helpers from this patch. Kai-Heng > > [...] > > Kind regards > Uffe
On Wed, 16 Feb 2022 at 05:21, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > On Fri, Feb 4, 2022 at 8:28 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Tue, 25 Jan 2022 at 06:50, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > > > > > Commit 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") doesn't > > > use pm_runtime_{get,put}() helpers when it should, so the RPM refcount > > > keeps at zero, hence its parent driver, rtsx_pci, has to do lots of > > > weird tricks to keep it from runtime suspending. > > > > > > So use those helpers at right places to properly manage runtime PM. > > > > > > Fixes: 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") > > > Cc: Ricky WU <ricky_wu@realtek.com> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > The runtime PM reference counting for the mmc host device is managed > > by the mmc core. Have a look at __mmc_claim_host() and > > mmc_release_host(). > > > > In other words, the runtime PM reference counting should not be needed > > in the mmc host driver, unless there are some specific cases, like for > > example during ->probe|remove(). > > > > So perhaps it's only the changes in the ->probe|remove() functions > > that you need to fix the problems? No? > > Yes you are right. Let me send a patch to remove redundant PM helpers > from this patch. Ohh, I didn't know that Greg already queued this up. Seems like I don't get more than one week to review. :-) Alright, I will look at your new patch then. Kind regards Uffe
diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index 58cfaffa3c2d8..2a3f14afe9f83 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -806,6 +806,7 @@ static void sd_request(struct work_struct *work) struct mmc_request *mrq = host->mrq; struct mmc_command *cmd = mrq->cmd; struct mmc_data *data = mrq->data; + struct device *dev = &host->pdev->dev; unsigned int data_size = 0; int err; @@ -822,6 +823,7 @@ static void sd_request(struct work_struct *work) } mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -858,6 +860,8 @@ static void sd_request(struct work_struct *work) data->bytes_xfered = data->blocks * data->blksz; } + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); finish: @@ -1080,6 +1084,7 @@ static void sdmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; + struct device *dev = &host->pdev->dev; if (host->eject) return; @@ -1088,6 +1093,7 @@ static void sdmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) return; mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -1121,6 +1127,8 @@ static void sdmmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) rtsx_pci_switch_clock(pcr, ios->clock, host->ssc_depth, host->initial_mode, host->double_clk, host->vpclk); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); } @@ -1128,6 +1136,7 @@ static int sdmmc_get_ro(struct mmc_host *mmc) { struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; + struct device *dev = &host->pdev->dev; int ro = 0; u32 val; @@ -1135,6 +1144,7 @@ static int sdmmc_get_ro(struct mmc_host *mmc) return -ENOMEDIUM; mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -1144,6 +1154,8 @@ static int sdmmc_get_ro(struct mmc_host *mmc) if (val & SD_WRITE_PROTECT) ro = 1; + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); return ro; @@ -1153,6 +1165,7 @@ static int sdmmc_get_cd(struct mmc_host *mmc) { struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; + struct device *dev = &host->pdev->dev; int cd = 0; u32 val; @@ -1160,6 +1173,7 @@ static int sdmmc_get_cd(struct mmc_host *mmc) return cd; mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -1169,6 +1183,8 @@ static int sdmmc_get_cd(struct mmc_host *mmc) if (val & SD_EXIST) cd = 1; + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); return cd; @@ -1251,6 +1267,7 @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) { struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; + struct device *dev = &host->pdev->dev; int err = 0; u8 voltage; @@ -1265,6 +1282,7 @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) return err; mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -1294,6 +1312,8 @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) err = rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); return err; @@ -1303,6 +1323,7 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) { struct realtek_pci_sdmmc *host = mmc_priv(mmc); struct rtsx_pcr *pcr = host->pcr; + struct device *dev = &host->pdev->dev; int err = 0; if (host->eject) @@ -1313,6 +1334,7 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) return err; mutex_lock(&pcr->pcr_mutex); + pm_runtime_get_sync(dev); rtsx_pci_start_run(pcr); @@ -1345,6 +1367,8 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode) err = sd_change_phase(host, DDR50_RX_PHASE(pcr), true); out: + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); mutex_unlock(&pcr->pcr_mutex); return err; @@ -1495,12 +1519,12 @@ static int rtsx_pci_sdmmc_drv_probe(struct platform_device *pdev) realtek_init_host(host); - if (pcr->rtd3_en) { - pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); - pm_runtime_use_autosuspend(&pdev->dev); - pm_runtime_enable(&pdev->dev); - } - + pm_runtime_no_callbacks(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, 200); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_use_autosuspend(&pdev->dev); mmc_add_host(mmc); @@ -1521,11 +1545,6 @@ static int rtsx_pci_sdmmc_drv_remove(struct platform_device *pdev) pcr->slots[RTSX_SD_CARD].card_event = NULL; mmc = host->mmc; - if (pcr->rtd3_en) { - pm_runtime_dont_use_autosuspend(&pdev->dev); - pm_runtime_disable(&pdev->dev); - } - cancel_work_sync(&host->work); mutex_lock(&host->host_mutex); @@ -1548,6 +1567,9 @@ static int rtsx_pci_sdmmc_drv_remove(struct platform_device *pdev) flush_work(&host->work); + pm_runtime_dont_use_autosuspend(&pdev->dev); + pm_runtime_disable(&pdev->dev); + mmc_free_host(mmc); dev_dbg(&(pdev->dev),
Commit 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") doesn't use pm_runtime_{get,put}() helpers when it should, so the RPM refcount keeps at zero, hence its parent driver, rtsx_pci, has to do lots of weird tricks to keep it from runtime suspending. So use those helpers at right places to properly manage runtime PM. Fixes: 5b4258f6721f ("misc: rtsx: rts5249 support runtime PM") Cc: Ricky WU <ricky_wu@realtek.com> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v6: - Change the auto suspend delay to 200ms as Realtek suggested. v5: - Revert back to v3 as Realtek suggested. v4: - Decrease the autosuspend delay to 200ms for more power saving. v3: v2: - No change. drivers/mmc/host/rtsx_pci_sdmmc.c | 44 +++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 11 deletions(-)