Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On Mon, Jan 21, 2013 at 11:08:43AM +0530, Sekhar Nori wrote:
+ if (IS_ERR_OR_NULL(r)) {
+ dev_err(dev, platform_get_resource() error: %ld\n,
+ PTR_ERR(r));
+
+ return PTR_ERR(r);
Sigh. Bug.
+ }
+ host1cfg_physaddr = (unsigned long)r-start;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0) {
+ dev_err(dev, platform_get_irq(pdev, 0) error: %d\n, irq);
+
+ return irq;
+ }
+
+ irq_data = irq_get_irq_data(irq);
+ if (IS_ERR_OR_NULL(irq_data)) {
+ dev_err(dev, irq_get_irq_data(%d) error: %ld\n,
+ irq, PTR_ERR(irq_data));
+
+ return PTR_ERR(irq_data);
Bug.
+ }
+ ack_fxn = irq_data-chip-irq_ack;
+
+ ret = request_threaded_irq(irq, davinci_rproc_callback, handle_event,
+ 0, davinci-remoteproc, drproc);
+ if (ret) {
+ dev_err(dev, request_threaded_irq error: %d\n, ret);
+
+ return ret;
+ }
+
+ syscfg0_base = ioremap(host1cfg_physaddr PAGE_MASK, SZ_4K);
+ host1cfg_offset = offset_in_page(host1cfg_physaddr);
+ writel(rproc-bootaddr, syscfg0_base + host1cfg_offset);
+
+ dsp_clk = clk_get(dev, NULL);
devm_clk_get() here.
+ if (IS_ERR_OR_NULL(dsp_clk)) {
+ dev_err(dev, clk_get error: %ld\n, PTR_ERR(dsp_clk));
+ ret = PTR_ERR(dsp_clk);
Bug.
See, yet again... almost every use of IS_ERR_OR_NULL() is a bug.
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Russell, thankyou for the review and notice, all this will be straightened out
in the next patch submission.
Regards,
- Rob
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Monday, January 21, 2013 8:41 AM
To: Nori, Sekhar
Cc: Tivy, Robert; o...@wizery.com; davinci-linux-open-
sou...@linux.davincidsp.com; linux-...@vger.kernel.org; Ring, Chris;
r...@landley.net; Grosen, Mark; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for
OMAP-L138 DSP
On Mon, Jan 21, 2013 at 11:08:43AM +0530, Sekhar Nori wrote:
+ if (IS_ERR_OR_NULL(r)) {
+ dev_err(dev, platform_get_resource() error: %ld\n,
+ PTR_ERR(r));
+
+ return PTR_ERR(r);
Sigh. Bug.
+ }
+ host1cfg_physaddr = (unsigned long)r-start;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0) {
+ dev_err(dev, platform_get_irq(pdev, 0) error: %d\n, irq);
+
+ return irq;
+ }
+
+ irq_data = irq_get_irq_data(irq);
+ if (IS_ERR_OR_NULL(irq_data)) {
+ dev_err(dev, irq_get_irq_data(%d) error: %ld\n,
+ irq, PTR_ERR(irq_data));
+
+ return PTR_ERR(irq_data);
Bug.
+ }
+ ack_fxn = irq_data-chip-irq_ack;
+
+ ret = request_threaded_irq(irq, davinci_rproc_callback,
handle_event,
+ 0, davinci-remoteproc, drproc);
+ if (ret) {
+ dev_err(dev, request_threaded_irq error: %d\n, ret);
+
+ return ret;
+ }
+
+ syscfg0_base = ioremap(host1cfg_physaddr PAGE_MASK, SZ_4K);
+ host1cfg_offset = offset_in_page(host1cfg_physaddr);
+ writel(rproc-bootaddr, syscfg0_base + host1cfg_offset);
+
+ dsp_clk = clk_get(dev, NULL);
devm_clk_get() here.
+ if (IS_ERR_OR_NULL(dsp_clk)) {
+ dev_err(dev, clk_get error: %ld\n, PTR_ERR(dsp_clk));
+ ret = PTR_ERR(dsp_clk);
Bug.
See, yet again... almost every use of IS_ERR_OR_NULL() is a bug.
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
-Original Message-
From: Nori, Sekhar
Sent: Sunday, January 20, 2013 9:39 PM
On 1/11/2013 5:53 AM, Robert Tivy wrote:
Adding a remoteproc driver implementation for OMAP-L138 DSP
Signed-off-by: Robert Tivy rt...@ti.com
---
drivers/remoteproc/Kconfig | 23 ++
drivers/remoteproc/Makefile|1 +
drivers/remoteproc/davinci_remoteproc.c| 307
include/linux/platform_data/da8xx-remoteproc.h | 33 +++
naming the driver davinci_remoteproc.c and platform data as
da8xx-remoteproc.h is odd. The driver looks really specific to omap-
l138
so may be call the driver da8xx-remoteproc.c?
At first when I started working on this stuff we intended to unify this naming,
to either davinci-based or da8xx-based, but after looking at it for a while
we realized that it is already so hopelessly mixed in many areas that it wasn't
worth the effort.
But for new stuff that's directly tied to omap-l138 we should be consistent, so
I agree. I'll change it to da8xx-remoteproc.c, at the same time changing all
the code/data names to be prefixed with da8xx_.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 96ce101..7d3a5e0 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -41,4 +41,27 @@ config STE_MODEM_RPROC
This can be either built-in or a loadable module.
If unsure say N.
+config DAVINCI_REMOTEPROC
+ tristate DaVinci DA850/OMAPL138 remoteproc support
(EXPERIMENTAL)
+ depends on ARCH_DAVINCI_DA850
+ select REMOTEPROC
+ select RPMSG
+ select FW_LOADER
+ select CMA
+ default n
+ help
+ Say y here to support DaVinci DA850/OMAPL138 remote processors
+ via the remote processor framework.
+
+ You want to say y here in order to enable AMP
+ use-cases to run on your platform (multimedia codecs are
+ offloaded to remote DSP processors using this framework).
+
+ It's safe to say n here if you're not interested in multimedia
+ offloading or just want a bare minimum kernel.
+ This feature is considered EXPERIMENTAL, due to it not having
+ any previous exposure to the general public and therefore
+ limited developer-based testing.
This is probably true in general for remoteproc (I am being warned a
lot
by the framework when using it) so may be you can drop this specific
reference.
OK, we'll let the overlying REMOTEPROC EXPERIMENTAL status preside.
diff --git a/drivers/remoteproc/davinci_remoteproc.c
b/drivers/remoteproc/davinci_remoteproc.c
new file mode 100644
index 000..fc6fd72
--- /dev/null
+++ b/drivers/remoteproc/davinci_remoteproc.c
@@ -0,0 +1,307 @@
+/*
+ * Remote processor machine-specific module for Davinci
+ *
+ * Copyright (C) 2011-2012 Texas Instruments, Inc.
2013?
Yep.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt)%s: fmt, __func__
You dont seem to be using this anywhere.
Will remove.
+
+#include linux/kernel.h
+#include linux/err.h
+#include linux/printk.h
+#include linux/bitops.h
+#include linux/platform_device.h
+#include linux/remoteproc.h
+#include linux/platform_data/da8xx-remoteproc.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/module.h
+#include linux/interrupt.h
+#include linux/irq.h
It will be nice to keep this sorted. It avoids duplicate includes
later.
OK
+static char *fw_name;
+module_param(fw_name, charp, S_IRUGO);
+MODULE_PARM_DESC(fw_name, \n\t\tName of DSP firmware file in
/lib/firmware);
+
+/*
+ * OMAP-L138 Technical References:
+ * http://www.ti.com/product/omap-l138
+ */
+#define SYSCFG_CHIPSIG_OFFSET 0x174
+#define SYSCFG_CHIPSIG_CLR_OFFSET 0x178
+#define SYSCFG_CHIPINT0 (1 0)
+#define SYSCFG_CHIPINT1 (1 1)
+#define SYSCFG_CHIPINT2 (1 2)
+#define SYSCFG_CHIPINT3 (1 3)
You can use BIT(x) here.
Will do.
+
+/**
+ * struct davinci_rproc - davinci remote processor state
+ * @rproc: rproc handle
+ */
+struct davinci_rproc {
+ struct rproc *rproc;
+ struct clk *dsp_clk;
+};
+
+static void __iomem *syscfg0_base;
+static struct platform_device *remoteprocdev;
+static struct irq_data *irq_data;
+static void (*ack_fxn)(struct irq_data *data);
+static int irq;
+
+/**
+ * handle_event() - inbound virtqueue message workqueue function
+ *
+ * This funciton is registered as a kernel thread and is scheduled
by the
+ * kernel handler.
+ */
+static irqreturn_t handle_event(int irq, void *p)
+{
+ struct rproc *rproc = platform_get_drvdata(remoteprocdev);
+
+ /* Process incoming buffers on our vring */
+ while (IRQ_HANDLED ==
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On 1/11/2013 5:53 AM, Robert Tivy wrote:
Adding a remoteproc driver implementation for OMAP-L138 DSP
Signed-off-by: Robert Tivy rt...@ti.com
---
drivers/remoteproc/Kconfig | 23 ++
drivers/remoteproc/Makefile|1 +
drivers/remoteproc/davinci_remoteproc.c| 307
include/linux/platform_data/da8xx-remoteproc.h | 33 +++
naming the driver davinci_remoteproc.c and platform data as
da8xx-remoteproc.h is odd. The driver looks really specific to omap-l138
so may be call the driver da8xx-remoteproc.c?
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 96ce101..7d3a5e0 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -41,4 +41,27 @@ config STE_MODEM_RPROC
This can be either built-in or a loadable module.
If unsure say N.
+config DAVINCI_REMOTEPROC
+ tristate DaVinci DA850/OMAPL138 remoteproc support (EXPERIMENTAL)
+ depends on ARCH_DAVINCI_DA850
+ select REMOTEPROC
+ select RPMSG
+ select FW_LOADER
+ select CMA
+ default n
+ help
+ Say y here to support DaVinci DA850/OMAPL138 remote processors
+ via the remote processor framework.
+
+ You want to say y here in order to enable AMP
+ use-cases to run on your platform (multimedia codecs are
+ offloaded to remote DSP processors using this framework).
+
+ It's safe to say n here if you're not interested in multimedia
+ offloading or just want a bare minimum kernel.
+ This feature is considered EXPERIMENTAL, due to it not having
+ any previous exposure to the general public and therefore
+ limited developer-based testing.
This is probably true in general for remoteproc (I am being warned a lot
by the framework when using it) so may be you can drop this specific
reference.
diff --git a/drivers/remoteproc/davinci_remoteproc.c
b/drivers/remoteproc/davinci_remoteproc.c
new file mode 100644
index 000..fc6fd72
--- /dev/null
+++ b/drivers/remoteproc/davinci_remoteproc.c
@@ -0,0 +1,307 @@
+/*
+ * Remote processor machine-specific module for Davinci
+ *
+ * Copyright (C) 2011-2012 Texas Instruments, Inc.
2013?
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt)%s: fmt, __func__
You dont seem to be using this anywhere.
+
+#include linux/kernel.h
+#include linux/err.h
+#include linux/printk.h
+#include linux/bitops.h
+#include linux/platform_device.h
+#include linux/remoteproc.h
+#include linux/platform_data/da8xx-remoteproc.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/module.h
+#include linux/interrupt.h
+#include linux/irq.h
It will be nice to keep this sorted. It avoids duplicate includes later.
+static char *fw_name;
+module_param(fw_name, charp, S_IRUGO);
+MODULE_PARM_DESC(fw_name, \n\t\tName of DSP firmware file in
/lib/firmware);
+
+/*
+ * OMAP-L138 Technical References:
+ * http://www.ti.com/product/omap-l138
+ */
+#define SYSCFG_CHIPSIG_OFFSET 0x174
+#define SYSCFG_CHIPSIG_CLR_OFFSET 0x178
+#define SYSCFG_CHIPINT0 (1 0)
+#define SYSCFG_CHIPINT1 (1 1)
+#define SYSCFG_CHIPINT2 (1 2)
+#define SYSCFG_CHIPINT3 (1 3)
You can use BIT(x) here.
+
+/**
+ * struct davinci_rproc - davinci remote processor state
+ * @rproc: rproc handle
+ */
+struct davinci_rproc {
+ struct rproc *rproc;
+ struct clk *dsp_clk;
+};
+
+static void __iomem *syscfg0_base;
+static struct platform_device *remoteprocdev;
+static struct irq_data *irq_data;
+static void (*ack_fxn)(struct irq_data *data);
+static int irq;
+
+/**
+ * handle_event() - inbound virtqueue message workqueue function
+ *
+ * This funciton is registered as a kernel thread and is scheduled by the
+ * kernel handler.
+ */
+static irqreturn_t handle_event(int irq, void *p)
+{
+ struct rproc *rproc = platform_get_drvdata(remoteprocdev);
+
+ /* 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);
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * davinci_rproc_callback() - inbound virtqueue message handler
+ *
+ * This handler is invoked directly by the kernel whenever the remote
+ * core (DSP) has modified the state of a virtqueue. There is no
+ * payload message indicating the virtqueue index as is the case with
+ * mailbox-based implementations on OMAP4. As such, this handler polls
+ * each known virtqueue index for every invocation.
+ */
+static irqreturn_t davinci_rproc_callback(int irq, void *p)
+{
+ if (readl(syscfg0_base + SYSCFG_CHIPSIG_OFFSET) SYSCFG_CHIPINT0) {
personally I
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On 1/12/2013 7:56 AM, Tivy, Robert wrote:
From: Ohad Ben-Cohen [mailto:o...@wizery.com]
Sent: Friday, January 11, 2013 4:26 AM
On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy rt...@ti.com wrote:
+static int davinci_rproc_probe(struct platform_device *pdev)
+{
+ struct da8xx_rproc_pdata *pdata = pdev-dev.platform_data;
+ struct davinci_rproc *drproc;
+ struct rproc *rproc;
+ struct clk *dsp_clk;
+ int ret;
+
+ if (!fw_name) {
+ dev_err(pdev-dev, No firmware file specified\n);
+
+ return -EINVAL;
+ }
There are a few issues with this fw_name module param:
1. Usually we don't rely on users providing the firmware file name for
drivers to work. Drivers should know the name beforehand, and if there
may be several different instances of firmwares (for different cores
you may have), then it's just better to get it from the platform data.
Is this suggesting that there be separate platform device instances for each
different potential fw, and that each platform device instance hardcodes the
fw filename?
I am not convinced firmware name should be in platform data (or DT)
since it is not hardware specific. User can choose multiple different
firmwares to load on the DSP depending the application he is running all
for the same platform (da850 evm).
2. You may still want to have such a module param in order to be able
to override the default firmware name (for debugging purposes?), but
I'm not sure it should be davinci-specific. if we do want it to be
then please prefix the name with 'davinci'.
Sekhar asked that there not be a default fw name, so there's conflicting
feedback on this point. I prefer to have a default name plus the module
parameter override (but don't have much opinion on whether it should be
davinci-specific (and passed with davinci_remoteproc.ko) or general (and
passed with remoteproc.ko), please advise).
Rob, I don't remember objecting to a default firmware name if module
parameter is not passed. On 29th November 2012 you wrote:
Sounds OK. I propose then to have the above be the default firmware
name, along with a module parameter that will override if specified.
and I wrote back:
Sounds good.
As you can see, there was no objection from me.
Since the fw file (i.e., DSP program) is typically paired with a particular
Linux app, I like the ability to specify the fw filename at runtime,
depending on the Linux app I need to run.
Right, and platform data is not the way to achieve this.
Thanks,
Sekhar
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Rob, On Sat, Jan 12, 2013 at 4:26 AM, Tivy, Robert rt...@ti.com wrote: Is FW_CONFIG above supposed to be FW_LOADER? That FW_CONFIG is completely bogus of course. care to fix it in your series? We're currently handling the CHIPINT lines as levels, since they're completely controlled by SW. The interruptor raises the line and the interruptee lowers (clears) it. In a situation where every interrupt is considered to be a signal of new data arrival we need to make sure that the other side has seen the previous interrupt before we raise another one. What if we don't ? Can the DSP side just go over the vrings until no new msg is left (similar to what you do on the Linux side) ? Is this suggesting that there be separate platform device instances for each different potential fw, and that each platform device instance hardcodes the fw filename? In principle this same driver can drive many instances of remote processors you may have on your board. E.g. in OMAP the same driver controls both the M3s and the DSP. pdata is then used to hold information specific to the hw instance. I'm not sure if there's (or will be) any DaVinci platform with several remote processors but it's a better practice to write the code as if there is/will be - it's usually cleaner and will just work in case a platform with multiple rprocs does show up one day. Sekhar asked that there not be a default fw name, so there's conflicting feedback on this point. I prefer to have a default name plus the module parameter override (but don't have much opinion on whether it should be davinci-specific (and passed with davinci_remoteproc.ko) or general (and passed with remoteproc.ko), please advise). Since the fw file (i.e., DSP program) is typically paired with a particular Linux app, I like the ability to specify the fw filename at runtime, depending on the Linux app I need to run. Sure, no reason why not to allow users to override the default fw name. Thanks, Ohad. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On Tue, Jan 15, 2013 at 11:15 AM, Sekhar Nori nsek...@ti.com wrote: Right, and platform data is not the way to achieve this. How do you suggest to handle platforms with multiple different remote processors (driven by the same driver) ? Requiring the user to indicate the fw name for each processor is somewhat error prone/cumbersome. Thanks, Ohad. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On 1/15/2013 3:33 PM, Ohad Ben-Cohen wrote: On Tue, Jan 15, 2013 at 11:15 AM, Sekhar Nori nsek...@ti.com wrote: Right, and platform data is not the way to achieve this. How do you suggest to handle platforms with multiple different remote processors (driven by the same driver) ? Requiring the user to indicate the fw name for each processor is somewhat error prone/cumbersome. May be rproc_alloc() could auto-assign the firmware name to something like 'rproc%d-fw' if firmware name passed to it is NULL? Thanks, Sekhar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori nsek...@ti.com wrote: May be rproc_alloc() could auto-assign the firmware name to something like 'rproc%d-fw' if firmware name passed to it is NULL? I prefer we use name-based filenames instead to make it easier for users (and us developers). We can probably do something like rproc-%s-fw with pdata-name assuming we/you do maintain a meaningful name in the latter. Thanks, Ohad. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
-Original Message- From: Ohad Ben-Cohen [mailto:o...@wizery.com] Sent: Tuesday, January 15, 2013 4:49 AM To: Nori, Sekhar Cc: Tivy, Robert; davinci-linux-open-source; linux-arm; Ring, Chris; Grosen, Mark; r...@landley.net; linux-...@vger.kernel.org; Chemparathy, Cyril Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori nsek...@ti.com wrote: May be rproc_alloc() could auto-assign the firmware name to something like 'rproc%d-fw' if firmware name passed to it is NULL? I prefer we use name-based filenames instead to make it easier for users (and us developers). We can probably do something like rproc-%s-fw with pdata-name assuming we/you do maintain a meaningful name in the latter. This sounds good, although it will introduce the need to handle dynamic storage for the generated name. I think I can jam that storage on the end of the already-dynamically-sized 'struct rproc + sizeof(pdata)' allocation in rproc_alloc(). Thanks Regards, - Rob Thanks, Ohad. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On Wed, Jan 16, 2013 at 1:06 AM, Tivy, Robert rt...@ti.com wrote: This sounds good, although it will introduce the need to handle dynamic storage for the generated name. I think I can jam that storage on the end of the already-dynamically-sized 'struct rproc + sizeof(pdata)' allocation in rproc_alloc(). Or you can generate it when needed, without storing the result. This rarely happens and anyway is negligible. Thanks, Ohad. ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On 1/15/2013 6:19 PM, Ohad Ben-Cohen wrote: On Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori nsek...@ti.com wrote: May be rproc_alloc() could auto-assign the firmware name to something like 'rproc%d-fw' if firmware name passed to it is NULL? I prefer we use name-based filenames instead to make it easier for users (and us developers). We can probably do something like rproc-%s-fw with pdata-name assuming we/you do maintain a meaningful name in the latter. Are you thinking of passing name of the remote processor (like m3, dsp0, dsp1 etc) in pdata-name? That sounds OK since the processor name is actually tied to the platform. BTW, the current driver seems to be written for OMAP-L138 rather tightly so you could as well hardcode the firmware name to 'rproc-dsp-fw'. Thanks, Sekhar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
On Fri, Jan 11, 2013 at 02:26:19PM +0200, Ohad Ben-Cohen wrote:
+static int davinci_rproc_start(struct rproc *rproc)
+{
+ struct platform_device *pdev =
to_platform_device(rproc-dev.parent);
+ struct device *dev = rproc-dev.parent;
+ struct davinci_rproc *drproc = rproc-priv;
+ struct clk *dsp_clk;
+ struct resource *r;
+ unsigned long host1cfg_physaddr;
+ unsigned int host1cfg_offset;
+ int ret;
+
+ remoteprocdev = pdev;
+
+ /* hw requires the start (boot) address be on 1KB boundary */
+ if (rproc-bootaddr 0x3ff) {
+ dev_err(dev, invalid boot address: must be aligned to
1KB\n);
+
+ return -EINVAL;
+ }
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR_OR_NULL(r)) {
No, this is buggy. Go and look up to see what the return ranges are
for this function.
+ dev_err(dev, platform_get_resource() error: %ld\n,
+ PTR_ERR(r));
+
+ return PTR_ERR(r);
Which results in this being a bug.
+ }
+ host1cfg_physaddr = (unsigned long)r-start;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0) {
+ dev_err(dev, platform_get_irq(pdev, 0) error: %d\n, irq);
+
+ return irq;
+ }
+
+ irq_data = irq_get_irq_data(irq);
+ if (IS_ERR_OR_NULL(irq_data)) {
Again, bug.
+ dev_err(dev, irq_get_irq_data(%d) error: %ld\n,
+ irq, PTR_ERR(irq_data));
+
+ return PTR_ERR(irq_data);
Which results in this being a bug.
+ }
+ ack_fxn = irq_data-chip-irq_ack;
+
+ ret = request_threaded_irq(irq, davinci_rproc_callback,
handle_event,
+ 0, davinci-remoteproc, drproc);
+ if (ret) {
+ dev_err(dev, request_threaded_irq error: %d\n, ret);
+
+ return ret;
+ }
+
+ syscfg0_base = ioremap(host1cfg_physaddr PAGE_MASK, SZ_4K);
+ host1cfg_offset = offset_in_page(host1cfg_physaddr);
+ writel(rproc-bootaddr, syscfg0_base + host1cfg_offset);
+
+ dsp_clk = clk_get(dev, NULL);
+ if (IS_ERR_OR_NULL(dsp_clk)) {
And another bug.
+ dev_err(dev, clk_get error: %ld\n, PTR_ERR(dsp_clk));
+ ret = PTR_ERR(dsp_clk);
And again, results in this being a bug.
+ goto fail;
+ }
...
+ ret = rproc_add(rproc);
+ if (ret)
+ goto free_rproc;
+
+ /*
+* rproc_add() can end up enabling the DSP's clk with the DSP
+* *not* in reset, but davinci_rproc_start() needs the DSP to be
+* held in reset at the time it is called.
+*/
+ dsp_clk = clk_get(rproc-dev.parent, NULL);
+ davinci_clk_reset_assert(dsp_clk);
+ clk_put(dsp_clk);
BUG: what if clk_get() fails here?
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Robert,
I'm happy to see this driver going upstream, thanks for pushing!
Please see below my few comments. PS - sorry for the belated review.
On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy rt...@ti.com wrote:
+config DAVINCI_REMOTEPROC
+ tristate DaVinci DA850/OMAPL138 remoteproc support (EXPERIMENTAL)
+ depends on ARCH_DAVINCI_DA850
+ select REMOTEPROC
+ select RPMSG
+ select FW_LOADER
This one gets selected by CONFIG_REMOTEPROC, so you don't need to do so too.
diff --git a/drivers/remoteproc/davinci_remoteproc.c
b/drivers/remoteproc/davinci_remoteproc.c
new file mode 100644
index 000..fc6fd72
--- /dev/null
+++ b/drivers/remoteproc/davinci_remoteproc.c
+static char *fw_name;
+module_param(fw_name, charp, S_IRUGO);
+MODULE_PARM_DESC(fw_name, \n\t\tName of DSP firmware file in
/lib/firmware);
+
+/*
+ * OMAP-L138 Technical References:
+ * http://www.ti.com/product/omap-l138
+ */
+#define SYSCFG_CHIPSIG_OFFSET 0x174
+#define SYSCFG_CHIPSIG_CLR_OFFSET 0x178
+#define SYSCFG_CHIPINT0 (1 0)
+#define SYSCFG_CHIPINT1 (1 1)
+#define SYSCFG_CHIPINT2 (1 2)
+#define SYSCFG_CHIPINT3 (1 3)
+
+/**
+ * struct davinci_rproc - davinci remote processor state
+ * @rproc: rproc handle
Add @dsp_clk ?
+ */
+struct davinci_rproc {
+ struct rproc *rproc;
+ struct clk *dsp_clk;
+};
+
+static void __iomem *syscfg0_base;
+static struct platform_device *remoteprocdev;
+static struct irq_data *irq_data;
+static void (*ack_fxn)(struct irq_data *data);
+static int irq;
Is it safe to maintain these as globals (i.e. what if we have more
than a single pdev instance) ?
+
+/**
+ * handle_event() - inbound virtqueue message workqueue function
+ *
+ * This funciton is registered as a kernel thread and is scheduled by the
typo
+ * kernel handler.
+ */
+static irqreturn_t handle_event(int irq, void *p)
+{
+ struct rproc *rproc = platform_get_drvdata(remoteprocdev);
It's probably better to pass this as an irq cookie instead of relying
on global data
+
+ /* Process incoming buffers on our vring */
+ while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
+ ;
This is interesting. IIUC, you want a single irq event to trigger
processing of all the available messages. It makes sense, but I'm not
sure we need this to be platform-specific.
+
+ /* Must allow wakeup of potenitally blocking senders: */
+ rproc_vq_interrupt(rproc, 1);
IIUC, you do this is because you have a single irq for all the vrings (PCMIIW).
We may want to add something in these lines to the generic remoteproc
framework, as additional platforms require it (e.g. keystone). I
remember a similar patch by Cyril was circulating internally but never
hit the mailing lists - you may want to take it even though it would
probably need to be refreshed.
+static int davinci_rproc_start(struct rproc *rproc)
+{
+ struct platform_device *pdev = to_platform_device(rproc-dev.parent);
+ struct device *dev = rproc-dev.parent;
+ struct davinci_rproc *drproc = rproc-priv;
+ struct clk *dsp_clk;
+ struct resource *r;
+ unsigned long host1cfg_physaddr;
+ unsigned int host1cfg_offset;
+ int ret;
+
+ remoteprocdev = pdev;
+
+ /* hw requires the start (boot) address be on 1KB boundary */
+ if (rproc-bootaddr 0x3ff) {
+ dev_err(dev, invalid boot address: must be aligned to
1KB\n);
+
+ return -EINVAL;
+ }
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR_OR_NULL(r)) {
+ dev_err(dev, platform_get_resource() error: %ld\n,
+ PTR_ERR(r));
+
+ return PTR_ERR(r);
+ }
+ host1cfg_physaddr = (unsigned long)r-start;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0) {
+ dev_err(dev, platform_get_irq(pdev, 0) error: %d\n, irq);
+
+ return irq;
+ }
+
+ irq_data = irq_get_irq_data(irq);
+ if (IS_ERR_OR_NULL(irq_data)) {
+ dev_err(dev, irq_get_irq_data(%d) error: %ld\n,
+ irq, PTR_ERR(irq_data));
+
+ return PTR_ERR(irq_data);
+ }
+ ack_fxn = irq_data-chip-irq_ack;
+
+ ret = request_threaded_irq(irq, davinci_rproc_callback, handle_event,
+ 0, davinci-remoteproc, drproc);
+ if (ret) {
+ dev_err(dev, request_threaded_irq error: %d\n, ret);
+
+ return ret;
+ }
+
+ syscfg0_base = ioremap(host1cfg_physaddr PAGE_MASK, SZ_4K);
+ host1cfg_offset = offset_in_page(host1cfg_physaddr);
+ writel(rproc-bootaddr, syscfg0_base + host1cfg_offset);
+
+ dsp_clk = clk_get(dev, NULL);
+ if (IS_ERR_OR_NULL(dsp_clk)) {
+ dev_err(dev, clk_get error: %ld\n, PTR_ERR(dsp_clk));
+ ret = PTR_ERR(dsp_clk);
RE: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Hi Ohad,
Glad to see you jump in the fray, please see responses below...
-Original Message-
From: Ohad Ben-Cohen [mailto:o...@wizery.com]
Sent: Friday, January 11, 2013 4:26 AM
To: Tivy, Robert
Cc: davinci-linux-open-source; linux-arm; Nori, Sekhar; Ring, Chris;
Grosen, Mark; r...@landley.net; linux-...@vger.kernel.org; Chemparathy,
Cyril
Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for
OMAP-L138 DSP
Hi Robert,
I'm happy to see this driver going upstream, thanks for pushing!
Please see below my few comments. PS - sorry for the belated review.
On Fri, Jan 11, 2013 at 2:23 AM, Robert Tivy rt...@ti.com wrote:
+config DAVINCI_REMOTEPROC
+ tristate DaVinci DA850/OMAPL138 remoteproc support
(EXPERIMENTAL)
+ depends on ARCH_DAVINCI_DA850
+ select REMOTEPROC
+ select RPMSG
+ select FW_LOADER
This one gets selected by CONFIG_REMOTEPROC, so you don't need to do so
too.
From drivers/remoteproc/Kconfig:
# REMOTEPROC gets selected by whoever wants it
config REMOTEPROC
tristate
depends on EXPERIMENTAL
depends on HAS_DMA
select FW_CONFIG
select VIRTIO
Is FW_CONFIG above supposed to be FW_LOADER?
I don't see any support for FW_CONFIG anywhere in the kernel.
diff --git a/drivers/remoteproc/davinci_remoteproc.c
b/drivers/remoteproc/davinci_remoteproc.c
new file mode 100644
index 000..fc6fd72
--- /dev/null
+++ b/drivers/remoteproc/davinci_remoteproc.c
+static char *fw_name;
+module_param(fw_name, charp, S_IRUGO);
+MODULE_PARM_DESC(fw_name, \n\t\tName of DSP firmware file in
/lib/firmware);
+
+/*
+ * OMAP-L138 Technical References:
+ * http://www.ti.com/product/omap-l138
+ */
+#define SYSCFG_CHIPSIG_OFFSET 0x174
+#define SYSCFG_CHIPSIG_CLR_OFFSET 0x178
+#define SYSCFG_CHIPINT0 (1 0)
+#define SYSCFG_CHIPINT1 (1 1)
+#define SYSCFG_CHIPINT2 (1 2)
+#define SYSCFG_CHIPINT3 (1 3)
+
+/**
+ * struct davinci_rproc - davinci remote processor state
+ * @rproc: rproc handle
Add @dsp_clk ?
Yep.
+ */
+struct davinci_rproc {
+ struct rproc *rproc;
+ struct clk *dsp_clk;
+};
+
+static void __iomem *syscfg0_base;
+static struct platform_device *remoteprocdev;
+static struct irq_data *irq_data;
+static void (*ack_fxn)(struct irq_data *data);
+static int irq;
Is it safe to maintain these as globals (i.e. what if we have more
than a single pdev instance) ?
I think it makes sense for some of them to be globals, will review/change
accordingly.
+
+/**
+ * handle_event() - inbound virtqueue message workqueue function
+ *
+ * This funciton is registered as a kernel thread and is scheduled
by the
typo
Will fix.
+ * kernel handler.
+ */
+static irqreturn_t handle_event(int irq, void *p)
+{
+ struct rproc *rproc = platform_get_drvdata(remoteprocdev);
It's probably better to pass this as an irq cookie instead of relying
on global data
Agreed, will change.
+
+ /* Process incoming buffers on our vring */
+ while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
+ ;
This is interesting. IIUC, you want a single irq event to trigger
processing of all the available messages. It makes sense, but I'm not
sure we need this to be platform-specific.
YUC :)
+
+ /* Must allow wakeup of potenitally blocking senders: */
+ rproc_vq_interrupt(rproc, 1);
IIUC, you do this is because you have a single irq for all the vrings
(PCMIIW).
YUC again.
We may want to add something in these lines to the generic remoteproc
framework, as additional platforms require it (e.g. keystone). I
remember a similar patch by Cyril was circulating internally but never
hit the mailing lists - you may want to take it even though it would
probably need to be refreshed.
Ok, I got Cyril's patch (sent privately by you) and will incorporate it in the
generic processing, and modify davinci_remoteproc.c to:
while (IRQ_HANDLED == rproc_vq_interrupt(rproc, -1))
;
+static int davinci_rproc_start(struct rproc *rproc)
+{
+ struct platform_device *pdev = to_platform_device(rproc-
dev.parent);
+ struct device *dev = rproc-dev.parent;
+ struct davinci_rproc *drproc = rproc-priv;
+ struct clk *dsp_clk;
+ struct resource *r;
+ unsigned long host1cfg_physaddr;
+ unsigned int host1cfg_offset;
+ int ret;
+
+ remoteprocdev = pdev;
+
+ /* hw requires the start (boot) address be on 1KB boundary */
+ if (rproc-bootaddr 0x3ff) {
+ dev_err(dev, invalid boot address: must be aligned
to 1KB\n);
+
+ return -EINVAL;
+ }
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR_OR_NULL(r)) {
+ dev_err
