Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671482 > unrolled thread

Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

Started byArnd Bergmann <arnd@arndb.de>
First post2017-06-21 10:20 +0200
Last post2017-07-03 10:20 +0200
Articles 12 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Arnd Bergmann <arnd@arndb.de> - 2017-06-21 10:20 +0200
    Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Arnd Bergmann <arnd@arndb.de> - 2017-06-21 12:30 +0200
      Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Andreas Färber <afaerber@suse.de> - 2017-06-21 18:50 +0200
        Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Arnd Bergmann <arnd@arndb.de> - 2017-06-21 20:10 +0200
    Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Arnd Bergmann <arnd@arndb.de> - 2017-06-29 17:10 +0200
      Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Andreas Färber <afaerber@suse.de> - 2017-06-29 17:30 +0200
        Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Arnd Bergmann <arnd@arndb.de> - 2017-06-29 18:00 +0200
          [PATCH] ARM: owl: smp: Drop bogus holding pen Andreas Färber <afaerber@suse.de> - 2017-07-01 23:40 +0200
            Re: [PATCH] ARM: owl: smp: Drop bogus holding pen Arnd Bergmann <arnd@arndb.de> - 2017-07-03 14:40 +0200
        答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 刘炜 <liuwei@actions-semi.com> - 2017-07-01 07:00 +0200
          Re: 答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 Andreas Färber <afaerber@suse.de> - 2017-07-01 22:00 +0200
            RE: RE: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500 刘炜 <liuwei@actions-semi.com> - 2017-07-03 10:20 +0200

#1671482 — Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-21 10:20 +0200
SubjectRe: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500
Message-ID<tUJ1M-3E2-27@gated-at.bofh.it>
On Tue, Jun 6, 2017 at 2:54 AM, Andreas Färber <afaerber@suse.de> wrote:
> Allow to bring up CPU1.
>
> Based on LeMaker linux-actions tree.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  v3 -> v4: Unchanged
>
>  v3: new
>
>  arch/arm/mach-actions/Makefile  |   3 +
>  arch/arm/mach-actions/headsmp.S |  68 ++++++++++++++++
>  arch/arm/mach-actions/platsmp.c | 166 ++++++++++++++++++++++++++++++++++++++++

I now see build errors in linux-next:

/git/arm-soc/arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
/git/arm-soc/arch/arm/mach-actions/platsmp.c:39:2: error:
'pen_release' undeclared (first use in this function); did you mean
'seq_release'?
  pen_release = val;
  ^~~~~~~~~~~
  seq_release
/git/arm-soc/arch/arm/mach-actions/platsmp.c:39:2: note: each
undeclared identifier is reported only once for each function it
appears in
/git/arm-soc/arch/arm/mach-actions/platsmp.c: In function
's500_wakeup_secondary':
/git/arm-soc/arch/arm/mach-actions/platsmp.c:79:2: error: implicit
declaration of function 'dsb_sev'
[-Werror=implicit-function-declaration]
  dsb_sev();
  ^~~~~~~
/git/arm-soc/arch/arm/mach-actions/platsmp.c: In function
's500_smp_boot_secondary':
/git/arm-soc/arch/arm/mach-actions/platsmp.c:108:7: error:
'pen_release' undeclared (first use in this function); did you mean
'seq_release'?


> +static DEFINE_SPINLOCK(boot_lock);
> +
> +static void write_pen_release(int val)
> +{
> +       pen_release = val;
> +       smp_wmb();
> +       __cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
> +       outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
> +}
> +
> +static void s500_smp_secondary_init(unsigned int cpu)
> +{
> +       /*
> +        * let the primary processor know we're out of the
> +        * pen, then head off into the C entry point
> +        */
> +       write_pen_release(-1);
> +
> +       spin_lock(&boot_lock);
> +       spin_unlock(&boot_lock);
> +}
> +
> +void owl_secondary_startup(void);
> +
> +static int s500_wakeup_secondary(unsigned int cpu)
> +{
> +       if (cpu > 3)
> +               return -EINVAL;
> +
> +       switch (cpu) {
> +       case 2:
> +       case 3:
> +               /* CPU2/3 are power-gated */
> +               return -EINVAL;
> +       }
> +
> +       /* wait for CPUx to run to WFE instruction */
> +       udelay(200);
> +
> +       writel(virt_to_phys(owl_secondary_startup),
> +              timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
> +       writel(OWL_CPUx_FLAG_BOOT,
> +              timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
> +
> +       dsb_sev();
> +       mb();
> +
> +       return 0;
> +}
> +
> +static int s500_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> +       unsigned long timeout;
> +       int ret;
> +
> +       ret = s500_wakeup_secondary(cpu);
> +       if (ret)
> +               return ret;
> +
> +       udelay(10);
> +
> +       spin_lock(&boot_lock);
> +
> +       /*
> +        * The secondary processor is waiting to be released from
> +        * the holding pen - release it, then wait for it to flag
> +        * that it has been released by resetting pen_release.
> +        */
> +       write_pen_release(cpu_logical_map(cpu));
> +       smp_send_reschedule(cpu);
> +
> +       timeout = jiffies + (1 * HZ);
> +       while (time_before(jiffies, timeout)) {
> +               if (pen_release == -1)
> +                       break;
> +       }
> +
> +       writel(0, timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
> +       writel(0, timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
> +
> +       spin_unlock(&boot_lock);
> +
> +       return pen_release != -1 ? -ENOSYS : 0;
> +}

This looks more complicated than necessary. Why do you need the holding
pen when you have a register to start up the CPU?

        Arnd

[toc] | [next] | [standalone]

#1671597

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-21 12:30 +0200
Message-ID<tUL3A-4SB-17@gated-at.bofh.it>
In reply to#1671482
On Wed, Jun 21, 2017 at 10:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 6, 2017 at 2:54 AM, Andreas Färber <afaerber@suse.de> wrote:
>> Allow to bring up CPU1.
>>
>> Based on LeMaker linux-actions tree.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  v3 -> v4: Unchanged
>>
>>  v3: new
>>
>>  arch/arm/mach-actions/Makefile  |   3 +
>>  arch/arm/mach-actions/headsmp.S |  68 ++++++++++++++++
>>  arch/arm/mach-actions/platsmp.c | 166 ++++++++++++++++++++++++++++++++++++++++
>
> I now see build errors in linux-next:
>
> /git/arm-soc/arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
> /git/arm-soc/arch/arm/mach-actions/platsmp.c:39:2: error:
> 'pen_release' undeclared (first use in this function); did you mean

I have a fix and tested it now. Please fold this into your patch:

       Arnd

Subject: [PATCH] ARM: owl: fix non-SMP build

arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
arch/arm/mach-actions/platsmp.c:39:2: error: 'pen_release' undeclared
(first use in this function); did you mean 'seq_release'?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm/mach-actions/Makefile b/arch/arm/mach-actions/Makefile
index 217e95d04b43..94f8dbafba21 100644
--- a/arch/arm/mach-actions/Makefile
+++ b/arch/arm/mach-actions/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_ARCH_ACTIONS) += owl.o
-obj-${CONFIG_ARCH_ACTIONS} += platsmp.o headsmp.o
+obj-y += owl.o
+obj-$(CONFIG_SMP) += platsmp.o headsmp.o

 AFLAGS_headsmp.o := -Wa,-march=armv7-a

[toc] | [prev] | [next] | [standalone]

#1671793

FromAndreas Färber <afaerber@suse.de>
Date2017-06-21 18:50 +0200
Message-ID<tUQZj-aB-7@gated-at.bofh.it>
In reply to#1671597
Am 21.06.2017 um 12:25 schrieb Arnd Bergmann:
> On Wed, Jun 21, 2017 at 10:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Jun 6, 2017 at 2:54 AM, Andreas Färber <afaerber@suse.de> wrote:
>>> Allow to bring up CPU1.
>>>
>>> Based on LeMaker linux-actions tree.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  v3 -> v4: Unchanged
>>>
>>>  v3: new
>>>
>>>  arch/arm/mach-actions/Makefile  |   3 +
>>>  arch/arm/mach-actions/headsmp.S |  68 ++++++++++++++++
>>>  arch/arm/mach-actions/platsmp.c | 166 ++++++++++++++++++++++++++++++++++++++++
>>
>> I now see build errors in linux-next:
>>
>> /git/arm-soc/arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
>> /git/arm-soc/arch/arm/mach-actions/platsmp.c:39:2: error:
>> 'pen_release' undeclared (first use in this function); did you mean
> 
> I have a fix and tested it now. Please fold this into your patch:
> 
>        Arnd
> 
> Subject: [PATCH] ARM: owl: fix non-SMP build
> 
> arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
> arch/arm/mach-actions/platsmp.c:39:2: error: 'pen_release' undeclared
> (first use in this function); did you mean 'seq_release'?
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/arch/arm/mach-actions/Makefile b/arch/arm/mach-actions/Makefile
> index 217e95d04b43..94f8dbafba21 100644
> --- a/arch/arm/mach-actions/Makefile
> +++ b/arch/arm/mach-actions/Makefile
> @@ -1,4 +1,4 @@
> -obj-$(CONFIG_ARCH_ACTIONS) += owl.o
> -obj-${CONFIG_ARCH_ACTIONS} += platsmp.o headsmp.o
> +obj-y += owl.o
> +obj-$(CONFIG_SMP) += platsmp.o headsmp.o

Sorry. Fixed up the two patches, thanks!

Interestingly the below issue had been found by automatic patch testing,
but the above wasn't. Maybe you can contribute your non-SMP config for
the bots, to help catch this earlier?

> 
>  AFLAGS_headsmp.o := -Wa,-march=armv7-a
> 

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1671867

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-21 20:10 +0200
Message-ID<tUSeJ-1fP-5@gated-at.bofh.it>
In reply to#1671793
On Wed, Jun 21, 2017 at 6:48 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 21.06.2017 um 12:25 schrieb Arnd Bergmann:
>> On Wed, Jun 21, 2017 at 10:16 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Tue, Jun 6, 2017 at 2:54 AM, Andreas Färber <afaerber@suse.de> wrote:
>>>> Allow to bring up CPU1.
>>>>
>>>> Based on LeMaker linux-actions tree.
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>>  v3 -> v4: Unchanged
>>>>
>>>>  v3: new
>>>>
>>>>  arch/arm/mach-actions/Makefile  |   3 +
>>>>  arch/arm/mach-actions/headsmp.S |  68 ++++++++++++++++
>>>>  arch/arm/mach-actions/platsmp.c | 166 ++++++++++++++++++++++++++++++++++++++++
>>>
>>> I now see build errors in linux-next:
>>>
>>> /git/arm-soc/arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
>>> /git/arm-soc/arch/arm/mach-actions/platsmp.c:39:2: error:
>>> 'pen_release' undeclared (first use in this function); did you mean
>>
>> I have a fix and tested it now. Please fold this into your patch:
>>
>>        Arnd
>>
>> Subject: [PATCH] ARM: owl: fix non-SMP build
>>
>> arch/arm/mach-actions/platsmp.c: In function 'write_pen_release':
>> arch/arm/mach-actions/platsmp.c:39:2: error: 'pen_release' undeclared
>> (first use in this function); did you mean 'seq_release'?
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> diff --git a/arch/arm/mach-actions/Makefile b/arch/arm/mach-actions/Makefile
>> index 217e95d04b43..94f8dbafba21 100644
>> --- a/arch/arm/mach-actions/Makefile
>> +++ b/arch/arm/mach-actions/Makefile
>> @@ -1,4 +1,4 @@
>> -obj-$(CONFIG_ARCH_ACTIONS) += owl.o
>> -obj-${CONFIG_ARCH_ACTIONS} += platsmp.o headsmp.o
>> +obj-y += owl.o
>> +obj-$(CONFIG_SMP) += platsmp.o headsmp.o
>
> Sorry. Fixed up the two patches, thanks!
>
> Interestingly the below issue had been found by automatic patch testing,
> but the above wasn't. Maybe you can contribute your non-SMP config for
> the bots, to help catch this earlier?

This was just a randconfig build. On ARM32, we do test
"multi_v7_defconfig+CONFIG_SMP=n"
in kernelci, but that only tests mach-actions after that gets added to
multi_v7_defconfig. Not sure if it's worth adding an allmodconfig+CONFIG_SMP=n"
build, the allmodconfig builds are rather expensive.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1677851

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-29 17:10 +0200
Message-ID<tXJeW-IJ-21@gated-at.bofh.it>
In reply to#1671482
>> +static int s500_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
>> +{
>> +       unsigned long timeout;
>> +       int ret;
>> +
>> +       ret = s500_wakeup_secondary(cpu);
>> +       if (ret)
>> +               return ret;
>> +
>> +       udelay(10);
>> +
>> +       spin_lock(&boot_lock);
>> +
>> +       /*
>> +        * The secondary processor is waiting to be released from
>> +        * the holding pen - release it, then wait for it to flag
>> +        * that it has been released by resetting pen_release.
>> +        */
>> +       write_pen_release(cpu_logical_map(cpu));
>> +       smp_send_reschedule(cpu);
>> +
>> +       timeout = jiffies + (1 * HZ);
>> +       while (time_before(jiffies, timeout)) {
>> +               if (pen_release == -1)
>> +                       break;
>> +       }
>> +
>> +       writel(0, timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>> +       writel(0, timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
>> +
>> +       spin_unlock(&boot_lock);
>> +
>> +       return pen_release != -1 ? -ENOSYS : 0;
>> +}
>
> This looks more complicated than necessary. Why do you need the holding
> pen when you have a register to start up the CPU?
>

It seems you missed my question here. Can you please follow up, and
if possible send a patch to remove the pen_release logic that appears
to be unnecessary here?

        Arnd

[toc] | [prev] | [next] | [standalone]

#1677867

FromAndreas Färber <afaerber@suse.de>
Date2017-06-29 17:30 +0200
Message-ID<tXJyj-P8-37@gated-at.bofh.it>
In reply to#1677851
Am 29.06.2017 um 17:07 schrieb Arnd Bergmann:
>>> +static int s500_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>> +{
>>> +       unsigned long timeout;
>>> +       int ret;
>>> +
>>> +       ret = s500_wakeup_secondary(cpu);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       udelay(10);
>>> +
>>> +       spin_lock(&boot_lock);
>>> +
>>> +       /*
>>> +        * The secondary processor is waiting to be released from
>>> +        * the holding pen - release it, then wait for it to flag
>>> +        * that it has been released by resetting pen_release.
>>> +        */
>>> +       write_pen_release(cpu_logical_map(cpu));
>>> +       smp_send_reschedule(cpu);
>>> +
>>> +       timeout = jiffies + (1 * HZ);
>>> +       while (time_before(jiffies, timeout)) {
>>> +               if (pen_release == -1)
>>> +                       break;
>>> +       }
>>> +
>>> +       writel(0, timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>> +       writel(0, timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
>>> +
>>> +       spin_unlock(&boot_lock);
>>> +
>>> +       return pen_release != -1 ? -ENOSYS : 0;
>>> +}
>>
>> This looks more complicated than necessary. Why do you need the holding
>> pen when you have a register to start up the CPU?
>>
> 
> It seems you missed my question here. Can you please follow up, and
> if possible send a patch to remove the pen_release logic that appears
> to be unnecessary here?

I do not have any documentation on these registers, only the downstream
code that I forward-ported here. If you tell me what you mean exactly, I
can do some testing and if it still works submit a patch to simplify it.

Comments from the so far quiet Actions Semi side would help, too.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1677890

FromArnd Bergmann <arnd@arndb.de>
Date2017-06-29 18:00 +0200
Message-ID<tXK1l-11t-17@gated-at.bofh.it>
In reply to#1677867
On Thu, Jun 29, 2017 at 5:22 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 29.06.2017 um 17:07 schrieb Arnd Bergmann:
>>
>> It seems you missed my question here. Can you please follow up, and
>> if possible send a patch to remove the pen_release logic that appears
>> to be unnecessary here?
>
> I do not have any documentation on these registers, only the downstream
> code that I forward-ported here. If you tell me what you mean exactly, I
> can do some testing and if it still works submit a patch to simplify it.
>
> Comments from the so far quiet Actions Semi side would help, too.

IIRC, there are two ways to implement SMP bootup: either you have
registers to tell the secondary CPU how to start up out of reset or they
get put into a holding pen during early boot where they spin waiting
for a variable to get written.

However, doing both is not necessary. See for example mach-sunxi
for an example without the holding pen. I think you can just delete
half of your file to do the same.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1679408 — [PATCH] ARM: owl: smp: Drop bogus holding pen

FromAndreas Färber <afaerber@suse.de>
Date2017-07-01 23:40 +0200
Subject[PATCH] ARM: owl: smp: Drop bogus holding pen
Message-ID<tYyhs-h9-7@gated-at.bofh.it>
In reply to#1677890
The S500 SoC can start secondary CPUs without busy-looping for pen_release,
so simplify the SMP code compared to the LeMaker kernel tree.

Fixes: 172067e0bc87 ("ARM: owl: Implement CPU enable-method for S500")
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Cc: David Liu <liuwei@actions-semi.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 arch/arm/mach-actions/headsmp.S | 20 ++------------------
 arch/arm/mach-actions/platsmp.c | 29 +----------------------------
 2 files changed, 3 insertions(+), 46 deletions(-)

diff --git a/arch/arm/mach-actions/headsmp.S b/arch/arm/mach-actions/headsmp.S
index dc4832fc101a..65f53bdb69e7 100644
--- a/arch/arm/mach-actions/headsmp.S
+++ b/arch/arm/mach-actions/headsmp.S
@@ -2,6 +2,8 @@
  * Copyright 2012 Actions Semi Inc.
  * Author: Actions Semi, Inc.
  *
+ * Copyright (c) 2017 Andreas Färber
+ *
  * 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;  either version 2 of the  License, or (at your
@@ -46,23 +48,5 @@ ENTRY(owl_v7_invalidate_l1)
 ENDPROC(owl_v7_invalidate_l1)
 
 ENTRY(owl_secondary_startup)
-	mrc	p15, 0, r0, c0, c0, 5
-	and	r0, r0, #0xf
-	adr	r4, 1f
-	ldmia	r4, {r5, r6}
-	sub	r4, r4, r5
-	add	r6, r6, r4
-pen:
-	ldr	r7, [r6]
-	cmp	r7, r0
-	bne	pen
-
-	/*
-	 * we've been released from the holding pen: secondary_stack
-	 * should now contain the SVC stack for this core
-	 */
 	bl	owl_v7_invalidate_l1
 	b	secondary_startup
-
-1:	.long	.
-	.long	pen_release
diff --git a/arch/arm/mach-actions/platsmp.c b/arch/arm/mach-actions/platsmp.c
index b4806ce0e9bb..12a9e331b432 100644
--- a/arch/arm/mach-actions/platsmp.c
+++ b/arch/arm/mach-actions/platsmp.c
@@ -41,26 +41,6 @@ static int ncores;
 
 static DEFINE_SPINLOCK(boot_lock);
 
-static void write_pen_release(int val)
-{
-	pen_release = val;
-	smp_wmb();
-	__cpuc_flush_dcache_area((void *)&pen_release, sizeof(pen_release));
-	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
-}
-
-static void s500_smp_secondary_init(unsigned int cpu)
-{
-	/*
-	 * let the primary processor know we're out of the
-	 * pen, then head off into the C entry point
-	 */
-	write_pen_release(-1);
-
-	spin_lock(&boot_lock);
-	spin_unlock(&boot_lock);
-}
-
 void owl_secondary_startup(void);
 
 static int s500_wakeup_secondary(unsigned int cpu)
@@ -115,12 +95,6 @@ static int s500_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 	spin_lock(&boot_lock);
 
-	/*
-	 * The secondary processor is waiting to be released from
-	 * the holding pen - release it, then wait for it to flag
-	 * that it has been released by resetting pen_release.
-	 */
-	write_pen_release(cpu_logical_map(cpu));
 	smp_send_reschedule(cpu);
 
 	timeout = jiffies + (1 * HZ);
@@ -134,7 +108,7 @@ static int s500_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 	spin_unlock(&boot_lock);
 
-	return pen_release != -1 ? -ENOSYS : 0;
+	return 0;
 }
 
 static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
@@ -192,7 +166,6 @@ static void __init s500_smp_prepare_cpus(unsigned int max_cpus)
 
 static const struct smp_operations s500_smp_ops __initconst = {
 	.smp_prepare_cpus = s500_smp_prepare_cpus,
-	.smp_secondary_init = s500_smp_secondary_init,
 	.smp_boot_secondary = s500_smp_boot_secondary,
 };
 CPU_METHOD_OF_DECLARE(s500_smp, "actions,s500-smp", &s500_smp_ops);
-- 
2.12.3

[toc] | [prev] | [next] | [standalone]

#1679965 — Re: [PATCH] ARM: owl: smp: Drop bogus holding pen

FromArnd Bergmann <arnd@arndb.de>
Date2017-07-03 14:40 +0200
SubjectRe: [PATCH] ARM: owl: smp: Drop bogus holding pen
Message-ID<tZ8NY-1sr-5@gated-at.bofh.it>
In reply to#1679408
On Sat, Jul 1, 2017 at 11:29 PM, Andreas Färber <afaerber@suse.de> wrote:
> The S500 SoC can start secondary CPUs without busy-looping for pen_release,
> so simplify the SMP code compared to the LeMaker kernel tree.
>
> Fixes: 172067e0bc87 ("ARM: owl: Implement CPU enable-method for S500")
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Cc: David Liu <liuwei@actions-semi.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Looks good to me. I applied both on top of next/soc before tagging the
branch for 4.13,
so this version is going to be what we merge.

I saw that David had another suggestion for simplifying it further.
That also looks
good, but it's less urgent, so please implement that as an add-on patch and send
it for 4.14, unless you decide there is a reason it needs to be
included in 4.13 as
well.

Thanks!

        Arnd

[toc] | [prev] | [next] | [standalone]

#1679264 — 答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

From刘炜 <liuwei@actions-semi.com>
Date2017-07-01 07:00 +0200
Subject答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500
Message-ID<tYiFI-6Cs-3@gated-at.bofh.it>
In reply to#1677867
Hi, Andrea

OWL_CPUx_ADDR is the physical address of CPUx wakeup function.
OWL_CPUx_FLAG is a valid flag of OWL_CPUx_ADDR. 

After CPUxs are wakeuped by SEV instruction, they will check their own OWL_CPUx_FLAG register. If the register vlaue is 0x55aa, CPUx will jump to OWL_CPUx_ADDR to boot up, otherwize go to sleep by WFE.

So the pen release staff is not necessary, you can remove these code safely.

BTW: CPU2/3 must exit the power gate state before wakeup, and CPU1 is always power on and has no power gate control.

Best Regards,
David Liu

-----邮件原件-----
发件人: Andreas Färber [mailto:afaerber@suse.de] 
发送时间: 2017年6月29日 23:22
收件人: Arnd Bergmann; Thomas Liau
抄送: Linux ARM; mp-cs; 张东风; 刘炜; 张天益; 96boards@ucrobotics.com; support@lemaker.org; Linux Kernel Mailing List; Russell King
主题: Re: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

Am 29.06.2017 um 17:07 schrieb Arnd Bergmann:
>>> +static int s500_smp_boot_secondary(unsigned int cpu, struct 
>>> +task_struct *idle) {
>>> +       unsigned long timeout;
>>> +       int ret;
>>> +
>>> +       ret = s500_wakeup_secondary(cpu);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       udelay(10);
>>> +
>>> +       spin_lock(&boot_lock);
>>> +
>>> +       /*
>>> +        * The secondary processor is waiting to be released from
>>> +        * the holding pen - release it, then wait for it to flag
>>> +        * that it has been released by resetting pen_release.
>>> +        */
>>> +       write_pen_release(cpu_logical_map(cpu));
>>> +       smp_send_reschedule(cpu);
>>> +
>>> +       timeout = jiffies + (1 * HZ);
>>> +       while (time_before(jiffies, timeout)) {
>>> +               if (pen_release == -1)
>>> +                       break;
>>> +       }
>>> +
>>> +       writel(0, timer_base_addr + OWL_CPU1_ADDR + (cpu - 1) * 4);
>>> +       writel(0, timer_base_addr + OWL_CPU1_FLAG + (cpu - 1) * 4);
>>> +
>>> +       spin_unlock(&boot_lock);
>>> +
>>> +       return pen_release != -1 ? -ENOSYS : 0; }
>>
>> This looks more complicated than necessary. Why do you need the 
>> holding pen when you have a register to start up the CPU?
>>
> 
> It seems you missed my question here. Can you please follow up, and if 
> possible send a patch to remove the pen_release logic that appears to 
> be unnecessary here?

I do not have any documentation on these registers, only the downstream code that I forward-ported here. If you tell me what you mean exactly, I can do some testing and if it still works submit a patch to simplify it.

Comments from the so far quiet Actions Semi side would help, too.

Regards,
Andreas

--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1679391 — Re: 答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

FromAndreas Färber <afaerber@suse.de>
Date2017-07-01 22:00 +0200
SubjectRe: 答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500
Message-ID<tYwIG-7BU-5@gated-at.bofh.it>
In reply to#1679264
Hi David,

Am 01.07.2017 um 06:42 schrieb 刘炜:
> OWL_CPUx_ADDR is the physical address of CPUx wakeup function.
> OWL_CPUx_FLAG is a valid flag of OWL_CPUx_ADDR. 
> 
> After CPUxs are wakeuped by SEV instruction, they will check their own OWL_CPUx_FLAG register. If the register vlaue is 0x55aa, CPUx will jump to OWL_CPUx_ADDR to boot up, otherwize go to sleep by WFE.
> 
> So the pen release staff is not necessary, you can remove these code safely.

Thank you for the quick confirmation!

I have just tested a patch, and it appears to work.

Is owl_v7_invalidate_l1 necessary? mach-sunxi (that Arnd pointed to as
example) uses secondary_startup directly, without custom assembler code.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-actions/headsmp.S

> BTW: CPU2/3 must exit the power gate state before wakeup, and CPU1 is always power on and has no power gate control.

Yes, the S500 SPS was luckily documented in the manual. The power-gating
for CPU2/3 is already implemented:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-actions/platsmp.c

For S900 however SPS is sadly not documented... If you know who at
Actions Semi could help there, please also take a look at:
https://github.com/96boards/documentation/issues/59

Best regards,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1679747

From刘炜 <liuwei@actions-semi.com>
Date2017-07-03 10:20 +0200
Message-ID<tZ4Km-6Tf-13@gated-at.bofh.it>
In reply to#1679391
Hi, Andreas

Actually the owl_v7_invalidate_l1 is not needed, and the headsmp-owl.S file can be removed if pen release staff is not used. The CPUx can jump to secondary_startup directly by setting OWL_CPUx_ADDR register.

Because S900 is aarch64 architecture, it cannot boot CPUx simply by setting SPS register. It depends on the low-level arm-trusted-firmware. Linux kernel can boot up CPUx by PSCI interface, and the arm-trusted-firmware will power on CPUx, initialize EL3 register and jump to EL2 entry in Linux kernel.

Best Regards,
David Liu

-----Original Message-----
From: Andreas Färber [mailto:afaerber@suse.de] 
Sent: Sunday, July 02, 2017 3:57 AM
To: 刘炜
Cc: Arnd Bergmann; Thomas Liau; Linux ARM; mp-cs; 张东风; 张天益; 96boards@ucrobotics.com; support@lemaker.org; Linux Kernel Mailing List; Russell King
Subject: Re: 答复: [PATCH v4 20/28] ARM: owl: Implement CPU enable-method for S500

Hi David,

Am 01.07.2017 um 06:42 schrieb 刘炜:
> OWL_CPUx_ADDR is the physical address of CPUx wakeup function.
> OWL_CPUx_FLAG is a valid flag of OWL_CPUx_ADDR. 
> 
> After CPUxs are wakeuped by SEV instruction, they will check their own OWL_CPUx_FLAG register. If the register vlaue is 0x55aa, CPUx will jump to OWL_CPUx_ADDR to boot up, otherwize go to sleep by WFE.
> 
> So the pen release staff is not necessary, you can remove these code safely.

Thank you for the quick confirmation!

I have just tested a patch, and it appears to work.

Is owl_v7_invalidate_l1 necessary? mach-sunxi (that Arnd pointed to as
example) uses secondary_startup directly, without custom assembler code.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-actions/headsmp.S

> BTW: CPU2/3 must exit the power gate state before wakeup, and CPU1 is always power on and has no power gate control.

Yes, the S500 SPS was luckily documented in the manual. The power-gating
for CPU2/3 is already implemented:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-actions/platsmp.c

For S900 however SPS is sadly not documented... If you know who at
Actions Semi could help there, please also take a look at:
https://github.com/96boards/documentation/issues/59

Best regards,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web