[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[York Sun]

For powerpc SoCs, the initial gd is in INIT_RAM, in most cases, resideing
in locked D-cache. At the time the function baord_inti_f() runs, no other
RAM is available as a stack. This technique has been used in
arch/powerpc/lib/board.c and should be added to generic board for powerpc.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 common/board_f.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index cbdf06f..3a00b92 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -970,7 +970,10 @@ static init_fnc_t init_sequence_f[] = {
 
 void board_init_f(ulong boot_flags)
 {
-#ifndef CONFIG_X86
+#ifdef CONFIG_PPC
+	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
+	__asm__ __volatile__("" : : : "memory");
+#elif !defined(CONFIG_X86)
 	gd_t data;
 
 	gd = &data;
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] common/board_f: Fix size variable

[York Sun]

DRAM size should use 64-bit variable when the size could be more than 4GB.
Caught and verified on P4080DS with 4GB DDR.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 common/board_f.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c
index 3a00b92..954605b 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -194,7 +194,7 @@ static int init_func_ram(void)
 
 static int show_dram_config(void)
 {
-	ulong size;
+	unsigned long long size;
 
 #ifdef CONFIG_NR_DRAM_BANKS
 	int i;
-- 
1.7.9.5

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[York Sun]

On 04/28/2014 03:51 PM, York Sun wrote:
> For powerpc SoCs, the initial gd is in INIT_RAM, in most cases, resideing
> in locked D-cache. At the time the function baord_inti_f() runs, no other
> RAM is available as a stack. This technique has been used in
> arch/powerpc/lib/board.c and should be added to generic board for powerpc.
> 
> Signed-off-by: York Sun <yorksun@freescale.com>
> ---
>  common/board_f.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index cbdf06f..3a00b92 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -970,7 +970,10 @@ static init_fnc_t init_sequence_f[] = {
>  
>  void board_init_f(ulong boot_flags)
>  {
> -#ifndef CONFIG_X86
> +#ifdef CONFIG_PPC
> +	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
> +	__asm__ __volatile__("" : : : "memory");
> +#elif !defined(CONFIG_X86)
>  	gd_t data;
>  
>  	gd = &data;
> 

Scott,

Please review this patch. You mentioned in my RFC patch review that "gd is
already initialized at the beginning of board_init_f()". I think that's not the
case. This change is still needed to get gd correct value.

York

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[Scott Wood]

On Wed, 2014-04-30 at 10:33 -0700, York Sun wrote:
> On 04/28/2014 03:51 PM, York Sun wrote:
> > For powerpc SoCs, the initial gd is in INIT_RAM, in most cases, resideing
> > in locked D-cache. At the time the function baord_inti_f() runs, no other
> > RAM is available as a stack. This technique has been used in
> > arch/powerpc/lib/board.c and should be added to generic board for powerpc.
> > 
> > Signed-off-by: York Sun <yorksun@freescale.com>
> > ---
> >  common/board_f.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/board_f.c b/common/board_f.c
> > index cbdf06f..3a00b92 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -970,7 +970,10 @@ static init_fnc_t init_sequence_f[] = {
> >  
> >  void board_init_f(ulong boot_flags)
> >  {
> > -#ifndef CONFIG_X86
> > +#ifdef CONFIG_PPC
> > +	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
> > +	__asm__ __volatile__("" : : : "memory");
> > +#elif !defined(CONFIG_X86)
> >  	gd_t data;
> >  
> >  	gd = &data;
> > 
> 
> Scott,
> 
> Please review this patch.

Could you respond to the comments in the RFC patch?  No point
duplicating them.

>  You mentioned in my RFC patch review that "gd is
> already initialized at the beginning of board_init_f()". I think that's not the
> case. This change is still needed to get gd correct value.

Could you elaborate?  You're setting it to the same value that
cpu_init_early_f() set it to (on mpc85xx -- not all PPC).

-Scott

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[York Sun]

On 04/30/2014 10:57 AM, Scott Wood wrote:
> On Wed, 2014-04-30 at 10:33 -0700, York Sun wrote:
>> On 04/28/2014 03:51 PM, York Sun wrote:
>>> For powerpc SoCs, the initial gd is in INIT_RAM, in most cases, resideing
>>> in locked D-cache. At the time the function baord_inti_f() runs, no other
>>> RAM is available as a stack. This technique has been used in
>>> arch/powerpc/lib/board.c and should be added to generic board for powerpc.
>>>
>>> Signed-off-by: York Sun <yorksun@freescale.com>
>>> ---
>>>  common/board_f.c |    5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index cbdf06f..3a00b92 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -970,7 +970,10 @@ static init_fnc_t init_sequence_f[] = {
>>>  
>>>  void board_init_f(ulong boot_flags)
>>>  {
>>> -#ifndef CONFIG_X86
>>> +#ifdef CONFIG_PPC
>>> +	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
>>> +	__asm__ __volatile__("" : : : "memory");
>>> +#elif !defined(CONFIG_X86)
>>>  	gd_t data;
>>>  
>>>  	gd = &data;
>>>
>>
>> Scott,
>>
>> Please review this patch.
> 
> Could you respond to the comments in the RFC patch?  No point
> duplicating them.
> 
>>  You mentioned in my RFC patch review that "gd is
>> already initialized at the beginning of board_init_f()". I think that's not the
>> case. This change is still needed to get gd correct value.
> 
> Could you elaborate?  You're setting it to the same value that
> cpu_init_early_f() set it to (on mpc85xx -- not all PPC).
> 

Before this change, we have

#ifndef CONFIG_X86
gd_t data;

gd = &data;
#endif

This is overriding the gd.

For PPC, gd is set in different places. Eg,  cpu_init_early_f() for mpc85xx,
cpu_init_f() for for mpc512x, mpc5xxx, mpc8260, mpc83xx, mpc86xx. They are all
in different files. Since we have been using this assignment in
arch/powerpc/lib/board.c for all PPC, it should be safe and clear to have
correct assignment here.

We probably don't need the memory boundary though.

York

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[Scott Wood]

On Wed, 2014-04-30 at 11:14 -0700, York Sun wrote:
> On 04/30/2014 10:57 AM, Scott Wood wrote:
> > On Wed, 2014-04-30 at 10:33 -0700, York Sun wrote:
> >> On 04/28/2014 03:51 PM, York Sun wrote:
> >>> For powerpc SoCs, the initial gd is in INIT_RAM, in most cases, resideing
> >>> in locked D-cache. At the time the function baord_inti_f() runs, no other
> >>> RAM is available as a stack. This technique has been used in
> >>> arch/powerpc/lib/board.c and should be added to generic board for powerpc.
> >>>
> >>> Signed-off-by: York Sun <yorksun@freescale.com>
> >>> ---
> >>>  common/board_f.c |    5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/common/board_f.c b/common/board_f.c
> >>> index cbdf06f..3a00b92 100644
> >>> --- a/common/board_f.c
> >>> +++ b/common/board_f.c
> >>> @@ -970,7 +970,10 @@ static init_fnc_t init_sequence_f[] = {
> >>>  
> >>>  void board_init_f(ulong boot_flags)
> >>>  {
> >>> -#ifndef CONFIG_X86
> >>> +#ifdef CONFIG_PPC
> >>> +	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
> >>> +	__asm__ __volatile__("" : : : "memory");
> >>> +#elif !defined(CONFIG_X86)
> >>>  	gd_t data;
> >>>  
> >>>  	gd = &data;
> >>>
> >>
> >> Scott,
> >>
> >> Please review this patch.
> > 
> > Could you respond to the comments in the RFC patch?  No point
> > duplicating them.
> > 
> >>  You mentioned in my RFC patch review that "gd is
> >> already initialized at the beginning of board_init_f()". I think that's not the
> >> case. This change is still needed to get gd correct value.
> > 
> > Could you elaborate?  You're setting it to the same value that
> > cpu_init_early_f() set it to (on mpc85xx -- not all PPC).
> > 
> 
> Before this change, we have
> 
> #ifndef CONFIG_X86
> gd_t data;
> 
> gd = &data;
> #endif
> 
> This is overriding the gd.

Yes, as I said, "If PPC needs gd before board_init_f(), then add PPC (or
some other relevant symbol if it's not all PPC) to the #ifndef X86."

> For PPC, gd is set in different places. Eg,  cpu_init_early_f() for mpc85xx,
> cpu_init_f() for for mpc512x, mpc5xxx, mpc8260, mpc83xx, mpc86xx. They are all
> in different files. Since we have been using this assignment in
> arch/powerpc/lib/board.c for all PPC, it should be safe and clear to have
> correct assignment here.

The generic board is an opportunity to clean up cruft.  Non-mpc85xx can
be dealt with when they get converted.

What 85xx currently does seems bad -- it initializes the gd, clears it,
may or may not use it, then clears it again.  Figure out if the 85xx
code is using gd before board_init_f().  If it is, skip the clear in
board_init_f(), or at least clearly document what the pre-board_init_f()
usage is and that none of those values will last (but if that's the
case, why not use the stack for such temporary values?).  If it's not
used, then get rid of the early gd setting and use gd on the stack like
the other arches do.

-Scott

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[York Sun]

On 04/30/2014 11:24 AM, Scott Wood wrote:
>>
>> Before this change, we have
>>
>> #ifndef CONFIG_X86
>> gd_t data;
>>
>> gd = &data;
>> #endif
>>
>> This is overriding the gd.
> 
> Yes, as I said, "If PPC needs gd before board_init_f(), then add PPC (or
> some other relevant symbol if it's not all PPC) to the #ifndef X86."
> 
>> For PPC, gd is set in different places. Eg,  cpu_init_early_f() for mpc85xx,
>> cpu_init_f() for for mpc512x, mpc5xxx, mpc8260, mpc83xx, mpc86xx. They are all
>> in different files. Since we have been using this assignment in
>> arch/powerpc/lib/board.c for all PPC, it should be safe and clear to have
>> correct assignment here.
> 
> The generic board is an opportunity to clean up cruft.  Non-mpc85xx can
> be dealt with when they get converted.
> 
> What 85xx currently does seems bad -- it initializes the gd, clears it,
> may or may not use it, then clears it again.  Figure out if the 85xx
> code is using gd before board_init_f().  If it is, skip the clear in
> board_init_f(), or at least clearly document what the pre-board_init_f()
> usage is and that none of those values will last (but if that's the
> case, why not use the stack for such temporary values?).  If it's not
> used, then get rid of the early gd setting and use gd on the stack like
> the other arches do.
> 

Some serious cleaning is needed. Hold on this patch. I will see how fast I can go.

York

[U-Boot] [PATCH 1/2] common/board_f: Add back gd init

[York Sun]

On 04/30/2014 11:24 AM, Scott Wood wrote:
>> Before this change, we have
>>
>> #ifndef CONFIG_X86
>> gd_t data;
>>
>> gd = &data;
>> #endif
>>
>> This is overriding the gd.
> 
> Yes, as I said, "If PPC needs gd before board_init_f(), then add PPC (or
> some other relevant symbol if it's not all PPC) to the #ifndef X86."
> 
>> For PPC, gd is set in different places. Eg,  cpu_init_early_f() for mpc85xx,
>> cpu_init_f() for for mpc512x, mpc5xxx, mpc8260, mpc83xx, mpc86xx. They are all
>> in different files. Since we have been using this assignment in
>> arch/powerpc/lib/board.c for all PPC, it should be safe and clear to have
>> correct assignment here.
> 
> The generic board is an opportunity to clean up cruft.  Non-mpc85xx can
> be dealt with when they get converted.
> 
> What 85xx currently does seems bad -- it initializes the gd, clears it,
> may or may not use it, then clears it again.  Figure out if the 85xx
> code is using gd before board_init_f().  If it is, skip the clear in
> board_init_f(), or at least clearly document what the pre-board_init_f()
> usage is and that none of those values will last (but if that's the
> case, why not use the stack for such temporary values?).  If it's not
> used, then get rid of the early gd setting and use gd on the stack like
> the other arches do.
> 

I took a deeper look at 85xx code. We use gd before board_init_f. I can move
some of them to use return value, but for "law" operations in
arch/powerpc/cpu/mpc8xxx/law.c, we need "gd".

York