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? > > 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. Also, platform data needs be added along with the driver. And the driver itself needs to be added before its platform users. > 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. > + > 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? > +}; > + > +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. 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. > + > +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). 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
