On 2/5/2020 11:37 PM, Evan Green wrote:
On Tue, Feb 4, 2020 at 8:14 PM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
On 2/5/2020 6:05 AM, Evan Green wrote:
On Mon, Feb 3, 2020 at 10:14 PM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
Currently rpmh ctrlr dirty flag is set for all cases regardless
of data is really changed or not.
Add changes to update it when data is updated to new values.
Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
---
drivers/soc/qcom/rpmh.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 035091f..c3d6f00 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -139,20 +139,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
existing:
switch (state) {
case RPMH_ACTIVE_ONLY_STATE:
- if (req->sleep_val != UINT_MAX)
+ if (req->sleep_val != UINT_MAX) {
req->wake_val = cmd->data;
+ ctrlr->dirty = true;
+ }
Don't you need to set dirty = true for ACTIVE_ONLY state always? The
conditional is just saying "if nobody set a sleep vote, then maintain
this vote when we wake back up".
The ACTIVE_ONLY vote is cached as wake_val to be apply when wakeup happens.
In case value didn't change,wake_val is still same as older value and
there is no need to mark the entire cache as dirty.
Ah, I see it now. We don't actually cache active_only votes anywhere,
since they're one time requests. The sleep/wake votes seem to be the
only thing that gets cached.
I was thinking it might be safer to also set dirty = true just after
list_add_tail, since in the non-existing case this is a new batch that
RPMh has never seen before and should always be written. But I suppose
your checks here should cover that case, since sleep_val and wake_val
are initialized to UINT_MAX. If you think the code might evolve, it
might still be nice to add it.
current change seems good.
While I'm looking at that, why do we have this needless INIT_LIST_HEAD?
INIT_LIST_HEAD(&req->list);
list_add_tail(&req->list, &ctrlr->cache);
-Evan
Thanks for pointing this, i will remove this unnecessary INIT in next
revision.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
