Re: [PATCH v4 2/2] remoteproc: qcom: Add notification types to SSR

From: Alex Elder
Date: Thu Jun 18 2020 - 19:00:42 EST


On 5/27/20 10:34 PM, Rishabh Bhatnagar wrote:
From: Siddharth Gupta <sidgup@xxxxxxxxxxxxxx>

The SSR subdevice only adds callback for the unprepare event. Add callbacks
for unprepare, start and prepare events. The client driver for a particular

for prepare, start, and stop events

remoteproc might be interested in knowing the status of the remoteproc
while undergoing SSR, not just when the remoteproc has finished shutting
down.

Signed-off-by: Siddharth Gupta <sidgup@xxxxxxxxxxxxxx>
Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
---
drivers/remoteproc/qcom_common.c | 46 +++++++++++++++++++++++++++++++++--
include/linux/remoteproc/qcom_rproc.h | 14 +++++++++++
2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 61ff2dd..5c5a1eb 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -228,7 +228,7 @@ struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)
*
* This registers the @nb notifier block as part the notifier chain for a
* remoteproc associated with @name. The notifier block's callback
- * will be invoked when the particular remote processor is stopped.
+ * will be invoked when the particular remote processor is started/stopped.

Maybe something more like:
will be invoked when an SSR event occurs for the named
remote processor.

*/
void *qcom_register_ssr_notifier(const char *name, struct notifier_block *nb)
{
@@ -258,6 +258,44 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
}
EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
+static int ssr_notify_prepare(struct rproc_subdev *subdev)
+{
+ struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+ struct qcom_ssr_notif_data data = {
+ .name = ssr->info->name,
+ .crashed = false,
+ };
+
+ srcu_notifier_call_chain(&ssr->info->notifier_list,
+ QCOM_SSR_BEFORE_POWERUP, &data);
+ return 0;
+}
+
+static int ssr_notify_start(struct rproc_subdev *subdev)
+{
+ struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+ struct qcom_ssr_notif_data data = {
+ .name = ssr->info->name,
+ .crashed = false,
+ };
+
+ srcu_notifier_call_chain(&ssr->info->notifier_list,
+ QCOM_SSR_AFTER_POWERUP, &data);
+ return 0;
+}
+
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
+{
+ struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+ struct qcom_ssr_notif_data data = {
+ .name = ssr->info->name,
+ .crashed = crashed,
+ };
+
+ srcu_notifier_call_chain(&ssr->info->notifier_list,
+ QCOM_SSR_BEFORE_SHUTDOWN, &data);
+}
+
static void ssr_notify_unprepare(struct rproc_subdev *subdev)
{
struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
@@ -266,7 +304,8 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)
.crashed = false,
};
- srcu_notifier_call_chain(&ssr->info->notifier_list, 0, &data);
+ srcu_notifier_call_chain(&ssr->info->notifier_list,
+ QCOM_SSR_AFTER_SHUTDOWN, &data);
}
@@ -294,6 +333,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
mutex_unlock(&qcom_ssr_subsys_lock);
ssr->info = info;
+ ssr->subdev.prepare = ssr_notify_prepare;
+ ssr->subdev.start = ssr_notify_start;
+ ssr->subdev.stop = ssr_notify_stop;
ssr->subdev.unprepare = ssr_notify_unprepare;
rproc_add_subdev(rproc, &ssr->subdev);
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index 58422b1..a558183 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -5,6 +5,20 @@
#if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
+/**
+ * enum qcom_ssr_notif_type - Different stages of remoteproc notifications
+ * @QCOM_SSR_BEFORE_SHUTDOWN: unprepare stage of remoteproc
+ * @QCOM_SSR_AFTER_SHUTDOWN: stop stage of remoteproc
+ * @QCOM_SSR_BEFORE_POWERUP: prepare stage of remoteproc
+ * @QCOM_SSR_AFTER_POWERUP: start stage of remoteproc

I think your explanations of these symbols are less clear than
the symbol names themselves... In any case, I wouldn't refer
to these as "stages of notifications" but instead something
more like startup/shutdown related events for a remote processor.

I personally might have ordered them differently too:
So maybe more like:
BEFORE_POWERUP Remoteproc about to start (prepare)
AFTER_POWERUP Remoteproc is running (start)
BEFORE_SHUTDOWN Remoteproc crashed, or shutting down (stop)
AFTER_SHUTDOWN Remoteproc is down (unprepare)

-Alex

+ */
+enum qcom_ssr_notif_type {
+ QCOM_SSR_BEFORE_SHUTDOWN,
+ QCOM_SSR_AFTER_SHUTDOWN,
+ QCOM_SSR_BEFORE_POWERUP,
+ QCOM_SSR_AFTER_POWERUP,
+};
+
struct qcom_ssr_notif_data {
const char *name;
bool crashed;