| Message ID | 20220125055010.1866563-1-kai.heng.feng@canonical.com |
|---|---|
| State | Superseded |
| 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
> -----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>; Yang Li <yang.lee@linux.alibaba.com>; > Christophe JAILLET <christophe.jaillet@wanadoo.fr>; > linux-kernel@vger.kernel.org > Subject: [PATCH v6 4/4] misc: rtsx: Quiesce rts5249 on system suspend > > Set more registers in force_power_down callback to avoid S3 wakeup from > hotplugging cards. > > This is originally written by Ricky WU. > > Link: > https://lore.kernel.org/lkml/c4525b4738f94483b9b8f8571fc80646@realtek.co > m/ > 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: > v5: > v4: > v3: > v2: > - No change. > > drivers/misc/cardreader/rtl8411.c | 2 +- > drivers/misc/cardreader/rts5209.c | 2 +- > drivers/misc/cardreader/rts5228.c | 2 +- > drivers/misc/cardreader/rts5229.c | 2 +- > drivers/misc/cardreader/rts5249.c | 31 ++++++++++++++++++++++++++++-- > drivers/misc/cardreader/rts5261.c | 2 +- > drivers/misc/cardreader/rtsx_pcr.c | 14 +++++++------- > drivers/misc/cardreader/rtsx_pcr.h | 1 + > include/linux/rtsx_pci.h | 2 +- > 9 files changed, 43 insertions(+), 15 deletions(-) > > diff --git a/drivers/misc/cardreader/rtl8411.c > b/drivers/misc/cardreader/rtl8411.c > index 4c5621b17a6fb..06457e875a90c 100644 > --- a/drivers/misc/cardreader/rtl8411.c > +++ b/drivers/misc/cardreader/rtl8411.c > @@ -76,7 +76,7 @@ static void rtl8411b_fetch_vendor_settings(struct > rtsx_pcr *pcr) > map_sd_drive(rtl8411b_reg_to_sd30_drive_sel_3v3(reg)); > } > > -static void rtl8411_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rtl8411_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) > { > rtsx_pci_write_register(pcr, FPDCTL, 0x07, 0x07); } diff --git > a/drivers/misc/cardreader/rts5209.c b/drivers/misc/cardreader/rts5209.c > index 29f5414072bf1..52b0a476ba51f 100644 > --- a/drivers/misc/cardreader/rts5209.c > +++ b/drivers/misc/cardreader/rts5209.c > @@ -47,7 +47,7 @@ static void rts5209_fetch_vendor_settings(struct rtsx_pcr > *pcr) > } > } > > -static void rts5209_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rts5209_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) > { > rtsx_pci_write_register(pcr, FPDCTL, 0x07, 0x07); } diff --git > a/drivers/misc/cardreader/rts5228.c b/drivers/misc/cardreader/rts5228.c > index ffc128278613b..ffe3afbf8bfed 100644 > --- a/drivers/misc/cardreader/rts5228.c > +++ b/drivers/misc/cardreader/rts5228.c > @@ -91,7 +91,7 @@ static int rts5228_optimize_phy(struct rtsx_pcr *pcr) > return rtsx_pci_write_phy_register(pcr, 0x07, 0x8F40); } > > -static void rts5228_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rts5228_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) > { > /* Set relink_time to 0 */ > rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 1, MASK_8_BIT_DEF, > 0); diff --git a/drivers/misc/cardreader/rts5229.c > b/drivers/misc/cardreader/rts5229.c > index c748eaf1ec1f9..b0edd8006d52f 100644 > --- a/drivers/misc/cardreader/rts5229.c > +++ b/drivers/misc/cardreader/rts5229.c > @@ -44,7 +44,7 @@ static void rts5229_fetch_vendor_settings(struct rtsx_pcr > *pcr) > map_sd_drive(rtsx_reg_to_sd30_drive_sel_3v3(reg)); > } > > -static void rts5229_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rts5229_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) > { > rtsx_pci_write_register(pcr, FPDCTL, 0x03, 0x03); } diff --git > a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c > index 53f3a1f45c4a7..91d240dd68faa 100644 > --- a/drivers/misc/cardreader/rts5249.c > +++ b/drivers/misc/cardreader/rts5249.c > @@ -74,7 +74,8 @@ static void rtsx_base_fetch_vendor_settings(struct > rtsx_pcr *pcr) > pci_read_config_dword(pdev, PCR_SETTING_REG2, ®); > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg); > > - pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); > > if (rtsx_check_mmc_support(reg)) > pcr->extra_caps |= EXTRA_CAPS_NO_MMC; @@ -143,6 +144,27 > @@ static int rts5249_init_from_hw(struct rtsx_pcr *pcr) > return 0; > } > > +static void rts52xa_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) { > + /* Set relink_time to 0 */ > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 1, MASK_8_BIT_DEF, > 0); > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 2, MASK_8_BIT_DEF, > 0); > + rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 3, > + RELINK_TIME_MASK, 0); > + > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, > + D3_DELINK_MODE_EN, D3_DELINK_MODE_EN); > + > + if (!runtime) { > + rtsx_pci_write_register(pcr, RTS524A_AUTOLOAD_CFG1, > + CD_RESUME_EN_MASK, 0); > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); > + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); > + } > + > + rtsx_pci_write_register(pcr, FPDCTL, ALL_POWER_DOWN, > ALL_POWER_DOWN); > +} > + > static void rts52xa_save_content_from_efuse(struct rtsx_pcr *pcr) { > u8 cnt, sv; > @@ -281,8 +303,11 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) > > rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF); > > - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > rtsx_pci_write_register(pcr, REG_VREF, PWD_SUSPND_EN, > PWD_SUSPND_EN); > + rtsx_pci_write_register(pcr, RTS524A_AUTOLOAD_CFG1, > + CD_RESUME_EN_MASK, CD_RESUME_EN_MASK); > + } > > if (pcr->rtd3_en) { > if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > { @@ -724,6 +749,7 @@ static const struct pcr_ops rts524a_pcr_ops = { > .card_power_on = rtsx_base_card_power_on, > .card_power_off = rtsx_base_card_power_off, > .switch_output_voltage = rtsx_base_switch_output_voltage, > + .force_power_down = rts52xa_force_power_down, > .set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0, }; > > @@ -841,6 +867,7 @@ static const struct pcr_ops rts525a_pcr_ops = { > .card_power_on = rts525a_card_power_on, > .card_power_off = rtsx_base_card_power_off, > .switch_output_voltage = rts525a_switch_output_voltage, > + .force_power_down = rts52xa_force_power_down, > .set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0, }; > > diff --git a/drivers/misc/cardreader/rts5261.c > b/drivers/misc/cardreader/rts5261.c > index 1fd4e0e507302..64333347c14a4 100644 > --- a/drivers/misc/cardreader/rts5261.c > +++ b/drivers/misc/cardreader/rts5261.c > @@ -91,7 +91,7 @@ static void rtsx5261_fetch_vendor_settings(struct > rtsx_pcr *pcr) > pcr->sd30_drive_sel_3v3 = rts5261_reg_to_sd30_drive_sel_3v3(reg); > } > > -static void rts5261_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rts5261_force_power_down(struct rtsx_pcr *pcr, u8 pm_state, > +bool runtime) > { > /* Set relink_time to 0 */ > rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 1, MASK_8_BIT_DEF, > 0); diff --git a/drivers/misc/cardreader/rtsx_pcr.c > b/drivers/misc/cardreader/rtsx_pcr.c > index 3c97d3b50456e..1cb6425e83697 100644 > --- a/drivers/misc/cardreader/rtsx_pcr.c > +++ b/drivers/misc/cardreader/rtsx_pcr.c > @@ -1086,7 +1086,7 @@ static void rtsx_pm_power_saving(struct rtsx_pcr > *pcr) > rtsx_comm_pm_power_saving(pcr); > } > > -static void rtsx_base_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > +static void rtsx_base_force_power_down(struct rtsx_pcr *pcr) > { > /* Set relink_time to 0 */ > rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 1, MASK_8_BIT_DEF, > 0); @@ -1100,7 +1100,7 @@ static void rtsx_base_force_power_down(struct > rtsx_pcr *pcr, u8 pm_state) > rtsx_pci_write_register(pcr, FPDCTL, ALL_POWER_DOWN, > ALL_POWER_DOWN); } > > -static void __maybe_unused rtsx_pci_power_off(struct rtsx_pcr *pcr, u8 > pm_state) > +static void __maybe_unused rtsx_pci_power_off(struct rtsx_pcr *pcr, u8 > +pm_state, bool runtime) > { > if (pcr->ops->turn_off_led) > pcr->ops->turn_off_led(pcr); > @@ -1112,9 +1112,9 @@ static void __maybe_unused > rtsx_pci_power_off(struct rtsx_pcr *pcr, u8 pm_state) > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, pm_state); > > if (pcr->ops->force_power_down) > - pcr->ops->force_power_down(pcr, pm_state); > + pcr->ops->force_power_down(pcr, pm_state, runtime); > else > - rtsx_base_force_power_down(pcr, pm_state); > + rtsx_base_force_power_down(pcr); > } > > void rtsx_pci_enable_ocp(struct rtsx_pcr *pcr) @@ -1669,7 +1669,7 @@ > static int __maybe_unused rtsx_pci_suspend(struct device *dev_d) > > mutex_lock(&pcr->pcr_mutex); > > - rtsx_pci_power_off(pcr, HOST_ENTER_S3); > + rtsx_pci_power_off(pcr, HOST_ENTER_S3, false); > > mutex_unlock(&pcr->pcr_mutex); > return 0; > @@ -1708,7 +1708,7 @@ static void rtsx_pci_shutdown(struct pci_dev > *pcidev) > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__); > > - rtsx_pci_power_off(pcr, HOST_ENTER_S1); > + rtsx_pci_power_off(pcr, HOST_ENTER_S1, false); > > pci_disable_device(pcidev); > free_irq(pcr->irq, (void *)pcr); > @@ -1754,7 +1754,7 @@ static int rtsx_pci_runtime_suspend(struct device > *device) > cancel_delayed_work_sync(&pcr->carddet_work); > > mutex_lock(&pcr->pcr_mutex); > - rtsx_pci_power_off(pcr, HOST_ENTER_S3); > + rtsx_pci_power_off(pcr, HOST_ENTER_S3, true); > > mutex_unlock(&pcr->pcr_mutex); > > diff --git a/drivers/misc/cardreader/rtsx_pcr.h > b/drivers/misc/cardreader/rtsx_pcr.h > index daf057c4eea62..aa0ebd6672277 100644 > --- a/drivers/misc/cardreader/rtsx_pcr.h > +++ b/drivers/misc/cardreader/rtsx_pcr.h > @@ -25,6 +25,7 @@ > #define REG_EFUSE_POWEROFF 0x00 > #define RTS5250_CLK_CFG3 0xFF79 > #define RTS525A_CFG_MEM_PD 0xF0 > +#define RTS524A_AUTOLOAD_CFG1 0xFF7C > #define RTS524A_PM_CTRL3 0xFF7E > #define RTS525A_BIOS_CFG 0xFF2D > #define RTS525A_LOAD_BIOS_FLAG 0x01 > diff --git a/include/linux/rtsx_pci.h b/include/linux/rtsx_pci.h index > 89b7d34e25b63..3d780b44e678a 100644 > --- a/include/linux/rtsx_pci.h > +++ b/include/linux/rtsx_pci.h > @@ -1095,7 +1095,7 @@ struct pcr_ops { > unsigned int (*cd_deglitch)(struct rtsx_pcr *pcr); > int (*conv_clk_and_div_n)(int clk, int dir); > void (*fetch_vendor_settings)(struct rtsx_pcr *pcr); > - void (*force_power_down)(struct rtsx_pcr *pcr, u8 pm_state); > + void (*force_power_down)(struct rtsx_pcr *pcr, u8 pm_state, bool > runtime); > void (*stop_cmd)(struct rtsx_pcr *pcr); > > void (*set_aspm)(struct rtsx_pcr *pcr, bool enable); > -- > 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(-)