linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sibi Sankar <sibis@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: bjorn.andersson@linaro.org, robh+dt@kernel.org,
	swboyd@chromium.org, ulf.hansson@linaro.org, rjw@rjwysocki.net,
	agross@kernel.org, ohad@wizery.com, mathieu.poirier@linaro.org,
	linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	dianders@chromium.org, rishabhb@codeaurora.org,
	sidgup@codeaurora.org
Subject: Re: [PATCH v3 04/13] remoteproc: qcom: q6v5: Use qmp_send to update co-processor load state
Date: Thu, 01 Jul 2021 01:01:25 +0530	[thread overview]
Message-ID: <82de11c77b8b46dcdfec4a2d2569ac95@codeaurora.org> (raw)
In-Reply-To: <YNZGls1wHbWgsEO5@google.com>

Hey Matthias,
Thanks for taking time to review the series.

On 2021-06-26 02:41, Matthias Kaehlcke wrote:
> On Fri, Jun 25, 2021 at 12:21:58AM +0530, Sibi Sankar wrote:
>> The power domains exposed by the AOSS QMP driver control the load 
>> state
>> resources linked to modem, adsp, cdsp remoteprocs. These are used to
>> notify the Always on Subsystem (AOSS) that a particular co-processor 
>> is
>> up/down. AOSS uses this information to wait for the co-processors to
>> suspend before starting its sleep sequence.
>> 
>> These co-processors enter low-power modes independent to that of the
>> application processor and the load state resources linked to them are
>> expected to remain unaltered across system suspend/resume cycles. To
>> achieve this behavior lets stop using the power-domains exposed by the
>> AOSS QMP node and replace them with generic qmp_send interface 
>> instead.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5.c      | 56 +++++++++++++++++++++++++-
>>  drivers/remoteproc/qcom_q6v5.h      |  7 +++-
>>  drivers/remoteproc/qcom_q6v5_adsp.c |  7 +++-
>>  drivers/remoteproc/qcom_q6v5_mss.c  | 44 ++++----------------
>>  drivers/remoteproc/qcom_q6v5_pas.c  | 80 
>> +++++++++----------------------------
>>  drivers/remoteproc/qcom_q6v5_wcss.c |  4 +-
>>  6 files changed, 94 insertions(+), 104 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5.c 
>> b/drivers/remoteproc/qcom_q6v5.c
>> index 9627a950928e..4a9a481c211b 100644
>> --- a/drivers/remoteproc/qcom_q6v5.c
>> +++ b/drivers/remoteproc/qcom_q6v5.c
>> @@ -16,8 +16,28 @@
>>  #include "qcom_common.h"
>>  #include "qcom_q6v5.h"
>> 
>> +#define Q6V5_LOAD_STATE_MSG_LEN	64
>>  #define Q6V5_PANIC_DELAY_MS	200
>> 
>> +static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool 
>> enable)
>> +{
>> +	char buf[Q6V5_LOAD_STATE_MSG_LEN] = {};
>> +	int ret;
>> +
>> +	if (IS_ERR(q6v5->qmp))
>> +		return 0;
>> +
>> +	snprintf(buf, sizeof(buf),
>> +		 "{class: image, res: load_state, name: %s, val: %s}",
>> +		 q6v5->load_state, enable ? "on" : "off");
>> +
>> +	ret = qmp_send(q6v5->qmp, buf, sizeof(buf));
>> +	if (ret)
>> +		dev_err(q6v5->dev, "failed to toggle load state\n");
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before 
>> start
>>   * @q6v5:	reference to qcom_q6v5 context to be reinitialized
>> @@ -26,6 +46,12 @@
>>   */
>>  int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5)
>>  {
>> +	int ret;
>> +
>> +	ret = q6v5_load_state_toggle(q6v5, true);
>> +	if (ret)
>> +		return ret;
>> +
>>  	reinit_completion(&q6v5->start_done);
>>  	reinit_completion(&q6v5->stop_done);
>> 
>> @@ -47,6 +73,7 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_prepare);
>>  int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5)
>>  {
>>  	disable_irq(q6v5->handover_irq);
>> +	q6v5_load_state_toggle(q6v5, false);
>> 
>>  	return !q6v5->handover_issued;
>>  }
>> @@ -196,12 +223,13 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
>>   * @pdev:	platform_device reference for acquiring resources
>>   * @rproc:	associated remoteproc instance
>>   * @crash_reason: SMEM id for crash reason string, or 0 if none
>> + * @load_state: load state resource string
>>   * @handover:	function to be called when proxy resources should be 
>> released
>>   *
>>   * Return: 0 on success, negative errno on failure
>>   */
>>  int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device 
>> *pdev,
>> -		   struct rproc *rproc, int crash_reason,
>> +		   struct rproc *rproc, int crash_reason, const char *load_state,
>>  		   void (*handover)(struct qcom_q6v5 *q6v5))
>>  {
>>  	int ret;
>> @@ -210,6 +238,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct 
>> platform_device *pdev,
>>  	q6v5->dev = &pdev->dev;
>>  	q6v5->crash_reason = crash_reason;
>>  	q6v5->handover = handover;
>> +	q6v5->load_state = kstrdup_const(load_state, GFP_KERNEL);
>> 
>>  	init_completion(&q6v5->start_done);
>>  	init_completion(&q6v5->stop_done);
>> @@ -286,9 +315,34 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct 
>> platform_device *pdev,
>>  		return PTR_ERR(q6v5->state);
>>  	}
>> 
>> +	q6v5->qmp = qmp_get(&pdev->dev);
>> +	if (IS_ERR(q6v5->qmp)) {
>> +		if (PTR_ERR(q6v5->qmp) != -ENODEV) {
>> +			if (PTR_ERR(q6v5->qmp) != -EPROBE_DEFER)
>> +				dev_err(&pdev->dev, "failed to acquire load state\n");
>> +			return PTR_ERR(q6v5->qmp);
>> +		}
>> +	} else {
>> +		if (!q6v5->load_state) {
>> +			dev_err(&pdev->dev, "load state resource string empty\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_q6v5_init);
>> 
>> +/**
>> + * qcom_q6v5_deinit() - deinitialize the q6v5 common struct
>> + * @q6v5:	reference to qcom_q6v5 context to be deinitialized
>> + * @pdev:	platform_device reference for acquiring resources
>> + */
>> +void qcom_q6v5_deinit(struct qcom_q6v5 *q6v5, struct platform_device 
>> *pdev)
>> +{
> 
> pdev isn't used, remove it?

thanks for catching this. It was an
artefact of the recent qmp api changes.

> 
> 	kfree_const(q6v5->load_state);
> 
>> +	qmp_put(q6v5->qmp);
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_q6v5_deinit);
>> +
>>  MODULE_LICENSE("GPL v2");
>>  MODULE_DESCRIPTION("Qualcomm Peripheral Image Loader for Q6V5");
>> diff --git a/drivers/remoteproc/qcom_q6v5.h 
>> b/drivers/remoteproc/qcom_q6v5.h
>> index 1c212f670cbc..3d9f525cb4ec 100644
>> --- a/drivers/remoteproc/qcom_q6v5.h
>> +++ b/drivers/remoteproc/qcom_q6v5.h
>> @@ -5,6 +5,7 @@
>> 
>>  #include <linux/kernel.h>
>>  #include <linux/completion.h>
>> +#include <linux/soc/qcom/qcom_aoss.h>
>> 
>>  struct rproc;
>>  struct qcom_smem_state;
>> @@ -15,6 +16,8 @@ struct qcom_q6v5 {
>>  	struct rproc *rproc;
>> 
>>  	struct qcom_smem_state *state;
>> +	struct qmp *qmp;
>> +
>>  	unsigned stop_bit;
>> 
>>  	int wdog_irq;
>> @@ -32,12 +35,14 @@ struct qcom_q6v5 {
>> 
>>  	bool running;
>> 
>> +	const char *load_state;
>>  	void (*handover)(struct qcom_q6v5 *q6v5);
>>  };
>> 
>>  int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device 
>> *pdev,
>> -		   struct rproc *rproc, int crash_reason,
>> +		   struct rproc *rproc, int crash_reason, const char *load_state,
>>  		   void (*handover)(struct qcom_q6v5 *q6v5));
>> +void qcom_q6v5_deinit(struct qcom_q6v5 *q6v5, struct platform_device 
>> *pdev);
>> 
>>  int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
>>  int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
>> b/drivers/remoteproc/qcom_q6v5_adsp.c
>> index 8b0d8bbacd2e..0f5e0fd216b4 100644
>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
>> @@ -185,7 +185,9 @@ static int adsp_start(struct rproc *rproc)
>>  	int ret;
>>  	unsigned int val;
>> 
>> -	qcom_q6v5_prepare(&adsp->q6v5);
>> +	ret = qcom_q6v5_prepare(&adsp->q6v5);
>> +	if (ret)
>> +		return ret;
>> 
>>  	ret = clk_prepare_enable(adsp->xo);
>>  	if (ret)
>> @@ -465,7 +467,7 @@ static int adsp_probe(struct platform_device 
>> *pdev)
>>  	if (ret)
>>  		goto disable_pm;
>> 
>> -	ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, 
>> desc->crash_reason_smem,
>> +	ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, 
>> desc->crash_reason_smem, NULL,
>>  			     qcom_adsp_pil_handover);
> 
> Doesn't passing a load_state of NULL cause qcom_q6v5_init() to fail
> with -EINVAL?

qmp_get is expected to error out with
-ENODEV since we don't mention a qmp
node in dt. When a client needs to use
qmp in the future we would replace NULL
with the proper load_state string value.

> 
>> --- a/drivers/remoteproc/qcom_q6v5_wcss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c
>> @@ -1044,8 +1044,7 @@ static int q6v5_wcss_probe(struct 
>> platform_device *pdev)
>>  	if (ret)
>>  		goto free_rproc;
>> 
>> -	ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, 
>> desc->crash_reason_smem,
>> -			     NULL);
>> +	ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, 
>> desc->crash_reason_smem, NULL, NULL);
> 
> Same as for adsp_probe(), doesn't a load_state of NULL cause _init() to 
> fail?

it won't for the same reason explained
above.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

  reply	other threads:[~2021-06-30 19:31 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-24 18:51 [PATCH v3 00/13] Use qmp_send to update co-processor load state Sibi Sankar
2021-06-24 18:51 ` [PATCH v3 01/13] dt-bindings: soc: qcom: aoss: Drop power-domain bindings Sibi Sankar
2021-06-25 21:31   ` Matthias Kaehlcke
2021-06-24 18:51 ` [PATCH v3 02/13] dt-bindings: remoteproc: qcom: pas: Add QMP bindings Sibi Sankar
2021-06-25 21:19   ` Matthias Kaehlcke
2021-06-30 19:51     ` Sibi Sankar
2021-07-14 19:34   ` Rob Herring
2021-07-19 16:30     ` Sibi Sankar
2021-07-19 19:44   ` Bjorn Andersson
2021-06-24 18:51 ` [PATCH v3 03/13] dt-bindings: remoteproc: qcom: " Sibi Sankar
2021-06-24 18:51 ` [PATCH v3 04/13] remoteproc: qcom: q6v5: Use qmp_send to update co-processor load state Sibi Sankar
2021-06-25 21:11   ` Matthias Kaehlcke
2021-06-30 19:31     ` Sibi Sankar [this message]
2021-06-24 18:51 ` [PATCH v3 05/13] arm64: dts: qcom: sc7180: Use QMP binding to control " Sibi Sankar
2021-06-25 21:35   ` Matthias Kaehlcke
2021-06-30 19:52     ` Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 06/13] arm64: dts: qcom: sc7280: " Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 07/13] arm64: dts: qcom: sdm845: " Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 08/13] arm64: dts: qcom: sm8150: " Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 09/13] arm64: dts: qcom: sm8250: " Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 10/13] arm64: dts: qcom: sm8350: " Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 11/13] soc: qcom: aoss: Drop power domain support Sibi Sankar
2021-06-25 22:01   ` Matthias Kaehlcke
2021-06-30 19:56     ` Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 12/13] dt-bindings: msm/dp: Remove aoss-qmp header Sibi Sankar
2021-06-24 18:52 ` [PATCH v3 13/13] dt-bindings: soc: qcom: aoss: Delete unused power-domain definitions Sibi Sankar
2021-06-25 22:16   ` Matthias Kaehlcke

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=82de11c77b8b46dcdfec4a2d2569ac95@codeaurora.org \
    --to=sibis@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=mka@chromium.org \
    --cc=ohad@wizery.com \
    --cc=rishabhb@codeaurora.org \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=sidgup@codeaurora.org \
    --cc=swboyd@chromium.org \
    --cc=ulf.hansson@linaro.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).