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

[Kai-Heng Feng]

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?

>
> 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.