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

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

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

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.