[PATCH 01/02] ARM: shmobile: Add r8a73a4 SMP support using APMU code

[Magnus Damm]

Hi Olof,

On Thu, Aug 29, 2013 at 3:16 AM, Olof Johansson <olof at lixom.net> wrote:
> On Wed, Aug 28, 2013 at 03:26:56PM +0900, Magnus Damm wrote:
>> Hi Olof,
>>
>> On Thu, Aug 22, 2013 at 2:48 PM, Olof Johansson <olof at lixom.net> wrote:
>> > [+khilman]
>> >
>> > Hi,
>> >
>> > On Wed, Aug 21, 2013 at 05:57:15PM +0900, Simon Horman wrote:
>> >> On Thu, Aug 08, 2013 at 02:52:32PM +0900, Magnus Damm wrote:
>> >> > Hi Arnd and Olof,
>> >> >
>> >> > On Thu, Aug 8, 2013 at 7:55 AM, Magnus Damm <magnus.damm at gmail.com> wrote:
>> >> > > From: Magnus Damm <damm at opensource.se>
>> >> > >
>> >> > > Add r8a73a4 SMP support using the shared APMU code. To enable
>> >> > > SMP the r8a73a4 specific DTS needs to be updated to include
>> >> > > CPU cores, and this is happening in a separate patch.
>> >> > >
>> >> > > Signed-off-by: Magnus Damm <damm at opensource.se>
>> >> > > ---
>> >> > >
>> >> > >  arch/arm/mach-shmobile/Makefile               |    1
>> >> > >  arch/arm/mach-shmobile/include/mach/r8a73a4.h |    1
>> >> > >  arch/arm/mach-shmobile/setup-r8a73a4.c        |    3 +
>> >> > >  arch/arm/mach-shmobile/smp-r8a73a4.c          |   75 +++++++++++++++++++++++++
>> >> > >  4 files changed, 80 insertions(+)
>> >> > >
>> >> > > --- 0001/arch/arm/mach-shmobile/Makefile
>> >> > > +++ work/arch/arm/mach-shmobile/Makefile        2013-08-07 20:07:31.000000000 +0900
>> >> > > @@ -33,6 +33,7 @@ endif
>> >> > >  # SMP objects
>> >> > >  smp-y                          := platsmp.o headsmp.o
>> >> > >  smp-$(CONFIG_ARCH_SH73A0)      += smp-sh73a0.o headsmp-scu.o platsmp-scu.o
>> >> > > +smp-$(CONFIG_ARCH_R8A73A4)     += smp-r8a73a4.o platsmp-apmu.o
>> >> > >  smp-$(CONFIG_ARCH_R8A7779)     += smp-r8a7779.o headsmp-scu.o platsmp-scu.o
>> >> > >  smp-$(CONFIG_ARCH_EMEV2)       += smp-emev2.o headsmp-scu.o platsmp-scu.o
>> >> > >
>> >> > > --- 0008/arch/arm/mach-shmobile/include/mach/r8a73a4.h
>> >> > > +++ work/arch/arm/mach-shmobile/include/mach/r8a73a4.h  2013-08-07 20:08:02.000000000 +0900
>> >> > > @@ -6,5 +6,6 @@ void r8a73a4_add_dt_devices(void);
>> >> > >  void r8a73a4_clock_init(void);
>> >> > >  void r8a73a4_pinmux_init(void);
>> >> > >  void r8a73a4_init_early(void);
>> >> > > +extern struct smp_operations r8a73a4_smp_ops;
>> >> > >
>> >> > >  #endif /* __ASM_R8A73A4_H__ */
>> >> > > --- 0008/arch/arm/mach-shmobile/setup-r8a73a4.c
>> >> > > +++ work/arch/arm/mach-shmobile/setup-r8a73a4.c 2013-08-07 20:07:48.000000000 +0900
>> >> > > @@ -212,6 +212,9 @@ void __init r8a73a4_init_early(void)
>> >> > >  #ifndef CONFIG_ARM_ARCH_TIMER
>> >> > >         shmobile_setup_delay(1500, 2, 4); /* Cortex-A15 @ 1500MHz */
>> >> > >  #endif
>> >> > > +#ifdef CONFIG_SMP
>> >> > > +       smp_set_ops(&r8a73a4_smp_ops);
>> >> > > +#endif
>> >> > >  }
>> >> >
>> >> > Arnd or Olof, may I ask for your advice here?
>> >> >
>> >> > I think it's quite nice to use smp_set_ops() in ->init_early() to
>> >> > install the per-SoC SMP operations. I prefer that over using the
>> >> > smp_ops directly in the per-board DT_MACHINE_START. Which way do you
>> >> > prefer?
>> >>
>> >> Olof, Arnd, Ping.
>> >
>> >
>> > Sorry, missed the original question when it was posted.
>> >
>> > I guess I don't see the benefit of doing it in code vs smp_init? You
>> > already have a DT_MACHINE section for r8a73a4 in this case.
>>
>> The benefit would be to reduce the number of callbacks used in
>> DT_MACHINE. Today on mach-shmobile we are already using ->init_early()
>> for delay and timer setup, and if we also can invoke smp_set_ops()
>> there then we would have per-SoC SMP support hooked in without having
>> to use either ->smp or ->smp_init() for every board.
>>
>> As you may have seen we have quite a few DT_MACHINE entires in
>> mach-shmobile, and I'd like to keep them as small and consistent as
>> ever possible. I can't really see how a board specific DT_MACHINE has
>> anything to do with state of SMP on a particular SoC, so for any given
>> board I'd prefer to keep the board code without any SMP dependencies
>> and simply use ->init_early() to setup delay, timers and SMP.
>>
>> > If you use .smp_init = smp_init_ops(r8a73a4_smp_ops) then there's no
>> > ifdef needed down in the DT_MACHINE section either.
>>
>> It's nice to not use the #ifdefs, I agree. I'd like to have a dummy
>> stub for smp_set_ops() too in the case of CONFIG_SMP=n, that would
>> make the above code a bit prettier.
>>
>> I hope this clarifies the reasons behind my question!
>>
>> If I understand you correctly then I guess you prefer keeping the code
>> as is with DT_MACHINE ->smp = smp_ops(foo) over using smp_set_ops() in
>> ->init_early()?
>
> Yes, that was my initial preference. However, that was based on the assumption
> that you would keep the current setup with several DT_MACHINE entries.
>
> If you think you will shortly start to distill down and only keep a couple of
> DT_MACHINE structures (and start sharing them between SoCs), then doing this in
> code makes sense. However, if that is further out than a release or two then
> I think doing it in DT_MACHINE makes more sense.

Thanks for your reply. I believe reducing the number of DT_MACHINE
will surely happen in 2014 but will still be relatively far away, so I
doubt anything will change in 1-2 releases. Based on that I will keep
the same approach as-is with DT_MACHINE .smp pointing to per-soc smp
ops.

Cheers,

/ magnus