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

[Albert ARIBAUD]

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

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

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

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?

> Thanks,
> Michal

Amicalement,
-- 
Albert.