Received-SPF: pass (google.com: domain of linux-kernel+bounces-198918-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <8d10fbbb-471e-4960-a52a-1658df9fbc0c@linaro.org>
Date: Mon, 3 Jun 2024 11:37:57 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V1] rpmsg: glink: Make glink smem interrupt wakeup capable
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
References: <20240603073648.3475123-1-quic_deesin@quicinc.com>
Content-Language: en-US
From: Caleb Connolly <caleb.connolly@linaro.org>
In-Reply-To: <20240603073648.3475123-1-quic_deesin@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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)