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

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

[Sergei Shtylyov]

Hello.

    It's not a good idea to send multiple patches with the same subject. 
Actually, in this case it's worth merging all 4 patches into one.

On 26-10-2012 0:35, 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.5 tag (commit 28a33cbc24e4256c143dce96c7d93bf423229f92) of
> Linus' mainline kernel at git.kernel.org.

    3.5 is too old. Why not to 3.6 or even 3.7-rc2?

> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index 0149fb4..bbb3c73 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
[...]
> @@ -1046,21 +1046,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",
> +			__func__, ret);
>
>  	/* configure the CFGCHIP3 register for RMII or MII */
>  	__raw_writel(val, cfg_chip3_base);
>
>  	ret = davinci_cfg_reg(DA850_GPIO2_6);
>  	if (ret)
> -		pr_warning("da850_evm_init:GPIO(2,6) mux setup "
> -							"failed\n");
> +		pr_warn("%s:GPIO(2,6) mux setup failed\n", __func__);

    Worth inserting space after colon here.

WBR, Sergei

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

[Tivy, Robert]

Thanks for your comments, Sergei.  Please see below...

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: Friday, October 26, 2012 2:46 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 v2 1/6] ARM: davinci: Changed pr_warning() to
> pr_warn()
> 
> Hello.
> 
>     It's not a good idea to send multiple patches with the same
> subject.
> Actually, in this case it's worth merging all 4 patches into one.

My first patch submission had them all as one patch, but Sekhar asked that they be split into 4 separate patches to make the merge easier.

I can make each one have a different subject, though.

> 
> On 26-10-2012 0:35, 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.5 tag (commit 28a33cbc24e4256c143dce96c7d93bf423229f92)
> of
> > Linus' mainline kernel at git.kernel.org.
> 
>     3.5 is too old. Why not to 3.6 or even 3.7-rc2?

I will attempt to recreate this patch series on 3.7-rc2, although I will need to change it to use the newer "rproc" APIs and data structures.

> 
> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-
> davinci/board-da850-evm.c
> > index 0149fb4..bbb3c73 100644
> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> [...]
> > @@ -1046,21 +1046,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",
> > +			__func__, ret);
> >
> >  	/* configure the CFGCHIP3 register for RMII or MII */
> >  	__raw_writel(val, cfg_chip3_base);
> >
> >  	ret = davinci_cfg_reg(DA850_GPIO2_6);
> >  	if (ret)
> > -		pr_warning("da850_evm_init:GPIO(2,6) mux setup "
> > -							"failed\n");
> > +		pr_warn("%s:GPIO(2,6) mux setup failed\n", __func__);
> 
>     Worth inserting space after colon here.

Will do.

> 
> WBR, Sergei

Thanks & Regards,

- Rob

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

[Sekhar Nori]

On 10/27/2012 6:29 AM, Tivy, Robert wrote:
> Thanks for your comments, Sergei.  Please see below...
> 
>> -----Original Message-----
>> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
>> Sent: Friday, October 26, 2012 2:46 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 v2 1/6] ARM: davinci: Changed pr_warning() to
>> pr_warn()
>>
>> Hello.
>>
>>     It's not a good idea to send multiple patches with the same
>> subject.
>> Actually, in this case it's worth merging all 4 patches into one.
> 
> My first patch submission had them all as one patch, but Sekhar asked that they be split into 4 separate patches to make the merge easier.
> 
> I can make each one have a different subject, though.

Yes, it was I who asked for the patch to be separated out. My main
motivation was to keep board and soc file changes from mixing together.
The subject line for each patch should be different though.

> 
>>
>> On 26-10-2012 0:35, 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.5 tag (commit 28a33cbc24e4256c143dce96c7d93bf423229f92)
>> of
>>> Linus' mainline kernel at git.kernel.org.
>>
>>     3.5 is too old. Why not to 3.6 or even 3.7-rc2?
> 
> I will attempt to recreate this patch series on 3.7-rc2, although I will need to change it to use the newer "rproc" APIs and data structures.

All the more reason to rebase to v3.7-rc2!

Thanks,
Sekhar

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

[Ben Gardiner]

Hi Rob,

On Thu, Oct 25, 2012 at 4:35 PM, Robert Tivy <rtivy@ti.com> wrote:
> [...]
> @@ -660,6 +667,99 @@ int __init da850_register_mmcsd1(struct davinci_mmc_config *config)
>  }
>  #endif
>
> +static struct platform_device *da8xx_dsp;
> +
> +#ifdef CONFIG_CMA
> +
> +/*
> + * The following address range was chosen because the XDC Platform for
> + * OMAP-L138 DSP has this range as its default code/data placement.
> + */
> +#define DA8XX_RPROC_CMA_BASE  (0xc3000000)
> [...]
> +/*
> + * The following address range was chosen because the XDC Platform for
> + * OMAP-L138 DSP has this range as its default code/data placement.
> + *
> + * System integrators must ensure that Linux does not own this range.
> + */
> +#define DA_CONTIG_BASE (0xc3000000)
> +#define DA_CONTIG_SIZE (0x02000000)

I am concerned with the rigidity of the memory hole as its definition
is currently proposed.

As you noted DA_CONTIG_BASE and DA_CONTIG_SIZE must describe a range
that is not used by Linux. Ideally this hole would not be in the
middle of the usuable memory but instead at the top. For L138 boards
with larger DDR packages this would mean carrying a patch to this
file.

I think the same also applies to DA8XX_RPROC_CMA_BASE but I have no
hands-on experience yet with CMA.

Is there any other means by which the hole's location and size can be
specified which does not require patching this file? I imagine KConfig
would work, but is this an acceptable use of KConfig?

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

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

[Prabhakar Lad]

Hi Robert,

On Thu, Nov 1, 2012 at 7:22 PM, Ben Gardiner <bengardiner@nanometrics.ca> wrote:
> Hi Rob,
>
> On Thu, Oct 25, 2012 at 4:35 PM, Robert Tivy <rtivy@ti.com> wrote:
>> [...]
>> @@ -660,6 +667,99 @@ int __init da850_register_mmcsd1(struct davinci_mmc_config *config)
>>  }
>>  #endif
>>
>> +static struct platform_device *da8xx_dsp;
>> +
>> +#ifdef CONFIG_CMA
>> +
>> +/*
>> + * The following address range was chosen because the XDC Platform for
>> + * OMAP-L138 DSP has this range as its default code/data placement.
>> + */
>> +#define DA8XX_RPROC_CMA_BASE  (0xc3000000)
>> [...]
>> +/*
>> + * The following address range was chosen because the XDC Platform for
>> + * OMAP-L138 DSP has this range as its default code/data placement.
>> + *
>> + * System integrators must ensure that Linux does not own this range.
>> + */
>> +#define DA_CONTIG_BASE (0xc3000000)
>> +#define DA_CONTIG_SIZE (0x02000000)
>
Is there a specific requirement for the dsp dev to have the buffers from
this '0xc3000000' address range only ? If yes then dma_declare_contiguous()
is must to ensure contiguous memory so the above macros cant be avoided.
If there isn?t a requirement of a specific region for dsp device you can use a
global CMA instead, so as to ensure you have contiguous memory.

Regards,
--Prabhakar Lad

> I am concerned with the rigidity of the memory hole as its definition
> is currently proposed.
>
> As you noted DA_CONTIG_BASE and DA_CONTIG_SIZE must describe a range
> that is not used by Linux. Ideally this hole would not be in the
> middle of the usuable memory but instead at the top. For L138 boards
> with larger DDR packages this would mean carrying a patch to this
> file.
>
> I think the same also applies to DA8XX_RPROC_CMA_BASE but I have no
> hands-on experience yet with CMA.
>
> Is there any other means by which the hole's location and size can be
> specified which does not require patching this file? I imagine KConfig
> would work, but is this an acceptable use of KConfig?
>
> Best Regards,
> Ben Gardiner
>
> ---
> Nanometrics Inc.
> http://www.nanometrics.ca
> _______________________________________________
> 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 v2 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Tivy, Robert]

Hi Prabhakar,

Thanks for your consideration, please see my response below...

> -----Original Message-----
> From: Prabhakar Lad [mailto:prabhakar.csengg at gmail.com]
> Sent: Monday, November 05, 2012 9:02 PM
> To: Tivy, Robert; Ben Gardiner
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
> kernel at lists.infradead.org; Marek Szyprowski
> Subject: Re: [PATCH v2 5/6] ARM: davinci: remoteproc board support for
> OMAP-L138 DSP
> 
> Hi Robert,
> 
> On Thu, Nov 1, 2012 at 7:22 PM, Ben Gardiner
> <bengardiner@nanometrics.ca> wrote:
> > Hi Rob,
> >
> > On Thu, Oct 25, 2012 at 4:35 PM, Robert Tivy <rtivy@ti.com> wrote:
> >> [...]
> >> @@ -660,6 +667,99 @@ int __init da850_register_mmcsd1(struct
> davinci_mmc_config *config)
> >>  }
> >>  #endif
> >>
> >> +static struct platform_device *da8xx_dsp;
> >> +
> >> +#ifdef CONFIG_CMA
> >> +
> >> +/*
> >> + * The following address range was chosen because the XDC Platform
> for
> >> + * OMAP-L138 DSP has this range as its default code/data placement.
> >> + */
> >> +#define DA8XX_RPROC_CMA_BASE  (0xc3000000)
> >> [...]
> >> +/*
> >> + * The following address range was chosen because the XDC Platform
> for
> >> + * OMAP-L138 DSP has this range as its default code/data placement.
> >> + *
> >> + * System integrators must ensure that Linux does not own this
> range.
> >> + */
> >> +#define DA_CONTIG_BASE (0xc3000000)
> >> +#define DA_CONTIG_SIZE (0x02000000)
> >
> Is there a specific requirement for the dsp dev to have the buffers
> from
> this '0xc3000000' address range only ? If yes then
> dma_declare_contiguous()
> is must to ensure contiguous memory so the above macros cant be
> avoided.
> If there isn't a requirement of a specific region for dsp device you
> can use a
> global CMA instead, so as to ensure you have contiguous memory.

The requirement is that the contiguous buffer matches the address range to which the DSP image was linked, including uninitialized sections that don't actually get loaded.

I was thinking of making those #defines into Kconfig variables, so that kernel sources don't need to be touched by the end customer.  Another alternative is to make them be kernel command line variables, which would prevent the need to even rebuild the kernel.  What are your thoughts regarding those alternatives?

The above address range was decided upon because it matches the RTSC platform's defined area for OMAP-L138 [1] (although, the customer can end up redefining that range).  One of the reasons that range was chosen for the RTSC platform is that it exists on boards with smaller DDR sizes.  In other words, for a board with 256MB of DDR, if a range at the top of the DDR address space was chosen then that DSP image wouldn't work with a board with only 128MB.

Regards,

- Rob

[1] http://rtsc.eclipse.org/cdoc-tip/index.html#ti/platforms/evmOMAPL138/Platform.html#per-instance_config_parameters, however, we extended the range an additional 16MB beyond the RTSC platform's range.

> 
> Regards,
> --Prabhakar Lad
> 
> > I am concerned with the rigidity of the memory hole as its definition
> > is currently proposed.
> >
> > As you noted DA_CONTIG_BASE and DA_CONTIG_SIZE must describe a range
> > that is not used by Linux. Ideally this hole would not be in the
> > middle of the usuable memory but instead at the top. For L138 boards
> > with larger DDR packages this would mean carrying a patch to this
> > file.
> >
> > I think the same also applies to DA8XX_RPROC_CMA_BASE but I have no
> > hands-on experience yet with CMA.
> >
> > Is there any other means by which the hole's location and size can be
> > specified which does not require patching this file? I imagine
> KConfig
> > would work, but is this an acceptable use of KConfig?
> >
> > Best Regards,
> > Ben Gardiner
> >
> > ---
> > Nanometrics Inc.
> > http://www.nanometrics.ca
> > _______________________________________________
> > 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 v2 5/6] ARM: davinci: remoteproc board support for OMAP-L138 DSP

[Prabhakar Lad]

Hi Robert,

On Wed, Nov 7, 2012 at 1:03 AM, Tivy, Robert <rtivy@ti.com> wrote:
> Hi Prabhakar,
>
> Thanks for your consideration, please see my response below...
>
>> -----Original Message-----
>> From: Prabhakar Lad [mailto:prabhakar.csengg at gmail.com]
>> Sent: Monday, November 05, 2012 9:02 PM
>> To: Tivy, Robert; Ben Gardiner
>> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-arm-
>> kernel at lists.infradead.org; Marek Szyprowski
>> Subject: Re: [PATCH v2 5/6] ARM: davinci: remoteproc board support for
>> OMAP-L138 DSP
>>
>> Hi Robert,
>>
>> On Thu, Nov 1, 2012 at 7:22 PM, Ben Gardiner
>> <bengardiner@nanometrics.ca> wrote:
>> > Hi Rob,
>> >
>> > On Thu, Oct 25, 2012 at 4:35 PM, Robert Tivy <rtivy@ti.com> wrote:
>> >> [...]
>> >> @@ -660,6 +667,99 @@ int __init da850_register_mmcsd1(struct
>> davinci_mmc_config *config)
>> >>  }
>> >>  #endif
>> >>
>> >> +static struct platform_device *da8xx_dsp;
>> >> +
>> >> +#ifdef CONFIG_CMA
>> >> +
>> >> +/*
>> >> + * The following address range was chosen because the XDC Platform
>> for
>> >> + * OMAP-L138 DSP has this range as its default code/data placement.
>> >> + */
>> >> +#define DA8XX_RPROC_CMA_BASE  (0xc3000000)
>> >> [...]
>> >> +/*
>> >> + * The following address range was chosen because the XDC Platform
>> for
>> >> + * OMAP-L138 DSP has this range as its default code/data placement.
>> >> + *
>> >> + * System integrators must ensure that Linux does not own this
>> range.
>> >> + */
>> >> +#define DA_CONTIG_BASE (0xc3000000)
>> >> +#define DA_CONTIG_SIZE (0x02000000)
>> >
>> Is there a specific requirement for the dsp dev to have the buffers
>> from
>> this '0xc3000000' address range only ? If yes then
>> dma_declare_contiguous()
>> is must to ensure contiguous memory so the above macros cant be
>> avoided.
>> If there isn't a requirement of a specific region for dsp device you
>> can use a
>> global CMA instead, so as to ensure you have contiguous memory.
>
> The requirement is that the contiguous buffer matches the address range to which the DSP image was linked, including uninitialized sections that don't actually get loaded.
>
> I was thinking of making those #defines into Kconfig variables, so that kernel sources don't need to be touched by the end customer.  Another alternative is to make them be kernel command line variables, which would prevent the need to even rebuild the kernel.  What are your thoughts regarding those alternatives?

In either cases you still need a value which defaults, if the range was not
specified in command line or Kconfig value wasn?t set. In this case
you still need a macro with default value.

I would prefer passing the range as part of command line as it would
avoid rebuilding.

Regards,
--Prabhakar Lad

>
> The above address range was decided upon because it matches the RTSC platform's defined area for OMAP-L138 [1] (although, the customer can end up redefining that range).  One of the reasons that range was chosen for the RTSC platform is that it exists on boards with smaller DDR sizes.  In other words, for a board with 256MB of DDR, if a range at the top of the DDR address space was chosen then that DSP image wouldn't work with a board with only 128MB.
>
> Regards,
>
> - Rob
>
> [1] http://rtsc.eclipse.org/cdoc-tip/index.html#ti/platforms/evmOMAPL138/Platform.html#per-instance_config_parameters, however, we extended the range an additional 16MB beyond the RTSC platform's range.
>
>>
>> Regards,
>> --Prabhakar Lad
>>
>> > I am concerned with the rigidity of the memory hole as its definition
>> > is currently proposed.
>> >
>> > As you noted DA_CONTIG_BASE and DA_CONTIG_SIZE must describe a range
>> > that is not used by Linux. Ideally this hole would not be in the
>> > middle of the usuable memory but instead at the top. For L138 boards
>> > with larger DDR packages this would mean carrying a patch to this
>> > file.
>> >
>> > I think the same also applies to DA8XX_RPROC_CMA_BASE but I have no
>> > hands-on experience yet with CMA.
>> >
>> > Is there any other means by which the hole's location and size can be
>> > specified which does not require patching this file? I imagine
>> KConfig
>> > would work, but is this an acceptable use of KConfig?
>> >
>> > Best Regards,
>> > Ben Gardiner
>> >
>> > ---
>> > Nanometrics Inc.
>> > http://www.nanometrics.ca
>> > _______________________________________________
>> > Davinci-linux-open-source mailing list
>> > Davinci-linux-open-source at linux.davincidsp.com
>> > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-
>> source