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@xxxxxxxxxx/
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@xxxxxxxxxxx>
---
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)
