On 10/17/2013 10:25 AM, Albert ARIBAUD wrote: > Hi Edgar, > > On Thu, 17 Oct 2013 09:37:40 +0200, "Edgar E. Iglesias" > <[email protected]> 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 >>> <[email protected]> wrote: >>> >>>> Hi Michal, >>>> >>>> On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek >>>> <[email protected]> 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 <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> On 10/02/2013 09:43 PM, Albert ARIBAUD wrote: >>>>>>>> Hi Michal, >>>>>>>> >>>>>>>> On Tue, 24 Sep 2013 12:38:38 +0200, Michal Simek <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> On 09/23/2013 02:31 PM, Albert ARIBAUD wrote: >>>>>>>>>>>> Hi Michal, >>>>>>>>>>>> >>>>>>>>>>>> On Thu, 22 Aug 2013 14:52:02 +0200, Michal Simek >>>>>>>>>>>> <[email protected]> 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 <[email protected]> >>>>>>>>>>>>> Signed-off-by: Michal Simek <[email protected]> >>>>>>>>>>>>> --- >>>>>>>>>>>>> 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.
Isn't it better just to enabled it when __ARM_NEON__ is exported by gcc? It should mean that gcc allows to emit these instructions. Thanks, Michal -- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
