| Message ID | 1626755807-11865-1-git-send-email-sibis@codeaurora.org |
|---|---|
| Headers | show |
| Series | Use qmp_send to update co-processor load state | expand |
On Tue, Jul 20, 2021 at 10:06:45AM +0530, Sibi Sankar wrote: > Strip out the load state power-domain support from the driver since the > low power mode signalling for the co-processors is now accessible through > the direct qmp message send interface. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Quoting Sibi Sankar (2021-07-19 21:36:38) > diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c > index 7e9244c748da..997ff21271f7 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] = {}; Just to confirm, we want to set the whole buffer to zero before writing it? Sounds good to not send stack junk over to to the other side but maybe we could skip initializing to zero for the first part of the buffer that isn't used? > + 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"); Should we WARN_ON() if the message doesn't fit into the 64-bytes? > + > + 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 > @@ -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; > @@ -286,9 +314,36 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev, > return PTR_ERR(q6v5->state); > } > > + q6v5->load_state = kstrdup_const(load_state, GFP_KERNEL); > + 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"); Use dev_err_probe()? > + kfree_const(q6v5->load_state); > + return PTR_ERR(q6v5->qmp); > + } > + } else { > + if (!q6v5->load_state) { Use else if and deindent? > + dev_err(&pdev->dev, "load state resource string empty\n"); > + return -EINVAL; > + } > + } > + > return 0; > } > EXPORT_SYMBOL_GPL(qcom_q6v5_init); >
Quoting Sibi Sankar (2021-07-19 21:36:45) > Strip out the load state power-domain support from the driver since the > low power mode signalling for the co-processors is now accessible through > the direct qmp message send interface. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Hey Matthias, Thanks for taking time to review the series. On 2021-07-21 04:40, Matthias Kaehlcke wrote: > On Tue, Jul 20, 2021 at 10:06:36AM +0530, Sibi Sankar wrote: >> The load state power-domain, used by the co-processors to notify the >> Always on Subsystem (AOSS) that a particular co-processor is up/down, >> suffers from the side-effect of changing states during suspend/resume. >> However the co-processors enter low-power modes independent to that of >> the application processor and their states are expected to remain >> unaltered across system suspend/resume cycles. To achieve this >> behavior >> let's drop the load state power-domain and replace them with the qmp >> property for all SoCs supporting low power mode signalling. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> >> v4: >> * Commit message change and sc8180x co-processor addition. >> [Rob/Bjorn] >> >> .../devicetree/bindings/remoteproc/qcom,adsp.yaml | 65 >> +++++++++++----------- >> 1 file changed, 33 insertions(+), 32 deletions(-) >> >> diff --git >> a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml >> b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml >> index c597ccced623..1182afb5f593 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.yaml >> @@ -78,11 +78,11 @@ properties: >> >> power-domains: >> minItems: 1 >> - maxItems: 3 >> + maxItems: 2 >> >> power-domain-names: >> minItems: 1 >> - maxItems: 3 >> + maxItems: 2 > > It seems maxItems should have been 4 in the first place and should > remain > unchanged after removing the load state power domain. With this patch: sc7180-mpss-pas actually uses only cx and mss. The mpss-pas compatible is overridden by the mss-pil compatible for all the platforms present upstream for sc7180, that's the reason we probably haven't run into any binding check failures. I'll keep the max-items to 2 and fix-up the sc7180 power-domain requirements instead. > > - if: > properties: > compatible: > contains: > enum: > - qcom,sc7180-mpss-pas > then: > properties: > power-domains: > items: > - description: CX power domain > - description: MX power domain > - description: MSS power domain > power-domain-names: > items: > - const: cx > - const: mx > - const: mss -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2021-07-21 10:56, Stephen Boyd wrote: > Quoting Sibi Sankar (2021-07-19 21:36:38) >> diff --git a/drivers/remoteproc/qcom_q6v5.c >> b/drivers/remoteproc/qcom_q6v5.c >> index 7e9244c748da..997ff21271f7 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] = {}; > > Just to confirm, we want to set the whole buffer to zero before writing > it? Sounds good to not send stack junk over to to the other side but > maybe we could skip initializing to zero for the first part of the > buffer that isn't used? Sure, it makes sense to incorporate a warn_on and memset the remainder of the buffer after populating it. > >> + 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"); > > Should we WARN_ON() if the message doesn't fit into the 64-bytes? > >> + >> + 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 >> @@ -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; >> @@ -286,9 +314,36 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct >> platform_device *pdev, >> return PTR_ERR(q6v5->state); >> } >> >> + q6v5->load_state = kstrdup_const(load_state, GFP_KERNEL); >> + 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"); > > Use dev_err_probe()? sure I'll use it. > >> + kfree_const(q6v5->load_state); >> + return PTR_ERR(q6v5->qmp); >> + } >> + } else { >> + if (!q6v5->load_state) { > > Use else if and deindent? lol, my bad. > >> + dev_err(&pdev->dev, "load state resource >> string empty\n"); >> + return -EINVAL; >> + } >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL_GPL(qcom_q6v5_init); >> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.