[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 at gmail.com> wrote:
>> On Wed, Dec 12, 2012 at 6:36 PM, Sekhar Nori <nsekhar at 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 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?
>>>
>> 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