ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): d9e1adbe6b12aab495f0292c4553c5c8f61c70b5.1359684780.git.rtivy@ti.com
PATCH v7 1/2 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Rob,
Quoted 4 lines · Robert Tivy
drivers/remoteproc/Kconfig | 29 ++- drivers/remoteproc/Makefile | 1 + drivers/remoteproc/da8xx_remoteproc.c | 346 +++++++++++++++++++++++++++++++++ drivers/remoteproc/remoteproc_core.c | 22 ++-
It looks like this patch squashes:
1. A fix to drivers/remoteproc/Kconfig
2. New functionality in remoteproc_core.c
3. A new da8xx driver
These are independent changes, so we better split them up to separate patches.
Quoted 3 lines
config
OMAP_REMOTEPROC
tristate "OMAP remoteproc support"
- depends on EXPERIMENTAL
...
Quoted 3 lines
config
STE_MODEM_RPROC
tristate "STE-Modem remoteproc support"
- depends on EXPERIMENTAL
These look unrelated to this patch, and it seems that Kees Cook submitted these awhile ago so it should probably already be in linux-next (haven't looked). I think we can drop these.
Quoted 6 lines
(quoted diff) +6
-0+/** + * handle_event() - inbound virtqueue message workqueue function + * + * This function is registered as a kernel thread and is scheduled by the + * kernel handler. + */
Review
Quoted 10 lines
(quoted diff) +10
-0+static irqreturn_t handle_event(int irq, void *p) +{ + struct rproc *rproc = (struct rproc *)p; + + /* Process incoming buffers on our vring */ + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) + ; + + /* Must allow wakeup of potenitally blocking senders */ + rproc_vq_interrupt(rproc, 1);
IIRC we agreed on removing this part, and instead adding it to the generic remoteproc framework (i.e. Cyril's patch).
Quoted 2 lines
(quoted diff) +2
-0+static int da8xx_rproc_start(struct rproc *rproc) +{
...
Review
Quoted 7 lines
(quoted diff) +7
-0+ dsp_clk = devm_clk_get(dev, NULL); + if (IS_ERR(dsp_clk)) { + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk)); + + return PTR_RET(dsp_clk); + } + drproc->dsp_clk = dsp_clk;
Have you considered doing this in ->probe() instead? Do we need to call get/put every time we start/stop the remote processor?
Quoted 11 lines
(quoted diff) +11
-0+/* kick a virtqueue */ +static void da8xx_rproc_kick(struct rproc *rproc, int vqid) +{ + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + int timed_out; + unsigned long timeout; + + timed_out = 0; + timeout = jiffies + HZ/100; + + /* Poll for ack from other side first */
Review
Quoted 10 lines
(quoted diff) +10
-0+ while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) + if (time_after(jiffies, timeout)) { + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) { + dev_err(rproc->dev.parent, + "failed to receive ack\n"); + timed_out = 1; + } + + break; + }
This still looks a bit out of place here.
Kicking should be a fast unilateral action, that doesn't depend on the other side.
Quoted 2 lines
(quoted diff) +2
-0+static int da8xx_rproc_probe(struct platform_device *pdev) +{
...
Quoted 10 lines
(quoted diff) +10
-0+ ret = rproc_add(rproc); + if (ret) { + dev_err(dev, "rproc_add failed: %d\n", ret); + goto free_rproc; + } + + drproc->chipsig = chipsig; + drproc->bootreg = bootreg; + drproc->ack_fxn = irq_data->chip->irq_ack; + drproc->irq_data = irq_data;
Review
Quoted 10 lines
(quoted diff) +10
-0+ drproc->irq = irq; + + /* everything the ISR needs is now setup, so hook it up */ + ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback, + handle_event, 0, "da8xx-remoteproc", rproc); + if (ret) { + dev_err(dev, "devm_request_threaded_irq error: %d\n", ret); + rproc_del(rproc); + goto free_rproc; + }
Shouldn't we be requesting this before we rproc_add() ?
Quoted 6 lines
(quoted diff) +6
-0+static int da8xx_rproc_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rproc *rproc = platform_get_drvdata(pdev); + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv; + int ret;
...
Quoted 1 line
+ ret =
rproc_del(rproc);
You can safely remove 'ret' altogether. We will just crash in rproc_put if rproc is NULL.
Quoted 29 lines
(quoted diff) +4
-0+ rproc_put(rproc); + + return ret; +}drivers/remoteproc/remoteproc_core.c +12
-0@@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, const char *firmware, int len) {struct rproc *rproc; +char *template ="rproc-%s-fw" ; +char *p;if (!dev || !name || !ops)return NULL ; +if (!firmware) +/* +* Make room for default firmware name (minus %s plus '\0'). +* If the caller didn't pass in a firmware name then +* construct a default name. We're already glomming 'len' +* bytes onto the end of the struct rproc allocation, so do +* a few more for the default firmware name (but only if +* the caller doesn't pass one). +*/ + len +=strlen (name) +strlen (template) -2 +1 ;
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, const char *name, · line 1250
Quoted 10 lines
drivers/remoteproc/remoteproc_core.c +4
-0+ rproc =kzalloc (sizeof (struct rproc ) + len,GFP_KERNEL );if (!rproc) {dev_err (dev,"%s: kzalloc failed\n" , __func__);return NULL ; } +if (!firmware) { + p = (char *)rproc +sizeof (struct rproc ) + len; +sprintf (p, template, name);
Looks like p you're writing to is outside of the rproc memory allocation.
Thanks,
Ohad.
PATCH v7 1/2 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Ohad,
Quoted 13 lines
-----Original Message-----
From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
Sent: Friday, February 15, 2013 12:07 AMHi Rob,
Quoted 3 lines · Robert Tivy
drivers/remoteproc/Kconfig | 29 ++- drivers/remoteproc/Makefile | 1 + drivers/remoteproc/da8xx_remoteproc.c | 346+++++++++++++++++++++++++++++++++
Quoted 1 line
drivers/remoteproc/remoteproc_core.c | 22 ++-
Quoted 7 lines
It looks like this patch squashes:
1. A fix todrivers/remoteproc/Kconfig
2. New functionality inremoteproc_core.c
3. A new da8xx driverThese are independent changes, so we better split them up to separate patches.
Ok, I will submit separate patches for these. In doing so, I will have a stand-alone patch for the drivers/remoteproc/Kconfig fix, as well as the driver-related addition to that file in the driver patch. PCMIIW.
Quoted 4 lines
Quoted 4 lines
config
OMAP_REMOTEPROC
tristate "OMAP remoteproc support"
- depends on EXPERIMENTAL
Quoted 8 lines
...
Quoted 3 lines
config
STE_MODEM_RPROC
tristate "STE-Modem remoteproc support"
- depends on EXPERIMENTALThese look unrelated to this patch, and it seems that Kees Cook submitted these awhile ago so it should probably already be in linux-next (haven't looked). I think we can drop these.
OK, will drop.
Quoted 6 lines
Quoted 5 lines
+/**
+ *handle_event()- inbound virtqueue message workqueue function
+ *
+ * This function is registered as a kernel thread and is scheduledby the
Quoted 15 lines
Review
Quoted 12 lines
+ * kernel handler.
+ */(quoted diff) +10
-0+static irqreturn_t handle_event(int irq, void *p) +{ + struct rproc *rproc = (struct rproc *)p; + + /* Process incoming buffers on our vring */ + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) + ; + + /* Must allow wakeup of potenitally blocking senders */ + rproc_vq_interrupt(rproc, 1);IIRC we agreed on removing this part, and instead adding it to the generic remoteproc framework (i.e. Cyril's patch).
I didn't like the idea of having extra overhead when calling the "all virtqueues" version.
We know that we want to call rproc_vq_interrupt(rproc, 1) just once, and don't care about its return value. If we did
/* Process incoming buffers on our vring */
while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1))
;
then that would essentially turn into:
do {
ret = IRQ_NONE;
if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED)
ret = IRQ_HANDLED;
if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED)
ret = IRQ_HANDLED;
} while (ret == IRQ_HANDLED);which will end up calling rproc_vq_interrupt(rproc, 1) too many times.
We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated.
Quoted 7 lines
Quoted 3 lines
(quoted diff) +2
-0+static int da8xx_rproc_start(struct rproc *rproc) +{...
Review
Quoted 3 lines
(quoted diff) +3
-0+ dsp_clk = devm_clk_get(dev, NULL); + if (IS_ERR(dsp_clk)) { + dev_err(dev, "clk_get error: %ld\n",
Quoted 8 lines
PTR_ERR(dsp_clk));Review
Quoted 4 lines
(quoted diff) +4
-0+ + return PTR_RET(dsp_clk); + } + drproc->dsp_clk = dsp_clk;Have you considered doing this in ->probe() instead? Do we need to call get/put every time we start/stop the remote processor?
I suppose we don't need to call it every time, I will try moving it to ->probe()
Quoted 25 lines
Quoted 6 lines
(quoted diff) +4
-0+/* kick a virtqueue */ +static void da8xx_rproc_kick(struct rproc *rproc, int vqid) +{ + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc-priv;
Review
Quoted 10 lines
(quoted diff) +10
-0+ int timed_out; + unsigned long timeout; + + timed_out = 0; + timeout = jiffies + HZ/100; + + /* Poll for ack from other side first */ + while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) + if (time_after(jiffies, timeout)) { + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2){
Review
Quoted 7 lines
(quoted diff) +7
-0+ dev_err(rproc->dev.parent, + "failed to receive ack\n"); + timed_out = 1; + } + + break; + }
Quoted 4 lines
This still looks a bit out of place here.
Kicking should be a fast unilateral action, that doesn't depend on the other side.
Ok, I'll drop this complexity.
While that code sorta looks like too much, in a smoothly working system it's just a register read and single "false" if-test, since SYSCFG_CHIPSIG2 will actually never be set there. If SYSCFG_CHIPSIG2 *is* set then it will likely not unset and the timed_out situation will happen.
Quoted 22 lines
Quoted 3 lines
(quoted diff) +2
-0+static int da8xx_rproc_probe(struct platform_device *pdev) +{...
Quoted 4 lines
(quoted diff) +4
-0+ ret = rproc_add(rproc); + if (ret) { + dev_err(dev, "rproc_add failed: %d\n", ret); + goto free_rproc;Review
Quoted 10 lines
(quoted diff) +10
-0+ } + + drproc->chipsig = chipsig; + drproc->bootreg = bootreg; + drproc->ack_fxn = irq_data->chip->irq_ack; + drproc->irq_data = irq_data; + drproc->irq = irq; + + /* everything the ISR needs is now setup, so hook it up */ + ret = devm_request_threaded_irq(dev, irq,
da8xx_rproc_callback,Review
Quoted 3 lines
(quoted diff) +3
-0+ handle_event, 0, "da8xx-remoteproc", rproc); + if (ret) { + dev_err(dev, "devm_request_threaded_irq error: %d\n",
Quoted 6 lines
ret);
Review
Quoted 3 lines
(quoted diff) +3
-0+ rproc_del(rproc); + goto free_rproc; + }Shouldn't we be requesting this before we
rproc_add()?
Yeah, that seems to be the more prudent ordering, will switch.
Quoted 9 lines
Quoted 8 lines
(quoted diff) +5
-0+static int da8xx_rproc_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rproc *rproc = platform_get_drvdata(pdev); + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc-priv;
+ int ret;...
Quoted 4 lines
Quoted 1 line
+ ret =
rproc_del(rproc);You can safely remove 'ret' altogether. We will just crash in
rproc_putif rproc is NULL.
Will do.
Quoted 43 lines
Review
Quoted 7 lines
(quoted diff) +4
-0+ rproc_put(rproc); + + return ret; +}(quoted diff) +0
-0
b/drivers/remoteproc/remoteproc_core.cdrivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev,
Quoted 4 lines
index dd3bfaf..e0f703e 100644
drivers/remoteproc/remoteproc_core.c +0
-0@@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev,const char *name,
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, · line 1233
Quoted 12 lines
drivers/remoteproc/remoteproc_core.c +5
-0const char *firmware, int len) {struct rproc *rproc; +char *template ="rproc-%s-fw" ; +char *p;if (!dev || !name || !ops)return NULL ; +if (!firmware) +/* +* Make room for default firmware name (minus %s plus '\0').
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, · line 1235
Quoted 2 lines
drivers/remoteproc/remoteproc_core.c +2
-0+* If the caller didn't pass in a firmware name then +* construct a default name. We're already glomming 'len'
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, · line 1236
Quoted 1 line
drivers/remoteproc/remoteproc_core.c +1
-0+* bytes onto the end of the struct rproc allocation, so do
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, · line 1237
Quoted 1 line
drivers/remoteproc/remoteproc_core.c +1
-0+* a few more for the default firmware name (but only if
Quoted 10 lines
drivers/remoteproc/remoteproc_core.c +4
-0+* the caller doesn't pass one). +*/ + len +=strlen (name) +strlen (template) -2 +1 ; + rproc =kzalloc (sizeof (struct rproc ) + len,GFP_KERNEL );if (!rproc) {dev_err (dev,"%s: kzalloc failed\n" , __func__);return NULL ; }
Quoted 6 lines
drivers/remoteproc/remoteproc_core.c struct rproc *rproc_alloc(struct device *dev, · line 1250
Quoted 3 lines
drivers/remoteproc/remoteproc_core.c +3
-0+if (!firmware) { + p = (char *)rproc +sizeof (struct rproc ) + len; +sprintf (p, template, name);Looks like p you're writing to is outside of the rproc memory allocation.
Yikes, must have been getting lucky when testing with the default name!
Thanks for the notice, will fix.
Regards,
- Rob
Quoted 3 lines
Thanks,
Ohad.
PATCH v7 1/2 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Rob,
Quoted 9 lines · Robert
Quoted 7 lines
It looks like this patch squashes:
1. A fix todrivers/remoteproc/Kconfig
2. New functionality inremoteproc_core.c
3. A new da8xx driverThese are independent changes, so we better split them up to separate patches.
Ok, I will submit separate patches for these. In doing so, I will have a stand-alone patch for the
drivers/remoteproc/Kconfigfix, as well as the driver-related addition to that file in the driver patch. PCMIIW.
We should have one patch for the Kconfig fix, another one for the new firmware-name functionality in rproc_alloc() and then a patch adding this new driver.
Quoted 28 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+static irqreturn_t handle_event(int irq, void *p) +{ + struct rproc *rproc = (struct rproc *)p; + + /* Process incoming buffers on our vring */Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0)) + ; + + /* Must allow wakeup of potenitally blocking senders */ + rproc_vq_interrupt(rproc, 1);IIRC we agreed on removing this part, and instead adding it to the generic remoteproc framework (i.e. Cyril's patch).
I didn't like the idea of having extra overhead when calling the "all virtqueues" version.
We know that we want to call
rproc_vq_interrupt(rproc, 1)just once, and don't care about its return value. If we did/* Process incoming buffers on our vring */ while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1)) ; then that would essentially turn into: do { ret = IRQ_NONE; if (rproc_vq_interrupt(rproc, 0) == IRQ_HANDLED) ret = IRQ_HANDLED; if (rproc_vq_interrupt(rproc, 1) == IRQ_HANDLED) ret = IRQ_HANDLED; } while (ret == IRQ_HANDLED);
Quoted 3 lines
which will end up calling
rproc_vq_interrupt(rproc, 1)too many times.We have a benchmark goal to keep wrt/ the whole round-trip time of messages and we're currently not even achieving that, so more overhead than today can't be tolerated.
Have you checked whether it really affects performance? I'd expect this to be very negligible. We can probably also maintain a bitmap with bits set for every IRQ_HANDLED vring, and stop processing vrings that has this bit cleared.
But it actually looks like there's a different problem here. rproc_vq_interrupt() may also return IRQ_HANDLED on the unlikely event that vq->broken is set. With the current approach taken by this patch, this may lock the system in a busy loop.
I took a brief look at the other virtio drivers, and it seems they process all pending messages available when kicked, so we should probably add this to rpmsg_recv_done() too. This way we don't have to manually do it in the remoteproc layer, and all platforms will benefit from it.
We should then only have to allow the option of asking the core to process all available vrings, as we discussed, and we're done.
Thanks,
Ohad.
v7 cover ARM: davinci: remoteproc support
In reply to a message not in the archiveQuoted 15 lines · Robert Tivy
This patch series adds remoteproc support for OMAP-L138, along with needed supporting mach-davinci infrastructure.
These patches are based on Sekhar's commit id
601ac613155476701ff2155aa11853cad70d7ce5
atgit://gitorious.org/linux-davinci/linux-davinci.gitSome notes for reviewers...
DOCUMENTATION maintainers: patch 2/2 in this series contains a change to kernel-parameters.txt that adds a description for a new kernel command-line parameter, along with the code that defines the new kernel command-line parameter. You are, of course, free to look at the whole series, but only patch 2/2 is of particular interest.
Quoted 5 lines
This is actually a subset of the complete patch series, since the other patches in this set have already been committed to a staging area by the tree maintainer.
Sekhar, once this is accepted, please remember to apply patch [PATCH v5 8/9].
Please add this patch to this series, next time you spin it.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): 78c80762cae70455b503d78702c3777c3a6d9321.1359684780.git.rtivy@ti.com
PATCH v7 2/2 ARM: davinci: Remoteproc platform device creation data/code
Hi Rob,
Quoted 6 lines · Robert Tivy
Added a new remoteproc platform device for DA8XX. Contains CMA-based reservation of physical memory block. A new kernel command-line parameter has been added to allow boot-time specification of the physical memory block.
Signed-off-by Robert Tivy <[email protected]>
This looks good to me. Except one minor nit below:
Review
Quoted 3 lines
(quoted diff) +3
-0+static struct platform_device da8xx_dsp = { + .name = "davinci-rproc", + .id = 0,
No need to zero initialize here.
Review
Quoted 6 lines
(quoted diff) +6
-0+ .dev = { + .coherent_dma_mask = DMA_BIT_MASK(32), + }, + .num_resources = ARRAY_SIZE(da8xx_rproc_resources), + .resource = da8xx_rproc_resources, +};
You can fix that and add my:
in case this goes from outside of DaVinci tree.
Thanks,
Sekhar