[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP
Tivy, Robert
rtivy at ti.com
Wed Nov 28 20:38:44 EST 2012
Hi Sekhar,
Please see comments and noob questions below...
> -----Original Message-----
> From: Nori, Sekhar
> Sent: Tuesday, November 20, 2012 4:27 AM
> To: Tivy, Robert
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
> OMAP-L138 DSP
>
> On 11/14/2012 6:03 AM, Robert Tivy wrote:
> > Requires CMA services.
> >
> > New 'clk_reset()' API added so that the DSP's reset state can be
> > toggled separately from its enable/disable operation.
>
> But we cannot add a private clk_ API without it being defined in
> include/linux/clk.h. So, this requires wider alignment.
>
> And I am not sure clock API should be extended to support reset since
> "resetting a clock" does not make a lot of sense. On DaVinci, clock
> gating and reset are handled by the same module, but that need not
> always be the case.
>
> What you need is a way to reset an IP. I do not know of an existing
> framework to do this; likely a new API needs to be created. I am
> guessing this never existed so far because most of the IPs can be reset
> internally (by writing a bit within its own register space). I guess
> DSP is different since its actually a co-processor and may not have
> such a bit.
>
> How about simulating a reset by making the DSP jump to its reset
> address. While I am sure its not quite the same as resetting the DSP
> using PSC, may be it could be used while you wait for consensus around
> handling module reset in kernel?
Since the ARM can't write the DSP's program counter I suspect such a temporary solution is not possible.
>
> >
> > Signed-off-by: Robert Tivy <rtivy at ti.com>
> > ---
> > arch/arm/mach-davinci/board-da850-evm.c | 10 +++
> > arch/arm/mach-davinci/board-omapl138-hawk.c | 10 +++
> > arch/arm/mach-davinci/clock.c | 22 +++++++
> > arch/arm/mach-davinci/clock.h | 1 +
> > arch/arm/mach-davinci/da850.c | 17 ++++++
> > arch/arm/mach-davinci/devices-da8xx.c | 78
> +++++++++++++++++++++++-
> > arch/arm/mach-davinci/include/mach/da8xx.h | 1 +
> > arch/arm/mach-davinci/include/mach/psc.h | 3 +
> > arch/arm/mach-davinci/psc.c | 27 ++++++++
> > arch/arm/mach-davinci/remoteproc.h | 23 +++++++
> > include/linux/clk.h | 17 ++++++
> > include/linux/platform_data/da8xx-remoteproc.h | 34 +++++++++++
>
> This patch is touching too many areas at once and needs to be split
> according to whether the changes are in the soc support or board
> support.
OK.
> Also, platform data needs be added along with the driver. And
> the driver itself needs to be added before its platform users.
Are these comments suggesting some change to the code, or are they more of a guide as to how to split this part of the patch up into related chunks? Please clarify.
>
> > 12 files changed, 242 insertions(+), 1 deletion(-) create mode
> > 100644 arch/arm/mach-davinci/remoteproc.h
> > create mode 100644 include/linux/platform_data/da8xx-remoteproc.h
> >
> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c
> > b/arch/arm/mach-davinci/board-da850-evm.c
> > index 6c172b3..4e86008 100644
> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> > @@ -48,6 +48,8 @@
> > #include <media/tvp514x.h>
> > #include <media/adv7343.h>
> >
> > +#include "remoteproc.h"
> > +
> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> > #define DA850_LCD_BL_PIN GPIO_TO_PIN(2, 15)
> > @@ -1550,6 +1552,11 @@ static __init void da850_evm_init(void)
> > pr_warn("%s: sata registration failed: %d\n", __func__,
> ret);
> >
> > da850_evm_setup_mac_addr();
> > +
> > + ret = da8xx_register_rproc();
> > + if (ret)
> > + pr_warn("%s: dsp/rproc registration failed: %d\n",
> > + __func__, ret);
> > }
> >
> > #ifdef CONFIG_SERIAL_8250_CONSOLE
> > @@ -1577,4 +1584,7 @@ MACHINE_START(DAVINCI_DA850_EVM, "DaVinci
> DA850/OMAP-L138/AM18x EVM")
> > .init_late = davinci_init_late,
> > .dma_zone_size = SZ_128M,
> > .restart = da8xx_restart,
> > +#ifdef CONFIG_CMA
> > + .reserve = da8xx_rproc_reserve_cma,
> > +#endif
> > MACHINE_END
> > diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
> > b/arch/arm/mach-davinci/board-omapl138-hawk.c
> > index 8aea169..a0b81c1 100644
> > --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> > +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> > @@ -21,6 +21,8 @@
> > #include <mach/da8xx.h>
> > #include <mach/mux.h>
> >
> > +#include "remoteproc.h"
> > +
> > #define HAWKBOARD_PHY_ID "davinci_mdio-0:07"
> > #define DA850_HAWK_MMCSD_CD_PIN GPIO_TO_PIN(3, 12)
> > #define DA850_HAWK_MMCSD_WP_PIN GPIO_TO_PIN(3, 13)
> > @@ -311,6 +313,11 @@ static __init void omapl138_hawk_init(void)
> > if (ret)
> > pr_warn("%s: watchdog registration failed: %d\n",
> > __func__, ret);
> > +
> > + ret = da8xx_register_rproc();
> > + if (ret)
> > + pr_warn("%s: dsp/rproc registration failed: %d\n",
> > + __func__, ret);
> > }
> >
> > #ifdef CONFIG_SERIAL_8250_CONSOLE
> > @@ -338,4 +345,7 @@ MACHINE_START(OMAPL138_HAWKBOARD, "AM18x/OMAP-
> L138 Hawkboard")
> > .init_late = davinci_init_late,
> > .dma_zone_size = SZ_128M,
> > .restart = da8xx_restart,
> > +#ifdef CONFIG_CMA
> > + .reserve = da8xx_rproc_reserve_cma,
> > +#endif
> > MACHINE_END
> > diff --git a/arch/arm/mach-davinci/clock.c
> > b/arch/arm/mach-davinci/clock.c index 34668ea..3fad759 100644
> > --- a/arch/arm/mach-davinci/clock.c
> > +++ b/arch/arm/mach-davinci/clock.c
> > @@ -31,6 +31,13 @@ static LIST_HEAD(clocks); static
> > DEFINE_MUTEX(clocks_mutex); static DEFINE_SPINLOCK(clockfw_lock);
> >
> > +static void __clk_reset(struct clk *clk, bool reset) {
> > + if (clk->flags & CLK_PSC)
> > + davinci_psc_reset_config(clk->domain, clk->gpsc, clk->lpsc,
> > + reset, clk->flags);
> > +}
> > +
> > static void __clk_enable(struct clk *clk) {
> > if (clk->parent)
> > @@ -52,6 +59,21 @@ static void __clk_disable(struct clk *clk)
> > __clk_disable(clk->parent);
> > }
> >
> > +int clk_reset(struct clk *clk, bool reset) {
> > + unsigned long flags;
> > +
> > + if (clk == NULL || IS_ERR(clk))
> > + return -EINVAL;
> > +
> > + spin_lock_irqsave(&clockfw_lock, flags);
> > + __clk_reset(clk, reset);
> > + spin_unlock_irqrestore(&clockfw_lock, flags);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(clk_reset);
> > +
> > int clk_enable(struct clk *clk)
> > {
> > unsigned long flags;
> > diff --git a/arch/arm/mach-davinci/clock.h
> > b/arch/arm/mach-davinci/clock.h index 46f0f1b..89aa95e 100644
> > --- a/arch/arm/mach-davinci/clock.h
> > +++ b/arch/arm/mach-davinci/clock.h
> > @@ -112,6 +112,7 @@ struct clk {
> > #define PRE_PLL BIT(4) /* source is before PLL
> mult/div */
> > #define PSC_SWRSTDISABLE BIT(5) /* Disable state is SwRstDisable
> */
> > #define PSC_FORCE BIT(6) /* Force module state transtition
> */
> > +#define PSC_LRST BIT(8) /* Use local reset on
> enable/disable */
> >
> > #define CLK(dev, con, ck) \
> > { \
> > diff --git a/arch/arm/mach-davinci/da850.c
> > b/arch/arm/mach-davinci/da850.c index b90c172..4008fdc 100644
> > --- a/arch/arm/mach-davinci/da850.c
> > +++ b/arch/arm/mach-davinci/da850.c
> > @@ -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,
> > +};
>
> Adding support for PLL0 sysclk1 can be separated out and can be taken
> ahead of other changes.
OK, will do.
>
> > +
> > static struct clk pll0_sysclk2 = {
> > .name = "pll0_sysclk2",
> > .parent = &pll0_clk,
> > @@ -362,10 +369,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,
> > +};
> > +
> > 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),
> > @@ -406,6 +422,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),
> > };
> >
> > diff --git a/arch/arm/mach-davinci/devices-da8xx.c
> > b/arch/arm/mach-davinci/devices-da8xx.c
> > index 466b70c..ae54769 100644
> > --- a/arch/arm/mach-davinci/devices-da8xx.c
> > +++ b/arch/arm/mach-davinci/devices-da8xx.c
> > @@ -12,10 +12,13 @@
> > */
> > #include <linux/init.h>
> > #include <linux/platform_device.h>
> > -#include <linux/dma-mapping.h>
> > +#ifdef CONFIG_CMA
> > +#include <linux/dma-contiguous.h>
> > +#endif
> > #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>
> > @@ -655,6 +658,79 @@ int __init da850_register_mmcsd1(struct
> > davinci_mmc_config *config) } #endif
> >
> > +static struct resource da8xx_rproc_resources[] = {
> > + { /* HOST1CFG syscfg offset - DSP boot address */
> > + .start = 0x44,
> > + .end = 0x44,
>
> These should be absolute addresses, not relative.
>
> > + .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",
> > + .firmware = "da8xx-dsp.xe674",
>
> Sounds like the user can name the firmware whatever he wants and so it
> should be a module parameter to the remote proc driver? There is
> nothing platform specific about the firmware name, no?
Sounds OK. I propose then to have the above be the default firmware name, along with a module parameter that will override if specified.
>
> > +};
> > +
> > +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,
> > +};
> > +
> > +#ifdef CONFIG_CMA
> > +
> > +static phys_addr_t rproc_base __initdata =
> > +CONFIG_DAVINCI_DSP_RPROC_CMA_BASE;
> > +static unsigned long rproc_size __initdata =
> > +CONFIG_DAVINCI_DSP_RPROC_CMA_SIZE;
>
> These are not defined at this point so the kernel build will fail after
> applying this patch. This breaks things for those who run git-bisect.
> Please verify kernel build after applying each patch.
>
> Looking at patch 6/6 where these are actually defined, it is not
> correct to define these using Kconfig. This prevents users from running
> a single kernel image on systems where the value needs to be different.
They can run a single image if the image supports overriding the Kconfig settings with kernel command-line arguments.
The most basic solution is constants in the .c file itself. A better solution is Kconfig settings used by the .c file. An even better solution is Kconfig settings with kernel command-line overrides.
> If you want the remoteproc driver to allocate a certain area of memory
> to the dsp, why don't you take that value as a module parameter so
> people who need different values can pass them differently? It is not
> clear to me why this memory management needs to be dealt with in
> platform code.
Unfortunetly we need to reserve this dsp memory during early boot, using CMA. Runtime module insertion is too late.
>
> > +
> > +static int __init early_rproc_mem(char *p) {
> > + char *endp;
> > +
> > + rproc_size = memparse(p, &endp);
> > + if (*endp == '@')
> > + rproc_base = memparse(endp + 1, NULL);
> > +
> > + return 0;
> > +}
> > +early_param("rproc_mem", early_rproc_mem);
>
> Use of non-standard kernel parameters is discouraged. All kernel
> parameters should be documented in Documentation/kernel-parameters.txt
> (this means each and every kernel parameter needs to be justified
> well).
Can I use the kernel command-line (i.e., u-boot bootargs variable) to specify non-kernel parameters? I guess my question is more one about the terminology "kernel parameter" - is there a way to specify a module parameter on the kernel command line (like, perhaps, rproc.membase and rproc.memsize)?
Regards,
- Rob
>
> I have not reviewed the rest of the code here. Lets get some of these
> fundamental issues resolved first.
>
> Thanks,
> Sekhar
More information about the linux-arm-kernel
mailing list