[PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[patrice.chotard]

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.

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

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Russell King - ARM Linux]

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
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Patrice CHOTARD]

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
>>
> 

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Patrice CHOTARD]

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.
> 

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 1/1] ARM: STi: Restore secondary CPU's bringup

[Patrice CHOTARD]

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