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: [email protected]; linux-arm- > [email protected]; 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 <[email protected]> > > --- > > 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 _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
