| Message ID | 20191213125537.11509-1-t-kristo@ti.com |
|---|---|
| Headers | show |
| Series | remoteproc: updates for omap remoteproc support | expand |
On Fri, Dec 13, 2019 at 02:55:26PM +0200, Tero Kristo wrote: > From: Suman Anna <s-anna@ti.com> > > The OMAP remoteproc driver has been enhanced to parse and store > the kernel mappings for different internal RAM memories that may > be present within each remote processor IP subsystem. Different > devices have varying memories present on current SoCs. The current > support handles the L2RAM for all IPU devices on OMAP4+ SoCs. The > DSPs on OMAP4/OMAP5 only have Unicaches and do not have any L1 or > L2 RAM memories. > > IPUs are expected to have the L2RAM at a fixed device address of > 0x20000000, based on the current limitations on Attribute MMU > configurations. > > NOTE: > The current logic doesn't handle the parsing of memories for DRA7 > remoteproc devices, and will be added alongside the DRA7 support. > > Signed-off-by: Suman Anna <s-anna@ti.com> > [t-kristo: converted to parse mem names / device addresses from pdata] > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/remoteproc/omap_remoteproc.c | 86 ++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > > diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > index d80f5d7b5931..844703507a74 100644 > --- a/drivers/remoteproc/omap_remoteproc.c > +++ b/drivers/remoteproc/omap_remoteproc.c > @@ -39,11 +39,27 @@ struct omap_rproc_boot_data { > unsigned int boot_reg; > }; > > +/* > + * struct omap_rproc_mem - internal memory structure > + * @cpu_addr: MPU virtual address of the memory region > + * @bus_addr: bus address used to access the memory region > + * @dev_addr: device address of the memory region from DSP view > + * @size: size of the memory region > + */ > +struct omap_rproc_mem { > + void __iomem *cpu_addr; > + phys_addr_t bus_addr; > + u32 dev_addr; > + size_t size; > +}; > + > /** > * struct omap_rproc - omap remote processor state > * @mbox: mailbox channel handle > * @client: mailbox client to request the mailbox channel > * @boot_data: boot data structure for setting processor boot address > + * @mem: internal memory regions data > + * @num_mems: number of internal memory regions > * @rproc: rproc handle > * @reset: reset handle > */ > @@ -51,6 +67,8 @@ struct omap_rproc { > struct mbox_chan *mbox; > struct mbox_client client; > struct omap_rproc_boot_data *boot_data; > + struct omap_rproc_mem *mem; > + int num_mems; > struct rproc *rproc; > struct reset_control *reset; > }; > @@ -59,10 +77,14 @@ struct omap_rproc { > * struct omap_rproc_dev_data - device data for the omap remote processor > * @device_name: device name of the remote processor > * @has_bootreg: true if this remote processor has boot register > + * @mem_names: memory names for this remote processor > + * @dev_addrs: device addresses corresponding to the memory names > */ > struct omap_rproc_dev_data { > const char *device_name; > bool has_bootreg; > + const char * const *mem_names; > + const u32 *dev_addrs; Bunching these two in a new structure like omap_rproc_mem_data would clean things up. That way the two arrays in the next hunk get merged and there can't be a difference in sizes, somthing that will sturdy the main loop in omap_rproc_of_get_internal_memories() below. > }; > > /** > @@ -216,6 +238,14 @@ static const struct rproc_ops omap_rproc_ops = { > .kick = omap_rproc_kick, > }; > > +static const char * const ipu_mem_names[] = { > + "l2ram", NULL > +}; > + > +static const u32 ipu_dev_addrs[] = { > + 0x20000000, > +}; > + > static const struct omap_rproc_dev_data omap4_dsp_dev_data = { > .device_name = "dsp", > .has_bootreg = true, > @@ -223,6 +253,8 @@ static const struct omap_rproc_dev_data omap4_dsp_dev_data = { > > static const struct omap_rproc_dev_data omap4_ipu_dev_data = { > .device_name = "ipu", > + .mem_names = ipu_mem_names, > + .dev_addrs = ipu_dev_addrs, > }; > > static const struct omap_rproc_dev_data omap5_dsp_dev_data = { > @@ -232,6 +264,8 @@ static const struct omap_rproc_dev_data omap5_dsp_dev_data = { > > static const struct omap_rproc_dev_data omap5_ipu_dev_data = { > .device_name = "ipu", > + .mem_names = ipu_mem_names, > + .dev_addrs = ipu_dev_addrs, > }; > > static const struct of_device_id omap_rproc_of_match[] = { > @@ -311,6 +345,54 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev, > return 0; > } > > +static int omap_rproc_of_get_internal_memories(struct platform_device *pdev, > + struct rproc *rproc) > +{ > + struct omap_rproc *oproc = rproc->priv; > + struct device *dev = &pdev->dev; > + const struct omap_rproc_dev_data *data; > + struct resource *res; > + int num_mems; > + int i; > + > + data = of_device_get_match_data(&pdev->dev); > + if (!data) > + return -ENODEV; > + > + if (!data->mem_names) > + return 0; > + > + for (num_mems = 0; data->mem_names[num_mems]; num_mems++) > + ; Instead of doing this function of_property_count_elems_of_size() can be used on the "reg" property. In the loop below a check should be done to see if data->mem_data[i] (see above comment) is valid before calling platform_get_resource_byname(). If not then an error can be returned. I'm running out of time for today - I will continue reviewing the other patches tomorrow. > + > + oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem), > + GFP_KERNEL); > + if (!oproc->mem) > + return -ENOMEM; > + > + for (i = 0; i < num_mems; i++) { > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + data->mem_names[i]); > + oproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res); > + if (IS_ERR(oproc->mem[i].cpu_addr)) { > + dev_err(dev, "failed to parse and map %s memory\n", > + data->mem_names[i]); > + return PTR_ERR(oproc->mem[i].cpu_addr); > + } > + oproc->mem[i].bus_addr = res->start; > + oproc->mem[i].dev_addr = data->dev_addrs[i]; > + oproc->mem[i].size = resource_size(res); > + > + dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n", > + data->mem_names[i], &oproc->mem[i].bus_addr, > + oproc->mem[i].size, oproc->mem[i].cpu_addr, > + oproc->mem[i].dev_addr); > + } > + oproc->num_mems = num_mems; > + > + return 0; > +} > + > static int omap_rproc_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -350,6 +432,10 @@ static int omap_rproc_probe(struct platform_device *pdev) > /* All existing OMAP IPU and DSP processors have an MMU */ > rproc->has_iommu = true; > > + ret = omap_rproc_of_get_internal_memories(pdev, rproc); > + if (ret) > + goto free_rproc; > + > ret = omap_rproc_get_boot_data(pdev, rproc); > if (ret) > goto free_rproc; > -- > 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 05:31, Tero Kristo <t-kristo@ti.com> wrote: > > On 18/12/2019 02:22, Mathieu Poirier wrote: > > On Fri, Dec 13, 2019 at 02:55:26PM +0200, Tero Kristo wrote: > >> From: Suman Anna <s-anna@ti.com> > >> > >> The OMAP remoteproc driver has been enhanced to parse and store > >> the kernel mappings for different internal RAM memories that may > >> be present within each remote processor IP subsystem. Different > >> devices have varying memories present on current SoCs. The current > >> support handles the L2RAM for all IPU devices on OMAP4+ SoCs. The > >> DSPs on OMAP4/OMAP5 only have Unicaches and do not have any L1 or > >> L2 RAM memories. > >> > >> IPUs are expected to have the L2RAM at a fixed device address of > >> 0x20000000, based on the current limitations on Attribute MMU > >> configurations. > >> > >> NOTE: > >> The current logic doesn't handle the parsing of memories for DRA7 > >> remoteproc devices, and will be added alongside the DRA7 support. > >> > >> Signed-off-by: Suman Anna <s-anna@ti.com> > >> [t-kristo: converted to parse mem names / device addresses from pdata] > >> Signed-off-by: Tero Kristo <t-kristo@ti.com> > >> --- > >> drivers/remoteproc/omap_remoteproc.c | 86 ++++++++++++++++++++++++++++ > >> 1 file changed, 86 insertions(+) > >> > >> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > >> index d80f5d7b5931..844703507a74 100644 > >> --- a/drivers/remoteproc/omap_remoteproc.c > >> +++ b/drivers/remoteproc/omap_remoteproc.c > >> @@ -39,11 +39,27 @@ struct omap_rproc_boot_data { > >> unsigned int boot_reg; > >> }; > >> > >> +/* > >> + * struct omap_rproc_mem - internal memory structure > >> + * @cpu_addr: MPU virtual address of the memory region > >> + * @bus_addr: bus address used to access the memory region > >> + * @dev_addr: device address of the memory region from DSP view > >> + * @size: size of the memory region > >> + */ > >> +struct omap_rproc_mem { > >> + void __iomem *cpu_addr; > >> + phys_addr_t bus_addr; > >> + u32 dev_addr; > >> + size_t size; > >> +}; > >> + > >> /** > >> * struct omap_rproc - omap remote processor state > >> * @mbox: mailbox channel handle > >> * @client: mailbox client to request the mailbox channel > >> * @boot_data: boot data structure for setting processor boot address > >> + * @mem: internal memory regions data > >> + * @num_mems: number of internal memory regions > >> * @rproc: rproc handle > >> * @reset: reset handle > >> */ > >> @@ -51,6 +67,8 @@ struct omap_rproc { > >> struct mbox_chan *mbox; > >> struct mbox_client client; > >> struct omap_rproc_boot_data *boot_data; > >> + struct omap_rproc_mem *mem; > >> + int num_mems; > >> struct rproc *rproc; > >> struct reset_control *reset; > >> }; > >> @@ -59,10 +77,14 @@ struct omap_rproc { > >> * struct omap_rproc_dev_data - device data for the omap remote processor > >> * @device_name: device name of the remote processor > >> * @has_bootreg: true if this remote processor has boot register > >> + * @mem_names: memory names for this remote processor > >> + * @dev_addrs: device addresses corresponding to the memory names > >> */ > >> struct omap_rproc_dev_data { > >> const char *device_name; > >> bool has_bootreg; > >> + const char * const *mem_names; > >> + const u32 *dev_addrs; > > > > Bunching these two in a new structure like omap_rproc_mem_data would clean > > things up. That way the two arrays in the next hunk get merged and there can't > > be a difference in sizes, somthing that will sturdy the main loop in > > omap_rproc_of_get_internal_memories() below. > > Will fix this. > > > > >> }; > >> > >> /** > >> @@ -216,6 +238,14 @@ static const struct rproc_ops omap_rproc_ops = { > >> .kick = omap_rproc_kick, > >> }; > >> > >> +static const char * const ipu_mem_names[] = { > >> + "l2ram", NULL > >> +}; > >> + > >> +static const u32 ipu_dev_addrs[] = { > >> + 0x20000000, > >> +}; > >> + > >> static const struct omap_rproc_dev_data omap4_dsp_dev_data = { > >> .device_name = "dsp", > >> .has_bootreg = true, > >> @@ -223,6 +253,8 @@ static const struct omap_rproc_dev_data omap4_dsp_dev_data = { > >> > >> static const struct omap_rproc_dev_data omap4_ipu_dev_data = { > >> .device_name = "ipu", > >> + .mem_names = ipu_mem_names, > >> + .dev_addrs = ipu_dev_addrs, > >> }; > >> > >> static const struct omap_rproc_dev_data omap5_dsp_dev_data = { > >> @@ -232,6 +264,8 @@ static const struct omap_rproc_dev_data omap5_dsp_dev_data = { > >> > >> static const struct omap_rproc_dev_data omap5_ipu_dev_data = { > >> .device_name = "ipu", > >> + .mem_names = ipu_mem_names, > >> + .dev_addrs = ipu_dev_addrs, > >> }; > >> > >> static const struct of_device_id omap_rproc_of_match[] = { > >> @@ -311,6 +345,54 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev, > >> return 0; > >> } > >> > >> +static int omap_rproc_of_get_internal_memories(struct platform_device *pdev, > >> + struct rproc *rproc) > >> +{ > >> + struct omap_rproc *oproc = rproc->priv; > >> + struct device *dev = &pdev->dev; > >> + const struct omap_rproc_dev_data *data; > >> + struct resource *res; > >> + int num_mems; > >> + int i; > >> + > >> + data = of_device_get_match_data(&pdev->dev); > >> + if (!data) > >> + return -ENODEV; > >> + > >> + if (!data->mem_names) > >> + return 0; > >> + > >> + for (num_mems = 0; data->mem_names[num_mems]; num_mems++) > >> + ; > > > > Instead of doing this function of_property_count_elems_of_size() can be used on > > the "reg" property. > > Hmm right, but the problem is then we don't know if someone left out one > of the memories in DT. We want to check the presence for all defined in > the platform data. > In my opinion (and to go along what I advocated in a comment on another patch) everything should be dictated from the DT. If an area of reserved memory is missing in the DT then the infrastructure should recognise it and refuse to move forward with initialisation. Thanks Mathieu > > > > In the loop below a check should be done to see if data->mem_data[i] (see above > > comment) is valid before calling platform_get_resource_byname(). If not then > > an error can be returned. > > Will add a check to it. > > -Tero > > > > > I'm running out of time for today - I will continue reviewing the other patches > > tomorrow. > > > >> + > >> + oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem), > >> + GFP_KERNEL); > >> + if (!oproc->mem) > >> + return -ENOMEM; > >> + > >> + for (i = 0; i < num_mems; i++) { > >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > >> + data->mem_names[i]); > >> + oproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res); > >> + if (IS_ERR(oproc->mem[i].cpu_addr)) { > >> + dev_err(dev, "failed to parse and map %s memory\n", > >> + data->mem_names[i]); > >> + return PTR_ERR(oproc->mem[i].cpu_addr); > >> + } > >> + oproc->mem[i].bus_addr = res->start; > >> + oproc->mem[i].dev_addr = data->dev_addrs[i]; > >> + oproc->mem[i].size = resource_size(res); > >> + > >> + dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n", > >> + data->mem_names[i], &oproc->mem[i].bus_addr, > >> + oproc->mem[i].size, oproc->mem[i].cpu_addr, > >> + oproc->mem[i].dev_addr); > >> + } > >> + oproc->num_mems = num_mems; > >> + > >> + return 0; > >> +} > >> + > >> static int omap_rproc_probe(struct platform_device *pdev) > >> { > >> struct device_node *np = pdev->dev.of_node; > >> @@ -350,6 +432,10 @@ static int omap_rproc_probe(struct platform_device *pdev) > >> /* All existing OMAP IPU and DSP processors have an MMU */ > >> rproc->has_iommu = true; > >> > >> + ret = omap_rproc_of_get_internal_memories(pdev, rproc); > >> + if (ret) > >> + goto free_rproc; > >> + > >> ret = omap_rproc_get_boot_data(pdev, rproc); > >> if (ret) > >> goto free_rproc; > >> -- > >> 2.17.1 > >> > >> -- > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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
On Thu, 19 Dec 2019 at 06:18, Tero Kristo <t-kristo@ti.com> wrote: > > On 18/12/2019 02:38, Mathieu Poirier wrote: > > On Fri, Dec 13, 2019 at 02:55:27PM +0200, Tero Kristo wrote: > >> From: Suman Anna <s-anna@ti.com> > >> > >> An implementation for the rproc ops .da_to_va() has been added > >> that provides the address translation between device addresses > >> to kernel virtual addresses for internal RAMs present on that > >> particular remote processor device. The implementation provides > >> the translations based on the addresses parsed and stored during > >> the probe. > >> > >> This ops gets invoked by the exported rproc_da_to_va() function > >> and allows the remoteproc core's ELF loader to be able to load > >> program data directly into the internal memories. > >> > >> Signed-off-by: Suman Anna <s-anna@ti.com> > >> Signed-off-by: Tero Kristo <t-kristo@ti.com> > >> --- > >> drivers/remoteproc/omap_remoteproc.c | 39 ++++++++++++++++++++++++++++ > >> 1 file changed, 39 insertions(+) > >> > >> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > >> index 844703507a74..28f14e24b389 100644 > >> --- a/drivers/remoteproc/omap_remoteproc.c > >> +++ b/drivers/remoteproc/omap_remoteproc.c > >> @@ -232,10 +232,49 @@ static int omap_rproc_stop(struct rproc *rproc) > >> return 0; > >> } > >> > >> +/** > >> + * omap_rproc_da_to_va() - internal memory translation helper > >> + * @rproc: remote processor to apply the address translation for > >> + * @da: device address to translate > >> + * @len: length of the memory buffer > >> + * > >> + * Custom function implementing the rproc .da_to_va ops to provide address > >> + * translation (device address to kernel virtual address) for internal RAMs > >> + * present in a DSP or IPU device). The translated addresses can be used > >> + * either by the remoteproc core for loading, or by any rpmsg bus drivers. > >> + * Returns the translated virtual address in kernel memory space, or NULL > >> + * in failure. > >> + */ > >> +static void *omap_rproc_da_to_va(struct rproc *rproc, u64 da, int len) > >> +{ > >> + struct omap_rproc *oproc = rproc->priv; > >> + int i; > >> + u32 offset; > >> + > >> + if (len <= 0) > >> + return NULL; > >> + > >> + if (!oproc->num_mems) > >> + return NULL; > >> + > >> + for (i = 0; i < oproc->num_mems; i++) { > >> + if (da >= oproc->mem[i].dev_addr && da + len <= > > > > Shouldn't this be '<' rather than '<=' ? > > No, I think <= is correct. You need to consider the initial byte in the > range also. Consider a simple case where you provide the exact da + len > corresponding to a specific memory range. For that specific case you are correct. On the flip side if @da falls somewhere after @mem[i].dev_addr, there is a possibility to clobber the first byte of the next range if <= is used. Thanks, Mathieu > > > > >> + oproc->mem[i].dev_addr + oproc->mem[i].size) { > > > > One space too many after the '+' . > > True, I wonder why checkpatch did not catch this. > > > > >> + offset = da - oproc->mem[i].dev_addr; > > > > One space too many after then '-' . > > Same, will fix these two. > > -Tero > > > > >> + /* __force to make sparse happy with type conversion */ > >> + return (__force void *)(oproc->mem[i].cpu_addr + > >> + offset); > >> + } > >> + } > >> + > >> + return NULL; > >> +} > >> + > >> static const struct rproc_ops omap_rproc_ops = { > >> .start = omap_rproc_start, > >> .stop = omap_rproc_stop, > >> .kick = omap_rproc_kick, > >> + .da_to_va = omap_rproc_da_to_va, > >> }; > >> > >> static const char * const ipu_mem_names[] = { > >> -- > >> 2.17.1 > >> > >> -- > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 12/19/19 11:37 AM, Mathieu Poirier wrote: > On Thu, 19 Dec 2019 at 05:31, Tero Kristo <t-kristo@ti.com> wrote: >> >> On 18/12/2019 02:22, Mathieu Poirier wrote: >>> On Fri, Dec 13, 2019 at 02:55:26PM +0200, Tero Kristo wrote: >>>> From: Suman Anna <s-anna@ti.com> >>>> >>>> The OMAP remoteproc driver has been enhanced to parse and store >>>> the kernel mappings for different internal RAM memories that may >>>> be present within each remote processor IP subsystem. Different >>>> devices have varying memories present on current SoCs. The current >>>> support handles the L2RAM for all IPU devices on OMAP4+ SoCs. The >>>> DSPs on OMAP4/OMAP5 only have Unicaches and do not have any L1 or >>>> L2 RAM memories. >>>> >>>> IPUs are expected to have the L2RAM at a fixed device address of >>>> 0x20000000, based on the current limitations on Attribute MMU >>>> configurations. >>>> >>>> NOTE: >>>> The current logic doesn't handle the parsing of memories for DRA7 >>>> remoteproc devices, and will be added alongside the DRA7 support. >>>> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>> [t-kristo: converted to parse mem names / device addresses from pdata] >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/remoteproc/omap_remoteproc.c | 86 ++++++++++++++++++++++++++++ >>>> 1 file changed, 86 insertions(+) >>>> >>>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >>>> index d80f5d7b5931..844703507a74 100644 >>>> --- a/drivers/remoteproc/omap_remoteproc.c >>>> +++ b/drivers/remoteproc/omap_remoteproc.c >>>> @@ -39,11 +39,27 @@ struct omap_rproc_boot_data { >>>> unsigned int boot_reg; >>>> }; >>>> >>>> +/* Somehow a second trailing * got dropped by mistake here. >>>> + * struct omap_rproc_mem - internal memory structure >>>> + * @cpu_addr: MPU virtual address of the memory region >>>> + * @bus_addr: bus address used to access the memory region >>>> + * @dev_addr: device address of the memory region from DSP view >>>> + * @size: size of the memory region >>>> + */ >>>> +struct omap_rproc_mem { >>>> + void __iomem *cpu_addr; >>>> + phys_addr_t bus_addr; >>>> + u32 dev_addr; >>>> + size_t size; >>>> +}; >>>> + >>>> /** >>>> * struct omap_rproc - omap remote processor state >>>> * @mbox: mailbox channel handle >>>> * @client: mailbox client to request the mailbox channel >>>> * @boot_data: boot data structure for setting processor boot address >>>> + * @mem: internal memory regions data >>>> + * @num_mems: number of internal memory regions >>>> * @rproc: rproc handle >>>> * @reset: reset handle >>>> */ >>>> @@ -51,6 +67,8 @@ struct omap_rproc { >>>> struct mbox_chan *mbox; >>>> struct mbox_client client; >>>> struct omap_rproc_boot_data *boot_data; >>>> + struct omap_rproc_mem *mem; >>>> + int num_mems; >>>> struct rproc *rproc; >>>> struct reset_control *reset; >>>> }; >>>> @@ -59,10 +77,14 @@ struct omap_rproc { >>>> * struct omap_rproc_dev_data - device data for the omap remote processor >>>> * @device_name: device name of the remote processor >>>> * @has_bootreg: true if this remote processor has boot register >>>> + * @mem_names: memory names for this remote processor >>>> + * @dev_addrs: device addresses corresponding to the memory names >>>> */ >>>> struct omap_rproc_dev_data { >>>> const char *device_name; >>>> bool has_bootreg; >>>> + const char * const *mem_names; >>>> + const u32 *dev_addrs; >>> >>> Bunching these two in a new structure like omap_rproc_mem_data would clean >>> things up. That way the two arrays in the next hunk get merged and there can't >>> be a difference in sizes, somthing that will sturdy the main loop in >>> omap_rproc_of_get_internal_memories() below. >> >> Will fix this. >> >>> >>>> }; >>>> >>>> /** >>>> @@ -216,6 +238,14 @@ static const struct rproc_ops omap_rproc_ops = { >>>> .kick = omap_rproc_kick, >>>> }; >>>> >>>> +static const char * const ipu_mem_names[] = { >>>> + "l2ram", NULL >>>> +}; >>>> + >>>> +static const u32 ipu_dev_addrs[] = { >>>> + 0x20000000, >>>> +}; >>>> + >>>> static const struct omap_rproc_dev_data omap4_dsp_dev_data = { >>>> .device_name = "dsp", >>>> .has_bootreg = true, >>>> @@ -223,6 +253,8 @@ static const struct omap_rproc_dev_data omap4_dsp_dev_data = { >>>> >>>> static const struct omap_rproc_dev_data omap4_ipu_dev_data = { >>>> .device_name = "ipu", >>>> + .mem_names = ipu_mem_names, >>>> + .dev_addrs = ipu_dev_addrs, >>>> }; >>>> >>>> static const struct omap_rproc_dev_data omap5_dsp_dev_data = { >>>> @@ -232,6 +264,8 @@ static const struct omap_rproc_dev_data omap5_dsp_dev_data = { >>>> >>>> static const struct omap_rproc_dev_data omap5_ipu_dev_data = { >>>> .device_name = "ipu", >>>> + .mem_names = ipu_mem_names, >>>> + .dev_addrs = ipu_dev_addrs, >>>> }; >>>> >>>> static const struct of_device_id omap_rproc_of_match[] = { >>>> @@ -311,6 +345,54 @@ static int omap_rproc_get_boot_data(struct platform_device *pdev, >>>> return 0; >>>> } >>>> >>>> +static int omap_rproc_of_get_internal_memories(struct platform_device *pdev, >>>> + struct rproc *rproc) >>>> +{ >>>> + struct omap_rproc *oproc = rproc->priv; >>>> + struct device *dev = &pdev->dev; >>>> + const struct omap_rproc_dev_data *data; >>>> + struct resource *res; >>>> + int num_mems; >>>> + int i; >>>> + >>>> + data = of_device_get_match_data(&pdev->dev); >>>> + if (!data) >>>> + return -ENODEV; >>>> + >>>> + if (!data->mem_names) >>>> + return 0; >>>> + >>>> + for (num_mems = 0; data->mem_names[num_mems]; num_mems++) >>>> + ; >>> >>> Instead of doing this function of_property_count_elems_of_size() can be used on >>> the "reg" property. >> >> Hmm right, but the problem is then we don't know if someone left out one >> of the memories in DT. We want to check the presence for all defined in >> the platform data. >> > > In my opinion (and to go along what I advocated in a comment on > another patch) everything should be dictated from the DT. If an area > of reserved memory is missing in the DT then the infrastructure should > recognise it and refuse to move forward with initialisation. That's actually what the code does too. It is just different remoteprocs have different internal memories, and that array of names is actually retrieved here based on match data. regards Suman > > Thanks > Mathieu > >>> >>> In the loop below a check should be done to see if data->mem_data[i] (see above >>> comment) is valid before calling platform_get_resource_byname(). If not then >>> an error can be returned. >> >> Will add a check to it. >> >> -Tero >> >>> >>> I'm running out of time for today - I will continue reviewing the other patches >>> tomorrow. >>> >>>> + >>>> + oproc->mem = devm_kcalloc(dev, num_mems, sizeof(*oproc->mem), >>>> + GFP_KERNEL); >>>> + if (!oproc->mem) >>>> + return -ENOMEM; >>>> + >>>> + for (i = 0; i < num_mems; i++) { >>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >>>> + data->mem_names[i]); >>>> + oproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res); >>>> + if (IS_ERR(oproc->mem[i].cpu_addr)) { >>>> + dev_err(dev, "failed to parse and map %s memory\n", >>>> + data->mem_names[i]); >>>> + return PTR_ERR(oproc->mem[i].cpu_addr); >>>> + } >>>> + oproc->mem[i].bus_addr = res->start; >>>> + oproc->mem[i].dev_addr = data->dev_addrs[i]; >>>> + oproc->mem[i].size = resource_size(res); >>>> + >>>> + dev_dbg(dev, "memory %8s: bus addr %pa size 0x%x va %p da 0x%x\n", >>>> + data->mem_names[i], &oproc->mem[i].bus_addr, >>>> + oproc->mem[i].size, oproc->mem[i].cpu_addr, >>>> + oproc->mem[i].dev_addr); >>>> + } >>>> + oproc->num_mems = num_mems; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int omap_rproc_probe(struct platform_device *pdev) >>>> { >>>> struct device_node *np = pdev->dev.of_node; >>>> @@ -350,6 +432,10 @@ static int omap_rproc_probe(struct platform_device *pdev) >>>> /* All existing OMAP IPU and DSP processors have an MMU */ >>>> rproc->has_iommu = true; >>>> >>>> + ret = omap_rproc_of_get_internal_memories(pdev, rproc); >>>> + if (ret) >>>> + goto free_rproc; >>>> + >>>> ret = omap_rproc_get_boot_data(pdev, rproc); >>>> if (ret) >>>> goto free_rproc; >>>> -- >>>> 2.17.1 >>>> >>>> -- >> >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 12/19/19 6:12 PM, Mathieu Poirier wrote: > On Thu, 19 Dec 2019 at 06:18, Tero Kristo <t-kristo@ti.com> wrote: >> >> On 18/12/2019 02:38, Mathieu Poirier wrote: >>> On Fri, Dec 13, 2019 at 02:55:27PM +0200, Tero Kristo wrote: >>>> From: Suman Anna <s-anna@ti.com> >>>> >>>> An implementation for the rproc ops .da_to_va() has been added >>>> that provides the address translation between device addresses >>>> to kernel virtual addresses for internal RAMs present on that >>>> particular remote processor device. The implementation provides >>>> the translations based on the addresses parsed and stored during >>>> the probe. >>>> >>>> This ops gets invoked by the exported rproc_da_to_va() function >>>> and allows the remoteproc core's ELF loader to be able to load >>>> program data directly into the internal memories. >>>> >>>> Signed-off-by: Suman Anna <s-anna@ti.com> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/remoteproc/omap_remoteproc.c | 39 ++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >>>> index 844703507a74..28f14e24b389 100644 >>>> --- a/drivers/remoteproc/omap_remoteproc.c >>>> +++ b/drivers/remoteproc/omap_remoteproc.c >>>> @@ -232,10 +232,49 @@ static int omap_rproc_stop(struct rproc *rproc) >>>> return 0; >>>> } >>>> >>>> +/** >>>> + * omap_rproc_da_to_va() - internal memory translation helper >>>> + * @rproc: remote processor to apply the address translation for >>>> + * @da: device address to translate >>>> + * @len: length of the memory buffer >>>> + * >>>> + * Custom function implementing the rproc .da_to_va ops to provide address >>>> + * translation (device address to kernel virtual address) for internal RAMs >>>> + * present in a DSP or IPU device). The translated addresses can be used >>>> + * either by the remoteproc core for loading, or by any rpmsg bus drivers. >>>> + * Returns the translated virtual address in kernel memory space, or NULL >>>> + * in failure. >>>> + */ >>>> +static void *omap_rproc_da_to_va(struct rproc *rproc, u64 da, int len) >>>> +{ >>>> + struct omap_rproc *oproc = rproc->priv; >>>> + int i; >>>> + u32 offset; >>>> + >>>> + if (len <= 0) >>>> + return NULL; >>>> + >>>> + if (!oproc->num_mems) >>>> + return NULL; >>>> + >>>> + for (i = 0; i < oproc->num_mems; i++) { >>>> + if (da >= oproc->mem[i].dev_addr && da + len <= >>> >>> Shouldn't this be '<' rather than '<=' ? >> >> No, I think <= is correct. You need to consider the initial byte in the >> range also. Consider a simple case where you provide the exact da + len >> corresponding to a specific memory range. > > For that specific case you are correct. On the flip side if @da falls > somewhere after @mem[i].dev_addr, there is a possibility to clobber > the first byte of the next range if <= is used. Not really, you will miss out on the last byte actually if you use just <. This is just address range check, any memcpy would actually end one byte before. Eg: 0x80000 of len 0x10000. I should perfectly be able to copy 0x1000 bytes at 0x8f000. regards Suman > > Thanks, > Mathieu > >> >>> >>>> + oproc->mem[i].dev_addr + oproc->mem[i].size) { >>> >>> One space too many after the '+' . >> >> True, I wonder why checkpatch did not catch this. >> >>> >>>> + offset = da - oproc->mem[i].dev_addr; >>> >>> One space too many after then '-' . >> >> Same, will fix these two. >> >> -Tero >> >>> >>>> + /* __force to make sparse happy with type conversion */ >>>> + return (__force void *)(oproc->mem[i].cpu_addr + >>>> + offset); >>>> + } >>>> + } >>>> + >>>> + return NULL; >>>> +} >>>> + >>>> static const struct rproc_ops omap_rproc_ops = { >>>> .start = omap_rproc_start, >>>> .stop = omap_rproc_stop, >>>> .kick = omap_rproc_kick, >>>> + .da_to_va = omap_rproc_da_to_va, >>>> }; >>>> >>>> static const char * const ipu_mem_names[] = { >>>> -- >>>> 2.17.1 >>>> >>>> -- >> >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 12/13/19 6:55 AM, Tero Kristo wrote: > From: Suman Anna <s-anna@ti.com> > > The omap_rproc_reserve_cma() function is not defined at the moment. > This prototype was to be used to define a function to declare a > remoteproc device-specific CMA pool. > > The remoteproc devices will be defined through DT going forward. A > device specific CMA pool will be defined under the reserved-memory > node, and will be associated with the appropriate remoteproc device > node. This function prototype will no longer be needed and has > therefore been cleaned up. > > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Tero Kristo <t-kristo@ti.com> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> With the structure removed, you can actually drop the file altogether. regards Suman > --- > include/linux/platform_data/remoteproc-omap.h | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/include/linux/platform_data/remoteproc-omap.h b/include/linux/platform_data/remoteproc-omap.h > index 6bea01e199fe..49c78805916f 100644 > --- a/include/linux/platform_data/remoteproc-omap.h > +++ b/include/linux/platform_data/remoteproc-omap.h > @@ -21,16 +21,4 @@ struct omap_rproc_pdata { > int (*device_shutdown)(struct platform_device *pdev); > }; > > -#if defined(CONFIG_OMAP_REMOTEPROC) || defined(CONFIG_OMAP_REMOTEPROC_MODULE) > - > -void __init omap_rproc_reserve_cma(void); > - > -#else > - > -static inline void __init omap_rproc_reserve_cma(void) > -{ > -} > - > -#endif > - > #endif /* _PLAT_REMOTEPROC_H */ >
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 04:47, Suman Anna wrote: > Hi Tero, > > On 12/13/19 6:55 AM, Tero Kristo wrote: >> From: Suman Anna <s-anna@ti.com> >> >> The omap_rproc_reserve_cma() function is not defined at the moment. >> This prototype was to be used to define a function to declare a >> remoteproc device-specific CMA pool. >> >> The remoteproc devices will be defined through DT going forward. A >> device specific CMA pool will be defined under the reserved-memory >> node, and will be associated with the appropriate remoteproc device >> node. This function prototype will no longer be needed and has >> therefore been cleaned up. >> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > With the structure removed, you can actually drop the file altogether. Yeah, let me check that out. -Tero > > regards > Suman > >> --- >> include/linux/platform_data/remoteproc-omap.h | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/include/linux/platform_data/remoteproc-omap.h b/include/linux/platform_data/remoteproc-omap.h >> index 6bea01e199fe..49c78805916f 100644 >> --- a/include/linux/platform_data/remoteproc-omap.h >> +++ b/include/linux/platform_data/remoteproc-omap.h >> @@ -21,16 +21,4 @@ struct omap_rproc_pdata { >> int (*device_shutdown)(struct platform_device *pdev); >> }; >> >> -#if defined(CONFIG_OMAP_REMOTEPROC) || defined(CONFIG_OMAP_REMOTEPROC_MODULE) >> - >> -void __init omap_rproc_reserve_cma(void); >> - >> -#else >> - >> -static inline void __init omap_rproc_reserve_cma(void) >> -{ >> -} >> - >> -#endif >> - >> #endif /* _PLAT_REMOTEPROC_H */ >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 20/12/2019 01:43, Mathieu Poirier wrote: > On Fri, Dec 13, 2019 at 02:55:35PM +0200, Tero Kristo wrote: >> From: Suman Anna <s-anna@ti.com> >> >> This patch enhances the PM support in the OMAP remoteproc driver to >> support the runtime auto-suspend. A remoteproc may not be required to >> be running all the time, and typically will need to be active only >> during certain usecases. As such, to save power, it should be turned >> off during potential long periods of inactivity between usecases. >> This suspend and resume of the device is a relatively heavy process >> in terms of latencies, so a remoteproc should be suspended only after >> a certain period of prolonged inactivity. The OMAP remoteproc driver >> leverages the runtime pm framework's auto_suspend feature to accomplish >> this functionality. This feature is automatically enabled when a remote >> processor has successfully booted. The 'autosuspend_delay_ms' for each >> device dictates the inactivity period/time to wait for before >> suspending the device. >> >> The runtime auto-suspend design relies on marking the last busy time >> on every communication (virtqueue kick) to and from the remote processor. >> When there has been no activity for 'autosuspend_delay_ms' time, the >> runtime PM framework invokes the driver's runtime pm suspend callback >> to suspend the device. The remote processor will be woken up on the >> initiation of the next communication message through the runtime pm >> resume callback. The current auto-suspend design also allows a remote >> processor to deny a auto-suspend attempt, if it wishes to, by sending a >> NACK response to the initial suspend request message sent to the remote >> processor as part of the suspend process. The auto-suspend request is >> also only attempted if the remote processor is idled and in standby at >> the time of inactivity timer expiry. This choice is made to avoid >> unnecessary messaging, and the auto-suspend is simply rescheduled to >> be attempted again after a further lapse of autosuspend_delay_ms. >> >> The runtime pm callbacks functionality in this patch reuses most of the >> core logic from the suspend/resume support code, and make use of an >> additional auto_suspend flag to differentiate the logic in common code >> from system suspend. The system suspend/resume sequences are also updated >> to reflect the proper pm_runtime statuses, and also to really perform a >> suspend/resume only if the remoteproc has not been auto-suspended at the >> time of request. The remote processor is left in suspended state on a >> system resume if it has been auto-suspended before, and will be woken up >> only when a usecase needs to run. The other significant change in this >> patch is to reset the remoteproc device's pm_domain so as to avoid >> conflicts with the ordering sequences in the device pm_domain's runtime >> callbacks and the reset management and clock management implemented >> within the runtime callbacks in the driver. >> >> The OMAP remoteproc driver currently uses a default value of 10 seconds >> for all OMAP remoteprocs, and a different value can be chosen either by >> choosing a positive value for the 'autosuspend_delay' in the device's >> omap_rproc_fw_data in the driver match data or by updating the >> 'autosuspend_delay_ms' field at runtime through the sysfs interface. >> Eg: To use 25 seconds for IPU2 on DRA7xx, >> echo 25000 > /sys/bus/platform/devices/55020000.ipu/power/autosuspend_delay_ms >> >> The runtime suspend feature can also be similarly enabled or disabled by >> writing 'auto' or 'on' to the device's 'control' power field. The default >> is enabled. >> Eg: To disable auto-suspend for IPU2 on DRA7xx SoC, >> echo on > /sys/bus/platform/devices/55020000.ipu/power/control >> >> 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 | 220 ++++++++++++++++++++++++++- >> 1 file changed, 214 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >> index 0a9b9f7d20da..463d6f60947a 100644 >> --- a/drivers/remoteproc/omap_remoteproc.c >> +++ b/drivers/remoteproc/omap_remoteproc.c >> @@ -20,6 +20,7 @@ >> #include <linux/of_device.h> >> #include <linux/of_reserved_mem.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/dma-mapping.h> >> #include <linux/remoteproc.h> >> #include <linux/mailbox_client.h> >> @@ -37,6 +38,9 @@ >> #include "omap_remoteproc.h" >> #include "remoteproc_internal.h" >> >> +/* default auto-suspend delay (ms) */ >> +#define DEFAULT_AUTOSUSPEND_DELAY 10000 >> + >> /** >> * struct omap_rproc_boot_data - boot data structure for the DSP omap rprocs >> * @syscon: regmap handle for the system control configuration module >> @@ -83,6 +87,8 @@ struct omap_rproc_timer { >> * @num_mems: number of internal memory regions >> * @num_timers: number of rproc timer(s) >> * @timers: timer(s) info used by rproc >> + * @autosuspend_delay: auto-suspend delay value to be used for runtime pm >> + * @need_resume: if true a resume is needed in the system resume callback >> * @rproc: rproc handle >> * @reset: reset handle >> * @pm_comp: completion primitive to sync for suspend response >> @@ -97,6 +103,8 @@ struct omap_rproc { >> int num_mems; >> int num_timers; >> struct omap_rproc_timer *timers; >> + int autosuspend_delay; >> + bool need_resume; >> struct rproc *rproc; >> struct reset_control *reset; >> struct completion pm_comp; >> @@ -111,6 +119,7 @@ struct omap_rproc { >> * @boot_reg_shift: bit shift for the boot register mask >> * @mem_names: memory names for this remote processor >> * @dev_addrs: device addresses corresponding to the memory names >> + * @autosuspend_delay: custom auto-suspend delay value in milliseconds >> */ >> struct omap_rproc_dev_data { >> const char *device_name; >> @@ -118,6 +127,7 @@ struct omap_rproc_dev_data { >> int boot_reg_shift; >> const char * const *mem_names; >> const u32 *dev_addrs; >> + int autosuspend_delay; >> }; >> >> /** >> @@ -384,11 +394,23 @@ static void omap_rproc_kick(struct rproc *rproc, int vqid) >> struct device *dev = rproc->dev.parent; >> int ret; >> >> + /* wake up the rproc before kicking it */ >> + ret = pm_runtime_get_sync(dev); >> + if (WARN_ON(ret < 0)) { >> + dev_err(dev, "pm_runtime_get_sync() failed during kick, ret = %d\n", >> + ret); >> + pm_runtime_put_noidle(dev); >> + return; >> + } >> + >> /* send the index of the triggered virtqueue in the mailbox payload */ >> ret = mbox_send_message(oproc->mbox, (void *)vqid); >> if (ret < 0) >> dev_err(dev, "failed to send mailbox message, status = %d\n", >> ret); >> + >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> } >> >> /** >> @@ -473,6 +495,19 @@ static int omap_rproc_start(struct rproc *rproc) >> goto put_mbox; >> } >> >> + /* >> + * remote processor is up, so update the runtime pm status and >> + * enable the auto-suspend. The device usage count is incremented >> + * manually for balancing it for auto-suspend >> + */ >> + pm_runtime_set_active(dev); >> + pm_runtime_set_autosuspend_delay(dev, oproc->autosuspend_delay); >> + pm_runtime_use_autosuspend(dev); >> + pm_runtime_get(dev); >> + pm_runtime_enable(dev); >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_put_autosuspend(dev); >> + >> reset_control_deassert(oproc->reset); >> >> return 0; >> @@ -485,9 +520,26 @@ static int omap_rproc_start(struct rproc *rproc) >> /* power off the remote processor */ >> static int omap_rproc_stop(struct rproc *rproc) >> { >> + struct device *dev = rproc->dev.parent; >> struct omap_rproc *oproc = rproc->priv; >> int ret; >> >> + /* >> + * cancel any possible scheduled runtime suspend by incrementing >> + * the device usage count, and resuming the device. The remoteproc >> + * also needs to be woken up if suspended, to avoid the remoteproc >> + * OS to continue to remember any context that it has saved, and >> + * avoid potential issues in misindentifying a subsequent device >> + * reboot as a power restore boot >> + */ >> + ret = pm_runtime_get_sync(dev); >> + if (ret < 0) { >> + pm_runtime_put_noidle(dev); >> + return ret; >> + } >> + >> + pm_runtime_put_sync(dev); >> + >> reset_control_assert(oproc->reset); >> >> ret = omap_rproc_disable_timers(rproc, true); >> @@ -496,6 +548,15 @@ static int omap_rproc_stop(struct rproc *rproc) >> >> mbox_free_channel(oproc->mbox); >> >> + /* >> + * update the runtime pm states and status now that the remoteproc >> + * has stopped >> + */ >> + pm_runtime_disable(dev); >> + pm_runtime_dont_use_autosuspend(dev); >> + pm_runtime_put_noidle(dev); >> + pm_runtime_set_suspended(dev); >> + >> return 0; >> } >> >> @@ -552,17 +613,19 @@ static bool _is_rproc_in_standby(struct omap_rproc *oproc) >> >> /* 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) >> +static int _omap_rproc_suspend(struct rproc *rproc, bool auto_suspend) >> { >> 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; >> + u32 suspend_msg = auto_suspend ? >> + RP_MBOX_SUSPEND_AUTO : RP_MBOX_SUSPEND_SYSTEM; >> int ret; >> >> reinit_completion(&oproc->pm_comp); >> oproc->suspend_acked = false; >> - ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); >> + ret = mbox_send_message(oproc->mbox, (void *)suspend_msg); >> if (ret < 0) { >> dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >> return ret; >> @@ -602,25 +665,55 @@ static int _omap_rproc_suspend(struct rproc *rproc) >> goto enable_device; >> } >> >> + /* >> + * IOMMUs would have to be disabled specifically for runtime suspend. >> + * They are handled automatically through System PM callbacks for >> + * regular system suspend >> + */ >> + if (auto_suspend) { >> + ret = omap_iommu_domain_deactivate(rproc->domain); >> + if (ret) { >> + dev_err(dev, "iommu domain deactivate failed %d\n", >> + ret); >> + goto enable_timers; >> + } >> + } >> + >> return 0; >> +enable_timers: >> + /* ignore errors on re-enabling code */ >> + omap_rproc_enable_timers(rproc, false); >> >> enable_device: >> reset_control_deassert(oproc->reset); >> return ret; >> } >> >> -static int _omap_rproc_resume(struct rproc *rproc) >> +static int _omap_rproc_resume(struct rproc *rproc, bool auto_suspend) >> { >> struct device *dev = rproc->dev.parent; >> struct omap_rproc *oproc = rproc->priv; >> int ret; >> >> + /* >> + * IOMMUs would have to be enabled specifically for runtime resume. >> + * They would have been already enabled automatically through System >> + * PM callbacks for regular system resume >> + */ >> + if (auto_suspend) { >> + ret = omap_iommu_domain_activate(rproc->domain); >> + if (ret) { >> + dev_err(dev, "omap_iommu activate failed %d\n", ret); >> + goto out; >> + } >> + } >> + >> /* 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; >> + goto suspend_iommu; > > The same needs to be done if omap_rproc_enable_timers() fails. Ok, will enhance the error handling logic a bit for v4. > >> } >> } >> >> @@ -633,6 +726,12 @@ static int _omap_rproc_resume(struct rproc *rproc) >> >> reset_control_deassert(oproc->reset); >> >> + return 0; >> + >> +suspend_iommu: >> + if (auto_suspend) >> + omap_iommu_domain_deactivate(rproc->domain); >> + >> out: >> return ret; >> } >> @@ -641,6 +740,7 @@ 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); >> + struct omap_rproc *oproc = rproc->priv; >> int ret = 0; >> >> mutex_lock(&rproc->lock); >> @@ -655,13 +755,25 @@ static int __maybe_unused omap_rproc_suspend(struct device *dev) >> goto out; >> } >> >> - ret = _omap_rproc_suspend(rproc); >> + ret = _omap_rproc_suspend(rproc, false); >> if (ret) { >> dev_err(dev, "suspend failed %d\n", ret); >> goto out; >> } >> >> + /* >> + * remoteproc is running at the time of system suspend, so remember >> + * it so as to wake it up during system resume >> + */ >> + oproc->need_resume = 1; > > Please use 'true' to be consistent with the type and omap_rproc_resume(). Right. > >> rproc->state = RPROC_SUSPENDED; >> + >> + /* >> + * update the runtime pm status to be suspended, without decrementing >> + * the device usage count >> + */ >> + pm_runtime_disable(dev); >> + pm_runtime_set_suspended(dev); >> out: >> mutex_unlock(&rproc->lock); >> return ret; >> @@ -671,6 +783,7 @@ 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); >> + struct omap_rproc *oproc = rproc->priv; >> int ret = 0; >> >> mutex_lock(&rproc->lock); >> @@ -682,17 +795,91 @@ static int __maybe_unused omap_rproc_resume(struct device *dev) >> goto out; >> } >> >> - ret = _omap_rproc_resume(rproc); >> + /* >> + * remoteproc was auto-suspended at the time of system suspend, >> + * so no need to wake-up the processor (leave it in suspended >> + * state, will be woken up during a subsequent runtime_resume) >> + */ >> + if (!oproc->need_resume) >> + goto out; >> + >> + ret = _omap_rproc_resume(rproc, false); >> if (ret) { >> dev_err(dev, "resume failed %d\n", ret); >> goto out; >> } >> + oproc->need_resume = false; >> >> rproc->state = RPROC_RUNNING; >> + >> + /* >> + * update the runtime pm status to be active, without incrementing >> + * the device usage count >> + */ >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + pm_runtime_mark_last_busy(dev); >> out: >> mutex_unlock(&rproc->lock); >> return ret; >> } >> + >> +static int omap_rproc_runtime_suspend(struct device *dev) >> +{ >> + struct rproc *rproc = dev_get_drvdata(dev); >> + struct omap_rproc *oproc = rproc->priv; >> + int ret; >> + >> + if (rproc->state == RPROC_CRASHED) { >> + dev_dbg(dev, "rproc cannot be runtime suspended when crashed!\n"); >> + return -EBUSY; >> + } >> + >> + if (WARN_ON(rproc->state != RPROC_RUNNING)) { >> + dev_err(dev, "rproc cannot be runtime suspended when not running!\n"); >> + return -EBUSY; >> + } >> + >> + /* >> + * do not even attempt suspend if the remote processor is not >> + * idled for runtime auto-suspend >> + */ >> + if (!_is_rproc_in_standby(oproc)) { >> + ret = -EBUSY; >> + goto abort; >> + } >> + >> + ret = _omap_rproc_suspend(rproc, true); >> + if (ret) >> + goto abort; >> + >> + rproc->state = RPROC_SUSPENDED; >> + return 0; >> + >> +abort: >> + pm_runtime_mark_last_busy(dev); >> + return ret; >> +} >> + >> +static int omap_rproc_runtime_resume(struct device *dev) >> +{ >> + struct rproc *rproc = dev_get_drvdata(dev); >> + int ret; >> + >> + if (WARN_ON(rproc->state != RPROC_SUSPENDED)) { >> + dev_err(dev, "rproc cannot be runtime resumed if not suspended!\n"); >> + return -EBUSY; >> + } >> + >> + ret = _omap_rproc_resume(rproc, true); >> + if (ret) { >> + dev_err(dev, "runtime resume failed %d\n", ret); >> + return ret; >> + } >> + >> + rproc->state = RPROC_RUNNING; >> + return 0; >> +} >> #endif /* CONFIG_PM */ >> >> static const char * const ipu_mem_names[] = { >> @@ -778,6 +965,20 @@ static const struct of_device_id omap_rproc_of_match[] = { >> }; >> MODULE_DEVICE_TABLE(of, omap_rproc_of_match); >> >> +static int omap_rproc_get_autosuspend_delay(struct platform_device *pdev) >> +{ >> + const struct omap_rproc_dev_data *data; >> + int delay; >> + >> + data = of_device_get_match_data(&pdev->dev); >> + if (!data) >> + return -ENODEV; > > This check is done in omap_rproc_of_get_internal_memories() and > omap_rproc_get_boot_data(). I think it would be best to do it once at the top > of the probe() function and be done with it. > > That being said and as noted in a previous comment, I would push all tuneables > to the DT. If the property is missing then things default to > DEFAULT_AUTOSUSPEND_DELAY. Ok, let me do that. -Tero > >> + >> + delay = data->autosuspend_delay; >> + >> + return (delay > 0) ? delay : DEFAULT_AUTOSUSPEND_DELAY; >> +} >> + >> static const char *omap_rproc_get_firmware(struct platform_device *pdev) >> { >> const char *fw_name; >> @@ -953,6 +1154,11 @@ static int omap_rproc_probe(struct platform_device *pdev) >> } >> >> init_completion(&oproc->pm_comp); >> + oproc->autosuspend_delay = omap_rproc_get_autosuspend_delay(pdev); >> + if (oproc->autosuspend_delay < 0) { >> + ret = oproc->autosuspend_delay; >> + goto free_rproc; >> + } >> >> oproc->fck = devm_clk_get(&pdev->dev, 0); >> if (IS_ERR(oproc->fck)) { >> @@ -994,6 +1200,8 @@ static int omap_rproc_remove(struct platform_device *pdev) >> >> static const struct dev_pm_ops omap_rproc_pm_ops = { >> SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >> + SET_RUNTIME_PM_OPS(omap_rproc_runtime_suspend, >> + omap_rproc_runtime_resume, NULL) >> }; >> >> static struct platform_driver omap_rproc_driver = { >> -- >> 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:24, Suman Anna wrote: > Hi Tero, > > On 12/19/19 5:43 PM, Mathieu Poirier wrote: >> On Fri, Dec 13, 2019 at 02:55:35PM +0200, Tero Kristo wrote: >>> From: Suman Anna <s-anna@ti.com> >>> >>> This patch enhances the PM support in the OMAP remoteproc driver to >>> support the runtime auto-suspend. A remoteproc may not be required to >>> be running all the time, and typically will need to be active only >>> during certain usecases. As such, to save power, it should be turned >>> off during potential long periods of inactivity between usecases. >>> This suspend and resume of the device is a relatively heavy process >>> in terms of latencies, so a remoteproc should be suspended only after >>> a certain period of prolonged inactivity. The OMAP remoteproc driver >>> leverages the runtime pm framework's auto_suspend feature to accomplish >>> this functionality. This feature is automatically enabled when a remote >>> processor has successfully booted. The 'autosuspend_delay_ms' for each >>> device dictates the inactivity period/time to wait for before >>> suspending the device. >>> >>> The runtime auto-suspend design relies on marking the last busy time >>> on every communication (virtqueue kick) to and from the remote processor. >>> When there has been no activity for 'autosuspend_delay_ms' time, the >>> runtime PM framework invokes the driver's runtime pm suspend callback >>> to suspend the device. The remote processor will be woken up on the >>> initiation of the next communication message through the runtime pm >>> resume callback. The current auto-suspend design also allows a remote >>> processor to deny a auto-suspend attempt, if it wishes to, by sending a >>> NACK response to the initial suspend request message sent to the remote >>> processor as part of the suspend process. The auto-suspend request is >>> also only attempted if the remote processor is idled and in standby at >>> the time of inactivity timer expiry. This choice is made to avoid >>> unnecessary messaging, and the auto-suspend is simply rescheduled to >>> be attempted again after a further lapse of autosuspend_delay_ms. >>> >>> The runtime pm callbacks functionality in this patch reuses most of the >>> core logic from the suspend/resume support code, and make use of an >>> additional auto_suspend flag to differentiate the logic in common code >>> from system suspend. The system suspend/resume sequences are also updated >>> to reflect the proper pm_runtime statuses, and also to really perform a >>> suspend/resume only if the remoteproc has not been auto-suspended at the >>> time of request. The remote processor is left in suspended state on a >>> system resume if it has been auto-suspended before, and will be woken up >>> only when a usecase needs to run. The other significant change in this >>> patch is to reset the remoteproc device's pm_domain so as to avoid >>> conflicts with the ordering sequences in the device pm_domain's runtime >>> callbacks and the reset management and clock management implemented >>> within the runtime callbacks in the driver. >>> >>> The OMAP remoteproc driver currently uses a default value of 10 seconds >>> for all OMAP remoteprocs, and a different value can be chosen either by >>> choosing a positive value for the 'autosuspend_delay' in the device's >>> omap_rproc_fw_data in the driver match data or by updating the >>> 'autosuspend_delay_ms' field at runtime through the sysfs interface. >>> Eg: To use 25 seconds for IPU2 on DRA7xx, >>> echo 25000 > /sys/bus/platform/devices/55020000.ipu/power/autosuspend_delay_ms >>> >>> The runtime suspend feature can also be similarly enabled or disabled by >>> writing 'auto' or 'on' to the device's 'control' power field. The default >>> is enabled. >>> Eg: To disable auto-suspend for IPU2 on DRA7xx SoC, >>> echo on > /sys/bus/platform/devices/55020000.ipu/power/control >>> >>> 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 | 220 ++++++++++++++++++++++++++- >>> 1 file changed, 214 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c >>> index 0a9b9f7d20da..463d6f60947a 100644 >>> --- a/drivers/remoteproc/omap_remoteproc.c >>> +++ b/drivers/remoteproc/omap_remoteproc.c >>> @@ -20,6 +20,7 @@ >>> #include <linux/of_device.h> >>> #include <linux/of_reserved_mem.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_runtime.h> >>> #include <linux/dma-mapping.h> >>> #include <linux/remoteproc.h> >>> #include <linux/mailbox_client.h> >>> @@ -37,6 +38,9 @@ >>> #include "omap_remoteproc.h" >>> #include "remoteproc_internal.h" >>> >>> +/* default auto-suspend delay (ms) */ >>> +#define DEFAULT_AUTOSUSPEND_DELAY 10000 >>> + >>> /** >>> * struct omap_rproc_boot_data - boot data structure for the DSP omap rprocs >>> * @syscon: regmap handle for the system control configuration module >>> @@ -83,6 +87,8 @@ struct omap_rproc_timer { >>> * @num_mems: number of internal memory regions >>> * @num_timers: number of rproc timer(s) >>> * @timers: timer(s) info used by rproc >>> + * @autosuspend_delay: auto-suspend delay value to be used for runtime pm >>> + * @need_resume: if true a resume is needed in the system resume callback >>> * @rproc: rproc handle >>> * @reset: reset handle >>> * @pm_comp: completion primitive to sync for suspend response >>> @@ -97,6 +103,8 @@ struct omap_rproc { >>> int num_mems; >>> int num_timers; >>> struct omap_rproc_timer *timers; >>> + int autosuspend_delay; >>> + bool need_resume; >>> struct rproc *rproc; >>> struct reset_control *reset; >>> struct completion pm_comp; >>> @@ -111,6 +119,7 @@ struct omap_rproc { >>> * @boot_reg_shift: bit shift for the boot register mask >>> * @mem_names: memory names for this remote processor >>> * @dev_addrs: device addresses corresponding to the memory names >>> + * @autosuspend_delay: custom auto-suspend delay value in milliseconds >>> */ >>> struct omap_rproc_dev_data { >>> const char *device_name; >>> @@ -118,6 +127,7 @@ struct omap_rproc_dev_data { >>> int boot_reg_shift; >>> const char * const *mem_names; >>> const u32 *dev_addrs; >>> + int autosuspend_delay; >>> }; >>> >>> /** >>> @@ -384,11 +394,23 @@ static void omap_rproc_kick(struct rproc *rproc, int vqid) >>> struct device *dev = rproc->dev.parent; >>> int ret; >>> >>> + /* wake up the rproc before kicking it */ >>> + ret = pm_runtime_get_sync(dev); >>> + if (WARN_ON(ret < 0)) { >>> + dev_err(dev, "pm_runtime_get_sync() failed during kick, ret = %d\n", >>> + ret); >>> + pm_runtime_put_noidle(dev); >>> + return; >>> + } >>> + >>> /* send the index of the triggered virtqueue in the mailbox payload */ >>> ret = mbox_send_message(oproc->mbox, (void *)vqid); >>> if (ret < 0) >>> dev_err(dev, "failed to send mailbox message, status = %d\n", >>> ret); >>> + >>> + pm_runtime_mark_last_busy(dev); >>> + pm_runtime_put_autosuspend(dev); >>> } >>> >>> /** >>> @@ -473,6 +495,19 @@ static int omap_rproc_start(struct rproc *rproc) >>> goto put_mbox; >>> } >>> >>> + /* >>> + * remote processor is up, so update the runtime pm status and >>> + * enable the auto-suspend. The device usage count is incremented >>> + * manually for balancing it for auto-suspend >>> + */ >>> + pm_runtime_set_active(dev); >>> + pm_runtime_set_autosuspend_delay(dev, oproc->autosuspend_delay); >>> + pm_runtime_use_autosuspend(dev); >>> + pm_runtime_get(dev); >>> + pm_runtime_enable(dev); >>> + pm_runtime_mark_last_busy(dev); >>> + pm_runtime_put_autosuspend(dev); >>> + >>> reset_control_deassert(oproc->reset); > > I see that you have flipped the reset call and all the pm_runtime calls > (w.r.t my original code sequence) and pm_runtime_get instead of > pm_runtime_get_noresume(). Is there a reason for it? What is the backend > that gets exercised with pm_runtime? PM runtime nowadays exercises ti-sysc, and we can't deassert reset before we enable the clocks for the device. Thus, these are flipped. In the old incarnation of this code, PM runtime just exercised the hacked up functionality implemented for omap rproc alone. > >>> >>> return 0; >>> @@ -485,9 +520,26 @@ static int omap_rproc_start(struct rproc *rproc) >>> /* power off the remote processor */ >>> static int omap_rproc_stop(struct rproc *rproc) >>> { >>> + struct device *dev = rproc->dev.parent; >>> struct omap_rproc *oproc = rproc->priv; >>> int ret; >>> >>> + /* >>> + * cancel any possible scheduled runtime suspend by incrementing >>> + * the device usage count, and resuming the device. The remoteproc >>> + * also needs to be woken up if suspended, to avoid the remoteproc >>> + * OS to continue to remember any context that it has saved, and >>> + * avoid potential issues in misindentifying a subsequent device >>> + * reboot as a power restore boot >>> + */ >>> + ret = pm_runtime_get_sync(dev); >>> + if (ret < 0) { >>> + pm_runtime_put_noidle(dev); >>> + return ret; >>> + } >>> + >>> + pm_runtime_put_sync(dev); >>> + > > I didn't have this call either. And get_sync() followed by put_sync() is > essentially a no-op. Am I missing something here? Hmm right, but you did have the device_shutdown() here, somehow this appears to simulate that, but you are right, it is just a no-op. I don't know if this code has actually ever done anything useful. Let me experiment with this one a bit and see what happens. It looks like it should be removed completely. > >>> reset_control_assert(oproc->reset); >>> >>> ret = omap_rproc_disable_timers(rproc, true); >>> @@ -496,6 +548,15 @@ static int omap_rproc_stop(struct rproc *rproc) >>> >>> mbox_free_channel(oproc->mbox); >>> >>> + /* >>> + * update the runtime pm states and status now that the remoteproc >>> + * has stopped >>> + */ >>> + pm_runtime_disable(dev); >>> + pm_runtime_dont_use_autosuspend(dev); >>> + pm_runtime_put_noidle(dev); >>> + pm_runtime_set_suspended(dev); >>> + >>> return 0; >>> } >>> >>> @@ -552,17 +613,19 @@ static bool _is_rproc_in_standby(struct omap_rproc *oproc) >>> >>> /* 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) >>> +static int _omap_rproc_suspend(struct rproc *rproc, bool auto_suspend) >>> { >>> 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; >>> + u32 suspend_msg = auto_suspend ? >>> + RP_MBOX_SUSPEND_AUTO : RP_MBOX_SUSPEND_SYSTEM; >>> int ret; >>> >>> reinit_completion(&oproc->pm_comp); >>> oproc->suspend_acked = false; >>> - ret = mbox_send_message(oproc->mbox, (void *)RP_MBOX_SUSPEND_SYSTEM); >>> + ret = mbox_send_message(oproc->mbox, (void *)suspend_msg); >>> if (ret < 0) { >>> dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >>> return ret; >>> @@ -602,25 +665,55 @@ static int _omap_rproc_suspend(struct rproc *rproc) >>> goto enable_device; >>> } >>> >>> + /* >>> + * IOMMUs would have to be disabled specifically for runtime suspend. >>> + * They are handled automatically through System PM callbacks for >>> + * regular system suspend >>> + */ >>> + if (auto_suspend) { >>> + ret = omap_iommu_domain_deactivate(rproc->domain); >>> + if (ret) { >>> + dev_err(dev, "iommu domain deactivate failed %d\n", >>> + ret); >>> + goto enable_timers; >>> + } >>> + } >>> + >>> return 0; > > blank line here, and remove the one before enable_device. Hmm why? I don't think there is any coding guideline that would dictate this. However, I'll fix this as this is effectively your file anyways. -Tero > > regards > Suman > >>> +enable_timers: >>> + /* ignore errors on re-enabling code */ >>> + omap_rproc_enable_timers(rproc, false); >>> >>> enable_device: >>> reset_control_deassert(oproc->reset); >>> return ret; >>> } >>> >>> -static int _omap_rproc_resume(struct rproc *rproc) >>> +static int _omap_rproc_resume(struct rproc *rproc, bool auto_suspend) >>> { >>> struct device *dev = rproc->dev.parent; >>> struct omap_rproc *oproc = rproc->priv; >>> int ret; >>> >>> + /* >>> + * IOMMUs would have to be enabled specifically for runtime resume. >>> + * They would have been already enabled automatically through System >>> + * PM callbacks for regular system resume >>> + */ >>> + if (auto_suspend) { >>> + ret = omap_iommu_domain_activate(rproc->domain); >>> + if (ret) { >>> + dev_err(dev, "omap_iommu activate failed %d\n", ret); >>> + goto out; >>> + } >>> + } >>> + >>> /* 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; >>> + goto suspend_iommu; >> >> The same needs to be done if omap_rproc_enable_timers() fails. >> >>> } >>> } >>> >>> @@ -633,6 +726,12 @@ static int _omap_rproc_resume(struct rproc *rproc) >>> >>> reset_control_deassert(oproc->reset); >>> >>> + return 0; >>> + >>> +suspend_iommu: >>> + if (auto_suspend) >>> + omap_iommu_domain_deactivate(rproc->domain); >>> + >>> out: >>> return ret; >>> } >>> @@ -641,6 +740,7 @@ 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); >>> + struct omap_rproc *oproc = rproc->priv; >>> int ret = 0; >>> >>> mutex_lock(&rproc->lock); >>> @@ -655,13 +755,25 @@ static int __maybe_unused omap_rproc_suspend(struct device *dev) >>> goto out; >>> } >>> >>> - ret = _omap_rproc_suspend(rproc); >>> + ret = _omap_rproc_suspend(rproc, false); >>> if (ret) { >>> dev_err(dev, "suspend failed %d\n", ret); >>> goto out; >>> } >>> >>> + /* >>> + * remoteproc is running at the time of system suspend, so remember >>> + * it so as to wake it up during system resume >>> + */ >>> + oproc->need_resume = 1; >> >> Please use 'true' to be consistent with the type and omap_rproc_resume(). >> >>> rproc->state = RPROC_SUSPENDED; >>> + >>> + /* >>> + * update the runtime pm status to be suspended, without decrementing >>> + * the device usage count >>> + */ >>> + pm_runtime_disable(dev); >>> + pm_runtime_set_suspended(dev); >>> out: >>> mutex_unlock(&rproc->lock); >>> return ret; >>> @@ -671,6 +783,7 @@ 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); >>> + struct omap_rproc *oproc = rproc->priv; >>> int ret = 0; >>> >>> mutex_lock(&rproc->lock); >>> @@ -682,17 +795,91 @@ static int __maybe_unused omap_rproc_resume(struct device *dev) >>> goto out; >>> } >>> >>> - ret = _omap_rproc_resume(rproc); >>> + /* >>> + * remoteproc was auto-suspended at the time of system suspend, >>> + * so no need to wake-up the processor (leave it in suspended >>> + * state, will be woken up during a subsequent runtime_resume) >>> + */ >>> + if (!oproc->need_resume) >>> + goto out; >>> + >>> + ret = _omap_rproc_resume(rproc, false); >>> if (ret) { >>> dev_err(dev, "resume failed %d\n", ret); >>> goto out; >>> } >>> + oproc->need_resume = false; >>> >>> rproc->state = RPROC_RUNNING; >>> + >>> + /* >>> + * update the runtime pm status to be active, without incrementing >>> + * the device usage count >>> + */ >>> + pm_runtime_set_active(dev); >>> + pm_runtime_enable(dev); >>> + pm_runtime_mark_last_busy(dev); >>> out: >>> mutex_unlock(&rproc->lock); >>> return ret; >>> } >>> + >>> +static int omap_rproc_runtime_suspend(struct device *dev) >>> +{ >>> + struct rproc *rproc = dev_get_drvdata(dev); >>> + struct omap_rproc *oproc = rproc->priv; >>> + int ret; >>> + >>> + if (rproc->state == RPROC_CRASHED) { >>> + dev_dbg(dev, "rproc cannot be runtime suspended when crashed!\n"); >>> + return -EBUSY; >>> + } >>> + >>> + if (WARN_ON(rproc->state != RPROC_RUNNING)) { >>> + dev_err(dev, "rproc cannot be runtime suspended when not running!\n"); >>> + return -EBUSY; >>> + } >>> + >>> + /* >>> + * do not even attempt suspend if the remote processor is not >>> + * idled for runtime auto-suspend >>> + */ >>> + if (!_is_rproc_in_standby(oproc)) { >>> + ret = -EBUSY; >>> + goto abort; >>> + } >>> + >>> + ret = _omap_rproc_suspend(rproc, true); >>> + if (ret) >>> + goto abort; >>> + >>> + rproc->state = RPROC_SUSPENDED; >>> + return 0; >>> + >>> +abort: >>> + pm_runtime_mark_last_busy(dev); >>> + return ret; >>> +} >>> + >>> +static int omap_rproc_runtime_resume(struct device *dev) >>> +{ >>> + struct rproc *rproc = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + if (WARN_ON(rproc->state != RPROC_SUSPENDED)) { >>> + dev_err(dev, "rproc cannot be runtime resumed if not suspended!\n"); >>> + return -EBUSY; >>> + } >>> + >>> + ret = _omap_rproc_resume(rproc, true); >>> + if (ret) { >>> + dev_err(dev, "runtime resume failed %d\n", ret); >>> + return ret; >>> + } >>> + >>> + rproc->state = RPROC_RUNNING; >>> + return 0; >>> +} >>> #endif /* CONFIG_PM */ >>> >>> static const char * const ipu_mem_names[] = { >>> @@ -778,6 +965,20 @@ static const struct of_device_id omap_rproc_of_match[] = { >>> }; >>> MODULE_DEVICE_TABLE(of, omap_rproc_of_match); >>> >>> +static int omap_rproc_get_autosuspend_delay(struct platform_device *pdev) >>> +{ >>> + const struct omap_rproc_dev_data *data; >>> + int delay; >>> + >>> + data = of_device_get_match_data(&pdev->dev); >>> + if (!data) >>> + return -ENODEV; >> >> This check is done in omap_rproc_of_get_internal_memories() and >> omap_rproc_get_boot_data(). I think it would be best to do it once at the top >> of the probe() function and be done with it. >> >> That being said and as noted in a previous comment, I would push all tuneables >> to the DT. If the property is missing then things default to >> DEFAULT_AUTOSUSPEND_DELAY. >> >>> + >>> + delay = data->autosuspend_delay; >>> + >>> + return (delay > 0) ? delay : DEFAULT_AUTOSUSPEND_DELAY; >>> +} >>> + >>> static const char *omap_rproc_get_firmware(struct platform_device *pdev) >>> { >>> const char *fw_name; >>> @@ -953,6 +1154,11 @@ static int omap_rproc_probe(struct platform_device *pdev) >>> } >>> >>> init_completion(&oproc->pm_comp); >>> + oproc->autosuspend_delay = omap_rproc_get_autosuspend_delay(pdev); >>> + if (oproc->autosuspend_delay < 0) { >>> + ret = oproc->autosuspend_delay; >>> + goto free_rproc; >>> + } >>> >>> oproc->fck = devm_clk_get(&pdev->dev, 0); >>> if (IS_ERR(oproc->fck)) { >>> @@ -994,6 +1200,8 @@ static int omap_rproc_remove(struct platform_device *pdev) >>> >>> static const struct dev_pm_ops omap_rproc_pm_ops = { >>> SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >>> + SET_RUNTIME_PM_OPS(omap_rproc_runtime_suspend, >>> + omap_rproc_runtime_resume, NULL) >>> }; >>> >>> static struct platform_driver omap_rproc_driver = { >>> -- >>> 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:24 AM, Tero Kristo wrote: > On 20/12/2019 05:24, Suman Anna wrote: >> Hi Tero, >> >> On 12/19/19 5:43 PM, Mathieu Poirier wrote: >>> On Fri, Dec 13, 2019 at 02:55:35PM +0200, Tero Kristo wrote: >>>> From: Suman Anna <s-anna@ti.com> >>>> >>>> This patch enhances the PM support in the OMAP remoteproc driver to >>>> support the runtime auto-suspend. A remoteproc may not be required to >>>> be running all the time, and typically will need to be active only >>>> during certain usecases. As such, to save power, it should be turned >>>> off during potential long periods of inactivity between usecases. >>>> This suspend and resume of the device is a relatively heavy process >>>> in terms of latencies, so a remoteproc should be suspended only after >>>> a certain period of prolonged inactivity. The OMAP remoteproc driver >>>> leverages the runtime pm framework's auto_suspend feature to accomplish >>>> this functionality. This feature is automatically enabled when a remote >>>> processor has successfully booted. The 'autosuspend_delay_ms' for each >>>> device dictates the inactivity period/time to wait for before >>>> suspending the device. >>>> >>>> The runtime auto-suspend design relies on marking the last busy time >>>> on every communication (virtqueue kick) to and from the remote >>>> processor. >>>> When there has been no activity for 'autosuspend_delay_ms' time, the >>>> runtime PM framework invokes the driver's runtime pm suspend callback >>>> to suspend the device. The remote processor will be woken up on the >>>> initiation of the next communication message through the runtime pm >>>> resume callback. The current auto-suspend design also allows a remote >>>> processor to deny a auto-suspend attempt, if it wishes to, by sending a >>>> NACK response to the initial suspend request message sent to the remote >>>> processor as part of the suspend process. The auto-suspend request is >>>> also only attempted if the remote processor is idled and in standby at >>>> the time of inactivity timer expiry. This choice is made to avoid >>>> unnecessary messaging, and the auto-suspend is simply rescheduled to >>>> be attempted again after a further lapse of autosuspend_delay_ms. >>>> >>>> The runtime pm callbacks functionality in this patch reuses most of the >>>> core logic from the suspend/resume support code, and make use of an >>>> additional auto_suspend flag to differentiate the logic in common code >>>> from system suspend. The system suspend/resume sequences are also >>>> updated >>>> to reflect the proper pm_runtime statuses, and also to really perform a >>>> suspend/resume only if the remoteproc has not been auto-suspended at >>>> the >>>> time of request. The remote processor is left in suspended state on a >>>> system resume if it has been auto-suspended before, and will be >>>> woken up >>>> only when a usecase needs to run. The other significant change in this >>>> patch is to reset the remoteproc device's pm_domain so as to avoid >>>> conflicts with the ordering sequences in the device pm_domain's runtime >>>> callbacks and the reset management and clock management implemented >>>> within the runtime callbacks in the driver. >>>> >>>> The OMAP remoteproc driver currently uses a default value of 10 seconds >>>> for all OMAP remoteprocs, and a different value can be chosen either by >>>> choosing a positive value for the 'autosuspend_delay' in the device's >>>> omap_rproc_fw_data in the driver match data or by updating the >>>> 'autosuspend_delay_ms' field at runtime through the sysfs interface. >>>> Eg: To use 25 seconds for IPU2 on DRA7xx, >>>> echo 25000 > >>>> /sys/bus/platform/devices/55020000.ipu/power/autosuspend_delay_ms >>>> >>>> The runtime suspend feature can also be similarly enabled or >>>> disabled by >>>> writing 'auto' or 'on' to the device's 'control' power field. The >>>> default >>>> is enabled. >>>> Eg: To disable auto-suspend for IPU2 on DRA7xx SoC, >>>> echo on > /sys/bus/platform/devices/55020000.ipu/power/control >>>> >>>> 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 | 220 >>>> ++++++++++++++++++++++++++- >>>> 1 file changed, 214 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/remoteproc/omap_remoteproc.c >>>> b/drivers/remoteproc/omap_remoteproc.c >>>> index 0a9b9f7d20da..463d6f60947a 100644 >>>> --- a/drivers/remoteproc/omap_remoteproc.c >>>> +++ b/drivers/remoteproc/omap_remoteproc.c >>>> @@ -20,6 +20,7 @@ >>>> #include <linux/of_device.h> >>>> #include <linux/of_reserved_mem.h> >>>> #include <linux/platform_device.h> >>>> +#include <linux/pm_runtime.h> >>>> #include <linux/dma-mapping.h> >>>> #include <linux/remoteproc.h> >>>> #include <linux/mailbox_client.h> >>>> @@ -37,6 +38,9 @@ >>>> #include "omap_remoteproc.h" >>>> #include "remoteproc_internal.h" >>>> +/* default auto-suspend delay (ms) */ >>>> +#define DEFAULT_AUTOSUSPEND_DELAY 10000 >>>> + >>>> /** >>>> * struct omap_rproc_boot_data - boot data structure for the DSP >>>> omap rprocs >>>> * @syscon: regmap handle for the system control configuration module >>>> @@ -83,6 +87,8 @@ struct omap_rproc_timer { >>>> * @num_mems: number of internal memory regions >>>> * @num_timers: number of rproc timer(s) >>>> * @timers: timer(s) info used by rproc >>>> + * @autosuspend_delay: auto-suspend delay value to be used for >>>> runtime pm >>>> + * @need_resume: if true a resume is needed in the system resume >>>> callback >>>> * @rproc: rproc handle >>>> * @reset: reset handle >>>> * @pm_comp: completion primitive to sync for suspend response >>>> @@ -97,6 +103,8 @@ struct omap_rproc { >>>> int num_mems; >>>> int num_timers; >>>> struct omap_rproc_timer *timers; >>>> + int autosuspend_delay; >>>> + bool need_resume; >>>> struct rproc *rproc; >>>> struct reset_control *reset; >>>> struct completion pm_comp; >>>> @@ -111,6 +119,7 @@ struct omap_rproc { >>>> * @boot_reg_shift: bit shift for the boot register mask >>>> * @mem_names: memory names for this remote processor >>>> * @dev_addrs: device addresses corresponding to the memory names >>>> + * @autosuspend_delay: custom auto-suspend delay value in milliseconds >>>> */ >>>> struct omap_rproc_dev_data { >>>> const char *device_name; >>>> @@ -118,6 +127,7 @@ struct omap_rproc_dev_data { >>>> int boot_reg_shift; >>>> const char * const *mem_names; >>>> const u32 *dev_addrs; >>>> + int autosuspend_delay; >>>> }; >>>> /** >>>> @@ -384,11 +394,23 @@ static void omap_rproc_kick(struct rproc >>>> *rproc, int vqid) >>>> struct device *dev = rproc->dev.parent; >>>> int ret; >>>> + /* wake up the rproc before kicking it */ >>>> + ret = pm_runtime_get_sync(dev); >>>> + if (WARN_ON(ret < 0)) { >>>> + dev_err(dev, "pm_runtime_get_sync() failed during kick, ret >>>> = %d\n", >>>> + ret); >>>> + pm_runtime_put_noidle(dev); >>>> + return; >>>> + } >>>> + >>>> /* send the index of the triggered virtqueue in the mailbox >>>> payload */ >>>> ret = mbox_send_message(oproc->mbox, (void *)vqid); >>>> if (ret < 0) >>>> dev_err(dev, "failed to send mailbox message, status = %d\n", >>>> ret); >>>> + >>>> + pm_runtime_mark_last_busy(dev); >>>> + pm_runtime_put_autosuspend(dev); >>>> } >>>> /** >>>> @@ -473,6 +495,19 @@ static int omap_rproc_start(struct rproc *rproc) >>>> goto put_mbox; >>>> } >>>> + /* >>>> + * remote processor is up, so update the runtime pm status and >>>> + * enable the auto-suspend. The device usage count is incremented >>>> + * manually for balancing it for auto-suspend >>>> + */ >>>> + pm_runtime_set_active(dev); >>>> + pm_runtime_set_autosuspend_delay(dev, oproc->autosuspend_delay); >>>> + pm_runtime_use_autosuspend(dev); >>>> + pm_runtime_get(dev); >>>> + pm_runtime_enable(dev); >>>> + pm_runtime_mark_last_busy(dev); >>>> + pm_runtime_put_autosuspend(dev); >>>> + >>>> reset_control_deassert(oproc->reset); >> >> I see that you have flipped the reset call and all the pm_runtime calls >> (w.r.t my original code sequence) and pm_runtime_get instead of >> pm_runtime_get_noresume(). Is there a reason for it? What is the backend >> that gets exercised with pm_runtime? > > PM runtime nowadays exercises ti-sysc, and we can't deassert reset > before we enable the clocks for the device. Thus, these are flipped. But the remoteprocs are not being added under a ti-sysc node from what I have seen so far, only the MMUs. Even if that were the case, you would have to actually revise the above pm_runtime call sequences, as replacing get_noresume() with get() are not equivalent. > > In the old incarnation of this code, PM runtime just exercised the > hacked up functionality implemented for omap rproc alone. Ha ha, it hadn't been easy to come up with that carefully crafted to deal with default hwmod code around reset issues. The logic enabled the clocking and reset independently while powering up and managed the pm_runtime status appropriately originally. > >> >>>> return 0; >>>> @@ -485,9 +520,26 @@ static int omap_rproc_start(struct rproc *rproc) >>>> /* power off the remote processor */ >>>> static int omap_rproc_stop(struct rproc *rproc) >>>> { >>>> + struct device *dev = rproc->dev.parent; >>>> struct omap_rproc *oproc = rproc->priv; >>>> int ret; >>>> + /* >>>> + * cancel any possible scheduled runtime suspend by incrementing >>>> + * the device usage count, and resuming the device. The remoteproc >>>> + * also needs to be woken up if suspended, to avoid the remoteproc >>>> + * OS to continue to remember any context that it has saved, and >>>> + * avoid potential issues in misindentifying a subsequent device >>>> + * reboot as a power restore boot >>>> + */ >>>> + ret = pm_runtime_get_sync(dev); >>>> + if (ret < 0) { >>>> + pm_runtime_put_noidle(dev); >>>> + return ret; >>>> + } >>>> + >>>> + pm_runtime_put_sync(dev); >>>> + >> >> I didn't have this call either. And get_sync() followed by put_sync() is >> essentially a no-op. Am I missing something here? > > Hmm right, but you did have the device_shutdown() here, somehow this > appears to simulate that, but you are right, it is just a no-op. I don't > know if this code has actually ever done anything useful. Let me > experiment with this one a bit and see what happens. It looks like it > should be removed completely. Yes, I did have the device_shutdown() here (turn off clocks and reset). The processor had to be woken up before shutting down (see the comments above the code hunk). > >> >>>> reset_control_assert(oproc->reset); Overall, the previous design relied on get_sync and put_sync calling the driver's callbacks once started and used regular clock and reset management outside of those during powering up (first time) and actual shut down. >>>> ret = omap_rproc_disable_timers(rproc, true); >>>> @@ -496,6 +548,15 @@ static int omap_rproc_stop(struct rproc *rproc) >>>> mbox_free_channel(oproc->mbox); >>>> + /* >>>> + * update the runtime pm states and status now that the remoteproc >>>> + * has stopped >>>> + */ >>>> + pm_runtime_disable(dev); >>>> + pm_runtime_dont_use_autosuspend(dev); >>>> + pm_runtime_put_noidle(dev); >>>> + pm_runtime_set_suspended(dev); >>>> + >>>> return 0; >>>> } >>>> @@ -552,17 +613,19 @@ static bool _is_rproc_in_standby(struct >>>> omap_rproc *oproc) >>>> /* 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) >>>> +static int _omap_rproc_suspend(struct rproc *rproc, bool auto_suspend) >>>> { >>>> 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; >>>> + u32 suspend_msg = auto_suspend ? >>>> + RP_MBOX_SUSPEND_AUTO : RP_MBOX_SUSPEND_SYSTEM; >>>> int ret; >>>> reinit_completion(&oproc->pm_comp); >>>> oproc->suspend_acked = false; >>>> - ret = mbox_send_message(oproc->mbox, (void >>>> *)RP_MBOX_SUSPEND_SYSTEM); >>>> + ret = mbox_send_message(oproc->mbox, (void *)suspend_msg); >>>> if (ret < 0) { >>>> dev_err(dev, "PM mbox_send_message failed: %d\n", ret); >>>> return ret; >>>> @@ -602,25 +665,55 @@ static int _omap_rproc_suspend(struct rproc >>>> *rproc) >>>> goto enable_device; >>>> } >>>> + /* >>>> + * IOMMUs would have to be disabled specifically for runtime >>>> suspend. >>>> + * They are handled automatically through System PM callbacks for >>>> + * regular system suspend >>>> + */ >>>> + if (auto_suspend) { >>>> + ret = omap_iommu_domain_deactivate(rproc->domain); >>>> + if (ret) { >>>> + dev_err(dev, "iommu domain deactivate failed %d\n", >>>> + ret); >>>> + goto enable_timers; >>>> + } >>>> + } >>>> + >>>> return 0; >> >> blank line here, and remove the one before enable_device. > > Hmm why? I don't think there is any coding guideline that would dictate > this. However, I'll fix this as this is effectively your file anyways. Thanks, easier for readability between the success and failure paths. regards Suman > > -Tero > >> >> regards >> Suman >> >>>> +enable_timers: >>>> + /* ignore errors on re-enabling code */ >>>> + omap_rproc_enable_timers(rproc, false); >>>> enable_device: >>>> reset_control_deassert(oproc->reset); >>>> return ret; >>>> } >>>> -static int _omap_rproc_resume(struct rproc *rproc) >>>> +static int _omap_rproc_resume(struct rproc *rproc, bool auto_suspend) >>>> { >>>> struct device *dev = rproc->dev.parent; >>>> struct omap_rproc *oproc = rproc->priv; >>>> int ret; >>>> + /* >>>> + * IOMMUs would have to be enabled specifically for runtime >>>> resume. >>>> + * They would have been already enabled automatically through >>>> System >>>> + * PM callbacks for regular system resume >>>> + */ >>>> + if (auto_suspend) { >>>> + ret = omap_iommu_domain_activate(rproc->domain); >>>> + if (ret) { >>>> + dev_err(dev, "omap_iommu activate failed %d\n", ret); >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> /* 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; >>>> + goto suspend_iommu; >>> >>> The same needs to be done if omap_rproc_enable_timers() fails. >>> >>>> } >>>> } >>>> @@ -633,6 +726,12 @@ static int _omap_rproc_resume(struct rproc >>>> *rproc) >>>> reset_control_deassert(oproc->reset); >>>> + return 0; >>>> + >>>> +suspend_iommu: >>>> + if (auto_suspend) >>>> + omap_iommu_domain_deactivate(rproc->domain); >>>> + >>>> out: >>>> return ret; >>>> } >>>> @@ -641,6 +740,7 @@ 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); >>>> + struct omap_rproc *oproc = rproc->priv; >>>> int ret = 0; >>>> mutex_lock(&rproc->lock); >>>> @@ -655,13 +755,25 @@ static int __maybe_unused >>>> omap_rproc_suspend(struct device *dev) >>>> goto out; >>>> } >>>> - ret = _omap_rproc_suspend(rproc); >>>> + ret = _omap_rproc_suspend(rproc, false); >>>> if (ret) { >>>> dev_err(dev, "suspend failed %d\n", ret); >>>> goto out; >>>> } >>>> + /* >>>> + * remoteproc is running at the time of system suspend, so >>>> remember >>>> + * it so as to wake it up during system resume >>>> + */ >>>> + oproc->need_resume = 1; >>> >>> Please use 'true' to be consistent with the type and >>> omap_rproc_resume(). >>> >>>> rproc->state = RPROC_SUSPENDED; >>>> + >>>> + /* >>>> + * update the runtime pm status to be suspended, without >>>> decrementing >>>> + * the device usage count >>>> + */ >>>> + pm_runtime_disable(dev); >>>> + pm_runtime_set_suspended(dev); >>>> out: >>>> mutex_unlock(&rproc->lock); >>>> return ret; >>>> @@ -671,6 +783,7 @@ 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); >>>> + struct omap_rproc *oproc = rproc->priv; >>>> int ret = 0; >>>> mutex_lock(&rproc->lock); >>>> @@ -682,17 +795,91 @@ static int __maybe_unused >>>> omap_rproc_resume(struct device *dev) >>>> goto out; >>>> } >>>> - ret = _omap_rproc_resume(rproc); >>>> + /* >>>> + * remoteproc was auto-suspended at the time of system suspend, >>>> + * so no need to wake-up the processor (leave it in suspended >>>> + * state, will be woken up during a subsequent runtime_resume) >>>> + */ >>>> + if (!oproc->need_resume) >>>> + goto out; >>>> + >>>> + ret = _omap_rproc_resume(rproc, false); >>>> if (ret) { >>>> dev_err(dev, "resume failed %d\n", ret); >>>> goto out; >>>> } >>>> + oproc->need_resume = false; >>>> rproc->state = RPROC_RUNNING; >>>> + >>>> + /* >>>> + * update the runtime pm status to be active, without incrementing >>>> + * the device usage count >>>> + */ >>>> + pm_runtime_set_active(dev); >>>> + pm_runtime_enable(dev); >>>> + pm_runtime_mark_last_busy(dev); >>>> out: >>>> mutex_unlock(&rproc->lock); >>>> return ret; >>>> } >>>> + >>>> +static int omap_rproc_runtime_suspend(struct device *dev) >>>> +{ >>>> + struct rproc *rproc = dev_get_drvdata(dev); >>>> + struct omap_rproc *oproc = rproc->priv; >>>> + int ret; >>>> + >>>> + if (rproc->state == RPROC_CRASHED) { >>>> + dev_dbg(dev, "rproc cannot be runtime suspended when >>>> crashed!\n"); >>>> + return -EBUSY; >>>> + } >>>> + >>>> + if (WARN_ON(rproc->state != RPROC_RUNNING)) { >>>> + dev_err(dev, "rproc cannot be runtime suspended when not >>>> running!\n"); >>>> + return -EBUSY; >>>> + } >>>> + >>>> + /* >>>> + * do not even attempt suspend if the remote processor is not >>>> + * idled for runtime auto-suspend >>>> + */ >>>> + if (!_is_rproc_in_standby(oproc)) { >>>> + ret = -EBUSY; >>>> + goto abort; >>>> + } >>>> + >>>> + ret = _omap_rproc_suspend(rproc, true); >>>> + if (ret) >>>> + goto abort; >>>> + >>>> + rproc->state = RPROC_SUSPENDED; >>>> + return 0; >>>> + >>>> +abort: >>>> + pm_runtime_mark_last_busy(dev); >>>> + return ret; >>>> +} >>>> + >>>> +static int omap_rproc_runtime_resume(struct device *dev) >>>> +{ >>>> + struct rproc *rproc = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + if (WARN_ON(rproc->state != RPROC_SUSPENDED)) { >>>> + dev_err(dev, "rproc cannot be runtime resumed if not >>>> suspended!\n"); >>>> + return -EBUSY; >>>> + } >>>> + >>>> + ret = _omap_rproc_resume(rproc, true); >>>> + if (ret) { >>>> + dev_err(dev, "runtime resume failed %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + rproc->state = RPROC_RUNNING; >>>> + return 0; >>>> +} >>>> #endif /* CONFIG_PM */ >>>> static const char * const ipu_mem_names[] = { >>>> @@ -778,6 +965,20 @@ static const struct of_device_id >>>> omap_rproc_of_match[] = { >>>> }; >>>> MODULE_DEVICE_TABLE(of, omap_rproc_of_match); >>>> +static int omap_rproc_get_autosuspend_delay(struct >>>> platform_device *pdev) >>>> +{ >>>> + const struct omap_rproc_dev_data *data; >>>> + int delay; >>>> + >>>> + data = of_device_get_match_data(&pdev->dev); >>>> + if (!data) >>>> + return -ENODEV; >>> >>> This check is done in omap_rproc_of_get_internal_memories() and >>> omap_rproc_get_boot_data(). I think it would be best to do it once >>> at the top >>> of the probe() function and be done with it. >>> >>> That being said and as noted in a previous comment, I would push all >>> tuneables >>> to the DT. If the property is missing then things default to >>> DEFAULT_AUTOSUSPEND_DELAY. >>> >>>> + >>>> + delay = data->autosuspend_delay; >>>> + >>>> + return (delay > 0) ? delay : DEFAULT_AUTOSUSPEND_DELAY; >>>> +} >>>> + >>>> static const char *omap_rproc_get_firmware(struct platform_device >>>> *pdev) >>>> { >>>> const char *fw_name; >>>> @@ -953,6 +1154,11 @@ static int omap_rproc_probe(struct >>>> platform_device *pdev) >>>> } >>>> init_completion(&oproc->pm_comp); >>>> + oproc->autosuspend_delay = omap_rproc_get_autosuspend_delay(pdev); >>>> + if (oproc->autosuspend_delay < 0) { >>>> + ret = oproc->autosuspend_delay; >>>> + goto free_rproc; >>>> + } >>>> oproc->fck = devm_clk_get(&pdev->dev, 0); >>>> if (IS_ERR(oproc->fck)) { >>>> @@ -994,6 +1200,8 @@ static int omap_rproc_remove(struct >>>> platform_device *pdev) >>>> static const struct dev_pm_ops omap_rproc_pm_ops = { >>>> SET_SYSTEM_SLEEP_PM_OPS(omap_rproc_suspend, omap_rproc_resume) >>>> + SET_RUNTIME_PM_OPS(omap_rproc_runtime_suspend, >>>> + omap_rproc_runtime_resume, NULL) >>>> }; >>>> static struct platform_driver omap_rproc_driver = { >>>> -- >>>> 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 19:34, Suman Anna <s-anna@ti.com> wrote: > > On 12/19/19 6:12 PM, Mathieu Poirier wrote: > > On Thu, 19 Dec 2019 at 06:18, Tero Kristo <t-kristo@ti.com> wrote: > >> > >> On 18/12/2019 02:38, Mathieu Poirier wrote: > >>> On Fri, Dec 13, 2019 at 02:55:27PM +0200, Tero Kristo wrote: > >>>> From: Suman Anna <s-anna@ti.com> > >>>> > >>>> An implementation for the rproc ops .da_to_va() has been added > >>>> that provides the address translation between device addresses > >>>> to kernel virtual addresses for internal RAMs present on that > >>>> particular remote processor device. The implementation provides > >>>> the translations based on the addresses parsed and stored during > >>>> the probe. > >>>> > >>>> This ops gets invoked by the exported rproc_da_to_va() function > >>>> and allows the remoteproc core's ELF loader to be able to load > >>>> program data directly into the internal memories. > >>>> > >>>> Signed-off-by: Suman Anna <s-anna@ti.com> > >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> > >>>> --- > >>>> drivers/remoteproc/omap_remoteproc.c | 39 ++++++++++++++++++++++++++++ > >>>> 1 file changed, 39 insertions(+) > >>>> > >>>> diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c > >>>> index 844703507a74..28f14e24b389 100644 > >>>> --- a/drivers/remoteproc/omap_remoteproc.c > >>>> +++ b/drivers/remoteproc/omap_remoteproc.c > >>>> @@ -232,10 +232,49 @@ static int omap_rproc_stop(struct rproc *rproc) > >>>> return 0; > >>>> } > >>>> > >>>> +/** > >>>> + * omap_rproc_da_to_va() - internal memory translation helper > >>>> + * @rproc: remote processor to apply the address translation for > >>>> + * @da: device address to translate > >>>> + * @len: length of the memory buffer > >>>> + * > >>>> + * Custom function implementing the rproc .da_to_va ops to provide address > >>>> + * translation (device address to kernel virtual address) for internal RAMs > >>>> + * present in a DSP or IPU device). The translated addresses can be used > >>>> + * either by the remoteproc core for loading, or by any rpmsg bus drivers. > >>>> + * Returns the translated virtual address in kernel memory space, or NULL > >>>> + * in failure. > >>>> + */ > >>>> +static void *omap_rproc_da_to_va(struct rproc *rproc, u64 da, int len) > >>>> +{ > >>>> + struct omap_rproc *oproc = rproc->priv; > >>>> + int i; > >>>> + u32 offset; > >>>> + > >>>> + if (len <= 0) > >>>> + return NULL; > >>>> + > >>>> + if (!oproc->num_mems) > >>>> + return NULL; > >>>> + > >>>> + for (i = 0; i < oproc->num_mems; i++) { > >>>> + if (da >= oproc->mem[i].dev_addr && da + len <= > >>> > >>> Shouldn't this be '<' rather than '<=' ? > >> > >> No, I think <= is correct. You need to consider the initial byte in the > >> range also. Consider a simple case where you provide the exact da + len > >> corresponding to a specific memory range. > > > > For that specific case you are correct. On the flip side if @da falls > > somewhere after @mem[i].dev_addr, there is a possibility to clobber > > the first byte of the next range if <= is used. > > Not really, you will miss out on the last byte actually if you use just > <. This is just address range check, any memcpy would actually end one > byte before. I am indeed worried about actual memory accesses but rproc_da_to_va() is using the same logic as you are when circling through carveouts. As such you can forget about my comment. Thanks, Mathieu > > Eg: 0x80000 of len 0x10000. I should perfectly be able to copy 0x1000 > bytes at 0x8f000. > > regards > Suman > > > > > > Thanks, > > Mathieu > > > >> > >>> > >>>> + oproc->mem[i].dev_addr + oproc->mem[i].size) { > >>> > >>> One space too many after the '+' . > >> > >> True, I wonder why checkpatch did not catch this. > >> > >>> > >>>> + offset = da - oproc->mem[i].dev_addr; > >>> > >>> One space too many after then '-' . > >> > >> Same, will fix these two. > >> > >> -Tero > >> > >>> > >>>> + /* __force to make sparse happy with type conversion */ > >>>> + return (__force void *)(oproc->mem[i].cpu_addr + > >>>> + offset); > >>>> + } > >>>> + } > >>>> + > >>>> + return NULL; > >>>> +} > >>>> + > >>>> static const struct rproc_ops omap_rproc_ops = { > >>>> .start = omap_rproc_start, > >>>> .stop = omap_rproc_stop, > >>>> .kick = omap_rproc_kick, > >>>> + .da_to_va = omap_rproc_da_to_va, > >>>> }; > >>>> > >>>> static const char * const ipu_mem_names[] = { > >>>> -- > >>>> 2.17.1 > >>>> > >>>> -- > >> > >> -- > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >