[U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init()
Albert ARIBAUD
albert.u.boot at aribaud.net
Thu Oct 17 10:25:25 CEST 2013
Hi Edgar,
On Thu, 17 Oct 2013 09:37:40 +0200, "Edgar E. Iglesias"
<edgar.iglesias at xilinx.com> wrote:
> On Thu, Oct 17, 2013 at 08:33:28AM +0200, Albert ARIBAUD wrote:
> > Hi Albert,
> >
> > On Thu, 3 Oct 2013 18:07:40 +0200, Albert ARIBAUD
> > <albert.u.boot at aribaud.net> wrote:
> >
> > > 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?
> >
> > Ping
>
> Hi Albert,
>
> I think the need to pass -mfpu and -mfloat flags to gcc depend on how the
> toolchain was configured. In the Zynq case, the toolchain is targeted at
> the setup of the cortex-a9 within the Zynq and pre-configured with these
> options enabled by default.
>
> I dont see the -mneon-for-64bits option in our version, but I assume
> something equivalent is used.
Thanks for the clarification. This confirms my initial opinion that
NEON can be enabled in start.S on a per-board basis, as it would workin
all situations -- This is assuming that enabling NEON would have no
adverse effect on builds which do not use it, of course.
> Cheers,
> Edgar
Amicalement,
--
Albert.
More information about the U-Boot
mailing list