[PATCH 0/3] Add changes to support 'HW solver' mode in rpmh driver

[PATCH 0/3] Add changes to support 'HW solver' mode in rpmh driver

[Maulik Shah]

RSC controllers may be in 'HW solver' state, where they could be in
autonomous mode executing low power modes for their hardware and as
such are not available for sending active votes.

This series adds changes to support the same in rpmh driver by disallowing
active requests when in solver mode and allowing the drivers to write
cached sleep and wake votes immediately.

Note: The series can land on its own but to avoid conflicts with
other change going in rpmh driver i have based changes on top of [1]
which hopefully can land soon.

[1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1606379490-4052-1-git-send-email-mkshah@codeaurora.org

Lina Iyer (1):
  drivers: qcom: rpmh: Disallow active requests in solver mode

Maulik Shah (2):
  soc: qcom: rpmh: Add rpmh_write_sleep_and_wake() function
  soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()

 drivers/soc/qcom/rpmh-internal.h | 10 ++++
 drivers/soc/qcom/rpmh-rsc.c      | 34 ++++++++++++++
 drivers/soc/qcom/rpmh.c          | 98 ++++++++++++++++++++++++++++++++++++++--
 drivers/soc/qcom/trace-rpmh.h    | 20 ++++++++
 include/soc/qcom/rpmh.h          | 10 ++++
 5 files changed, 168 insertions(+), 4 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/3] drivers: qcom: rpmh: Disallow active requests in solver mode

[Maulik Shah]

From: Lina Iyer <ilina@codeaurora.org>

Controllers may be in 'solver' state, where they could be in autonomous
mode executing low power modes for their hardware and as such are not
available for sending active votes. Device driver may notify RPMH
that the controller is in solver mode and when in such mode, disallow
requests from platform drivers for state change using the RSC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh-internal.h |  5 ++++
 drivers/soc/qcom/rpmh-rsc.c      | 31 ++++++++++++++++++++++
 drivers/soc/qcom/rpmh.c          | 56 ++++++++++++++++++++++++++++++++++++++++
 drivers/soc/qcom/trace-rpmh.h    | 20 ++++++++++++++
 include/soc/qcom/rpmh.h          |  5 ++++
 5 files changed, 117 insertions(+)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 344ba68..79486d6 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -77,12 +77,14 @@ struct rpmh_request {
  * @cache: the list of cached requests
  * @cache_lock: synchronize access to the cache data
  * @dirty: was the cache updated since flush
+ * @in_solver_mode: Controller is busy in solver mode
  * @batch_cache: Cache sleep and wake requests sent as batch
  */
 struct rpmh_ctrlr {
 	struct list_head cache;
 	spinlock_t cache_lock;
 	bool dirty;
+	bool in_solver_mode;
 	struct list_head batch_cache;
 };
 
@@ -94,6 +96,7 @@ struct rpmh_ctrlr {
  * @tcs_base:           Start address of the TCS registers in this controller.
  * @id:                 Instance id in the controller (Direct Resource Voter).
  * @num_tcs:            Number of TCSes in this DRV.
+ * @in_solver_mode:     Controller is busy in solver mode
  * @rsc_pm:             CPU PM notifier for controller.
  *                      Used when solver mode is not present.
  * @cpus_in_pm:         Number of CPUs not in idle power collapse.
@@ -116,6 +119,7 @@ struct rsc_drv {
 	void __iomem *tcs_base;
 	int id;
 	int num_tcs;
+	bool in_solver_mode;
 	struct notifier_block rsc_pm;
 	atomic_t cpus_in_pm;
 	struct tcs_group tcs[TCS_TYPE_NR];
@@ -129,6 +133,7 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
 int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
 			     const struct tcs_request *msg);
 void rpmh_rsc_invalidate(struct rsc_drv *drv);
+int rpmh_rsc_mode_solver_set(struct rsc_drv *drv, bool enable);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);
 int rpmh_flush(struct rpmh_ctrlr *ctrlr);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 37969dc..ffb4ca7 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -638,6 +638,12 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 
 	spin_lock_irqsave(&drv->lock, flags);
 
+	if (drv->in_solver_mode) {
+		/* Controller is busy in 'solver' mode */
+		spin_unlock_irqrestore(&drv->lock, flags);
+		return -EBUSY;
+	}
+
 	/* Wait forever for a free tcs. It better be there eventually! */
 	wait_event_lock_irq(drv->tcs_wait,
 			    (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
@@ -859,6 +865,31 @@ static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
 	return ret;
 }
 
+/**
+ * rpmh_rsc_mode_solver_set() - Enable/disable solver mode.
+ * @drv:     The controller.
+ * @enable:  Boolean state to be set - true/false
+ *
+ * Return:
+ * * 0			- success
+ * * -EBUSY		- AMCs are busy
+ */
+int rpmh_rsc_mode_solver_set(struct rsc_drv *drv, bool enable)
+{
+	int ret = -EBUSY;
+
+	if (spin_trylock(&drv->lock)) {
+		if (!enable || !rpmh_rsc_ctrlr_is_busy(drv)) {
+			drv->in_solver_mode = enable;
+			trace_rpmh_solver_set(drv, enable);
+			ret = 0;
+		}
+		spin_unlock(&drv->lock);
+	}
+
+	return ret;
+}
+
 static int rpmh_probe_tcs_config(struct platform_device *pdev,
 				 struct rsc_drv *drv, void __iomem *base)
 {
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 01765ee..cbe6b96 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -76,6 +76,22 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
 	return &drv->client;
 }
 
+static int check_ctrlr_state(struct rpmh_ctrlr *ctrlr, enum rpmh_state state)
+{
+	int ret = 0;
+
+	if (state != RPMH_ACTIVE_ONLY_STATE)
+		return ret;
+
+	/* Do not allow sending active votes when in solver mode */
+	spin_lock(&ctrlr->cache_lock);
+	if (ctrlr->in_solver_mode)
+		ret = -EBUSY;
+	spin_unlock(&ctrlr->cache_lock);
+
+	return ret;
+}
+
 void rpmh_tx_done(const struct tcs_request *msg, int r)
 {
 	struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
@@ -229,9 +245,14 @@ static int __fill_rpmh_msg(struct rpmh_request *req, enum rpmh_state state,
 int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 		     const struct tcs_cmd *cmd, u32 n)
 {
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
 	struct rpmh_request *rpm_msg;
 	int ret;
 
+	ret = check_ctrlr_state(ctrlr, state);
+	if (ret)
+		return ret;
+
 	rpm_msg = kzalloc(sizeof(*rpm_msg), GFP_ATOMIC);
 	if (!rpm_msg)
 		return -ENOMEM;
@@ -262,8 +283,13 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
 {
 	DECLARE_COMPLETION_ONSTACK(compl);
 	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
 	int ret;
 
+	ret = check_ctrlr_state(ctrlr, state);
+	if (ret)
+		return ret;
+
 	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
 	if (ret)
 		return ret;
@@ -338,6 +364,10 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 	int ret, i;
 	void *ptr;
 
+	ret = check_ctrlr_state(ctrlr, state);
+	if (ret)
+		return ret;
+
 	if (!cmd || !n)
 		return -EINVAL;
 
@@ -505,3 +535,29 @@ void rpmh_invalidate(const struct device *dev)
 	spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
 }
 EXPORT_SYMBOL(rpmh_invalidate);
+
+/**
+ * rpmh_mode_solver_set() - Indicate that the RSC controller hardware has
+ * been configured to be in solver mode
+ *
+ * @dev: The device making the request
+ * @enable: Boolean value indicating if the controller is in solver mode.
+ *
+ * Return:
+ * * 0          - Success
+ * * Error code - Otherwise
+ */
+int rpmh_mode_solver_set(const struct device *dev, bool enable)
+{
+	int ret;
+	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
+
+	spin_lock(&ctrlr->cache_lock);
+	ret = rpmh_rsc_mode_solver_set(ctrlr_to_drv(ctrlr), enable);
+	if (!ret)
+		ctrlr->in_solver_mode = enable;
+	spin_unlock(&ctrlr->cache_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(rpmh_mode_solver_set);
diff --git a/drivers/soc/qcom/trace-rpmh.h b/drivers/soc/qcom/trace-rpmh.h
index feb0cb4..b2b934c 100644
--- a/drivers/soc/qcom/trace-rpmh.h
+++ b/drivers/soc/qcom/trace-rpmh.h
@@ -71,6 +71,26 @@ TRACE_EVENT(rpmh_send_msg,
 		  __entry->addr, __entry->data, __entry->wait)
 );
 
+TRACE_EVENT(rpmh_solver_set,
+
+	TP_PROTO(struct rsc_drv *d, bool set),
+
+	TP_ARGS(d, set),
+
+	TP_STRUCT__entry(
+			 __string(name, d->name)
+			 __field(bool, set)
+	),
+
+	TP_fast_assign(
+		       __assign_str(name, d->name);
+		       __entry->set = set;
+	),
+
+	TP_printk("%s: solver mode set: %d",
+		  __get_str(name), __entry->set)
+);
+
 #endif /* _TRACE_RPMH_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index bdbee1a..fa8bb53 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -20,6 +20,8 @@ int rpmh_write_async(const struct device *dev, enum rpmh_state state,
 int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 		     const struct tcs_cmd *cmd, u32 *n);
 
+int rpmh_mode_solver_set(const struct device *dev, bool enable);
+
 void rpmh_invalidate(const struct device *dev);
 
 #else
@@ -38,6 +40,9 @@ static inline int rpmh_write_batch(const struct device *dev,
 				   const struct tcs_cmd *cmd, u32 *n)
 { return -ENODEV; }
 
+static int rpmh_mode_solver_set(const struct device *dev, bool enable)
+{ return -ENODEV; }
+
 static inline void rpmh_invalidate(const struct device *dev)
 {
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/3] soc: qcom: rpmh: Add rpmh_write_sleep_and_wake() function

[Maulik Shah]

Let RPMH clients call rpmh_write_sleep_and_wake() to immediately
write cached sleep and wake data to the TCSes.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh.c | 16 ++++++++++++++++
 include/soc/qcom/rpmh.h |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index cbe6b96..725b8f0 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -515,6 +515,22 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 }
 
 /**
+ * rpmh_write_sleep_and_wake() - Writes the buffered wake and sleep sets
+ * to TCSes
+ *
+ * @dev: The device making the request
+ *
+ * Return:
+ * * 0          - Success
+ * * Error code - Otherwise
+ */
+int rpmh_write_sleep_and_wake(const struct device *dev)
+{
+	return rpmh_flush(get_rpmh_ctrlr(dev));
+}
+EXPORT_SYMBOL(rpmh_write_sleep_and_wake);
+
+/**
  * rpmh_invalidate: Invalidate sleep and wake sets in batch_cache
  *
  * @dev: The device making the request
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index fa8bb53..59b68f8 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -22,6 +22,8 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
 
 int rpmh_mode_solver_set(const struct device *dev, bool enable);
 
+int rpmh_write_sleep_and_wake(const struct device *dev);
+
 void rpmh_invalidate(const struct device *dev);
 
 #else
@@ -43,6 +45,9 @@ static inline int rpmh_write_batch(const struct device *dev,
 static int rpmh_mode_solver_set(const struct device *dev, bool enable)
 { return -ENODEV; }
 
+static int rpmh_write_sleep_and_wake(const struct device *dev)
+{ return -ENODEV; }
+
 static inline void rpmh_invalidate(const struct device *dev)
 {
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()

[Maulik Shah]

lockdep_assert_irqs_disabled() was added to check rpmh_flush()
can only be invoked when irqs are disabled, this is true for
APPS RSC as the last CPU going to deepest low power mode is
writing sleep and wake TCSes.

However for RSCs that support solver mode, drivers can invoke
rpmh_write_sleep_and_wake() to immediately write cached sleep
and wake sets to TCSes from any CPU. Conditionally check if RSC
controller supports 'HW solver' mode then do not check for irqs
disabled as such RSCs can write sleepand wake TCSes at any point.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/rpmh-internal.h |  5 +++++
 drivers/soc/qcom/rpmh-rsc.c      |  3 +++
 drivers/soc/qcom/rpmh.c          | 26 ++++++++++++++++++++++----
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 79486d6..39fa3c5 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -17,6 +17,9 @@
 #define MAX_TCS_NR			(MAX_TCS_PER_TYPE * TCS_TYPE_NR)
 #define MAX_TCS_SLOTS			(MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
 
+/* CTRLR specific flags */
+#define SOLVER_PRESENT			1
+
 struct rsc_drv;
 
 /**
@@ -78,6 +81,7 @@ struct rpmh_request {
  * @cache_lock: synchronize access to the cache data
  * @dirty: was the cache updated since flush
  * @in_solver_mode: Controller is busy in solver mode
+ * @flags: Controller specific flags
  * @batch_cache: Cache sleep and wake requests sent as batch
  */
 struct rpmh_ctrlr {
@@ -85,6 +89,7 @@ struct rpmh_ctrlr {
 	spinlock_t cache_lock;
 	bool dirty;
 	bool in_solver_mode;
+	u32 flags;
 	struct list_head batch_cache;
 };
 
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index ffb4ca7..4caaddf 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -1031,6 +1031,9 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
 	if (!solver_config) {
 		drv->rsc_pm.notifier_call = rpmh_rsc_cpu_pm_callback;
 		cpu_pm_register_notifier(&drv->rsc_pm);
+		drv->client.flags &= ~SOLVER_PRESENT;
+	} else {
+		drv->client.flags |= SOLVER_PRESENT;
 	}
 
 	/* Enable the active TCS to send requests immediately */
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 725b8f0..604d511 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -83,6 +83,9 @@ static int check_ctrlr_state(struct rpmh_ctrlr *ctrlr, enum rpmh_state state)
 	if (state != RPMH_ACTIVE_ONLY_STATE)
 		return ret;
 
+	if (!(ctrlr->flags & SOLVER_PRESENT))
+		return ret;
+
 	/* Do not allow sending active votes when in solver mode */
 	spin_lock(&ctrlr->cache_lock);
 	if (ctrlr->in_solver_mode)
@@ -468,12 +471,24 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
 	struct cache_req *p;
 	int ret = 0;
 
-	lockdep_assert_irqs_disabled();
+	/*
+	 * For RSC that don't have solver mode,
+	 * rpmh_flush() is only called when we think we're running
+	 * on the last CPU with irqs_disabled.
+	 *
+	 * For RSC that have solver mode,
+	 * rpmh_flush() can be invoked with irqs enabled by any CPU.
+	 *
+	 * Conditionally check for irqs_disabled only when solver mode
+	 * is not available.
+	 */
+
+	if (!(ctrlr->flags & SOLVER_PRESENT))
+		lockdep_assert_irqs_disabled();
 
 	/*
-	 * Currently rpmh_flush() is only called when we think we're running
-	 * on the last processor.  If the lock is busy it means another
-	 * processor is up and it's better to abort than spin.
+	 * If the lock is busy it means another transaction is on going,
+	 * in such case it's better to abort than spin.
 	 */
 	if (!spin_trylock(&ctrlr->cache_lock))
 		return -EBUSY;
@@ -568,6 +583,9 @@ int rpmh_mode_solver_set(const struct device *dev, bool enable)
 	int ret;
 	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
 
+	if (!(ctrlr->flags & SOLVER_PRESENT))
+		return -EINVAL;
+
 	spin_lock(&ctrlr->cache_lock);
 	ret = rpmh_rsc_mode_solver_set(ctrlr_to_drv(ctrlr), enable);
 	if (!ret)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()

[Stephen Boyd]

Quoting Maulik Shah (2020-11-26 02:18:18)
> lockdep_assert_irqs_disabled() was added to check rpmh_flush()
> can only be invoked when irqs are disabled, this is true for
> APPS RSC as the last CPU going to deepest low power mode is
> writing sleep and wake TCSes.
> 
> However for RSCs that support solver mode, drivers can invoke
> rpmh_write_sleep_and_wake() to immediately write cached sleep
> and wake sets to TCSes from any CPU. Conditionally check if RSC
> controller supports 'HW solver' mode then do not check for irqs
> disabled as such RSCs can write sleepand wake TCSes at any point.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/soc/qcom/rpmh-internal.h |  5 +++++
>  drivers/soc/qcom/rpmh-rsc.c      |  3 +++
>  drivers/soc/qcom/rpmh.c          | 26 ++++++++++++++++++++++----
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index 79486d6..39fa3c5 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -17,6 +17,9 @@
>  #define MAX_TCS_NR                     (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
>  #define MAX_TCS_SLOTS                  (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
>  
> +/* CTRLR specific flags */
> +#define SOLVER_PRESENT                 1
> +
>  struct rsc_drv;
>  
>  /**
> @@ -78,6 +81,7 @@ struct rpmh_request {
>   * @cache_lock: synchronize access to the cache data
>   * @dirty: was the cache updated since flush
>   * @in_solver_mode: Controller is busy in solver mode
> + * @flags: Controller specific flags
>   * @batch_cache: Cache sleep and wake requests sent as batch
>   */
>  struct rpmh_ctrlr {
> @@ -85,6 +89,7 @@ struct rpmh_ctrlr {
>         spinlock_t cache_lock;
>         bool dirty;
>         bool in_solver_mode;
> +       u32 flags;

Maybe unsigned long is more appropriate? Do we rely on 32-bits vs.
64-bits?

>         struct list_head batch_cache;
>  };
>  
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index ffb4ca7..4caaddf 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -1031,6 +1031,9 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
>         if (!solver_config) {
>                 drv->rsc_pm.notifier_call = rpmh_rsc_cpu_pm_callback;
>                 cpu_pm_register_notifier(&drv->rsc_pm);
> +               drv->client.flags &= ~SOLVER_PRESENT;
> +       } else {
> +               drv->client.flags |= SOLVER_PRESENT;

It looks like this could be tested by checking for
drv->rsc_pm.notifier_call being non-NULL?

>         }
>  
>         /* Enable the active TCS to send requests immediately */
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index 725b8f0..604d511 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -83,6 +83,9 @@ static int check_ctrlr_state(struct rpmh_ctrlr *ctrlr, enum rpmh_state state)
>         if (state != RPMH_ACTIVE_ONLY_STATE)
>                 return ret;
>  
> +       if (!(ctrlr->flags & SOLVER_PRESENT))
> +               return ret;
> +
>         /* Do not allow sending active votes when in solver mode */
>         spin_lock(&ctrlr->cache_lock);
>         if (ctrlr->in_solver_mode)
> @@ -468,12 +471,24 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
>         struct cache_req *p;
>         int ret = 0;
>  
> -       lockdep_assert_irqs_disabled();
> +       /*
> +        * For RSC that don't have solver mode,
> +        * rpmh_flush() is only called when we think we're running
> +        * on the last CPU with irqs_disabled.
> +        *
> +        * For RSC that have solver mode,
> +        * rpmh_flush() can be invoked with irqs enabled by any CPU.
> +        *
> +        * Conditionally check for irqs_disabled only when solver mode
> +        * is not available.
> +        */
> +
> +       if (!(ctrlr->flags & SOLVER_PRESENT))
> +               lockdep_assert_irqs_disabled();

Can we have a different function that is called for the case where
solver mode is present and where solver mode isn't present? It would be
good to clearly show that rpmh_flush() thinks it is being called from
the last CPU vs. from some other random place because the code is
assuming solver vs. non-solver enabled state. It would be clearer from
the call site too.

>  
>         /*
> -        * Currently rpmh_flush() is only called when we think we're running
> -        * on the last processor.  If the lock is busy it means another
> -        * processor is up and it's better to abort than spin.
> +        * If the lock is busy it means another transaction is on going,
> +        * in such case it's better to abort than spin.
>          */
>         if (!spin_trylock(&ctrlr->cache_lock))
>                 return -EBUSY;

Re: [PATCH 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()

[Maulik Shah]

Hi Stephen,

On 12/3/2020 12:31 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-11-26 02:18:18)
>> lockdep_assert_irqs_disabled() was added to check rpmh_flush()
>> can only be invoked when irqs are disabled, this is true for
>> APPS RSC as the last CPU going to deepest low power mode is
>> writing sleep and wake TCSes.
>>
>> However for RSCs that support solver mode, drivers can invoke
>> rpmh_write_sleep_and_wake() to immediately write cached sleep
>> and wake sets to TCSes from any CPU. Conditionally check if RSC
>> controller supports 'HW solver' mode then do not check for irqs
>> disabled as such RSCs can write sleepand wake TCSes at any point.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/soc/qcom/rpmh-internal.h |  5 +++++
>>   drivers/soc/qcom/rpmh-rsc.c      |  3 +++
>>   drivers/soc/qcom/rpmh.c          | 26 ++++++++++++++++++++++----
>>   3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>> index 79486d6..39fa3c5 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> @@ -17,6 +17,9 @@
>>   #define MAX_TCS_NR                     (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
>>   #define MAX_TCS_SLOTS                  (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
>>   
>> +/* CTRLR specific flags */
>> +#define SOLVER_PRESENT                 1
>> +
>>   struct rsc_drv;
>>   
>>   /**
>> @@ -78,6 +81,7 @@ struct rpmh_request {
>>    * @cache_lock: synchronize access to the cache data
>>    * @dirty: was the cache updated since flush
>>    * @in_solver_mode: Controller is busy in solver mode
>> + * @flags: Controller specific flags
>>    * @batch_cache: Cache sleep and wake requests sent as batch
>>    */
>>   struct rpmh_ctrlr {
>> @@ -85,6 +89,7 @@ struct rpmh_ctrlr {
>>          spinlock_t cache_lock;
>>          bool dirty;
>>          bool in_solver_mode;
>> +       u32 flags;
> Maybe unsigned long is more appropriate? Do we rely on 32-bits vs.
> 64-bits?
We don't rely on 32-bits vs. 64-bits, u32 should be fine.
>
>>          struct list_head batch_cache;
>>   };
>>   
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index ffb4ca7..4caaddf 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -1031,6 +1031,9 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
>>          if (!solver_config) {
>>                  drv->rsc_pm.notifier_call = rpmh_rsc_cpu_pm_callback;
>>                  cpu_pm_register_notifier(&drv->rsc_pm);
>> +               drv->client.flags &= ~SOLVER_PRESENT;
>> +       } else {
>> +               drv->client.flags |= SOLVER_PRESENT;
> It looks like this could be tested by checking for
> drv->rsc_pm.notifier_call being non-NULL?
It may for now, but going forward we may have different flags to 
indicate various functions supported by RSC.
>
>>          }
>>   
>>          /* Enable the active TCS to send requests immediately */
>> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
>> index 725b8f0..604d511 100644
>> --- a/drivers/soc/qcom/rpmh.c
>> +++ b/drivers/soc/qcom/rpmh.c
>> @@ -83,6 +83,9 @@ static int check_ctrlr_state(struct rpmh_ctrlr *ctrlr, enum rpmh_state state)
>>          if (state != RPMH_ACTIVE_ONLY_STATE)
>>                  return ret;
>>   
>> +       if (!(ctrlr->flags & SOLVER_PRESENT))
>> +               return ret;
>> +
>>          /* Do not allow sending active votes when in solver mode */
>>          spin_lock(&ctrlr->cache_lock);
>>          if (ctrlr->in_solver_mode)
>> @@ -468,12 +471,24 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
>>          struct cache_req *p;
>>          int ret = 0;
>>   
>> -       lockdep_assert_irqs_disabled();
>> +       /*
>> +        * For RSC that don't have solver mode,
>> +        * rpmh_flush() is only called when we think we're running
>> +        * on the last CPU with irqs_disabled.
>> +        *
>> +        * For RSC that have solver mode,
>> +        * rpmh_flush() can be invoked with irqs enabled by any CPU.
>> +        *
>> +        * Conditionally check for irqs_disabled only when solver mode
>> +        * is not available.
>> +        */
>> +
>> +       if (!(ctrlr->flags & SOLVER_PRESENT))
>> +               lockdep_assert_irqs_disabled();
> Can we have a different function that is called for the case where
> solver mode is present and where solver mode isn't present? It would be
> good to clearly show that rpmh_flush() thinks it is being called from
> the last CPU vs. from some other random place because the code is
> assuming solver vs. non-solver enabled state. It would be clearer from
> the call site too.
Hmm, we can. Patch 2 of this series already added different function 
which will be called where solver is present.
Let me modify this to indicate whether called from last cpu or not.

Thanks,
Maulik

>
>>   
>>          /*
>> -        * Currently rpmh_flush() is only called when we think we're running
>> -        * on the last processor.  If the lock is busy it means another
>> -        * processor is up and it's better to abort than spin.
>> +        * If the lock is busy it means another transaction is on going,
>> +        * in such case it's better to abort than spin.
>>           */
>>          if (!spin_trylock(&ctrlr->cache_lock))
>>                  return -EBUSY;

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation