Hello.
On 26-01-2013 6:45, Robert Tivy wrote:
Adding a remoteproc driver implementation for OMAP-L138 DSP
You forgot to sign off the patch.
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 96ce101..e923599 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
[...]
@@ -41,4 +41,28 @@ config STE_MODEM_RPROC
This can be either built-in or a loadable module.
If unsure say N.
+config DA8XX_REMOTEPROC
+ tristate "DaVinci DA850/OMAPL138 remoteproc support (EXPERIMENTAL)"
Neither DA850 nor OMAP-L138 are true DaVinci processors. Please drop the
"DaVinci" word.
+ depends on ARCH_DAVINCI_DA850
It's also not clear why you limit the driver d\to DA850 while you call it
da8xx_remoteproc.c. There's at least one more member to DA8xx familiy (at
least supported in the community): DA830/OMAP-L137.
+ select REMOTEPROC
+ select RPMSG
+ select CMA
+ default n
+ help
+ Say y here to support DaVinci DA850/OMAPL138 remote processors
Same here.
diff --git a/drivers/remoteproc/da8xx_remoteproc.c
b/drivers/remoteproc/da8xx_remoteproc.c
new file mode 100644
index 0000000..c6eb6bf
--- /dev/null
+++ b/drivers/remoteproc/da8xx_remoteproc.c
@@ -0,0 +1,327 @@
+/*
+ * Remote processor machine-specific module for DA8XX
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * 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.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include <mach/clock.h> /* for davinci_clk_reset_assert/deassert() */
+
+#include "remoteproc_internal.h"
+
+static char *da8xx_fw_name;
+module_param(da8xx_fw_name, charp, S_IRUGO);
+MODULE_PARM_DESC(da8xx_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
<mach/da8xx.h> has SYSCFG register #define's ending with '_REG', not
'_OFFSET' -- I'd like this tradition to be kept. And perhaps we should #define
these registers there instead of the driver?
+#define SYSCFG_CHIPINT0 BIT(0)
+#define SYSCFG_CHIPINT1 BIT(1)
+#define SYSCFG_CHIPINT2 BIT(2)
+#define SYSCFG_CHIPINT3 BIT(3)
DA830/OMAP-l137 has the same registers. Only the datasheet calls the bits
CHIPSIGn there. Bit 4 also exists and means DSP NMI.
+static int da8xx_rproc_probe(struct platform_device *pdev)
[...]
+ chipsig = devm_request_and_ioremap(dev, chipsig_res);
+ if (!chipsig) {
+ dev_err(dev, "unable to map CHIPSIG register\n");
+ return -EINVAL;
Why -EINVAL? Comment to devm_request_and_ioremap() suggests returning
-EADDRNOTAVAIL.
+ }
+
+ bootreg = devm_request_and_ioremap(dev, bootreg_res);
+ if (!bootreg) {
+ dev_err(dev, "unable to map boot register\n");
+ return -EINVAL;
Same here.
+static int __devexit da8xx_rproc_remove(struct platform_device *pdev)
+{
+ struct rproc *rproc = platform_get_drvdata(pdev);
+ struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
+ int ret;
+
+ /*
+ * The devm subsystem might end up releasing things before
+ * freeing the irq, thus allowing an interrupt to sneak in while
+ * the device is being removed. This should prevent that.
+ */
+ disable_irq(drproc->irq);
Will the IRQ be enabled properly upon reloading the driver? You're
effectively disabling it twice, once here and once in devm_free_irq(), aren't you?
+static struct platform_driver da8xx_rproc_driver = {
+ .probe = da8xx_rproc_probe,
+ .remove = __devexit_p(da8xx_rproc_remove),
Isn't _devexit_p() removed now? I thought __devinit and friends have all
been removed in 3.8-rc1...
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index dd3bfaf..ac4449a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -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)
It makes sense to use {} despite singkle-statement branch.
+ /*
Indent with tabs please.
+ * 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;
Same here.
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);
+ }
+ else
+ p = (char *)firmware;
+
+ rproc->firmware = p;
Same here.
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
