Re: [PATCH v2 5/6] soc: qcom: rpmh powerdomain driver

[Lina Iyer]

On Fri, May 25 2018 at 19:08 -0600, David Collins wrote:
> Hello Rajendra,
>
> On 05/25/2018 03:01 AM, Rajendra Nayak wrote:
> > +
> > +	to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
> > +
> > +	if (peer && peer->enabled)
> > +		to_active_sleep(peer, peer->corner, &peer_corner,
> > +				&peer_sleep_corner);
> > +
> > +	active_corner = max(this_corner, peer_corner);
> > +
> > +	ret = rpmhpd_send_corner(pd, RPMH_ACTIVE_ONLY_STATE, active_corner);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sleep_corner = max(this_sleep_corner, peer_sleep_corner);
> > +
> > +	return rpmhpd_send_corner(pd, RPMH_SLEEP_STATE, sleep_corner);
> > +}
>
> This aggregation function as well as the rpmhpd_send_corner() calls below
> are not sufficient for RPMh.  There are 3 states that must all be used:
> RPMH_ACTIVE_ONLY_STATE, RPMH_WAKE_ONLY_STATE, and RPMH_SLEEP_STATE.  The
> naming is somewhat confusing as rpmhpd is defining a different concept of
> active-only.
>
> For power domains without active-only or peers, only
> RPMH_ACTIVE_ONLY_STATE should be used.  This instructs RPMh to issue the
> request immediately.
>
> For power domains with active-only, requests will need to be made for all
> three.  active_corner would be sent for both RPMH_ACTIVE_ONLY_STATE (so
> that the request takes effect immediately) and RPMH_WAKE_ONLY_STATE (so
> that the request is inserted into the wake TCS).  sleep_corner would be
> sent for RPMH_SLEEP_STATE (so that the request is inserted into the sleep
> TCS).
>
> You can see how this is handled in the RPMh clock driver in patch [3].
>
> You may be able to get away with using only RPMH_SLEEP_STATE and
> RPMH_ACTIVE_ONLY_STATE assuming that you issue the RPMH_SLEEP_STATE
> request first due to the rpmh driver caching behavior added in the
> cache_rpm_request() function in [4].  However, could you please confirm
> with Lina that this usage will continue to work in the future?  I'm not
> sure what guarantees are made at the rpmh API level.
We expect to cache all active values into wake if there was a sleep
value already defined. Expect to continue this behavior in the future
as well. But it would be safer for you to send sleep and wake votes in
addition to active votes. It shouldn't add too much of an overhead.

-- Lina