From: Sibi Sankar <email@example.com> To: Stephen Boyd <firstname.lastname@example.org> Cc: Doug Anderson <email@example.com>, 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>, email@example.com Subject: Re: [PATCH 1/2] soc: qcom: aoss: Don't wait for IRQ if we might be in suspend/resume noirq Date: Thu, 06 Aug 2020 20:04:23 +0530 Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> On 2020-08-06 04:32, Stephen Boyd wrote: > +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. Doug/Stephen, Yes this is a bug, we wouldn't want to disable aoss_qmp genpd for modem during suspend (when the modem is running). The qmp send for modem is the primary means through which aoss determines whether to wait for modem before proceeding to sleep. So looks like updating the flag with GENPD_FLAG_ACTIVE_WAKEUP is the way to go. But introducing another flag that doesn't touch genpd's during suspend/resume should also work. >> >> 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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
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 2020-08-06 14:34 ` Sibi Sankar [this message] 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 \ --firstname.lastname@example.org \ /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 \ email@example.com 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