ARC rework SMP boot waiting to handle IO-Coherency case

PATCH In review Vineet Gupta 2 participants 11 messages Jan 16, 2017 – Jan 17, 2017 Synopsys ARC architecture

Share & tools

Vineet Gupta

author

2017-01-16 20:57 UTC #0 lore raw

cover ARC rework SMP boot waiting to handle IO-Coherency case

ARC SMP supports halt-on-reset and run-on-reset for non master cores. run-on-reset is applicable for internal bitfiles as well as debugger assisted boots (where all cores are started together)

run-on-reset uses a poll-shared-flag-in-mem-n-spin approach which is not efficient and moreover is wrong when IO-Coherency is being setup by Master in early boot, expecting absolutely NO noise on coherency unit by other cores.

To solve this we wither need to move all the SLC/IOC setup code in head.S which is super pain, or we make run-to-reset behave as if halt-on-reset with hardware assist to resume the non masters at right time.

Vineet Gupta (4):
ARC: smp-boot: waiting API for run-from-reset need not jump to entry
point
ARC: smp-boot: run-on-reset: add callback to allow non masters to wait
ARCv2: smp: MCIP: remove debug aid to halt all cores when one halts
ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non
master cpus

 arch/arc/include/asm/mcip.h |  1 +
 arch/arc/include/asm/smp.h  |  3 +++
 arch/arc/kernel/head.S      | 14 +++++++-------
 arch/arc/kernel/mcip.c      | 34 ++++++++++++++++++++++++++++++----
 arch/arc/kernel/smp.c       | 15 +++++++++++++--
 5 files changed, 54 insertions(+), 13 deletions(-)

Signature · 1 line

2.7.4

Vineet Gupta

author

2017-01-16 20:57 UTC #1 lore raw

PATCH 1/4 ARC: smp-boot: waiting API for run-from-reset need not jump to entry point

For run-on-reset SMP configs, non master cores call a routine which waits until Master gives it a "go" signal (currently using a shared mem flag). The same routine then jumps off the well known entry point of all non Master cores i.e. @first_lines_of_secondary

This patch moves the last part out waiting routine into one single place in early boot code.

This is better in terms of absraction (the wait codes only waits) and returns, leaving out the "start off to" part.

Moreover this makes way for alternate implementation of wait (using hardware assist) without having to duplicate the jump part.

In terms of implementation this requires some restructuring of the early boot code in head.S as Master now jumps to BSS setup explicitly, vs. falling thru into it before.

Signed-off-by Vineet Gupta <vgupta at synopsys.com>

---
 arch/arc/kernel/head.S | 14 +++++++-------
 arch/arc/kernel/smp.c  |  6 ++++--
 2 files changed, 11 insertions(+), 9 deletions(-)

arch/arc/kernel/head.S +7 -7

index 689dd867fdff..8b90d25a15cc 100644
@@ -71,14 +71,14 @@ ENTRY(stext)
 	GET_CPU_ID  r5
 	cmp	r5, 0
 	mov.nz	r0, r5
-#ifdef CONFIG_ARC_SMP_HALT_ON_RESET
-	; Non-Master can proceed as system would be booted sufficiently
-	jnz	first_lines_of_secondary
-#else
+	bz	.Lmaster_proceed
+
 	; Non-Masters wait for Master to boot enough and bring them up
-	jnz	arc_platform_smp_wait_to_boot
-#endif
-	; Master falls thru
+	; when they resume, tail-call to entry point
+	mov	blink, @first_lines_of_secondary
+	j	arc_platform_smp_wait_to_boot
+
+.Lmaster_proceed:
 #endif
 
 	; Clear BSS before updating any globals

arch/arc/kernel/smp.c +4 -2

index 88674d972c9d..44a0d21ed342 100644
@@ -98,14 +98,16 @@ static void arc_default_smp_cpu_kick(int cpu, unsigned long pc)
 
 void arc_platform_smp_wait_to_boot(int cpu)
 {
+	/* for halt-on-reset, we've waited already */
+	if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET))
+		return;
+
 	while (wake_flag != cpu)
 		;
 
 	wake_flag = 0;
-	__asm__ __volatile__("j @first_lines_of_secondary	\n");
 }
 
-
 const char *arc_platform_smp_cpuinfo(void)
 {
 	return plat_smp_ops.info ? : "";

Signature · 1 line

2.7.4

Vineet Gupta

author

2017-01-16 20:57 UTC #2 lore raw

PATCH 2/4 ARC: smp-boot: run-on-reset: add callback to allow non masters to wait

In reply to Vineet Gupta

Currently for halt-on-reset, non masters spin on a shared memory which is wiggled by Master at right time. This is not efficient when hardware support exists to stop and resume them from Master (using an external IP block). More importantly Master might be setting up SLC or IO-Coherency aperutes in early boot duting which other cores need NOT perturb the coherency unit, thus need a mechanism that doesn't rely on polling memory.

This just adds infrastructure for next patch which will add the hardware support (MCIP Inter Core Debug Unit).

Signed-off-by Vineet Gupta <vgupta at synopsys.com>

---
 arch/arc/include/asm/smp.h |  3 +++
 arch/arc/kernel/smp.c      | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

arch/arc/include/asm/smp.h +3 -0

index 0861007d9ef3..6187b9d14b03 100644
@@ -50,6 +50,8 @@ extern int smp_ipi_irq_setup(int cpu, irq_hw_number_t hwirq);
  * 			mach_desc->init_early()
  * @init_per_cpu:	Called for each core so SMP h/w block driver can do
  * 			any needed setup per cpu (e.g. IPI request)
+ * @cpu_wait:		Non masters wait to be resumed later by master (to avoid
+ * 			perturbing SLC / cache coherency in early boot)
  * @cpu_kick:		For Master to kickstart a cpu (optionally at a PC)
  * @ipi_send:		To send IPI to a @cpu
  * @ips_clear:		To clear IPI received at @irq

@@ -58,6 +60,7 @@ struct plat_smp_ops {
 	const char 	*info;
 	void		(*init_early_smp)(void);
 	void		(*init_per_cpu)(int cpu);
+	void		(*cpu_wait)(int cpu);
 	void		(*cpu_kick)(int cpu, unsigned long pc);
 	void		(*ipi_send)(int cpu);
 	void		(*ipi_clear)(int irq);

arch/arc/kernel/smp.c +13 -4

index 44a0d21ed342..e60c11b8f4b9 100644
@@ -96,16 +96,25 @@ static void arc_default_smp_cpu_kick(int cpu, unsigned long pc)
 	wake_flag = cpu;
 }
 
+static void arc_default_smp_wait_to_boot(int cpu)
+{
+	while (wake_flag != cpu)
+		;
+
+	wake_flag = 0;
+}
+
 void arc_platform_smp_wait_to_boot(int cpu)
 {
 	/* for halt-on-reset, we've waited already */
 	if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET))
 		return;
 
-	while (wake_flag != cpu)
-		;
-
-	wake_flag = 0;
+	/* platform callback might fail */
+	if (plat_smp_ops.cpu_wait)
+		plat_smp_ops.cpu_wait(cpu);
+	else
+		arc_default_smp_wait_to_boot(cpu);
 }
 
 const char *arc_platform_smp_cpuinfo(void)

Signature · 1 line

2.7.4

Alexey Brodkin

2017-01-17 20:58 UTC #3 lore raw

RE: [PATCH 2/4] ARC: smp-boot: run-on-reset: add callback to allow non masters to wait

Hi Vineet,

Quoted 53 lines

-----Original Message-----
From: Vineet Gupta
Sent: Monday, January 16, 2017 11:58 PM
To: linux-snps-arc at lists.infradead.org; Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-kernel at vger.kernel.org; Vineet Gupta <vgupta at synopsys.com>
Subject: [PATCH 2/4] ARC: smp-boot: run-on-reset: add callback to allow non masters to wait
Currently for halt-on-reset, non masters spin on a shared memory which is wiggled by Master at right time. This is not efficient when hardware support exists to stop and resume them from Master (using an external IP block). More importantly Master might be setting up SLC or IO-Coherency aperutes in early boot duting which other cores need NOT perturb the coherency unit, thus need a mechanism that doesn't rely on polling memory.
This just adds infrastructure for next patch which will add the hardware support (MCIP Inter Core Debug Unit).
Signed-off-by Vineet Gupta <vgupta at synopsys.com>
---
 arch/arc/include/asm/smp.h |  3 +++
 arch/arc/kernel/smp.c      | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)
arch/arc/include/asm/smp.h index renamed from arch/arc/include/asm/smp.h +0 -0
0861007d9ef3..6187b9d14b03 100644
arch/arc/include/asm/smp.h +1 -0
@@ -50,6 +50,8 @@ extern int smp_ipi_irq_setup(int cpu,
 irq_hw_number_t hwirq);
  * 			mach_desc->init_early()
  * @init_per_cpu:	Called for each core so SMP h/w block driver can do
  * 			any needed setup per cpu (e.g. IPI request)
+ * @cpu_wait:		Non masters wait to be resumed later by
master (to avoid
+ * perturbing SLC / cache coherency in early boot)
* @cpu_kick: For Master to kickstart a cpu (optionally at a PC)
* @ipi_send: To send IPI to a @cpu
* @ips_clear: To clear IPI received at @irq
arch/arc/include/asm/smp.h +1 -0
@@ -58,6 +60,7 @@ struct plat_smp_ops {
 	const char 	*info;
 	void		(*init_early_smp)(void);
 	void		(*init_per_cpu)(int cpu);
+	void		(*cpu_wait)(int cpu);
 	void		(*cpu_kick)(int cpu, unsigned long pc);
 	void		(*ipi_send)(int cpu);
 	void		(*ipi_clear)(int irq);
arch/arc/kernel/smp.c index renamed from arch/arc/kernel/smp.c +0 -0
44a0d21ed342..e60c11b8f4b9 100644

arch/arc/kernel/smp.c static void arc_default_smp_cpu_kick(int cpu, · line 104

Quoted 9 lines

arch/arc/kernel/smp.c +5 -0

@@ -96,16 +96,25 @@ static void arc_default_smp_cpu_kick(int cpu,
 unsigned long pc)
 	wake_flag = cpu;
 }
 
+static void arc_default_smp_wait_to_boot(int cpu) {
+	while (wake_flag != cpu)
+		;
+
+	wake_flag = 0;

Why don't we convert "wake_flag" into bit-field so each core uses its special bit.
It is IMHO beneficial for 2 reasons:
1. If we ever decide to have master core with ARCID != 0 implementation of that procedure won't change, because "wake_flag" for core with ARCID=0 will be 1 but not 0, see for example http://patchwork.ozlabs.org/patch/703645/

2. There's no need in resetting "wake_flag" to 0 at all as well because each core has its own bit and they not affect anybody else. And in that case ...

arch/arc/kernel/smp.c static void arc_default_smp_cpu_kick(int cpu, unsigned long pc) · line 111

Quoted 6 lines

arch/arc/kernel/smp.c +2 -0

+}
+
 void arc_platform_smp_wait_to_boot(int cpu)  {
 	/* for halt-on-reset, we've waited already */
 	if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET))
 		return;

...we may just remove above part. Master core by that time has already set our bit in "wake_flag" so we will effectively fall through the following "while".

arch/arc/kernel/smp.c static void arc_default_smp_cpu_kick(int cpu, unsigned long pc) · line 106

Quoted 2 lines

arch/arc/kernel/smp.c +0 -2
-	while (wake_flag != cpu)
-		;

-Alexey

Vineet Gupta

author

2017-01-17 22:02 UTC #4 lore raw

Re: [PATCH 2/4] ARC: smp-boot: run-on-reset: add callback to allow non masters to wait

Quoted 11 lines · Alexey Brodkin

arch/arc/kernel/smp.c static void arc_default_smp_cpu_kick(int cpu, unsigned long pc) · line 104
Quoted 6 lines
arch/arc/kernel/smp.c +5 -0
+static void arc_default_smp_wait_to_boot(int cpu) {
+	while (wake_flag != cpu)
+		;
+
+	wake_flag = 0;
Why don't we convert "wake_flag" into bit-field so each core uses its special bit.
It is IMHO beneficial for 2 reasons:
1. If we ever decide to have master core with ARCID != 0 implementation of that procedure won't change, because "wake_flag" for core with ARCID=0 will be 1 but not 0, see for example http://patchwork.ozlabs.org/patch/703645/

That's not a real use case - it is just a debug exercise ...

Quoted 3 lines

2. There's no need in resetting "wake_flag" to 0 at all as well because each core has its own bit and they not affect anybody else. And in that case ...

True, but you need to do a read-modify-write. More importantly, the cores are setup one at a time by the master - so there just was no need to do this to begin with - one at a time was just sufficient. If you really want to do this in right way - it will not a bit filed either, it needs to be a strictly per cpu variable.

Quoted 9 lines

arch/arc/kernel/smp.c static void arc_default_smp_cpu_kick(int cpu, unsigned long pc) · line 111
Quoted 6 lines
arch/arc/kernel/smp.c +2 -0
+}
+
 void arc_platform_smp_wait_to_boot(int cpu)  {
 	/* for halt-on-reset, we've waited already */
 	if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET))
 		return;
...we may just remove above part. Master core by that time has already set our bit in "wake_flag" so we will effectively fall through the following "while".

No. They way this works is, same routine arc_platform_smp_wait_to_boot() is called from early boot code for both halt-on-reset and run-on-reset. For latter we need to actually wait. For former, they were already halted and when they land here, they've waited enough so we need to return !

This is same as what was before, I've just moved the #ifdef from head.s (where it looked ugly) to here.

-Vineet

Vineet Gupta

author

2017-01-16 20:57 UTC #5 lore raw

PATCH 3/4 ARCv2: smp: MCIP: remove debug aid to halt all cores when one halts

In reply to Vineet Gupta

This was really usefull when doing initial bringup and also for occassional debug of software and hardware bugs. However in the new smp-boot regime, non masters will "self" halt even for run-on-reset configs. This will misinteract with the debug feature as in even master will get halted when the non masters self halt.

Signed-off-by Vineet Gupta <vgupta at synopsys.com>

---
 arch/arc/kernel/mcip.c | 5 -----
 1 file changed, 5 deletions(-)

arch/arc/kernel/mcip.c +0 -5

index f39142acc89e..933382e0edd0 100644
@@ -101,11 +101,6 @@ static void mcip_probe_n_setup(void)
 		IS_AVAIL1(mp.gfrc, "GFRC"));
 
 	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;
-
-	if (mp.dbg) {
-		__mcip_cmd_data(CMD_DEBUG_SET_SELECT, 0, 0xf);
-		__mcip_cmd_data(CMD_DEBUG_SET_MASK, 0xf, 0xf);
-	}
 }
 
 struct plat_smp_ops plat_smp_ops = {

Signature · 1 line

2.7.4

Alexey Brodkin

2017-01-17 20:13 UTC #6 lore raw

RE: [PATCH 3/4] ARCv2: smp: MCIP: remove debug aid to halt all cores when one halts

Hi Vineet,

Quoted 25 lines

-----Original Message-----
From: Vineet Gupta
Sent: Monday, January 16, 2017 11:58 PM
To: linux-snps-arc at lists.infradead.org; Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-kernel at vger.kernel.org; Vineet Gupta <vgupta at synopsys.com>
Subject: [PATCH 3/4] ARCv2: smp: MCIP: remove debug aid to halt all cores when one halts
This was really usefull when doing initial bringup and also for occassional debug of software and hardware bugs. However in the new smp-boot regime, non masters will "self" halt even for run-on-reset configs. This will misinteract with the debug feature as in even master will get halted when the non masters self halt.
Signed-off-by Vineet Gupta <vgupta at synopsys.com>
---
 arch/arc/kernel/mcip.c | 5 -----
 1 file changed, 5 deletions(-)
arch/arc/kernel/mcip.c index renamed from arch/arc/kernel/mcip.c +0 -0
f39142acc89e..933382e0edd0 100644

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 104

Quoted 9 lines

arch/arc/kernel/mcip.c +0 -5

@@ -101,11 +101,6 @@ static void mcip_probe_n_setup(void)
 		IS_AVAIL1(mp.gfrc, "GFRC"));
 
 	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;
-
-	if (mp.dbg) {
-		__mcip_cmd_data(CMD_DEBUG_SET_SELECT, 0, 0xf);
-		__mcip_cmd_data(CMD_DEBUG_SET_MASK, 0xf, 0xf);
-	}
 }

I've been doing that for quite some time locally because that used to be a must for execution of SMP vmlinux in "pseudo"-UP mode, i.e. on SMP hardware but with only 1 core being used.

I'm really happy to see this commit upstream - will definitely make life a bit simpler.

-Alexey

Vineet Gupta

author

2017-01-16 20:57 UTC #7 lore raw

PATCH 4/4 ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non master cpus

In reply to Vineet Gupta

This essentially converts a run-on-reset to halt-on-reset - so non masters self halt in early boot code. And later they are resumed from halted PC using MCIP ICD assist.

As mentioned in prev commits, this paves way for radio silence on coherency unit, while master is setting up IOC and such.

Signed-off-by Vineet Gupta <vgupta at synopsys.com>

---
 arch/arc/include/asm/mcip.h |  1 +
 arch/arc/kernel/mcip.c      | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

arch/arc/include/asm/mcip.h +1 -0

index c8fbe4114bad..a6ae4363c388 100644
@@ -36,6 +36,7 @@ struct mcip_cmd {
 #define CMD_SEMA_CLAIM_AND_READ		0x11
 #define CMD_SEMA_RELEASE		0x12
 
+#define CMD_DEBUG_RUN			0x33
 #define CMD_DEBUG_SET_MASK		0x34
 #define CMD_DEBUG_SET_SELECT		0x36

arch/arc/kernel/mcip.c +31 -0

index 933382e0edd0..0a1822d2fe52 100644
@@ -103,12 +103,43 @@ static void mcip_probe_n_setup(void)
 	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;
 }
 
+static void __init mcip_cpu_wait(int cpu)
+{
+	struct mcip_bcr mp;
+
+	READ_BCR(ARC_REG_MCIP_BCR, mp);
+
+	/*
+	 * self halt for waiting as Master will resume us using MCIP ICD assist
+	 * Note: if ICD is not configured, we are hosed, but panic here is
+	 *       not going to help as UART access might not even work
+	 */
+	if (mp.dbg)
+		asm volatile("flag 1	\n");
+}
+
+static void __init mcip_cpu_kick(int cpu, unsigned long pc)
+{
+	struct mcip_bcr mp;
+
+	READ_BCR(ARC_REG_MCIP_BCR, mp);
+
+	if (mp.dbg)
+		__mcip_cmd_data(CMD_DEBUG_RUN, 0, (1 << cpu));
+	else
+		panic("SMP boot issues: MCIP lacks ICD\n");
+}
+
 struct plat_smp_ops plat_smp_ops = {
 	.info		= smp_cpuinfo_buf,
 	.init_early_smp	= mcip_probe_n_setup,
 	.init_per_cpu	= mcip_setup_per_cpu,
 	.ipi_send	= mcip_ipi_send,
 	.ipi_clear	= mcip_ipi_clear,
+	.cpu_kick	= mcip_cpu_kick,
+#ifndef CONFIG_ARC_SMP_HALT_ON_RESET
+	.cpu_wait	= mcip_cpu_wait,
+#endif
 };
 
 #endif

Signature · 1 line

2.7.4

Alexey Brodkin

2017-01-17 21:41 UTC #8 lore raw

RE: [PATCH 4/4] ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non master cpus

Hi Vineet,

Quoted 39 lines

-----Original Message-----
From: Vineet Gupta
Sent: Monday, January 16, 2017 11:58 PM
To: linux-snps-arc at lists.infradead.org; Alexey Brodkin <abrodkin at synopsys.com>
Cc: linux-kernel at vger.kernel.org; Vineet Gupta <vgupta at synopsys.com>
Subject: [PATCH 4/4] ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non master cpus
This essentially converts a run-on-reset to halt-on-reset - so non masters self halt in early boot code. And later they are resumed from halted PC using MCIP ICD assist.
As mentioned in prev commits, this paves way for radio silence on coherency unit, while master is setting up IOC and such.
Signed-off-by Vineet Gupta <vgupta at synopsys.com>
---
 arch/arc/include/asm/mcip.h |  1 +
 arch/arc/kernel/mcip.c      | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
arch/arc/include/asm/mcip.h +1 -0
index c8fbe4114bad..a6ae4363c388 100644
@@ -36,6 +36,7 @@ struct mcip_cmd {
 #define CMD_SEMA_CLAIM_AND_READ		0x11
 #define CMD_SEMA_RELEASE		0x12
 
+#define CMD_DEBUG_RUN			0x33
 #define CMD_DEBUG_SET_MASK		0x34
 #define CMD_DEBUG_SET_SELECT		0x36
 
arch/arc/kernel/mcip.c index renamed from arch/arc/kernel/mcip.c +0 -0
933382e0edd0..0a1822d2fe52 100644

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 105

Quoted 3 lines

arch/arc/kernel/mcip.c +1 -0

@@ -103,12 +103,43 @@ static void mcip_probe_n_setup(void)
 	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;  }
 
+static void __init mcip_cpu_wait(int cpu) {

Has this one passed checkpatch? Above "{" on the same line as function name and closing one merged with the previous line look strange.

At least here it is said that way:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n129

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 118

Quoted 11 lines

arch/arc/kernel/mcip.c +11 -0

+	struct mcip_bcr mp;
+
+	READ_BCR(ARC_REG_MCIP_BCR, mp);
+
+	/*
+	 * self halt for waiting as Master will resume us using MCIP ICD assist
+	 * Note: if ICD is not configured, we are hosed, but panic here is
+	 *       not going to help as UART access might not even work
+	 */
+	if (mp.dbg)
+		asm volatile("flag 1	\n");

Are you sure that won't trigger MDB stop?
I would imagine if MDB saw coreX running and then it unexpectedly [for MDB] gets halted MDB stops, no? Essentially I'm talking about properly set CPMD session.

I have no board handy ATM so just thinking out loud.

Quoted 10 lines

arch/arc/kernel/mcip.c +10 -0

+}
+
+static void __init mcip_cpu_kick(int cpu, unsigned long pc) {
+	struct mcip_bcr mp;
+
+	READ_BCR(ARC_REG_MCIP_BCR, mp);
+
+	if (mp.dbg)
+		__mcip_cmd_data(CMD_DEBUG_RUN, 0, (1 << cpu));
+	else

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 140

Quoted 10 lines

arch/arc/kernel/mcip.c +4 -0

+		panic("SMP boot issues: MCIP lacks ICD\n"); }
+
 struct plat_smp_ops plat_smp_ops = {
 	.info		= smp_cpuinfo_buf,
 	.init_early_smp	= mcip_probe_n_setup,
 	.init_per_cpu	= mcip_setup_per_cpu,
 	.ipi_send	= mcip_ipi_send,
 	.ipi_clear	= mcip_ipi_clear,
+	.cpu_kick	= mcip_cpu_kick,
+#ifndef CONFIG_ARC_SMP_HALT_ON_RESET

I really hate compile-time defined stuff and would prefer to remove most of that stuff at least in ARC code instaed of adding more items that stops us from using the same binary on wider range of ARC cores.

In 2/4 you already do check if core was configured [actually Linux kernel was configured but not the HW] -------------------------->8-------------------------
if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET)) -------------------------->8-------------------------
so "plat_smp_ops.cpu_wait(cpu)" won't be executed anyways.

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 143

Quoted 3 lines

arch/arc/kernel/mcip.c +2 -0
+	.cpu_wait	= mcip_cpu_wait,
+#endif
 };

So why don't we implement it all much simpler regardless CONFIG_ARC_SMP_HALT_ON_RESET? Like that:
-------------------------->8-------------------------
static void __init mcip_cpu_wait(int cpu)

{
	struct mcip_bcr mp;

	/* Check if master has already set "wake_flag" wanting us to run */
	if (wake_flag != cpu) { // or similar construction if we switch to bitfield

		READ_BCR(ARC_REG_MCIP_BCR, mp);

		/*
		 * self halt for waiting as Master will resume us using MCIP ICD assist
		 * Note: if ICD is not configured, we are hosed, but panic here is
		 *       not going to help as UART access might not even work
		 */
		if (mp.dbg)
			asm volatile("flag 1	\n");
	}
}

-------------------------->8-------------------------

And I think we may then keep mcip_cpu_kick() as it is.
In ARConnect databook there's no mention of side-effects for CMD_DEBUG_RUN being used against already running core.

IMHO with this approach we'll be able to handle cases when [pre-]bootloader inverted HALT/RUN_ON_RESET state.

-Alexey

Vineet Gupta

author

2017-01-17 22:14 UTC #9 lore raw

Re: [PATCH 4/4] ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non master cpus

Quoted 3 lines · Alexey Brodkin

Has this one passed checkpatch? Above "{" on the same line as function name and closing one merged with the previous line look strange.

Nope - I didn't :-(
I will fix it. Thx for spotting this.

Quoted 5 lines

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 118
Quoted 3 lines
arch/arc/kernel/mcip.c +3 -0
+	 */
+	if (mp.dbg)
+		asm volatile("flag 1	\n");
Are you sure that won't trigger MDB stop?

Yes and why is that a problem. mdb can start all cores and they are free to self halt themselves for any reason whatsoever. In corresponding disassembly window(s), such cores will be parked on the flag 1 instruction.

Quoted 4 lines

I would imagine if MDB saw coreX running and then it unexpectedly [for MDB] gets halted MDB stops, no? Essentially I'm talking about properly set CPMD session.
I have no board handy ATM so just thinking out loud.

This series has been smoke tested on AXS103 hardware with quad core bitfile.

Quoted 6 lines

arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 140
Quoted 2 lines
arch/arc/kernel/mcip.c +2 -0
+	.cpu_kick	= mcip_cpu_kick,
+#ifndef CONFIG_ARC_SMP_HALT_ON_RESET
I really hate compile-time defined stuff and would prefer to remove most of that stuff at least in ARC code instaed of adding more items that stops us from using the same binary on wider range of ARC cores.

Right, I live by that mantra too - but halt-on-reset and run-reset is not something we can derive from any build info - it is SoC specific. And unfortunately they imply different semantics.

Quoted 10 lines

In 2/4 you already do check if core was configured [actually Linux kernel was configured but not the HW] -------------------------->8-------------------------
if (IS_ENABLED(CONFIG_ARC_SMP_HALT_ON_RESET)) -------------------------->8-------------------------
so "plat_smp_ops.cpu_wait(cpu)" won't be executed anyways.
arch/arc/kernel/mcip.c static void mcip_probe_n_setup(void) · line 143
Quoted 3 lines
arch/arc/kernel/mcip.c +2 -0
+	.cpu_wait	= mcip_cpu_wait,
+#endif
 };

As I mention in the prev reply, #ifdef in 2/4 is independent of any MCIP wait or not - it needs to be there for a different reason. Here, the #ifdef ensured we don't plug in MCIP specific wait routine - which will halt the cores - which is not needed if they already halted on reset.

Quoted 13 lines

So why don't we implement it all much simpler regardless CONFIG_ARC_SMP_HALT_ON_RESET? Like that:
-------------------------->8-------------------------
static void __init mcip_cpu_wait(int cpu)
{
	struct mcip_bcr mp;

	/* Check if master has already set "wake_flag" wanting us to run */
	if (wake_flag != cpu) { // or similar construction if we switch to bitfield

		READ_BCR(ARC_REG_MCIP_BCR, mp);

		/*

Quoted 8 lines

		 * self halt for waiting as Master will resume us using MCIP ICD assist
		 * Note: if ICD is not configured, we are hosed, but panic here is
		 *       not going to help as UART access might not even work
		 */
		if (mp.dbg)
			asm volatile("flag 1	\n");
	}
}

My design is to keep 2 implementations if wait-wake protocol.
In absence of mcip ICD hardware assist, there is the original polling on wake_flag based mechanism. If mcip ICD exists we use that instead and do away with any polling on memory. This is specially important since for IOC setup the polling on memory just can;t be done as it causes traffic on coherency fabric. Arguably we could uncached polling but why even bother.

We don't mix the 2 approaches - which you seem to be implying here.

Quoted 10 lines

-------------------------->8-------------------------
And I think we may then keep mcip_cpu_kick() as it is.
In ARConnect databook there's no mention of side-effects for CMD_DEBUG_RUN being used against already running core.
IMHO with this approach we'll be able to handle cases when [pre-]bootloader inverted HALT/RUN_ON_RESET state.
-Alexey

Vineet Gupta

author

2017-01-17 22:16 UTC #10 lore raw

Re: [PATCH 4/4] ARCv2: smp-boot: MCIP: use Inter-Core-Debug unit to kick start non master cpus

Quoted 5 lines · Vineet Gupta

Quoted 2 lines
Has this one passed checkpatch? Above "{" on the same line as function name and closing one merged with the previous line look strange.
Nope - I didn't :-(
I will fix it. Thx for spotting this.

Is there some issue with ur mailer - my patch seems the { to be indented properly Check the patch on mailing list !

-Vineet