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.
next prev parent 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).