All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alex Elder <elder@ieee.org>
To: Rishabh Bhatnagar <rishabhb@codeaurora.org>,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: bjorn.andersson@linaro.org, tsoni@codeaurora.org,
	psodagud@codeaurora.org, sidgup@codeaurora.org
Subject: Re: [PATCH v4 2/2] remoteproc: qcom: Add notification types to SSR
Date: Thu, 18 Jun 2020 18:00:34 -0500	[thread overview]
Message-ID: <c7c2d4ac-3d09-d28b-0d21-9ac6e9e10172@ieee.org> (raw)
In-Reply-To: <1590636883-30866-3-git-send-email-rishabhb@codeaurora.org>

On 5/27/20 10:34 PM, Rishabh Bhatnagar wrote:
> From: Siddharth Gupta <sidgup@codeaurora.org>
> 
> 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@codeaurora.org>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> ---
>   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;
> 


      reply	other threads:[~2020-06-18 23:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28  3:34 [PATCH v4 0/2] Extend SSR notifications framework Rishabh Bhatnagar
2020-05-28  3:34 ` [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification Rishabh Bhatnagar
2020-05-30 10:30   ` kbuild test robot
2020-05-30 10:30     ` kbuild test robot
2020-06-18 23:00   ` Alex Elder
2020-06-18 23:35     ` Bjorn Andersson
2020-06-20 19:48       ` rishabhb
2020-06-22 18:51         ` Bjorn Andersson
2020-05-28  3:34 ` [PATCH v4 2/2] remoteproc: qcom: Add notification types to SSR Rishabh Bhatnagar
2020-06-18 23:00   ` Alex Elder [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c7c2d4ac-3d09-d28b-0d21-9ac6e9e10172@ieee.org \
    --to=elder@ieee.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=psodagud@codeaurora.org \
    --cc=rishabhb@codeaurora.org \
    --cc=sidgup@codeaurora.org \
    --cc=tsoni@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.