diff mbox series

[v6,1/4] mmc: rtsx: Use pm_runtime_{get,put}() to handle runtime PM

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

Commit Message

Kai-Heng Feng Jan. 25, 2022, 5:50 a.m. UTC
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(-)

Comments

Ricky WU Jan. 25, 2022, 6:37 a.m. UTC | #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>; 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
Ricky WU Jan. 25, 2022, 6:38 a.m. UTC | #2
> -----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, &reg);
>  	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
Ulf Hansson Feb. 4, 2022, 12:28 p.m. UTC | #3
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
Kai-Heng Feng Feb. 16, 2022, 4:21 a.m. UTC | #4
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
Ulf Hansson Feb. 16, 2022, 10:22 a.m. UTC | #5
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 mbox series

Patch

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),