| Message ID | 1628180254-758-1-git-send-email-deesin@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V1,1/1] soc: qcom: smp2p: Add wakeup capability to SMP2P IRQ | expand |
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);
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);
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?
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?
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.
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
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.
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
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);
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(+)