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