diff mbox

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

Message ID 230d876ac5e8ec8d7a16c90415263154650d0486.1377175919.git.michal.simek@xilinx.com
State Awaiting Upstream
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Michal Simek Aug. 22, 2013, 12:52 p.m. UTC
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@tuhh.de>
Signed-off-by: Michal Simek <michal.simek@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(+)

--
1.8.2.3

Comments

Albert ARIBAUD Sept. 23, 2013, 12:31 p.m. UTC | #1
Hi Michal,

On Thu, 22 Aug 2013 14:52:02 +0200, Michal Simek
<michal.simek@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@tuhh.de>
> Signed-off-by: Michal Simek <michal.simek@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.
 
> +int arch_cpu_init(void)
> +{
>  	zynq_slcr_unlock();
>  	/* remap DDR to zero, FILTERSTART */
>  	writel(0, &scu_base->filter_start);
> @@ -31,6 +35,8 @@ void lowlevel_init(void)
>  	writel(0xC, &slcr_base->ddr_urgent);
> 
>  	zynq_slcr_lock();
> +
> +	return 0;
>  }
> 
>  void reset_cpu(ulong addr)
> --
> 1.8.2.3

Amicalement,
Michal Simek Sept. 23, 2013, 2:19 p.m. UTC | #2
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@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@tuhh.de>
>> Signed-off-by: Michal Simek <michal.simek@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()?

Thanks,
Michal
Albert ARIBAUD Sept. 23, 2013, 2:37 p.m. UTC | #3
Hi Michal,

On Mon, 23 Sep 2013 16:19:52 +0200, Michal Simek <monstr@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@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@tuhh.de>
> >> Signed-off-by: Michal Simek <michal.simek@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.

> Thanks,
> Michal

Amicalement,
Michal Simek Sept. 23, 2013, 2:56 p.m. UTC | #4
On 09/23/2013 04:37 PM, Albert ARIBAUD wrote:
> Hi Michal,
> 
> On Mon, 23 Sep 2013 16:19:52 +0200, Michal Simek <monstr@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@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@tuhh.de>
>>>> Signed-off-by: Michal Simek <michal.simek@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.

No problem with that.

Thanks,
Michal
Michal Simek Sept. 24, 2013, 10:38 a.m. UTC | #5
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@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@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@tuhh.de>
>>>> Signed-off-by: Michal Simek <michal.simek@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.

Thanks,
Michal
Albert ARIBAUD Oct. 2, 2013, 7:43 p.m. UTC | #6
Hi Michal,

On Tue, 24 Sep 2013 12:38:38 +0200, Michal Simek <monstr@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@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@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@tuhh.de>
> >>>> Signed-off-by: Michal Simek <michal.simek@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?
So we're talking about neon insns in C code only, not asm, correct?

If this is correct, then does something prevent you from enabling
neon instructions as early as possible, in e.g. the lowlevel_init
routine?

> Thanks,
> Michal

Amicalement,
Michal Simek Oct. 3, 2013, 6:58 a.m. UTC | #7
On 10/02/2013 09:43 PM, Albert ARIBAUD wrote:
> Hi Michal,
> 
> On Tue, 24 Sep 2013 12:38:38 +0200, Michal Simek <monstr@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@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@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@tuhh.de>
>>>>>> Signed-off-by: Michal Simek <michal.simek@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.

Thanks,
Michal
Albert ARIBAUD Oct. 3, 2013, 8:41 a.m. UTC | #8
Hi Michal,

On Thu, 03 Oct 2013 08:58:38 +0200, Michal Simek <monstr@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@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@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@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@tuhh.de>
> >>>>>> Signed-off-by: Michal Simek <michal.simek@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 ARIBAUD Oct. 17, 2013, 6:49 a.m. UTC | #9
Hi Michal,

On Thu, 22 Aug 2013 14:52:02 +0200, Michal Simek
<michal.simek@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@tuhh.de>
> Signed-off-by: Michal Simek <michal.simek@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)
>  {
> +}
> +
> +int arch_cpu_init(void)
> +{
>  	zynq_slcr_unlock();
>  	/* remap DDR to zero, FILTERSTART */
>  	writel(0, &scu_base->filter_start);
> @@ -31,6 +35,8 @@ void lowlevel_init(void)
>  	writel(0xC, &slcr_base->ddr_urgent);
> 
>  	zynq_slcr_lock();
> +
> +	return 0;
>  }
> 
>  void reset_cpu(ulong addr)
> --
> 1.8.2.3
> 
> 
Applied to u-boot-arm/master, thanks!

Amicalement,
diff mbox

Patch

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)
 {
+}
+
+int arch_cpu_init(void)
+{
 	zynq_slcr_unlock();
 	/* remap DDR to zero, FILTERSTART */
 	writel(0, &scu_base->filter_start);
@@ -31,6 +35,8 @@  void lowlevel_init(void)
 	writel(0xC, &slcr_base->ddr_urgent);

 	zynq_slcr_lock();
+
+	return 0;
 }

 void reset_cpu(ulong addr)