RE: [PATCH] misc: rtsx: modify rtd3 flow

[Ricky WU <ricky_wu@realtek.com>]

> -----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, &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) {
> > +       /* 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.