diff mbox

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

Message ID 20130807225540.9856.67383.sendpatchset@w520 (mailing list archive)
State New, archived
Headers show

Commit Message

Magnus Damm Aug. 7, 2013, 10:55 p.m. UTC
From: Magnus Damm <damm@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@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(+)

Comments

Magnus Damm Aug. 8, 2013, 5:52 a.m. UTC | #1
Hi Arnd and Olof,

On Thu, Aug 8, 2013 at 7:55 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm@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@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?

Thanks,

/ magnus
Simon Horman Aug. 21, 2013, 8:57 a.m. UTC | #2
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@gmail.com> wrote:
> > From: Magnus Damm <damm@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@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.
Olof Johansson Aug. 22, 2013, 5:48 a.m. UTC | #3
[+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@gmail.com> wrote:
> > > From: Magnus Damm <damm@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@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.

If you use .smp_init = smp_init_ops(r8a73a4_smp_ops) then there's no
ifdef needed down in the DT_MACHINE section either.


-Olof
Simon Horman Aug. 22, 2013, 6:34 a.m. UTC | #4
On Wed, Aug 21, 2013 at 10:48:34PM -0700, Olof Johansson 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@gmail.com> wrote:
> > > > From: Magnus Damm <damm@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@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.
> 
> If you use .smp_init = smp_init_ops(r8a73a4_smp_ops) then there's no
> ifdef needed down in the DT_MACHINE section either.

Magnus, would you care to respin this series and the corresponding
r8a7790 series accordingly?
Sudeep KarkadaNagesha Aug. 22, 2013, 2:54 p.m. UTC | #5
On 07/08/13 23:55, Magnus Damm wrote:
> From: Magnus Damm <damm@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@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
platsmp-apmu.c is introducing new bindings which IMO is not required.
I have responded to that patch, but it would be nice to combine that
patch in a single series for easier review for at least one platform
using it.

>  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
>  }
>  
>  #ifdef CONFIG_USE_OF
> --- /dev/null
> +++ work/arch/arm/mach-shmobile/smp-r8a73a4.c	2013-08-07 20:09:10.000000000 +0900
> @@ -0,0 +1,75 @@
> +/*
> + * SMP support for r8a73a4
> + *
> + * Copyright (C) 2012-2013 Renesas Solutions Corp.
> + * Copyright (C) 2012 Takashi Yoshii <takashi.yoshii.ze@renesas.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/smp.h>
> +#include <asm/smp_plat.h>
> +#include <mach/common.h>
> +
> +#define SYSC		0xe6180000
> +#define CA7BAR		0x4020
> +#define CA15BAR		0x6020
> +#define RESCNT		0x801c
> +#define MERAM		0xe8080000
> +#define CCI_BASE	0xf0190000
> +#define CCI_SLAVE3	0x4000
> +#define CCI_SLAVE4	0x5000
> +#define CCI_SNOOP	0x0000
> +#define CCI_STATUS	0x000c
Have a look at cci driver and its bindings, these slave addresses needs
to be part of DTS.

> +
> +static void __init r8a73a4_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +	u32 bar;
> +	void __iomem *p;
> +
> +	/* let APMU code install data related to shmobile_boot_vector */
> +	shmobile_smp_apmu_prepare_cpus(max_cpus);
> +
> +	/* MERAM for jump stub, because BAR requires 256KB aligned address */
> +	p = ioremap_nocache(MERAM, shmobile_boot_size);
> +	memcpy_toio(p, shmobile_boot_vector, shmobile_boot_size);
> +	iounmap(p);
> +
> +	/* setup reset vector and disable reset */
> +	p = ioremap_nocache(SYSC, 0x9000);
> +	bar = (MERAM >> 8) & 0xfffffc00;
> +	writel_relaxed(bar, p + CA15BAR);
> +	writel_relaxed(bar, p + CA7BAR);
> +	writel_relaxed(bar | 0x10, p + CA15BAR);
> +	writel_relaxed(bar | 0x10, p + CA7BAR);
> +	writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 10), p + RESCNT);
> +	writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 9), p + RESCNT);
> +	iounmap(p);
> +
> +	/* enable snoop and DVM */
> +	p = ioremap_nocache(CCI_BASE, 0x8000);
> +	writel_relaxed(3, p + CCI_SLAVE3 + CCI_SNOOP);	/* CA15 */
> +	writel_relaxed(3, p + CCI_SLAVE4 + CCI_SNOOP);	/* CA7 */
> +	while (readl_relaxed(p + CCI_STATUS))
> +		/* wait for pending bit low */;
> +	iounmap(p);
You can use cci driver APIs for CCI control.

> +}
> +
> +struct smp_operations r8a73a4_smp_ops __initdata = {
> +	.smp_prepare_cpus	= r8a73a4_smp_prepare_cpus,
> +	.smp_boot_secondary	= shmobile_smp_apmu_boot_secondary,
> +#ifdef CONFIG_HOTPLUG_CPU
> +	.cpu_disable		= shmobile_smp_cpu_disable,
> +	.cpu_die		= shmobile_smp_apmu_cpu_die,
> +	.cpu_kill		= shmobile_smp_apmu_cpu_kill,
> +#endif
> +};
If you are running Linux in secure mode, you can reuse mcpm_smp_ops and
define these as part of mcpm_platform_ops

> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Magnus Damm Aug. 28, 2013, 6:13 a.m. UTC | #6
Hi again Sudeep,

On Thu, Aug 22, 2013 at 11:54 PM, Sudeep KarkadaNagesha
<Sudeep.KarkadaNagesha@arm.com> wrote:
> On 07/08/13 23:55, Magnus Damm wrote:
>> From: Magnus Damm <damm@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@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
> platsmp-apmu.c is introducing new bindings which IMO is not required.

I guess we will have to discuss a bit more how to support the APMU
hardware. Regarding using DT or static configuration, I believe
starting out with static and incrementally adding DT binding support
is the easiest way forward.

> I have responded to that patch, but it would be nice to combine that
> patch in a single series for easier review for at least one platform
> using it.

Good idea. I will resend a the next version as a single series to make
future review easier.

>>  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
>>  }
>>
>>  #ifdef CONFIG_USE_OF
>> --- /dev/null
>> +++ work/arch/arm/mach-shmobile/smp-r8a73a4.c 2013-08-07 20:09:10.000000000 +0900
>> @@ -0,0 +1,75 @@
>> +/*
>> + * SMP support for r8a73a4
>> + *
>> + * Copyright (C) 2012-2013 Renesas Solutions Corp.
>> + * Copyright (C) 2012 Takashi Yoshii <takashi.yoshii.ze@renesas.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/smp.h>
>> +#include <asm/smp_plat.h>
>> +#include <mach/common.h>
>> +
>> +#define SYSC         0xe6180000
>> +#define CA7BAR               0x4020
>> +#define CA15BAR              0x6020
>> +#define RESCNT               0x801c
>> +#define MERAM                0xe8080000
>> +#define CCI_BASE     0xf0190000
>> +#define CCI_SLAVE3   0x4000
>> +#define CCI_SLAVE4   0x5000
>> +#define CCI_SNOOP    0x0000
>> +#define CCI_STATUS   0x000c
> Have a look at cci driver and its bindings, these slave addresses needs
> to be part of DTS.

Ok, I will look into how we can make use of the CCI driver.

>> +
>> +static void __init r8a73a4_smp_prepare_cpus(unsigned int max_cpus)
>> +{
>> +     u32 bar;
>> +     void __iomem *p;
>> +
>> +     /* let APMU code install data related to shmobile_boot_vector */
>> +     shmobile_smp_apmu_prepare_cpus(max_cpus);
>> +
>> +     /* MERAM for jump stub, because BAR requires 256KB aligned address */
>> +     p = ioremap_nocache(MERAM, shmobile_boot_size);
>> +     memcpy_toio(p, shmobile_boot_vector, shmobile_boot_size);
>> +     iounmap(p);
>> +
>> +     /* setup reset vector and disable reset */
>> +     p = ioremap_nocache(SYSC, 0x9000);
>> +     bar = (MERAM >> 8) & 0xfffffc00;
>> +     writel_relaxed(bar, p + CA15BAR);
>> +     writel_relaxed(bar, p + CA7BAR);
>> +     writel_relaxed(bar | 0x10, p + CA15BAR);
>> +     writel_relaxed(bar | 0x10, p + CA7BAR);
>> +     writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 10), p + RESCNT);
>> +     writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 9), p + RESCNT);
>> +     iounmap(p);
>> +
>> +     /* enable snoop and DVM */
>> +     p = ioremap_nocache(CCI_BASE, 0x8000);
>> +     writel_relaxed(3, p + CCI_SLAVE3 + CCI_SNOOP);  /* CA15 */
>> +     writel_relaxed(3, p + CCI_SLAVE4 + CCI_SNOOP);  /* CA7 */
>> +     while (readl_relaxed(p + CCI_STATUS))
>> +             /* wait for pending bit low */;
>> +     iounmap(p);
> You can use cci driver APIs for CCI control.

Ok!

>> +}
>> +
>> +struct smp_operations r8a73a4_smp_ops __initdata = {
>> +     .smp_prepare_cpus       = r8a73a4_smp_prepare_cpus,
>> +     .smp_boot_secondary     = shmobile_smp_apmu_boot_secondary,
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +     .cpu_disable            = shmobile_smp_cpu_disable,
>> +     .cpu_die                = shmobile_smp_apmu_cpu_die,
>> +     .cpu_kill               = shmobile_smp_apmu_cpu_kill,
>> +#endif
>> +};
> If you are running Linux in secure mode, you can reuse mcpm_smp_ops and
> define these as part of mcpm_platform_ops

Thanks for pointing that out, but we're not running in secure more.

Thanks for the review!

Cheers,

/ magnus
Magnus Damm Aug. 28, 2013, 6:26 a.m. UTC | #7
Hi Olof,

On Thu, Aug 22, 2013 at 2:48 PM, Olof Johansson <olof@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@gmail.com> wrote:
>> > > From: Magnus Damm <damm@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@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()?

Cheers,

/ magnus
Sudeep KarkadaNagesha Aug. 28, 2013, 1:12 p.m. UTC | #8
On 28/08/13 07:13, Magnus Damm wrote:
> Hi again Sudeep,
> 
> On Thu, Aug 22, 2013 at 11:54 PM, Sudeep KarkadaNagesha
> <Sudeep.KarkadaNagesha@arm.com> wrote:
>> On 07/08/13 23:55, Magnus Damm wrote:
>>> From: Magnus Damm <damm@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@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
>> platsmp-apmu.c is introducing new bindings which IMO is not required.
> 
> I guess we will have to discuss a bit more how to support the APMU
> hardware. Regarding using DT or static configuration, I believe
> starting out with static and incrementally adding DT binding support
> is the easiest way forward.
> 
As I mentioned in the other email, you need use APMU as part of MCPM
platform ops. You still need APMU DT node and all the necessary
functions to access it. I said only 'cpus' property in DT node is
unnecessary and the way its used.

>> I have responded to that patch, but it would be nice to combine that
>> patch in a single series for easier review for at least one platform
>> using it.
> 
> Good idea. I will resend a the next version as a single series to make
> future review easier.
> 
Good.

[...]
>>> +}
>>> +
>>> +struct smp_operations r8a73a4_smp_ops __initdata = {
>>> +     .smp_prepare_cpus       = r8a73a4_smp_prepare_cpus,
>>> +     .smp_boot_secondary     = shmobile_smp_apmu_boot_secondary,
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +     .cpu_disable            = shmobile_smp_cpu_disable,
>>> +     .cpu_die                = shmobile_smp_apmu_cpu_die,
>>> +     .cpu_kill               = shmobile_smp_apmu_cpu_kill,
>>> +#endif
>>> +};
>> If you are running Linux in secure mode, you can reuse mcpm_smp_ops and
>> define these as part of mcpm_platform_ops
> 
> Thanks for pointing that out, but we're not running in secure more.
> 
Again I am not sure if you want to avoid supporting PSCI. I understand
it needs secure firmware but that's unavoidable if any of the CPU PM
functionality on that hardware is accessible only in secure mode and you
are running Linux in non-secure mode.

Regards,
Sudeep
Olof Johansson Aug. 28, 2013, 6:16 p.m. UTC | #9
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@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@gmail.com> wrote:
> >> > > From: Magnus Damm <damm@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@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.


-Olof
Magnus Damm Aug. 29, 2013, 4:37 a.m. UTC | #10
Hi Olof,

On Thu, Aug 29, 2013 at 3:16 AM, Olof Johansson <olof@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@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@gmail.com> wrote:
>> >> > > From: Magnus Damm <damm@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@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
diff mbox

Patch

--- 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
 }
 
 #ifdef CONFIG_USE_OF
--- /dev/null
+++ work/arch/arm/mach-shmobile/smp-r8a73a4.c	2013-08-07 20:09:10.000000000 +0900
@@ -0,0 +1,75 @@ 
+/*
+ * SMP support for r8a73a4
+ *
+ * Copyright (C) 2012-2013 Renesas Solutions Corp.
+ * Copyright (C) 2012 Takashi Yoshii <takashi.yoshii.ze@renesas.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/smp.h>
+#include <asm/smp_plat.h>
+#include <mach/common.h>
+
+#define SYSC		0xe6180000
+#define CA7BAR		0x4020
+#define CA15BAR		0x6020
+#define RESCNT		0x801c
+#define MERAM		0xe8080000
+#define CCI_BASE	0xf0190000
+#define CCI_SLAVE3	0x4000
+#define CCI_SLAVE4	0x5000
+#define CCI_SNOOP	0x0000
+#define CCI_STATUS	0x000c
+
+static void __init r8a73a4_smp_prepare_cpus(unsigned int max_cpus)
+{
+	u32 bar;
+	void __iomem *p;
+
+	/* let APMU code install data related to shmobile_boot_vector */
+	shmobile_smp_apmu_prepare_cpus(max_cpus);
+
+	/* MERAM for jump stub, because BAR requires 256KB aligned address */
+	p = ioremap_nocache(MERAM, shmobile_boot_size);
+	memcpy_toio(p, shmobile_boot_vector, shmobile_boot_size);
+	iounmap(p);
+
+	/* setup reset vector and disable reset */
+	p = ioremap_nocache(SYSC, 0x9000);
+	bar = (MERAM >> 8) & 0xfffffc00;
+	writel_relaxed(bar, p + CA15BAR);
+	writel_relaxed(bar, p + CA7BAR);
+	writel_relaxed(bar | 0x10, p + CA15BAR);
+	writel_relaxed(bar | 0x10, p + CA7BAR);
+	writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 10), p + RESCNT);
+	writel_relaxed(readl_relaxed(p + RESCNT) & ~(1 << 9), p + RESCNT);
+	iounmap(p);
+
+	/* enable snoop and DVM */
+	p = ioremap_nocache(CCI_BASE, 0x8000);
+	writel_relaxed(3, p + CCI_SLAVE3 + CCI_SNOOP);	/* CA15 */
+	writel_relaxed(3, p + CCI_SLAVE4 + CCI_SNOOP);	/* CA7 */
+	while (readl_relaxed(p + CCI_STATUS))
+		/* wait for pending bit low */;
+	iounmap(p);
+}
+
+struct smp_operations r8a73a4_smp_ops __initdata = {
+	.smp_prepare_cpus	= r8a73a4_smp_prepare_cpus,
+	.smp_boot_secondary	= shmobile_smp_apmu_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_disable		= shmobile_smp_cpu_disable,
+	.cpu_die		= shmobile_smp_apmu_cpu_die,
+	.cpu_kill		= shmobile_smp_apmu_cpu_kill,
+#endif
+};