From: Stephen Boyd <email@example.com> To: Doug Anderson <firstname.lastname@example.org>, Sibi Sankar <email@example.com> Cc: Bjorn Andersson <firstname.lastname@example.org>, Alex Elder <email@example.com>, Matthias Kaehlcke <firstname.lastname@example.org>, Andy Gross <email@example.com>, Vinod Koul <firstname.lastname@example.org>, linux-arm-msm <email@example.com>, LKML <firstname.lastname@example.org> 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: <email@example.com> (raw) In-Reply-To: <CAD=FV=UK+xHV6qsr2AsPB=BzmzN77AT-du8G2tt1QZMQUpGgKg@mail.gmail.com> +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 <firstname.lastname@example.org> 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, > GENPD_FLAG_ACTIVE_WAKEUP, GENPD_FLAG_RPM_ALWAYS_ON makes me even more > 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 > GENPD_FLAG_ALWAYS_ON. > > 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.
next prev parent 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: 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 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/ https://lore.kernel.org/lkml \ firstname.lastname@example.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git