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 <mon...@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 <mon...@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 <mon...@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.si...@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.schwer...@tuhh.de>
>>>>>>>> Signed-off-by: Michal Simek <michal.si...@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.
Thanks,
Michal
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
