| Message ID | 1352853207-20602-5-git-send-email-rtivy@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <rtivy@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. 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
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@linux.davincidsp.com; linux-arm- > kernel@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@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
Hi Rob, On 11/29/2012 7:08 AM, Tivy, Robert wrote: > 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@linux.davincidsp.com; linux-arm- >> kernel@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. Okay. I think the way forward on this is to start a separate thread including the ARM list, LKML and the remoteproc folks to see if it makes sense to create a reset API in kernel. You can describe the usecase you have. > >> >>> >>> Signed-off-by: Robert Tivy <rtivy@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. Its about how the patches should be split and structured. [...] >>> +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. Sounds good. >>> +#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 have kernel command line, you could default to some values which are sane in most cases if they are not provided. No, need to have a Kconfig as well. That will be too many hooks to control the same things and probably not necessary. > >> 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. Then I guess most of the time the module would be built into the kernel and will be initialized using an early enough initcall. > >> >>> + >>> +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)? Right. Module parameters are passed from kernel command line when the module is built into the kernel. Thanks, Sekhar
Hi Sekhar, > -----Original Message----- > From: Nori, Sekhar > Sent: Friday, November 30, 2012 2:51 AM > To: Tivy, Robert > Cc: davinci-linux-open-source@linux.davincidsp.com; linux-arm- > kernel@lists.infradead.org; Ring, Chris; Grosen, Mark > Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for > OMAP-L138 DSP > > Hi Rob, > > On 11/29/2012 7:08 AM, Tivy, Robert wrote: > > 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@linux.davincidsp.com; linux-arm- > >> kernel@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. > > Okay. I think the way forward on this is to start a separate thread > including the ARM list, LKML and the remoteproc folks to see if it > makes sense to create a reset API in kernel. You can describe the > usecase you have. Instead of fighting that fight, I thought of another way. The da8xx_dsp platform_device has private data registered in its device struct. This private data can contain a function pointer for a "DSP reset" function, and davinci_remoteproc.c can call the registered function. Does that sound OK? Regards, - Rob
Hi Rob, On 12/1/2012 7:41 AM, Tivy, Robert wrote: > Hi Sekhar, > >> -----Original Message----- >> From: Nori, Sekhar >> Sent: Friday, November 30, 2012 2:51 AM >> To: Tivy, Robert >> Cc: davinci-linux-open-source@linux.davincidsp.com; linux-arm- >> kernel@lists.infradead.org; Ring, Chris; Grosen, Mark >> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for >> OMAP-L138 DSP >> >> Hi Rob, >> >> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>> 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@linux.davincidsp.com; linux-arm- >>>> kernel@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. >> >> Okay. I think the way forward on this is to start a separate thread >> including the ARM list, LKML and the remoteproc folks to see if it >> makes sense to create a reset API in kernel. You can describe the >> usecase you have. > > Instead of fighting that fight, I thought of another way. The da8xx_dsp platform_device has private data registered in its device struct. This private data can contain a function pointer for a "DSP reset" function, and davinci_remoteproc.c can call the registered function. Does that sound OK? Passing function pointers from platform code will not work when converting to device tree (DT). DA850 will gain DT boot with v3.8 and there is work ongoing on converting drivers to be DT-aware. Adding a new driver which is DT-incompatible will not be right. Hence, I will not recommend this now. Thanks, Sekhar
On 12/03/2012 09:41 AM, Sekhar Nori wrote: > Hi Rob, > > On 12/1/2012 7:41 AM, Tivy, Robert wrote: >> Hi Sekhar, >> >>> -----Original Message----- >>> From: Nori, Sekhar >>> Sent: Friday, November 30, 2012 2:51 AM >>> To: Tivy, Robert >>> Cc: davinci-linux-open-source@linux.davincidsp.com; linux-arm- >>> kernel@lists.infradead.org; Ring, Chris; Grosen, Mark >>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for >>> OMAP-L138 DSP >>> >>> Hi Rob, >>> >>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>> 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@linux.davincidsp.com; linux-arm- >>>>> kernel@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. >>> >>> Okay. I think the way forward on this is to start a separate thread >>> including the ARM list, LKML and the remoteproc folks to see if it >>> makes sense to create a reset API in kernel. You can describe the >>> usecase you have. >> >> Instead of fighting that fight, I thought of another way. The da8xx_dsp platform_device has private data registered in its device struct. This private data can contain a function pointer for a "DSP reset" function, and davinci_remoteproc.c can call the registered function. Does that sound OK? > > Passing function pointers from platform code will not work when > converting to device tree (DT). DA850 will gain DT boot with v3.8 and > there is work ongoing on converting drivers to be DT-aware. Adding a new > driver which is DT-incompatible will not be right. Hence, I will not > recommend this now. > Not sure if this solves your problem, but you could add a new clock type (PSC_LRESET?) as a child under the PSC clock node for the DSP. Something like: | +-- PSC x (DSP0 clock) | | | +-- PSC-LRESET x (DSP0 reset control) | +-- PSC y (DSP1 clock) | | | +-- PSC-LRESET y (DSP1 reset control) | +-- PSC z (DSP2 clock) | | | +-- PSC-LRESET z (DSP2 reset control) The idea here being that if you enable the PSC clocks, you enable the clock gates, but leave the DSPs in reset. On the other hand, if you enable the reset clock, the code implicitly enables the gate (via parent), and takes the DSP out of reset as well. This is not quite the cleanest way to do it, considering that reset lines have no business in the clock tree, but then, this is probably the simplest way to fit in. Thoughts? BTW, we have the same situation on Keystone. Thanks -- Cyril.
> -----Original Message----- > From: Chemparathy, Cyril > Sent: Monday, December 03, 2012 12:13 PM > To: Nori, Sekhar > Cc: Tivy, Robert; davinci-linux-open-source@linux.davincidsp.com; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for > OMAP-L138 DSP > > On 12/03/2012 09:41 AM, Sekhar Nori wrote: > > Hi Rob, > > > > On 12/1/2012 7:41 AM, Tivy, Robert wrote: > >> Hi Sekhar, > >> > >>> -----Original Message----- > >>> From: Nori, Sekhar > >>> Sent: Friday, November 30, 2012 2:51 AM > >>> To: Tivy, Robert > >>> Cc: davinci-linux-open-source@linux.davincidsp.com; linux-arm- > >>> kernel@lists.infradead.org; Ring, Chris; Grosen, Mark > >>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support > >>> for > >>> OMAP-L138 DSP > >>> > >>> Hi Rob, > >>> > >>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: > >>>> 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@linux.davincidsp.com; linux-arm- > >>>>> kernel@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. > >>> > >>> Okay. I think the way forward on this is to start a separate thread > >>> including the ARM list, LKML and the remoteproc folks to see if it > >>> makes sense to create a reset API in kernel. You can describe the > >>> usecase you have. > >> > >> Instead of fighting that fight, I thought of another way. The > da8xx_dsp platform_device has private data registered in its device > struct. This private data can contain a function pointer for a "DSP > reset" function, and davinci_remoteproc.c can call the registered > function. Does that sound OK? > > > > Passing function pointers from platform code will not work when > > converting to device tree (DT). DA850 will gain DT boot with v3.8 and > > there is work ongoing on converting drivers to be DT-aware. Adding a > > new driver which is DT-incompatible will not be right. Hence, I will > > not recommend this now. > > > > Not sure if this solves your problem, but you could add a new clock > type > (PSC_LRESET?) as a child under the PSC clock node for the DSP. > Something like: > > | > +-- PSC x (DSP0 clock) > | | > | +-- PSC-LRESET x (DSP0 reset control) > | > +-- PSC y (DSP1 clock) > | | > | +-- PSC-LRESET y (DSP1 reset control) > | > +-- PSC z (DSP2 clock) > | | > | +-- PSC-LRESET z (DSP2 reset control) > > > The idea here being that if you enable the PSC clocks, you enable the > clock gates, but leave the DSPs in reset. On the other hand, if you > enable the reset clock, the code implicitly enables the gate (via > parent), and takes the DSP out of reset as well. > > This is not quite the cleanest way to do it, considering that reset > lines have no business in the clock tree, but then, this is probably > the simplest way to fit in. Thoughts? > > BTW, we have the same situation on Keystone. > > Thanks > -- Cyril. Cyril, Thankyou for the good suggestion. In trying it out to see if it would work I ran into an issue where the clk_register() function failed when called for the new child clock, complaining that its parent has no rate. This is true, since the parent was previously just a leaf and handled by the PSC (and the parent's parent is a CLK_PLL clock). So, I would need to add code to arch/arm/mach-davinci/clock.c that prevents rate operations from being performed on a clock of this PSC_LRESET type (as well as code that allows for no "rate" or "parent->rate" to be associated with it, instead of failing). I can modify clock.c to allow for this new clock type in this way, unless you have a better suggestion (I'm not too thrilled about changing clock.c, seems I should be able to live within its framework). Regards, - Rob
On 12/4/2012 1:43 AM, Cyril Chemparathy wrote: > On 12/03/2012 09:41 AM, Sekhar Nori wrote: >> On 12/1/2012 7:41 AM, Tivy, Robert wrote: >> Passing function pointers from platform code will not work when >> converting to device tree (DT). DA850 will gain DT boot with v3.8 and >> there is work ongoing on converting drivers to be DT-aware. Adding a new >> driver which is DT-incompatible will not be right. Hence, I will not >> recommend this now. > Not sure if this solves your problem, but you could add a new clock type > (PSC_LRESET?) as a child under the PSC clock node for the DSP. Something > like: > > | > +-- PSC x (DSP0 clock) > | | > | +-- PSC-LRESET x (DSP0 reset control) > | > +-- PSC y (DSP1 clock) > | | > | +-- PSC-LRESET y (DSP1 reset control) > | > +-- PSC z (DSP2 clock) > | | > | +-- PSC-LRESET z (DSP2 reset control) > > > The idea here being that if you enable the PSC clocks, you enable the > clock gates, but leave the DSPs in reset. On the other hand, if you > enable the reset clock, the code implicitly enables the gate (via > parent), and takes the DSP out of reset as well. > > This is not quite the cleanest way to do it, considering that reset > lines have no business in the clock tree, but then, this is probably the > simplest way to fit in. Thoughts? This may work (on a technical level), but I am not really a fan of this since this is essentially a hack, as you (almost) point out. It does not model the hardware clock tree correctly and we are still struck with using clock APIs for reset control. I still think we need to find a better method of dealing with IP reset. May be an acceptable method already exists. Some one needs to summarize the problem statement well enough and I am sure finding the right solution will not be too long drawn. Thanks, Sekhar
On 12/04/2012 12:58 AM, Sekhar Nori wrote: > On 12/4/2012 1:43 AM, Cyril Chemparathy wrote: > >> On 12/03/2012 09:41 AM, Sekhar Nori wrote: >>> On 12/1/2012 7:41 AM, Tivy, Robert wrote: >>> Passing function pointers from platform code will not work when >>> converting to device tree (DT). DA850 will gain DT boot with v3.8 and >>> there is work ongoing on converting drivers to be DT-aware. Adding a new >>> driver which is DT-incompatible will not be right. Hence, I will not >>> recommend this now. >> Not sure if this solves your problem, but you could add a new clock type >> (PSC_LRESET?) as a child under the PSC clock node for the DSP. Something >> like: >> >> | >> +-- PSC x (DSP0 clock) >> | | >> | +-- PSC-LRESET x (DSP0 reset control) >> | >> +-- PSC y (DSP1 clock) >> | | >> | +-- PSC-LRESET y (DSP1 reset control) >> | >> +-- PSC z (DSP2 clock) >> | | >> | +-- PSC-LRESET z (DSP2 reset control) >> >> >> The idea here being that if you enable the PSC clocks, you enable the >> clock gates, but leave the DSPs in reset. On the other hand, if you >> enable the reset clock, the code implicitly enables the gate (via >> parent), and takes the DSP out of reset as well. >> >> This is not quite the cleanest way to do it, considering that reset >> lines have no business in the clock tree, but then, this is probably the >> simplest way to fit in. Thoughts? > This may work (on a technical level), but I am not really a fan of this > since this is essentially a hack, as you (almost) point out. It does not > model the hardware clock tree correctly and we are still struck with > using clock APIs for reset control. > > I still think we need to find a better method of dealing with IP reset. > May be an acceptable method already exists. Some one needs to summarize > the problem statement well enough and I am sure finding the right > solution will not be too long drawn. > > Thanks, > Sekhar > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source > > I saw some proposal in the mailing list for doing exactly the same thing. The proposal suggest adding reset framework with providers and users. Providers implements the reset handle for a specific ip module and users uses it to reset the IP. I think this is a common problem that requires a solution than a hack. I will see if I can locate the email and forward it if I can find it. Murali
On 12/04/2012 12:58 AM, Sekhar Nori wrote: > On 12/4/2012 1:43 AM, Cyril Chemparathy wrote: > >> On 12/03/2012 09:41 AM, Sekhar Nori wrote: >>> On 12/1/2012 7:41 AM, Tivy, Robert wrote: >>> Passing function pointers from platform code will not work when >>> converting to device tree (DT). DA850 will gain DT boot with v3.8 and >>> there is work ongoing on converting drivers to be DT-aware. Adding a new >>> driver which is DT-incompatible will not be right. Hence, I will not >>> recommend this now. >> Not sure if this solves your problem, but you could add a new clock type >> (PSC_LRESET?) as a child under the PSC clock node for the DSP. Something >> like: >> >> | >> +-- PSC x (DSP0 clock) >> | | >> | +-- PSC-LRESET x (DSP0 reset control) >> | >> +-- PSC y (DSP1 clock) >> | | >> | +-- PSC-LRESET y (DSP1 reset control) >> | >> +-- PSC z (DSP2 clock) >> | | >> | +-- PSC-LRESET z (DSP2 reset control) >> >> >> The idea here being that if you enable the PSC clocks, you enable the >> clock gates, but leave the DSPs in reset. On the other hand, if you >> enable the reset clock, the code implicitly enables the gate (via >> parent), and takes the DSP out of reset as well. >> >> This is not quite the cleanest way to do it, considering that reset >> lines have no business in the clock tree, but then, this is probably the >> simplest way to fit in. Thoughts? > This may work (on a technical level), but I am not really a fan of this > since this is essentially a hack, as you (almost) point out. It does not > model the hardware clock tree correctly and we are still struck with > using clock APIs for reset control. > > I still think we need to find a better method of dealing with IP reset. > May be an acceptable method already exists. Some one needs to summarize > the problem statement well enough and I am sure finding the right > solution will not be too long drawn. > > Thanks, > Sekhar > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source > > I believe this is addressing the same issue. This is a DT based solution, which I believe should add a framework and enhance it to support DT. Murali
On 12/04/2012 09:53 AM, Murali Karicheri wrote: > On 12/04/2012 12:58 AM, Sekhar Nori wrote: >> On 12/4/2012 1:43 AM, Cyril Chemparathy wrote: >> >>> On 12/03/2012 09:41 AM, Sekhar Nori wrote: >>>> On 12/1/2012 7:41 AM, Tivy, Robert wrote: >>>> Passing function pointers from platform code will not work when >>>> converting to device tree (DT). DA850 will gain DT boot with v3.8 and >>>> there is work ongoing on converting drivers to be DT-aware. Adding >>>> a new >>>> driver which is DT-incompatible will not be right. Hence, I will not >>>> recommend this now. >>> Not sure if this solves your problem, but you could add a new clock >>> type >>> (PSC_LRESET?) as a child under the PSC clock node for the DSP. >>> Something >>> like: >>> >>> | >>> +-- PSC x (DSP0 clock) >>> | | >>> | +-- PSC-LRESET x (DSP0 reset control) >>> | >>> +-- PSC y (DSP1 clock) >>> | | >>> | +-- PSC-LRESET y (DSP1 reset control) >>> | >>> +-- PSC z (DSP2 clock) >>> | | >>> | +-- PSC-LRESET z (DSP2 reset control) >>> >>> >>> The idea here being that if you enable the PSC clocks, you enable the >>> clock gates, but leave the DSPs in reset. On the other hand, if you >>> enable the reset clock, the code implicitly enables the gate (via >>> parent), and takes the DSP out of reset as well. >>> >>> This is not quite the cleanest way to do it, considering that reset >>> lines have no business in the clock tree, but then, this is probably >>> the >>> simplest way to fit in. Thoughts? >> This may work (on a technical level), but I am not really a fan of this >> since this is essentially a hack, as you (almost) point out. It does not >> model the hardware clock tree correctly and we are still struck with >> using clock APIs for reset control. >> >> I still think we need to find a better method of dealing with IP reset. >> May be an acceptable method already exists. Some one needs to summarize >> the problem statement well enough and I am sure finding the right >> solution will not be too long drawn. >> >> Thanks, >> Sekhar >> _______________________________________________ >> Davinci-linux-open-source mailing list >> Davinci-linux-open-source@linux.davincidsp.com >> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source >> >> > I believe this is addressing the same issue. This is a DT based > solution, which I believe should add a framework and enhance it to > support DT. Forgot to paste the link. Here we go https://patchwork.kernel.org/patch/1635051/ > > Murali > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
> -----Original Message----- > From: davinci-linux-open-source-bounces@linux.davincidsp.com > [mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On > Behalf Of Karicheri, Muralidharan > Sent: Tuesday, December 04, 2012 8:13 AM > To: davinci-linux-open-source@linux.davincidsp.com > Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for > OMAP-L138 DSP > > On 12/04/2012 09:53 AM, Murali Karicheri wrote: > >> > >> > > I believe this is addressing the same issue. This is a DT based > > solution, which I believe should add a framework and enhance it to > > support DT. > Forgot to paste the link. Here we go > > https://patchwork.kernel.org/patch/1635051/ > Thanks Murali, very interesting. The thread in your link above mentions omap_hwmod at the end, and I've been looking at how that's handled but it's quite complex. Would a similar solution (davinci_hwmod) be a good approach? Regards, - Rob
adding back the ARM list. On 12/5/2012 7:53 AM, Tivy, Robert wrote: >> -----Original Message----- >> From: davinci-linux-open-source-bounces@linux.davincidsp.com >> [mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On >> Behalf Of Karicheri, Muralidharan >> Sent: Tuesday, December 04, 2012 8:13 AM >> To: davinci-linux-open-source@linux.davincidsp.com >> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for >> OMAP-L138 DSP >> >> On 12/04/2012 09:53 AM, Murali Karicheri wrote: >>>> >>>> >>> I believe this is addressing the same issue. This is a DT based >>> solution, which I believe should add a framework and enhance it to >>> support DT. >> Forgot to paste the link. Here we go >> >> https://patchwork.kernel.org/patch/1635051/ >> > > Thanks Murali, very interesting. > > The thread in your link above mentions omap_hwmod at the end, and I've been looking at how that's handled but it's quite complex. Would a similar solution (davinci_hwmod) be a good approach? Um, no. I don't think the thread is proposing hwmod kind of implementation for each machine that implements the (to be proposed) reset API. Mike is just asking to have a look at omap hwmod to see how it handles device resets for an inspiration on how the proposed API can work. The final API will generic with machine specific implementation for each machine that implements it. OMAP created HWMOD to handle the complexities of that chip, there is no need to replicate that in DaVinci. The link is definitely interesting and exactly what we were looking for. It will be better to continue the discussion on that thread there so all interested parties are on a single thread. Thanks, Sekhar
On 12/04/2012 09:23 PM, Tivy, Robert wrote: >> -----Original Message----- >> From: davinci-linux-open-source-bounces@linux.davincidsp.com >> [mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On >> Behalf Of Karicheri, Muralidharan >> Sent: Tuesday, December 04, 2012 8:13 AM >> To: davinci-linux-open-source@linux.davincidsp.com >> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for >> OMAP-L138 DSP >> >> On 12/04/2012 09:53 AM, Murali Karicheri wrote: >>>> >>> I believe this is addressing the same issue. This is a DT based >>> solution, which I believe should add a framework and enhance it to >>> support DT. >> Forgot to paste the link. Here we go >> >> https://patchwork.kernel.org/patch/1635051/ >> > Thanks Murali, very interesting. > > The thread in your link above mentions omap_hwmod at the end, and I've been looking at how that's handled but it's quite complex. Would a similar solution (davinci_hwmod) be a good approach? > > Regards, > > - Rob > Rob, I suggest you channel your response to the above thread from Stephen that would help in coming up with an API to do this as a generic solution. Murali
On 12/4/2012 9:43 PM, Murali Karicheri wrote: > On 12/04/2012 09:53 AM, Murali Karicheri wrote: >> On 12/04/2012 12:58 AM, Sekhar Nori wrote: >>> On 12/4/2012 1:43 AM, Cyril Chemparathy wrote: >>> >>>> On 12/03/2012 09:41 AM, Sekhar Nori wrote: >>>>> On 12/1/2012 7:41 AM, Tivy, Robert wrote: >>>>> Passing function pointers from platform code will not work when >>>>> converting to device tree (DT). DA850 will gain DT boot with v3.8 and >>>>> there is work ongoing on converting drivers to be DT-aware. Adding >>>>> a new >>>>> driver which is DT-incompatible will not be right. Hence, I will not >>>>> recommend this now. >>>> Not sure if this solves your problem, but you could add a new clock >>>> type >>>> (PSC_LRESET?) as a child under the PSC clock node for the DSP. >>>> Something >>>> like: >>>> >>>> | >>>> +-- PSC x (DSP0 clock) >>>> | | >>>> | +-- PSC-LRESET x (DSP0 reset control) >>>> | >>>> +-- PSC y (DSP1 clock) >>>> | | >>>> | +-- PSC-LRESET y (DSP1 reset control) >>>> | >>>> +-- PSC z (DSP2 clock) >>>> | | >>>> | +-- PSC-LRESET z (DSP2 reset control) >>>> >>>> >>>> The idea here being that if you enable the PSC clocks, you enable the >>>> clock gates, but leave the DSPs in reset. On the other hand, if you >>>> enable the reset clock, the code implicitly enables the gate (via >>>> parent), and takes the DSP out of reset as well. >>>> >>>> This is not quite the cleanest way to do it, considering that reset >>>> lines have no business in the clock tree, but then, this is probably >>>> the >>>> simplest way to fit in. Thoughts? >>> This may work (on a technical level), but I am not really a fan of this >>> since this is essentially a hack, as you (almost) point out. It does not >>> model the hardware clock tree correctly and we are still struck with >>> using clock APIs for reset control. >>> >>> I still think we need to find a better method of dealing with IP reset. >>> May be an acceptable method already exists. Some one needs to summarize >>> the problem statement well enough and I am sure finding the right >>> solution will not be too long drawn. >>> >>> Thanks, >>> Sekhar >>> _______________________________________________ >>> Davinci-linux-open-source mailing list >>> Davinci-linux-open-source@linux.davincidsp.com >>> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source >>> >>> >> I believe this is addressing the same issue. This is a DT based >> solution, which I believe should add a framework and enhance it to >> support DT. > Forgot to paste the link. Here we go > > https://patchwork.kernel.org/patch/1635051/ Rob, Since there is a solution for reset handling proposed but seems to be slightly far from taking a concrete shape, I suggest you go ahead and implement davinci reset using a platform private API ala Tegra. You can create and send the patches. I will review and accept them but not send them upstream immediately. Lets wait till v3.8-rc4. If there is a generic solution that is merged by that time, I will ask you to use that instead of your API (this will give you about 2 weeks to convert). If the generic solution is not merged by that time, I will send your private API to the upstreams for v3.9 and we can convert to generic solution for later kernel versions. This should give you a fair chance to get remoteproc working on DA850 for v3.9. Sounds like a plan? The remoteproc folks need to agree too. I am copying Ohad here. Meanwhile I do suggest you work with Stephen and Mike in giving shape to the reset API so it suits your needs when it gets ready. Thanks, Sekhar
> -----Original Message----- > From: Nori, Sekhar > Sent: Friday, December 07, 2012 12:24 AM > On 12/4/2012 9:43 PM, Murali Karicheri wrote: > > On 12/04/2012 09:53 AM, Murali Karicheri wrote: > >> I believe this is addressing the same issue. This is a DT based > >> solution, which I believe should add a framework and enhance it to > >> support DT. > > Forgot to paste the link. Here we go > > > > https://patchwork.kernel.org/patch/1635051/ > > Rob, > > Since there is a solution for reset handling proposed but seems to be > slightly far from taking a concrete shape, I suggest you go ahead and > implement davinci reset using a platform private API ala Tegra. > > You can create and send the patches. I will review and accept them but > not send them upstream immediately. > > Lets wait till v3.8-rc4. If there is a generic solution that is merged > by that time, I will ask you to use that instead of your API (this will > give you about 2 weeks to convert). If the generic solution is not > merged by that time, I will send your private API to the upstreams for > v3.9 and we can convert to generic solution for later kernel versions. > > This should give you a fair chance to get remoteproc working on DA850 > for v3.9. > > Sounds like a plan? The remoteproc folks need to agree too. I am > copying > Ohad here. I'm on board with that plan, thanks very much for the suggestion, I agree completely. > > Meanwhile I do suggest you work with Stephen and Mike in giving shape > to > the reset API so it suits your needs when it gets ready. Will do. Thanks & Regards, - Rob > > Thanks, > Sekhar
> -----Original Message----- > From: Nori, Sekhar > Sent: Friday, November 30, 2012 2:51 AM > > Hi Rob, > > On 11/29/2012 7:08 AM, Tivy, Robert wrote: > > Hi Sekhar, > > > > Please see comments and noob questions below... > > > > 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 have kernel command line, you could default to some values which > are sane in most cases if they are not provided. No, need to have a > Kconfig as well. That will be too many hooks to control the same things > and probably not necessary. > > > > >> 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. > > Then I guess most of the time the module would be built into the kernel > and will be initialized using an early enough initcall. That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). > >>> + > >>> +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)? > > Right. Module parameters are passed from kernel command line when the > module is built into the kernel. Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? If not, is this a strong enough use case to justify a new kernel parameter? I guess I don't understand why non-standard kernel parameters are discouraged. I can adorn the name with enough module-specific naming to reduce the chances of any namespace collisions to a minimum. Regards, - Rob > > Thanks, > Sekhar
On 12/12/2012 7:06 AM, Tivy, Robert wrote: >> -----Original Message----- >> From: Nori, Sekhar >> Sent: Friday, November 30, 2012 2:51 AM >> >> Hi Rob, >> >> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>> Hi Sekhar, >>> >>> Please see comments and noob questions below... >>> >>> 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 have kernel command line, you could default to some values which >> are sane in most cases if they are not provided. No, need to have a >> Kconfig as well. That will be too many hooks to control the same things >> and probably not necessary. >> >>> >>>> 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. >> >> Then I guess most of the time the module would be built into the kernel >> and will be initialized using an early enough initcall. > > That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). I meant use an early initcall in the driver. > >>>>> + >>>>> +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)? >> >> Right. Module parameters are passed from kernel command line when the >> module is built into the kernel. > > Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. By "later normal __init" you mean module_init()? And you see dma_declare_contiguous() returning error by this time? > > Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? Nothing else comes to mind. Can you share your code in some public repo? It will allow me to try if something comes to mind. > > If not, is this a strong enough use case to justify a new kernel parameter? May be it it is. You could try adding it. Just update the documentation too. And add the documentation maintainers when you respin the patch. > > I guess I don't understand why non-standard kernel parameters are discouraged. I can adorn the name with enough module-specific naming to reduce the chances of any namespace collisions to a minimum. I guess it is to make sure generic solutions are followed and every problem is not seen as unique. Thanks, Sekhar
Hi Sekhar, On Wed, Dec 12, 2012 at 4:04 PM, Sekhar Nori <nsekhar@ti.com> wrote: > On 12/12/2012 7:06 AM, Tivy, Robert wrote: >>> -----Original Message----- >>> From: Nori, Sekhar >>> Sent: Friday, November 30, 2012 2:51 AM >>> >>> Hi Rob, >>> >>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>> Hi Sekhar, >>>> >>>> Please see comments and noob questions below... >>>> >>>> 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 have kernel command line, you could default to some values which >>> are sane in most cases if they are not provided. No, need to have a >>> Kconfig as well. That will be too many hooks to control the same things >>> and probably not necessary. >>> >>>> >>>>> 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. >>> >>> Then I guess most of the time the module would be built into the kernel >>> and will be initialized using an early enough initcall. >> >> That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). > > I meant use an early initcall in the driver. > >> >>>>>> + >>>>>> +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)? >>> >>> Right. Module parameters are passed from kernel command line when the >>> module is built into the kernel. >> >> Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. > > By "later normal __init" you mean module_init()? And you see > dma_declare_contiguous() returning error by this time? > >> >> Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? > > Nothing else comes to mind. Can you share your code in some public repo? > It will allow me to try if something comes to mind. > >> >> If not, is this a strong enough use case to justify a new kernel parameter? > > May be it it is. You could try adding it. Just update the documentation > too. And add the documentation maintainers when you respin the patch. > I tried finding some alternatives apart from early_param, but there aren’t any. The only thought I got from Marek is to have device tree support. How about that as an option ? Regards, --Prabhakar Lad >> >> I guess I don't understand why non-standard kernel parameters are discouraged. I can adorn the name with enough module-specific naming to reduce the chances of any namespace collisions to a minimum. > > I guess it is to make sure generic solutions are followed and every > problem is not seen as unique. > > Thanks, > Sekhar > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
On 12/12/2012 4:31 PM, Prabhakar Lad wrote: > Hi Sekhar, > > On Wed, Dec 12, 2012 at 4:04 PM, Sekhar Nori <nsekhar@ti.com> wrote: >> On 12/12/2012 7:06 AM, Tivy, Robert wrote: >>>> -----Original Message----- >>>> From: Nori, Sekhar >>>> Sent: Friday, November 30, 2012 2:51 AM >>>> >>>> Hi Rob, >>>> >>>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>>> Hi Sekhar, >>>>> >>>>> Please see comments and noob questions below... >>>>> >>>>> 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 have kernel command line, you could default to some values which >>>> are sane in most cases if they are not provided. No, need to have a >>>> Kconfig as well. That will be too many hooks to control the same things >>>> and probably not necessary. >>>> >>>>> >>>>>> 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. >>>> >>>> Then I guess most of the time the module would be built into the kernel >>>> and will be initialized using an early enough initcall. >>> >>> That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). >> >> I meant use an early initcall in the driver. >> >>> >>>>>>> + >>>>>>> +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)? >>>> >>>> Right. Module parameters are passed from kernel command line when the >>>> module is built into the kernel. >>> >>> Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. >> >> By "later normal __init" you mean module_init()? And you see >> dma_declare_contiguous() returning error by this time? >> >>> >>> Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? >> >> Nothing else comes to mind. Can you share your code in some public repo? >> It will allow me to try if something comes to mind. >> >>> >>> If not, is this a strong enough use case to justify a new kernel parameter? >> >> May be it it is. You could try adding it. Just update the documentation >> too. And add the documentation maintainers when you respin the patch. >> > I tried finding some alternatives apart from early_param, but there aren’t any. > The only thought I got from Marek is to have device tree support. How about > that as an option ? Can you explain some more? How does device tree help here? May be give a link to this discussion so I can read? Thanks, Sekhar
On Wed, Dec 12, 2012 at 6:36 PM, Sekhar Nori <nsekhar@ti.com> wrote: > On 12/12/2012 4:31 PM, Prabhakar Lad wrote: >> Hi Sekhar, >> >> On Wed, Dec 12, 2012 at 4:04 PM, Sekhar Nori <nsekhar@ti.com> wrote: >>> On 12/12/2012 7:06 AM, Tivy, Robert wrote: >>>>> -----Original Message----- >>>>> From: Nori, Sekhar >>>>> Sent: Friday, November 30, 2012 2:51 AM >>>>> >>>>> Hi Rob, >>>>> >>>>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>>>> Hi Sekhar, >>>>>> >>>>>> Please see comments and noob questions below... >>>>>> >>>>>> 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 have kernel command line, you could default to some values which >>>>> are sane in most cases if they are not provided. No, need to have a >>>>> Kconfig as well. That will be too many hooks to control the same things >>>>> and probably not necessary. >>>>> >>>>>> >>>>>>> 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. >>>>> >>>>> Then I guess most of the time the module would be built into the kernel >>>>> and will be initialized using an early enough initcall. >>>> >>>> That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). >>> >>> I meant use an early initcall in the driver. >>> >>>> >>>>>>>> + >>>>>>>> +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)? >>>>> >>>>> Right. Module parameters are passed from kernel command line when the >>>>> module is built into the kernel. >>>> >>>> Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. >>> >>> By "later normal __init" you mean module_init()? And you see >>> dma_declare_contiguous() returning error by this time? >>> >>>> >>>> Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? >>> >>> Nothing else comes to mind. Can you share your code in some public repo? >>> It will allow me to try if something comes to mind. >>> >>>> >>>> If not, is this a strong enough use case to justify a new kernel parameter? >>> >>> May be it it is. You could try adding it. Just update the documentation >>> too. And add the documentation maintainers when you respin the patch. >>> >> I tried finding some alternatives apart from early_param, but there aren’t any. >> The only thought I got from Marek is to have device tree support. How about >> that as an option ? > > Can you explain some more? How does device tree help here? May be give a > link to this discussion so I can read? > This patch http://thread.gmane.org/gmane.linux.kernel.samsung-soc/12911/ should explain it. Regards, --Prabhakar Lad > Thanks, > Sekhar
Sekhar, On Wed, Dec 12, 2012 at 6:43 PM, Prabhakar Lad <prabhakar.csengg@gmail.com> wrote: > On Wed, Dec 12, 2012 at 6:36 PM, Sekhar Nori <nsekhar@ti.com> wrote: >> On 12/12/2012 4:31 PM, Prabhakar Lad wrote: >>> Hi Sekhar, >>> >>> On Wed, Dec 12, 2012 at 4:04 PM, Sekhar Nori <nsekhar@ti.com> wrote: >>>> On 12/12/2012 7:06 AM, Tivy, Robert wrote: >>>>>> -----Original Message----- >>>>>> From: Nori, Sekhar >>>>>> Sent: Friday, November 30, 2012 2:51 AM >>>>>> >>>>>> Hi Rob, >>>>>> >>>>>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>>>>> Hi Sekhar, >>>>>>> >>>>>>> Please see comments and noob questions below... >>>>>>> >>>>>>> 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 have kernel command line, you could default to some values which >>>>>> are sane in most cases if they are not provided. No, need to have a >>>>>> Kconfig as well. That will be too many hooks to control the same things >>>>>> and probably not necessary. >>>>>> >>>>>>> >>>>>>>> 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. >>>>>> >>>>>> Then I guess most of the time the module would be built into the kernel >>>>>> and will be initialized using an early enough initcall. >>>>> >>>>> That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). >>>> >>>> I meant use an early initcall in the driver. >>>> >>>>> >>>>>>>>> + >>>>>>>>> +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)? >>>>>> >>>>>> Right. Module parameters are passed from kernel command line when the >>>>>> module is built into the kernel. >>>>> >>>>> Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. >>>> >>>> By "later normal __init" you mean module_init()? And you see >>>> dma_declare_contiguous() returning error by this time? >>>> >>>>> >>>>> Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? >>>> >>>> Nothing else comes to mind. Can you share your code in some public repo? >>>> It will allow me to try if something comes to mind. >>>> >>>>> >>>>> If not, is this a strong enough use case to justify a new kernel parameter? >>>> >>>> May be it it is. You could try adding it. Just update the documentation >>>> too. And add the documentation maintainers when you respin the patch. >>>> >>> I tried finding some alternatives apart from early_param, but there aren’t any. >>> The only thought I got from Marek is to have device tree support. How about >>> that as an option ? >> >> Can you explain some more? How does device tree help here? May be give a >> link to this discussion so I can read? >> > This patch http://thread.gmane.org/gmane.linux.kernel.samsung-soc/12911/ > should explain it. > Sorry for my previous short mail. >From the chain of previous mails what I understand is you need to pass the base and size which is user provided, currently which is implemented using early_param(), but use of non-standard kernel parameters is discouraged, The only alternative could be pass the base address through device tree, As similarly implemented in the patch pointed above that would avoid rebuilding the kernel and just build/rebuild the dtbs changing the bases. Regards, --Prabhakar Lad > Regards, > --Prabhakar Lad > >> Thanks, >> Sekhar
> -----Original Message----- > From: Nori, Sekhar > Sent: Wednesday, December 12, 2012 2:34 AM > On 12/12/2012 7:06 AM, Tivy, Robert wrote: > >> -----Original Message----- > >> From: Nori, Sekhar > >> Sent: Friday, November 30, 2012 2:51 AM > >> > >> Hi Rob, > >> > >> On 11/29/2012 7:08 AM, Tivy, Robert wrote: > >>> Hi Sekhar, > >>> > >>> Please see comments and noob questions below... > >>> > >>> 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 have kernel command line, you could default to some values > which > >> are sane in most cases if they are not provided. No, need to have a > >> Kconfig as well. That will be too many hooks to control the same > things > >> and probably not necessary. > >> > >>> > >>>> 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. > >> > >> Then I guess most of the time the module would be built into the > kernel > >> and will be initialized using an early enough initcall. > > > > That's right, a .reserve function is assigned to the MACHINE_START > structure, and this function calls dma_declare_contiguous(). > > I meant use an early initcall in the driver. > > > > >>>>> + > >>>>> +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)? > >> > >> Right. Module parameters are passed from kernel command line when > the > >> module is built into the kernel. > > > > Unfortunately, it seems that module parameters, when stated on the > kernel command line with module-name.var-name syntax, are not yet > assigned when the kernel calls the early init .reserve functions. For > the "char *" I'm using, I observed a NULL value during the early init > call and the proper assigned value during a later normal __init > function. > > By "later normal __init" you mean module_init()? And you see > dma_declare_contiguous() returning error by this time? That's right, dma_declare_contiguous() must be called very early in the boot, else it fails. The module_init() function is too late, and I even tried with a function qualified with early_initcall() instead of module_init(). Here's some text from the header comment for dma_declare_contiguous(): * This function reserves memory for specified device. It should be * called by board specific code when early allocator (memblock or bootmem) * is still activate. > > > > > Is there any other method that allows specifying a parameter for an > early CMA reservation without having to rebuild the kernel? > > Nothing else comes to mind. Can you share your code in some public > repo? > It will allow me to try if something comes to mind. I will attempt to put my code on github. Thanks & Regards, - Rob > > Thanks, > Sekhar
On 12/13/2012 1:48 PM, Prabhakar Lad wrote: > Sekhar, > > On Wed, Dec 12, 2012 at 6:43 PM, Prabhakar Lad > <prabhakar.csengg@gmail.com> wrote: >> On Wed, Dec 12, 2012 at 6:36 PM, Sekhar Nori <nsekhar@ti.com> wrote: >>> On 12/12/2012 4:31 PM, Prabhakar Lad wrote: >>>> Hi Sekhar, >>>> >>>> On Wed, Dec 12, 2012 at 4:04 PM, Sekhar Nori <nsekhar@ti.com> wrote: >>>>> On 12/12/2012 7:06 AM, Tivy, Robert wrote: >>>>>>> -----Original Message----- >>>>>>> From: Nori, Sekhar >>>>>>> Sent: Friday, November 30, 2012 2:51 AM >>>>>>> >>>>>>> Hi Rob, >>>>>>> >>>>>>> On 11/29/2012 7:08 AM, Tivy, Robert wrote: >>>>>>>> Hi Sekhar, >>>>>>>> >>>>>>>> Please see comments and noob questions below... >>>>>>>> >>>>>>>> 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 have kernel command line, you could default to some values which >>>>>>> are sane in most cases if they are not provided. No, need to have a >>>>>>> Kconfig as well. That will be too many hooks to control the same things >>>>>>> and probably not necessary. >>>>>>> >>>>>>>> >>>>>>>>> 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. >>>>>>> >>>>>>> Then I guess most of the time the module would be built into the kernel >>>>>>> and will be initialized using an early enough initcall. >>>>>> >>>>>> That's right, a .reserve function is assigned to the MACHINE_START structure, and this function calls dma_declare_contiguous(). >>>>> >>>>> I meant use an early initcall in the driver. >>>>> >>>>>> >>>>>>>>>> + >>>>>>>>>> +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)? >>>>>>> >>>>>>> Right. Module parameters are passed from kernel command line when the >>>>>>> module is built into the kernel. >>>>>> >>>>>> Unfortunately, it seems that module parameters, when stated on the kernel command line with module-name.var-name syntax, are not yet assigned when the kernel calls the early init .reserve functions. For the "char *" I'm using, I observed a NULL value during the early init call and the proper assigned value during a later normal __init function. >>>>> >>>>> By "later normal __init" you mean module_init()? And you see >>>>> dma_declare_contiguous() returning error by this time? >>>>> >>>>>> >>>>>> Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel? >>>>> >>>>> Nothing else comes to mind. Can you share your code in some public repo? >>>>> It will allow me to try if something comes to mind. >>>>> >>>>>> >>>>>> If not, is this a strong enough use case to justify a new kernel parameter? >>>>> >>>>> May be it it is. You could try adding it. Just update the documentation >>>>> too. And add the documentation maintainers when you respin the patch. >>>>> >>>> I tried finding some alternatives apart from early_param, but there aren’t any. >>>> The only thought I got from Marek is to have device tree support. How about >>>> that as an option ? >>> >>> Can you explain some more? How does device tree help here? May be give a >>> link to this discussion so I can read? >>> >> This patch http://thread.gmane.org/gmane.linux.kernel.samsung-soc/12911/ >> should explain it. >> > Sorry for my previous short mail. > >>From the chain of previous mails what I understand is you need to pass > the base and size which is user provided, currently which is implemented > using early_param(), but use of non-standard kernel parameters is discouraged, > The only alternative could be pass the base address through device tree, > As similarly implemented in the patch pointed above that would avoid > rebuilding the kernel > and just build/rebuild the dtbs changing the bases. Right, thanks for the link. Samsung did solve this by passing the addresses from DT (I have my reservations with the approach though since this is not strictly hardware data). We support non-DT boot also. Unless Rob is okay to have a DT only version of the driver, we still need a solution for the non-DT case. Thanks, Sekhar
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, +}; + 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, + .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", +}; + +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; + +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); + +void __init da8xx_rproc_reserve_cma(void) +{ + int ret; + + 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); +} + +#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; + } + + dev_set_name(&da8xx_dsp.dev, "%s.%d", da8xx_dsp.name, da8xx_dsp.id); + + return 0; +}; + static struct resource da8xx_rtc_resources[] = { { .start = DA8XX_RTC_BASE, diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h index aaccdc4..5c7f7f7 100644 --- a/arch/arm/mach-davinci/include/mach/da8xx.h +++ b/arch/arm/mach-davinci/include/mach/da8xx.h @@ -95,6 +95,7 @@ int da850_register_cpufreq(char *async_clk); int da8xx_register_cpuidle(void); void __iomem * __init da8xx_get_mem_ctlr(void); int da850_register_pm(struct platform_device *pdev); +int da8xx_register_rproc(void); int __init da850_register_sata(unsigned long refclkpn); int __init da850_register_vpif(void); int __init da850_register_vpif_display diff --git a/arch/arm/mach-davinci/include/mach/psc.h b/arch/arm/mach-davinci/include/mach/psc.h index 40a0027..21746bd 100644 --- a/arch/arm/mach-davinci/include/mach/psc.h +++ b/arch/arm/mach-davinci/include/mach/psc.h @@ -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) @@ -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 domain, unsigned int ctlr, + unsigned int id, bool reset, u32 flags); extern void davinci_psc_config(unsigned int domain, unsigned int ctlr, unsigned int id, bool enable, u32 flags); diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c index bddaba9..2be8b8a 100644 --- a/arch/arm/mach-davinci/psc.c +++ b/arch/arm/mach-davinci/psc.c @@ -48,6 +48,33 @@ int __init davinci_psc_is_clk_active(unsigned int ctlr, unsigned int id) return mdstat & BIT(12); } +void davinci_psc_reset_config(unsigned int domain, unsigned int ctlr, + unsigned int id, bool reset, u32 flags) +{ + u32 mdctl; + void __iomem *psc_base; + struct davinci_soc_info *soc_info = &davinci_soc_info; + + if (!soc_info->psc_bases || (ctlr >= soc_info->psc_bases_num)) { + pr_warn("PSC: Bad psc data: 0x%x[%d]\n", + (int)soc_info->psc_bases, ctlr); + return; + } + + psc_base = ioremap(soc_info->psc_bases[ctlr], SZ_4K); + + if (flags & PSC_LRST) { + mdctl = readl(psc_base + MDCTL + 4 * id); + if (reset) + mdctl &= ~MDCTL_LRST; + else + mdctl |= MDCTL_LRST; + writel(mdctl, psc_base + MDCTL + 4 * id); + } + + iounmap(psc_base); +} + /* Enable or disable a PSC domain */ void davinci_psc_config(unsigned int domain, unsigned int ctlr, unsigned int id, bool enable, u32 flags) diff --git a/arch/arm/mach-davinci/remoteproc.h b/arch/arm/mach-davinci/remoteproc.h new file mode 100644 index 0000000..a94c013 --- /dev/null +++ b/arch/arm/mach-davinci/remoteproc.h @@ -0,0 +1,23 @@ +/* + * 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. + */ + +#ifndef _MACH_REMOTEPROC_H +#define _MACH_REMOTEPROC_H + +#ifdef CONFIG_CMA +extern void __init da8xx_rproc_reserve_cma(void); +#endif + +#endif /* _MACH_REMOTEPROC_H */ diff --git a/include/linux/clk.h b/include/linux/clk.h index b3ac22d..9b1a56f 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -187,6 +187,23 @@ int clk_enable(struct clk *clk); void clk_disable(struct clk *clk); /** + * clk_reset - inform the system when the clock source should be reset or + * taken out of reset. + * @clk: clock source + * + * Applies to clock sources that have an associated reset for the device + * controlled by the clock. + * + * If the device being clocked can not be reset/taken-out-of-reset, this + * should return success. + * + * May be called from atomic contexts. + * + * Returns success (0) or negative errno. + */ +int clk_reset(struct clk *clk, bool reset); + +/** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. * @clk: clock source diff --git a/include/linux/platform_data/da8xx-remoteproc.h b/include/linux/platform_data/da8xx-remoteproc.h new file mode 100644 index 0000000..1e79809 --- /dev/null +++ b/include/linux/platform_data/da8xx-remoteproc.h @@ -0,0 +1,34 @@ +/* + * 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. + */ + +#ifndef __DA8XX_REMOTEPROC_H__ +#define __DA8XX_REMOTEPROC_H__ + +#include <linux/remoteproc.h> + +/** + * struct da8xx_rproc_pdata - da8xx remoteproc's platform data + * @name: the remoteproc's name + * @clk_name: the remoteproc's clock + * @firmware: name of firmware file to load + * @ops: start/stop rproc handlers + */ +struct da8xx_rproc_pdata { + const char *name; + const char *firmware; + const struct rproc_ops *ops; +}; + +#endif /* __DA8XX_REMOTEPROC_H__ */
Requires CMA services. New 'clk_reset()' API added so that the DSP's reset state can be toggled separately from its enable/disable operation. Signed-off-by: Robert Tivy <rtivy@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 +++++++++++ 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