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

[Kai-Heng Feng]

On Mon, Jan 17, 2022 at 11:34 AM Ricky WU <ricky_wu@xxxxxxxxxxx> wrote:
>
> > -----Original Message-----
> > From: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > Sent: Friday, January 14, 2022 8:09 PM
> > To: Ricky WU <ricky_wu@xxxxxxxxxxx>
> > Cc: arnd@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> > christophe.jaillet@xxxxxxxxxx; yang.lee@xxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
> >
> > On Fri, Jan 14, 2022 at 4:51 PM Ricky WU <ricky_wu@xxxxxxxxxxx> 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.