| Message ID | 1545144493-11600-1-git-send-email-patrice.chotard@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] ARM: STi: Restore secondary CPU's bringup | expand |
On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: > From: Patrice Chotard <patrice.chotard@st.com> > > Due to pen_release and boot_lock removal, secondary CPU's bringup > was broken. Restore CPU's bringup by reworking properly > .smp_prepare_cpus and .smp_boot_secondary STi callbacks. Sorry, maybe I don't understand your commit message, but you seem to be saying that removal of the pen_release and boot_lock broke STi's secondary CPU bring up? Please clarify, and explain how that happened. Thanks. > > Signed-off-by: Patrice Chotard <patrice.chotard@st.com> > --- > arch/arm/mach-sti/platsmp.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c > index 2166850..9bfc93a 100644 > --- a/arch/arm/mach-sti/platsmp.c > +++ b/arch/arm/mach-sti/platsmp.c > @@ -28,8 +28,21 @@ > > #include "smp.h" > > +static u32 __iomem *cpu_strt_ptr; > + > static int sti_boot_secondary(unsigned int cpu, struct task_struct *idle) > { > + unsigned long entry_pa = __pa_symbol(secondary_startup); > + > + __raw_writel(entry_pa, cpu_strt_ptr); > + > + /* > + * wmb so that data is actually written > + * before cache flush is done > + */ > + smp_wmb(); > + sync_cache_w(cpu_strt_ptr); > + > /* > * Send the secondary CPU a soft interrupt, thereby causing > * it to jump to the secondary entrypoint. > @@ -43,10 +56,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) > { > struct device_node *np; > void __iomem *scu_base; > - u32 __iomem *cpu_strt_ptr; > u32 release_phys; > int cpu; > - unsigned long entry_pa = __pa_symbol(secondary_startup); > > np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); > > @@ -74,8 +85,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) > } > > /* > - * holding pen is usually configured in SBC DMEM but can also be > - * in RAM. > + * cpu-release-addr is usually configured in SBC DMEM but can > + * also be in RAM. > */ > > if (!memblock_is_memory(release_phys)) > @@ -85,17 +96,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) > cpu_strt_ptr = > (u32 __iomem *)phys_to_virt(release_phys); > > - __raw_writel(entry_pa, cpu_strt_ptr); > - > - /* > - * wmb so that data is actually written > - * before cache flush is done > - */ > - smp_wmb(); > - sync_cache_w(cpu_strt_ptr); > - > - if (!memblock_is_memory(release_phys)) > - iounmap(cpu_strt_ptr); > + set_cpu_possible(cpu, true); > } > } > > -- > 1.9.1 >
Hi Russell On 12/18/18 4:52 PM, Russell King - ARM Linux wrote: > On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: >> From: Patrice Chotard <patrice.chotard@st.com> >> >> Due to pen_release and boot_lock removal, secondary CPU's bringup >> was broken. Restore CPU's bringup by reworking properly >> .smp_prepare_cpus and .smp_boot_secondary STi callbacks. > > Sorry, maybe I don't understand your commit message, but you seem to be > saying that removal of the pen_release and boot_lock broke STi's secondary > CPU bring up? Please clarify, and explain how that happened. Correct, CPU1 failed to come online. It seems that writing secondary_startup address at cpu-release-addr in .smp_prepare_cpus callback was too early. Doing it in .smp_boot_secondary callback, insures that secondary_data struct is populated in __cpu_up() (stack, pgdir and swapper_pg_dir fields). If you are ok, i will pick up your patch [1] and this one to prepare a STi pull-request. Thanks Patrice [1] https://patchwork.kernel.org/patch/10729479/ > > Thanks. > >> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com> >> --- >> arch/arm/mach-sti/platsmp.c | 31 ++++++++++++++++--------------- >> 1 file changed, 16 insertions(+), 15 deletions(-) >> >> diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c >> index 2166850..9bfc93a 100644 >> --- a/arch/arm/mach-sti/platsmp.c >> +++ b/arch/arm/mach-sti/platsmp.c >> @@ -28,8 +28,21 @@ >> >> #include "smp.h" >> >> +static u32 __iomem *cpu_strt_ptr; >> + >> static int sti_boot_secondary(unsigned int cpu, struct task_struct *idle) >> { >> + unsigned long entry_pa = __pa_symbol(secondary_startup); >> + >> + __raw_writel(entry_pa, cpu_strt_ptr); >> + >> + /* >> + * wmb so that data is actually written >> + * before cache flush is done >> + */ >> + smp_wmb(); >> + sync_cache_w(cpu_strt_ptr); >> + >> /* >> * Send the secondary CPU a soft interrupt, thereby causing >> * it to jump to the secondary entrypoint. >> @@ -43,10 +56,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) >> { >> struct device_node *np; >> void __iomem *scu_base; >> - u32 __iomem *cpu_strt_ptr; >> u32 release_phys; >> int cpu; >> - unsigned long entry_pa = __pa_symbol(secondary_startup); >> >> np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); >> >> @@ -74,8 +85,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) >> } >> >> /* >> - * holding pen is usually configured in SBC DMEM but can also be >> - * in RAM. >> + * cpu-release-addr is usually configured in SBC DMEM but can >> + * also be in RAM. >> */ >> >> if (!memblock_is_memory(release_phys)) >> @@ -85,17 +96,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) >> cpu_strt_ptr = >> (u32 __iomem *)phys_to_virt(release_phys); >> >> - __raw_writel(entry_pa, cpu_strt_ptr); >> - >> - /* >> - * wmb so that data is actually written >> - * before cache flush is done >> - */ >> - smp_wmb(); >> - sync_cache_w(cpu_strt_ptr); >> - >> - if (!memblock_is_memory(release_phys)) >> - iounmap(cpu_strt_ptr); >> + set_cpu_possible(cpu, true); >> } >> } >> >> -- >> 1.9.1 >> >
On Tue, Dec 18, 2018 at 05:05:18PM +0000, Patrice CHOTARD wrote: > Hi Russell > > On 12/18/18 4:52 PM, Russell King - ARM Linux wrote: > > On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: > >> From: Patrice Chotard <patrice.chotard@st.com> > >> > >> Due to pen_release and boot_lock removal, secondary CPU's bringup > >> was broken. Restore CPU's bringup by reworking properly > >> .smp_prepare_cpus and .smp_boot_secondary STi callbacks. > > > > Sorry, maybe I don't understand your commit message, but you seem to be > > saying that removal of the pen_release and boot_lock broke STi's secondary > > CPU bring up? Please clarify, and explain how that happened. > > Correct, CPU1 failed to come online. > > It seems that writing secondary_startup address at cpu-release-addr in > .smp_prepare_cpus callback was too early. > > Doing it in .smp_boot_secondary callback, insures that secondary_data > struct is populated in __cpu_up() (stack, pgdir and swapper_pg_dir fields). Ah, you're saying that it causes the CPU to jump to secondary_startup while the boot CPU is in smp_prepare_cpus()? What triggers the CPU to jump to the address written to cpu_strt_ptr? What you're saying seems to suggest that it's the write to that address, rather than the IPI that's sent in sti_boot_secondary(). If the IPI in sti_boot_secondary() isn't doing anything, it ought to be removed. It'd also be a good idea to document what's going on as comments in the code for future maintanence. > If you are ok, i will pick up your patch [1] and this one to prepare a > STi pull-request. Yes, although I'll have to delay patch 9 as a result.
Hi Russell On 12/18/18 6:27 PM, Russell King - ARM Linux wrote: > On Tue, Dec 18, 2018 at 05:05:18PM +0000, Patrice CHOTARD wrote: >> Hi Russell >> >> On 12/18/18 4:52 PM, Russell King - ARM Linux wrote: >>> On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: >>>> From: Patrice Chotard <patrice.chotard@st.com> >>>> >>>> Due to pen_release and boot_lock removal, secondary CPU's bringup >>>> was broken. Restore CPU's bringup by reworking properly >>>> .smp_prepare_cpus and .smp_boot_secondary STi callbacks. >>> >>> Sorry, maybe I don't understand your commit message, but you seem to be >>> saying that removal of the pen_release and boot_lock broke STi's secondary >>> CPU bring up? Please clarify, and explain how that happened. >> >> Correct, CPU1 failed to come online. >> >> It seems that writing secondary_startup address at cpu-release-addr in >> .smp_prepare_cpus callback was too early. >> >> Doing it in .smp_boot_secondary callback, insures that secondary_data >> struct is populated in __cpu_up() (stack, pgdir and swapper_pg_dir fields). > > Ah, you're saying that it causes the CPU to jump to secondary_startup > while the boot CPU is in smp_prepare_cpus()? What triggers the CPU Yes > to jump to the address written to cpu_strt_ptr? What you're saying > seems to suggest that it's the write to that address, rather than the > IPI that's sent in sti_boot_secondary(). At platform startup, an U-Bootrom firmware initialize secondary CPU and make it spinning waiting for a jump address to be written at cpu_strt_ptr. I didn't pay attention to the IPI, you are right IPI is useless, i will remove it. > > If the IPI in sti_boot_secondary() isn't doing anything, it ought to > be removed. It'd also be a good idea to document what's going on as > comments in the code for future maintanence. Agree, i will add a comment. Thanks Patrice > >> If you are ok, i will pick up your patch [1] and this one to prepare a >> STi pull-request. > > Yes, although I'll have to delay patch 9 as a result. >
On Wed, Dec 19, 2018 at 10:31:35AM +0000, Patrice CHOTARD wrote: > Hi Russell > > On 12/18/18 6:27 PM, Russell King - ARM Linux wrote: > > On Tue, Dec 18, 2018 at 05:05:18PM +0000, Patrice CHOTARD wrote: > >> Hi Russell > >> > >> On 12/18/18 4:52 PM, Russell King - ARM Linux wrote: > >>> On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: > >>>> From: Patrice Chotard <patrice.chotard@st.com> > >>>> > >>>> Due to pen_release and boot_lock removal, secondary CPU's bringup > >>>> was broken. Restore CPU's bringup by reworking properly > >>>> .smp_prepare_cpus and .smp_boot_secondary STi callbacks. > >>> > >>> Sorry, maybe I don't understand your commit message, but you seem to be > >>> saying that removal of the pen_release and boot_lock broke STi's secondary > >>> CPU bring up? Please clarify, and explain how that happened. > >> > >> Correct, CPU1 failed to come online. > >> > >> It seems that writing secondary_startup address at cpu-release-addr in > >> .smp_prepare_cpus callback was too early. > >> > >> Doing it in .smp_boot_secondary callback, insures that secondary_data > >> struct is populated in __cpu_up() (stack, pgdir and swapper_pg_dir fields). > > > > Ah, you're saying that it causes the CPU to jump to secondary_startup > > while the boot CPU is in smp_prepare_cpus()? What triggers the CPU > > Yes > > > to jump to the address written to cpu_strt_ptr? What you're saying > > seems to suggest that it's the write to that address, rather than the > > IPI that's sent in sti_boot_secondary(). > > At platform startup, an U-Bootrom firmware initialize secondary CPU and > make it spinning waiting for a jump address to be written at cpu_strt_ptr. > > I didn't pay attention to the IPI, you are right IPI is useless, i will > remove it. Okay, in that case may I suggest an alternative to taking my patch which will break this, and then fixing it in a subsequent patch - please merge the two patches together so it becomes one "clean up" patch which doesn't cause any breakage. Thanks.
On 12/19/18 12:28 PM, Russell King - ARM Linux wrote: > On Wed, Dec 19, 2018 at 10:31:35AM +0000, Patrice CHOTARD wrote: >> Hi Russell >> >> On 12/18/18 6:27 PM, Russell King - ARM Linux wrote: >>> On Tue, Dec 18, 2018 at 05:05:18PM +0000, Patrice CHOTARD wrote: >>>> Hi Russell >>>> >>>> On 12/18/18 4:52 PM, Russell King - ARM Linux wrote: >>>>> On Tue, Dec 18, 2018 at 03:48:13PM +0100, patrice.chotard@st.com wrote: >>>>>> From: Patrice Chotard <patrice.chotard@st.com> >>>>>> >>>>>> Due to pen_release and boot_lock removal, secondary CPU's bringup >>>>>> was broken. Restore CPU's bringup by reworking properly >>>>>> .smp_prepare_cpus and .smp_boot_secondary STi callbacks. >>>>> >>>>> Sorry, maybe I don't understand your commit message, but you seem to be >>>>> saying that removal of the pen_release and boot_lock broke STi's secondary >>>>> CPU bring up? Please clarify, and explain how that happened. >>>> >>>> Correct, CPU1 failed to come online. >>>> >>>> It seems that writing secondary_startup address at cpu-release-addr in >>>> .smp_prepare_cpus callback was too early. >>>> >>>> Doing it in .smp_boot_secondary callback, insures that secondary_data >>>> struct is populated in __cpu_up() (stack, pgdir and swapper_pg_dir fields). >>> >>> Ah, you're saying that it causes the CPU to jump to secondary_startup >>> while the boot CPU is in smp_prepare_cpus()? What triggers the CPU >> >> Yes >> >>> to jump to the address written to cpu_strt_ptr? What you're saying >>> seems to suggest that it's the write to that address, rather than the >>> IPI that's sent in sti_boot_secondary(). >> >> At platform startup, an U-Bootrom firmware initialize secondary CPU and >> make it spinning waiting for a jump address to be written at cpu_strt_ptr. >> >> I didn't pay attention to the IPI, you are right IPI is useless, i will >> remove it. > > Okay, in that case may I suggest an alternative to taking my patch > which will break this, and then fixing it in a subsequent patch - > please merge the two patches together so it becomes one "clean up" > patch which doesn't cause any breakage. > > Thanks. > Ok, agree with your proposal. I will squash our 2 patches. Thanks Patrice
diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c index 2166850..9bfc93a 100644 --- a/arch/arm/mach-sti/platsmp.c +++ b/arch/arm/mach-sti/platsmp.c @@ -28,8 +28,21 @@ #include "smp.h" +static u32 __iomem *cpu_strt_ptr; + static int sti_boot_secondary(unsigned int cpu, struct task_struct *idle) { + unsigned long entry_pa = __pa_symbol(secondary_startup); + + __raw_writel(entry_pa, cpu_strt_ptr); + + /* + * wmb so that data is actually written + * before cache flush is done + */ + smp_wmb(); + sync_cache_w(cpu_strt_ptr); + /* * Send the secondary CPU a soft interrupt, thereby causing * it to jump to the secondary entrypoint. @@ -43,10 +56,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) { struct device_node *np; void __iomem *scu_base; - u32 __iomem *cpu_strt_ptr; u32 release_phys; int cpu; - unsigned long entry_pa = __pa_symbol(secondary_startup); np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); @@ -74,8 +85,8 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) } /* - * holding pen is usually configured in SBC DMEM but can also be - * in RAM. + * cpu-release-addr is usually configured in SBC DMEM but can + * also be in RAM. */ if (!memblock_is_memory(release_phys)) @@ -85,17 +96,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus) cpu_strt_ptr = (u32 __iomem *)phys_to_virt(release_phys); - __raw_writel(entry_pa, cpu_strt_ptr); - - /* - * wmb so that data is actually written - * before cache flush is done - */ - smp_wmb(); - sync_cache_w(cpu_strt_ptr); - - if (!memblock_is_memory(release_phys)) - iounmap(cpu_strt_ptr); + set_cpu_possible(cpu, true); } }