* [PATCH] misc: rtsx: modify rtd3 flow
@ 2022-01-13 10:50 Ricky WU
2022-01-13 11:32 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Ricky WU @ 2022-01-13 10:50 UTC (permalink / raw)
To: arnd, gregkh, Ricky WU, christophe.jaillet, kai.heng.feng,
yang.lee, linux-kernel
move pm_runtime_get() to _runtime_resume
when System enter S3, do not have sd_request and do not
call start_run to pm_runtime_get() cause is_runtime_suspended status
not correct
set more register in power_down flow to make plugin or unplug
card do not wake up system when system is at S3
Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
---
drivers/misc/cardreader/rts5249.c | 31 ++++++++++++++++++++++++++++--
drivers/misc/cardreader/rtsx_pcr.c | 17 ++++++++--------
drivers/misc/cardreader/rtsx_pcr.h | 1 +
3 files changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c
index 53f3a1f45c4a..69e32f075ca9 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)
+{
+ /* 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 (!pcr->is_runtime_suspended) {
+ 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/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
index 6ac509c1821c..a83adfb122dc 100644
--- a/drivers/misc/cardreader/rtsx_pcr.c
+++ b/drivers/misc/cardreader/rtsx_pcr.c
@@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
if (pcr->remove_pci)
return;
- if (pcr->rtd3_en)
- if (pcr->is_runtime_suspended) {
- pm_runtime_get(&(pcr->pci->dev));
- pcr->is_runtime_suspended = false;
- }
-
if (pcr->state != PDEV_STAT_RUN) {
pcr->state = PDEV_STAT_RUN;
if (pcr->ops->enable_auto_blink)
@@ -1597,6 +1591,7 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr + HOST_CMDS_BUF_LEN;
pcr->card_inserted = 0;
pcr->card_removed = 0;
+ pcr->rtd3_en = 0;
INIT_DELAYED_WORK(&pcr->carddet_work, rtsx_pci_card_detect);
INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
@@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct device *device)
pcr = handle->pcr;
dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
+ pcr->is_runtime_suspended = true;
+
cancel_delayed_work(&pcr->carddet_work);
cancel_delayed_work(&pcr->rtd3_work);
cancel_delayed_work(&pcr->idle_work);
mutex_lock(&pcr->pcr_mutex);
rtsx_pci_power_off(pcr, HOST_ENTER_S3);
-
mutex_unlock(&pcr->pcr_mutex);
- pcr->is_runtime_suspended = true;
-
return 0;
}
@@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct device *device)
pcr = handle->pcr;
dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
+ if (pcr->is_runtime_suspended) {
+ pm_runtime_get(&(pcr->pci->dev));
+ pcr->is_runtime_suspended = false;
+ }
+
mutex_lock(&pcr->pcr_mutex);
rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00);
diff --git a/drivers/misc/cardreader/rtsx_pcr.h b/drivers/misc/cardreader/rtsx_pcr.h
index daf057c4eea6..b93975268e6d 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
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-13 10:50 [PATCH] misc: rtsx: modify rtd3 flow Ricky WU
@ 2022-01-13 11:32 ` Kai-Heng Feng
2022-01-14 6:56 ` Ricky WU
0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2022-01-13 11:32 UTC (permalink / raw)
To: Ricky WU; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
On Thu, Jan 13, 2022 at 6:50 PM Ricky WU <ricky_wu@realtek.com> wrote:
>
> move pm_runtime_get() to _runtime_resume
> when System enter S3, do not have sd_request and do not
> call start_run to pm_runtime_get() cause is_runtime_suspended status
> not correct
>
> set more register in power_down flow to make plugin or unplug
> card do not wake up system when system is at S3
>
> Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> ---
> drivers/misc/cardreader/rts5249.c | 31 ++++++++++++++++++++++++++++--
> drivers/misc/cardreader/rtsx_pcr.c | 17 ++++++++--------
> drivers/misc/cardreader/rtsx_pcr.h | 1 +
> 3 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c
> index 53f3a1f45c4a..69e32f075ca9 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)
> +{
> + /* 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 (!pcr->is_runtime_suspended) {
> + 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/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c
> index 6ac509c1821c..a83adfb122dc 100644
> --- a/drivers/misc/cardreader/rtsx_pcr.c
> +++ b/drivers/misc/cardreader/rtsx_pcr.c
> @@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> if (pcr->remove_pci)
> return;
>
> - if (pcr->rtd3_en)
> - if (pcr->is_runtime_suspended) {
> - pm_runtime_get(&(pcr->pci->dev));
> - pcr->is_runtime_suspended = false;
> - }
> -
> if (pcr->state != PDEV_STAT_RUN) {
> pcr->state = PDEV_STAT_RUN;
> if (pcr->ops->enable_auto_blink)
> @@ -1597,6 +1591,7 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
> pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr + HOST_CMDS_BUF_LEN;
> pcr->card_inserted = 0;
> pcr->card_removed = 0;
> + pcr->rtd3_en = 0;
> INIT_DELAYED_WORK(&pcr->carddet_work, rtsx_pci_card_detect);
> INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
>
> @@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct device *device)
> pcr = handle->pcr;
> dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
>
> + pcr->is_runtime_suspended = true;
> +
> cancel_delayed_work(&pcr->carddet_work);
> cancel_delayed_work(&pcr->rtd3_work);
> cancel_delayed_work(&pcr->idle_work);
>
> mutex_lock(&pcr->pcr_mutex);
> rtsx_pci_power_off(pcr, HOST_ENTER_S3);
> -
> mutex_unlock(&pcr->pcr_mutex);
>
> - pcr->is_runtime_suspended = true;
> -
> return 0;
> }
>
> @@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct device *device)
> pcr = handle->pcr;
> dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
>
> + if (pcr->is_runtime_suspended) {
> + pm_runtime_get(&(pcr->pci->dev));
> + pcr->is_runtime_suspended = false;
> + }
If the runtime resume routine is called for system wide suspend, the
runtime suspend isn't allowed during the period.
So I don't quite understand what this patch is for.
Kai-Heng
> +
> mutex_lock(&pcr->pcr_mutex);
>
> rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00);
> diff --git a/drivers/misc/cardreader/rtsx_pcr.h b/drivers/misc/cardreader/rtsx_pcr.h
> index daf057c4eea6..b93975268e6d 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
> --
> 2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-13 11:32 ` Kai-Heng Feng
@ 2022-01-14 6:56 ` Ricky WU
2022-01-14 7:29 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Ricky WU @ 2022-01-14 6:56 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Thursday, January 13, 2022 7:33 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
>
> On Thu, Jan 13, 2022 at 6:50 PM Ricky WU <ricky_wu@realtek.com> wrote:
> >
> > move pm_runtime_get() to _runtime_resume when System enter S3, do not
> > have sd_request and do not call start_run to pm_runtime_get() cause
> > is_runtime_suspended status not correct
> >
> > set more register in power_down flow to make plugin or unplug card do
> > not wake up system when system is at S3
> >
> > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > ---
> > drivers/misc/cardreader/rts5249.c | 31
> > ++++++++++++++++++++++++++++-- drivers/misc/cardreader/rtsx_pcr.c |
> > 17 ++++++++-------- drivers/misc/cardreader/rtsx_pcr.h | 1 +
> > 3 files changed, 38 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/misc/cardreader/rts5249.c
> > b/drivers/misc/cardreader/rts5249.c
> > index 53f3a1f45c4a..69e32f075ca9 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) {
> > + /* 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 (!pcr->is_runtime_suspended) {
> > + 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/rtsx_pcr.c
> > b/drivers/misc/cardreader/rtsx_pcr.c
> > index 6ac509c1821c..a83adfb122dc 100644
> > --- a/drivers/misc/cardreader/rtsx_pcr.c
> > +++ b/drivers/misc/cardreader/rtsx_pcr.c
> > @@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> > if (pcr->remove_pci)
> > return;
> >
> > - if (pcr->rtd3_en)
> > - if (pcr->is_runtime_suspended) {
> > - pm_runtime_get(&(pcr->pci->dev));
> > - pcr->is_runtime_suspended = false;
> > - }
> > -
> > if (pcr->state != PDEV_STAT_RUN) {
> > pcr->state = PDEV_STAT_RUN;
> > if (pcr->ops->enable_auto_blink) @@ -1597,6 +1591,7
> @@
> > static int rtsx_pci_probe(struct pci_dev *pcidev,
> > pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr +
> HOST_CMDS_BUF_LEN;
> > pcr->card_inserted = 0;
> > pcr->card_removed = 0;
> > + pcr->rtd3_en = 0;
> > INIT_DELAYED_WORK(&pcr->carddet_work,
> rtsx_pci_card_detect);
> > INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
> >
> > @@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct
> device *device)
> > pcr = handle->pcr;
> > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> >
> > + pcr->is_runtime_suspended = true;
> > +
> > cancel_delayed_work(&pcr->carddet_work);
> > cancel_delayed_work(&pcr->rtd3_work);
> > cancel_delayed_work(&pcr->idle_work);
> >
> > mutex_lock(&pcr->pcr_mutex);
> > rtsx_pci_power_off(pcr, HOST_ENTER_S3);
> > -
> > mutex_unlock(&pcr->pcr_mutex);
> >
> > - pcr->is_runtime_suspended = true;
> > -
> > return 0;
> > }
> >
> > @@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct device
> *device)
> > pcr = handle->pcr;
> > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> >
> > + if (pcr->is_runtime_suspended) {
> > + pm_runtime_get(&(pcr->pci->dev));
> > + pcr->is_runtime_suspended = false;
> > + }
>
> If the runtime resume routine is called for system wide suspend, the runtime
> suspend isn't allowed during the period.
> So I don't quite understand what this patch is for.
>
We don’t want to entry D3 frequently
So we need to call pm_runtime_get() at start
And call pm_runtime_put() in delay-work (rtd3_work)
But we found If we keep this if statement in start_run
if (pcr->is_runtime_suspended) {
pm_runtime_get(&(pcr->pci->dev));
pcr->is_runtime_suspended = false;
}
pcr->is_runtime_suspended this status are not correct when enter S3
because enter S3 not call start_run()
> Kai-Heng
>
> > +
> > mutex_lock(&pcr->pcr_mutex);
> >
> > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00);
> > diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > b/drivers/misc/cardreader/rtsx_pcr.h
> > index daf057c4eea6..b93975268e6d 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
> > --
> > 2.25.1
> ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-14 6:56 ` Ricky WU
@ 2022-01-14 7:29 ` Kai-Heng Feng
2022-01-14 8:51 ` Ricky WU
0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 7:29 UTC (permalink / raw)
To: Ricky WU; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
On Fri, Jan 14, 2022 at 2:56 PM Ricky WU <ricky_wu@realtek.com> wrote:
>
> > -----Original Message-----
> > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Sent: Thursday, January 13, 2022 7:33 PM
> > To: Ricky WU <ricky_wu@realtek.com>
> > Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> > christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
> >
> > On Thu, Jan 13, 2022 at 6:50 PM Ricky WU <ricky_wu@realtek.com> wrote:
> > >
> > > move pm_runtime_get() to _runtime_resume when System enter S3, do not
> > > have sd_request and do not call start_run to pm_runtime_get() cause
> > > is_runtime_suspended status not correct
> > >
> > > set more register in power_down flow to make plugin or unplug card do
> > > not wake up system when system is at S3
> > >
> > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > ---
> > > drivers/misc/cardreader/rts5249.c | 31
> > > ++++++++++++++++++++++++++++-- drivers/misc/cardreader/rtsx_pcr.c |
> > > 17 ++++++++-------- drivers/misc/cardreader/rtsx_pcr.h | 1 +
> > > 3 files changed, 38 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/misc/cardreader/rts5249.c
> > > b/drivers/misc/cardreader/rts5249.c
> > > index 53f3a1f45c4a..69e32f075ca9 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) {
> > > + /* 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 (!pcr->is_runtime_suspended) {
> > > + 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/rtsx_pcr.c
> > > b/drivers/misc/cardreader/rtsx_pcr.c
> > > index 6ac509c1821c..a83adfb122dc 100644
> > > --- a/drivers/misc/cardreader/rtsx_pcr.c
> > > +++ b/drivers/misc/cardreader/rtsx_pcr.c
> > > @@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> > > if (pcr->remove_pci)
> > > return;
> > >
> > > - if (pcr->rtd3_en)
> > > - if (pcr->is_runtime_suspended) {
> > > - pm_runtime_get(&(pcr->pci->dev));
> > > - pcr->is_runtime_suspended = false;
> > > - }
> > > -
> > > if (pcr->state != PDEV_STAT_RUN) {
> > > pcr->state = PDEV_STAT_RUN;
> > > if (pcr->ops->enable_auto_blink) @@ -1597,6 +1591,7
> > @@
> > > static int rtsx_pci_probe(struct pci_dev *pcidev,
> > > pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr +
> > HOST_CMDS_BUF_LEN;
> > > pcr->card_inserted = 0;
> > > pcr->card_removed = 0;
> > > + pcr->rtd3_en = 0;
> > > INIT_DELAYED_WORK(&pcr->carddet_work,
> > rtsx_pci_card_detect);
> > > INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
> > >
> > > @@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct
> > device *device)
> > > pcr = handle->pcr;
> > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > >
> > > + pcr->is_runtime_suspended = true;
> > > +
> > > cancel_delayed_work(&pcr->carddet_work);
> > > cancel_delayed_work(&pcr->rtd3_work);
> > > cancel_delayed_work(&pcr->idle_work);
> > >
> > > mutex_lock(&pcr->pcr_mutex);
> > > rtsx_pci_power_off(pcr, HOST_ENTER_S3);
> > > -
> > > mutex_unlock(&pcr->pcr_mutex);
> > >
> > > - pcr->is_runtime_suspended = true;
> > > -
> > > return 0;
> > > }
> > >
> > > @@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct device
> > *device)
> > > pcr = handle->pcr;
> > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > >
> > > + if (pcr->is_runtime_suspended) {
> > > + pm_runtime_get(&(pcr->pci->dev));
> > > + pcr->is_runtime_suspended = false;
> > > + }
> >
> > If the runtime resume routine is called for system wide suspend, the runtime
> > suspend isn't allowed during the period.
> > So I don't quite understand what this patch is for.
> >
>
> We don’t want to entry D3 frequently
> So we need to call pm_runtime_get() at start
> And call pm_runtime_put() in delay-work (rtd3_work)
Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
rtsx_pci_runtime_suspend() does?
And for this case maybe cancel_delayed_work_sync() is more preferred.
>
> But we found If we keep this if statement in start_run
> if (pcr->is_runtime_suspended) {
> pm_runtime_get(&(pcr->pci->dev));
> pcr->is_runtime_suspended = false;
> }
> pcr->is_runtime_suspended this status are not correct when enter S3
> because enter S3 not call start_run()
Maybe because the driver is trying to trick the runtime PM core on its
real power status?
I.e. the driver is maintaining its own PM state machine. Fully
cooperating the driver with PM core should solve the issue.
Kai-Heng
>
> > Kai-Heng
> >
> > > +
> > > mutex_lock(&pcr->pcr_mutex);
> > >
> > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00);
> > > diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > index daf057c4eea6..b93975268e6d 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
> > > --
> > > 2.25.1
> > ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-14 7:29 ` Kai-Heng Feng
@ 2022-01-14 8:51 ` Ricky WU
2022-01-14 12:09 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Ricky WU @ 2022-01-14 8:51 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Friday, January 14, 2022 3:29 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
>
> On Fri, Jan 14, 2022 at 2:56 PM Ricky WU <ricky_wu@realtek.com> wrote:
> >
> > > -----Original Message-----
> > > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > Sent: Thursday, January 13, 2022 7:33 PM
> > > To: Ricky WU <ricky_wu@realtek.com>
> > > Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> > > christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
> > >
> > > On Thu, Jan 13, 2022 at 6:50 PM Ricky WU <ricky_wu@realtek.com>
> wrote:
> > > >
> > > > move pm_runtime_get() to _runtime_resume when System enter S3, do
> > > > not have sd_request and do not call start_run to pm_runtime_get()
> > > > cause is_runtime_suspended status not correct
> > > >
> > > > set more register in power_down flow to make plugin or unplug card
> > > > do not wake up system when system is at S3
> > > >
> > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
> > > > ---
> > > > drivers/misc/cardreader/rts5249.c | 31
> > > > ++++++++++++++++++++++++++++-- drivers/misc/cardreader/rtsx_pcr.c
> > > > ++++++++++++++++++++++++++++|
> > > > 17 ++++++++-------- drivers/misc/cardreader/rtsx_pcr.h | 1 +
> > > > 3 files changed, 38 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/cardreader/rts5249.c
> > > > b/drivers/misc/cardreader/rts5249.c
> > > > index 53f3a1f45c4a..69e32f075ca9 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) {
> > > > + /* 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 (!pcr->is_runtime_suspended) {
> > > > + 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/rtsx_pcr.c
> > > > b/drivers/misc/cardreader/rtsx_pcr.c
> > > > index 6ac509c1821c..a83adfb122dc 100644
> > > > --- a/drivers/misc/cardreader/rtsx_pcr.c
> > > > +++ b/drivers/misc/cardreader/rtsx_pcr.c
> > > > @@ -152,12 +152,6 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr)
> > > > if (pcr->remove_pci)
> > > > return;
> > > >
> > > > - if (pcr->rtd3_en)
> > > > - if (pcr->is_runtime_suspended) {
> > > > - pm_runtime_get(&(pcr->pci->dev));
> > > > - pcr->is_runtime_suspended = false;
> > > > - }
> > > > -
> > > > if (pcr->state != PDEV_STAT_RUN) {
> > > > pcr->state = PDEV_STAT_RUN;
> > > > if (pcr->ops->enable_auto_blink) @@ -1597,6
> > > > +1591,7
> > > @@
> > > > static int rtsx_pci_probe(struct pci_dev *pcidev,
> > > > pcr->host_sg_tbl_addr = pcr->rtsx_resv_buf_addr +
> > > HOST_CMDS_BUF_LEN;
> > > > pcr->card_inserted = 0;
> > > > pcr->card_removed = 0;
> > > > + pcr->rtd3_en = 0;
> > > > INIT_DELAYED_WORK(&pcr->carddet_work,
> > > rtsx_pci_card_detect);
> > > > INIT_DELAYED_WORK(&pcr->idle_work, rtsx_pci_idle_work);
> > > >
> > > > @@ -1796,17 +1791,16 @@ static int rtsx_pci_runtime_suspend(struct
> > > device *device)
> > > > pcr = handle->pcr;
> > > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > > >
> > > > + pcr->is_runtime_suspended = true;
> > > > +
> > > > cancel_delayed_work(&pcr->carddet_work);
> > > > cancel_delayed_work(&pcr->rtd3_work);
> > > > cancel_delayed_work(&pcr->idle_work);
> > > >
> > > > mutex_lock(&pcr->pcr_mutex);
> > > > rtsx_pci_power_off(pcr, HOST_ENTER_S3);
> > > > -
> > > > mutex_unlock(&pcr->pcr_mutex);
> > > >
> > > > - pcr->is_runtime_suspended = true;
> > > > -
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1820,6 +1814,11 @@ static int rtsx_pci_runtime_resume(struct
> > > > device
> > > *device)
> > > > pcr = handle->pcr;
> > > > dev_dbg(&(pcidev->dev), "--> %s\n", __func__);
> > > >
> > > > + if (pcr->is_runtime_suspended) {
> > > > + pm_runtime_get(&(pcr->pci->dev));
> > > > + pcr->is_runtime_suspended = false;
> > > > + }
> > >
> > > If the runtime resume routine is called for system wide suspend, the
> > > runtime suspend isn't allowed during the period.
> > > So I don't quite understand what this patch is for.
> > >
> >
> > We don’t want to entry D3 frequently
> > So we need to call pm_runtime_get() at start And call pm_runtime_put()
> > in delay-work (rtd3_work)
>
> Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
> rtsx_pci_runtime_suspend() does?
> And for this case maybe cancel_delayed_work_sync() is more preferred.
>
I think you misunderstand what I means
This delay_work() is for not enter D3 <-> D0 frequently
That delay is we need, we don’t want to power_on and power_off frequently on our device
This patch want to solve pcr->is_runtime_suspended this value
because we need to set more register at power_down flow when Device support D3 and System going to S3
> >
> > But we found If we keep this if statement in start_run if
> > (pcr->is_runtime_suspended) {
> > pm_runtime_get(&(pcr->pci->dev));
> > pcr->is_runtime_suspended = false;
> > }
> > pcr->is_runtime_suspended this status are not correct when enter S3
> > because enter S3 not call start_run()
>
> Maybe because the driver is trying to trick the runtime PM core on its real
> power status?
> I.e. the driver is maintaining its own PM state machine. Fully cooperating the
> driver with PM core should solve the issue.
>
System not call start_run() because do not have any sd_request at that time,
so we need to update value(pcr->is_runtime_suspended) at rtsx_pci_runtime_resume
but if we only update value here not to call pm_runtime_get(), the if-statement always be FALSE at start_run()
that is why we move this if-statement from start_run() to rtsx_pci_runtime_resume()
> > >
> > > > +
> > > > mutex_lock(&pcr->pcr_mutex);
> > > >
> > > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03,
> > > > 0x00); diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > > index daf057c4eea6..b93975268e6d 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
> > > > --
> > > > 2.25.1
> > > ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-14 8:51 ` Ricky WU
@ 2022-01-14 12:09 ` Kai-Heng Feng
2022-01-17 3:34 ` Ricky WU
0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 12:09 UTC (permalink / raw)
To: Ricky WU; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
On Fri, Jan 14, 2022 at 4:51 PM Ricky WU <ricky_wu@realtek.com> wrote:
[snip]
> > >
> > > We don’t want to entry D3 frequently
> > > So we need to call pm_runtime_get() at start And call pm_runtime_put()
> > > in delay-work (rtd3_work)
> >
> > Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
> > rtsx_pci_runtime_suspend() does?
> > And for this case maybe cancel_delayed_work_sync() is more preferred.
> >
>
> I think you misunderstand what I means
> This delay_work() is for not enter D3 <-> D0 frequently
This is doable with current pm_runtime_*() helpers.
> That delay is we need, we don’t want to power_on and power_off frequently on our device
Is this to avoid the pm_runtime_resume() call before system suspend?
IOW, to avoid D3 -> D0 -> D3 for S3?
>
> This patch want to solve pcr->is_runtime_suspended this value
> because we need to set more register at power_down flow when Device support D3 and System going to S3
So is it possible to introduce a new parameter for force_power_down()
and get rid of is_runtime_suspended completely?
>
> > >
> > > But we found If we keep this if statement in start_run if
> > > (pcr->is_runtime_suspended) {
> > > pm_runtime_get(&(pcr->pci->dev));
> > > pcr->is_runtime_suspended = false;
> > > }
> > > pcr->is_runtime_suspended this status are not correct when enter S3
> > > because enter S3 not call start_run()
> >
> > Maybe because the driver is trying to trick the runtime PM core on its real
> > power status?
> > I.e. the driver is maintaining its own PM state machine. Fully cooperating the
> > driver with PM core should solve the issue.
> >
>
> System not call start_run() because do not have any sd_request at that time,
> so we need to update value(pcr->is_runtime_suspended) at rtsx_pci_runtime_resume
> but if we only update value here not to call pm_runtime_get(), the if-statement always be FALSE at start_run()
Sounds like the RPM refcount goes to zero so it was runtime suspended again.
The correct usage should be merging rtsx_pci_start_run() with
rtsx_pci_runtime_resume(), then guard each op of mmc_host_ops with
pm_runtime_get_sync() at the beginning and pm_runtime_idle() at and
end of each ops.
Increase the RPM refcount in runtime resume routine to prevent the
driver from suspending doesn't really make sense to me.
Kai-Heng
> that is why we move this if-statement from start_run() to rtsx_pci_runtime_resume()
>
> > > >
> > > > > +
> > > > > mutex_lock(&pcr->pcr_mutex);
> > > > >
> > > > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03,
> > > > > 0x00); diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > > > index daf057c4eea6..b93975268e6d 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
> > > > > --
> > > > > 2.25.1
> > > > ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-14 12:09 ` Kai-Heng Feng
@ 2022-01-17 3:34 ` Ricky WU
2022-01-17 5:11 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Ricky WU @ 2022-01-17 3:34 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Friday, January 14, 2022 8:09 PM
> To: Ricky WU <ricky_wu@realtek.com>
> Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
>
> On Fri, Jan 14, 2022 at 4:51 PM Ricky WU <ricky_wu@realtek.com> wrote:
> [snip]
> > > >
> > > > We don’t want to entry D3 frequently So we need to call
> > > > pm_runtime_get() at start And call pm_runtime_put() in delay-work
> > > > (rtd3_work)
> > >
> > > Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
> > > rtsx_pci_runtime_suspend() does?
> > > And for this case maybe cancel_delayed_work_sync() is more preferred.
> > >
> >
> > I think you misunderstand what I means This delay_work() is for not
> > enter D3 <-> D0 frequently
>
> This is doable with current pm_runtime_*() helpers.
>
> > That delay is we need, we don’t want to power_on and power_off
> > frequently on our device
>
> Is this to avoid the pm_runtime_resume() call before system suspend?
> IOW, to avoid D3 -> D0 -> D3 for S3?
>
No, this only for make sure our device D0 -> D3 at least 10 sec, this only about device status
not affect System suspend (S3)
You can see the current code rtsx_pci_idle_work(struct work_struct *work)
System suspend (S3) flow is different it will call rtsx_pci_suspend()
> >
> > This patch want to solve pcr->is_runtime_suspended this value because
> > we need to set more register at power_down flow when Device support D3
> > and System going to S3
>
> So is it possible to introduce a new parameter for force_power_down() and get
> rid of is_runtime_suspended completely?
>
This just distinguish 524a, 525a go to the new power down flow
Add more register setting for RTD3 enable
> >
> > > >
> > > > But we found If we keep this if statement in start_run if
> > > > (pcr->is_runtime_suspended) {
> > > > pm_runtime_get(&(pcr->pci->dev));
> > > > pcr->is_runtime_suspended = false; }
> > > > pcr->is_runtime_suspended this status are not correct when enter
> > > > pcr->S3
> > > > because enter S3 not call start_run()
> > >
> > > Maybe because the driver is trying to trick the runtime PM core on
> > > its real power status?
> > > I.e. the driver is maintaining its own PM state machine. Fully
> > > cooperating the driver with PM core should solve the issue.
> > >
> >
> > System not call start_run() because do not have any sd_request at that
> > time, so we need to update value(pcr->is_runtime_suspended) at
> > rtsx_pci_runtime_resume but if we only update value here not to call
> > pm_runtime_get(), the if-statement always be FALSE at start_run()
>
> Sounds like the RPM refcount goes to zero so it was runtime suspended again.
> The correct usage should be merging rtsx_pci_start_run() with
> rtsx_pci_runtime_resume(), then guard each op of mmc_host_ops with
> pm_runtime_get_sync() at the beginning and pm_runtime_idle() at and end of
> each ops.
>
> Increase the RPM refcount in runtime resume routine to prevent the driver
> from suspending doesn't really make sense to me.
>
> Kai-Heng
We want to control the interval time between D0->D3
This is for good user experience and more stable for our device.
And this completely unaffected System suspend, because System want to enter S3
need to make sure Device back to D0 and then go to S3 and put Device to D3 if Device support D3
>
> > that is why we move this if-statement from start_run() to
> > rtsx_pci_runtime_resume()
> >
> > > > >
> > > > > > +
> > > > > > mutex_lock(&pcr->pcr_mutex);
> > > > > >
> > > > > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03,
> > > > > > 0x00); diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > > > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > > > > index daf057c4eea6..b93975268e6d 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
> > > > > > --
> > > > > > 2.25.1
> > > > > ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] misc: rtsx: modify rtd3 flow
2022-01-17 3:34 ` Ricky WU
@ 2022-01-17 5:11 ` Kai-Heng Feng
0 siblings, 0 replies; 8+ messages in thread
From: Kai-Heng Feng @ 2022-01-17 5:11 UTC (permalink / raw)
To: Ricky WU; +Cc: arnd, gregkh, christophe.jaillet, yang.lee, linux-kernel
On Mon, Jan 17, 2022 at 11:34 AM Ricky WU <ricky_wu@realtek.com> wrote:
>
> > -----Original Message-----
> > From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Sent: Friday, January 14, 2022 8:09 PM
> > To: Ricky WU <ricky_wu@realtek.com>
> > Cc: arnd@arndb.de; gregkh@linuxfoundation.org;
> > christophe.jaillet@wanadoo.fr; yang.lee@linux.alibaba.com;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
> >
> > On Fri, Jan 14, 2022 at 4:51 PM Ricky WU <ricky_wu@realtek.com> wrote:
> > [snip]
> > > > >
> > > > > We don’t want to entry D3 frequently So we need to call
> > > > > pm_runtime_get() at start And call pm_runtime_put() in delay-work
> > > > > (rtd3_work)
> > > >
> > > > Maybe use 'cancel_delayed_work(&pcr->rtd3_work)' like what
> > > > rtsx_pci_runtime_suspend() does?
> > > > And for this case maybe cancel_delayed_work_sync() is more preferred.
> > > >
> > >
> > > I think you misunderstand what I means This delay_work() is for not
> > > enter D3 <-> D0 frequently
> >
> > This is doable with current pm_runtime_*() helpers.
> >
> > > That delay is we need, we don’t want to power_on and power_off
> > > frequently on our device
> >
> > Is this to avoid the pm_runtime_resume() call before system suspend?
> > IOW, to avoid D3 -> D0 -> D3 for S3?
> >
>
> No, this only for make sure our device D0 -> D3 at least 10 sec, this only about device status
> not affect System suspend (S3)
> You can see the current code rtsx_pci_idle_work(struct work_struct *work)
>
> System suspend (S3) flow is different it will call rtsx_pci_suspend()
>
> > >
> > > This patch want to solve pcr->is_runtime_suspended this value because
> > > we need to set more register at power_down flow when Device support D3
> > > and System going to S3
> >
> > So is it possible to introduce a new parameter for force_power_down() and get
> > rid of is_runtime_suspended completely?
> >
>
> This just distinguish 524a, 525a go to the new power down flow
> Add more register setting for RTD3 enable
>
> > >
> > > > >
> > > > > But we found If we keep this if statement in start_run if
> > > > > (pcr->is_runtime_suspended) {
> > > > > pm_runtime_get(&(pcr->pci->dev));
> > > > > pcr->is_runtime_suspended = false; }
> > > > > pcr->is_runtime_suspended this status are not correct when enter
> > > > > pcr->S3
> > > > > because enter S3 not call start_run()
> > > >
> > > > Maybe because the driver is trying to trick the runtime PM core on
> > > > its real power status?
> > > > I.e. the driver is maintaining its own PM state machine. Fully
> > > > cooperating the driver with PM core should solve the issue.
> > > >
> > >
> > > System not call start_run() because do not have any sd_request at that
> > > time, so we need to update value(pcr->is_runtime_suspended) at
> > > rtsx_pci_runtime_resume but if we only update value here not to call
> > > pm_runtime_get(), the if-statement always be FALSE at start_run()
> >
> > Sounds like the RPM refcount goes to zero so it was runtime suspended again.
> > The correct usage should be merging rtsx_pci_start_run() with
> > rtsx_pci_runtime_resume(), then guard each op of mmc_host_ops with
> > pm_runtime_get_sync() at the beginning and pm_runtime_idle() at and end of
> > each ops.
> >
> > Increase the RPM refcount in runtime resume routine to prevent the driver
> > from suspending doesn't really make sense to me.
> >
> > Kai-Heng
>
>
> We want to control the interval time between D0->D3
> This is for good user experience and more stable for our device.
> And this completely unaffected System suspend, because System want to enter S3
> need to make sure Device back to D0 and then go to S3 and put Device to D3 if Device support D3
Everything can become easier to incorporate the state machine to
pm_runtime_*() helpers directly.
Let me work on a patch to achieve it.
Kai-Heng
>
>
> >
> > > that is why we move this if-statement from start_run() to
> > > rtsx_pci_runtime_resume()
> > >
> > > > > >
> > > > > > > +
> > > > > > > mutex_lock(&pcr->pcr_mutex);
> > > > > > >
> > > > > > > rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03,
> > > > > > > 0x00); diff --git a/drivers/misc/cardreader/rtsx_pcr.h
> > > > > > > b/drivers/misc/cardreader/rtsx_pcr.h
> > > > > > > index daf057c4eea6..b93975268e6d 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
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > ------Please consider the environment before printing this e-mail.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-17 5:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 10:50 [PATCH] misc: rtsx: modify rtd3 flow Ricky WU
2022-01-13 11:32 ` Kai-Heng Feng
2022-01-14 6:56 ` Ricky WU
2022-01-14 7:29 ` Kai-Heng Feng
2022-01-14 8:51 ` Ricky WU
2022-01-14 12:09 ` Kai-Heng Feng
2022-01-17 3:34 ` Ricky WU
2022-01-17 5:11 ` Kai-Heng Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.