[PATCH v6 1/2] ARM: davinci: Remoteproc driver support for OMAP-L138 DSP

[sshtylyov@mvista.com (Sergei Shtylyov)]

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