[U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init()

[Albert ARIBAUD]

Hi Michal,

On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek
<michal.simek at xilinx.com> wrote:

> Hi Albert,
> 
> On 10/03/2013 10:41 AM, Albert ARIBAUD wrote:
> > Hi Michal,
> > 
> > On Thu, 03 Oct 2013 08:58:38 +0200, Michal Simek <monstr at monstr.eu>
> > wrote:
> > 
> >> On 10/02/2013 09:43 PM, Albert ARIBAUD wrote:
> >>> Hi Michal,
> >>>
> >>> On Tue, 24 Sep 2013 12:38:38 +0200, Michal Simek <monstr at monstr.eu>
> >>> wrote:
> >>>
> >>>> Hi Albert,
> >>>>
> >>>> On 09/23/2013 04:37 PM, Albert ARIBAUD wrote:
> >>>>> Hi Michal,
> >>>>>
> >>>>> On Mon, 23 Sep 2013 16:19:52 +0200, Michal Simek <monstr at monstr.eu>
> >>>>> wrote:
> >>>>>
> >>>>>> On 09/23/2013 02:31 PM, Albert ARIBAUD wrote:
> >>>>>>> Hi Michal,
> >>>>>>>
> >>>>>>> On Thu, 22 Aug 2013 14:52:02 +0200, Michal Simek
> >>>>>>> <michal.simek at xilinx.com> wrote:
> >>>>>>>
> >>>>>>>> Zynq lowlevel_init() was implemented in C but stack
> >>>>>>>> pointer is setup after function call in _main().
> >>>>>>>> Move architecture setup to arch_cpu_init() which is call
> >>>>>>>> as the first function in board_init_f() which
> >>>>>>>> already have correct stack pointer.
> >>>>>>>>
> >>>>>>>> Reported-by: Sven Schwermer <sven.schwermer at tuhh.de>
> >>>>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
> >>>>>>>> ---
> >>>>>>>> I can't see any problem to call zynq setup a little
> >>>>>>>> bit later. There is already expectation that u-boot
> >>>>>>>> runs from DDR.
> >>>>>>>> Moving lowlevel_init from C to ASM is possible but
> >>>>>>>> I will have to introduce new macros with hardcoded
> >>>>>>>> values. Using structures is much nicer.
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>>  arch/arm/cpu/armv7/zynq/cpu.c | 6 ++++++
> >>>>>>>>  1 file changed, 6 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/arm/cpu/armv7/zynq/cpu.c b/arch/arm/cpu/armv7/zynq/cpu.c
> >>>>>>>> index 4367d1a..8846f30 100644
> >>>>>>>> --- a/arch/arm/cpu/armv7/zynq/cpu.c
> >>>>>>>> +++ b/arch/arm/cpu/armv7/zynq/cpu.c
> >>>>>>>> @@ -11,6 +11,10 @@
> >>>>>>>>
> >>>>>>>>  void lowlevel_init(void)
> >>>>>>>>  {
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> I'd rather you deleted lowlevel_init() as a C function with this
> >>>>>>> name should not exist.
> >>>>>>
> >>>>>> Ok. Do you want me to create almost empty low_level.S or just use
> >>>>>> arch/arm/cpu/arvm7/lowlevel_init.S and define empty s_init()?
> >>>>>
> >>>>> Urgh. I realize removing the C function would give you more work than
> >>>>> simply keeping it empty until the whole s_init() mess is cleaned up. :(
> >>>>>
> >>>>> I'll take your change as-is, sorry for the noise.
> >>>>
> >>>> In connection to this topic we have recently found one issue
> >>>> regarding to neon instruction which u-boot uses.
> >>>>
> >>>> We have this code to enable them in asm and adding this to lowlevel_init.S
> >>>> is straight way how to do so.
> >>>>         mov     r0, r0
> >>>>         mrc     p15, 0, r1, c1, c0, 2
> >>>>         orr     r1, r1, #(0xf << 20)
> >>>>         mcr     p15, 0, r1, c1, c0, 2
> >>>>
> >>>>         fmrx    r1, FPEXC
> >>>>         orr     r1,r1, #(1<<30)
> >>>>         fmxr    FPEXC, r1
> >>>>
> >>>> Is it ok to create zynq asm specific lowlevel function
> >>>> or doing this through s_init() or you have nice a clean way how
> >>>> this should be solved when you are saying that s_init() is mess.
> >>>
> >>> Sorry for responding slowly.
> >>>
> >>> I suspect when you say neon instruction that U-Boot uses, you mean neon
> >>> instructions that GCC is allowed to emit while building U-Boot, right?
> >>
> >> yes.
> >>
> >>> So we're talking about neon insns in C code only, not asm, correct?
> >>
> >> yes. gcc emits neon instruction in timer code. Not in asm.
> >>
> >>
> >>> If this is correct, then does something prevent you from enabling
> >>> neon instructions as early as possible, in e.g. the lowlevel_init
> >>> routine?
> >>
> >> ok let me clear this. I think location of this code is clear.
> >> It is asm code and it will be called ASAP even
> >> we know exactly which code emits neon instructions.
> >> My point was if we should create specific lowlevel_init asm function
> >> and add this code there.
> >> Or use arch/arm/cpu/armv7/lowlevel_init.S and create just s_init function.
> >>
> >> You mentioned above that s_init() is a mess and needs to be clean up
> >> but you didn't mentioned how.
> >>
> >> It means my point is if you tell us how should be clean up we can
> >> just submit code which is compatible with this cleanup activity.
> > 
> > If I knew how to clean s_init() up, I'd have sent out patches
> > already. :)
> 
> Fair enough. :-)
> 
> > Anyway, I'm not sure that I see how s_init() and your need for a NEON
> > enable sequence would be related: this sequence does not *need* to be in
> > s_init().
> 
> ok. s_init is not asm function - but C function.
> 
> 
> > Indeed, enabling NEON is, IMO, similar to enabling alignment checks
> > or setting the CPU mode, so I guess it could find its way in start.S,
> > inside a preprocessor conditional (since e.g. not all Cortex-A9 will
> > support NEON).
> 
> ok. That sound good to me.
> 
> 
> > BTW, where in U-Boot does GCC get instructed to emit NEON instructions
> > at the moment? There is no -mfpu or -mfloat-abi option in the code base
> > right now, so I suspect you're going to introduce it along with the
> > enable sequence, correct?
> 
> file: arch/arm/cpu/armv7/zynq/timer.c
> fce: void __udelay(unsigned long usec)
> line: countticks = (u32) (((unsigned long long) TIMER_TICK_HZ * usec) /
>                                                                 1000000);
> This is what I have got from Edgar.
> 
> "A significant difference between the u-boot builds is that the failing
> one is using NEON instructions for some of the div/mod helpers.
> AFAIK, NEON instructions are disabled after reset and will cause undef
> exceptions if issued while disabled. "
> 
> That difference in builds which is mentioned above is when this patch is
> applied.
> 
> diff --git a/arch/arm/cpu/armv7/zynq/timer.c b/arch/arm/cpu/armv7/zynq/timer.c
> index 875903a..38594cb 100644
> --- a/arch/arm/cpu/armv7/zynq/timer.c
> +++ b/arch/arm/cpu/armv7/zynq/timer.c
> @@ -119,12 +119,13 @@ void __udelay(unsigned long usec)
>  	u32 timeend;
>  	u32 timediff;
>  	u32 timenow;
> +	u64 temp;
> 
>  	if (usec == 0)
>  		return;
> 
> -	countticks = (u32) (((unsigned long long) TIMER_TICK_HZ * usec) /
> -								1000000);
> +	temp = (TIMER_TICK_HZ * usec)/1000000;
> +	countticks = (u32)temp;
> 
>  	/* decrementing timer */
>  	timeend = readl(&timer_base->counter) - countticks;
> 
> 
> We haven't seen any problem in normal flow because NEON instructions
> are enabled in the fsbl(first stage bootloader) that's why we didn't see
> any problem with original code.

My question is not "which part of the U-Boot C source code causes issues
because it is emitted with NEON instructions in it", but "which part of
the U-Boot makefile system tells GCC that it can emit NEON instruction
at all".

IOW, the current makefiles contain no -mfpu=neon* or -mfloat-abi=*. I
see GCC has an option called -mneon-for-64bits, but the doc says it is
disabled by default, and we don't enable it.

So where does GCC find in U-Boot that it is allowed to emit NEON
insns in the first place?

> Thanks,
> Michal

Amicalement,
-- 
Albert.