Re: [RFCv2] remoteproc: Add STE modem driver for remoteproc

[Ohad Ben-Cohen]

Hi Sjur,

On Wed, Sep 19, 2012 at 1:08 PM, Sjur BRENDELAND
<sjur.brandeland@xxxxxxxxxxxxxx> wrote:
>> >  include/linux/modem_shm/ste_modem.h  |   71 ++++++
>>
>> Why did you decide to create a separate folder for this header ? if
>> it's STE specific, maybe use an 'ste' prefix for it too ?
>
> There has been some attempt to upstream a shm driver for another modem
> vendor as well, see https://lkml.org/lkml/2012/8/27/15.
>
> This driver used  .../driver/modem_shm, and
> .../include/linux/modem_shm/. I feel that this driver belongs in the same
> family. This other driver did not include any vendor prefix though.
>
> What about .../include/linux/modem_shm/ste/modem.h or maybe just
> .../include/linux/modem_shm/modem.h?

Do your driver and Arun's share any code ? More importantly - is there
any common functionality to consolidate ?

If it's just the name "modem_shm" that is common, I'm not sure there's
merit in having a common folder ?

But if you do, why not use ste_modem_shm ? It looks like Arun comes
from STE too :)

>> > +       /*
>> > +        * STE-modem requires the firmware to be located
>> > +        * at the start of the shared memory region. So we need to
>> > +        * reserve space for firmware at the start.
>> > +        * This cannot be done in the function sproc_load_segments because
>> > +        * then dma_alloc_coherent is already called by Core and the
>> > +        * start of the share memory area would already have been occupied.
>> > +        */
>> > +       if (!sproc->fw_addr) {
>> > +               sproc->fw_addr = dma_alloc_coherent(rproc->dev.parent, fw-
>> >size,
>>
>> This doesn't look good: this function should just find an offset
>> within the firmware and return it, and not do any memory allocations.
>>
>> I understand the reason why you do that, ...
>
> I am afraid I *must* put the TOC at the start of memory. There is no way
> around this.

Sure that's fine. Let's just do it the right way :)

> But I can pre-allocate space for firmware and just bail out if
> it is not enough room. This is a much simpler approach.
>
>> but I think we had a nice
>> generic solution which shouldn't be too hard to implement (i.e. let
>> remoteproc maintain dedicated, purpose-specific, memory pools).
>> Moreover, if we implement it into the core, others could use it too.
>> Any chance you can look into it ? Ludovic started spinning some code
>> internally but was probably sucked away for other tasks meanwhile.
>
> I propose we pre-allocate some memory for now

Care to elaborate what do you mean exactly ? or just send a patch :)

>> > +static int __init sproc_init(void)
>> > +{
>> > +       return platform_driver_register(&sproc_driver.drv);
>> > +}
>> > +module_init(sproc_init);
>> > +
>> > +static void __exit sproc_exit(void)
>> > +{
>> > +       platform_driver_unregister(&sproc_driver.drv);
>> > +}
>> > +module_exit(sproc_exit);
>>
>> Replace boilerplate code with module_platform_driver ?
>
> I tried, but the macros cannot handle the sproc_driver.drv as argument.

I see, but I wonder why did you have to create the ste_modem_driver
struct in the first place ? How does the modem code access
sproc_kick_callback ?

Can you instead just ditch ste_modem_driver and provide
sproc_kick_callback in kick_subscribe or something ?

I'm asking not because using module_platform_driver is so important,
but because this may suggest something more importantly may need to be
fixed here.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/