[PATCH v3 1/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 1)

[PATCH v3 1/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 1)

[Sergei Shtylyov]

Hello.

On 14-11-2012 4:33, Robert Tivy wrote:

     These subjects are not very good too -- it's better to specify the scope 
of the changes, like "ARM: DaVinci: DA850 EVM: change pr_warning() to pr_warn()".

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

> Signed-off-by: Robert Tivy <rtivy@ti.com>
> ---
> 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(-)

> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index 32ee3f8..6c172b3 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -347,13 +347,13 @@ static inline void da850_evm_setup_nor_nand(void)
>   	if (!HAS_MMC) {
>   		ret = davinci_cfg_reg_list(da850_evm_nand_pins);
>   		if (ret)
> -			pr_warning("da850_evm_init: nand mux setup failed: "
> -					"%d\n", ret);
> +			pr_warn("%s: nand mux setup failed: %d\n",

    My preference is to have the acronyms capitalized, so I'd changed to 
"NAND", while at it.

> +				__func__, ret);
>
>   		ret = davinci_cfg_reg_list(da850_evm_nor_pins);
>   		if (ret)
> -			pr_warning("da850_evm_init: nor mux setup failed: %d\n",
> -				ret);
> +			pr_warn("%s: nor mux setup failed: %d\n",

    ... and to "NOR" here.

> @@ -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) {
> -		pr_warning("Could not register baseboard GPIO expander keys");
> +		pr_warn("Could not register baseboard GPIO expander keys");
>   		goto io_exp_setup_sw_fail;
>   	}
>
>   	da850_evm_bb_leds_init(gpio);
>   	ret = platform_device_register(&da850_evm_bb_leds_device);
>   	if (ret) {
> -		pr_warning("Could not register baseboard GPIO expander LEDS");
> +		pr_warn("Could not register baseboard GPIO expander LEDS");

    It's "LEDs".

> @@ -1060,21 +1060,19 @@ static int __init da850_evm_config_emac(void)
>   	}
>
>   	if (ret)
> -		pr_warning("da850_evm_init: cpgmac/rmii mux setup failed: %d\n",
> -				ret);
> +		pr_warn("%s: cpgmac/rmii mux setup failed: %d\n",

    I'd have changed to "CPGMAC/RMII".

> @@ -1085,8 +1083,7 @@ static int __init da850_evm_config_emac(void)
>
>   	ret = da8xx_register_emac();
>   	if (ret)
> -		pr_warning("da850_evm_init: emac registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: emac registration failed: %d\n", __func__, ret);

    ... and to "EMAC" here.

> @@ -1438,57 +1435,53 @@ static __init void da850_evm_init(void)
>
>   	ret = pmic_tps65070_init();
>   	if (ret)
> -		pr_warning("da850_evm_init: TPS65070 PMIC init failed: %d\n",
> -				ret);
> +		pr_warn("%s: TPS65070 PMIC init failed: %d\n", __func__, ret);
>
>   	ret = da850_register_edma(da850_edma_rsv);
>   	if (ret)
> -		pr_warning("da850_evm_init: edma registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: edma registration failed: %d\n", __func__, ret);

    ... and to "EDMA" here.

>   	ret = davinci_cfg_reg_list(da850_i2c0_pins);
>   	if (ret)
> -		pr_warning("da850_evm_init: i2c0 mux setup failed: %d\n",
> -				ret);
> +		pr_warn("%s: i2c0 mux setup failed: %d\n", __func__, ret);

    ... and to "I2C0" here.

>
>   	ret = da8xx_register_i2c(0, &da850_evm_i2c_0_pdata);
>   	if (ret)
> -		pr_warning("da850_evm_init: i2c0 registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: i2c0 registration failed: %d\n", __func__, ret);

    ... and here.

>   	if (HAS_MMC) {
>   		ret = davinci_cfg_reg_list(da850_evm_mmcsd0_pins);
>   		if (ret)
> -			pr_warning("da850_evm_init: mmcsd0 mux setup failed:"
> -					" %d\n", ret);
> +			pr_warn("%s: mmcsd0 mux setup failed: %d\n",

    ... and to "MMCSD0" here.

[...]
>   		if (ret)
> -			pr_warning("da850_evm_init: mmcsd0 registration failed:"
> -					" %d\n", ret);
> +			pr_warn("%s: mmcsd0 registration failed: %d\n",

    ... and here.

> +				__func__, ret);
>
>   		ret = da850_wl12xx_init();
>   		if (ret)
> -			pr_warning("da850_evm_init: wl12xx initialization"
> -				   " failed: %d\n", ret);
> +			pr_warn("%s: wl12xx initialization failed: %d\n",

    ... and to "WL12xx" here.

> @@ -1506,64 +1499,55 @@ static __init void da850_evm_init(void)
>
>   	ret = davinci_cfg_reg_list(da850_evm_mcasp_pins);
>   	if (ret)
> -		pr_warning("da850_evm_init: mcasp mux setup failed: %d\n",
> -				ret);
> +		pr_warn("%s: mcasp mux setup failed: %d\n", __func__, ret);

    To "McASP" here.

>
>   	da8xx_register_mcasp(0, &da850_evm_snd_data);
>
>   	ret = davinci_cfg_reg_list(da850_lcdcntl_pins);
>   	if (ret)
> -		pr_warning("da850_evm_init: lcdcntl mux setup failed: %d\n",
> -				ret);
> +		pr_warn("%s: lcdcntl mux setup failed: %d\n", __func__, ret);

    To "LCDC" here.
>
>   	/* Handle board specific muxing for LCD here */
>   	ret = davinci_cfg_reg_list(da850_evm_lcdc_pins);
>   	if (ret)
> -		pr_warning("da850_evm_init: evm specific lcd mux setup "
> -				"failed: %d\n",	ret);
> +		pr_warn("%s: evm specific lcd mux setup failed: %d\n",

    "EVM" and "LCD" here.

> +			__func__, ret);
>
>   	ret = da850_lcd_hw_init();
>   	if (ret)
> -		pr_warning("da850_evm_init: lcd initialization failed: %d\n",
> -				ret);
> +		pr_warn("%s: lcd initialization failed: %d\n", __func__, ret);

    "LCD" here.

>
>   	sharp_lk043t1dg01_pdata.panel_power_ctrl = da850_panel_power_ctrl,
>   	ret = da8xx_register_lcdc(&sharp_lk043t1dg01_pdata);
>   	if (ret)
> -		pr_warning("da850_evm_init: lcdc registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: lcdc registration failed: %d\n", __func__, ret);

    "LCDC" here.

>
>   	ret = da8xx_register_rtc();
>   	if (ret)
> -		pr_warning("da850_evm_init: rtc setup failed: %d\n", ret);
> +		pr_warn("%s: rtc setup failed: %d\n", __func__, ret);

    "RTC" here.

>
>   	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);
> +		pr_warn("%s: suspend registration failed: %d\n", __func__, ret);
>
>   	da850_vpif_init();
>
>   	ret = da8xx_register_spi(1, da850evm_spi_info,
>   				 ARRAY_SIZE(da850evm_spi_info));
>   	if (ret)
> -		pr_warning("da850_evm_init: spi 1 registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: spi 1 registration failed: %d\n", __func__, ret);

    "SPI" here.

>
>   	ret = da850_register_sata(DA850EVM_SATA_REFCLKPN_RATE);
>   	if (ret)
> -		pr_warning("da850_evm_init: sata registration failed: %d\n",
> -				ret);
> +		pr_warn("%s: sata registration failed: %d\n", __func__, ret);

    "SATA" here.

WBR, Sergei

[PATCH v3 2/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 2)

[Sergei Shtylyov]

Hello.

On 14-11-2012 4:33, Robert Tivy wrote:

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

> Signed-off-by: Robert Tivy <rtivy@ti.com>
> ---
> 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(-)

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

> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
> index dc1208e..8aea169 100644
> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> @@ -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.

>   		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

[PATCH v3 2/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 2)

[Tivy, Robert]

Hi Sergei,

Thanks for your feedback, please see below...

> -----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.
> 
> On 14-11-2012 4:33, Robert Tivy wrote:
> 
> > Also, while modifying those pr_warning() calls I changed hardcoded
> > function names to use '"%s:", __func__' instead
> 
> > Signed-off-by: Robert Tivy <rtivy@ti.com>
> > ---
> > 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(-)
> 
>     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...

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.

> 
> > diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
> > b/arch/arm/mach-davinci/board-omapl138-hawk.c
> > index dc1208e..8aea169 100644
> > --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> > +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> > @@ -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.

I didn't originate those acronyms so I'm not inclined to change them.

Regards,

- Rob

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

[PATCH v3 2/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 2)

[Sergei Shtylyov]

Hello.

On 15-11-2012 5:53, Tivy, Robert wrote:

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

>>> Signed-off-by: Robert Tivy <rtivy@ti.com>
>>> ---
>>> 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(-)

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

    I meant "talking". :-/

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

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

WBR, Sergei

[PATCH v3 2/6] ARM: davinci: Changed pr_warning() to pr_warn() (part 2)

[Sekhar Nori]

On 11/15/2012 3:16 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 15-11-2012 5:53, Tivy, Robert wrote:
> 
>>>> Also, while modifying those pr_warning() calls I changed hardcoded
>>>> function names to use '"%s:", __func__' instead
> 
>>>> Signed-off-by: Robert Tivy <rtivy@ti.com>
>>>> ---
>>>> 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(-)
> 
>>>      Taksing of separation of board and SoC specific changes, this patch
> 
>    I meant "talking". :-/
> 
>>> 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.
> 
>    To quote Sekhar: "My main motivation was to keep board and soc file
> changes from mixing together." You just went too far.

Since this has already gone back and forth a couple of times (probably
due to me not being entirely clear on what I want), lets proceed this way:

Merge patches 1/6 and 2/6 into a patch titled:

ARM: davinci: da850 board: change pr_warning() to pr_warn()

Re-headline 3/6 as:

ARM: davinci: devices-da8xx.c: change pr_warning() to pr_warn()

Re-headline 4/6 as:

ARM: davinci: psc.c: change pr_warning() to pr_warn()

I hope that's acceptable to everyone involved.

Thanks,
Sekhar

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

Hi Sekhar,

Please see comments and noob questions below...

> -----Original Message-----
> From: Nori, Sekhar
> Sent: Tuesday, November 20, 2012 4:27 AM
> To: Tivy, Robert
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
> OMAP-L138 DSP
> 
> On 11/14/2012 6:03 AM, Robert Tivy wrote:
> > Requires CMA services.
> >
> > New 'clk_reset()' API added so that the DSP's reset state can be
> > toggled separately from its enable/disable operation.
> 
> But we cannot add a private clk_ API without it being defined in
> include/linux/clk.h. So, this requires wider alignment.
> 
> And I am not sure clock API should be extended to support reset since
> "resetting a clock" does not make a lot of sense. On DaVinci, clock
> gating and reset are handled by the same module, but that need not
> always be the case.
> 
> What you need is a way to reset an IP. I do not know of an existing
> framework to do this; likely a new API needs to be created. I am
> guessing this never existed so far because most of the IPs can be reset
> internally (by writing a bit within its own register space). I guess
> DSP is different since its actually a co-processor and may not have
> such a bit.
> 
> How about simulating a reset by making the DSP jump to its reset
> address. While I am sure its not quite the same as resetting the DSP
> using PSC, may be it could be used while you wait for consensus around
> handling module reset in kernel?

Since the ARM can't write the DSP's program counter I suspect such a temporary solution is not possible.

> 
> >
> > Signed-off-by: Robert Tivy <rtivy@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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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 at linux.davincidsp.com; linux-arm-
>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support for
>> OMAP-L138 DSP
>>
>> On 11/14/2012 6:03 AM, Robert Tivy wrote:
>>> Requires CMA services.
>>>
>>> New 'clk_reset()' API added so that the DSP's reset state can be
>>> toggled separately from its enable/disable operation.
>>
>> But we cannot add a private clk_ API without it being defined in
>> include/linux/clk.h. So, this requires wider alignment.
>>
>> And I am not sure clock API should be extended to support reset since
>> "resetting a clock" does not make a lot of sense. On DaVinci, clock
>> gating and reset are handled by the same module, but that need not
>> always be the case.
>>
>> What you need is a way to reset an IP. I do not know of an existing
>> framework to do this; likely a new API needs to be created. I am
>> guessing this never existed so far because most of the IPs can be reset
>> internally (by writing a bit within its own register space). I guess
>> DSP is different since its actually a co-processor and may not have
>> such a bit.
>>
>> How about simulating a reset by making the DSP jump to its reset
>> address. While I am sure its not quite the same as resetting the DSP
>> using PSC, may be it could be used while you wait for consensus around
>> handling module reset in kernel?
> 
> Since the ARM can't write the DSP's program counter I suspect such a temporary solution is not possible.

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

Hi Sekhar,

> -----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,
> 
> 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 at linux.davincidsp.com; linux-arm-
> >> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> >> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support
> >> for
> >> OMAP-L138 DSP
> >>
> >> On 11/14/2012 6:03 AM, Robert Tivy wrote:
> >>> Requires CMA services.
> >>>
> >>> New 'clk_reset()' API added so that the DSP's reset state can be
> >>> toggled separately from its enable/disable operation.
> >>
> >> But we cannot add a private clk_ API without it being defined in
> >> include/linux/clk.h. So, this requires wider alignment.
> >>
> >> And I am not sure clock API should be extended to support reset
> since
> >> "resetting a clock" does not make a lot of sense. On DaVinci, clock
> >> gating and reset are handled by the same module, but that need not
> >> always be the case.
> >>
> >> What you need is a way to reset an IP. I do not know of an existing
> >> framework to do this; likely a new API needs to be created. I am
> >> guessing this never existed so far because most of the IPs can be
> >> reset internally (by writing a bit within its own register space). I
> >> guess DSP is different since its actually a co-processor and may not
> >> have such a bit.
> >>
> >> How about simulating a reset by making the DSP jump to its reset
> >> address. While I am sure its not quite the same as resetting the DSP
> >> using PSC, may be it could be used while you wait for consensus
> >> around handling module reset in kernel?
> >
> > Since the ARM can't write the DSP's program counter I suspect such a
> temporary solution is not possible.
> 
> 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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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 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,
>>
>> 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 at linux.davincidsp.com; linux-arm-
>>>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
>>>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support
>>>> for
>>>> OMAP-L138 DSP
>>>>
>>>> On 11/14/2012 6:03 AM, Robert Tivy wrote:
>>>>> Requires CMA services.
>>>>>
>>>>> New 'clk_reset()' API added so that the DSP's reset state can be
>>>>> toggled separately from its enable/disable operation.
>>>>
>>>> But we cannot add a private clk_ API without it being defined in
>>>> include/linux/clk.h. So, this requires wider alignment.
>>>>
>>>> And I am not sure clock API should be extended to support reset
>> since
>>>> "resetting a clock" does not make a lot of sense. On DaVinci, clock
>>>> gating and reset are handled by the same module, but that need not
>>>> always be the case.
>>>>
>>>> What you need is a way to reset an IP. I do not know of an existing
>>>> framework to do this; likely a new API needs to be created. I am
>>>> guessing this never existed so far because most of the IPs can be
>>>> reset internally (by writing a bit within its own register space). I
>>>> guess DSP is different since its actually a co-processor and may not
>>>> have such a bit.
>>>>
>>>> How about simulating a reset by making the DSP jump to its reset
>>>> address. While I am sure its not quite the same as resetting the DSP
>>>> using PSC, may be it could be used while you wait for consensus
>>>> around handling module reset in kernel?
>>>
>>> Since the ARM can't write the DSP's program counter I suspect such a
>> temporary solution is not possible.
>>
>> 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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Cyril Chemparathy]

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 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,
>>>
>>> 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 at linux.davincidsp.com; linux-arm-
>>>>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
>>>>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board support
>>>>> for
>>>>> OMAP-L138 DSP
>>>>>
>>>>> On 11/14/2012 6:03 AM, Robert Tivy wrote:
>>>>>> Requires CMA services.
>>>>>>
>>>>>> New 'clk_reset()' API added so that the DSP's reset state can be
>>>>>> toggled separately from its enable/disable operation.
>>>>>
>>>>> But we cannot add a private clk_ API without it being defined in
>>>>> include/linux/clk.h. So, this requires wider alignment.
>>>>>
>>>>> And I am not sure clock API should be extended to support reset
>>> since
>>>>> "resetting a clock" does not make a lot of sense. On DaVinci, clock
>>>>> gating and reset are handled by the same module, but that need not
>>>>> always be the case.
>>>>>
>>>>> What you need is a way to reset an IP. I do not know of an existing
>>>>> framework to do this; likely a new API needs to be created. I am
>>>>> guessing this never existed so far because most of the IPs can be
>>>>> reset internally (by writing a bit within its own register space). I
>>>>> guess DSP is different since its actually a co-processor and may not
>>>>> have such a bit.
>>>>>
>>>>> How about simulating a reset by making the DSP jump to its reset
>>>>> address. While I am sure its not quite the same as resetting the DSP
>>>>> using PSC, may be it could be used while you wait for consensus
>>>>> around handling module reset in kernel?
>>>>
>>>> Since the ARM can't write the DSP's program counter I suspect such a
>>> temporary solution is not possible.
>>>
>>> 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.

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

> -----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
> 
> 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 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,
> >>>
> >>> 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 at linux.davincidsp.com; linux-arm-
> >>>>> kernel at lists.infradead.org; Ring, Chris; Grosen, Mark
> >>>>> Subject: Re: [PATCH v3 5/6] ARM: davinci: remoteproc board
> support
> >>>>> for
> >>>>> OMAP-L138 DSP
> >>>>>
> >>>>> On 11/14/2012 6:03 AM, Robert Tivy wrote:
> >>>>>> Requires CMA services.
> >>>>>>
> >>>>>> New 'clk_reset()' API added so that the DSP's reset state can be
> >>>>>> toggled separately from its enable/disable operation.
> >>>>>
> >>>>> But we cannot add a private clk_ API without it being defined in
> >>>>> include/linux/clk.h. So, this requires wider alignment.
> >>>>>
> >>>>> And I am not sure clock API should be extended to support reset
> >>> since
> >>>>> "resetting a clock" does not make a lot of sense. On DaVinci,
> >>>>> clock gating and reset are handled by the same module, but that
> >>>>> need not always be the case.
> >>>>>
> >>>>> What you need is a way to reset an IP. I do not know of an
> >>>>> existing framework to do this; likely a new API needs to be
> >>>>> created. I am guessing this never existed so far because most of
> >>>>> the IPs can be reset internally (by writing a bit within its own
> >>>>> register space). I guess DSP is different since its actually a
> >>>>> co-processor and may not have such a bit.
> >>>>>
> >>>>> How about simulating a reset by making the DSP jump to its reset
> >>>>> address. While I am sure its not quite the same as resetting the
> >>>>> DSP using PSC, may be it could be used while you wait for
> >>>>> consensus around handling module reset in kernel?
> >>>>
> >>>> Since the ARM can't write the DSP's program counter I suspect such
> >>>> a
> >>> temporary solution is not possible.
> >>>
> >>> 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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

adding back the ARM list.

On 12/5/2012 7:53 AM, Tivy, Robert wrote:
>> -----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
>>
>> 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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Prabhakar Lad]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Prabhakar Lad]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Prabhakar Lad]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Sekhar Nori]

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

[PATCH v3 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

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

[PATCH v3 6/6] ARM: davinci: remoteproc driver support for OMAP-L138 DSP

[Sekhar Nori]

On 11/14/2012 6:03 AM, Robert Tivy wrote:
> Signed-off-by: Robert Tivy <rtivy@ti.com>
> 
> ---
>  drivers/remoteproc/Kconfig              |   41 +++++
>  drivers/remoteproc/Makefile             |    1 +
>  drivers/remoteproc/davinci_remoteproc.c |  292 +++++++++++++++++++++++++++++++

scripts/get_maintainer.pl tells me that Ohad Ben-Cohen needs to take
this driver or at least say he is happy with it. Please CC him and LKML
on your next post.

Thanks,
Sekhar