[maverick] UBUNTU: SAUCE: Load SysLink modules on start-up on OMAP4 devices

This patch will become part of the Syslink driver code and is not due to go upstream until
the remaining Syslink code is ready to do so, hence SAUCE. It is believed that the next 
version TI release will be upstreamable.

This patch registers the SysLink driver-set as a platform device.
During the registration process a uevent will be fired, allowing
udev to pick it up and load the necessary loadable modules.

The following changes since commit 05cd276ae6bfe20745244f2285fab2b3d40ca1e3:
  Tim Gardner (1):
        UBUNTU: Start new release

are available in the git repository at:

  git://kernel.ubuntu.com/lag/ubuntu-maverick.git ti-omap4-syslink

Lee Jones (1):
      UBUNTU: SAUCE: Load SysLink modules on start-up on OMAP4 devices

 arch/arm/mach-omap2/omap4-common.c          |   23 +++++++++++++++++++++++
 drivers/dsp/syslink/multicore_ipc/ipc_drv.c |    2 ++
 2 files changed, 25 insertions(+), 0 deletions(-)

Signed-off-by: Lee Jones <lee.jones@canonical.com>
---

Lee,

Thanks for your patch. I have some comments below.

>
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

-----Original Message-----

> From: kernel-team-bounces@lists.ubuntu.com [mailto:kernel-team-
> bounces@lists.ubuntu.com] On Behalf Of Lee Jones
> Sent: Wednesday, July 07, 2010 11:08 AM
> To: kernel-team
> Subject: [maverick][PATCH] UBUNTU: SAUCE: Load SysLink modules on start-up
> on OMAP4 devices

snip

>
>
> diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-
> omap2/omap4-common.c
> index 91b5d36..193d233 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -71,4 +71,27 @@ static int __init omap_l2_cache_init(void)
>  early_initcall(omap_l2_cache_init);
>  #endif
>
> +static struct platform_device omap4_syslink_device = {
> +     .name          = "syslink_ipc",
> +     .id            = -1,
> +     .num_resources = 0,
> +};
>
> +static struct platform_device *omap4_syslink_devices[] = {
> +        &omap4_syslink_device,
> +};
> +

is it worth using an array of platform_device for just 1 device? At least the array should be marked as __initdata so that it does not take memory after init. However I would prefer not to use an array.


> +static int __init omap4_syslink_init(void)
> +{
> +     int retval;
> +
> +     retval = platform_add_devices(omap4_syslink_devices,
> ARRAY_SIZE(omap4_syslink_devices));
> +
> +     if (retval != 0)
> +             pr_err("%s: Failed to register devices: %d\n", __func__,
> retval);
> +     else
> +             pr_info("%s: Successfully registered devices\n", __func__);
> +
> +     return retval;
> +}
> +module_init(omap4_syslink_init);

you aren't in a module here, this code is built-in in the kernel (and will always be). This is supposed to be called during platform init, not during do_initcall. Can we call this function from the machine init function instead of using this trick? (e.g. omap_4430sdp_init() and omap_panda_init())


> diff --git a/drivers/dsp/syslink/multicore_ipc/ipc_drv.c
> b/drivers/dsp/syslink/multicore_ipc/ipc_drv.c
> index e95eab2..e11998d 100644
> --- a/drivers/dsp/syslink/multicore_ipc/ipc_drv.c
> +++ b/drivers/dsp/syslink/multicore_ipc/ipc_drv.c
> @@ -240,3 +240,5 @@ static void __exit ipc_exit(void)
>   */
>  module_init(ipc_init);
>  module_exit(ipc_exit);
> +
> +MODULE_ALIAS("platform:" IPC_NAME);
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Hi Nicolas,

Thank you for your comments.

>> >> +static struct platform_device omap4_syslink_device = {
>> >> +     .name          = "syslink_ipc",
>> >> +     .id            = -1,
>> >> +     .num_resources = 0,
>> >> +};
>> >>
>> >> +static struct platform_device *omap4_syslink_devices[] = {
>> >> +        &omap4_syslink_device,
>> >> +};
>> >> +
> > 
> > is it worth using an array of platform_device for just 1 device? At least the array should be marked as __initdata so that 
> > it does not take memory after init. However I would prefer not to use an array.
> > 

I thought by doing this way it would be easier if we were to add more devices at a later date.

I did have __initdata there. I took it out during debugging, as it was causing compile errors. I would be fine now though.

I have no preference either way - pick one and we'll run with it.

>> >> +module_init(omap4_syslink_init);
> > 
> > you aren't in a module here, this code is built-in in the kernel (and will always be). This is supposed to be called during 
> > platform init, not during do_initcall. Can we call this function from the machine init function instead of using this trick? (e.g. omap_4430sdp_init() and omap_panda_init())

Yes, this code is built-in, but it is part of the SysLink code, which is a module and is happy to be initiated at the same time.

Do: "grep -nHre module_init arch/*", and you'll see it is used many times within architecture specific code.

> > Can we call this function from the machine init function instead of using this trick? (e.g. omap_4430sdp_init() and omap_panda_init())

We can call the function from each machine, but it will need to be added to every OMAP4 machine that you release.

There are other *_initcalls available if you'd rather change it - some will work, some won't:

early_initcall    - kills the kernel (I think this is done before the platform code is initiated)
core_initcall     - not tried
postcore_initcall - not tried
arch_initcall     - not tried
subsys_initcall   - not tried
fs_initcall       - not tried
rootfs_initcall   - not tried
device_initcall   - not tried
late_initcall     - not tried

If anyone else has any better ideas (or knows the _correct_ solution), feel free to chirp in.

Kind regards,
Lee

>> +module_init(omap4_syslink_init);
> 
> you aren't in a module here, this code is built-in in the kernel (and will always be). 
> This is supposed to be called during platform init, not during do_initcall. Can we call 
> this function from the machine init function instead of using this trick? 
> (e.g. omap_4430sdp_init() and omap_panda_init())

Okay, I've had a bit of a poke around.

module_init() == device_initcall() 

Would device_initcall be a more appropriate naming convention for you?

All of the *_init* calls fall back to the same line of code.

#define __define_initcall(level,fn,id) \
	static initcall_t __initcall_##fn##id __used \
	__attribute__((__section__(".initcall" level ".init"))) = fn

>
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

-----Original Message-----

> From: Lee Jones [mailto:lee.jones@canonical.com]
> Sent: Wednesday, July 07, 2010 2:05 PM
> To: kernel-team
> Cc: Dechesne, Nicolas
> Subject: Re: [maverick][PATCH] UBUNTU: SAUCE: Load SysLink modules on
> start-up on OMAP4 devices
>
> >> +module_init(omap4_syslink_init);
> >
> > you aren't in a module here, this code is built-in in the kernel (and
> will always be).
> > This is supposed to be called during platform init, not during
> do_initcall. Can we call
> > this function from the machine init function instead of using this trick?
> > (e.g. omap_4430sdp_init() and omap_panda_init())
>
> Okay, I've had a bit of a poke around.
>
> module_init() == device_initcall()
>
> Would device_initcall be a more appropriate naming convention for you?

if module_init is already used in many places, I am fine with this approach.

>
> All of the *_init* calls fall back to the same line of code.
>
> #define __define_initcall(level,fn,id) \
>       static initcall_t __initcall_##fn##id __used \
>       __attribute__((__section__(".initcall" level ".init"))) = fn