LKML Archive on
 help / color / Atom feed
From: Stephen Boyd <>
To: Doug Anderson <>,
	Sibi Sankar <>
Cc: Bjorn Andersson <>,
	Alex Elder <>,
	Matthias Kaehlcke <>,
	Andy Gross <>, Vinod Koul <>,
	linux-arm-msm <>,
	LKML <>
Subject: Re: [PATCH 1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq
Date: Wed, 05 Aug 2020 16:02:05 -0700
Message-ID: <> (raw)
In-Reply-To: <>

+Sibi who wrote the code

Quoting Doug Anderson (2020-08-05 13:24:06)
> On Wed, Aug 5, 2020 at 10:36 AM Stephen Boyd <> wrote:
> >
> > Why is the genpd being powered off at all? It looks like the driver is
> > written in a way that it doesn't expect this to happen. See where
> > adsp_pds_disable() is called from. Looks like the remoteproc "stop"
> > callback should be called or the driver should be detached.
> >
> > It sort of looks like the genpd is expected to be at the max level all
> > the time (it sets INT_MAX in adsp_pds_enable(), cool).
> In general in Linux there are some things that, at suspend time, get
> done behind a driver's back.  The regulator API, for instance, allows
> for regulators to be turned off in suspend even if a driver leaves
> them on.  Sure, it's good practice for a driver to be explicit but the
> regulator suspend states do allow for the more heavy-handed approach.
> I guess I assume that genpd is a bit similar.  If a driver leaves a
> genpd on all the time then it will still be turned off at suspend time
> and then turned back on at resume time.  It seems like it must be part
> of the genpd API.  Specifically genpd_sync_power_off() says: "Check if
> the given PM domain can be powered off (during system suspend or
> hibernation) and do that if so."  That makes it seem like it's how
> genpd works.
> Reading all the descriptions of things like GENPD_FLAG_ALWAYS_ON,
> convinced that it's normal (unless otherwise specified) for genpds to
> get turned off in suspend even if a driver just blindly left them on.
> Presumably if this "modem" genpd is supposed to stay on in suspend
> time it should have been marked "always on"?  I'd guess we'd need to
> add "GENPD_FLAG_ALWAYS_ON" in some (or all?) cases in qmp_pd_add() if
> this was true?

Agreed. I can't read the mind of Sibi so I can only guess that Sibi
wasn't expecting this behavior by reading the driver structure. That
could be a wrong assumption.

> > Maybe we need to
> > add some sort of suspend hooks to the remote proc driver instead? Where
> > those suspend hooks are called earlier and drop the genpd performance
> > state request but otherwise leave it enabled across suspend?
> I think you're saying:
> a) You think it's a bug today that the "modem" genpd is being powered
> off in suspend.  Any evidence to back this up?
> b) Assuming it's a bug today, we should mark the "modem" as
> c) If there are genpds that sometimes should be left on in suspend but
> sometimes not (and that doesn't match up with what
> GENPD_FLAG_ACTIVE_WAKEUP does), then we'd have to pass
> GENPD_FLAG_ALWAYS_ON as a flag and then add suspend hooks to make the
> decision for us.
> Did I understand that correctly?
> ...or are you suggesting that we work around the fact that
> qmp_pd_power_off() can't be called at "noirq" time by forcing it to
> suspend earlier?
> ...or am I just totally confused and you meant something else?
> > I know this isn't clearing the land mine that is calling this code from
> > noirq phase of suspend, but I'm just looking at the driver and thinking
> > that it never expected to be called from this phase of suspend to begin
> > with.
> You're saying that qmp_pd_power_off() wasn't expecting to be called
> from the noirq phase of suspend?  Sure, I guess not given the bug.
> ...but once we fix the bug, it works fine, doesn't it?  ...and it
> appears that it's part of the genpd API to be able to be called from
> the noirq phase.  To me that means that, even if we were supposed to
> be keeping this particular PD on during suspend we should take my
> patch.
> So the summary is: I still think my patch is correct, but I could
> certainly still be convinced otherwise.

I'm trying to say that the driver looks like it expects to power off the
genpd in the adsp_stop() callback. That same callback sends some sort of
message to the modem saying that it is being stopped (see
qcom_q6v5_request_stop()). Turning the performance state down, or
turning the power domain off completely, without telling the modem that
it's happening like as is done in adsp_stop() looks wrong. But who
knows, maybe the modem is happy with that and doesn't care?

In general, the whole thing looks weird to me because I would expect the
modem to take care of its own power requirements, including this
"load_state" one. Anyway, I hope Sibi can clarify what's going on.

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-05 16:16 Douglas Anderson
2020-08-05 16:16 ` [PATCH 2/2] soc: qcom: aoss: qmp_send() can't handle interruptions so stop pretending Douglas Anderson
2020-08-05 17:28   ` Stephen Boyd
2020-08-05 20:25     ` Doug Anderson
2020-08-05 17:36 ` [PATCH 1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq Stephen Boyd
2020-08-05 20:24   ` Doug Anderson
2020-08-05 23:02     ` Stephen Boyd [this message]
2020-08-06 14:34       ` Sibi Sankar
2020-08-06 17:10         ` Doug Anderson
2020-08-06 17:33           ` Sibi Sankar
2020-08-11 21:21             ` Doug Anderson

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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on

Archives are clonable:
	git clone --mirror lkml/git/0.git
	git clone --mirror lkml/git/1.git
	git clone --mirror lkml/git/2.git
	git clone --mirror lkml/git/3.git
	git clone --mirror lkml/git/4.git
	git clone --mirror lkml/git/5.git
	git clone --mirror lkml/git/6.git
	git clone --mirror lkml/git/7.git
	git clone --mirror lkml/git/8.git
	git clone --mirror lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ \
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone