[PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Deepak Kumar Singh]

Some use cases require SMP2P interrupts to wake up the host
from suspend. Mark smp2p interrupt as wakeup capable to abort
the suspend.

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

diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 2df4883..f8659b0 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,19 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
 		goto unwind_interfaces;
 	}
 
+	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

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
> Some use cases require SMP2P interrupts to wake up the host
> from suspend.

Please elaborate on this point so we understand what sort of scenarios
want to wakeup from suspend.

> Mark smp2p interrupt as wakeup capable to abort
> the suspend.
>
> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> ---
>  drivers/soc/qcom/smp2p.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index 2df4883..f8659b0 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,19 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>                 goto unwind_interfaces;
>         }
>
> +       ret = device_init_wakeup(&pdev->dev, true);

Is smp2p supposed to wake up the device by default? If not, then this
should be device_set_wakeup_capable() instead so that userspace can
decide if it wants to get the wakeup.

> +       if (ret)
> +               goto unwind_interfaces;
> +
> +       ret = dev_pm_set_wake_irq(&pdev->dev, irq);
> +       if (ret)
> +               goto set_wakeup_failed;

Otherwise this looks good to me.

>
>         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);

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Deepak Kumar Singh]

On 8/6/2021 1:10 AM, Stephen Boyd wrote:
> Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
>> Some use cases require SMP2P interrupts to wake up the host
>> from suspend.
> Please elaborate on this point so we understand what sort of scenarios
> want to wakeup from suspend.

Once such scenario is where WiFi/modem crashes and notifies crash to 
local host through smp2p

if local host is in suspend it should wake up to handle the crash and 
reboot the WiFi/modem.

>> Mark smp2p interrupt as wakeup capable to abort
>> the suspend.
>>
>> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
>> ---
>>   drivers/soc/qcom/smp2p.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
>> index 2df4883..f8659b0 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,19 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>>                  goto unwind_interfaces;
>>          }
>>
>> +       ret = device_init_wakeup(&pdev->dev, true);
> Is smp2p supposed to wake up the device by default? If not, then this
> should be device_set_wakeup_capable() instead so that userspace can
> decide if it wants to get the wakeup.
yes, we want smp2p to be wake up capable by default.
>> +       if (ret)
>> +               goto unwind_interfaces;
>> +
>> +       ret = dev_pm_set_wake_irq(&pdev->dev, irq);
>> +       if (ret)
>> +               goto set_wakeup_failed;
> Otherwise this looks good to me.
>
>>          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);

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Stephen Boyd]

Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
>
> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
> > Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
> >> Some use cases require SMP2P interrupts to wake up the host
> >> from suspend.
> > Please elaborate on this point so we understand what sort of scenarios
> > want to wakeup from suspend.
>
> Once such scenario is where WiFi/modem crashes and notifies crash to
> local host through smp2p
>
> if local host is in suspend it should wake up to handle the crash and
> reboot the WiFi/modem.

Does anything go wrong if the firmware crashes during suspend and the
local host doesn't handle it until it wakes for some other reason? I'd
like to understand if the crash handling can be delayed/combined with
another wakeup.

>
> >> Mark smp2p interrupt as wakeup capable to abort
> >> the suspend.
> >>
> >> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
> >> ---
> >>   drivers/soc/qcom/smp2p.c | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> >> index 2df4883..f8659b0 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,19 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
> >>                  goto unwind_interfaces;
> >>          }
> >>
> >> +       ret = device_init_wakeup(&pdev->dev, true);
> > Is smp2p supposed to wake up the device by default? If not, then this
> > should be device_set_wakeup_capable() instead so that userspace can
> > decide if it wants to get the wakeup.
> yes, we want smp2p to be wake up capable by default.

Why?

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Sibi Sankar]

On 2021-08-09 23:28, Stephen Boyd wrote:
> Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
>> 
>> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
>> > Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
>> >> Some use cases require SMP2P interrupts to wake up the host
>> >> from suspend.
>> > Please elaborate on this point so we understand what sort of scenarios
>> > want to wakeup from suspend.
>> 
>> Once such scenario is where WiFi/modem crashes and notifies crash to
>> local host through smp2p
>> 
>> if local host is in suspend it should wake up to handle the crash and
>> reboot the WiFi/modem.
> 
> Does anything go wrong if the firmware crashes during suspend and the
> local host doesn't handle it until it wakes for some other reason? I'd
> like to understand if the crash handling can be delayed/combined with
> another wakeup.

If the modem firmware crashes
during suspend, the system comes
out of xo-shutdown and AFAIK stays
there until we handle the interrupt.

> 
>> 
>> >> Mark smp2p interrupt as wakeup capable to abort
>> >> the suspend.
>> >>
>> >> Signed-off-by: Deepak Kumar Singh <deesin@codeaurora.org>
>> >> ---
>> >>   drivers/soc/qcom/smp2p.c | 11 +++++++++++
>> >>   1 file changed, 11 insertions(+)
>> >>
>> >> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
>> >> index 2df4883..f8659b0 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,19 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>> >>                  goto unwind_interfaces;
>> >>          }
>> >>
>> >> +       ret = device_init_wakeup(&pdev->dev, true);
>> > Is smp2p supposed to wake up the device by default? If not, then this
>> > should be device_set_wakeup_capable() instead so that userspace can
>> > decide if it wants to get the wakeup.
>> yes, we want smp2p to be wake up capable by default.
> 
> Why?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Stephen Boyd]

Quoting Sibi Sankar (2021-08-10 10:24:32)
> On 2021-08-09 23:28, Stephen Boyd wrote:
> > Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
> >>
> >> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
> >> > Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
> >> >> Some use cases require SMP2P interrupts to wake up the host
> >> >> from suspend.
> >> > Please elaborate on this point so we understand what sort of scenarios
> >> > want to wakeup from suspend.
> >>
> >> Once such scenario is where WiFi/modem crashes and notifies crash to
> >> local host through smp2p
> >>
> >> if local host is in suspend it should wake up to handle the crash and
> >> reboot the WiFi/modem.
> >
> > Does anything go wrong if the firmware crashes during suspend and the
> > local host doesn't handle it until it wakes for some other reason? I'd
> > like to understand if the crash handling can be delayed/combined with
> > another wakeup.
>
> If the modem firmware crashes
> during suspend, the system comes
> out of xo-shutdown and AFAIK stays
> there until we handle the interrupt.
>

So you're saying we waste power if we don't wakeup the AP and leave the
SoC in a shallow low power state? That would be good to have indicated
in the code via a comment and in the commit text so we know that we want
to handle the wakeup by default.

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Bjorn Andersson]

On Tue 10 Aug 14:18 CDT 2021, Stephen Boyd wrote:

> Quoting Sibi Sankar (2021-08-10 10:24:32)
> > On 2021-08-09 23:28, Stephen Boyd wrote:
> > > Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
> > >>
> > >> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
> > >> > Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
> > >> >> Some use cases require SMP2P interrupts to wake up the host
> > >> >> from suspend.
> > >> > Please elaborate on this point so we understand what sort of scenarios
> > >> > want to wakeup from suspend.
> > >>
> > >> Once such scenario is where WiFi/modem crashes and notifies crash to
> > >> local host through smp2p
> > >>
> > >> if local host is in suspend it should wake up to handle the crash and
> > >> reboot the WiFi/modem.
> > >
> > > Does anything go wrong if the firmware crashes during suspend and the
> > > local host doesn't handle it until it wakes for some other reason? I'd
> > > like to understand if the crash handling can be delayed/combined with
> > > another wakeup.
> >
> > If the modem firmware crashes
> > during suspend, the system comes
> > out of xo-shutdown and AFAIK stays
> > there until we handle the interrupt.
> >
> 
> So you're saying we waste power if we don't wakeup the AP and leave the
> SoC in a shallow low power state? That would be good to have indicated
> in the code via a comment and in the commit text so we know that we want
> to handle the wakeup by default.

Sounds like in a system without autosleep (or userspace equivalent) it
would be desirable to leave the SoC in this lower state than to wake up
the system handle the crash and then just idle?

But leaving the system in this state will result in you missing your
important phone calls...

Regards,
Bjorn

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Stephen Boyd]

Quoting Bjorn Andersson (2021-08-10 16:11:10)
> On Tue 10 Aug 14:18 CDT 2021, Stephen Boyd wrote:
>
> > Quoting Sibi Sankar (2021-08-10 10:24:32)
> > > On 2021-08-09 23:28, Stephen Boyd wrote:
> > > > Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
> > > >>
> > > >> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
> > > >> > Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
> > > >> >> Some use cases require SMP2P interrupts to wake up the host
> > > >> >> from suspend.
> > > >> > Please elaborate on this point so we understand what sort of scenarios
> > > >> > want to wakeup from suspend.
> > > >>
> > > >> Once such scenario is where WiFi/modem crashes and notifies crash to
> > > >> local host through smp2p
> > > >>
> > > >> if local host is in suspend it should wake up to handle the crash and
> > > >> reboot the WiFi/modem.
> > > >
> > > > Does anything go wrong if the firmware crashes during suspend and the
> > > > local host doesn't handle it until it wakes for some other reason? I'd
> > > > like to understand if the crash handling can be delayed/combined with
> > > > another wakeup.
> > >
> > > If the modem firmware crashes
> > > during suspend, the system comes
> > > out of xo-shutdown and AFAIK stays
> > > there until we handle the interrupt.
> > >
> >
> > So you're saying we waste power if we don't wakeup the AP and leave the
> > SoC in a shallow low power state? That would be good to have indicated
> > in the code via a comment and in the commit text so we know that we want
> > to handle the wakeup by default.
>
> Sounds like in a system without autosleep (or userspace equivalent) it
> would be desirable to leave the SoC in this lower state than to wake up
> the system handle the crash and then just idle?
>
> But leaving the system in this state will result in you missing your
> important phone calls...
>

Yes I think we should just add a comment to the code and commit text and
move on.

Re: [PATCH V1 1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ

[Deepak Kumar Singh]

On 8/11/2021 9:55 AM, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-08-10 16:11:10)
>> On Tue 10 Aug 14:18 CDT 2021, Stephen Boyd wrote:
>>
>>> Quoting Sibi Sankar (2021-08-10 10:24:32)
>>>> On 2021-08-09 23:28, Stephen Boyd wrote:
>>>>> Quoting Deepak Kumar Singh (2021-08-09 04:05:08)
>>>>>> On 8/6/2021 1:10 AM, Stephen Boyd wrote:
>>>>>>> Quoting Deepak Kumar Singh (2021-08-05 09:17:33)
>>>>>>>> Some use cases require SMP2P interrupts to wake up the host
>>>>>>>> from suspend.
>>>>>>> Please elaborate on this point so we understand what sort of scenarios
>>>>>>> want to wakeup from suspend.
>>>>>> Once such scenario is where WiFi/modem crashes and notifies crash to
>>>>>> local host through smp2p
>>>>>>
>>>>>> if local host is in suspend it should wake up to handle the crash and
>>>>>> reboot the WiFi/modem.
>>>>> Does anything go wrong if the firmware crashes during suspend and the
>>>>> local host doesn't handle it until it wakes for some other reason? I'd
>>>>> like to understand if the crash handling can be delayed/combined with
>>>>> another wakeup.
>>>> If the modem firmware crashes
>>>> during suspend, the system comes
>>>> out of xo-shutdown and AFAIK stays
>>>> there until we handle the interrupt.
>>>>
>>> So you're saying we waste power if we don't wakeup the AP and leave the
>>> SoC in a shallow low power state? That would be good to have indicated
>>> in the code via a comment and in the commit text so we know that we want
>>> to handle the wakeup by default.
>> Sounds like in a system without autosleep (or userspace equivalent) it
>> would be desirable to leave the SoC in this lower state than to wake up
>> the system handle the crash and then just idle?
>>
>> But leaving the system in this state will result in you missing your
>> important phone calls...
>>
> Yes I think we should just add a comment to the code and commit text and
> move on.
Thanks, updated in patch set V2