[U-Boot] [PATCH v3 3/5] ls102xa: HYP/non-sec: support for ls102xa boards

[Li.Xiubo at freescale.com]

Hi Albert,

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Tuesday, November 18, 2014 3:18 PM
> To: Xiubo Li-B47053
> Cc: Sun York-R58495; Jin Zhengxiong-R64188; u-boot at lists.denx.de
> Subject: Re: [PATCH v3 3/5] ls102xa: HYP/non-sec: support for ls102xa boards
> 
> Hello Li.Xiubo at freescale.com,
> 
> On Tue, 18 Nov 2014 02:01:02 +0000, Li.Xiubo at freescale.com
> <Li.Xiubo at freescale.com> wrote:
> > Hi Albert,
> >
> >
> > > > > > > > +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> > > > > > > > +/* Setting the address at which secondary cores start from.*/
> > > > > > > > +void smp_set_core_boot_addr(unsigned long addr, int corenr)
> > > > > > > > +{
> > > > > > > > +	struct ccsr_gur __iomem *gur = (void
> *)(CONFIG_SYS_FSL_GUTS_ADDR);
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * After setting the secondary cores start address,
> > > > > > > > +	 * just release them to boot.
> > > > > > > > +	 */
> > > > > > > > +	out_be32(&gur->scratchrw[0], addr);
> > > > > > > > +	out_be32(&gur->brrl, 0x2);
> > > > > > > > +}
> > > > > > >
> > > > > > > This function does not exactly "[set] the address at which
> secondary
> > > > > > > cores start from"; it sets *a* secondary core's boot address, and
> then
> > > > > > > it *boots* it.
> > > > > > >
> > > > > >
> > > > > > Okay, I will fix it later.
> > > > > >
> > > > > > > Why does this version of smp_set_core_boot_addr() need to boot the
> > > core
> > > > > > > in addition to setting the address, whereas the existing ones in
> > > > > > > virt_v7, vexpress_common and arndale don't boot the cores?
> > > > > > >
> > > > > >
> > > > > > Yes, they don't doing the release operation.
> > > > > >
> > > > > > For Low Power Management requirement, maybe only one core will be
> used,
> > > and
> > > > > then
> > > > > > We also make sure that the secondary core must be in low power and
> deep
> > > > > sleep
> > > > > > mode(using wfi). So I just release it here, to make sure that the
> wfi
> > > > > instruction
> > > > > > will be executed as early as possible.
> > > > >
> > > > > Right after smp_set_core_boot_addr() is called, kick_all_cpus()
> isgoing
> > > > > to be called. Wouldn't that boot your CPUs just as well?
> > > > >
> > > >
> > > > Yes, it will.
> > > >
> > > > But before that we must do the holdoff bit set operation as the SoC's
> > > requirement.
> > > >
> > > > The BRR contains control bits for enabling boot for each core. On
> exiting
> > > HRESET or
> > > > PORESET, the RCW BOOT_HO field optionally allows for logical core 0 to
> be
> > > released
> > > > for booting or to remain in boot holdoff. All other cores remain in boot
> > > holdoff until their
> > > > corresponding bit is set.
> > > >
> > > > Maybe the comment is not very clear and a bit confusing.
> > >
> > > Before I'm lost entirely, do you mean that the comment:
> > >
> > > > > > > > +	/*
> > > > > > > > +	 * After setting the secondary cores start address,
> > > > > > > > +	 * just release them to boot.
> > > > > > > > +	 */
> > >
> > > Is actually wrong, and the instructions that follow it do not actually
> > > boot the secondary core(s)?
> > >
> >
> > The comment should be:
> >     /*
> >      * After setting the secondary core's start address,
> >      * just release it from holdoff.
> >      */
> > From my tests, for most time the release instructions will boot the
> secondary
> > core(s) without smp_kick_all_cpus(). One time has failed.
> >
> > So I think the release can not make sure that it will boot the secondary
> core(s).
> 
> Thanks for clarifying.
> 
> If a holdoff release is the right way to boot a secondary core for you,
> then I think the right place to do it is not smp_set_core_boot_addr()
> but smp_kick_all_cpus(), of which you could make a strong version which
> would do the holdoff release instead of whatever the weak version does.
> 

Yes, I do think a strong version will be okay.

In file arch/arm/cpu/armv7/ls102xa/cpu.c, add the strong version:

+/* Release the secondary core from holdoff state and boot it */
+void smp_kick_all_cpus(void)
+{
+       struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
+
+       out_be32(&gur->brrl, 0x2);
+}
+
Is this okay ?

I have test the holdoff release in two boards(including the old one before
I used) for 37 times and all has passed. I have a check the before failed logs,
It is another issue led to the failure. And also get confirmation that the
Holdoff release will do reset and then boot the secondary core.

Thanks,

BRs
Xiubo


> That is, unless you also need the weak version to run, in which case
> I /still/ think you should find a way for all of this to happen at the
> 'kick" stage (maybe with a .weakref in arch/arm/cpu/armv8/start.S, to
> give the weak smp_kick_all_cpus symbol another name that could be
> referred to by the strong version).
> 
> > Thanks,
> >
> > BRs
> > Xiubo
> 
> Amicalement,
> --
> Albert.