Re: [PATCH RESEND v3 2/3] drivers: qcom: rpmh-rsc: return if the controller is idle

[Lina Iyer]

On Wed, Mar 06 2019 at 15:12 -0700, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-03-04 09:14:50)
> > On Fri, Mar 01 2019 at 10:58 -0700, Stephen Boyd wrote:
> > >Quoting Lina Iyer (2019-02-27 14:29:13)
> > >> Hi Stephen,
> > >>
> > >> On Tue, Feb 26 2019 at 17:49 -0700, Stephen Boyd wrote:
> > >
> > >Ok, can you explain why it's even a problem for the TCSes to be active
> > >during suspend? I would hope that for suspend/resume, if this is
> > >actually a problem, the RPMh driver itself can block suspend with a
> > >driver suspend callback that checks for idleness.
> > The RSC can transmit TCS executed from Linux and when all the CPUs have
> > powered down, could execute a firmware in the RSC to deliver the sleep
> > state requests. The firmware cannot run when there are active requests
> > being processed. To ensure that case, we bail out of sleep or suspend,
> > when the last CPU is powering down, if there are active requests.
>
> Ok, do we actually bail out or just pick a shallower idle state that
> wouldn't trigger the firmware to run something that may conflict with
> the active requests (i.e. some light CPU sleep mode)? The commit text
> seems to imply we block certain idle states.
We bail out of idle and let cpuidle determine the state again. We don't
go into a shallower state.
> > >But I suspect that in
> > >the system wide suspend/resume case, any callers that could make TCS
> > >requests are child devices of the RPMh controller and therefore they
> > >would already be suspended if they didn't have anything pending they're
> > >waiting for a response on or they would be blocking suspend themselves
> > >if they're waiting for the response. So why are we even checking the
> > >TCSes in system suspend path at all? Assume that callers know what
> > >they're doing and will block suspend if they care?
> > >
> > In suspend, they probably would do what you mention above. All CPUs
> > might conincidentally be idle at the same idle, when a request is being
> > processed.
> >
> > >Following that same logic, is this more of an API that is planned for
> > >use by CPU idle? Where the case is much more of a runtime PM design.
> > >Even then, I don't get it. A device that's runtime active and making
> > >RPMh requests might need to block some forms of CPU idle states because
> > >a request hasn't been processed yet that may change the decision for
> > >certain deep idle states?
> > >
> > A process waiting on a RPMH request, may let the CPU go to sleep and
> > therefore this is a possibility.
>
> Ok thanks for the info. Can these details be included in the commit text
> so we don't lose sight of the bigger picture? And can this patch series
> be combined with a larger cpuidle/suspend patch series so we don't have
> to review this in isolation? I don't understand the need to add more
> APIs that aren't used yet.
Agreed.

--Lina