diff mbox

[U-Boot,v2] add new board nas62x0

Message ID 20120321003435.GB17427@w500.lan
State Superseded
Headers show

Commit Message

Luka Perkov March 21, 2012, 12:34 a.m. UTC 
Hi Marek,

On Tue, Mar 20, 2012 at 07:48:05AM +0100, Marek Vasut wrote:
> > > > > > +#define CONFIG_SKIP_LOWLEVEL_INIT	/* disable board lowlevel_init
> > > 
> > > */
> > > 
> > > > > Are you sure you want to skip lowlevel init? It'll break cache setup
> > > > > etc. I believe.
> > > > 
> > > > I will retest and send v4 once I get your feedback on other items.
> > > 
> > > Ok, what's the result? From IRC I take it you must define this ... why?
> > 
> > It generates error when building without it:
> > 
> > /home/luka/uboot/arch/arm/cpu/arm926ejs/start.S:393: undefined reference to
> > `lowlevel_init' arm-openwrt-linux-ld: BFD (GNU Binutils) 2.22 assertion
> > fail elf32-arm.c:13830
> 
> Define it empty in your arch/arm/cpu/..../kirkwood.c and be done with it ;-)

Yes, this seems to fix it:


I'll clean this up and resend after we commit this...

> > All other kirkwood targets I looked at define CONFIG_SKIP_LOWLEVEL_INIT,
> > including the ones mentioned above; here are their configs for
> > comparison:
> > 
> > include/configs/dreamplug.h
> > include/configs/sheevaplug.h
> > include/configs/dockstar.h
> 
> Why do you need to skip it? Does it hang or something?

See above. I guess compile error also for other boards.
 
> > This is my proposal - I'll resend v4 and it should be ok to commit
> > without fixes for:
> > 
> >  1) IB62x0_OE_LOW and IB62x0_OE_HIGH
> >  2) CONFIG_SKIP_LOWLEVEL_INIT
> >  3) ifdef indentation
> > 
> > Because fixing the 1) and 2) is more than adding support for this new
> > board, and if it was in the same patch I would need to separate it. That
> > is a different issue.
> 
> You can wait for Prafulla with #1 and #2, also for #2 check my comment. But we 
> have two bugs going on for granted here at least and they're not your boards 
> fault. On the other hand, it'd be cool if you could fix them prior to adding 
> your board ;-)

I'll resend v4 now and work on patches for this stuff later.
 
> > I'll put on my TODO list, and work on this after commit:
> > 
> >  * replace tabs with spaces in boards.config
> >  * look at IB62x0_OE_LOW and IB62x0_OE_HIGH issue
> >  * look at CONFIG_SKIP_LOWLEVEL_INIT issue

For this one we have a patch now :)

Thank you Marek.

Bye,
Luka

Comments

Marek Vasut March 21, 2012, 7:21 a.m. UTC | #1 
Dear Luka Perkov,

> Hi Marek,
> 
> On Tue, Mar 20, 2012 at 07:48:05AM +0100, Marek Vasut wrote:
> > > > > > > +#define CONFIG_SKIP_LOWLEVEL_INIT	/* disable board
> > > > > > > lowlevel_init
> > > > 
> > > > */
> > > > 
> > > > > > Are you sure you want to skip lowlevel init? It'll break cache
> > > > > > setup etc. I believe.
> > > > > 
> > > > > I will retest and send v4 once I get your feedback on other items.
> > > > 
> > > > Ok, what's the result? From IRC I take it you must define this ...
> > > > why?
> > > 
> > > It generates error when building without it:
> > > 
> > > /home/luka/uboot/arch/arm/cpu/arm926ejs/start.S:393: undefined
> > > reference to `lowlevel_init' arm-openwrt-linux-ld: BFD (GNU Binutils)
> > > 2.22 assertion fail elf32-arm.c:13830
> > 
> > Define it empty in your arch/arm/cpu/..../kirkwood.c and be done with it
> > ;-)
> 
> Yes, this seems to fix it:
> 
> diff --git a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c index fba5e01..ec2026c 100644
> --- a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
> @@ -33,6 +33,8 @@
> 
>  #define BUFLEN 16
> 
> +inline void lowlevel_init(void) {}
> +
>  void reset_cpu(unsigned long ignored)
>  {
>         struct kwcpu_registers *cpureg =
> diff --git a/include/configs/ib62x0.h b/include/configs/ib62x0.h
> index 1c4778d..9808a04 100644
> --- a/include/configs/ib62x0.h
> +++ b/include/configs/ib62x0.h
> @@ -43,7 +43,6 @@
>  #define CONFIG_KIRKWOOD                        /* SOC Family Name */
>  #define CONFIG_KW88F6281               /* SOC Name */
>  #define CONFIG_MACH_NAS6210            /* Machine type */
> -#define CONFIG_SKIP_LOWLEVEL_INIT      /* disable board lowlevel_init */
> 
>  /*
>   * Other required minimal configurations
> 
> I'll clean this up and resend after we commit this...

Commit what? Looking forward to V3 btw :)

> 
> > > All other kirkwood targets I looked at define
> > > CONFIG_SKIP_LOWLEVEL_INIT, including the ones mentioned above; here
> > > are their configs for comparison:
> > > 
> > > include/configs/dreamplug.h
> > > include/configs/sheevaplug.h
> > > include/configs/dockstar.h
> > 
> > Why do you need to skip it? Does it hang or something?
> 
> See above. I guess compile error also for other boards.

Still you're missing cpu_init_crit in start.S, which might cause trouble. Now 
that you defined lowlevel_init(), you can as well remove this define SKIP... 
right?

> 
> > > This is my proposal - I'll resend v4 and it should be ok to commit
> > > 
> > > without fixes for:
> > >  1) IB62x0_OE_LOW and IB62x0_OE_HIGH
> > >  2) CONFIG_SKIP_LOWLEVEL_INIT
> > >  3) ifdef indentation
> > > 
> > > Because fixing the 1) and 2) is more than adding support for this new
> > > board, and if it was in the same patch I would need to separate it.
> > > That is a different issue.
> > 
> > You can wait for Prafulla with #1 and #2, also for #2 check my comment.
> > But we have two bugs going on for granted here at least and they're not
> > your boards fault. On the other hand, it'd be cool if you could fix them
> > prior to adding your board ;-)
> 
> I'll resend v4 now and work on patches for this stuff later.
> 
> > > I'll put on my TODO list, and work on this after commit:
> > >  * replace tabs with spaces in boards.config
> > >  * look at IB62x0_OE_LOW and IB62x0_OE_HIGH issue
> > >  * look at CONFIG_SKIP_LOWLEVEL_INIT issue
> 
> For this one we have a patch now :)
> 
> Thank you Marek.

Thank you for your good work so far :)

> 
> Bye,
> Luka

Best regards,
Marek Vasut
Prafulla Wadaskar March 21, 2012, 9:51 a.m. UTC | #2 
> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: 21 March 2012 12:52
> To: Luka Perkov
> Cc: u-boot@lists.denx.de; dreagle@doukki.net; Wolfgang Denk; Prafulla
> Wadaskar
> Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> 
> Dear Luka Perkov,
> 
> > Hi Marek,
> >
...snip...
> > > > This is my proposal - I'll resend v4 and it should be ok to
> commit
> > > >
> > > > without fixes for:
> > > >  1) IB62x0_OE_LOW and IB62x0_OE_HIGH
> > > >  2) CONFIG_SKIP_LOWLEVEL_INIT
> > > >  3) ifdef indentation
> > > >
> > > > Because fixing the 1) and 2) is more than adding support for
> this new
> > > > board, and if it was in the same patch I would need to separate
> it.
> > > > That is a different issue.
> > >
> > > You can wait for Prafulla with #1 and #2, also for #2 check my
> comment.
> > > But we have two bugs going on for granted here at least and
> they're not
> > > your boards fault. On the other hand, it'd be cool if you could
> fix them
> > > prior to adding your board ;-)

Hi Luka

#1: Defining these values as 0xffffffff, indicates that all GPIOs are configured high by default. so this configuration solely depends upon your board requirement. 

#2: on kirkwood, you should define CONFIG_SKIP_LOWLEVEL_INIT since lowlevel_init is not needed on Kirkwood platforms. (ref: doc/README.kwbimage)

Regards..
Prafulla . . .
Marek Vasut March 21, 2012, 10:02 a.m. UTC | #3 
Dear Prafulla Wadaskar,

> > -----Original Message-----
> > From: Marek Vasut [mailto:marex@denx.de]
> > Sent: 21 March 2012 12:52
> > To: Luka Perkov
> > Cc: u-boot@lists.denx.de; dreagle@doukki.net; Wolfgang Denk; Prafulla
> > Wadaskar
> > Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> > 
> > Dear Luka Perkov,
> > 
> > > Hi Marek,
> 
> ...snip...
> 
> > > > > This is my proposal - I'll resend v4 and it should be ok to
> > 
> > commit
> > 
> > > > > without fixes for:
> > > > >  1) IB62x0_OE_LOW and IB62x0_OE_HIGH
> > > > >  2) CONFIG_SKIP_LOWLEVEL_INIT
> > > > >  3) ifdef indentation
> > > > > 
> > > > > Because fixing the 1) and 2) is more than adding support for
> > 
> > this new
> > 
> > > > > board, and if it was in the same patch I would need to separate
> > 
> > it.
> > 
> > > > > That is a different issue.
> > > > 
> > > > You can wait for Prafulla with #1 and #2, also for #2 check my
> > 
> > comment.
> > 
> > > > But we have two bugs going on for granted here at least and
> > 
> > they're not
> > 
> > > > your boards fault. On the other hand, it'd be cool if you could
> > 
> > fix them
> > 
> > > > prior to adding your board ;-)
> 
> Hi Luka
> 
> #1: Defining these values as 0xffffffff, indicates that all GPIOs are
> configured high by default. so this configuration solely depends upon your
> board requirement.
> 
> #2: on kirkwood, you should define CONFIG_SKIP_LOWLEVEL_INIT since
> lowlevel_init is not needed on Kirkwood platforms. (ref:
> doc/README.kwbimage)

Prafulla, you're then missing the fiddling with CPSR bits, which might be quite 
necessary.

> 
> Regards..
> Prafulla . . .

Best regards,
Marek Vasut
Prafulla Wadaskar March 21, 2012, 10:15 a.m. UTC | #4 
> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: 21 March 2012 15:33
> To: Prafulla Wadaskar
> Cc: Luka Perkov; u-boot@lists.denx.de; dreagle@doukki.net; Wolfgang
> Denk
> Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> 
> Dear Prafulla Wadaskar,
...snip...
> >
> > Hi Luka
> >
> > #1: Defining these values as 0xffffffff, indicates that all GPIOs
> are
> > configured high by default. so this configuration solely depends
> upon your
> > board requirement.
> >
> > #2: on kirkwood, you should define CONFIG_SKIP_LOWLEVEL_INIT since
> > lowlevel_init is not needed on Kirkwood platforms. (ref:
> > doc/README.kwbimage)
> 
> Prafulla, you're then missing the fiddling with CPSR bits, which might
> be quite
> necessary.

Hi Marek.
May be, may you please explain these bits? Or any pointers? Can't these be addressed in kwimage.cfg?

Regards..
Prafulla . . .
Marek Vasut March 21, 2012, 10:56 a.m. UTC | #5 
Dear Prafulla Wadaskar,

> > -----Original Message-----
> > From: Marek Vasut [mailto:marex@denx.de]
> > Sent: 21 March 2012 15:33
> > To: Prafulla Wadaskar
> > Cc: Luka Perkov; u-boot@lists.denx.de; dreagle@doukki.net; Wolfgang
> > Denk
> > Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> > 
> > Dear Prafulla Wadaskar,
> 
> ...snip...
> 
> > > Hi Luka
> > > 
> > > #1: Defining these values as 0xffffffff, indicates that all GPIOs
> > 
> > are
> > 
> > > configured high by default. so this configuration solely depends
> > 
> > upon your
> > 
> > > board requirement.
> > > 
> > > #2: on kirkwood, you should define CONFIG_SKIP_LOWLEVEL_INIT since
> > > lowlevel_init is not needed on Kirkwood platforms. (ref:
> > > doc/README.kwbimage)
> > 
> > Prafulla, you're then missing the fiddling with CPSR bits, which might
> > be quite
> > necessary.
> 
> Hi Marek.
> May be, may you please explain these bits? Or any pointers? Can't these be
> addressed in kwimage.cfg?

I have no idea, I'm no kirkwood expert. And about these bits, check start.S, 
what it does with them.

> 
> Regards..
> Prafulla . . .

Best regards,
Marek Vasut
Prafulla Wadaskar March 21, 2012, 12:01 p.m. UTC | #6 
> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: 21 March 2012 16:27
> To: Prafulla Wadaskar
> Cc: Luka Perkov; u-boot@lists.denx.de; dreagle@doukki.net; Wolfgang
> Denk
> Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> 
> Dear Prafulla Wadaskar,
> 
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex@denx.de]
> > > Sent: 21 March 2012 15:33
> > > To: Prafulla Wadaskar
> > > Cc: Luka Perkov; u-boot@lists.denx.de; dreagle@doukki.net;
> Wolfgang
> > > Denk
> > > Subject: Re: [U-Boot] [PATCH v2] add new board nas62x0
> > >
> > > Dear Prafulla Wadaskar,
> >
> > ...snip...
> >
> > > > Hi Luka
> > > >
> > > > #1: Defining these values as 0xffffffff, indicates that all
> GPIOs
> > >
> > > are
> > >
> > > > configured high by default. so this configuration solely depends
> > >
> > > upon your
> > >
> > > > board requirement.
> > > >
> > > > #2: on kirkwood, you should define CONFIG_SKIP_LOWLEVEL_INIT
> since
> > > > lowlevel_init is not needed on Kirkwood platforms. (ref:
> > > > doc/README.kwbimage)
> > >
> > > Prafulla, you're then missing the fiddling with CPSR bits, which
> might
> > > be quite
> > > necessary.
> >
> > Hi Marek.
> > May be, may you please explain these bits? Or any pointers? Can't
> these be
> > addressed in kwimage.cfg?
> 
> I have no idea, I'm no kirkwood expert. And about these bits, check
> start.S,
> what it does with them.

Hi Marek,

I have checked arc/arm/cpu/arm926ejs/start.S, and I didn't find any issue, lowlevel_init will be called from cpu_init_crit which should be okay, there is cache init related code in it, if that can be a problem then it should be kept out.
And on Kirkwood, internal bootloader does the job of lowlevel_init prior to start uboot execution.

Regards..
Prafulla . . .
diff mbox

Patch

diff --git a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
index fba5e01..ec2026c 100644
--- a/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
+++ b/arch/arm/cpu/arm926ejs/kirkwood/cpu.c
@@ -33,6 +33,8 @@ 
 
 #define BUFLEN 16
 
+inline void lowlevel_init(void) {}
+
 void reset_cpu(unsigned long ignored)
 {
        struct kwcpu_registers *cpureg =
diff --git a/include/configs/ib62x0.h b/include/configs/ib62x0.h
index 1c4778d..9808a04 100644
--- a/include/configs/ib62x0.h
+++ b/include/configs/ib62x0.h
@@ -43,7 +43,6 @@ 
 #define CONFIG_KIRKWOOD                        /* SOC Family Name */
 #define CONFIG_KW88F6281               /* SOC Name */
 #define CONFIG_MACH_NAS6210            /* Machine type */
-#define CONFIG_SKIP_LOWLEVEL_INIT      /* disable board lowlevel_init */
 
 /*
  * Other required minimal configurations