| Message ID | 20191213125537.11509-13-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: updates for omap remoteproc support | expand |
On Fri, Dec 13, 2019 at 02:55:34PM +0200, Tero Kristo wrote: > From: Suman Anna <s-anna@ti.com> > > This patch adds the support for system suspend/resume to the > OMAP remoteproc driver so that the OMAP remoteproc devices can > be suspended/resumed during a system suspend/resume. The support > is added through the driver PM .suspend/.resume callbacks, and > requires appropriate support from the OS running on the remote > processors. > > The IPU & DSP remote processors typically have their own private > modules like registers, internal memories, caches etc. The context > of these modules need to be saved and restored properly for a > suspend/resume to work. These are in general not accessible from > the MPU, so the remote processors themselves have to implement > the logic for the context save & restore of these modules. > > The OMAP remoteproc driver initiates a suspend by sending a mailbox > message requesting the remote processor to save its context and > enter into an idle/standby state. The remote processor should > usually stop whatever processing it is doing to switch to a context > save mode. The OMAP remoteproc driver detects the completion of > the context save by checking the module standby status for the > remoteproc device. It also stops any resources used by the remote > processors like the timers. The timers need to be running only > when the processor is active and executing, and need to be stopped > otherwise to allow the timer driver to reach low-power states. The > IOMMUs are automatically suspended by the PM core during the late > suspend stage, after the remoteproc suspend process is completed by > putting the remote processor cores into reset. Thereafter, the Linux > kernel can put the domain into further lower power states as possible. > > The resume sequence undoes the operations performed in the PM suspend > callback, by starting the timers and finally releasing the processors > from reset. This requires that the remote processor side OS be able to > distinguish a power-resume boot from a power-on/cold boot, restore the > context of its private modules saved during the suspend phase, and > resume executing code from where it was suspended. The IOMMUs would > have been resumed by the PM core during early resume, so they are > already enabled by the time remoteproc resume callback gets invoked. > > The remote processors should save their context into System RAM (DDR), > as any internal memories are not guaranteed to retain context as it > depends on the lowest power domain that the remote processor device > is put into. The management of the DDR contents will be managed by > the Linux kernel. > > Signed-off-by: Suman Anna <s-anna@ti.com> > [t-kristo@ti.com: converted to use ti-sysc instead of hwmod] > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/remoteproc/omap_remoteproc.c | 179 +++++++++++++++++++++++++++ > drivers/remoteproc/omap_remoteproc.h | 18 ++- > 2 files changed, 195 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > index 9c750c2ab29d..0a9b9f7d20da 100644 > --- a/drivers/remoteproc/omap_remoteproc.c > +++ b/drivers/remoteproc/omap_remoteproc.c > @@ -16,6 +16,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/err.h> > +#include <linux/io.h> > #include <linux/of_device.h> > #include <linux/of_reserved_mem.h> > #include <linux/platform_device.h> > @@ -23,10 +24,13 @@ > #include <linux/remoteproc.h> > #include <linux/mailbox_client.h> > #include <linux/omap-mailbox.h> > +#include <linux/omap-iommu.h> Please move this up by one line. > #include <linux/regmap.h> > #include <linux/mfd/syscon.h> > #include <linux/reset.h> > #include <clocksource/timer-ti-dm.h> > +#include <linux/clk.h> > +#include <linux/clk/ti.h> Unless there is a dependency with timer-ti-dm.h, these should go just above linux/err.h > > #include <linux/platform_data/dmtimer-omap.h> > > @@ -81,6 +85,9 @@ struct omap_rproc_timer { > * @timers: timer(s) info used by rproc > * @rproc: rproc handle > * @reset: reset handle > + * @pm_comp: completion primitive to sync for suspend response > + * @fck: functional clock for the remoteproc > + * @suspend_acked: state machine flag to store the suspend request ack > */ > struct omap_rproc { > struct mbox_chan *mbox; > @@ -92,6 +99,9 @@ struct omap_rproc { > struct omap_rproc_timer *timers; > struct rproc *rproc; > struct reset_control *reset; > + struct completion pm_comp; > + struct clk *fck; > + bool suspend_acked; > }; > > /** > @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data) > case RP_MBOX_ECHO_REPLY: > dev_info(dev, "received echo reply from %s\n", name); > break; > + case RP_MBOX_SUSPEND_ACK: We can't get away with implicit fallthroughs anymore - please add /* Fall through */" > + case RP_MBOX_SUSPEND_CANCEL: > + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; > + complete(&oproc->pm_comp); > + break; > default: > if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) > return; > @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { > .da_to_va = omap_rproc_da_to_va, > }; > > +#ifdef CONFIG_PM > +static bool _is_rproc_in_standby(struct omap_rproc *oproc) > +{ > + return ti_clk_is_in_standby(oproc->fck); > +} > + > +/* 1 sec is long enough time to let the remoteproc side suspend the device */ > +#define DEF_SUSPEND_TIMEOUT 1000 > +static int _omap_rproc_suspend(struct rproc *rproc) > +{ > + struct device *dev = rproc->dev.parent; > + struct omap_rproc *oproc = rproc->priv; > + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); > + unsigned long ta = jiffies + to; > + int ret; > + > + reinit_completion(&oproc->pm_comp); > + oproc->suspend_acked = false; > + ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); > + if (ret < 0) { > + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); > + return ret; > + } > + > + ret = wait_for_completion_timeout(&oproc->pm_comp, to); > + if (!oproc->suspend_acked) > + return -EBUSY; > + > + /* > + * The remoteproc side is returning the ACK message before saving the > + * context, because the context saving is performed within a SYS/BIOS > + * function, and it cannot have any inter-dependencies against the IPC > + * layer. Also, as the SYS/BIOS needs to preserve properly the processor > + * register set, sending this ACK or signalling the completion of the > + * context save through a shared memory variable can never be the > + * absolute last thing to be executed on the remoteproc side, and the > + * MPU cannot use the ACK message as a sync point to put the remoteproc > + * into reset. The only way to ensure that the remote processor has > + * completed saving the context is to check that the module has reached > + * STANDBY state (after saving the context, the SYS/BIOS executes the > + * appropriate target-specific WFI instruction causing the module to > + * enter STANDBY). > + */ > + while (!_is_rproc_in_standby(oproc)) { > + if (time_after(jiffies, ta)) > + return -ETIME; > + schedule(); > + } > + > + reset_control_assert(oproc->reset); > + > + ret = omap_rproc_disable_timers(rproc, false); > + if (ret) { > + dev_err(dev, "disabling timers during suspend failed %d\n", > + ret); > + goto enable_device; > + } > + > + return 0; > + > +enable_device: > + reset_control_deassert(oproc->reset); > + return ret; > +} > + > +static int _omap_rproc_resume(struct rproc *rproc) > +{ > + struct device *dev = rproc->dev.parent; > + struct omap_rproc *oproc = rproc->priv; > + int ret; > + > + /* boot address could be lost after suspend, so restore it */ > + if (oproc->boot_data) { > + ret = omap_rproc_write_dsp_boot_addr(rproc); > + if (ret) { > + dev_err(dev, "boot address restore failed %d\n", ret); > + goto out; > + } > + } > + > + ret = omap_rproc_enable_timers(rproc, false); > + if (ret) { > + dev_err(dev, "enabling timers during resume failed %d\n", > + ret); The "ret);" fits on the live above. > + goto out; > + } > + > + reset_control_deassert(oproc->reset); > + > +out: > + return ret; > +} > + > +static int __maybe_unused omap_rproc_suspend(struct device *dev) The "__maybe_unused" can be dropped because this is within the #ifdef CONFIG_PM block. > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct rproc *rproc = platform_get_drvdata(pdev); > + int ret = 0; > + > + mutex_lock(&rproc->lock); > + if (rproc->state == RPROC_OFFLINE) > + goto out; > + > + if (rproc->state == RPROC_SUSPENDED) > + goto out; > + > + if (rproc->state != RPROC_RUNNING) { > + ret = -EBUSY; > + goto out; > + } > + > + ret = _omap_rproc_suspend(rproc); > + if (ret) { > + dev_err(dev, "suspend failed %d\n", ret); > + goto out; > + } > + > + rproc->state = RPROC_SUSPENDED; > +out: > + mutex_unlock(&rproc->lock); > + return ret; > +} > + > +static int __maybe_unused omap_rproc_resume(struct device *dev) Same comment as above. > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct rproc *rproc = platform_get_drvdata(pdev); > + int ret = 0; > + > + mutex_lock(&rproc->lock); > + if (rproc->state == RPROC_OFFLINE) > + goto out; > + > + if (rproc->state != RPROC_SUSPENDED) { > + ret = -EBUSY; > + goto out; > + } > + > + ret = _omap_rproc_resume(rproc); > + if (ret) { > + dev_err(dev, "resume failed %d\n", ret); > + goto out; > + } > + > + rproc->state = RPROC_RUNNING; > +out: > + mutex_unlock(&rproc->lock); > + return ret; > +} > +#endif /* CONFIG_PM */ > + > static const char * const ipu_mem_names[] = { > "l2ram", NULL > }; > @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct platform_device *pdev) > oproc->num_timers); > } > > + init_completion(&oproc->pm_comp); > + > + oproc->fck = devm_clk_get(&pdev->dev, 0); > + if (IS_ERR(oproc->fck)) { > + ret = PTR_ERR(oproc->fck); > + goto free_rproc; > + } > + oproc->fck is not released if an error occurs in of_reserved_mem_device_init() and rproc_add(). > ret = of_reserved_mem_device_init(&pdev->dev); > if (ret) { > dev_err(&pdev->dev, "device does not have specific CMA pool\n"); > @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct platform_device *pdev) > return 0; > } > > +static const struct dev_pm_ops omap_rproc_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) > +}; > + > static struct platform_driver omap_rproc_driver = { > .probe = omap_rproc_probe, > .remove = omap_rproc_remove, > .driver = { > .name = "omap-rproc", > + .pm = &omap_rproc_pm_ops, > .of_match_table = omap_rproc_of_match, > }, > }; > diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h > index 72f656c93caa..c73383e707c7 100644 > --- a/drivers/remoteproc/omap_remoteproc.h > +++ b/drivers/remoteproc/omap_remoteproc.h > @@ -1,7 +1,7 @@ > /* > * Remote processor messaging > * > - * Copyright (C) 2011 Texas Instruments, Inc. > + * Copyright (C) 2011-2018 Texas Instruments, Inc. > * Copyright (C) 2011 Google, Inc. > * All rights reserved. > * > @@ -57,6 +57,16 @@ > * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the > * recovery mechanism (to some extent). > * > + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote processor > + * > + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote processor > + * > + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor for a > + * suspend request > + * > + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor > + * on a suspend request > + * > * Introduce new message definitions if any here. > * > * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core > @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { > RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, > RP_MBOX_ECHO_REPLY = 0xFFFFFF04, > RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, > - RP_MBOX_END_MSG = 0xFFFFFF06, > + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, > + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, > + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, > + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, > + RP_MBOX_END_MSG = 0xFFFFFF14, > }; > > #endif /* _OMAP_RPMSG_H */ > -- > 2.17.1 > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Mathieu, Tero, On 12/19/19 3:46 PM, Mathieu Poirier wrote: > On Fri, Dec 13, 2019 at 02:55:34PM +0200, Tero Kristo wrote: >> From: Suman Anna <s-anna@ti.com> >> >> This patch adds the support for system suspend/resume to the >> OMAP remoteproc driver so that the OMAP remoteproc devices can >> be suspended/resumed during a system suspend/resume. The support >> is added through the driver PM .suspend/.resume callbacks, and >> requires appropriate support from the OS running on the remote >> processors. >> >> The IPU & DSP remote processors typically have their own private >> modules like registers, internal memories, caches etc. The context >> of these modules need to be saved and restored properly for a >> suspend/resume to work. These are in general not accessible from >> the MPU, so the remote processors themselves have to implement >> the logic for the context save & restore of these modules. >> >> The OMAP remoteproc driver initiates a suspend by sending a mailbox >> message requesting the remote processor to save its context and >> enter into an idle/standby state. The remote processor should >> usually stop whatever processing it is doing to switch to a context >> save mode. The OMAP remoteproc driver detects the completion of >> the context save by checking the module standby status for the >> remoteproc device. It also stops any resources used by the remote >> processors like the timers. The timers need to be running only >> when the processor is active and executing, and need to be stopped >> otherwise to allow the timer driver to reach low-power states. The >> IOMMUs are automatically suspended by the PM core during the late >> suspend stage, after the remoteproc suspend process is completed by >> putting the remote processor cores into reset. Thereafter, the Linux >> kernel can put the domain into further lower power states as possible. >> >> The resume sequence undoes the operations performed in the PM suspend >> callback, by starting the timers and finally releasing the processors >> from reset. This requires that the remote processor side OS be able to >> distinguish a power-resume boot from a power-on/cold boot, restore the >> context of its private modules saved during the suspend phase, and >> resume executing code from where it was suspended. The IOMMUs would >> have been resumed by the PM core during early resume, so they are >> already enabled by the time remoteproc resume callback gets invoked. >> >> The remote processors should save their context into System RAM (DDR), >> as any internal memories are not guaranteed to retain context as it >> depends on the lowest power domain that the remote processor device >> is put into. The management of the DDR contents will be managed by >> the Linux kernel. >> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> [t-kristo@ti.com: converted to use ti-sysc instead of hwmod] >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/remoteproc/omap_remoteproc.c | 179 +++++++++++++++++++++++++++ >> drivers/remoteproc/omap_remoteproc.h | 18 ++- >> 2 files changed, 195 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >> index 9c750c2ab29d..0a9b9f7d20da 100644 >> --- a/drivers/remoteproc/omap_remoteproc.c >> +++ b/drivers/remoteproc/omap_remoteproc.c >> @@ -16,6 +16,7 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/err.h> >> +#include <linux/io.h> >> #include <linux/of_device.h> >> #include <linux/of_reserved_mem.h> >> #include <linux/platform_device.h> >> @@ -23,10 +24,13 @@ >> #include <linux/remoteproc.h> >> #include <linux/mailbox_client.h> >> #include <linux/omap-mailbox.h> >> +#include <linux/omap-iommu.h> > > Please move this up by one line. > >> #include <linux/regmap.h> >> #include <linux/mfd/syscon.h> >> #include <linux/reset.h> >> #include <clocksource/timer-ti-dm.h> >> +#include <linux/clk.h> >> +#include <linux/clk/ti.h> > > Unless there is a dependency with timer-ti-dm.h, these should go just above linux/err.h No depencencies, can be reordered. > >> >> #include <linux/platform_data/dmtimer-omap.h> >> >> @@ -81,6 +85,9 @@ struct omap_rproc_timer { >> * @timers: timer(s) info used by rproc >> * @rproc: rproc handle >> * @reset: reset handle >> + * @pm_comp: completion primitive to sync for suspend response >> + * @fck: functional clock for the remoteproc >> + * @suspend_acked: state machine flag to store the suspend request ack >> */ >> struct omap_rproc { >> struct mbox_chan *mbox; >> @@ -92,6 +99,9 @@ struct omap_rproc { >> struct omap_rproc_timer *timers; >> struct rproc *rproc; >> struct reset_control *reset; >> + struct completion pm_comp; >> + struct clk *fck; >> + bool suspend_acked; >> }; >> >> /** >> @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data) >> case RP_MBOX_ECHO_REPLY: >> dev_info(dev, "received echo reply from %s\n", name); >> break; >> + case RP_MBOX_SUSPEND_ACK: > > We can't get away with implicit fallthroughs anymore - please add /* Fall through */" > >> + case RP_MBOX_SUSPEND_CANCEL: >> + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; >> + complete(&oproc->pm_comp); >> + break; >> default: >> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) >> return; >> @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { >> .da_to_va = omap_rproc_da_to_va, >> }; >> >> +#ifdef CONFIG_PM >> +static bool _is_rproc_in_standby(struct omap_rproc *oproc) >> +{ >> + return ti_clk_is_in_standby(oproc->fck); >> +} >> + >> +/* 1 sec is long enough time to let the remoteproc side suspend the device */ >> +#define DEF_SUSPEND_TIMEOUT 1000 >> +static int _omap_rproc_suspend(struct rproc *rproc) >> +{ >> + struct device *dev = rproc->dev.parent; >> + struct omap_rproc *oproc = rproc->priv; >> + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); >> + unsigned long ta = jiffies + to; >> + int ret; >> + >> + reinit_completion(&oproc->pm_comp); >> + oproc->suspend_acked = false; >> + ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); >> + if (ret < 0) { >> + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >> + return ret; >> + } >> + >> + ret = wait_for_completion_timeout(&oproc->pm_comp, to); >> + if (!oproc->suspend_acked) >> + return -EBUSY; >> + >> + /* >> + * The remoteproc side is returning the ACK message before saving the >> + * context, because the context saving is performed within a SYS/BIOS >> + * function, and it cannot have any inter-dependencies against the IPC >> + * layer. Also, as the SYS/BIOS needs to preserve properly the processor >> + * register set, sending this ACK or signalling the completion of the >> + * context save through a shared memory variable can never be the >> + * absolute last thing to be executed on the remoteproc side, and the >> + * MPU cannot use the ACK message as a sync point to put the remoteproc >> + * into reset. The only way to ensure that the remote processor has >> + * completed saving the context is to check that the module has reached >> + * STANDBY state (after saving the context, the SYS/BIOS executes the >> + * appropriate target-specific WFI instruction causing the module to >> + * enter STANDBY). >> + */ >> + while (!_is_rproc_in_standby(oproc)) { >> + if (time_after(jiffies, ta)) >> + return -ETIME; >> + schedule(); >> + } >> + >> + reset_control_assert(oproc->reset); >> + >> + ret = omap_rproc_disable_timers(rproc, false); >> + if (ret) { >> + dev_err(dev, "disabling timers during suspend failed %d\n", >> + ret); >> + goto enable_device; >> + } >> + >> + return 0; >> + >> +enable_device: >> + reset_control_deassert(oproc->reset); >> + return ret; >> +} >> + >> +static int _omap_rproc_resume(struct rproc *rproc) >> +{ >> + struct device *dev = rproc->dev.parent; >> + struct omap_rproc *oproc = rproc->priv; >> + int ret; >> + >> + /* boot address could be lost after suspend, so restore it */ >> + if (oproc->boot_data) { >> + ret = omap_rproc_write_dsp_boot_addr(rproc); >> + if (ret) { >> + dev_err(dev, "boot address restore failed %d\n", ret); >> + goto out; >> + } >> + } >> + >> + ret = omap_rproc_enable_timers(rproc, false); >> + if (ret) { >> + dev_err(dev, "enabling timers during resume failed %d\n", >> + ret); > > The "ret);" fits on the live above. > >> + goto out; >> + } >> + >> + reset_control_deassert(oproc->reset); >> + >> +out: >> + return ret; >> +} >> + >> +static int __maybe_unused omap_rproc_suspend(struct device *dev) > > The "__maybe_unused" can be dropped because this is within the #ifdef CONFIG_PM > block. These are suspend/resume callbacks, so are actually controlled by CONFIG_PM_SLEEP which can be disabled independently from runtime suspend, and hence the annotation. > >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct rproc *rproc = platform_get_drvdata(pdev); >> + int ret = 0; >> + >> + mutex_lock(&rproc->lock); >> + if (rproc->state == RPROC_OFFLINE) >> + goto out; >> + >> + if (rproc->state == RPROC_SUSPENDED) >> + goto out; >> + >> + if (rproc->state != RPROC_RUNNING) { >> + ret = -EBUSY; >> + goto out; >> + } >> + >> + ret = _omap_rproc_suspend(rproc); >> + if (ret) { >> + dev_err(dev, "suspend failed %d\n", ret); >> + goto out; >> + } >> + >> + rproc->state = RPROC_SUSPENDED; >> +out: >> + mutex_unlock(&rproc->lock); >> + return ret; >> +} >> + >> +static int __maybe_unused omap_rproc_resume(struct device *dev) > > Same comment as above. > >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct rproc *rproc = platform_get_drvdata(pdev); >> + int ret = 0; >> + >> + mutex_lock(&rproc->lock); >> + if (rproc->state == RPROC_OFFLINE) >> + goto out; >> + >> + if (rproc->state != RPROC_SUSPENDED) { >> + ret = -EBUSY; >> + goto out; >> + } >> + >> + ret = _omap_rproc_resume(rproc); >> + if (ret) { >> + dev_err(dev, "resume failed %d\n", ret); >> + goto out; >> + } >> + >> + rproc->state = RPROC_RUNNING; >> +out: >> + mutex_unlock(&rproc->lock); >> + return ret; >> +} >> +#endif /* CONFIG_PM */ >> + >> static const char * const ipu_mem_names[] = { >> "l2ram", NULL >> }; >> @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct platform_device *pdev) >> oproc->num_timers); >> } >> >> + init_completion(&oproc->pm_comp); >> + >> + oproc->fck = devm_clk_get(&pdev->dev, 0); >> + if (IS_ERR(oproc->fck)) { >> + ret = PTR_ERR(oproc->fck); >> + goto free_rproc; >> + } >> + > > oproc->fck is not released if an error occurs in of_reserved_mem_device_init() > and rproc_add(). We are using a devres managed API, so why do we need to release it specifically? > >> ret = of_reserved_mem_device_init(&pdev->dev); >> if (ret) { >> dev_err(&pdev->dev, "device does not have specific CMA pool\n"); >> @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static const struct dev_pm_ops omap_rproc_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >> +}; >> + >> static struct platform_driver omap_rproc_driver = { >> .probe = omap_rproc_probe, >> .remove = omap_rproc_remove, >> .driver = { >> .name = "omap-rproc", >> + .pm = &omap_rproc_pm_ops, >> .of_match_table = omap_rproc_of_match, >> }, >> }; >> diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h >> index 72f656c93caa..c73383e707c7 100644 >> --- a/drivers/remoteproc/omap_remoteproc.h >> +++ b/drivers/remoteproc/omap_remoteproc.h >> @@ -1,7 +1,7 @@ >> /* >> * Remote processor messaging >> * >> - * Copyright (C) 2011 Texas Instruments, Inc. >> + * Copyright (C) 2011-2018 Texas Instruments, Inc. %s/2018/2019/ regards Suman >> * Copyright (C) 2011 Google, Inc. >> * All rights reserved. >> * >> @@ -57,6 +57,16 @@ >> * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the >> * recovery mechanism (to some extent). >> * >> + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote processor >> + * >> + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote processor >> + * >> + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor for a >> + * suspend request >> + * >> + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor >> + * on a suspend request >> + * >> * Introduce new message definitions if any here. >> * >> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core >> @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { >> RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, >> RP_MBOX_ECHO_REPLY = 0xFFFFFF04, >> RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, >> - RP_MBOX_END_MSG = 0xFFFFFF06, >> + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, >> + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, >> + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, >> + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, >> + RP_MBOX_END_MSG = 0xFFFFFF14, >> }; >> >> #endif /* _OMAP_RPMSG_H */ >> -- >> 2.17.1 >> >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 20/12/2019 05:11, Suman Anna wrote: > Hi Mathieu, Tero, > > On 12/19/19 3:46 PM, Mathieu Poirier wrote: >> On Fri, Dec 13, 2019 at 02:55:34PM +0200, Tero Kristo wrote: >>> From: Suman Anna <s-anna@ti.com> >>> >>> This patch adds the support for system suspend/resume to the >>> OMAP remoteproc driver so that the OMAP remoteproc devices can >>> be suspended/resumed during a system suspend/resume. The support >>> is added through the driver PM .suspend/.resume callbacks, and >>> requires appropriate support from the OS running on the remote >>> processors. >>> >>> The IPU & DSP remote processors typically have their own private >>> modules like registers, internal memories, caches etc. The context >>> of these modules need to be saved and restored properly for a >>> suspend/resume to work. These are in general not accessible from >>> the MPU, so the remote processors themselves have to implement >>> the logic for the context save & restore of these modules. >>> >>> The OMAP remoteproc driver initiates a suspend by sending a mailbox >>> message requesting the remote processor to save its context and >>> enter into an idle/standby state. The remote processor should >>> usually stop whatever processing it is doing to switch to a context >>> save mode. The OMAP remoteproc driver detects the completion of >>> the context save by checking the module standby status for the >>> remoteproc device. It also stops any resources used by the remote >>> processors like the timers. The timers need to be running only >>> when the processor is active and executing, and need to be stopped >>> otherwise to allow the timer driver to reach low-power states. The >>> IOMMUs are automatically suspended by the PM core during the late >>> suspend stage, after the remoteproc suspend process is completed by >>> putting the remote processor cores into reset. Thereafter, the Linux >>> kernel can put the domain into further lower power states as possible. >>> >>> The resume sequence undoes the operations performed in the PM suspend >>> callback, by starting the timers and finally releasing the processors >>> from reset. This requires that the remote processor side OS be able to >>> distinguish a power-resume boot from a power-on/cold boot, restore the >>> context of its private modules saved during the suspend phase, and >>> resume executing code from where it was suspended. The IOMMUs would >>> have been resumed by the PM core during early resume, so they are >>> already enabled by the time remoteproc resume callback gets invoked. >>> >>> The remote processors should save their context into System RAM (DDR), >>> as any internal memories are not guaranteed to retain context as it >>> depends on the lowest power domain that the remote processor device >>> is put into. The management of the DDR contents will be managed by >>> the Linux kernel. >>> >>> Signed-off-by: Suman Anna <s-anna@ti.com> >>> [t-kristo@ti.com: converted to use ti-sysc instead of hwmod] >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> drivers/remoteproc/omap_remoteproc.c | 179 +++++++++++++++++++++++++++ >>> drivers/remoteproc/omap_remoteproc.h | 18 ++- >>> 2 files changed, 195 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >>> index 9c750c2ab29d..0a9b9f7d20da 100644 >>> --- a/drivers/remoteproc/omap_remoteproc.c >>> +++ b/drivers/remoteproc/omap_remoteproc.c >>> @@ -16,6 +16,7 @@ >>> #include <linux/kernel.h> >>> #include <linux/module.h> >>> #include <linux/err.h> >>> +#include <linux/io.h> >>> #include <linux/of_device.h> >>> #include <linux/of_reserved_mem.h> >>> #include <linux/platform_device.h> >>> @@ -23,10 +24,13 @@ >>> #include <linux/remoteproc.h> >>> #include <linux/mailbox_client.h> >>> #include <linux/omap-mailbox.h> >>> +#include <linux/omap-iommu.h> >> >> Please move this up by one line. >> >>> #include <linux/regmap.h> >>> #include <linux/mfd/syscon.h> >>> #include <linux/reset.h> >>> #include <clocksource/timer-ti-dm.h> >>> +#include <linux/clk.h> >>> +#include <linux/clk/ti.h> >> >> Unless there is a dependency with timer-ti-dm.h, these should go just above linux/err.h > > No depencencies, can be reordered. Will fix these. > >> >>> >>> #include <linux/platform_data/dmtimer-omap.h> >>> >>> @@ -81,6 +85,9 @@ struct omap_rproc_timer { >>> * @timers: timer(s) info used by rproc >>> * @rproc: rproc handle >>> * @reset: reset handle >>> + * @pm_comp: completion primitive to sync for suspend response >>> + * @fck: functional clock for the remoteproc >>> + * @suspend_acked: state machine flag to store the suspend request ack >>> */ >>> struct omap_rproc { >>> struct mbox_chan *mbox; >>> @@ -92,6 +99,9 @@ struct omap_rproc { >>> struct omap_rproc_timer *timers; >>> struct rproc *rproc; >>> struct reset_control *reset; >>> + struct completion pm_comp; >>> + struct clk *fck; >>> + bool suspend_acked; >>> }; >>> >>> /** >>> @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data) >>> case RP_MBOX_ECHO_REPLY: >>> dev_info(dev, "received echo reply from %s\n", name); >>> break; >>> + case RP_MBOX_SUSPEND_ACK: >> >> We can't get away with implicit fallthroughs anymore - please add /* Fall through */" Ok, will do. >> >>> + case RP_MBOX_SUSPEND_CANCEL: >>> + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; >>> + complete(&oproc->pm_comp); >>> + break; >>> default: >>> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) >>> return; >>> @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { >>> .da_to_va = omap_rproc_da_to_va, >>> }; >>> >>> +#ifdef CONFIG_PM >>> +static bool _is_rproc_in_standby(struct omap_rproc *oproc) >>> +{ >>> + return ti_clk_is_in_standby(oproc->fck); >>> +} >>> + >>> +/* 1 sec is long enough time to let the remoteproc side suspend the device */ >>> +#define DEF_SUSPEND_TIMEOUT 1000 >>> +static int _omap_rproc_suspend(struct rproc *rproc) >>> +{ >>> + struct device *dev = rproc->dev.parent; >>> + struct omap_rproc *oproc = rproc->priv; >>> + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); >>> + unsigned long ta = jiffies + to; >>> + int ret; >>> + >>> + reinit_completion(&oproc->pm_comp); >>> + oproc->suspend_acked = false; >>> + ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); >>> + if (ret < 0) { >>> + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = wait_for_completion_timeout(&oproc->pm_comp, to); >>> + if (!oproc->suspend_acked) >>> + return -EBUSY; >>> + >>> + /* >>> + * The remoteproc side is returning the ACK message before saving the >>> + * context, because the context saving is performed within a SYS/BIOS >>> + * function, and it cannot have any inter-dependencies against the IPC >>> + * layer. Also, as the SYS/BIOS needs to preserve properly the processor >>> + * register set, sending this ACK or signalling the completion of the >>> + * context save through a shared memory variable can never be the >>> + * absolute last thing to be executed on the remoteproc side, and the >>> + * MPU cannot use the ACK message as a sync point to put the remoteproc >>> + * into reset. The only way to ensure that the remote processor has >>> + * completed saving the context is to check that the module has reached >>> + * STANDBY state (after saving the context, the SYS/BIOS executes the >>> + * appropriate target-specific WFI instruction causing the module to >>> + * enter STANDBY). >>> + */ >>> + while (!_is_rproc_in_standby(oproc)) { >>> + if (time_after(jiffies, ta)) >>> + return -ETIME; >>> + schedule(); >>> + } >>> + >>> + reset_control_assert(oproc->reset); >>> + >>> + ret = omap_rproc_disable_timers(rproc, false); >>> + if (ret) { >>> + dev_err(dev, "disabling timers during suspend failed %d\n", >>> + ret); >>> + goto enable_device; >>> + } >>> + >>> + return 0; >>> + >>> +enable_device: >>> + reset_control_deassert(oproc->reset); >>> + return ret; >>> +} >>> + >>> +static int _omap_rproc_resume(struct rproc *rproc) >>> +{ >>> + struct device *dev = rproc->dev.parent; >>> + struct omap_rproc *oproc = rproc->priv; >>> + int ret; >>> + >>> + /* boot address could be lost after suspend, so restore it */ >>> + if (oproc->boot_data) { >>> + ret = omap_rproc_write_dsp_boot_addr(rproc); >>> + if (ret) { >>> + dev_err(dev, "boot address restore failed %d\n", ret); >>> + goto out; >>> + } >>> + } >>> + >>> + ret = omap_rproc_enable_timers(rproc, false); >>> + if (ret) { >>> + dev_err(dev, "enabling timers during resume failed %d\n", >>> + ret); >> >> The "ret);" fits on the live above. Ok, will fix. >> >>> + goto out; >>> + } >>> + >>> + reset_control_deassert(oproc->reset); >>> + >>> +out: >>> + return ret; >>> +} >>> + >>> +static int __maybe_unused omap_rproc_suspend(struct device *dev) >> >> The "__maybe_unused" can be dropped because this is within the #ifdef CONFIG_PM >> block. > > These are suspend/resume callbacks, so are actually controlled by > CONFIG_PM_SLEEP which can be disabled independently from runtime > suspend, and hence the annotation. I'll add these two behind CONFIG_PM_SLEEP, in addition to the whole lot being behind CONFIG_PM. > >> >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct rproc *rproc = platform_get_drvdata(pdev); >>> + int ret = 0; >>> + >>> + mutex_lock(&rproc->lock); >>> + if (rproc->state == RPROC_OFFLINE) >>> + goto out; >>> + >>> + if (rproc->state == RPROC_SUSPENDED) >>> + goto out; >>> + >>> + if (rproc->state != RPROC_RUNNING) { >>> + ret = -EBUSY; >>> + goto out; >>> + } >>> + >>> + ret = _omap_rproc_suspend(rproc); >>> + if (ret) { >>> + dev_err(dev, "suspend failed %d\n", ret); >>> + goto out; >>> + } >>> + >>> + rproc->state = RPROC_SUSPENDED; >>> +out: >>> + mutex_unlock(&rproc->lock); >>> + return ret; >>> +} >>> + >>> +static int __maybe_unused omap_rproc_resume(struct device *dev) >> >> Same comment as above. >> >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct rproc *rproc = platform_get_drvdata(pdev); >>> + int ret = 0; >>> + >>> + mutex_lock(&rproc->lock); >>> + if (rproc->state == RPROC_OFFLINE) >>> + goto out; >>> + >>> + if (rproc->state != RPROC_SUSPENDED) { >>> + ret = -EBUSY; >>> + goto out; >>> + } >>> + >>> + ret = _omap_rproc_resume(rproc); >>> + if (ret) { >>> + dev_err(dev, "resume failed %d\n", ret); >>> + goto out; >>> + } >>> + >>> + rproc->state = RPROC_RUNNING; >>> +out: >>> + mutex_unlock(&rproc->lock); >>> + return ret; >>> +} >>> +#endif /* CONFIG_PM */ >>> + >>> static const char * const ipu_mem_names[] = { >>> "l2ram", NULL >>> }; >>> @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct platform_device *pdev) >>> oproc->num_timers); >>> } >>> >>> + init_completion(&oproc->pm_comp); >>> + >>> + oproc->fck = devm_clk_get(&pdev->dev, 0); >>> + if (IS_ERR(oproc->fck)) { >>> + ret = PTR_ERR(oproc->fck); >>> + goto free_rproc; >>> + } >>> + >> >> oproc->fck is not released if an error occurs in of_reserved_mem_device_init() >> and rproc_add(). > > We are using a devres managed API, so why do we need to release it > specifically? Yea I don't think this is needed. > >> >>> ret = of_reserved_mem_device_init(&pdev->dev); >>> if (ret) { >>> dev_err(&pdev->dev, "device does not have specific CMA pool\n"); >>> @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct platform_device *pdev) >>> return 0; >>> } >>> >>> +static const struct dev_pm_ops omap_rproc_pm_ops = { >>> + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >>> +}; >>> + >>> static struct platform_driver omap_rproc_driver = { >>> .probe = omap_rproc_probe, >>> .remove = omap_rproc_remove, >>> .driver = { >>> .name = "omap-rproc", >>> + .pm = &omap_rproc_pm_ops, >>> .of_match_table = omap_rproc_of_match, >>> }, >>> }; >>> diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h >>> index 72f656c93caa..c73383e707c7 100644 >>> --- a/drivers/remoteproc/omap_remoteproc.h >>> +++ b/drivers/remoteproc/omap_remoteproc.h >>> @@ -1,7 +1,7 @@ >>> /* >>> * Remote processor messaging >>> * >>> - * Copyright (C) 2011 Texas Instruments, Inc. >>> + * Copyright (C) 2011-2018 Texas Instruments, Inc. > > %s/2018/2019/ Yep, will fix. -Tero > > regards > Suman > >>> * Copyright (C) 2011 Google, Inc. >>> * All rights reserved. >>> * >>> @@ -57,6 +57,16 @@ >>> * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the >>> * recovery mechanism (to some extent). >>> * >>> + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote processor >>> + * >>> + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote processor >>> + * >>> + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor for a >>> + * suspend request >>> + * >>> + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor >>> + * on a suspend request >>> + * >>> * Introduce new message definitions if any here. >>> * >>> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core >>> @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { >>> RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, >>> RP_MBOX_ECHO_REPLY = 0xFFFFFF04, >>> RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, >>> - RP_MBOX_END_MSG = 0xFFFFFF06, >>> + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, >>> + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, >>> + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, >>> + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, >>> + RP_MBOX_END_MSG = 0xFFFFFF14, >>> }; >>> >>> #endif /* _OMAP_RPMSG_H */ >>> -- >>> 2.17.1 >>> >>> -- > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 12/20/19 5:04 AM, Tero Kristo wrote: > On 20/12/2019 05:11, Suman Anna wrote: >> Hi Mathieu, Tero, >> >> On 12/19/19 3:46 PM, Mathieu Poirier wrote: >>> On Fri, Dec 13, 2019 at 02:55:34PM +0200, Tero Kristo wrote: >>>> From: Suman Anna <s-anna@ti.com> >>>> >>>> This patch adds the support for system suspend/resume to the >>>> OMAP remoteproc driver so that the OMAP remoteproc devices can >>>> be suspended/resumed during a system suspend/resume. The support >>>> is added through the driver PM .suspend/.resume callbacks, and >>>> requires appropriate support from the OS running on the remote >>>> processors. >>>> >>>> The IPU & DSP remote processors typically have their own private >>>> modules like registers, internal memories, caches etc. The context >>>> of these modules need to be saved and restored properly for a >>>> suspend/resume to work. These are in general not accessible from >>>> the MPU, so the remote processors themselves have to implement >>>> the logic for the context save & restore of these modules. >>>> >>>> The OMAP remoteproc driver initiates a suspend by sending a mailbox >>>> message requesting the remote processor to save its context and >>>> enter into an idle/standby state. The remote processor should >>>> usually stop whatever processing it is doing to switch to a context >>>> save mode. The OMAP remoteproc driver detects the completion of >>>> the context save by checking the module standby status for the >>>> remoteproc device. It also stops any resources used by the remote >>>> processors like the timers. The timers need to be running only >>>> when the processor is active and executing, and need to be stopped >>>> otherwise to allow the timer driver to reach low-power states. The >>>> IOMMUs are automatically suspended by the PM core during the late >>>> suspend stage, after the remoteproc suspend process is completed by >>>> putting the remote processor cores into reset. Thereafter, the Linux >>>> kernel can put the domain into further lower power states as possible. >>>> >>>> The resume sequence undoes the operations performed in the PM suspend >>>> callback, by starting the timers and finally releasing the processors >>>> from reset. This requires that the remote processor side OS be able to >>>> distinguish a power-resume boot from a power-on/cold boot, restore the >>>> context of its private modules saved during the suspend phase, and >>>> resume executing code from where it was suspended. The IOMMUs would >>>> have been resumed by the PM core during early resume, so they are >>>> already enabled by the time remoteproc resume callback gets invoked. >>>> >>>> The remote processors should save their context into System RAM (DDR), >>>> as any internal memories are not guaranteed to retain context as it >>>> depends on the lowest power domain that the remote processor device >>>> is put into. The management of the DDR contents will be managed by >>>> the Linux kernel. >>>> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>> [t-kristo@ti.com: converted to use ti-sysc instead of hwmod] >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/remoteproc/omap_remoteproc.c | 179 >>>> +++++++++++++++++++++++++++ >>>> drivers/remoteproc/omap_remoteproc.h | 18 ++- >>>> 2 files changed, 195 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/remoteproc/omap_remoteproc.c >>>> b/drivers/remoteproc/omap_remoteproc.c >>>> index 9c750c2ab29d..0a9b9f7d20da 100644 >>>> --- a/drivers/remoteproc/omap_remoteproc.c >>>> +++ b/drivers/remoteproc/omap_remoteproc.c >>>> @@ -16,6 +16,7 @@ >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> #include <linux/err.h> >>>> +#include <linux/io.h> >>>> #include <linux/of_device.h> >>>> #include <linux/of_reserved_mem.h> >>>> #include <linux/platform_device.h> >>>> @@ -23,10 +24,13 @@ >>>> #include <linux/remoteproc.h> >>>> #include <linux/mailbox_client.h> >>>> #include <linux/omap-mailbox.h> >>>> +#include <linux/omap-iommu.h> >>> >>> Please move this up by one line. >>> >>>> #include <linux/regmap.h> >>>> #include <linux/mfd/syscon.h> >>>> #include <linux/reset.h> >>>> #include <clocksource/timer-ti-dm.h> >>>> +#include <linux/clk.h> >>>> +#include <linux/clk/ti.h> >>> >>> Unless there is a dependency with timer-ti-dm.h, these should go just >>> above linux/err.h >> >> No depencencies, can be reordered. > > Will fix these. > >> >>> >>>> #include <linux/platform_data/dmtimer-omap.h> >>>> @@ -81,6 +85,9 @@ struct omap_rproc_timer { >>>> * @timers: timer(s) info used by rproc >>>> * @rproc: rproc handle >>>> * @reset: reset handle >>>> + * @pm_comp: completion primitive to sync for suspend response >>>> + * @fck: functional clock for the remoteproc >>>> + * @suspend_acked: state machine flag to store the suspend request ack >>>> */ >>>> struct omap_rproc { >>>> struct mbox_chan *mbox; >>>> @@ -92,6 +99,9 @@ struct omap_rproc { >>>> struct omap_rproc_timer *timers; >>>> struct rproc *rproc; >>>> struct reset_control *reset; >>>> + struct completion pm_comp; >>>> + struct clk *fck; >>>> + bool suspend_acked; >>>> }; >>>> /** >>>> @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct >>>> mbox_client *client, void *data) >>>> case RP_MBOX_ECHO_REPLY: >>>> dev_info(dev, "received echo reply from %s\n", name); >>>> break; >>>> + case RP_MBOX_SUSPEND_ACK: >>> >>> We can't get away with implicit fallthroughs anymore - please add /* >>> Fall through */" > > Ok, will do. > >>> >>>> + case RP_MBOX_SUSPEND_CANCEL: >>>> + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; >>>> + complete(&oproc->pm_comp); >>>> + break; >>>> default: >>>> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) >>>> return; >>>> @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { >>>> .da_to_va = omap_rproc_da_to_va, >>>> }; >>>> +#ifdef CONFIG_PM >>>> +static bool _is_rproc_in_standby(struct omap_rproc *oproc) >>>> +{ >>>> + return ti_clk_is_in_standby(oproc->fck); >>>> +} >>>> + >>>> +/* 1 sec is long enough time to let the remoteproc side suspend the >>>> device */ >>>> +#define DEF_SUSPEND_TIMEOUT 1000 >>>> +static int _omap_rproc_suspend(struct rproc *rproc) >>>> +{ >>>> + struct device *dev = rproc->dev.parent; >>>> + struct omap_rproc *oproc = rproc->priv; >>>> + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); >>>> + unsigned long ta = jiffies + to; >>>> + int ret; >>>> + >>>> + reinit_completion(&oproc->pm_comp); >>>> + oproc->suspend_acked = false; >>>> + ret = mbox_send_message(oproc->mbox, (void >>>> *)RP_MBOX_SUSPEND_SYSTEM); >>>> + if (ret < 0) { >>>> + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = wait_for_completion_timeout(&oproc->pm_comp, to); >>>> + if (!oproc->suspend_acked) >>>> + return -EBUSY; >>>> + >>>> + /* >>>> + * The remoteproc side is returning the ACK message before >>>> saving the >>>> + * context, because the context saving is performed within a >>>> SYS/BIOS >>>> + * function, and it cannot have any inter-dependencies against >>>> the IPC >>>> + * layer. Also, as the SYS/BIOS needs to preserve properly the >>>> processor >>>> + * register set, sending this ACK or signalling the completion >>>> of the >>>> + * context save through a shared memory variable can never be the >>>> + * absolute last thing to be executed on the remoteproc side, >>>> and the >>>> + * MPU cannot use the ACK message as a sync point to put the >>>> remoteproc >>>> + * into reset. The only way to ensure that the remote processor >>>> has >>>> + * completed saving the context is to check that the module has >>>> reached >>>> + * STANDBY state (after saving the context, the SYS/BIOS >>>> executes the >>>> + * appropriate target-specific WFI instruction causing the >>>> module to >>>> + * enter STANDBY). >>>> + */ >>>> + while (!_is_rproc_in_standby(oproc)) { >>>> + if (time_after(jiffies, ta)) >>>> + return -ETIME; >>>> + schedule(); >>>> + } >>>> + >>>> + reset_control_assert(oproc->reset); >>>> + >>>> + ret = omap_rproc_disable_timers(rproc, false); >>>> + if (ret) { >>>> + dev_err(dev, "disabling timers during suspend failed %d\n", >>>> + ret); >>>> + goto enable_device; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> +enable_device: >>>> + reset_control_deassert(oproc->reset); >>>> + return ret; >>>> +} >>>> + >>>> +static int _omap_rproc_resume(struct rproc *rproc) >>>> +{ >>>> + struct device *dev = rproc->dev.parent; >>>> + struct omap_rproc *oproc = rproc->priv; >>>> + int ret; >>>> + >>>> + /* boot address could be lost after suspend, so restore it */ >>>> + if (oproc->boot_data) { >>>> + ret = omap_rproc_write_dsp_boot_addr(rproc); >>>> + if (ret) { >>>> + dev_err(dev, "boot address restore failed %d\n", ret); >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> + ret = omap_rproc_enable_timers(rproc, false); >>>> + if (ret) { >>>> + dev_err(dev, "enabling timers during resume failed %d\n", >>>> + ret); >>> >>> The "ret);" fits on the live above. > > Ok, will fix. > >>> >>>> + goto out; >>>> + } >>>> + >>>> + reset_control_deassert(oproc->reset); >>>> + >>>> +out: >>>> + return ret; >>>> +} >>>> + >>>> +static int __maybe_unused omap_rproc_suspend(struct device *dev) >>> >>> The "__maybe_unused" can be dropped because this is within the #ifdef >>> CONFIG_PM >>> block. >> >> These are suspend/resume callbacks, so are actually controlled by >> CONFIG_PM_SLEEP which can be disabled independently from runtime >> suspend, and hence the annotation. > > I'll add these two behind CONFIG_PM_SLEEP, in addition to the whole lot > being behind CONFIG_PM. AFAIK, the preferred method currently is to use the __may_unused annotation. The SET_SYSTEM_SLEEP_PM_OPS is under that ifdef and the compiler is smart enough to drop it when CONFIG_PM_SLEEP is not defined. So, no need to change this. regards Suman > >> >>> >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct rproc *rproc = platform_get_drvdata(pdev); >>>> + int ret = 0; >>>> + >>>> + mutex_lock(&rproc->lock); >>>> + if (rproc->state == RPROC_OFFLINE) >>>> + goto out; >>>> + >>>> + if (rproc->state == RPROC_SUSPENDED) >>>> + goto out; >>>> + >>>> + if (rproc->state != RPROC_RUNNING) { >>>> + ret = -EBUSY; >>>> + goto out; >>>> + } >>>> + >>>> + ret = _omap_rproc_suspend(rproc); >>>> + if (ret) { >>>> + dev_err(dev, "suspend failed %d\n", ret); >>>> + goto out; >>>> + } >>>> + >>>> + rproc->state = RPROC_SUSPENDED; >>>> +out: >>>> + mutex_unlock(&rproc->lock); >>>> + return ret; >>>> +} >>>> + >>>> +static int __maybe_unused omap_rproc_resume(struct device *dev) >>> >>> Same comment as above. >>> >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct rproc *rproc = platform_get_drvdata(pdev); >>>> + int ret = 0; >>>> + >>>> + mutex_lock(&rproc->lock); >>>> + if (rproc->state == RPROC_OFFLINE) >>>> + goto out; >>>> + >>>> + if (rproc->state != RPROC_SUSPENDED) { >>>> + ret = -EBUSY; >>>> + goto out; >>>> + } >>>> + >>>> + ret = _omap_rproc_resume(rproc); >>>> + if (ret) { >>>> + dev_err(dev, "resume failed %d\n", ret); >>>> + goto out; >>>> + } >>>> + >>>> + rproc->state = RPROC_RUNNING; >>>> +out: >>>> + mutex_unlock(&rproc->lock); >>>> + return ret; >>>> +} >>>> +#endif /* CONFIG_PM */ >>>> + >>>> static const char * const ipu_mem_names[] = { >>>> "l2ram", NULL >>>> }; >>>> @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct >>>> platform_device *pdev) >>>> oproc->num_timers); >>>> } >>>> + init_completion(&oproc->pm_comp); >>>> + >>>> + oproc->fck = devm_clk_get(&pdev->dev, 0); >>>> + if (IS_ERR(oproc->fck)) { >>>> + ret = PTR_ERR(oproc->fck); >>>> + goto free_rproc; >>>> + } >>>> + >>> >>> oproc->fck is not released if an error occurs in >>> of_reserved_mem_device_init() >>> and rproc_add(). >> >> We are using a devres managed API, so why do we need to release it >> specifically? > > Yea I don't think this is needed. > >> >>> >>>> ret = of_reserved_mem_device_init(&pdev->dev); >>>> if (ret) { >>>> dev_err(&pdev->dev, "device does not have specific CMA >>>> pool\n"); >>>> @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct >>>> platform_device *pdev) >>>> return 0; >>>> } >>>> +static const struct dev_pm_ops omap_rproc_pm_ops = { >>>> + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >>>> +}; >>>> + >>>> static struct platform_driver omap_rproc_driver = { >>>> .probe = omap_rproc_probe, >>>> .remove = omap_rproc_remove, >>>> .driver = { >>>> .name = "omap-rproc", >>>> + .pm = &omap_rproc_pm_ops, >>>> .of_match_table = omap_rproc_of_match, >>>> }, >>>> }; >>>> diff --git a/drivers/remoteproc/omap_remoteproc.h >>>> b/drivers/remoteproc/omap_remoteproc.h >>>> index 72f656c93caa..c73383e707c7 100644 >>>> --- a/drivers/remoteproc/omap_remoteproc.h >>>> +++ b/drivers/remoteproc/omap_remoteproc.h >>>> @@ -1,7 +1,7 @@ >>>> /* >>>> * Remote processor messaging >>>> * >>>> - * Copyright (C) 2011 Texas Instruments, Inc. >>>> + * Copyright (C) 2011-2018 Texas Instruments, Inc. >> >> %s/2018/2019/ > > Yep, will fix. > > -Tero > >> >> regards >> Suman >> >>>> * Copyright (C) 2011 Google, Inc. >>>> * All rights reserved. >>>> * >>>> @@ -57,6 +57,16 @@ >>>> * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for >>>> testing the >>>> * recovery mechanism (to some extent). >>>> * >>>> + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote >>>> processor >>>> + * >>>> + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote >>>> processor >>>> + * >>>> + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor >>>> for a >>>> + * suspend request >>>> + * >>>> + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote >>>> processor >>>> + * on a suspend request >>>> + * >>>> * Introduce new message definitions if any here. >>>> * >>>> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from >>>> remote core >>>> @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { >>>> RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, >>>> RP_MBOX_ECHO_REPLY = 0xFFFFFF04, >>>> RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, >>>> - RP_MBOX_END_MSG = 0xFFFFFF06, >>>> + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, >>>> + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, >>>> + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, >>>> + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, >>>> + RP_MBOX_END_MSG = 0xFFFFFF14, >>>> }; >>>> #endif /* _OMAP_RPMSG_H */ >>>> -- >>>> 2.17.1 >>>> >>>> -- >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Thu, 19 Dec 2019 at 20:11, Suman Anna <s-anna@ti.com> wrote: > > Hi Mathieu, Tero, > > On 12/19/19 3:46 PM, Mathieu Poirier wrote: > > On Fri, Dec 13, 2019 at 02:55:34PM +0200, Tero Kristo wrote: > >> From: Suman Anna <s-anna@ti.com> > >> > >> This patch adds the support for system suspend/resume to the > >> OMAP remoteproc driver so that the OMAP remoteproc devices can > >> be suspended/resumed during a system suspend/resume. The support > >> is added through the driver PM .suspend/.resume callbacks, and > >> requires appropriate support from the OS running on the remote > >> processors. > >> > >> The IPU & DSP remote processors typically have their own private > >> modules like registers, internal memories, caches etc. The context > >> of these modules need to be saved and restored properly for a > >> suspend/resume to work. These are in general not accessible from > >> the MPU, so the remote processors themselves have to implement > >> the logic for the context save & restore of these modules. > >> > >> The OMAP remoteproc driver initiates a suspend by sending a mailbox > >> message requesting the remote processor to save its context and > >> enter into an idle/standby state. The remote processor should > >> usually stop whatever processing it is doing to switch to a context > >> save mode. The OMAP remoteproc driver detects the completion of > >> the context save by checking the module standby status for the > >> remoteproc device. It also stops any resources used by the remote > >> processors like the timers. The timers need to be running only > >> when the processor is active and executing, and need to be stopped > >> otherwise to allow the timer driver to reach low-power states. The > >> IOMMUs are automatically suspended by the PM core during the late > >> suspend stage, after the remoteproc suspend process is completed by > >> putting the remote processor cores into reset. Thereafter, the Linux > >> kernel can put the domain into further lower power states as possible. > >> > >> The resume sequence undoes the operations performed in the PM suspend > >> callback, by starting the timers and finally releasing the processors > >> from reset. This requires that the remote processor side OS be able to > >> distinguish a power-resume boot from a power-on/cold boot, restore the > >> context of its private modules saved during the suspend phase, and > >> resume executing code from where it was suspended. The IOMMUs would > >> have been resumed by the PM core during early resume, so they are > >> already enabled by the time remoteproc resume callback gets invoked. > >> > >> The remote processors should save their context into System RAM (DDR), > >> as any internal memories are not guaranteed to retain context as it > >> depends on the lowest power domain that the remote processor device > >> is put into. The management of the DDR contents will be managed by > >> the Linux kernel. > >> > >> Signed-off-by: Suman Anna <s-anna@ti.com> > >> [t-kristo@ti.com: converted to use ti-sysc instead of hwmod] > >> Signed-off-by: Tero Kristo <t-kristo@ti.com> > >> --- > >> drivers/remoteproc/omap_remoteproc.c | 179 +++++++++++++++++++++++++++ > >> drivers/remoteproc/omap_remoteproc.h | 18 ++- > >> 2 files changed, 195 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > >> index 9c750c2ab29d..0a9b9f7d20da 100644 > >> --- a/drivers/remoteproc/omap_remoteproc.c > >> +++ b/drivers/remoteproc/omap_remoteproc.c > >> @@ -16,6 +16,7 @@ > >> #include <linux/kernel.h> > >> #include <linux/module.h> > >> #include <linux/err.h> > >> +#include <linux/io.h> > >> #include <linux/of_device.h> > >> #include <linux/of_reserved_mem.h> > >> #include <linux/platform_device.h> > >> @@ -23,10 +24,13 @@ > >> #include <linux/remoteproc.h> > >> #include <linux/mailbox_client.h> > >> #include <linux/omap-mailbox.h> > >> +#include <linux/omap-iommu.h> > > > > Please move this up by one line. > > > >> #include <linux/regmap.h> > >> #include <linux/mfd/syscon.h> > >> #include <linux/reset.h> > >> #include <clocksource/timer-ti-dm.h> > >> +#include <linux/clk.h> > >> +#include <linux/clk/ti.h> > > > > Unless there is a dependency with timer-ti-dm.h, these should go just above linux/err.h > > No depencencies, can be reordered. > > > > >> > >> #include <linux/platform_data/dmtimer-omap.h> > >> > >> @@ -81,6 +85,9 @@ struct omap_rproc_timer { > >> * @timers: timer(s) info used by rproc > >> * @rproc: rproc handle > >> * @reset: reset handle > >> + * @pm_comp: completion primitive to sync for suspend response > >> + * @fck: functional clock for the remoteproc > >> + * @suspend_acked: state machine flag to store the suspend request ack > >> */ > >> struct omap_rproc { > >> struct mbox_chan *mbox; > >> @@ -92,6 +99,9 @@ struct omap_rproc { > >> struct omap_rproc_timer *timers; > >> struct rproc *rproc; > >> struct reset_control *reset; > >> + struct completion pm_comp; > >> + struct clk *fck; > >> + bool suspend_acked; > >> }; > >> > >> /** > >> @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data) > >> case RP_MBOX_ECHO_REPLY: > >> dev_info(dev, "received echo reply from %s\n", name); > >> break; > >> + case RP_MBOX_SUSPEND_ACK: > > > > We can't get away with implicit fallthroughs anymore - please add /* Fall through */" > > > >> + case RP_MBOX_SUSPEND_CANCEL: > >> + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; > >> + complete(&oproc->pm_comp); > >> + break; > >> default: > >> if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) > >> return; > >> @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { > >> .da_to_va = omap_rproc_da_to_va, > >> }; > >> > >> +#ifdef CONFIG_PM > >> +static bool _is_rproc_in_standby(struct omap_rproc *oproc) > >> +{ > >> + return ti_clk_is_in_standby(oproc->fck); > >> +} > >> + > >> +/* 1 sec is long enough time to let the remoteproc side suspend the device */ > >> +#define DEF_SUSPEND_TIMEOUT 1000 > >> +static int _omap_rproc_suspend(struct rproc *rproc) > >> +{ > >> + struct device *dev = rproc->dev.parent; > >> + struct omap_rproc *oproc = rproc->priv; > >> + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); > >> + unsigned long ta = jiffies + to; > >> + int ret; > >> + > >> + reinit_completion(&oproc->pm_comp); > >> + oproc->suspend_acked = false; > >> + ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); > >> + if (ret < 0) { > >> + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = wait_for_completion_timeout(&oproc->pm_comp, to); > >> + if (!oproc->suspend_acked) > >> + return -EBUSY; > >> + > >> + /* > >> + * The remoteproc side is returning the ACK message before saving the > >> + * context, because the context saving is performed within a SYS/BIOS > >> + * function, and it cannot have any inter-dependencies against the IPC > >> + * layer. Also, as the SYS/BIOS needs to preserve properly the processor > >> + * register set, sending this ACK or signalling the completion of the > >> + * context save through a shared memory variable can never be the > >> + * absolute last thing to be executed on the remoteproc side, and the > >> + * MPU cannot use the ACK message as a sync point to put the remoteproc > >> + * into reset. The only way to ensure that the remote processor has > >> + * completed saving the context is to check that the module has reached > >> + * STANDBY state (after saving the context, the SYS/BIOS executes the > >> + * appropriate target-specific WFI instruction causing the module to > >> + * enter STANDBY). > >> + */ > >> + while (!_is_rproc_in_standby(oproc)) { > >> + if (time_after(jiffies, ta)) > >> + return -ETIME; > >> + schedule(); > >> + } > >> + > >> + reset_control_assert(oproc->reset); > >> + > >> + ret = omap_rproc_disable_timers(rproc, false); > >> + if (ret) { > >> + dev_err(dev, "disabling timers during suspend failed %d\n", > >> + ret); > >> + goto enable_device; > >> + } > >> + > >> + return 0; > >> + > >> +enable_device: > >> + reset_control_deassert(oproc->reset); > >> + return ret; > >> +} > >> + > >> +static int _omap_rproc_resume(struct rproc *rproc) > >> +{ > >> + struct device *dev = rproc->dev.parent; > >> + struct omap_rproc *oproc = rproc->priv; > >> + int ret; > >> + > >> + /* boot address could be lost after suspend, so restore it */ > >> + if (oproc->boot_data) { > >> + ret = omap_rproc_write_dsp_boot_addr(rproc); > >> + if (ret) { > >> + dev_err(dev, "boot address restore failed %d\n", ret); > >> + goto out; > >> + } > >> + } > >> + > >> + ret = omap_rproc_enable_timers(rproc, false); > >> + if (ret) { > >> + dev_err(dev, "enabling timers during resume failed %d\n", > >> + ret); > > > > The "ret);" fits on the live above. > > > >> + goto out; > >> + } > >> + > >> + reset_control_deassert(oproc->reset); > >> + > >> +out: > >> + return ret; > >> +} > >> + > >> +static int __maybe_unused omap_rproc_suspend(struct device *dev) > > > > The "__maybe_unused" can be dropped because this is within the #ifdef CONFIG_PM > > block. > > These are suspend/resume callbacks, so are actually controlled by > CONFIG_PM_SLEEP which can be disabled independently from runtime > suspend, and hence the annotation. > > > > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct rproc *rproc = platform_get_drvdata(pdev); > >> + int ret = 0; > >> + > >> + mutex_lock(&rproc->lock); > >> + if (rproc->state == RPROC_OFFLINE) > >> + goto out; > >> + > >> + if (rproc->state == RPROC_SUSPENDED) > >> + goto out; > >> + > >> + if (rproc->state != RPROC_RUNNING) { > >> + ret = -EBUSY; > >> + goto out; > >> + } > >> + > >> + ret = _omap_rproc_suspend(rproc); > >> + if (ret) { > >> + dev_err(dev, "suspend failed %d\n", ret); > >> + goto out; > >> + } > >> + > >> + rproc->state = RPROC_SUSPENDED; > >> +out: > >> + mutex_unlock(&rproc->lock); > >> + return ret; > >> +} > >> + > >> +static int __maybe_unused omap_rproc_resume(struct device *dev) > > > > Same comment as above. > > > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct rproc *rproc = platform_get_drvdata(pdev); > >> + int ret = 0; > >> + > >> + mutex_lock(&rproc->lock); > >> + if (rproc->state == RPROC_OFFLINE) > >> + goto out; > >> + > >> + if (rproc->state != RPROC_SUSPENDED) { > >> + ret = -EBUSY; > >> + goto out; > >> + } > >> + > >> + ret = _omap_rproc_resume(rproc); > >> + if (ret) { > >> + dev_err(dev, "resume failed %d\n", ret); > >> + goto out; > >> + } > >> + > >> + rproc->state = RPROC_RUNNING; > >> +out: > >> + mutex_unlock(&rproc->lock); > >> + return ret; > >> +} > >> +#endif /* CONFIG_PM */ > >> + > >> static const char * const ipu_mem_names[] = { > >> "l2ram", NULL > >> }; > >> @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct platform_device *pdev) > >> oproc->num_timers); > >> } > >> > >> + init_completion(&oproc->pm_comp); > >> + > >> + oproc->fck = devm_clk_get(&pdev->dev, 0); > >> + if (IS_ERR(oproc->fck)) { > >> + ret = PTR_ERR(oproc->fck); > >> + goto free_rproc; > >> + } > >> + > > > > oproc->fck is not released if an error occurs in of_reserved_mem_device_init() > > and rproc_add(). > > We are using a devres managed API, so why do we need to release it > specifically? Disregard this comment - I looked at the implementation again and you are right. > > > > >> ret = of_reserved_mem_device_init(&pdev->dev); > >> if (ret) { > >> dev_err(&pdev->dev, "device does not have specific CMA pool\n"); > >> @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> +static const struct dev_pm_ops omap_rproc_pm_ops = { > >> + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) > >> +}; > >> + > >> static struct platform_driver omap_rproc_driver = { > >> .probe = omap_rproc_probe, > >> .remove = omap_rproc_remove, > >> .driver = { > >> .name = "omap-rproc", > >> + .pm = &omap_rproc_pm_ops, > >> .of_match_table = omap_rproc_of_match, > >> }, > >> }; > >> diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h > >> index 72f656c93caa..c73383e707c7 100644 > >> --- a/drivers/remoteproc/omap_remoteproc.h > >> +++ b/drivers/remoteproc/omap_remoteproc.h > >> @@ -1,7 +1,7 @@ > >> /* > >> * Remote processor messaging > >> * > >> - * Copyright (C) 2011 Texas Instruments, Inc. > >> + * Copyright (C) 2011-2018 Texas Instruments, Inc. > > %s/2018/2019/ > > regards > Suman > > >> * Copyright (C) 2011 Google, Inc. > >> * All rights reserved. > >> * > >> @@ -57,6 +57,16 @@ > >> * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the > >> * recovery mechanism (to some extent). > >> * > >> + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote processor > >> + * > >> + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote processor > >> + * > >> + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor for a > >> + * suspend request > >> + * > >> + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor > >> + * on a suspend request > >> + * > >> * Introduce new message definitions if any here. > >> * > >> * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core > >> @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { > >> RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, > >> RP_MBOX_ECHO_REPLY = 0xFFFFFF04, > >> RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, > >> - RP_MBOX_END_MSG = 0xFFFFFF06, > >> + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, > >> + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, > >> + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, > >> + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, > >> + RP_MBOX_END_MSG = 0xFFFFFF14, > >> }; > >> > >> #endif /* _OMAP_RPMSG_H */ > >> -- > >> 2.17.1 > >> > >> -- > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c index 9c750c2ab29d..0a9b9f7d20da 100644 --- a/drivers/remoteproc/omap_remoteproc.c +++ b/drivers/remoteproc/omap_remoteproc.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/err.h> +#include <linux/io.h> #include <linux/of_device.h> #include <linux/of_reserved_mem.h> #include <linux/platform_device.h> @@ -23,10 +24,13 @@ #include <linux/remoteproc.h> #include <linux/mailbox_client.h> #include <linux/omap-mailbox.h> +#include <linux/omap-iommu.h> #include <linux/regmap.h> #include <linux/mfd/syscon.h> #include <linux/reset.h> #include <clocksource/timer-ti-dm.h> +#include <linux/clk.h> +#include <linux/clk/ti.h> #include <linux/platform_data/dmtimer-omap.h> @@ -81,6 +85,9 @@ struct omap_rproc_timer { * @timers: timer(s) info used by rproc * @rproc: rproc handle * @reset: reset handle + * @pm_comp: completion primitive to sync for suspend response + * @fck: functional clock for the remoteproc + * @suspend_acked: state machine flag to store the suspend request ack */ struct omap_rproc { struct mbox_chan *mbox; @@ -92,6 +99,9 @@ struct omap_rproc { struct omap_rproc_timer *timers; struct rproc *rproc; struct reset_control *reset; + struct completion pm_comp; + struct clk *fck; + bool suspend_acked; }; /** @@ -349,6 +359,11 @@ static void omap_rproc_mbox_callback(struct mbox_client *client, void *data) case RP_MBOX_ECHO_REPLY: dev_info(dev, "received echo reply from %s\n", name); break; + case RP_MBOX_SUSPEND_ACK: + case RP_MBOX_SUSPEND_CANCEL: + oproc->suspend_acked = msg == RP_MBOX_SUSPEND_ACK; + complete(&oproc->pm_comp); + break; default: if (msg >= RP_MBOX_READY && msg < RP_MBOX_END_MSG) return; @@ -529,6 +544,157 @@ static const struct rproc_ops omap_rproc_ops = { .da_to_va = omap_rproc_da_to_va, }; +#ifdef CONFIG_PM +static bool _is_rproc_in_standby(struct omap_rproc *oproc) +{ + return ti_clk_is_in_standby(oproc->fck); +} + +/* 1 sec is long enough time to let the remoteproc side suspend the device */ +#define DEF_SUSPEND_TIMEOUT 1000 +static int _omap_rproc_suspend(struct rproc *rproc) +{ + struct device *dev = rproc->dev.parent; + struct omap_rproc *oproc = rproc->priv; + unsigned long to = msecs_to_jiffies(DEF_SUSPEND_TIMEOUT); + unsigned long ta = jiffies + to; + int ret; + + reinit_completion(&oproc->pm_comp); + oproc->suspend_acked = false; + ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); + if (ret < 0) { + dev_err(dev, "PM mbox_send_message failed: %d\n", ret); + return ret; + } + + ret = wait_for_completion_timeout(&oproc->pm_comp, to); + if (!oproc->suspend_acked) + return -EBUSY; + + /* + * The remoteproc side is returning the ACK message before saving the + * context, because the context saving is performed within a SYS/BIOS + * function, and it cannot have any inter-dependencies against the IPC + * layer. Also, as the SYS/BIOS needs to preserve properly the processor + * register set, sending this ACK or signalling the completion of the + * context save through a shared memory variable can never be the + * absolute last thing to be executed on the remoteproc side, and the + * MPU cannot use the ACK message as a sync point to put the remoteproc + * into reset. The only way to ensure that the remote processor has + * completed saving the context is to check that the module has reached + * STANDBY state (after saving the context, the SYS/BIOS executes the + * appropriate target-specific WFI instruction causing the module to + * enter STANDBY). + */ + while (!_is_rproc_in_standby(oproc)) { + if (time_after(jiffies, ta)) + return -ETIME; + schedule(); + } + + reset_control_assert(oproc->reset); + + ret = omap_rproc_disable_timers(rproc, false); + if (ret) { + dev_err(dev, "disabling timers during suspend failed %d\n", + ret); + goto enable_device; + } + + return 0; + +enable_device: + reset_control_deassert(oproc->reset); + return ret; +} + +static int _omap_rproc_resume(struct rproc *rproc) +{ + struct device *dev = rproc->dev.parent; + struct omap_rproc *oproc = rproc->priv; + int ret; + + /* boot address could be lost after suspend, so restore it */ + if (oproc->boot_data) { + ret = omap_rproc_write_dsp_boot_addr(rproc); + if (ret) { + dev_err(dev, "boot address restore failed %d\n", ret); + goto out; + } + } + + ret = omap_rproc_enable_timers(rproc, false); + if (ret) { + dev_err(dev, "enabling timers during resume failed %d\n", + ret); + goto out; + } + + reset_control_deassert(oproc->reset); + +out: + return ret; +} + +static int __maybe_unused omap_rproc_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct rproc *rproc = platform_get_drvdata(pdev); + int ret = 0; + + mutex_lock(&rproc->lock); + if (rproc->state == RPROC_OFFLINE) + goto out; + + if (rproc->state == RPROC_SUSPENDED) + goto out; + + if (rproc->state != RPROC_RUNNING) { + ret = -EBUSY; + goto out; + } + + ret = _omap_rproc_suspend(rproc); + if (ret) { + dev_err(dev, "suspend failed %d\n", ret); + goto out; + } + + rproc->state = RPROC_SUSPENDED; +out: + mutex_unlock(&rproc->lock); + return ret; +} + +static int __maybe_unused omap_rproc_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct rproc *rproc = platform_get_drvdata(pdev); + int ret = 0; + + mutex_lock(&rproc->lock); + if (rproc->state == RPROC_OFFLINE) + goto out; + + if (rproc->state != RPROC_SUSPENDED) { + ret = -EBUSY; + goto out; + } + + ret = _omap_rproc_resume(rproc); + if (ret) { + dev_err(dev, "resume failed %d\n", ret); + goto out; + } + + rproc->state = RPROC_RUNNING; +out: + mutex_unlock(&rproc->lock); + return ret; +} +#endif /* CONFIG_PM */ + static const char * const ipu_mem_names[] = { "l2ram", NULL }; @@ -786,6 +952,14 @@ static int omap_rproc_probe(struct platform_device *pdev) oproc->num_timers); } + init_completion(&oproc->pm_comp); + + oproc->fck = devm_clk_get(&pdev->dev, 0); + if (IS_ERR(oproc->fck)) { + ret = PTR_ERR(oproc->fck); + goto free_rproc; + } + ret = of_reserved_mem_device_init(&pdev->dev); if (ret) { dev_err(&pdev->dev, "device does not have specific CMA pool\n"); @@ -818,11 +992,16 @@ static int omap_rproc_remove(struct platform_device *pdev) return 0; } +static const struct dev_pm_ops omap_rproc_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) +}; + static struct platform_driver omap_rproc_driver = { .probe = omap_rproc_probe, .remove = omap_rproc_remove, .driver = { .name = "omap-rproc", + .pm = &omap_rproc_pm_ops, .of_match_table = omap_rproc_of_match, }, }; diff --git a/drivers/remoteproc/omap_remoteproc.h b/drivers/remoteproc/omap_remoteproc.h index 72f656c93caa..c73383e707c7 100644 --- a/drivers/remoteproc/omap_remoteproc.h +++ b/drivers/remoteproc/omap_remoteproc.h @@ -1,7 +1,7 @@ /* * Remote processor messaging * - * Copyright (C) 2011 Texas Instruments, Inc. + * Copyright (C) 2011-2018 Texas Instruments, Inc. * Copyright (C) 2011 Google, Inc. * All rights reserved. * @@ -57,6 +57,16 @@ * @RP_MBOX_ABORT_REQUEST: a "please crash" request, used for testing the * recovery mechanism (to some extent). * + * @RP_MBOX_SUSPEND_AUTO: auto suspend request for the remote processor + * + * @RP_MBOX_SUSPEND_SYSTEM: system suspend request for the remote processor + * + * @RP_MBOX_SUSPEND_ACK: successful response from remote processor for a + * suspend request + * + * @RP_MBOX_SUSPEND_CANCEL: a cancel suspend response from a remote processor + * on a suspend request + * * Introduce new message definitions if any here. * * @RP_MBOX_END_MSG: Indicates end of known/defined messages from remote core @@ -70,7 +80,11 @@ enum omap_rp_mbox_messages { RP_MBOX_ECHO_REQUEST = 0xFFFFFF03, RP_MBOX_ECHO_REPLY = 0xFFFFFF04, RP_MBOX_ABORT_REQUEST = 0xFFFFFF05, - RP_MBOX_END_MSG = 0xFFFFFF06, + RP_MBOX_SUSPEND_AUTO = 0xFFFFFF10, + RP_MBOX_SUSPEND_SYSTEM = 0xFFFFFF11, + RP_MBOX_SUSPEND_ACK = 0xFFFFFF12, + RP_MBOX_SUSPEND_CANCEL = 0xFFFFFF13, + RP_MBOX_END_MSG = 0xFFFFFF14, }; #endif /* _OMAP_RPMSG_H */