[U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init()
Michal Simek
monstr at monstr.eu
Thu Oct 17 10:30:01 CEST 2013
On 10/17/2013 10:25 AM, Albert ARIBAUD wrote:
> 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.
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131017/7de25804/attachment.pgp>
More information about the U-Boot
mailing list