ARM: davinci: Changed pr_warning() to pr_warn() (part 1)

Also, while modifying those pr_warning() calls I changed hardcoded function names to use '"%s:", __func__' instead

Signed-off-by Robert Tivy <[email protected]>

---
Clean up files that will be otherwise modified in subsequent patch.

Applies to v3.7-rc2 tag (commit 6f0c0580b70c89094b3422ba81118c7b959c7556) of Linus' mainline kernel at git.kernel.org.

  arch/arm/mach-davinci/board-da850-evm.c |  102 +++++++++++++------------------
  1 file changed, 43 insertions(+), 59 deletions(-)

arch/arm/mach-davinci/board-da850-evm.c +0 -0

index 32ee3f8..6c172b3 100644

arch/arm/mach-davinci/board-da850-evm.c +0 -0

@@ -688,14 +688,14 @@ static int da850_evm_bb_expander_setup(struct i2c_client *client,
  	da850_evm_bb_keys_init(gpio);
  	ret = platform_device_register(&da850_evm_bb_keys_device);
  	if (ret) {

arch/arm/mach-davinci/board-da850-evm.c +2 -6

  	ret = da850_evm_init_cpufreq();
  	if (ret)
-		pr_warning("da850_evm_init: cpufreq registration failed: %d\n",
-				ret);
+		pr_warn("%s: cpufreq registration failed: %d\n", __func__, ret);
 
  	ret = da8xx_register_cpuidle();
  	if (ret)
-		pr_warning("da850_evm_init: cpuidle registration failed: %d\n",
-				ret);
+		pr_warn("%s: cpuidle registration failed: %d\n", __func__, ret);
 
  	ret = da850_register_pm(&da850_pm_device);
  	if (ret)
-		pr_warning("da850_evm_init: suspend registration failed: %d\n",
-				ret);

Also, while modifying those pr_warning() calls I changed hardcoded function names to use '"%s:", __func__' instead

Signed-off-by Robert Tivy <[email protected]>

---
Clean up files that will be otherwise modified in subsequent patch.

Applies to v3.7-rc2 tag (commit 6f0c0580b70c89094b3422ba81118c7b959c7556) of Linus' mainline kernel at git.kernel.org.

  arch/arm/mach-davinci/board-omapl138-hawk.c |   30 ++++++++++-----------------
  1 file changed, 11 insertions(+), 19 deletions(-)

-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
Sent: Wednesday, November 14, 2012 2:17 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark; Nori, Sekhar
Subject: Re: [PATCH v3 2/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 2)

Hello.

Quoted 9 lines · Robert Tivy

Also, while modifying those pr_warning() calls I changed hardcoded function names to use '"%s:", __func__' instead

Signed-off-by Robert Tivy <[email protected]>

---
Clean up files that will be otherwise modified in subsequent patch.

Applies to v3.7-rc2 tag (commit 6f0c0580b70c89094b3422ba81118c7b959c7556) of Linus' mainline kernel

at git.kernel.org.

Quoted 1 line

  arch/arm/mach-davinci/board-omapl138-hawk.c |   30 ++++++++++------

-----------

Quoted 1 line

  1 file changed, 11 insertions(+), 19 deletions(-)

Taksing of separation of board and SoC specific changes, this patch
shouldn't have been separated from patch 1 at all -- since it's two boards built around the same chip, OMAP-L138...

arch/arm/mach-davinci/board-omapl138-hawk.c static __init void omapl138_hawk_config_emac(void) · line 51

Quoted 13 lines

arch/arm/mach-davinci/board-omapl138-hawk.c +1 -2

index dc1208e..8aea169 100644
@@ -48,8 +48,7 @@ static __init void omapl138_hawk_config_emac(void)
  	val &= ~BIT(8);
  	ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
  	if (ret) {
-		pr_warning("%s: cpgmac/mii mux setup failed: %d\n",
-			__func__, ret);
+		pr_warn("%s: cpgmac/mii mux setup failed: %d\n", __func__,

ret);

I'd have preferred this as "CPGMAC/MII". Almost all other acronyms
in the messages are capitalized.

arch/arm/mach-davinci/board-omapl138-hawk.c static __init void omapl138_hawk_config_emac(void) · line 63

Quoted 11 lines

arch/arm/mach-davinci/board-omapl138-hawk.c +1 -2

  		return;
  	}
 

@@ -61,8 +60,7 @@ static __init void omapl138_hawk_config_emac(void)
 
  	ret = da8xx_register_emac();
  	if (ret)
-		pr_warning("%s: emac registration failed: %d\n",
-			__func__, ret);
+		pr_warn("%s: emac registration failed: %d\n", __func__,

ret);

    ... and "EMAC" here.

WBR, Sergei

Quoted 9 lines

Quoted 9 lines

Also, while modifying those pr_warning() calls I changed hardcoded function names to use '"%s:", __func__' instead

Signed-off-by Robert Tivy <[email protected]>

---
Clean up files that will be otherwise modified in subsequent patch.

Applies to v3.7-rc2 tag (commit 6f0c0580b70c89094b3422ba81118c7b959c7556) of Linus' mainline kernel

Quoted 7 lines

at git.kernel.org.

Quoted 1 line

   arch/arm/mach-davinci/board-omapl138-hawk.c |   30 ++++++++++------

-----------

Quoted 1 line

   1 file changed, 11 insertions(+), 19 deletions(-)

Taksing of separation of board and SoC specific changes, this patch

Quoted 2 lines

shouldn't have been separated from patch 1 at all -- since it's two boards built around the same chip, OMAP-L138...

The 4 patches that are of the same nature ("Changed pr_warning() to pr_warn() (part #)") were split as 4 separate patches on request by Sekhar, for the purpose of making it easier to merge later.

Hello.

Quoted 9 lines · Robert

Quoted 9 lines

Quoted 9 lines

Also, while modifying those pr_warning() calls I changed hardcoded function names to use '"%s:", __func__' instead

Signed-off-by Robert Tivy <[email protected]>

---
Clean up files that will be otherwise modified in subsequent patch.

Applies to v3.7-rc2 tag (commit 6f0c0580b70c89094b3422ba81118c7b959c7556) of Linus' mainline kernel

Quoted 7 lines

Quoted 7 lines

at git.kernel.org.

Quoted 1 line

   arch/arm/mach-davinci/board-omapl138-hawk.c |   30 ++++++++++------

-----------

Quoted 1 line

   1 file changed, 11 insertions(+), 19 deletions(-)

Taksing of separation of board and SoC specific changes, this patch

   I meant "talking". :-/

Quoted 2 lines

Quoted 2 lines

shouldn't have been separated from patch 1 at all -- since it's two boards built around the same chip, OMAP-L138...

Quoted 3 lines

The 4 patches that are of the same nature ("Changed pr_warning() to pr_warn() (part #)") were split as 4 separate patches on request by Sekhar, for the purpose of making it easier to merge later.

To quote Sekhar: "My main motivation was to keep board and soc file
changes from mixing together." You just went too far.

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 <[email protected]>

---
 arch/arm/mach-davinci/board-da850-evm.c        |   10 +++
 arch/arm/mach-davinci/board-omapl138-hawk.c    |   10 +++
 arch/arm/mach-davinci/clock.c                  |   22 +++++++
 arch/arm/mach-davinci/clock.h                  |    1 +
 arch/arm/mach-davinci/da850.c                  |   17 ++++++
 arch/arm/mach-davinci/devices-da8xx.c          |   78 +++++++++++++++++++++++-
 arch/arm/mach-davinci/include/mach/da8xx.h     |    1 +
 arch/arm/mach-davinci/include/mach/psc.h       |    3 +
 arch/arm/mach-davinci/psc.c                    |   27 ++++++++
 arch/arm/mach-davinci/remoteproc.h             |   23 +++++++
 include/linux/clk.h                            |   17 ++++++
 include/linux/platform_data/da8xx-remoteproc.h |   34 +++++++++++

 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

arch/arm/mach-davinci/board-da850-evm.c +10 -0

index 6c172b3..4e86008 100644
@@ -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

arch/arm/mach-davinci/board-omapl138-hawk.c +10 -0

index 8aea169..a0b81c1 100644
@@ -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

arch/arm/mach-davinci/clock.c +22 -0

index 34668ea..3fad759 100644
@@ -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;

arch/arm/mach-davinci/clock.h +1 -0

index 46f0f1b..89aa95e 100644
@@ -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) 	\
 	{			\

arch/arm/mach-davinci/da850.c +0 -0

index b90c172..4008fdc 100644

arch/arm/mach-davinci/da850.c +11 -0

+
 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),
 };
 

arch/arm/mach-davinci/devices-da8xx.c +4 -1

index 466b70c..ae54769 100644
@@ -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>

arch/arm/mach-davinci/devices-da8xx.c +7 -0

+};
+
+static struct platform_device da8xx_dsp = {
+	.name	= "davinci-rproc",
+	.id	= 0,
+	.dev	= {
+		.platform_data		= &rproc_pdata,

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Quoted 4 lines · Robert Tivy

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?

Quoted 10 lines

Signed-off-by Robert Tivy <[email protected]>

---
 arch/arm/mach-davinci/board-da850-evm.c        |   10 +++
 arch/arm/mach-davinci/board-omapl138-hawk.c    |   10 +++
 arch/arm/mach-davinci/clock.c                  |   22 +++++++
 arch/arm/mach-davinci/clock.h                  |    1 +
 arch/arm/mach-davinci/da850.c                  |   17 ++++++
 arch/arm/mach-davinci/devices-da8xx.c          |   78

+++++++++++++++++++++++-

Quoted 6 lines

 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.

Quoted 20 lines

 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

arch/arm/mach-davinci/board-da850-evm.c +2 -0

index 6c172b3..4e86008 100644
@@ -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)

arch/arm/mach-davinci/board-da850-evm.c static __init void da850_evm_init(void) · line 1552

Quoted 1 line

arch/arm/mach-davinci/board-da850-evm.c +0 -0

@@ -1550,6 +1552,11 @@ static __init void da850_evm_init(void)
 		pr_warn("%s: sata registration failed: %d\n", __func__,

ret);

arch/arm/mach-davinci/board-da850-evm.c MACHINE_START(DAVINCI_DA850_EVM, "DaVinci

Quoted 11 lines

arch/arm/mach-davinci/board-da850-evm.c +5 -0

 	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")

arch/arm/mach-davinci/board-omapl138-hawk.c MACHINE_START(OMAPL138_HAWKBOARD, "AM18x/OMAP-

Quoted 34 lines

arch/arm/mach-davinci/board-da850-evm.c +3 -0

 	.init_late	= davinci_init_late,
 	.dma_zone_size	= SZ_128M,
 	.restart	= da8xx_restart,
+#ifdef CONFIG_CMA
+	.reserve	= da8xx_rproc_reserve_cma,
+#endif
 MACHINE_END

arch/arm/mach-davinci/board-omapl138-hawk.c +7 -0

index 8aea169..a0b81c1 100644
@@ -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")

Quoted 48 lines

arch/arm/mach-davinci/board-omapl138-hawk.c +3 -0

 	.init_late	= davinci_init_late,
 	.dma_zone_size	= SZ_128M,
 	.restart	= da8xx_restart,
+#ifdef CONFIG_CMA
+	.reserve	= da8xx_rproc_reserve_cma,
+#endif
 MACHINE_END

(quoted diff) +0 -0

b/arch/arm/mach-davinci/clock.c index 34668ea..3fad759 100644

arch/arm/mach-davinci/clock.c +20 -0

@@ -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;

(quoted diff) +0 -0

b/arch/arm/mach-davinci/clock.h index 46f0f1b..89aa95e 100644

arch/arm/mach-davinci/clock.h struct clk { · line 112

Quoted 1 line

arch/arm/mach-davinci/clock.h +0 -0

@@ -112,6 +112,7 @@ struct clk {
 #define PRE_PLL			BIT(4) /* source is before PLL

mult/div */

arch/arm/mach-davinci/clock.h struct clk { · line 113

Quoted 1 line

arch/arm/mach-davinci/clock.h +0 -0

 #define PSC_SWRSTDISABLE	BIT(5) /* Disable state is SwRstDisable

*/

arch/arm/mach-davinci/clock.h struct clk { · line 114

Quoted 1 line

arch/arm/mach-davinci/clock.h +0 -0

 #define PSC_FORCE		BIT(6) /* Force module state transtition

*/

arch/arm/mach-davinci/clock.h struct clk { · line 115

Quoted 1 line

arch/arm/mach-davinci/clock.h +1 -0

+#define PSC_LRST		BIT(8) /* Use local reset on

enable/disable */

Quoted 11 lines

arch/arm/mach-davinci/clock.h +0 -0

 #define CLK(dev, con, ck) 	\
 	{			\

(quoted diff) +0 -0

b/arch/arm/mach-davinci/da850.c index b90c172..4008fdc 100644

arch/arm/mach-davinci/da850.c +0 -0

@@ -76,6 +76,13 @@ static struct clk pll0_aux_clk = {
 	.flags		= CLK_PLL | PRE_PLL,
 };
 

arch/arm/mach-davinci/da850.c static struct clk pll0_aux_clk = { · line 84

Quoted 6 lines

arch/arm/mach-davinci/da850.c +6 -0

+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.

Quoted 56 lines

arch/arm/mach-davinci/da850.c +11 -0

+
 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),
 };
 

arch/arm/mach-davinci/devices-da8xx.c +4 -1

index 466b70c..ae54769 100644
@@ -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

arch/arm/mach-davinci/devices-da8xx.c

Quoted 4 lines

arch/arm/mach-davinci/devices-da8xx.c +4 -0

+static struct resource da8xx_rproc_resources[] = {
+	{ /* HOST1CFG syscfg offset - DSP boot address */
+		.start		= 0x44,
+		.end		= 0x44,

These should be absolute addresses, not relative.

arch/arm/mach-davinci/devices-da8xx.c

Quoted 12 lines

arch/arm/mach-davinci/devices-da8xx.c +12 -0

+		.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?

Quoted 10 lines

arch/arm/mach-davinci/devices-da8xx.c +9 -0

+};
+
+static struct platform_device da8xx_dsp = {
+	.name	= "davinci-rproc",
+	.id	= 0,
+	.dev	= {
+		.platform_data		= &rproc_pdata,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},

arch/arm/mach-davinci/devices-da8xx.c

Quoted 10 lines

arch/arm/mach-davinci/devices-da8xx.c +10 -0

+	.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.

arch/arm/mach-davinci/devices-da8xx.c

Quoted 12 lines

arch/arm/mach-davinci/devices-da8xx.c +11 -0

+
+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...

Quoted 30 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Quoted 4 lines · Robert Tivy

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.

Quoted 4 lines

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.

Quoted 19 lines

Quoted 11 lines

Signed-off-by Robert Tivy <[email protected]>

---
 arch/arm/mach-davinci/board-da850-evm.c        |   10 +++
 arch/arm/mach-davinci/board-omapl138-hawk.c    |   10 +++
 arch/arm/mach-davinci/clock.c                  |   22 +++++++
 arch/arm/mach-davinci/clock.h                  |    1 +
 arch/arm/mach-davinci/da850.c                  |   17 ++++++
 arch/arm/mach-davinci/devices-da8xx.c          |   78

+++++++++++++++++++++++-

Quoted 6 lines

 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 +++++++++++

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+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.

Quoted 11 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+#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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 18 lines

Review

Quoted 13 lines

(quoted diff) +11 -0

+
+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)?

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Hi Rob,

Quoted 24 lines · Robert

Hi Sekhar,

Please see comments and noob questions below...

Quoted 20 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
OMAP-L138 DSP

Quoted 4 lines · Robert Tivy

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

Quoted 16 lines

Quoted 15 lines

"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?

Quoted 1 line

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.

Hi Sekhar,

Quoted 54 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Hi Rob,

Quoted 24 lines · Robert

Hi Sekhar,

Please see comments and noob questions below...

Quoted 20 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
OMAP-L138 DSP

Quoted 4 lines · Robert Tivy

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

Quoted 17 lines

Quoted 11 lines

"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.

Quoted 4 lines

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

Quoted 6 lines

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?

Hi Rob,

Quoted 63 lines · Robert

Hi Sekhar,

Quoted 60 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Hi Rob,

Quoted 24 lines · Robert

Hi Sekhar,

Please see comments and noob questions below...

Quoted 20 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
OMAP-L138 DSP

Quoted 4 lines · Robert Tivy

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

Quoted 11 lines

Quoted 11 lines

"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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

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.

-----Original Message-----
From: Chemparathy, Cyril
Sent: Monday, December 03, 2012 12:13 PM
To: Nori, Sekhar
Cc: Tivy, Robert; davinci-linux-open-source at linux.davincidsp.com; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Quoted 29 lines · Sekhar Nori

Hi Rob,

Quoted 26 lines · Robert

Hi Sekhar,

Quoted 24 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
OMAP-L138 DSP

Hi Rob,

Quoted 11 lines · Robert

Hi Sekhar,

Please see comments and noob questions below...

Quoted 7 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Tuesday, November 20, 2012 4:27 AM
To: Tivy, Robert
Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm- kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board

support

Quoted 32 lines

Quoted 32 lines

Quoted 32 lines

Quoted 6 lines

Quoted 6 lines

for
OMAP-L138 DSP

Quoted 2 lines · Robert Tivy

Requires CMA services.

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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

Quoted 18 lines

Quoted 11 lines

"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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 7 lines · Sekhar Nori

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?

Quoted 15 lines

-----Original Message-----
From: davinci-linux-open-source-bounces at linux.davincidsp.com [mailto:davinci-linux-open-source-bounces at linux.davincidsp.com] On Behalf Of Karicheri, Muralidharan
Sent: Tuesday, December 04, 2012 8:13 AM
To: davinci-linux-open-source at linux.davincidsp.com
Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

Quoted 5 lines · Murali Karicheri

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 58 lines · Murali Karicheri

Quoted 57 lines · Sekhar Nori

Quoted 33 lines · Cyril Chemparathy

Quoted 7 lines · Sekhar Nori

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.

Quoted 4 lines

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 at linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Quoted 3 lines

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/

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, December 07, 2012 12:24 AM

Quoted 7 lines · Murali Karicheri

Quoted 3 lines · Murali Karicheri

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, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+
+static int __init early_rproc_mem(char *p) {
+	char *endp;
+
+	rproc_size = memparse(p, &endp);
+	if (*endp == '@')

Quoted 8 lines

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		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

Quoted 3 lines

Quoted 2 lines

(this means each and every kernel parameter needs to be justified well).

Quoted 1 line

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

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 5 lines

Quoted 1 line

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().

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 2 lines

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.

Quoted 32 lines · Robert

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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 at linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Hi Sekhar,

Quoted 83 lines · Sekhar Nori

Quoted 32 lines · Robert

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

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 ?

Quoted 91 lines · Prabhakar Lad

Hi Sekhar,

Quoted 88 lines · Sekhar Nori

Quoted 32 lines · Robert

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 96 lines · Sekhar Nori

Quoted 94 lines · Prabhakar Lad

Hi Sekhar,

Quoted 79 lines · Sekhar Nori

Quoted 32 lines · Robert

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel?

Quoted 9 lines

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.

Quoted 2 lines

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 ?

Quoted 3 lines

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.

From the chain of previous mails what I understand is you need to pass

Regards,
--Prabhakar Lad

Quoted 2 lines

Thanks,
Sekhar

Sekhar,

Quoted 97 lines · Prabhakar Lad

Quoted 96 lines · Sekhar Nori

Quoted 94 lines · Prabhakar Lad

Hi Sekhar,

Quoted 79 lines · Sekhar Nori

Quoted 32 lines · Robert

Quoted 32 lines

-----Original Message-----
From: Nori, Sekhar
Sent: Friday, November 30, 2012 2:51 AM

Hi Rob,

Quoted 5 lines · Robert

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

clear to me why this memory management needs to be dealt with in platform code.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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

Quoted 4 lines

Quoted 2 lines

(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)?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

Is there any other method that allows specifying a parameter for an early CMA reservation without having to rebuild the kernel?

Quoted 9 lines

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.

Quoted 2 lines

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 ?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

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.

-----Original Message-----
From: Nori, Sekhar
Sent: Wednesday, December 12, 2012 2:34 AM

Quoted 7 lines · Robert

Quoted 7 lines

-----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:

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 2 lines

The most basic solution is constants in the .c file itself. A

better

Quoted 4 lines

Quoted 4 lines

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

Quoted 2 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 7 lines

and probably not necessary.

Quoted 2 lines

Quoted 2 lines

If you want the remoteproc driver to allocate a certain area of

memory

Quoted 2 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 8 lines

not

Quoted 4 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+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-

Quoted 5 lines

Quoted 5 lines

parameters.txt

Quoted 4 lines

Quoted 2 lines

(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

Quoted 1 line

Quoted 1 line

specify non-kernel parameters? I guess my question is more one

about

Quoted 1 line

Quoted 1 line

the terminology "kernel parameter" - is there a way to specify a

module

Quoted 1 line

Quoted 1 line

parameter on the kernel command line (like, perhaps, rproc.membase

and

Quoted 3 lines

Quoted 3 lines

rproc.memsize)?

Right. Module parameters are passed from kernel command line when

the

Quoted 3 lines

Quoted 1 line

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?

Quoted 3 lines

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.

Thanks,
Sekhar

Signed-off-by Robert Tivy <[email protected]>

---
 drivers/remoteproc/Kconfig              |   41 +++++
 drivers/remoteproc/Makefile             |    1 +
 drivers/remoteproc/davinci_remoteproc.c |  292 +++++++++++++++++++++++++++++++