LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ
@ 2021-08-16 10:05 Deepak Kumar Singh
  2021-08-16 18:31 ` Matthias Kaehlcke
  2021-08-16 20:23 ` Stephen Boyd
  0 siblings, 2 replies; 5+ messages in thread
From: Deepak Kumar Singh @ 2021-08-16 10:05 UTC (permalink / raw)
  To: bjorn.andersson, swboyd, clew, sibis
  Cc: linux-kernel, linux-arm-msm, linux-remoteproc,
	Deepak Kumar Singh, Andy Gross

Remote susbsystems notify fatal crash throught smp2p interrupt.
When modem/wifi crashes it can cause soc to come out of low power state
and may not allow again to enter in low power state until crash is handled.

Mark smp2p interrupt wakeup capable so that interrupt handler is executed
and remote susbsystem crash can be handled in system  resume path.

Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
---
 drivers/soc/qcom/smp2p.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 2df4883..646848b 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -18,6 +18,7 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/spinlock.h>
+#include <linux/pm_wakeirq.h>
 
 /*
  * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
@@ -538,9 +539,20 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 		goto unwind_interfaces;
 	}
 
+	/* Setup smp2p interrupt as wakeup source */
+	ret = device_init_wakeup(&pdev->dev, true);
+	if (ret)
+		goto unwind_interfaces;
+
+	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
+	if (ret)
+		goto set_wakeup_failed;
 
 	return 0;
 
+set_wakeup_failed:
+	device_init_wakeup(&pdev->dev, false);
+
 unwind_interfaces:
 	list_for_each_entry(entry, &smp2p->inbound, node)
 		irq_domain_remove(entry->domain);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ
  2021-08-16 10:05 [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ Deepak Kumar Singh
@ 2021-08-16 18:31 ` Matthias Kaehlcke
  2021-08-16 20:23 ` Stephen Boyd
  1 sibling, 0 replies; 5+ messages in thread
From: Matthias Kaehlcke @ 2021-08-16 18:31 UTC (permalink / raw)
  To: Deepak Kumar Singh
  Cc: bjorn.andersson, swboyd, clew, sibis, linux-kernel,
	linux-arm-msm, linux-remoteproc, Andy Gross

On Mon, Aug 16, 2021 at 03:35:35PM +0530, Deepak Kumar Singh wrote:
> Remote susbsystems notify fatal crash throught smp2p interrupt.
> When modem/wifi crashes it can cause soc to come out of low power state
> and may not allow again to enter in low power state until crash is handled.
> 
> Mark smp2p interrupt wakeup capable so that interrupt handler is executed
> and remote susbsystem crash can be handled in system  resume path.
> 
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/soc/qcom/smp2p.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 2df4883..646848b 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -18,6 +18,7 @@
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
>  #include <linux/spinlock.h>
> +#include <linux/pm_wakeirq.h>
>  
>  /*
>   * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
> @@ -538,9 +539,20 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  		goto unwind_interfaces;
>  	}
>  
> +	/* Setup smp2p interrupt as wakeup source */
> +	ret = device_init_wakeup(&pdev->dev, true);
> +	if (ret)
> +		goto unwind_interfaces;
> +
> +	ret = dev_pm_set_wake_irq(&pdev->dev, irq);
> +	if (ret)
> +		goto set_wakeup_failed;
>  
>  	return 0;
>  
> +set_wakeup_failed:
> +	device_init_wakeup(&pdev->dev, false);
> +

I think you need to call dev_pm_clear_wake_irq() and
device_init_wakeup(..., false) in qcom_smp2p_remove()
to free all resources.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ
  2021-08-16 10:05 [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ Deepak Kumar Singh
  2021-08-16 18:31 ` Matthias Kaehlcke
@ 2021-08-16 20:23 ` Stephen Boyd
  2021-09-13 17:45   ` Deepak Kumar Singh
  1 sibling, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2021-08-16 20:23 UTC (permalink / raw)
  To: Deepak Kumar Singh, bjorn.andersson, clew, sibis
  Cc: linux-kernel, linux-arm-msm, linux-remoteproc, Andy Gross

Quoting Deepak Kumar Singh (2021-08-16 03:05:35)
> Remote susbsystems notify fatal crash throught smp2p interrupt.
> When modem/wifi crashes it can cause soc to come out of low power state
> and may not allow again to enter in low power state until crash is handled.
>
> Mark smp2p interrupt wakeup capable so that interrupt handler is executed
> and remote susbsystem crash can be handled in system  resume path.
>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/soc/qcom/smp2p.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 2df4883..646848b 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -18,6 +18,7 @@
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
>  #include <linux/spinlock.h>
> +#include <linux/pm_wakeirq.h>

Please sort alphabetically by include name, 'p' before 's'.

>
>  /*
>   * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
> @@ -538,9 +539,20 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>                 goto unwind_interfaces;
>         }
>
> +       /* Setup smp2p interrupt as wakeup source */

This comment is bad. Please don't reiterate what the code is doing.
Instead, write something like

	/*
	 * Treat remoteproc crashes as wakeups by default so we handle
	 * them sooner rather than along with the next wakeup (e.g.
	 * power button). This avoids leaving the system in a shallower
	 * suspend power state if a remoteproc crashes during suspend,
	 * but requires userspace to actively suspend the device after
	 * handling the crash, or CONFIG_PM_AUTOSLEEP to be true.
	 */

> +       ret = device_init_wakeup(&pdev->dev, true);

I still wonder if it's better to leave this off by default and only
enable it if the kernel is using autosuspend (PM_AUTOSLEEP). Then
userspace is responsible to decide if it can handle the wakeup with the
screen off, reload the remoteproc, and go back to suspend if it isn't
using autosuspend.

> +       if (ret)
> +               goto unwind_interfaces;
> +

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ
  2021-08-16 20:23 ` Stephen Boyd
@ 2021-09-13 17:45   ` Deepak Kumar Singh
  2021-09-13 19:42     ` Stephen Boyd
  0 siblings, 1 reply; 5+ messages in thread
From: Deepak Kumar Singh @ 2021-09-13 17:45 UTC (permalink / raw)
  To: Stephen Boyd, bjorn.andersson, clew, sibis
  Cc: linux-kernel, linux-arm-msm, linux-remoteproc, Andy Gross


On 8/17/2021 1:53 AM, Stephen Boyd wrote:
> Quoting Deepak Kumar Singh (2021-08-16 03:05:35)
>> Remote susbsystems notify fatal crash throught smp2p interrupt.
>> When modem/wifi crashes it can cause soc to come out of low power state
>> and may not allow again to enter in low power state until crash is handled.
>>
>> Mark smp2p interrupt wakeup capable so that interrupt handler is executed
>> and remote susbsystem crash can be handled in system  resume path.
>>
>> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
>> ---
>>   drivers/soc/qcom/smp2p.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
>> index 2df4883..646848b 100644
>> --- a/drivers/soc/qcom/smp2p.c
>> +++ b/drivers/soc/qcom/smp2p.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/soc/qcom/smem.h>
>>   #include <linux/soc/qcom/smem_state.h>
>>   #include <linux/spinlock.h>
>> +#include <linux/pm_wakeirq.h>
> Please sort alphabetically by include name, 'p' before 's'.
>
>>   /*
>>    * The Shared Memory Point to Point (SMP2P) protocol facilitates communication
>> @@ -538,9 +539,20 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>>                  goto unwind_interfaces;
>>          }
>>
>> +       /* Setup smp2p interrupt as wakeup source */
> This comment is bad. Please don't reiterate what the code is doing.
> Instead, write something like
>
> 	/*
> 	 * Treat remoteproc crashes as wakeups by default so we handle
> 	 * them sooner rather than along with the next wakeup (e.g.
> 	 * power button). This avoids leaving the system in a shallower
> 	 * suspend power state if a remoteproc crashes during suspend,
> 	 * but requires userspace to actively suspend the device after
> 	 * handling the crash, or CONFIG_PM_AUTOSLEEP to be true.
> 	 */
>
>> +       ret = device_init_wakeup(&pdev->dev, true);
> I still wonder if it's better to leave this off by default and only
> enable it if the kernel is using autosuspend (PM_AUTOSLEEP). Then
> userspace is responsible to decide if it can handle the wakeup with the
> screen off, reload the remoteproc, and go back to suspend if it isn't
> using autosuspend.

Seems like not all targets use PM_AUTOSLEEP feature, even those targets 
may require wakeup to handle

modem crash so that important modem events are not missed. I think we 
can keep wake up as default behavior

and let the user space disable it through sysfs if it doesn't want it as 
wake up source.

>> +       if (ret)
>> +               goto unwind_interfaces;
>> +

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ
  2021-09-13 17:45   ` Deepak Kumar Singh
@ 2021-09-13 19:42     ` Stephen Boyd
  0 siblings, 0 replies; 5+ messages in thread
From: Stephen Boyd @ 2021-09-13 19:42 UTC (permalink / raw)
  To: Deepak Kumar Singh, bjorn.andersson, clew, sibis
  Cc: linux-kernel, linux-arm-msm, linux-remoteproc, Andy Gross

Quoting Deepak Kumar Singh (2021-09-13 10:45:19)
>
> On 8/17/2021 1:53 AM, Stephen Boyd wrote:
> >> +       ret = device_init_wakeup(&pdev->dev, true);
> > I still wonder if it's better to leave this off by default and only
> > enable it if the kernel is using autosuspend (PM_AUTOSLEEP). Then
> > userspace is responsible to decide if it can handle the wakeup with the
> > screen off, reload the remoteproc, and go back to suspend if it isn't
> > using autosuspend.
>
> Seems like not all targets use PM_AUTOSLEEP feature, even those targets
> may require wakeup to handle
>
> modem crash so that important modem events are not missed. I think we
> can keep wake up as default behavior
>
> and let the user space disable it through sysfs if it doesn't want it as
> wake up source.

I don't understand. What if userspace is simple and doesn't use
autosleep and will turn the screen on when the kernel resumes? Why do we
expect the modem crashing and causing the screen to turn on to be a good
user experience?

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-09-13 19:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-16 10:05 [PATCH V2 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ Deepak Kumar Singh
2021-08-16 18:31 ` Matthias Kaehlcke
2021-08-16 20:23 ` Stephen Boyd
2021-09-13 17:45   ` Deepak Kumar Singh
2021-09-13 19:42     ` Stephen Boyd

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 NNTP newsgroup(s).