[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d10fbbb-471e-4960-a52a-1658df9fbc0c@linaro.org>
Date: Mon, 3 Jun 2024 11:37:57 +0200
From: Caleb Connolly <caleb.connolly@...aro.org>
To: Deepak Kumar Singh <quic_deesin@...cinc.com>, quic_bjorande@...cinc.com,
andersson@...nel.org, quic_clew@...cinc.com, mathieu.poirier@...aro.org
Cc: linux-kernel@...r.kernel.org, quic_sarannya@...cinc.com,
linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH V1] rpmsg: glink: Make glink smem interrupt wakeup capable
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@...cinc.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)
Powered by blists - more mailing lists