All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
To: Ricky WU <ricky_wu@realtek.com>
Cc: "arnd@arndb.de" <arnd@arndb.de>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"christophe.jaillet@wanadoo.fr" <christophe.jaillet@wanadoo.fr>,
	"yang.lee@linux.alibaba.com" <yang.lee@linux.alibaba.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] misc: rtsx: modify rtd3 flow
Date: Fri, 14 Jan 2022 20:09:18 +0800	[thread overview]
Message-ID: <CAAd53p7nHqaKcZx7+asZz5k2jRJK-nJeCM21Nhvw6AeVE3=Qpw@mail.gmail.com> (raw)
In-Reply-To: <ccb30393c86e42f489c2d7445e3534a9@realtek.com>

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.

  reply	other threads:[~2022-01-14 12:10 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2022-01-17  3:34           ` Ricky WU
2022-01-17  5:11             ` Kai-Heng Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAd53p7nHqaKcZx7+asZz5k2jRJK-nJeCM21Nhvw6AeVE3=Qpw@mail.gmail.com' \
    --to=kai.heng.feng@canonical.com \
    --cc=arnd@arndb.de \
    --cc=christophe.jaillet@wanadoo.fr \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ricky_wu@realtek.com \
    --cc=yang.lee@linux.alibaba.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.