| Message ID | 1582164713-6413-3-git-send-email-sidgup@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: core: Add core functionality to the remoteproc framework | expand |
On Wed, Feb 19, 2020 at 06:11:53PM -0800, Siddharth Gupta wrote: > Remoteproc recovery should be fast and any delay will have an impact on the > user-experience. Use power management APIs (pm_stay_awake and pm_relax) to > ensure that the system does not go to sleep. When you say "ensure the system does not go to sleep", you're referring to the system going idle from the CPUidle subsystem? > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 5ab65a4..52e318c 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work) > > if (!rproc->recovery_disabled) > rproc_trigger_recovery(rproc); > + > + pm_relax(&rproc->dev); > } > > /** > @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type) > return; > } > > + pm_stay_awake(&rproc->dev); > + I fail to understand how this can be useful since there is no HW associted to rproc->dev... Is it possible for you to elaborate more on the problem you're trying to fix? Thanks, Mathieu > dev_err(&rproc->dev, "crash detected in %s: type %s\n", > rproc->name, rproc_crash_to_string(type)); > > -- > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
Hey Mathieu, I will be sending a revised patchset soon. Will try to address your comments there. Thanks, Siddharth On 2/24/2020 10:53 AM, Mathieu Poirier wrote: > On Wed, Feb 19, 2020 at 06:11:53PM -0800, Siddharth Gupta wrote: >> Remoteproc recovery should be fast and any delay will have an impact on the >> user-experience. Use power management APIs (pm_stay_awake and pm_relax) to >> ensure that the system does not go to sleep. > When you say "ensure the system does not go to sleep", you're referring to the > system going idle from the CPUidle subsystem? > >> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 5ab65a4..52e318c 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work) >> >> if (!rproc->recovery_disabled) >> rproc_trigger_recovery(rproc); >> + >> + pm_relax(&rproc->dev); >> } >> >> /** >> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type) >> return; >> } >> >> + pm_stay_awake(&rproc->dev); >> + > I fail to understand how this can be useful since there is no HW associted to > rproc->dev... Is it possible for you to elaborate more on the problem you're > trying to fix? > > Thanks, > Mathieu > >> dev_err(&rproc->dev, "crash detected in %s: type %s\n", >> rproc->name, rproc_crash_to_string(type)); >> >> -- >> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project
On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote: > Remoteproc recovery should be fast and any delay will have an impact on the > user-experience. Use power management APIs (pm_stay_awake and pm_relax) to > ensure that the system does not go to sleep. > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 5ab65a4..52e318c 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work) > > if (!rproc->recovery_disabled) > rproc_trigger_recovery(rproc); > + > + pm_relax(&rproc->dev); > } > > /** > @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type) > return; > } > > + pm_stay_awake(&rproc->dev); Following Mathieu's question I was expecting you to do this on rproc->dev.parent. But looking at the implementation of pm_stay_awake(), it ends up being a nop if dev->power.wakeup isn't specified. This in turn seems to come from device_wakeup_enable(), which will bail if dev->power.can_wakeup is not set. But I don't see where this would be set for either the platform driver or the remoteproc's struct device - and neither one of them have a "wakeup" attribute in sysfs. Is there some additional plumbing needed for this? Regards, Bjorn > + > dev_err(&rproc->dev, "crash detected in %s: type %s\n", > rproc->name, rproc_crash_to_string(type)); > > -- > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
On 2020-04-07 15:29, Bjorn Andersson wrote: > On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote: > >> Remoteproc recovery should be fast and any delay will have an impact >> on the >> user-experience. Use power management APIs (pm_stay_awake and >> pm_relax) to >> ensure that the system does not go to sleep. >> >> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c >> b/drivers/remoteproc/remoteproc_core.c >> index 5ab65a4..52e318c 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct >> work_struct *work) >> >> if (!rproc->recovery_disabled) >> rproc_trigger_recovery(rproc); >> + >> + pm_relax(&rproc->dev); >> } >> >> /** >> @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, >> enum rproc_crash_type type) >> return; >> } >> >> + pm_stay_awake(&rproc->dev); > > Following Mathieu's question I was expecting you to do this on > rproc->dev.parent. > > But looking at the implementation of pm_stay_awake(), it ends up being > a > nop if dev->power.wakeup isn't specified. This in turn seems to come > from device_wakeup_enable(), which will bail if dev->power.can_wakeup > is > not set. But I don't see where this would be set for either the > platform > driver or the remoteproc's struct device - and neither one of them have > a "wakeup" attribute in sysfs. > > Is there some additional plumbing needed for this? We should be able to create a standalone wakeup source using wakeup_source_init. Then we can use _pm_stay_awake and _pm_relax on it. > > Regards, > Bjorn > >> + >> dev_err(&rproc->dev, "crash detected in %s: type %s\n", >> rproc->name, rproc_crash_to_string(type)); >> >> -- >> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project
On Tue 07 Apr 15:59 PDT 2020, rishabhb@codeaurora.org wrote: > On 2020-04-07 15:29, Bjorn Andersson wrote: > > On Wed 19 Feb 18:11 PST 2020, Siddharth Gupta wrote: > > > > > Remoteproc recovery should be fast and any delay will have an impact > > > on the > > > user-experience. Use power management APIs (pm_stay_awake and > > > pm_relax) to > > > ensure that the system does not go to sleep. > > > > > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > > > --- > > > drivers/remoteproc/remoteproc_core.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c > > > b/drivers/remoteproc/remoteproc_core.c > > > index 5ab65a4..52e318c 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct > > > work_struct *work) > > > > > > if (!rproc->recovery_disabled) > > > rproc_trigger_recovery(rproc); > > > + > > > + pm_relax(&rproc->dev); > > > } > > > > > > /** > > > @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, > > > enum rproc_crash_type type) > > > return; > > > } > > > > > > + pm_stay_awake(&rproc->dev); > > > > Following Mathieu's question I was expecting you to do this on > > rproc->dev.parent. > > > > But looking at the implementation of pm_stay_awake(), it ends up being a > > nop if dev->power.wakeup isn't specified. This in turn seems to come > > from device_wakeup_enable(), which will bail if dev->power.can_wakeup is > > not set. But I don't see where this would be set for either the platform > > driver or the remoteproc's struct device - and neither one of them have > > a "wakeup" attribute in sysfs. > > > > Is there some additional plumbing needed for this? > We should be able to create a standalone wakeup source using > wakeup_source_init. > Then we can use _pm_stay_awake and _pm_relax on it. Afaict the way to do this would be to call device_wakeup_enable() on either the remoteproc or platform driver's struct device. Given that the resources related to waking up the system are associated with the platform driver I think this should be done on the platform driver's struct device and these calls should operate on the rproc's parent. Regards, Bjorn > > > > Regards, > > Bjorn > > > > > + > > > dev_err(&rproc->dev, "crash detected in %s: type %s\n", > > > rproc->name, rproc_crash_to_string(type)); > > > > > > -- > > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > a Linux Foundation Collaborative Project
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 5ab65a4..52e318c 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1712,6 +1712,8 @@ static void rproc_crash_handler_work(struct work_struct *work) if (!rproc->recovery_disabled) rproc_trigger_recovery(rproc); + + pm_relax(&rproc->dev); } /** @@ -2242,6 +2244,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type) return; } + pm_stay_awake(&rproc->dev); + dev_err(&rproc->dev, "crash detected in %s: type %s\n", rproc->name, rproc_crash_to_string(type));
Remoteproc recovery should be fast and any delay will have an impact on the user-experience. Use power management APIs (pm_stay_awake and pm_relax) to ensure that the system does not go to sleep. Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> --- drivers/remoteproc/remoteproc_core.c | 4 ++++ 1 file changed, 4 insertions(+)