Invoke rpmh_flush for non OSI targets

Add device bindings for cpuidle states for cpu devices.

Cc [email protected]

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

Reviewed-by Srinivas Rao L <[email protected]>

---

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 newer values.

Also move dirty flag updates to happen from within cache_lock.

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

Reviewed-by Srinivas Rao L <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

drivers/soc/qcom/rpmh.c +7 -2

index eb0ded0..83ba4e0 100644
@@ -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;
+               }
                break;
        case RPMH_WAKE_ONLY_STATE:
-               req->wake_val = cmd->data;
+               if (req->wake_val != cmd->data) {
+                       req->wake_val = cmd->data;
+                       ctrlr->dirty = true;
+               }

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

        }
 
-       ctrlr->dirty = true;
 unlock:
        spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 

@@ -323,6 +331,7 @@ static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
        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);
 }
 

@@ -456,6 +465,7 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
 int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 {
        struct cache_req *p;
+       unsigned long flags;
        int ret;
 
        if (!ctrlr->dirty) {

Quoting Maulik Shah (2020-02-25 21:27:12)

Quoted 8 lines

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 newer values.

Also move dirty flag updates to happen from within cache_lock.

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

Reviewed-by Srinivas Rao L <[email protected]>

Probably worth adding a Fixes tag here? Doesn't make sense to mark something dirty when it isn't changed.

Quoted 30 lines

---
  drivers/soc/qcom/rpmh.c | 21 ++++++++++++++++-----
  1 file changed, 16 insertions(+), 5 deletions(-)

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

index eb0ded0..83ba4e0 100644
@@ -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;
+               }
                 break;
         case RPMH_WAKE_ONLY_STATE:
-               req->wake_val = cmd->data;
+               if (req->wake_val != cmd->data) {
+                       req->wake_val = cmd->data;
+                       ctrlr->dirty = true;
+               }
                 break;
         case RPMH_SLEEP_STATE:
-               req->sleep_val = cmd->data;
+               if (req->sleep_val != cmd->data) {
+                       req->sleep_val = cmd->data;
+                       ctrlr->dirty = true;

drivers/soc/qcom/rpmh.c static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, · line 160

Quoted 4 lines

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

+               }
                 break;
         default:
                 break;

Please remove the default case. There are only three states in the enum. The compiler will warn if a switch statement doesn't cover all cases and we'll know to add something here if another enum value is added in the future.

Quoted 26 lines

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

         }
 
-       ctrlr->dirty = true;
  unlock:
         spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 

@@ -323,6 +331,7 @@ static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
         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);
  }
 

@@ -456,6 +465,7 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
  int rpmh_flush(struct rpmh_ctrlr *ctrlr)
  {
         struct cache_req *p;
+       unsigned long flags;
         int ret;
 
         if (!ctrlr->dirty) {

@@ -488,7 +498,9 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
                         return ret;
         }
 

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

Quoted 3 lines

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

+       spin_lock_irqsave(&ctrlr->cache_lock, flags);
         ctrlr->dirty = false;
+       spin_unlock_irqrestore(&ctrlr->cache_lock, flags);

So we take the spinlock to update it here. But we don't hold the spinlock to test for !dirty up above. Seems like either rpmh_flush() can only be called sequentially, or the lock added here needs to be held during the whole flush. Which way is it?

Add changes to invoke rpmh flush when the data in cache is dirty.

This is done only if OSI is not supported in PSCI. If OSI is supported rpmh_flush can get invoked when the last cpu going to power collapse

deepest low power mode.

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

Reviewed-by Srinivas Rao L <[email protected]>

---
 drivers/soc/qcom/rpmh.c | 6 ++++++
 1 file changed, 6 insertions(+)

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

index 83ba4e0..839af8d 100644
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/psci.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>

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

                return 0;
        }
 

Quoting Maulik Shah (2020-02-25 21:27:13)

Quoted 4 lines

Add changes to invoke rpmh flush when the data in cache is dirty.

This is done only if OSI is not supported in PSCI. If OSI is supported rpmh_flush can get invoked when the last cpu going to power collapse

Please write rpmh_flush() so we know it's a function and not a variable.

Quoted 21 lines

deepest low power mode.

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

Reviewed-by Srinivas Rao L <[email protected]>

---
  drivers/soc/qcom/rpmh.c | 6 ++++++
  1 file changed, 6 insertions(+)

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

index 83ba4e0..839af8d 100644
@@ -12,6 +12,7 @@
  #include <linux/module.h>
  #include <linux/of.h>
  #include <linux/platform_device.h>
+#include <linux/psci.h>
  #include <linux/slab.h>
  #include <linux/spinlock.h>
  #include <linux/types.h>

drivers/soc/qcom/rpmh.c static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr, · line 167

Quoted 4 lines

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

@@ -163,6 +164,9 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
  unlock:
         spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 
+       if (ctrlr->dirty && !psci_has_osi_support())

Can we introduce a stub function for psci_has_osi_support() when CONFIG_ARM_PSCI_FW=n? This driver currently has:

config QCOM_RPMH
bool "Qualcomm RPM-Hardened (RPMH) Communication"
depends on ARCH_QCOM && ARM64 || COMPILE_TEST

which implies that this will break build testing once built on something that isn't arm64.

Quoted 7 lines

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

+               return rpmh_flush(ctrlr) ? ERR_PTR(-EINVAL) : req;
+
         return req;
  }
 

@@ -391,6 +395,8 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 

drivers/soc/qcom/rpmh.c int rpmh_write_batch(const struct device *dev, enum rpmh_state state, · line 399

Quoted 4 lines

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

         if (state != RPMH_ACTIVE_ONLY_STATE) {
                 cache_batch(ctrlr, req);
+               if (!psci_has_osi_support())
+                       return rpmh_flush(ctrlr);

While the diff is small it is also sad that we turn around after adding it to a list and immediately take it off the list and send it. Can't we do this without having to do the list add/remove dance?

Quoted 4 lines

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

                 return 0;
         }