ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Subsystems (3)
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): [email protected]
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.
Review
Quoted 6 lines · Robert Tivy
(quoted diff) +6
-0+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.
Quoted 13 lines
drivers/remoteproc/davinci_remoteproc.c new file +8
-0+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 +*/
drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+#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 ?
drivers/remoteproc/davinci_remoteproc.c
Quoted 11 lines
+ */
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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) ?
drivers/remoteproc/davinci_remoteproc.c
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +5
-0+ +/** +* handle_event() - inbound virtqueue message workqueue function +* +* This funciton is registered as a kernel thread and is scheduled by the
typo
drivers/remoteproc/davinci_remoteproc.c
Quoted 5 lines
+ * kernel handler.
+ */drivers/remoteproc/davinci_remoteproc.c new file +3
-0+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
drivers/remoteproc/davinci_remoteproc.c
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+ +/* 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.
drivers/remoteproc/davinci_remoteproc.c
Quoted 3 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+ +/* 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.
Quoted 53 lines
drivers/remoteproc/davinci_remoteproc.c new file +53
-0+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; + } +
drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ 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); + goto fail; + }
There's a lot in this ->start() method that better move to ->probe() because it should be invoked only once throughout the life cycle of this driver (probably most of the code above). Can you please take a look?
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c new file +8
-0+/* kick a virtqueue */ +static void davinci_rproc_kick (struct rproc *rproc,int vqid) +{ +int timed_out; +unsigned long timeout; + + timed_out =0 ; + timeout = jiffies +HZ /100 ;
drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ +/* Poll for ack from other side first */ +while (readl (syscfg0_base +SYSCFG_CHIPSIG_OFFSET ) & +SYSCFG_CHIPINT2 ) +if (time_after (jiffies, timeout)) { +dev_err (rproc->dev.parent,"failed to receive ack\n" ); + timed_out =1 ; + +break ; + }
Can you please explain what this ack is a bit ? Just out of curiosity.
drivers/remoteproc/davinci_remoteproc.c
Quoted 13 lines
drivers/remoteproc/davinci_remoteproc.c new file +13
-0+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.
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'.
drivers/remoteproc/davinci_remoteproc.c
Quoted 12 lines
drivers/remoteproc/davinci_remoteproc.c new file +12
-0+ 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);
This looks racy. Don't you prefer to assert the reset line before you call rproc_add ?
drivers/remoteproc/davinci_remoteproc.c
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c new file +8
-0+static int __devexit davinci_rproc_remove(struct platform_device *pdev) +{ +struct rproc *rproc =platform_get_drvdata (pdev); +int ret; + + ret =rproc_del (rproc); +if (ret) +return ret;
Personally I'd not test ret here.
I know there's a nice check for !rproc inside rproc_del, but frankly I'd prefer the system to crash if the developer blew up so badly. It's nothing I'd want to be silently buried.
Quoted 15 lines
include/linux/platform_data/da8xx-remoteproc.h new file +9
-0@@ -0,0 +1,33 @@ +/* +* Remote Processor +* +* Copyright (C) 2011-2012 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/platform_data/da8xx-remoteproc.h line 19
Quoted 10 lines
include/linux/platform_data/da8xx-remoteproc.h new file +10
-0+* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __DA8XX_REMOTEPROC_H__ +#define __DA8XX_REMOTEPROC_H__ + +#include <linux/remoteproc.h>
You should be able to avoid including this header by forward declaring struct rproc_ops.
Thanks,
Ohad.
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...
Quoted 25 lines
-----Original Message-----
From: Ohad Ben-Cohen [mailto:ohad at 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; rob at landley.net; linux-doc at vger.kernel.org; Chemparathy, Cyril
Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSPHi 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.
Review
Quoted 2 lines · Robert Tivy
(quoted diff) +2
-0+config DAVINCI_REMOTEPROC + tristate "DaVinci DA850/OMAPL138 remoteproc support(EXPERIMENTAL)"
Review
Quoted 4 lines
(quoted diff) +4
-0+ depends on ARCH_DAVINCI_DA850 + select REMOTEPROC + select RPMSG + select FW_LOADER
Quoted 4 lines
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.
Quoted 20 lines
Review
Quoted 2 lines
(quoted diff) +0
-0
b/drivers/remoteproc/davinci_remoteproc.cdrivers/remoteproc/davinci_remoteproc.c
Quoted 7 lines
new file mode 100644
index 0000000..fc6fd72drivers/remoteproc/davinci_remoteproc.c new file +3
-0+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");Quoted 9 lines
drivers/remoteproc/davinci_remoteproc.c new file +9
-0+ +/* +* 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)
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 6 lines
drivers/remoteproc/davinci_remoteproc.c new file +6
-0+#define SYSCFG_CHIPINT2 (1 << 2) +#define SYSCFG_CHIPINT3 (1 << 3) + +/** +* struct davinci_rproc - davinci remote processor state +* @rproc: rproc handle Add @dsp_clk ?
Yep.
Quoted 15 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 12 lines
+ */
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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.
Quoted 9 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 6 lines
drivers/remoteproc/davinci_remoteproc.c new file +5
-0+ +/** +* handle_event() - inbound virtqueue message workqueue function +* +* This funciton is registered as a kernel thread and is scheduled by the
typo
Will fix.
Quoted 9 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 6 lines
+ * kernel handler.
+ */drivers/remoteproc/davinci_remoteproc.c new file +3
-0+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.
Quoted 9 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+ +/* 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 :)
Quoted 7 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+ +/* 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.
Quoted 6 lines
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)) ;
Quoted 67 lines
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+static int davinci_rproc_start (struct rproc *rproc) +{ + struct platform_device *pdev = to_platform_device(rproc-dev.parent);
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+struct device *dev = rproc->dev.parent; +struct davinci_rproc *drproc = rproc->priv; +struct clk *dsp_clk;drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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 alignedto 1KB\n");
Quoted 6 lines
drivers/remoteproc/davinci_remoteproc.c new file +6
-0+ +return -EINVAL ; + } + + r =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (IS_ERR_OR_NULL (r)) {drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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);
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+ +return irq; + } +drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ 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,drivers/remoteproc/davinci_remoteproc.c
Quoted 3 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+ 0, "davinci-remoteproc", drproc); +if (ret) { + dev_err(dev, "request_threaded_irq error: %d\n",ret);
drivers/remoteproc/davinci_remoteproc.c
Quoted 11 lines
drivers/remoteproc/davinci_remoteproc.c new file +11
-0+ +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));
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 3 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+ ret =PTR_ERR (dsp_clk); + goto fail; + }There's a lot in this ->start() method that better move to ->probe() because it should be invoked only once throughout the life cycle of this driver (probably most of the code above). Can you please take a look?
Will take a look.
Quoted 16 lines
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+/* kick a virtqueue */ +static void davinci_rproc_kick (struct rproc *rproc,int vqid) +{ +int timed_out;drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+unsigned long timeout; + + timed_out =0 ; + timeout = jiffies +HZ /100 ; + +/* Poll for ack from other side first */ +while (readl (syscfg0_base +SYSCFG_CHIPSIG_OFFSET ) & +SYSCFG_CHIPINT2 ) +if (time_after (jiffies, timeout)) { + dev_err(rproc->dev.parent, "failed to receiveack\n");
Quoted 6 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+ timed_out =1 ; + +break ; + }Can you please explain what this ack is a bit ? Just out of curiosity.
We're currently handling the CHIPINT lines as "level"s, 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. For the OMAP-L138 DSP VirtQueue implementation (in the sysbios-rpmsg repo) this is currently the case - each DSP interrupt indicates new vring availability. This behavior is due to the fact that the OMAP-L138 port is trying to emulate an omap mailbox.
Quoted 17 lines
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+static int davinci_rproc_probe (struct platform_device *pdev) +{ +struct da8xx_rproc_pdata *pdata = pdev->dev.platform_data;drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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_namemodule param:
Quoted 4 lines
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?
Quoted 5 lines
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).
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.
Quoted 9 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c new file +7
-0+ 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
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +5
-0+* 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);This looks racy. Don't you prefer to assert the reset line before you call
rproc_add?
I would prefer that, as long as the reset state is not changed by rproc_add(). I will look into it.
Quoted 13 lines
Quoted 2 lines
+static int
__devexitdavinci_rproc_remove(structplatform_device*pdev)
drivers/remoteproc/davinci_remoteproc.c
Quoted 7 lines
drivers/remoteproc/davinci_remoteproc.c new file +7
-0+{ +struct rproc *rproc =platform_get_drvdata (pdev); +int ret; + + ret =rproc_del (rproc); +if (ret) +return ret;Personally I'd not test ret here.
Quoted 3 lines
I know there's a nice check for !rproc inside
rproc_del, but frankly I'd prefer the system to crash if the developer blew up so badly. It's nothing I'd want to be silently buried.
Ok, I'll trust you on this one :)
Quoted 22 lines
Review
Quoted 2 lines
(quoted diff) +0
-0
b/include/linux/platform_data/da8xx-remoteproc.hQuoted 19 lines
new file mode 100644
index 0000000..50e8c55include/linux/platform_data/da8xx-remoteproc.h new file +14
-0@@ -0,0 +1,33 @@ +/* +* Remote Processor +* +* Copyright (C) 2011-2012 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/
Quoted 8 lines
include/linux/platform_data/da8xx-remoteproc.h line 19
Quoted 5 lines
include/linux/platform_data/da8xx-remoteproc.h new file +5
-0+ +#ifndef __DA8XX_REMOTEPROC_H__ +#define __DA8XX_REMOTEPROC_H__ + +#include <linux/remoteproc.h> You should be able to avoid including this header by forward declaring struct
rproc_ops.
Will do.
Thanks & Regards,
- Rob
Quoted 3 lines
Thanks,
Ohad.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 20 lines · Robert
Quoted 20 lines
From: Ohad Ben-Cohen [mailto:ohad at wizery.com]
Sent: Friday, January 11, 2013 4:26 AMReview
Quoted 13 lines · Robert Tivy
(quoted diff) +13
-0+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_namemodule param:
Quoted 6 lines
Quoted 4 lines
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).
Quoted 8 lines
Quoted 6 lines
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.
Quoted 2 lines
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
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 1 line · Sekhar Nori
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.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 8 lines · Ohad Ben-Cohen
Quoted 1 line · Sekhar Nori
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
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 2 lines · Sekhar Nori
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.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 11 lines
-----Original Message-----
From: Ohad Ben-Cohen [mailto:ohad at 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; rob at landley.net; linux-doc at vger.kernel.org; Chemparathy, Cyril
Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSPOn Tue, Jan 15, 2013 at 2:29 PM, Sekhar Nori <[email protected]> wrote:
Quoted 8 lines
Quoted 2 lines
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->nameassuming 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
Quoted 3 lines
Thanks,
Ohad.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 1 line · Robert
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.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
In reply to Ohad Ben-CohenQuoted 9 lines · Ohad Ben-Cohen
Quoted 2 lines · Sekhar Nori
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->nameassuming 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
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
In reply to Tivy, RobertHi Rob,
Quoted 1 line · Robert
Is
FW_CONFIGabove supposed to beFW_LOADER?
That FW_CONFIG is completely bogus of course. care to fix it in your series?
Quoted 1 line
We're currently handling the CHIPINT lines as "level"s, 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) ?
Quoted 1 line
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.
Quoted 3 lines
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.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
In reply to Ohad Ben-CohenQuoted 14 lines · Ohad Ben-Cohen
Quoted 14 lines
(quoted diff) +14
-0+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 */
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ 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.
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ dev_err(dev, "platform_get_resource() error: %ld\n", + PTR_ERR(r)); + + return PTR_ERR(r);
Which results in this being a bug.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+ } + host1cfg_physaddr = (unsigned long)r->start; + + irq = platform_get_irq(pdev, 0);
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-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.
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Quoted 9 lines
Quoted 9 lines
(quoted diff) +9
-0+ } + 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;
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ } + + 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.
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+ goto fail; + }
...
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+ ret = rproc_add(rproc); + if (ret) + goto free_rproc; +
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* + * 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?
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
In reply to a message not in the archiveQuoted 8 lines · Robert Tivy
Adding a remoteproc driver implementation for OMAP-L138 DSP
Signed-off-by Robert Tivy <[email protected]>--- 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?
Quoted 20 lines
drivers/remoteproc/Kconfig +12
-0@@ -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. +
drivers/remoteproc/Kconfig config STE_MODEM_RPROC · line 65
Quoted 10 lines
drivers/remoteproc/Kconfig +9
-0+ 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.
drivers/remoteproc/davinci_remoteproc.c line 4
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0@@ -0,0 +1,307 @@ +/* +* Remote processor machine-specific module for Davinci +* +* Copyright (C) 2011-2012 Texas Instruments, Inc.
2013?
drivers/remoteproc/davinci_remoteproc.c line 11
Quoted 7 lines
drivers/remoteproc/davinci_remoteproc.c new file +7
-0+* +* 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.
drivers/remoteproc/davinci_remoteproc.c line 24
Quoted 13 lines
drivers/remoteproc/davinci_remoteproc.c new file +13
-0+ +#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.
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+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" ); +
drivers/remoteproc/davinci_remoteproc.c line 38
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+/* +* 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.
Quoted 38 lines
drivers/remoteproc/davinci_remoteproc.c new file +38
-0+ +/** +* 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
drivers/remoteproc/davinci_remoteproc.c line 86
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+* +* 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 think using a variable to read the register and then testing its value inside if() is more readable.
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +4
-0+/* Clear interrupt level source */ +writel (SYSCFG_CHIPINT0 , + syscfg0_base +SYSCFG_CHIPSIG_CLR_OFFSET ); +
drivers/remoteproc/davinci_remoteproc.c line 100
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+/* +* ACK intr to AINTC. +* +* It has already been ack'ed by the kernel before calling +* this function, but since the ARM<->DSP interrupts in the +* CHIPSIG register are "level" instead of "pulse" variety, +* we need to ack it after taking down the level else we'll +* be called again immediately after returning. +*/ +ack_fxn (irq_data);
Don't really like interrupt controller functions being invoked like this but I don't understand the underlying issue well enough to suggest an alternate.
Quoted 18 lines
drivers/remoteproc/davinci_remoteproc.c new file +18
-0+ +return IRQ_WAKE_THREAD ; + } + +return IRQ_HANDLED ; +} + +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; +
drivers/remoteproc/davinci_remoteproc.c line 128
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ 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 );
Along with moving this to probe as Ohad requested, you can use devm_request_and_ioremap() to simplify the error paths here. Have a look
at: Documentation/driver-model/devres.txt
Quoted 27 lines
drivers/remoteproc/davinci_remoteproc.c new file +27
-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) {
drivers/remoteproc/davinci_remoteproc.c line 165
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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.
Quoted 33 lines
drivers/remoteproc/davinci_remoteproc.c new file +33
-0+if (IS_ERR_OR_NULL (dsp_clk)) { +dev_err (dev,"clk_get error: %ld\n" ,PTR_ERR (dsp_clk)); + ret =PTR_ERR (dsp_clk); + goto fail; + } +clk_enable (dsp_clk); +davinci_clk_reset_deassert (dsp_clk); + + drproc->dsp_clk = dsp_clk; + +return 0 ; +fail: +free_irq (irq, drproc); +iounmap (syscfg0_base); + +return ret; +} + +static int davinci_rproc_stop (struct rproc *rproc) +{ +struct davinci_rproc *drproc = rproc->priv; +struct clk *dsp_clk = drproc->dsp_clk; + +clk_disable (dsp_clk); +clk_put (dsp_clk); +iounmap (syscfg0_base); +free_irq (irq, drproc); + +return 0 ; +} + +/* kick a virtqueue */ +static void davinci_rproc_kick (struct rproc *rproc,int vqid)
drivers/remoteproc/davinci_remoteproc.c line 208
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+{ +int timed_out; +unsigned long timeout; + + timed_out =0 ; + timeout = jiffies +HZ /100 ; + +/* Poll for ack from other side first */ +while (readl (syscfg0_base +SYSCFG_CHIPSIG_OFFSET ) & +SYSCFG_CHIPINT2 )
If there is a context switch here long enough ..
drivers/remoteproc/davinci_remoteproc.c line 209
Quoted 1 line
drivers/remoteproc/davinci_remoteproc.c new file +1
-0+if (time_after (jiffies, timeout)) {
.. then time_after() might return true and you will erroneously report a timeout even though hardware is working perfectly fine.
Thanks,
Sekhar
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
Quoted 5 lines · Sekhar Nori
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (IS_ERR_OR_NULL(r)) { + dev_err(dev, "platform_get_resource() error: %ld\n", + PTR_ERR(r)); + + return PTR_ERR(r);
Sigh. Bug.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+ } + 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;
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ } + + 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.
Quoted 19 lines
Quoted 6 lines
(quoted diff) +6
-0+ } + ack_fxn = irq_data->chip->irq_ack; + + ret = request_threaded_irq(irq, davinci_rproc_callback, handle_event, + 0, "davinci-remoteproc", drproc); + if (ret) {Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
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
Quoted 65 lines
-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Monday, January 21, 2013 8:41 AM
To: Nori, Sekhar
Cc: Tivy, Robert; ohad at wizery.com; davinci-linux-open- source at linux.davincidsp.com; linux-doc at vger.kernel.org; Ring, Chris; rob at landley.net; Grosen, Mark; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v5 6/9] ARM: davinci: Remoteproc driver support for OMAP-L138 DSPQuoted 5 lines · Sekhar Nori
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (IS_ERR_OR_NULL(r)) { + dev_err(dev, "platform_get_resource() error: %ld\n", + PTR_ERR(r)); + + return PTR_ERR(r);Sigh. Bug.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+ } + 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;Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ } + + 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.
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ } + ack_fxn = irq_data->chip->irq_ack; + + ret = request_threaded_irq(irq, davinci_rproc_callback,
handle_event,Quoted 15 lines
Review
Quoted 12 lines
(quoted diff) +12
-0+ 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.Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
PATCH v5 6/9 ARM: davinci: Remoteproc driver support for OMAP-L138 DSP
In reply to Sekhar NoriQuoted 12 lines
-----Original Message-----
From: Nori, Sekhar
Sent: Sunday, January 20, 2013 9:39 PMQuoted 7 lines · Robert Tivy
Adding a remoteproc driver implementation for OMAP-L138 DSP
Signed-off-by Robert Tivy <[email protected]>--- drivers/remoteproc/Kconfig | 23 ++ drivers/remoteproc/Makefile | 1 + drivers/remoteproc/davinci_remoteproc.c | 307
Quoted 7 lines
++++++++++++++++++++++++
Quoted 1 line
include/linux/platform_data/da8xx-remoteproc.h | 33 +++naming the driver
davinci_remoteproc.cand platform data asda8xx-remoteproc.his odd. The driver looks really specific to omap- l138
so may be call the driverda8xx-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_".
Quoted 29 lines
drivers/remoteproc/Kconfig config STE_MODEM_RPROC · line 45
Quoted 11 lines
drivers/remoteproc/Kconfig +2
-0@@ -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)"
Quoted 16 lines
drivers/remoteproc/Kconfig +16
-0+ 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.
Quoted 8 lines
drivers/remoteproc/Kconfig config STE_MODEM_RPROC · line 64
Quoted 3 lines
drivers/remoteproc/Kconfig +3
-0+ 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.
Quoted 14 lines
Review
Quoted 2 lines
(quoted diff) +0
-0
b/drivers/remoteproc/davinci_remoteproc.cdrivers/remoteproc/davinci_remoteproc.c line 4
Quoted 9 lines
new file mode 100644
index 0000000..fc6fd72drivers/remoteproc/davinci_remoteproc.c new file +4
-0@@ -0,0 +1,307 @@ +/* +* Remote processor machine-specific module for Davinci +* +* Copyright (C) 2011-2012 Texas Instruments, Inc. 2013?
Yep.
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c line 11
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c new file +7
-0+* +* 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.
Quoted 17 lines
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+ +#include <linux/kernel.h> +#include <linux/err.h> drivers/remoteproc/davinci_remoteproc.c line 24
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+#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
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c line 27
Quoted 4 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+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");
Quoted 13 lines
drivers/remoteproc/davinci_remoteproc.c line 38
Quoted 11 lines
drivers/remoteproc/davinci_remoteproc.c new file +11
-0+ +/* +* 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.
Quoted 55 lines
Quoted 11 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ +/** +* struct davinci_rproc - davinci remote processor state +* @rproc: rproc handle +*/ +struct davinci_rproc { +struct rproc *rproc; +struct clk *dsp_clk; +}; +drivers/remoteproc/davinci_remoteproc.c line 58
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+* 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 )) + ; +drivers/remoteproc/davinci_remoteproc.c line 78
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-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
drivers/remoteproc/davinci_remoteproc.c line 80
Quoted 2 lines
drivers/remoteproc/davinci_remoteproc.c new file +2
-0+* core (DSP) has modified the state of a virtqueue. There is no +* "payload" message indicating the virtqueue index as is the case with
drivers/remoteproc/davinci_remoteproc.c line 81
Quoted 1 line
drivers/remoteproc/davinci_remoteproc.c new file +1
-0+* mailbox-based implementations on OMAP4. As such, this handler "polls"
drivers/remoteproc/davinci_remoteproc.c line 86
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +5
-0+* 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) {
Quoted 2 lines
personally I think using a variable to read the register and then testing its value inside
if()is more readable.
Agreed.
Quoted 12 lines
Quoted 12 lines
drivers/remoteproc/davinci_remoteproc.c new file +11
-0+/* Clear interrupt level source */ +writel (SYSCFG_CHIPINT0 , + syscfg0_base +SYSCFG_CHIPSIG_CLR_OFFSET ); + +/* +* ACK intr to AINTC. +* +* It has already been ack'ed by the kernel before calling +* this function, but since the ARM<->DSP interrupts in the +* CHIPSIG register are "level" instead of "pulse" variety, +* we need to ack it after taking down the level else we'll
Quoted 8 lines
drivers/remoteproc/davinci_remoteproc.c line 100
Quoted 3 lines
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+* be called again immediately after returning. +*/ +ack_fxn (irq_data);Don't really like interrupt controller functions being invoked like this
but I don't understand the underlying issue well enough to suggest an alternate.
I've looked for an alternative with no success, but will start a discussion in earnest after tackling this initial commit.
Quoted 26 lines
Quoted 15 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ +return IRQ_WAKE_THREAD ; + } + +return IRQ_HANDLED ; +} + +static int davinci_rproc_start (struct rproc *rproc) +{ + struct platform_device *pdev = to_platform_device(rproc-dev.parent);
drivers/remoteproc/davinci_remoteproc.c new file +3
-0+struct device *dev = rproc->dev.parent; +struct davinci_rproc *drproc = rproc->priv; +struct clk *dsp_clk;drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+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 to1KB\n");
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 5 lines
drivers/remoteproc/davinci_remoteproc.c new file +5
-0+ +return -EINVAL ; + } + + r =platform_get_resource (pdev,IORESOURCE_MEM ,0 );Along with moving this to probe as Ohad requested, you can use
devm_request_and_ioremap()to simplify the error paths here. Have a look
at:Documentation/driver-model/devres.txt
So it seems that I should also use devm_request_threaded_irq()?
In general, should I always use the "devm_*" APIs when available?
Quoted 27 lines
Quoted 16 lines
drivers/remoteproc/davinci_remoteproc.c new file +15
-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; + } +drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+ 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,
Quoted 14 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 12 lines
drivers/remoteproc/davinci_remoteproc.c new file +12
-0+ 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.
OK.
Quoted 47 lines
Quoted 34 lines
drivers/remoteproc/davinci_remoteproc.c new file +33
-0+if (IS_ERR_OR_NULL (dsp_clk)) { +dev_err (dev,"clk_get error: %ld\n" ,PTR_ERR (dsp_clk)); + ret =PTR_ERR (dsp_clk); + goto fail; + } +clk_enable (dsp_clk); +davinci_clk_reset_deassert (dsp_clk); + + drproc->dsp_clk = dsp_clk; + +return 0 ; +fail: +free_irq (irq, drproc); +iounmap (syscfg0_base); + +return ret; +} + +static int davinci_rproc_stop (struct rproc *rproc) +{ +struct davinci_rproc *drproc = rproc->priv; +struct clk *dsp_clk = drproc->dsp_clk; + +clk_disable (dsp_clk); +clk_put (dsp_clk); +iounmap (syscfg0_base); +free_irq (irq, drproc); + +return 0 ; +} + +/* kick a virtqueue */ +static void davinci_rproc_kick (struct rproc *rproc,int vqid)drivers/remoteproc/davinci_remoteproc.c
Quoted 10 lines
drivers/remoteproc/davinci_remoteproc.c new file +10
-0+{ +int timed_out; +unsigned long timeout; + + timed_out =0 ; + timeout = jiffies +HZ /100 ; + +/* Poll for ack from other side first */ +while (readl (syscfg0_base +SYSCFG_CHIPSIG_OFFSET ) & +SYSCFG_CHIPINT2 )If there is a context switch here long enough ..
Quoted 6 lines
drivers/remoteproc/davinci_remoteproc.c
Quoted 1 line
drivers/remoteproc/davinci_remoteproc.c new file +1
-0+if (time_after (jiffies, timeout)) {.. then
time_after()might return true and you will erroneously report a
timeout even though hardware is working perfectly fine.
I will probably drop the whole check completely.
Otherwise I'll add another check after detecting timeout.
Thanks & Regards,
- Rob
Quoted 3 lines
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 1/9 ARM: davinci: da850 board: change pr_warning() to pr_warn()
Quoted 7 lines · Robert Tivy
Changed all
pr_warning()calls topr_warn(), as advised by checkpatch.pl.Also, while modifying those
pr_warning()calls I changed hardcoded function names to use '"%s:",__func__' instead, and converted acronym usage to upper caseSigned-off-by Robert Tivy <[email protected]>
Applying this patch for v3.9. While committing I updated the commit message to change past tense to present and also removed the first person reference.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 2/9 ARM: davinci: devices-da8xx.c: change pr_warning() to pr_warn()
Quoted 24 lines · Robert Tivy
Changed all
pr_warning()calls topr_warn(), as advised by checkpatch.pl.Signed-off-by Robert Tivy <[email protected]>---
Clean up files that will be otherwise modified in subsequent patch.arch/arm/mach-davinci/devices-da8xx.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm/mach-davinci/devices-da8xx.c +1
-1@@ -751,7 +751,7 @@ void __iomem * __init da8xx_get_mem_ctlr(void) da8xx_ddr2_ctlr_base =ioremap (DA8XX_DDR2_CTL_BASE ,SZ_32K );if (!da8xx_ddr2_ctlr_base) -pr_warning ("%s: Unable to map DDR2 controller" , __func__); +pr_warn ("%s: Unable to map DDR2 controller" , __func__);return da8xx_ddr2_ctlr_base; }
arch/arm/mach-davinci/devices-da8xx.c int __init da8xx_register_spi(int instance, const struct spi_board_info *info, · line 880
Quoted 7 lines
arch/arm/mach-davinci/devices-da8xx.c +2
-2@@ -876,8 +876,8 @@ int __init da8xx_register_spi(int instance, const struct spi_board_info *info, ret =spi_register_board_info (info, len);if (ret) -pr_warning ("%s: failed to register board info for spi %d :" -" %d\n" , __func__, instance, ret); +pr_warn ("%s: failed to register board info for spi %d : %d\n" , + __func__, instance, ret);
This part is not required anymore since this is already fixed by Vivien Didelot as part of his SPI fixes. I applied rest of the patch for v3.9. As with previous patch, I fixed the past tense to present in commit text while applying.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 4/9 ARM: davinci: da850: added pll0_sysclk1 for DSP usage
Quoted 3 lines · Robert Tivy
Added
pll0_sysclk1to allow clocking control for DSPSigned-off-by Robert Tivy <[email protected]>
Applied patches 3 and 4 as well.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 5/9 ARM: davinci: New reset functionality/API provided for Davinci DSP
Quoted 8 lines · Robert Tivy
Since there is no general "reset" support for SoC devices, and since the remoteproc driver needs explicit control of the DSP's reset line, a new Davinci-specific API is added.
This private API will disappear with DT migration. Some discussion regarding a proposed DT "reset" binding is here:
https://patchwork.kernel.org/patch/1635051/
Quoted 5 lines
Modified
davinci_clk_init()to set clk "reset" function for clocks that indicatePSC_LRSTsupport. Also fixed indentation issue with function opening curly brace.Signed-off-by Robert Tivy <[email protected]>
I applied this patch for v3.9. The subject seemed too long with repeated references to davinci so I shortened it to:
ARM: davinci: psc: introduce reset API
Quoted 11 lines
arch/arm/mach-davinci/include/mach/psc.h +1
-0@@ -246,6 +246,7 @@#define MDSTAT_STATE_MASK 0x3f#define PDSTAT_STATE_MASK 0x1f +#define MDCTL_LRST BIT(8)#define MDCTL_FORCE BIT(31)#define PDCTL_NEXT BIT(0)#define PDCTL_EPCGOOD BIT(8)
arch/arm/mach-davinci/include/mach/psc.h line 258
Quoted 5 lines
arch/arm/mach-davinci/include/mach/psc.h +2
-0@@ -253,6 +254,8 @@#ifndef __ASSEMBLER__extern int davinci_psc_is_clk_active (unsigned int ctlr,unsigned int id); +extern void davinci_psc_reset_config (unsigned int ctlr,unsigned int id, +bool reset);
I felt the word 'config' in the name is not really required since the functionality is fixed (as opposed to the davinci_psc_config() function which could do multiple configurations)
I updated the function name to davinci_psc_reset() when I committed the patch locally.
Hope that's okay with you.
Thanks,
Sekhar
PATCH v5 5/9 ARM: davinci: New reset functionality/API provided for Davinci DSP
Thankyou for all your help Sekhar. I'm fine with your fixups (both commentary and code naming).
Regards,
- Rob
Quoted 63 lines
-----Original Message-----
From: Nori, Sekhar
Sent: Thursday, January 17, 2013 3:33 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark; ohad at wizery.com; rob at landley.net; linux-doc at vger.kernel.org
Subject: Re: [PATCH v5 5/9] ARM: davinci: New reset functionality/API provided for Davinci DSPQuoted 1 line · Robert Tivy
Since there is no general "reset" support for SoC devices, and since
the
Quoted 1 line
remoteproc driver needs explicit control of the DSP's reset line, a
new
Quoted 11 lines
Davinci-specific API is added.
This private API will disappear with DT migration. Some discussion regarding a proposed DT "reset" binding is here:
https://patchwork.kernel.org/patch/1635051/Modified
davinci_clk_init()to set clk "reset" function for clocks that indicatePSC_LRSTsupport. Also fixed indentation issue with function opening curly brace.Signed-off-by Robert Tivy <[email protected]>I applied this patch for v3.9. The subject seemed too long with repeated
references to davinci so I shortened it to:ARM: davinci: psc: introduce reset API
Quoted 11 lines
arch/arm/mach-davinci/include/mach/psc.h +1
-0@@ -246,6 +246,7 @@#define MDSTAT_STATE_MASK 0x3f#define PDSTAT_STATE_MASK 0x1f +#define MDCTL_LRST BIT(8)#define MDCTL_FORCE BIT(31)#define PDCTL_NEXT BIT(0)#define PDCTL_EPCGOOD BIT(8)arch/arm/mach-davinci/include/mach/psc.h line 256
Quoted 3 lines
arch/arm/mach-davinci/include/mach/psc.h +0
-0@@ -253,6 +254,8 @@#ifndef __ASSEMBLER__ extern int davinci_psc_is_clk_active(unsigned int ctlr, unsigned intid);
arch/arm/mach-davinci/include/mach/psc.h line 257
Quoted 1 line
arch/arm/mach-davinci/include/mach/psc.h +1
-0+extern void davinci_psc_reset_config(unsigned int ctlr, unsigned intid,
arch/arm/mach-davinci/include/mach/psc.h line 258
Quoted 1 line
arch/arm/mach-davinci/include/mach/psc.h +1
-0+ bool reset);I felt the word 'config' in the name is not really required since the functionality is fixed (as opposed to the
davinci_psc_config()function which could do multiple configurations)I updated the function name to
davinci_psc_reset()when I committed the patch locally.Hope that's okay with you.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 7/9 ARM: davinci: Remoteproc platform device creation data/code
Quoted 25 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]>--- Documentation/kernel-parameters.txt | 7 +++ arch/arm/mach-davinci/devices-da8xx.c | 93 +++++++++++++++++++++++++++- arch/arm/mach-davinci/include/mach/da8xx.h | 4 ++ 3 files changed, 103 insertions(+), 1 deletion(-)Documentation/kernel-parameters.txt +1
-0@@ -44,6 +44,7 @@ parameter is applicable: AVR32 AVR32 architecture is enabled. AX25 Appropriate AX.25 support is enabled. BLACKFIN Blackfin architecture is enabled. + CMA Contiguous Memory Area support is enabled. DRM Direct Rendering Management support is enabled. DYNAMIC_DEBUG Build in debug messages and enable them at runtime EDD BIOS Enhanced Disk Drive Services (EDD) is enabled
Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 2642
Quoted 8 lines
Documentation/kernel-parameters.txt +5
-0@@ -2634,6 +2635,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted. Useful for devices that are detected asynchronously (e.g. USB and MMC devices). + rproc_mem=nn[KMG][@address] + [KNL,ARM,CMA] Remoteproc physical memory block. + Memory area to be used by remote processor image, + managed by CMA. Suitable defaults exist if not + specified.
There are no defaults now, right?
Quoted 31 lines
Documentation/kernel-parameters.txt +1
-0+ rw [KNL] Mount root device read-write on boot S [KNL] Run init in single modearch/arm/mach-davinci/devices-da8xx.c +8
-1@@ -12,10 +12,11 @@*/ #include <linux/init.h> #include <linux/platform_device.h> -#include <linux/dma-mapping.h> +#include <linux/dma-contiguous.h> #include <linux/serial_8250.h> #include <linux/ahci_platform.h> #include <linux/clk.h> +#include <linux/platform_data/da8xx-remoteproc.h> #include <mach/cputype.h> #include <mach/common.h> @@ -706,6 +707,96 @@ int __init da850_register_mmcsd1(struct davinci_mmc_config *config) }#endif +static struct resource da8xx_rproc_resources[] = { + {/* DSP boot address */ + .start =DA8XX_SYSCFG0_BASE +DA8XX_HOST1CFG_REG , + .end =DA8XX_SYSCFG0_BASE +DA8XX_HOST1CFG_REG +3 , + .flags =IORESOURCE_MEM , + },
arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct davinci_mmc_config *config) · line 725
Quoted 10 lines
arch/arm/mach-davinci/devices-da8xx.c +10
-0+ {/* dsp irq */ + .start = IRQ_DA8XX_CHIPINT0, + .end = IRQ_DA8XX_CHIPINT0, + .flags = IORESOURCE_IRQ, + }, +}; + +static struct da8xx_rproc_pdata rproc_pdata = { + .name ="dsp" , +};
Since the driver is only for da850 so the name of the remote processor is fixed and can probably be hardcoded in the driver itself.
Quoted 90 lines
arch/arm/mach-davinci/devices-da8xx.c +74
-0+ +static struct platform_device da8xx_dsp = { + .name ="davinci-rproc" , + .id =0 , + .dev = { + .platform_data = &rproc_pdata, + .coherent_dma_mask =DMA_BIT_MASK (32 ), + }, + .num_resources =ARRAY_SIZE (da8xx_rproc_resources), + .resource = da8xx_rproc_resources, +}; + +#if IS_ENABLED (CONFIG_DAVINCI_REMOTEPROC ) + +static phys_addr_t rproc_base__initdata ; +static unsigned long rproc_size__initdata ; + +static int __init early_rproc_mem (char *p) +{ +char *endp; + +if (p ==NULL ) +return 0 ; + + rproc_size =memparse (p, &endp); +if (*endp =='@' ) + rproc_base =memparse (endp +1 ,NULL ); + +return 0 ; +} +early_param ("rproc_mem" , early_rproc_mem); + +void __init da8xx_rproc_reserve_cma (void ) +{ +int ret; + +if (!rproc_base || !rproc_size) { +pr_err ("%s: 'rproc_mem=nn at address' badly specified\n" +" 'nn' and 'address' must both be non-zero\n" , + __func__); + +return ; + } + +pr_info ("%s: reserving 0x%lx @ 0x%lx...\n" , + __func__, rproc_size, (unsigned long )rproc_base); + + ret =dma_declare_contiguous (&da8xx_dsp.dev, rproc_size, rproc_base,0 ); +if (ret) +pr_err ("%s: dma_declare_contiguous failed %d\n" , __func__, ret); +} + +#else + +void __init da8xx_rproc_reserve_cma (void ) +{ +} + +#endif + +int __init da8xx_register_rproc (void ) +{ +int ret; + + ret =platform_device_register (&da8xx_dsp); +if (ret) { +pr_err ("%s: platform_device_register: %d\n" , __func__, ret); + +return ret; + } + +return 0 ; +}; +static struct resource da8xx_rtc_resources[] = { { .start =DA8XX_RTC_BASE ,arch/arm/mach-davinci/include/mach/da8xx.h +1
-0@@ -54,6 +54,7 @@ extern unsigned int da850_max_speed;#define DA8XX_SYSCFG0_BASE (IO_PHYS + 0x14000)#define DA8XX_SYSCFG0_VIRT (x) (da8xx_syscfg0_base + (x))#define DA8XX_JTAG_ID_REG 0x18 +#define DA8XX_HOST1CFG_REG 0x44#define DA8XX_CFGCHIP0_REG 0x17c#define DA8XX_CFGCHIP2_REG 0x184#define DA8XX_CFGCHIP3_REG 0x188
arch/arm/mach-davinci/include/mach/da8xx.h int __init da850_register_vpif_display · line 109
Quoted 4 lines
arch/arm/mach-davinci/include/mach/da8xx.h +1
-0@@ -105,6 +106,9 @@ int __init da850_register_vpif_displayint __init da850_register_vpif_capture (struct vpif_capture_config *capture_config);void da8xx_restart (char mode,const char *cmd); +void __init da8xx_rproc_reserve_cma (void );
No need of __init qualifier in function prototypes.
The patch looks good to me otherwise, but I will not apply it right away and instead wait for the driver to get accepted first.
Thanks,
Sekhar
PATCH v5 7/9 ARM: davinci: Remoteproc platform device creation data/code
Quoted 43 lines
-----Original Message-----
From: Nori, Sekhar
Sent: Monday, January 21, 2013 12:34 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark; ohad at wizery.com; rob at landley.net; linux-doc at vger.kernel.org
Subject: Re: [PATCH v5 7/9] ARM: davinci: Remoteproc platform device creation data/codeQuoted 9 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]>--- Documentation/kernel-parameters.txt | 7 +++ arch/arm/mach-davinci/devices-da8xx.c | 93+++++++++++++++++++++++++++-
Review
Quoted 4 lines
arch/arm/mach-davinci/include/mach/da8xx.h | 4 ++ 3 files changed, 103 insertions(+), 1 deletion(-)(quoted diff) +0
-0
b/Documentation/kernel-parameters.txtDocumentation/kernel-parameters.txt bytes respectively. Such letter suffixes can
Quoted 12 lines
index 363e348..e95afb1 100644
Documentation/kernel-parameters.txt +1
-0@@ -44,6 +44,7 @@ parameter is applicable: AVR32 AVR32 architecture is enabled. AX25 Appropriate AX.25 support is enabled. BLACKFIN Blackfin architecture is enabled. + CMA Contiguous Memory Area support is enabled. DRM Direct Rendering Management support is enabled. DYNAMIC_DEBUG Build in debug messages and enable them at runtime EDD BIOS Enhanced Disk Drive Services (EDD) is enabled@@ -2634,6 +2635,12 @@ bytes respectively. Such letter suffixes canalso be entirely omitted.
Quoted 3 lines
Documentation/kernel-parameters.txt +0
-0Useful for devices that are detected asynchronously (e.g. USB and MMC devices).
Quoted 7 lines
Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can · line 2642
Quoted 5 lines
Documentation/kernel-parameters.txt +5
-0+ rproc_mem=nn[KMG][@address] + [KNL,ARM,CMA] Remoteproc physical memory block. + Memory area to be used by remote processor image, + managed by CMA. Suitable defaults exist if not + specified.There are no defaults now, right?
That's right, booter must specify rproc_mem in u-boot bootargs.
Quoted 32 lines
arch/arm/mach-
Quoted 6 lines
Documentation/kernel-parameters.txt +1
-0+ rw [KNL] Mount root device read-write on boot S [KNL] Run init in single modearch/arm/mach- renamed from arch/arm/mach-davinci/devices-da8xx.c +0
-0
davinci/devices-da8xx.carch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct
Quoted 17 lines
index fb2f51b..e8016ca 100644
arch/arm/mach-davinci/devices-da8xx.c +2
-1@@ -12,10 +12,11 @@*/ #include <linux/init.h> #include <linux/platform_device.h> -#include <linux/dma-mapping.h> +#include <linux/dma-contiguous.h> #include <linux/serial_8250.h> #include <linux/ahci_platform.h> #include <linux/clk.h> +#include <linux/platform_data/da8xx-remoteproc.h> #include <mach/cputype.h> #include <mach/common.h> @@ -706,6 +707,96 @@ int __init da850_register_mmcsd1(struct
davinci_mmc_config*config)arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct · line 712
Quoted 6 lines
arch/arm/mach-davinci/devices-da8xx.c +3
-0}#endif +static struct resource da8xx_rproc_resources[] = { + {/* DSP boot address */ + .start = DA8XX_SYSCFG0_BASE +
DA8XX_HOST1CFG_REG,
Quoted 16 lines
arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct · line 725
Quoted 13 lines
arch/arm/mach-davinci/devices-da8xx.c +13
-0+ .end = DA8XX_SYSCFG0_BASE + DA8XX_HOST1CFG_REG + 3, + .flags = IORESOURCE_MEM, + }, + {/* dsp irq */ + .start = IRQ_DA8XX_CHIPINT0, + .end = IRQ_DA8XX_CHIPINT0, + .flags = IORESOURCE_IRQ, + }, +}; + +static struct da8xx_rproc_pdata rproc_pdata = { + .name ="dsp" , +};Since the driver is only for da850 so the name of the remote processor is fixed and can probably be hardcoded in the driver itself.
In which case I wouldn't even need the da8xx_rproc_pdata at all, which would be nice.
Quoted 102 lines
Quoted 39 lines
arch/arm/mach-davinci/devices-da8xx.c +38
-0+ +static struct platform_device da8xx_dsp = { + .name ="davinci-rproc" , + .id =0 , + .dev = { + .platform_data = &rproc_pdata, + .coherent_dma_mask =DMA_BIT_MASK (32 ), + }, + .num_resources =ARRAY_SIZE (da8xx_rproc_resources), + .resource = da8xx_rproc_resources, +}; + +#if IS_ENABLED (CONFIG_DAVINCI_REMOTEPROC ) + +static phys_addr_t rproc_base__initdata ; +static unsigned long rproc_size__initdata ; + +static int __init early_rproc_mem (char *p) +{ +char *endp; + +if (p ==NULL ) +return 0 ; + + rproc_size =memparse (p, &endp); +if (*endp =='@' ) + rproc_base =memparse (endp +1 ,NULL ); + +return 0 ; +} +early_param ("rproc_mem" , early_rproc_mem); + +void __init da8xx_rproc_reserve_cma (void ) +{ +int ret; + +if (!rproc_base || !rproc_size) { + pr_err("%s: 'rproc_mem=nn at address' badly specified\n"arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct · line 773
Quoted 10 lines
arch/arm/mach-davinci/devices-da8xx.c +10
-0+" 'nn' and 'address' must both be non-zero\n" , + __func__); + +return ; + } + +pr_info ("%s: reserving 0x%lx @ 0x%lx...\n" , + __func__, rproc_size, (unsigned long )rproc_base); + + ret = dma_declare_contiguous(&da8xx_dsp.dev, rproc_size,
rproc_base, 0);arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct · line 775
Quoted 2 lines
arch/arm/mach-davinci/devices-da8xx.c +2
-0+if (ret) + pr_err("%s: dma_declare_contiguous failed %d\n", __func__,ret);
Quoted 7 lines
arch/arm/mach-davinci/devices-da8xx.c +7
-0+} + +#else + +void __init da8xx_rproc_reserve_cma (void ) +{ +}arch/arm/mach-davinci/devices-da8xx.c int __init da850_register_mmcsd1(struct · line 792
Quoted 10 lines
arch/arm/mach-davinci/devices-da8xx.c +10
-0+ +#endif + +int __init da8xx_register_rproc (void ) +{ +int ret; + + ret =platform_device_register (&da8xx_dsp); +if (ret) { + pr_err("%s: platform_device_register: %d\n", __func__,ret);
Review
Quoted 11 lines
arch/arm/mach-davinci/devices-da8xx.c +7
-0+ +return ret; + } + +return 0 ; +}; +static struct resource da8xx_rtc_resources[] = { { .start =DA8XX_RTC_BASE ,(quoted diff) +0
-0
b/arch/arm/mach-davinci/include/mach/da8xx.hQuoted 12 lines
index 700d311..e9295bd 100644
arch/arm/mach-davinci/include/mach/da8xx.h +1
-0@@ -54,6 +54,7 @@ extern unsigned int da850_max_speed;#define DA8XX_SYSCFG0_BASE (IO_PHYS + 0x14000)#define DA8XX_SYSCFG0_VIRT (x) (da8xx_syscfg0_base + (x))#define DA8XX_JTAG_ID_REG 0x18 +#define DA8XX_HOST1CFG_REG 0x44#define DA8XX_CFGCHIP0_REG 0x17c#define DA8XX_CFGCHIP2_REG 0x184#define DA8XX_CFGCHIP3_REG 0x188arch/arm/mach-davinci/include/mach/da8xx.h int __init da850_register_vpif_display · line 109
Quoted 4 lines
arch/arm/mach-davinci/include/mach/da8xx.h +1
-0@@ -105,6 +106,9 @@ int __init da850_register_vpif_displayint __init da850_register_vpif_capture (struct vpif_capture_config *capture_config);void da8xx_restart (char mode,const char *cmd); +void __init da8xx_rproc_reserve_cma (void );No need of
__initqualifier in function prototypes.
Quoted 3 lines
The patch looks good to me otherwise, but I will not apply it right away
and instead wait for the driver to get accepted first.
Sounds like a plan.
Thanks & Regards,
- Rob
Quoted 3 lines
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 8/9 ARM: davinci: da850 board: Added .reserve function and rproc platform registration
Quoted 5 lines · Robert Tivy
Added .reserve function for reserving CMA memory block to
MACHINE_START.Added call to remoteproc platform device registration function during init.
Signed-off-by Robert Tivy <[email protected]>
Looks good to me. I will apply after the driver is accepted.
Thanks,
Sekhar
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v5 9/9 ARM: davinci: da850: Added dsp clock definition
Quoted 3 lines · Robert Tivy
Added dsp clock definition, keyed to "davinci-rproc.0".
Signed-off-by Robert Tivy <[email protected]>
I think this can be done along with 4/9 so I committed it by merging it with 4/9
Thanks,
Sekhar
PATCH v5 9/9 ARM: davinci: da850: Added dsp clock definition
In reply to a message not in the archiveRob,
Quoted 27 lines · Robert Tivy
Added dsp clock definition, keyed to "davinci-rproc.0".
Signed-off-by Robert Tivy <[email protected]>--- arch/arm/mach-davinci/da850.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/mach-davinci/da850.c +8
-0@@ -375,6 +375,14 @@ static struct clk sata_clk = { .flags = PSC_FORCE, }; +static struct clk dsp_clk = { + .name ="dsp" , + .parent = &pll0_sysclk1, + .domain =DAVINCI_GPSC_DSPDOMAIN , + .lpsc =DA8XX_LPSC0_GEM , + .flags =PSC_LRST , +}; +static struct clk_lookup da850_clks[] = {CLK (NULL ,"ref" , &ref_clk),CLK (NULL ,"pll0" , &pll0_clk),
arch/arm/mach-davinci/da850.c static struct clk_lookup da850_clks[] = { · line 432
Quoted 4 lines
arch/arm/mach-davinci/da850.c +1
-0@@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {CLK ("spi_davinci.1" ,NULL , &spi1_clk),CLK ("vpif" ,NULL , &vpif_clk),CLK ("ahci" ,NULL , &sata_clk), + CLK("davinci-rproc.0", NULL, &dsp_clk),
Adding this clock node without the having the driver probed leads to kernel hang while booting. With CONFIG_DAVINCI_RESET_CLOCKS=n, the kernel boot fine. It looks like there is some trouble disabling the clock if it is not used. Can you please check this issue?
Thanks,
Sekhar
PATCH v5 9/9 ARM: davinci: da850: Added dsp clock definition
Quoted 45 lines
-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, January 22, 2013 4:04 AMRob,
arch/arm/mach-
Quoted 8 lines · Robert Tivy
Added dsp clock definition, keyed to "davinci-rproc.0".
Signed-off-by Robert Tivy <[email protected]>--- arch/arm/mach-davinci/da850.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/mach- renamed from arch/arm/mach-davinci/da850.c +0
-0
davinci/da850.cQuoted 19 lines
index 097fcb2..50107c5 100644
arch/arm/mach-davinci/da850.c +8
-0@@ -375,6 +375,14 @@ static struct clk sata_clk = { .flags = PSC_FORCE, }; +static struct clk dsp_clk = { + .name ="dsp" , + .parent = &pll0_sysclk1, + .domain =DAVINCI_GPSC_DSPDOMAIN , + .lpsc =DA8XX_LPSC0_GEM , + .flags =PSC_LRST , +}; +static struct clk_lookup da850_clks[] = {CLK (NULL ,"ref" , &ref_clk),CLK (NULL ,"pll0" , &pll0_clk),arch/arm/mach-davinci/da850.c static struct clk_lookup da850_clks[] = { · line 432
Quoted 4 lines
arch/arm/mach-davinci/da850.c +1
-0@@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {CLK ("spi_davinci.1" ,NULL , &spi1_clk),CLK ("vpif" ,NULL , &vpif_clk),CLK ("ahci" ,NULL , &sata_clk), + CLK("davinci-rproc.0", NULL, &dsp_clk),Adding this clock node without the having the driver probed leads to kernel hang while booting. With
CONFIG_DAVINCI_RESET_CLOCKS=n, the kernel boot fine. It looks like there is some trouble disabling the clock if it is not used. Can you please check this issue?
Quoted 2 lines
Thanks,
Sekhar
Sekhar,
Thankyou for trying that out.
I discovered that the kernel boot hang is caused when trying to disable this clk during init, from within the function davinci_clk_disable_unused(). That function calls davinci_psc_config() which attempts to disable the DSP clock. Since this clk is enabled after reset, disabling it involves a state transition, and therefore the function must wait for GOSTAT[1] to be 0. For most peripherals this happens automatically, but for the DSP (and ARM, too), the DSP must execute the IDLE instruction to allow a clock enable->disable transition. Since this is bootup and there is no real program on the DSP yet, this doesn't happen.
I was able to overcome this by adding PSC_FORCE to the clk->flags for dsp_clk. In fact, without PSC_FORCE I was not able to "rmmod davinci_remoteproc" since the firmware file that I'm loading did not execute IDLE. It feels like for davinci we should have a way to control the clk->flags' PSC_FORCE setting at run-time. The PSC_FORCE would be set initially (during boot) and the user (or system integrator) would have a way to unset it dynamically. That way, when the DSP firmware does actually have a structured way to enter IDLE, the user can say "I'd like a structured shutdown" and unset PSC_FORCE (via a clean API). But for now I'm just going to turn on PSC_FORCE:
.flags = PSC_LRST | PSC_FORCE,
Thanks & Regards,
- Rob
PATCH v5 9/9 ARM: davinci: da850: Added dsp clock definition
Quoted 48 lines · Robert
Quoted 45 lines
-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, January 22, 2013 4:04 AMRob,
arch/arm/mach-
Quoted 8 lines · Robert Tivy
Added dsp clock definition, keyed to "davinci-rproc.0".
Signed-off-by Robert Tivy <[email protected]>--- arch/arm/mach-davinci/da850.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/mach- renamed from arch/arm/mach-davinci/da850.c +0
-0
davinci/da850.cQuoted 19 lines
index 097fcb2..50107c5 100644
arch/arm/mach-davinci/da850.c +8
-0@@ -375,6 +375,14 @@ static struct clk sata_clk = { .flags = PSC_FORCE, }; +static struct clk dsp_clk = { + .name ="dsp" , + .parent = &pll0_sysclk1, + .domain =DAVINCI_GPSC_DSPDOMAIN , + .lpsc =DA8XX_LPSC0_GEM , + .flags =PSC_LRST , +}; +static struct clk_lookup da850_clks[] = {CLK (NULL ,"ref" , &ref_clk),CLK (NULL ,"pll0" , &pll0_clk),arch/arm/mach-davinci/da850.c static struct clk_lookup da850_clks[] = { · line 432
Quoted 4 lines
arch/arm/mach-davinci/da850.c +1
-0@@ -421,6 +429,7 @@ static struct clk_lookup da850_clks[] = {CLK ("spi_davinci.1" ,NULL , &spi1_clk),CLK ("vpif" ,NULL , &vpif_clk),CLK ("ahci" ,NULL , &sata_clk), + CLK("davinci-rproc.0", NULL, &dsp_clk),Adding this clock node without the having the driver probed leads to kernel hang while booting. With
CONFIG_DAVINCI_RESET_CLOCKS=n, the kernel boot fine. It looks like there is some trouble disabling the clock if it is not used. Can you please check this issue?Quoted 2 lines
Thanks,
Sekhar
Quoted 8 lines
Sekhar,
Thankyou for trying that out.
I discovered that the kernel boot hang is caused when trying to disable this clk during init, from within the function
davinci_clk_disable_unused(). That function callsdavinci_psc_config()which attempts to disable the DSP clock. Since this clk is enabled after reset, disabling it involves a state transition, and therefore the function must wait for GOSTAT[1] to be 0. For most peripherals this happens automatically, but for the DSP (and ARM, too), the DSP must execute the IDLE instruction to allow a clockenable->disabletransition. Since this is bootup and there is no real program on the DSP yet, this doesn't happen.I was able to overcome this by adding
PSC_FORCEto theclk->flagsfordsp_clk. In fact, withoutPSC_FORCEI was not able to "rmmoddavinci_remoteproc" since the firmware file that I'm loading did not execute IDLE. It feels like for davinci we should have a way to control theclk->flags'PSC_FORCEsetting at run-time. ThePSC_FORCEwould be set initially (during boot) and the user (or system integrator) would have a way to unset it dynamically. That way, when the DSP firmware does actually have a structured way to enter IDLE, the user can say "I'd like a structured shutdown" and unsetPSC_FORCE(via a clean API). But for now I'm just going to turn onPSC_FORCE:.flags = PSC_LRST | PSC_FORCE,
Thanks for the debug. It works for me after I added PSC_FORCE. Here is the patch I am finally committing.
Thanks,
Sekhar
commit 09810a853b9a7920ba8c250d18815ef236effc47
Author: Robert Tivy <[email protected]> AuthorDate: Thu Jan 10 16:23:22 2013 -0800 Commit: Sekhar Nori <[email protected]> CommitDate: Thu Jan 24 10:54:08 2013 +0530
ARM: davinci: da850: add dsp clock definition
Added dsp clock definition, keyed to "davinci-rproc.0". DSP clocks is derived from pll0 sysclk1. Add a clock tree node for that too.
[nsekhar at ti.com: merge addition of pll0 sysclk1 and dsp clock into one commit. Add PSC_FORCE to dsp clock node to handle the case where DSP does not go into IDLE and its clock needs to be disabled.]
arch/arm/mach-davinci/da850.c +17 -0
@@ -76,6 +76,13 @@ static struct clk pll0_aux_clk = { .flags = CLK_PLL | PRE_PLL, }; +static struct clk pll0_sysclk1 = { + .name ="pll0_sysclk1" , + .parent = &pll0_clk, + .flags =CLK_PLL , + .div_reg =PLLDIV1 , +}; +static struct clk pll0_sysclk2 = { .name ="pll0_sysclk2" , .parent = &pll0_clk,
@@ -368,10 +375,19 @@ static struct clk sata_clk = { .flags = PSC_FORCE, }; +static struct clk dsp_clk = { + .name ="dsp" , + .parent = &pll0_sysclk1, + .domain =DAVINCI_GPSC_DSPDOMAIN , + .lpsc =DA8XX_LPSC0_GEM , + .flags =PSC_LRST |PSC_FORCE , +}; +static struct clk_lookup da850_clks[] = {CLK (NULL ,"ref" , &ref_clk),CLK (NULL ,"pll0" , &pll0_clk),CLK (NULL ,"pll0_aux" , &pll0_aux_clk), +CLK (NULL ,"pll0_sysclk1" , &pll0_sysclk1),CLK (NULL ,"pll0_sysclk2" , &pll0_sysclk2),CLK (NULL ,"pll0_sysclk3" , &pll0_sysclk3),CLK (NULL ,"pll0_sysclk4" , &pll0_sysclk4),
@@ -413,6 +429,7 @@ static struct clk_lookup da850_clks[] = {CLK ("spi_davinci.1" ,NULL , &spi1_clk),CLK ("vpif" ,NULL , &vpif_clk),CLK ("ahci" ,NULL , &sata_clk), +CLK ("davinci-rproc.0" ,NULL , &dsp_clk), CLK(NULL, NULL, NULL), };