[U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init()
Albert ARIBAUD
albert.u.boot at aribaud.net
Thu Oct 17 08:33:28 CEST 2013
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
> > Thanks,
> > Michal
>
> Amicalement,
Amicalement,
--
Albert.
More information about the U-Boot
mailing list