Invoke rpmh_flush for non OSI targets

TCSes have previously programmed data when rpmh_flush is called.

This can cause old data to trigger along with newly flushed.

Fix this by cleaning SLEEP and WAKE TCSes before new data is flushed.

With this there is no need to invoke rpmh_rsc_invalidate() call from rpmh_invalidate().

Simplify rpmh_invalidate() by moving invalidate_batch() inside.

Fixes 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests")

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

drivers/soc/qcom/rpmh.c +4 -8

index 03630ae..a75f3df 100644
@@ -498,24 +492,25 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 }
 
 /**
- * rpmh_invalidate: Invalidate all sleep and active sets
- * sets.
+ * rpmh_invalidate: Invalidate sleep and wake sets in batch_cache
  *
  * @dev: The device making the request
  *
- * Invalidate the sleep and active values in the TCS blocks.
+ * Invalidate the sleep and wake values in batch_cache.
  */
 int rpmh_invalidate(const struct device *dev)
 {
        struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
-       int ret;
-
-       invalidate_batch(ctrlr);
+       struct batch_cache_req *req, *tmp;
+       unsigned long flags;
 
-       do {
-               ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));

Quoting Maulik Shah (2020-04-05 23:32:18)

Quoted 1 line

TCSes have previously programmed data when rpmh_flush is called.

rpmh_flush()

Quoted 53 lines

This can cause old data to trigger along with newly flushed.

Fix this by cleaning SLEEP and WAKE TCSes before new data is flushed.

With this there is no need to invoke rpmh_rsc_invalidate() call from rpmh_invalidate().

Simplify rpmh_invalidate() by moving invalidate_batch() inside.

Fixes 600513dfeef3 ("drivers: qcom: rpmh: cache sleep/wake state requests")

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
  drivers/soc/qcom/rpmh.c | 41 ++++++++++++++++++-----------------------
  1 file changed, 18 insertions(+), 23 deletions(-)

drivers/soc/qcom/rpmh.c +10 -9

index 03630ae..a75f3df 100644
@@ -498,24 +492,25 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
  }
 
  /**
- * rpmh_invalidate: Invalidate all sleep and active sets
- * sets.
+ * rpmh_invalidate: Invalidate sleep and wake sets in batch_cache
   *
   * @dev: The device making the request
   *
- * Invalidate the sleep and active values in the TCS blocks.
+ * Invalidate the sleep and wake values in batch_cache.
   */
  int rpmh_invalidate(const struct device *dev)
  {
         struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
-       int ret;
-
-       invalidate_batch(ctrlr);
+       struct batch_cache_req *req, *tmp;
+       unsigned long flags;
 
-       do {
-               ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
-       } while (ret == -EAGAIN);
+       spin_lock_irqsave(&ctrlr->cache_lock, flags);
+       list_for_each_entry_safe(req, tmp, &ctrlr->batch_cache, list)
+               kfree(req);
+       INIT_LIST_HEAD(&ctrlr->batch_cache);
+       ctrlr->dirty = true;
+       spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 

drivers/soc/qcom/rpmh.c int rpmh_flush(struct rpmh_ctrlr *ctrlr) · line 514

Quoted 2 lines

drivers/soc/qcom/rpmh.c +1 -1

-       return ret;
+       return 0;

Now this always returns 0. Maybe it should become a void function, but doing that requires a change in the interconnect code so maybe do it later.

Add changes to invoke rpmh flush() from CPU PM notification. This is done when the last the cpu is entering power collapse and controller is not busy.

Controllers that do have 'HW solver' mode do not need to register

for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
 drivers/soc/qcom/rpmh-internal.h |  25 +++++---
 drivers/soc/qcom/rpmh-rsc.c      | 123 +++++++++++++++++++++++++++++++++++----
 drivers/soc/qcom/rpmh.c          |  26 +++------
 3 files changed, 137 insertions(+), 37 deletions(-)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index b718221..fbe1f3e 100644
@@ -6,6 +6,7 @@

+ cpumask_set_cpu(raw_smp_processor_id(),

drivers/soc/qcom/rpmh-rsc.c +29 -0

+                               &drv->cpus_entered_pm);
+
+               if (!cpumask_equal(&drv->cpus_entered_pm, cpu_online_mask))
+                       goto exit;
+               break;
+       case CPU_PM_ENTER_FAILED:
+       case CPU_PM_EXIT:
+               cpumask_clear_cpu(raw_smp_processor_id(),
+                                 &drv->cpus_entered_pm);
+               goto exit;
+       }
+
+       ret = rpmh_rsc_ctrlr_is_busy(drv);
+       if (ret) {
+               ret = NOTIFY_BAD;
+               goto exit;
+       }
+
+       ret = rpmh_flush(&drv->client);
+       if (ret)
+               ret = NOTIFY_BAD;
+       else
+               ret = NOTIFY_OK;
+
+exit:
+       spin_unlock(&drv->pm_lock);
+       return ret;
+}
+

Quoting Maulik Shah (2020-04-05 23:32:19)

Quoted 5 lines

Add changes to invoke rpmh flush() from CPU PM notification. This is done when the last the cpu is entering power collapse and controller is not busy.

Controllers that do have 'HW solver' mode do not need to register

Controllers that have 'HW solver' mode don't need to register? The 'do have' is throwing me off.

Quoted 17 lines

for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
  drivers/soc/qcom/rpmh-internal.h |  25 +++++---
  drivers/soc/qcom/rpmh-rsc.c      | 123 +++++++++++++++++++++++++++++++++++----
  drivers/soc/qcom/rpmh.c          |  26 +++------
  3 files changed, 137 insertions(+), 37 deletions(-)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index b718221..fbe1f3e 100644
@@ -6,6 +6,7 @@

[...]

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 577

Quoted 11 lines

drivers/soc/qcom/rpmh-rsc.c +11 -0

+
+static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *v)
+{
+       struct rsc_drv *drv = container_of(nfb, struct rsc_drv, rsc_pm);
+       int ret = NOTIFY_OK;
+
+       spin_lock(&drv->pm_lock);
+
+       switch (action) {
+       case CPU_PM_ENTER:

I thought CPU_PM notifiers weren't supposed to be used anymore? Or at least, the genpd work that has gone on for cpuidle could be used here in place of CPU_PM notifiers?

And so this isn't actually any different
than what was proposed originally to use genpd for this?

Quoted 1 line

+ cpumask_set_cpu(raw_smp_processor_id(),

Why do we need to use raw_smp_processor_id()? smp_processor_id() should work just as well?

Quoted 22 lines

drivers/soc/qcom/rpmh-rsc.c +21 -0

+                               &drv->cpus_entered_pm);
+
+               if (!cpumask_equal(&drv->cpus_entered_pm, cpu_online_mask))
+                       goto exit;
+               break;
+       case CPU_PM_ENTER_FAILED:
+       case CPU_PM_EXIT:
+               cpumask_clear_cpu(raw_smp_processor_id(),
+                                 &drv->cpus_entered_pm);
+               goto exit;
+       }
+
+       ret = rpmh_rsc_ctrlr_is_busy(drv);
+       if (ret) {
+               ret = NOTIFY_BAD;
+               goto exit;
+       }
+
+       ret = rpmh_flush(&drv->client);
+       if (ret)
+               ret = NOTIFY_BAD;

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 607

Quoted 8 lines

drivers/soc/qcom/rpmh-rsc.c +8 -0

+       else
+               ret = NOTIFY_OK;
+
+exit:
+       spin_unlock(&drv->pm_lock);
+       return ret;
+}
+

Hi,

Quoted 19 lines · Stephen Boyd

Quoting Maulik Shah (2020-04-05 23:32:19)

Quoted 17 lines

for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
  drivers/soc/qcom/rpmh-internal.h |  25 +++++---
  drivers/soc/qcom/rpmh-rsc.c      | 123 +++++++++++++++++++++++++++++++++++----
  drivers/soc/qcom/rpmh.c          |  26 +++------
  3 files changed, 137 insertions(+), 37 deletions(-)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index b718221..fbe1f3e 100644
@@ -6,6 +6,7 @@

[...]

Quoted 14 lines

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 577

Quoted 11 lines

drivers/soc/qcom/rpmh-rsc.c +11 -0

+
+static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *v)
+{
+       struct rsc_drv *drv = container_of(nfb, struct rsc_drv, rsc_pm);
+       int ret = NOTIFY_OK;
+
+       spin_lock(&drv->pm_lock);
+
+       switch (action) {
+       case CPU_PM_ENTER:

I thought CPU_PM notifiers weren't supposed to be used anymore? Or at least, the genpd work that has gone on for cpuidle could be used here in place of CPU_PM notifiers?

genpd was used in v3 and v4 of this series, where from pd's .power_off function, rpmh_flush() was invoked.

genpd can be useful if target firmware supports PSCI's OSI mode, while sc7180 is non-OSI target.

The current approch (using cpu pm notification) can be used for both OSI and non-OSI targets to invoke rpmh_flush() when last cpu goes to power down.

Quoted 4 lines

And so this isn't actually any different
than what was proposed originally to use genpd for this?

Quoting Maulik Shah (2020-04-08 00:08:48)

Quoted 36 lines

Hi,

Quoted 19 lines · Stephen Boyd

Quoting Maulik Shah (2020-04-05 23:32:19)

Quoted 17 lines

for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

---
   drivers/soc/qcom/rpmh-internal.h |  25 +++++---
   drivers/soc/qcom/rpmh-rsc.c      | 123 +++++++++++++++++++++++++++++++++++----
   drivers/soc/qcom/rpmh.c          |  26 +++------
   3 files changed, 137 insertions(+), 37 deletions(-)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index b718221..fbe1f3e 100644
@@ -6,6 +6,7 @@

[...]

Quoted 14 lines

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 577

Quoted 11 lines

drivers/soc/qcom/rpmh-rsc.c +11 -0

+
+static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *v)
+{
+       struct rsc_drv *drv = container_of(nfb, struct rsc_drv, rsc_pm);
+       int ret = NOTIFY_OK;
+
+       spin_lock(&drv->pm_lock);
+
+       switch (action) {
+       case CPU_PM_ENTER:

I thought CPU_PM notifiers weren't supposed to be used anymore? Or at least, the genpd work that has gone on for cpuidle could be used here in place of CPU_PM notifiers?

Quoted 8 lines

genpd was used in v3 and v4 of this series, where from pd's .power_off function, rpmh_flush() was invoked.

genpd can be useful if target firmware supports PSCI's OSI mode, while sc7180 is non-OSI target.

The current approch (using cpu pm notification) can be used for both OSI and non-OSI targets to invoke rpmh_flush() when last cpu goes to power down.

Ok. Doug and I talked today and I re-read the earlier series and I think Sudeep was suggesting that if we're doing last man down activities here then we're better off using OSI vs. PC mode. But I can only assume that's because the concern is something here requires software's help for last man down activities like lowering a CPU voltage setting or turning off some power switch to a hardware block through some i2c message. The way I understand it the last man down activities here are just setting up the sleep and wake TCS FIFOs to "do the right thing" when the last CPU actually goes down and the first CPU wakes up by running through the pile of "instructions" that we program into the FIFOs.

The execution of those instructions is all done in hardware so any aggregation or coordination between CPUs is not really important here. All that matters is that we set up the sleep and wake TCS FIFOs properly so that _if_ the whole CPU subsystem goes to sleep we're going to let the hardware turn off the right stuff and lower voltages, etc. and vice-versa for wake. If we didn't have to share the TCS FIFOs with active mode control then we could just tweak the sleep and wake TCS buckets at runtime and let the hardware state of the CPUs decide to trigger them at the right time.

Unfortunately, we don't have that luxury
and we're stuck repurposing the sleep TCS FIFO to control things like regulator voltages when the CPU is awake. Yuck!

Quoted 4 lines

Quoted 4 lines

And so this isn't actually any different
than what was proposed originally to use genpd for this?

I guess this answer to this is yes. Which is fine. CPU PM notifiers are still used by various drivers to do things like save/restore state of devices that lose state when the CPUs power down. The use of genpd is helpful for OSI mode because it can describe how/when big and little clusters are powered off by putting them in different genpds. For counting the last CPU to turn off it seems simpler to just register for CPU PM notifiers and not care about genpd logic and nesting clusters, etc. I'm happy to see this not be a blocker.

Add changes to invoke rpmh flush() from CPU PM notification. This is done when the last the cpu is entering power collapse and

controller is not busy.

Controllers that do have 'HW solver' mode do not need to register for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

drivers/soc/qcom/rpmh-rsc.c +5 -0

index b718221..fbe1f3e 100644
@@ -521,8 +527,86 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
        return tcs_ctrl_write(drv, msg);
 }
 
+/**
+ * rpmh_rsc_ctrlr_is_busy() - Check if any of the AMCs are busy.
+ *
+ * @drv: The controller
+ *

drivers/soc/qcom/rpmh-rsc.c +14 -0

+        *
+        * Since this is called from the last cpu, need not take drv or tcs
+        * lock before checking tcs_is_free().
+        */
+       if (!tcs->num_tcs)
+               tcs = get_tcs_of_type(drv, WAKE_TCS);
+
+       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+               if (!tcs_is_free(drv, m))
+                       return true;
+       }
+
+       return false;
+}

drivers/soc/qcom/rpmh-rsc.c +1 -0

+

drivers/soc/qcom/rpmh.c +0 -0

index a75f3df..88f8801 100644
@@ -433,16 +430,17 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
 }
 
 /**

drivers/soc/qcom/rpmh.c +1 -3

 {

@@ -455,9 +453,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
        }
 
        /* Invalidate the TCSes first to avoid stale data */
-       do {
-               ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
-       } while (ret == -EAGAIN);
+       ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
        if (ret)
                return ret;
 

Quoted 2 lines

int rpmh_flush(struct rpmh_ctrlr *ctrlr)

This function name keeps throwing me off. Can we please call it something like rpmh_configure_tcs_sleep_wake()? The word "flush" seems to imply there's some sort of cache going on, but that's not really the case. We're programming a couple TCS FIFOs so that they can be used across deep CPU sleep states.

Hi,

Quoted 6 lines · Stephen Boyd

Quoted 1 line

int rpmh_flush(struct rpmh_ctrlr *ctrlr)

This function name keeps throwing me off. Can we please call it something like rpmh_configure_tcs_sleep_wake()? The word "flush" seems to imply there's some sort of cache going on, but that's not really the case. We're programming a couple TCS FIFOs so that they can be used across deep CPU sleep states.

I'm hoping this rename can be deferred until Maulik's series and my cleanup series land. While I agree that rpmh_flush() is a bit of a confusing name, it's not a new name and renaming it midway through when there are a bunch of patches in-flight is going to be a hassle.

Assuming others agree, my thought is that Maulik will do one more v17 spin with small nits fixed up, then his series can land early next week when (presumably) -rc1 comes out. If my current cleanup doesn't apply cleanly (or if Bjorn/Andy don't want to fix the two nits in commit messages when applying) I can repost my series and that can land in short order. Once those land:

* Maulik can post this rpmh_flush() rename atop.

* I can post the patch to remove the "pm_lock" that was introduced in this series. A preview at <https://crrev.com/c/2142823>.

* Maulik can post some of the cleanups that Maulik wanted to do in rpmh.c with regards to __fill_rpmh_msg()

...assuming those are not controversial perhaps they can be reviewed quickly and land quickly? I suppose I can always dream...

-Doug

Quoting Maulik Shah (2020-04-05 23:32:19)

Quoted 2 lines

Add changes to invoke rpmh flush() from CPU PM notification. This is done when the last the cpu is entering power collapse and

'power collapse' is a qcom-ism. Maybe say something like "deep CPU idle states"?

Quoted 7 lines

controller is not busy.

Controllers that do have 'HW solver' mode do not need to register for CPU PM notification. They may be in autonomous mode executing low power mode and do not require rpmh_flush() to happen from CPU PM notification.

Can you provide an example of a HW solver mode controller? Presumably the display RSC is one of these?

Quoted 19 lines

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

drivers/soc/qcom/rpmh-rsc.c +9 -0

index b718221..fbe1f3e 100644
@@ -521,8 +527,86 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
         return tcs_ctrl_write(drv, msg);
  }
 
+/**
+ * rpmh_rsc_ctrlr_is_busy() - Check if any of the AMCs are busy.
+ *
+ * @drv: The controller
+ *
+ * Checks if any of the AMCs are busy in handling ACTIVE sets.
+ * This is called from the last cpu powering down before flushing
+ * SLEEP and WAKE sets. If AMCs are busy, controller can not enter
+ * power collapse, so deny from the last cpu's pm notification.

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 544

Quoted 6 lines

drivers/soc/qcom/rpmh-rsc.c +6 -0

+ *
+ * Return:
+ * * False             - AMCs are idle
+ * * True              - AMCs are busy
+ */
+static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)

Can drv be const? Would be nice to make it const in some places in this driver.

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 552

Quoted 9 lines

drivers/soc/qcom/rpmh-rsc.c +8 -0

+{
+       int m;
+       struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
+
+       /*
+        * If we made an active request on a RSC that does not have a
+        * dedicated TCS for active state use, then re-purposed wake TCSes
+        * should be checked for not busy.

not busy, because we use wake TCSes for active requests in this case.

Quoted 15 lines

drivers/soc/qcom/rpmh-rsc.c +14 -0

+        *
+        * Since this is called from the last cpu, need not take drv or tcs
+        * lock before checking tcs_is_free().
+        */
+       if (!tcs->num_tcs)
+               tcs = get_tcs_of_type(drv, WAKE_TCS);
+
+       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+               if (!tcs_is_free(drv, m))
+                       return true;
+       }
+
+       return false;
+}

[...]

Quoted 8 lines

drivers/soc/qcom/rpmh-rsc.c +1 -0

+

drivers/soc/qcom/rpmh.c +0 -0

index a75f3df..88f8801 100644
@@ -433,16 +430,17 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
  }
 

drivers/soc/qcom/rpmh.c static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state, · line 438

Quoted 11 lines

drivers/soc/qcom/rpmh.c +4 -4

  /**
- * rpmh_flush: Flushes the buffered active and sleep sets to TCS
+ * rpmh_flush() - Flushes the buffered sleep and wake sets to TCSes
   *
- * @ctrlr: controller making request to flush cached data
+ * @ctrlr: Controller making request to flush cached data
   *
- * Return: -EBUSY if the controller is busy, probably waiting on a response
- * to a RPMH request sent earlier.
+ * This function is called from sleep code on the last CPU
+ * (thus no spinlock needed).

Might be good to stick a lockdep_assert_irqs_disabled() in this function then. That would document that this function should only be called when irqs are disabled.

drivers/soc/qcom/rpmh.c static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state, · line 445

Quoted 11 lines

drivers/soc/qcom/rpmh.c +4 -3

   *
- * This function is always called from the sleep code from the last CPU
- * that is powering down the entire system. Since no other RPMH API would be
- * executing at this time, it is safe to run lockless.
+ * Return:
+ * * 0          - Success
+ * * -EAGAIN    - Retry again
+ * * Error code - Otherwise
   */
  int rpmh_flush(struct rpmh_ctrlr *ctrlr)

This function name keeps throwing me off. Can we please call it something like rpmh_configure_tcs_sleep_wake()? The word "flush" seems to imply there's some sort of cache going on, but that's not really the case. We're programming a couple TCS FIFOs so that they can be used across deep CPU sleep states.

Quoted 4 lines

drivers/soc/qcom/rpmh.c +0 -0

  {

@@ -455,9 +453,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
         }
 

drivers/soc/qcom/rpmh.c int rpmh_flush(struct rpmh_ctrlr *ctrlr) · line 458

Quoted 7 lines

drivers/soc/qcom/rpmh.c +1 -3

         /* Invalidate the TCSes first to avoid stale data */
-       do {
-               ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
-       } while (ret == -EAGAIN);
+       ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
         if (ret)
                 return ret;