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

[Vineet Gupta]

On 01/17/2017 02:14 PM, Vineet Gupta wrote:
>> 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

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

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

[Vineet Gupta]

On 01/17/2017 01:41 PM, Alexey Brodkin wrote:
> 
> 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.

>> + */
>> +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.

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


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

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


> 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");
>   }
> }

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.


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


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

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

[Vineet Gupta]

On 01/17/2017 12:58 PM, Alexey Brodkin wrote:
>>
>> +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 ...

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

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

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

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

[Alexey Brodkin]

Hi Vineet,

> -Original Message-
> From: Vineet Gupta
> Sent: Monday, January 16, 2017 11:58 PM
> To: linux-snps-arc@lists.infradead.org; Alexey Brodkin
> 
> Cc: linux-ker...@vger.kernel.org; Vineet Gupta 
> 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 
> ---
>  arch/arc/include/asm/mcip.h |  1 +
>  arch/arc/kernel/mcip.c  | 31 +++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/arch/arc/include/asm/mcip.h b/arch/arc/include/asm/mcip.h
> index c8fbe4114bad..a6ae4363c388 100644
> --- a/arch/arc/include/asm/mcip.h
> +++ b/arch/arc/include/asm/mcip.h
> @@ -36,6 +36,7 @@ struct mcip_cmd {
>  #define CMD_SEMA_CLAIM_AND_READ  0x11
>  #define CMD_SEMA_RELEASE 0x12
> 
> +#define CMD_DEBUG_RUN0x33
>  #define CMD_DEBUG_SET_MASK   0x34
>  #define CMD_DEBUG_SET_SELECT 0x36
> 
> diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c index
> 933382e0edd0..0a1822d2fe52 100644
> --- a/arch/arc/kernel/mcip.c
> +++ b/arch/arc/kernel/mcip.c
> @@ -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

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

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

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.

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

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

[Alexey Brodkin]

Hi Vineet,

> -Original Message-
> From: Vineet Gupta
> Sent: Monday, January 16, 2017 11:58 PM
> To: linux-snps-arc@lists.infradead.org; Alexey Brodkin
> 
> Cc: linux-ker...@vger.kernel.org; Vineet Gupta 
> 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 
> ---
>  arch/arc/include/asm/smp.h |  3 +++
>  arch/arc/kernel/smp.c  | 17 +
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arc/include/asm/smp.h b/arch/arc/include/asm/smp.h index
> 0861007d9ef3..6187b9d14b03 100644
> --- a/arch/arc/include/asm/smp.h
> +++ b/arch/arc/include/asm/smp.h
> @@ -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);
> diff --git a/arch/arc/kernel/smp.c b/arch/arc/kernel/smp.c index
> 44a0d21ed342..e60c11b8f4b9 100644
> --- a/arch/arc/kernel/smp.c
> +++ b/arch/arc/kernel/smp.c
> @@ -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 ...

> +}
> +
>  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".

> - while (wake_flag != cpu)
> - ;

-Alexey


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

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

[Alexey Brodkin]

Hi Vineet,

> -Original Message-
> From: Vineet Gupta
> Sent: Monday, January 16, 2017 11:58 PM
> To: linux-snps-arc@lists.infradead.org; Alexey Brodkin
> 
> Cc: linux-ker...@vger.kernel.org; Vineet Gupta 
> 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 
> ---
>  arch/arc/kernel/mcip.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c index
> f39142acc89e..933382e0edd0 100644
> --- a/arch/arc/kernel/mcip.c
> +++ b/arch/arc/kernel/mcip.c
> @@ -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


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc