From: Caleb Connolly <caleb.connolly@linaro.org>
To: Deepak Kumar Singh <quic_deesin@quicinc.com>,
quic_bjorande@quicinc.com, andersson@kernel.org,
quic_clew@quicinc.com, mathieu.poirier@linaro.org
Cc: linux-kernel@vger.kernel.org, quic_sarannya@quicinc.com,
linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org
Subject: Re: [PATCH V1] rpmsg: glink: Make glink smem interrupt wakeup capable
Date: Mon, 3 Jun 2024 11:37:57 +0200 [thread overview]
Message-ID: <8d10fbbb-471e-4960-a52a-1658df9fbc0c@linaro.org> (raw)
In-Reply-To: <20240603073648.3475123-1-quic_deesin@quicinc.com>
Hi Deepak,
On 03/06/2024 09:36, Deepak Kumar Singh wrote:
> There are certain usecases which require glink interrupt to be
> wakeup capable. For example if handset is in sleep state and
> usb charger is plugged in, dsp wakes up and sends glink interrupt
> to host for glink pmic channel communication. Glink is suppose to
> wakeup host processor completely for further glink data handling.
> IRQF_NO_SUSPEND does not gurantee complete wakeup, system may again
> enter sleep after interrupt handling and glink data may not be
> handled by pmic client driver.
>
> To ensure data handling by client configure glink smem device as
> wakeup source and attach glink interrupt as wakeup irq. Remove
> IRQF_NO_SUSPEND flag as it is no longer required.
I'm not sure I agree with this approach, glink is used for lots of
things -- like QRTR, where the sensor DSP and modem may also need to
wake the system up (e.g. for "wake on pickup" on mobile, or for incoming
calls/sms).
Configuring this to always wake up the system fully will result in a lot
of spurious wakeups for arbitrary modem notifications (e.g. signal
strength changes) if userspace hasn't properly configured these
(something ModemManager currently lacks support for).
IRQF_NO_SUSPEND is presumably necessary to keep the DSPs happy? iirc
downstream Qualcomm kernels have historically taken this approach to
avoid spurious wakeups.
I proposed an alternative approach some time back that would allow the
wakeup to be configured on a per-channel basis.
https://lore.kernel.org/linux-arm-msm/20230117142414.983946-1-caleb.connolly@linaro.org/
Back then Bjorn proposed using some socket specific mechanism to handle
this for QRTR, but given this is now a common issue for multiple glink
channels, maybe it's something we could revisit.
Requiring the wakeup be enabled by userspace clearly doesn't make sense
for your proposed usecase, perhaps there's a way to configure this on a
per-channel basis in-kernel (maybe as the rpmsg API?).
Thanks and regards,
>
> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> ---
> drivers/rpmsg/qcom_glink_smem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 7a982c60a8dd..f1b553efab13 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -22,6 +22,7 @@
> #include <linux/regmap.h>
> #include <linux/workqueue.h>
> #include <linux/list.h>
> +#include <linux/pm_wakeirq.h>
>
> #include <linux/rpmsg/qcom_glink.h>
>
> @@ -306,8 +307,7 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
>
> smem->irq = of_irq_get(smem->dev.of_node, 0);
> ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
> - IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
> - "glink-smem", smem);
> + IRQF_NO_AUTOEN, "glink-smem", smem);
> if (ret) {
> dev_err(&smem->dev, "failed to request IRQ\n");
> goto err_put_dev;
> @@ -346,6 +346,8 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
>
> smem->glink = glink;
>
> + device_init_wakeup(dev, true);
> + dev_pm_set_wake_irq(dev, smem->irq);
> enable_irq(smem->irq);
>
> return smem;
> @@ -365,6 +367,8 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
> struct qcom_glink *glink = smem->glink;
>
> disable_irq(smem->irq);
> + dev_pm_clear_wake_irq(&smem->dev);
> + device_init_wakeup(&smem->dev, false);
>
> qcom_glink_native_remove(glink);
>
--
// Caleb (they/them)
next prev parent reply other threads:[~2024-06-03 9:38 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-03 7:36 [PATCH V1] rpmsg: glink: Make glink smem interrupt wakeup capable Deepak Kumar Singh
2024-06-03 9:37 ` Caleb Connolly [this message]
2024-06-04 22:12 ` Chris Lew
2024-06-13 10:35 ` Deepak Kumar Singh
2024-06-18 23:57 ` Bjorn Andersson
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=8d10fbbb-471e-4960-a52a-1658df9fbc0c@linaro.org \
--to=caleb.connolly@linaro.org \
--cc=andersson@kernel.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=quic_bjorande@quicinc.com \
--cc=quic_clew@quicinc.com \
--cc=quic_deesin@quicinc.com \
--cc=quic_sarannya@quicinc.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).