Re: [PATCH v10 2/3] soc: qcom: rpmh: Update dirty flag only when data changes

[Doug Anderson]

Hi,

On Thu, Mar 5, 2020 at 3:10 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> > To summarize:
> >
> > a) If the only allowable use of "WAKE_ONLY" is to undo "SLEEP_ONLY"
> > then we should re-think the API and stop letting callers to
> > rpmh_write(), rpmh_write_async(), or rpmh_write_batch() ever specify
> > "WAKE_ONLY".  The code should just assume that "wake_only =
> > active_only if (active_only != sleep_only)".  In other words, RPMH
> > should programmatically figure out the "wake" state based on the
> > sleep/active state and not force callers to do this.
> >
> > b) If "WAKE_ONLY" is allowed to do other things (or if it's not RPMH's
> > job to enforce/assume this) then we should fully skip calling
> > cache_rpm_request() for RPMH_ACTIVE_ONLY_STATE.
> >
> >
> > NOTE: this discussion also makes me wonder about the is_req_valid()
> > function.  That will skip sending a sleep/wake entry if the sleep and
> > wake entries are equal to each other.  ...but if sleep and wake are
> > both different than "active" it'll be a problem.
>
> Hi Doug,
>
> To answer above points, yes in general itâs the understanding that wake is
> almost always need to be equal to active. However, there can be valid reasons
> for which the callers are enforced to call them differently in the first place.
>
> At present caller send 3 types of request.
> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99);
> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0);
> rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x99);
>
> Now, Lets assume we handle this in rpmh driver since wake=active and the caller
> send only 2 type of request (lets call it active and sleep, since we have assumption
> of wake=active, and we donât want 3rd request as its handled in rpmh driver)
> So callers will now invoke 2 types of request.
>
> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99);
> rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0);
>
> with first type request, it now needs to serve 2 purpose
> (a)    cache ACTIVE request votes as WAKE votes
> (b)    trigger it out immediately (in ACTIVE TCS) as it need to be also complete immediately.
>
> For SLEEP, nothing changes. Now when entering to sleep we do rpmh_flush() to program all
> WAKE and SLEEP requestâso far so goodâ
>
> Now consider a corner case,
>
> There is something called a solver mode in RSC where HW could be in autonomous mode executing
> low power modes. For this it may want to âonlyâ program WAKE and SLEEP votes and then controller
> would be in solver mode entering and exiting sleep autonomously.
>
> There is no ACTIVE set request and hence no requirement to send it right away as ACTIVE vote.
>
> If we have only 2 type of request, caller again need to differentiate to tell rpmh driver that
> when it invoke
>
> rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99);
>
> with this caching it as WAKE is fine  ((a) in above) but do not trigger it ((b) in above)
>
> so we need to again modify this API and pass another argument saying whether to do (a + b) or only (a).
> but caller can already differentiate by using RPMH_ACTIVE_ONLY_STATE or RPMH_WAKE_ONLY_STATE.
>
> i think at least for now, leave it as it is, unless we really see any impact by caller invoking all
> 3 types of request and take in account all such corner cases before i make any such change.
> we can take it separate if needed along with optimization pointed in v9 series discussions.

I totally don't understand what solver mode is for and when it's used,
but I'm willing to set that aside for now I guess.  From looking at
what you did for v12 it looks like the way you're expecting things to
function is this:

* ACTIVE: set wake state and trigger active change right away.

* SLEEP: set only sleep state

* WAKE: set only wake state, will take effect after next sleep/wake
unless changed again before that happens.


...I'll look at the code with this understanding, now.  Presumably also:

* We should document this.

* If we see clients that are explicitly setting _both_ active and wake
to the same thing then we can change the clients.  That means the only
people using "WAKE" mode would be those clients that explicitly want
the deferred action (presumably those using "solver" mode).

Do those seem correct?


If that's correct, I guess one subtle corner-case bug in
is_req_valid().  Specifically if it's ever valid to do this:

rpmh_write(RPMH_ACTIVE_ONLY_STATE, addr=0x10, data=0x99);
rpmh_write(RPMH_SLEEP_STATE, addr=0x10, data=0x0);
rpmh_write(RPMH_WAKE_ONLY_STATE, addr=0x10, data=0x0);

...then it won't work.  You'll transition between sleep/wake and stay
with "data=0x99".  Since "sleep == wake" then is_req_valid() will
return "false" and we won't bother programming the commands for
sleep/wake.  One simple way to solve this is remove the
"req->sleep_val != req->wake_val" optimization in is_req_valid().


I guess we should also document that "batch" doesn't work like this.
The "batch" API is really designed around having exactly one "batch"
caller (the interconnect code) and we assume that the batch code will
be sending us pre-optimized commands I guess?  Specifically there
doesn't seem to be anything trying to catch batch writes to "active"
and also applying them to "wake".


-Doug