| Message ID | 1435600352-32733-1-git-send-email-vitalya@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: > This commit add cpu_die implementation > > Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > --- > > The discussion of the "keystone: psci: adds cpu_die implementation" commit > shows that if PCSI is enabled platform code doesn't need that implementation > at all. Having PSCI commands in DTB should be sufficient. Unfortunately > Keystone with LPAE enable requires some additional development. I don't follow. What do you need to implement for LPAE? Why not implement that rather than adding more platform code that you'll likely want to remove later anyway? Thanks, Mark. > To support HOTPLUG_CPU w/o PSCI we need platform implementation of > the cpu_die(), which is added by this patch. > > arch/arm/mach-keystone/keystone.h | 1 + > arch/arm/mach-keystone/platsmp.c | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h > index cd04a1c..93549cf 100644 > --- a/arch/arm/mach-keystone/keystone.h > +++ b/arch/arm/mach-keystone/keystone.h > @@ -12,6 +12,7 @@ > #define __KEYSTONE_H__ > > #define KEYSTONE_MON_CPU_UP_IDX 0x00 > +#define KEYSTONE_MON_CPU_DIE_IDX 0x84000002 > > #ifndef __ASSEMBLER__ > > diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c > index 5f46a7c..4e5bdde 100644 > --- a/arch/arm/mach-keystone/platsmp.c > +++ b/arch/arm/mach-keystone/platsmp.c > @@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu) > {} > #endif > > + > +#ifdef CONFIG_HOTPLUG_CPU > +static void keystone_cpu_die(unsigned int cpu) > +{ > + int error; > + > + pr_debug("keystone-smp: cpu_die %d\n", cpu); > + > + error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0); > + if (error) > + pr_err("CPU %d die failed with %d\n", cpu, error); > + > + /* > + * we shouldn't come here. But in case something went > + * wrong the code below prevents kernel from crush > + */ > + while (1) > + cpu_do_idle(); > +} > +#endif > + > struct smp_operations keystone_smp_ops __initdata = { > .smp_boot_secondary = keystone_smp_boot_secondary, > .smp_secondary_init = keystone_smp_secondary_initmem, > +#ifdef CONFIG_HOTPLUG_CPU > + .cpu_die = keystone_cpu_die, > +#endif > }; > -- > 1.9.1 >
On 06/29/2015 01:52 PM, Mark Rutland wrote: > On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: >> This commit add cpu_die implementation >> >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >> --- >> >> The discussion of the "keystone: psci: adds cpu_die implementation" commit >> shows that if PCSI is enabled platform code doesn't need that implementation >> at all. Having PSCI commands in DTB should be sufficient. Unfortunately >> Keystone with LPAE enable requires some additional development. > > I don't follow. > > What do you need to implement for LPAE? Hi Mark, The Keystone platform needs to set ttbr1 when it boots secondary core. It is done in the keystone_smp_secondary_initmem(), which is .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way how I can add similar function to psci_smp_ops. Do you have any idea? > > Why not implement that rather than adding more platform code that you'll > likely want to remove later anyway? If I can solve the above problem, I will not add this code. Thanks, Vitaly > > Thanks, > Mark. > >> To support HOTPLUG_CPU w/o PSCI we need platform implementation of >> the cpu_die(), which is added by this patch. >> >> arch/arm/mach-keystone/keystone.h | 1 + >> arch/arm/mach-keystone/platsmp.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h >> index cd04a1c..93549cf 100644 >> --- a/arch/arm/mach-keystone/keystone.h >> +++ b/arch/arm/mach-keystone/keystone.h >> @@ -12,6 +12,7 @@ >> #define __KEYSTONE_H__ >> >> #define KEYSTONE_MON_CPU_UP_IDX 0x00 >> +#define KEYSTONE_MON_CPU_DIE_IDX 0x84000002 >> >> #ifndef __ASSEMBLER__ >> >> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c >> index 5f46a7c..4e5bdde 100644 >> --- a/arch/arm/mach-keystone/platsmp.c >> +++ b/arch/arm/mach-keystone/platsmp.c >> @@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu) >> {} >> #endif >> >> + >> +#ifdef CONFIG_HOTPLUG_CPU >> +static void keystone_cpu_die(unsigned int cpu) >> +{ >> + int error; >> + >> + pr_debug("keystone-smp: cpu_die %d\n", cpu); >> + >> + error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0); >> + if (error) >> + pr_err("CPU %d die failed with %d\n", cpu, error); >> + >> + /* >> + * we shouldn't come here. But in case something went >> + * wrong the code below prevents kernel from crush >> + */ >> + while (1) >> + cpu_do_idle(); >> +} >> +#endif >> + >> struct smp_operations keystone_smp_ops __initdata = { >> .smp_boot_secondary = keystone_smp_boot_secondary, >> .smp_secondary_init = keystone_smp_secondary_initmem, >> +#ifdef CONFIG_HOTPLUG_CPU >> + .cpu_die = keystone_cpu_die, >> +#endif >> }; >> -- >> 1.9.1 >>
On 6/29/15 11:43 AM, Vitaly Andrianov wrote: > > > On 06/29/2015 01:52 PM, Mark Rutland wrote: >> On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: >>> This commit add cpu_die implementation >>> >>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >>> --- >>> >>> The discussion of the "keystone: psci: adds cpu_die implementation" >>> commit >>> shows that if PCSI is enabled platform code doesn't need that >>> implementation >>> at all. Having PSCI commands in DTB should be sufficient. Unfortunately >>> Keystone with LPAE enable requires some additional development. >> >> I don't follow. >> >> What do you need to implement for LPAE? > Hi Mark, > > The Keystone platform needs to set ttbr1 when it boots secondary core. > It is done in the keystone_smp_secondary_initmem(), which is > .smp_secondary_init member of the keystone_smp_ops. I couldn't find a > way how I can add similar function to psci_smp_ops. > > Do you have any idea? > >> >> Why not implement that rather than adding more platform code that you'll >> likely want to remove later anyway? > > If I can solve the above problem, I will not add this code. > This problem is already solved. :-) After [1], there is no longer need to fiddle with TTBR1 setup for secondary bringup. I believe its already in RMK's queue and it should show up in linus's tree after 4.2 merge window. Regards, Santosh [1] http://www.spinics.net/lists/arm-kernel/msg416129.html
On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote: > > > On 06/29/2015 01:52 PM, Mark Rutland wrote: > >On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: > >>This commit add cpu_die implementation > >> > >>Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > >>--- > >> > >>The discussion of the "keystone: psci: adds cpu_die implementation" commit > >>shows that if PCSI is enabled platform code doesn't need that implementation > >>at all. Having PSCI commands in DTB should be sufficient. Unfortunately > >>Keystone with LPAE enable requires some additional development. > > > >I don't follow. > > > >What do you need to implement for LPAE? > Hi Mark, > > The Keystone platform needs to set ttbr1 when it boots secondary core. > It is done in the keystone_smp_secondary_initmem(), which is > .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way > how I can add similar function to psci_smp_ops. TTBR1 will be set by generic code. You don't need to do anything special now that my fixes for TI's horrid physical address space switch are in. (you may remember, you tested the patches...) secondary_startup now loads r4/r5 with the 64-bit physical swapper page table address, and r8 contains the _pfn_ of the swapper_pg_dir. It then calls through to (eventually) __v7_setup, which passes these registers through to the v7_ttb_setup asm macro - r4 as ttbr0l, r5 as ttbr0h, and r8 as ttbr1. v7_ttb_setup converts the PFN to a physical address and programs it into TTBR1. TTBR0 is set directly from r4/r5 in generic code paths in head.S::__enable_mmu. There is no need to do any messing around when starting up a secondary core on Keystone2. In fact, my patches that I pointed you at a few months ago ripped out all that code. The entire keystone2 platsmp.c file now contains only this code: static int keystone_smp_boot_secondary(unsigned int cpu, struct task_struct *idle) { unsigned long start = virt_to_idmap(&secondary_startup); int error; pr_debug("keystone-smp: booting cpu %d, vector %08lx\n", cpu, start); error = keystone_cpu_smc(KEYSTONE_MON_CPU_UP_IDX, cpu, start); if (error) pr_err("CPU %d bringup failed with %d\n", cpu, error); return error; } struct smp_operations keystone_smp_ops __initdata = { .smp_boot_secondary = keystone_smp_boot_secondary, }; which is a lot simpler than it was.
On Mon, Jun 29, 2015 at 10:28:14PM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote: > > > > > > On 06/29/2015 01:52 PM, Mark Rutland wrote: > > >On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: > > >>This commit add cpu_die implementation > > >> > > >>Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > > >>--- > > >> > > >>The discussion of the "keystone: psci: adds cpu_die implementation" commit > > >>shows that if PCSI is enabled platform code doesn't need that implementation > > >>at all. Having PSCI commands in DTB should be sufficient. Unfortunately > > >>Keystone with LPAE enable requires some additional development. > > > > > >I don't follow. > > > > > >What do you need to implement for LPAE? > > Hi Mark, > > > > The Keystone platform needs to set ttbr1 when it boots secondary core. > > It is done in the keystone_smp_secondary_initmem(), which is > > .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way > > how I can add similar function to psci_smp_ops. > > TTBR1 will be set by generic code. You don't need to do anything special > now that my fixes for TI's horrid physical address space switch are in. > (you may remember, you tested the patches...) Oh, it was Murali who tested it, not yourself. Sorry. Suggest you dig out the patches either from mainline (they're in Linus' tip) or ask Murali for them...
On 06/29/2015 05:37 PM, Russell King - ARM Linux wrote: > On Mon, Jun 29, 2015 at 10:28:14PM +0100, Russell King - ARM Linux wrote: >> On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote: >>> >>> >>> On 06/29/2015 01:52 PM, Mark Rutland wrote: >>>> On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote: >>>>> This commit add cpu_die implementation >>>>> >>>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >>>>> --- >>>>> >>>>> The discussion of the "keystone: psci: adds cpu_die implementation" commit >>>>> shows that if PCSI is enabled platform code doesn't need that implementation >>>>> at all. Having PSCI commands in DTB should be sufficient. Unfortunately >>>>> Keystone with LPAE enable requires some additional development. >>>> >>>> I don't follow. >>>> >>>> What do you need to implement for LPAE? >>> Hi Mark, >>> >>> The Keystone platform needs to set ttbr1 when it boots secondary core. >>> It is done in the keystone_smp_secondary_initmem(), which is >>> .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way >>> how I can add similar function to psci_smp_ops. >> >> TTBR1 will be set by generic code. You don't need to do anything special >> now that my fixes for TI's horrid physical address space switch are in. >> (you may remember, you tested the patches...) > > Oh, it was Murali who tested it, not yourself. Sorry. Suggest you > dig out the patches either from mainline (they're in Linus' tip) or > ask Murali for them... > Thanks Russell, Excellent. I'll test how it will work using PSCI framework. Regards, Vitaly
On Tue, Jun 30, 2015 at 09:47:55AM -0400, Vitaly Andrianov wrote: > > > On 06/29/2015 05:37 PM, Russell King - ARM Linux wrote: > >Oh, it was Murali who tested it, not yourself. Sorry. Suggest you > >dig out the patches either from mainline (they're in Linus' tip) or > >ask Murali for them... > > > Thanks Russell, > > Excellent. I'll test how it will work using PSCI framework. The commits in mainline are: b2c3e38a5471 ARM: redo TTBR setup code for LPAE 1221ed10f2a5 ARM: cleanup early_paging_init() calling d8dc7fbd53ee ARM: re-implement physical address space switching c0b759d87eab ARM: keystone2: rename init_meminfo to pv_fixup 39b74fe82f73 ARM: keystone2: move address space switch printk into generic code c8ca2b4b2928 ARM: keystone2: move update of the phys-to-virt constants into generic code 30b5f4d6128e ARM: keystone2: move platform notifier initialisation into platform init There are an additional three which are worthwhile testing with them as they clean up this same code (and fix a subtle but very minor bug in the errata code paths): c76f238e261b ARM: proc-v7: sanitise and document registers around errata 4419496884ed ARM: proc-v7: clean up MIDR access 17e7bf86690e ARM: proc-v7: move CPU errata out of line So, picking the range b787f68c36d4..c76f238e261b will get all of them. (That's v4.1-rc1 to the tip of the branch holding those commits.)
diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h index cd04a1c..93549cf 100644 --- a/arch/arm/mach-keystone/keystone.h +++ b/arch/arm/mach-keystone/keystone.h @@ -12,6 +12,7 @@ #define __KEYSTONE_H__ #define KEYSTONE_MON_CPU_UP_IDX 0x00 +#define KEYSTONE_MON_CPU_DIE_IDX 0x84000002 #ifndef __ASSEMBLER__ diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c index 5f46a7c..4e5bdde 100644 --- a/arch/arm/mach-keystone/platsmp.c +++ b/arch/arm/mach-keystone/platsmp.c @@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu) {} #endif + +#ifdef CONFIG_HOTPLUG_CPU +static void keystone_cpu_die(unsigned int cpu) +{ + int error; + + pr_debug("keystone-smp: cpu_die %d\n", cpu); + + error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0); + if (error) + pr_err("CPU %d die failed with %d\n", cpu, error); + + /* + * we shouldn't come here. But in case something went + * wrong the code below prevents kernel from crush + */ + while (1) + cpu_do_idle(); +} +#endif + struct smp_operations keystone_smp_ops __initdata = { .smp_boot_secondary = keystone_smp_boot_secondary, .smp_secondary_init = keystone_smp_secondary_initmem, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_die = keystone_cpu_die, +#endif };
This commit add cpu_die implementation Signed-off-by: Vitaly Andrianov <vitalya@ti.com> --- The discussion of the "keystone: psci: adds cpu_die implementation" commit shows that if PCSI is enabled platform code doesn't need that implementation at all. Having PSCI commands in DTB should be sufficient. Unfortunately Keystone with LPAE enable requires some additional development. To support HOTPLUG_CPU w/o PSCI we need platform implementation of the cpu_die(), which is added by this patch. arch/arm/mach-keystone/keystone.h | 1 + arch/arm/mach-keystone/platsmp.c | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+)