* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() @ 2013-08-22 12:52 Michal Simek 2013-09-23 12:31 ` Albert ARIBAUD 2013-10-17 6:49 ` Albert ARIBAUD 0 siblings, 2 replies; 17+ messages in thread From: Michal Simek @ 2013-08-22 12:52 UTC (permalink / raw) To: u-boot 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 198 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130822/ee64ce22/attachment.pgp> ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-08-22 12:52 [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() Michal Simek @ 2013-09-23 12:31 ` Albert ARIBAUD 2013-09-23 14:19 ` Michal Simek 2013-10-17 6:49 ` Albert ARIBAUD 1 sibling, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-09-23 12:31 UTC (permalink / raw) To: u-boot 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, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-09-23 12:31 ` Albert ARIBAUD @ 2013-09-23 14:19 ` Michal Simek 2013-09-23 14:37 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Michal Simek @ 2013-09-23 14:19 UTC (permalink / raw) To: u-boot 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 -- 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/20130923/2f3e0ad4/attachment.pgp> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-09-23 14:19 ` Michal Simek @ 2013-09-23 14:37 ` Albert ARIBAUD 2013-09-23 14:56 ` Michal Simek 2013-09-24 10:38 ` Michal Simek 0 siblings, 2 replies; 17+ messages in thread From: Albert ARIBAUD @ 2013-09-23 14:37 UTC (permalink / raw) To: u-boot 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, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-09-23 14:37 ` Albert ARIBAUD @ 2013-09-23 14:56 ` Michal Simek 2013-09-24 10:38 ` Michal Simek 1 sibling, 0 replies; 17+ messages in thread From: Michal Simek @ 2013-09-23 14:56 UTC (permalink / raw) To: u-boot 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, 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/20130923/464a9366/attachment.pgp> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-09-23 14:37 ` Albert ARIBAUD 2013-09-23 14:56 ` Michal Simek @ 2013-09-24 10:38 ` Michal Simek 2013-10-02 19:43 ` Albert ARIBAUD 1 sibling, 1 reply; 17+ messages in thread From: Michal Simek @ 2013-09-24 10:38 UTC (permalink / raw) To: u-boot 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 -- 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/20130924/415330e8/attachment.pgp> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-09-24 10:38 ` Michal Simek @ 2013-10-02 19:43 ` Albert ARIBAUD 2013-10-03 6:58 ` Michal Simek 0 siblings, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-02 19:43 UTC (permalink / raw) To: u-boot 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, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-02 19:43 ` Albert ARIBAUD @ 2013-10-03 6:58 ` Michal Simek 2013-10-03 8:41 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Michal Simek @ 2013-10-03 6:58 UTC (permalink / raw) To: u-boot 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 -- 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/20131003/31ffc4bd/attachment.pgp> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-03 6:58 ` Michal Simek @ 2013-10-03 8:41 ` Albert ARIBAUD 2013-10-03 9:56 ` Michal Simek 0 siblings, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-03 8:41 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-03 8:41 ` Albert ARIBAUD @ 2013-10-03 9:56 ` Michal Simek 2013-10-03 16:07 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Michal Simek @ 2013-10-03 9:56 UTC (permalink / raw) To: u-boot 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@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. :) 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-03 9:56 ` Michal Simek @ 2013-10-03 16:07 ` Albert ARIBAUD 2013-10-17 6:33 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-03 16:07 UTC (permalink / raw) To: u-boot Hi Michal, On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek <michal.simek@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@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. :) > > 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? > Thanks, > Michal Amicalement, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-03 16:07 ` Albert ARIBAUD @ 2013-10-17 6:33 ` Albert ARIBAUD 2013-10-17 7:37 ` Edgar E. Iglesias 0 siblings, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-17 6:33 UTC (permalink / raw) To: u-boot Hi Albert, On Thu, 3 Oct 2013 18:07:40 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Michal, > > On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek > <michal.simek@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@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. :) > > > > 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 > > Thanks, > > Michal > > Amicalement, Amicalement, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-17 6:33 ` Albert ARIBAUD @ 2013-10-17 7:37 ` Edgar E. Iglesias 2013-10-17 8:25 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Edgar E. Iglesias @ 2013-10-17 7:37 UTC (permalink / raw) To: u-boot 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@aribaud.net> wrote: > > > Hi Michal, > > > > On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek > > <michal.simek@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@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. :) > > > > > > 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. Cheers, Edgar ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-17 7:37 ` Edgar E. Iglesias @ 2013-10-17 8:25 ` Albert ARIBAUD 2013-10-17 8:30 ` Michal Simek 0 siblings, 1 reply; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-17 8:25 UTC (permalink / raw) To: u-boot Hi Edgar, On Thu, 17 Oct 2013 09:37:40 +0200, "Edgar E. Iglesias" <edgar.iglesias@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@aribaud.net> wrote: > > > > > Hi Michal, > > > > > > On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek > > > <michal.simek@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@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. :) > > > > > > > > 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. > Cheers, > Edgar Amicalement, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-17 8:25 ` Albert ARIBAUD @ 2013-10-17 8:30 ` Michal Simek 2013-10-17 12:43 ` Albert ARIBAUD 0 siblings, 1 reply; 17+ messages in thread From: Michal Simek @ 2013-10-17 8:30 UTC (permalink / raw) To: u-boot 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@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@aribaud.net> wrote: >>> >>>> Hi Michal, >>>> >>>> On Thu, 3 Oct 2013 11:56:20 +0200, Michal Simek >>>> <michal.simek@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@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. :) >>>>> >>>>> 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-10-17 8:30 ` Michal Simek @ 2013-10-17 12:43 ` Albert ARIBAUD 0 siblings, 0 replies; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-17 12:43 UTC (permalink / raw) To: u-boot Hi Michal, > > 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. Even better. :) > Thanks, > Michal Amicalement, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() 2013-08-22 12:52 [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() Michal Simek 2013-09-23 12:31 ` Albert ARIBAUD @ 2013-10-17 6:49 ` Albert ARIBAUD 1 sibling, 0 replies; 17+ messages in thread From: Albert ARIBAUD @ 2013-10-17 6:49 UTC (permalink / raw) To: u-boot 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, -- Albert. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-10-17 12:43 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-22 12:52 [U-Boot] [PATCH] zynq: Use arch_cpu_init() instead of lowlevel_init() Michal Simek 2013-09-23 12:31 ` Albert ARIBAUD 2013-09-23 14:19 ` Michal Simek 2013-09-23 14:37 ` Albert ARIBAUD 2013-09-23 14:56 ` Michal Simek 2013-09-24 10:38 ` Michal Simek 2013-10-02 19:43 ` Albert ARIBAUD 2013-10-03 6:58 ` Michal Simek 2013-10-03 8:41 ` Albert ARIBAUD 2013-10-03 9:56 ` Michal Simek 2013-10-03 16:07 ` Albert ARIBAUD 2013-10-17 6:33 ` Albert ARIBAUD 2013-10-17 7:37 ` Edgar E. Iglesias 2013-10-17 8:25 ` Albert ARIBAUD 2013-10-17 8:30 ` Michal Simek 2013-10-17 12:43 ` Albert ARIBAUD 2013-10-17 6:49 ` Albert ARIBAUD
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox