Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-30 Thread York Sun
On 04/29/2014 04:12 PM, Scott Wood wrote:
 On Sat, 2014-04-26 at 09:36 -0700, York Sun wrote:
 On 04/26/2014 02:22 AM, Wolfgang Denk wrote:
 +#ifdef CONFIG_PPC
 +  gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
 +  __asm__ __volatile__(:::memory);
 +#endif

 Again, this is a global change.  Why is this now needed?


 It has been this way for powerpc. Do we have an alternative?
 
 gd is already initialized at the beginning of board_init_f().  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.  In any case, there
 should be no need to add yet another initialization of gd.
 cpu_init_early_f() already initialized and cleared it.  Also, if gd
 really is needed before board_init_f(), then we probably need to skip
 clearing gd in board_init_f().

For variant PPC part, gd is initialized in cpu_init_f or cpu_init_early_f. It
shouldn't be overwritten by gd = data; in board_init_f.

gd is not cleared in board_init_f for the SoCs we care. But gd may be missed for
74xx and other old SoCs if not set in board_init_f.

 
 As for the memory clobber, if nobody can come up with a reason for its
 existence, then just let it go away.  At the very least, don't copy the
 barrier without also copying the comment that went with it -- but I'm
 really not seeing what it's trying to order.  gd is a register, not
 memory.  Maybe some versions of GCC had a bug that the clobber worked
 around -- does it apply to any recent GCC?  In any case, for mpc85xx, gd
 was previously initalized as discussed above.
 

We can probably remove the memory boundary. I wouldn't know if any legacy
compiler will have any issue though.

York


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-29 Thread Scott Wood
On Sat, 2014-04-26 at 09:36 -0700, York Sun wrote:
 On 04/26/2014 02:22 AM, Wolfgang Denk wrote:
  +#ifdef CONFIG_PPC
  +  gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
  +  __asm__ __volatile__(:::memory);
  +#endif
  
  Again, this is a global change.  Why is this now needed?
  
 
 It has been this way for powerpc. Do we have an alternative?

gd is already initialized at the beginning of board_init_f().  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.  In any case, there
should be no need to add yet another initialization of gd.
cpu_init_early_f() already initialized and cleared it.  Also, if gd
really is needed before board_init_f(), then we probably need to skip
clearing gd in board_init_f().

As for the memory clobber, if nobody can come up with a reason for its
existence, then just let it go away.  At the very least, don't copy the
barrier without also copying the comment that went with it -- but I'm
really not seeing what it's trying to order.  gd is a register, not
memory.  Maybe some versions of GCC had a bug that the clobber worked
around -- does it apply to any recent GCC?  In any case, for mpc85xx, gd
was previously initalized as discussed above.

-Scott


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-29 Thread Scott Wood
On Sat, 2014-04-26 at 21:36 +0200, Wolfgang Denk wrote:
 Dear York Sun,
 
 In message 535be09f.80...@freescale.com you wrote:
   Change return value for mac_read_from_eeprom() when mismatch happens to
   prevent calling hang().
   
   You mean, you just ignore the error?  This is a change of the cpolicy
   that has nothing to do with generic board support, right?  Why should
   this be done now, i. e. why has it been accepted and considered to be
   working before?
  
  This function is helpful but not critical. If reading fails, the board 
  should
  continue to boot then users will have a chance to fix it. The new generic 
  board
  treats this as other functions in board_init_r. Any error will cause 
  hanging.
 
 You repeat yourself, but you do not answer my questions.

Isn't this the first time that mac_read_from_eeprom() in
board/freescale/common/sys_eeprom.c was used with common/board_r.c?
Previously it was called in arch/powerpc/lib/board.c which only used the
initfunc array approach for board_init_f().  board_init_r() called
mac_read_from_eeprom() and ignored the return.

It does seem excessive to hang on initcall failure, rather than print an
error and continue.

-Scott


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-26 Thread Wolfgang Denk
Dear York Sun,

In message 1398474623-4709-1-git-send-email-york...@freescale.com you wrote:

 Add #ifdef CONFIG_OF_CONTROL for reserve_fdt(), setup_fdt(), reloc_fdt().

This looks wrong to me.  This is a global file, and you are affecting
a ton of unrelated boards.

 Set initial value for gd. Powerpc SoCs use locked cache as init RAM.

Well, some of them do, not all.

 Change return value for mac_read_from_eeprom() when mismatch happens to
 prevent calling hang().

You mean, you just ignore the error?  This is a change of the cpolicy
that has nothing to do with generic board support, right?  Why should
this be done now, i. e. why has it been accepted and considered to be
working before?

  board/freescale/common/sys_eeprom.c |2 +-
  common/board_f.c|   18 +-
  include/configs/MPC8536DS.h |2 ++
  3 files changed, 20 insertions(+), 2 deletions(-)

I think thease are at least 2, eventually 3 independent changes.  You
should split them in several commits.

 +#ifdef CONFIG_PPC
 + gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
 + __asm__ __volatile__(:::memory);
 +#endif

Again, this is a global change.  Why is this now needed?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The high cost of living hasn't affected its popularity.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-26 Thread York Sun
On 04/26/2014 02:22 AM, Wolfgang Denk wrote:
 Dear York Sun,
 
 In message 1398474623-4709-1-git-send-email-york...@freescale.com you wrote:

 Add #ifdef CONFIG_OF_CONTROL for reserve_fdt(), setup_fdt(), reloc_fdt().
 
 This looks wrong to me.  This is a global file, and you are affecting
 a ton of unrelated boards.

Understood it is a global file. These functions deal with FDT. They should not
be in the path if targets don't use device tree to configure their devices. If
there is another more appropriate macro to use, please let me know. Take the
example I used, MPC8536DS, the gd-fdt_blob would have incorrect value because
it doesn't use device tree.

 
 Set initial value for gd. Powerpc SoCs use locked cache as init RAM.
 
 Well, some of them do, not all.

arch/powerpc/lib/board.c uses this way. I presume it is safe to use for all PPC
parts.nn

 
 Change return value for mac_read_from_eeprom() when mismatch happens to
 prevent calling hang().
 
 You mean, you just ignore the error?  This is a change of the cpolicy
 that has nothing to do with generic board support, right?  Why should
 this be done now, i. e. why has it been accepted and considered to be
 working before?

This function is helpful but not critical. If reading fails, the board should
continue to boot then users will have a chance to fix it. The new generic board
treats this as other functions in board_init_r. Any error will cause hanging.


 
  board/freescale/common/sys_eeprom.c |2 +-
  common/board_f.c|   18 +-
  include/configs/MPC8536DS.h |2 ++
  3 files changed, 20 insertions(+), 2 deletions(-)
 
 I think thease are at least 2, eventually 3 independent changes.  You
 should split them in several commits.

Again, this patch is for discussion. Once we are clear what we should fix, I
will generate appropriate patch set.

 
 +#ifdef CONFIG_PPC
 +gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
 +__asm__ __volatile__(:::memory);
 +#endif
 
 Again, this is a global change.  Why is this now needed?
 

It has been this way for powerpc. Do we have an alternative?

York


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-26 Thread Wolfgang Denk
Dear York Sun,

In message 535be09f.80...@freescale.com you wrote:

  Add #ifdef CONFIG_OF_CONTROL for reserve_fdt(), setup_fdt(), reloc_fdt().
  
  This looks wrong to me.  This is a global file, and you are affecting
  a ton of unrelated boards.
 
 Understood it is a global file. These functions deal with FDT. They should not

It appears you assume every board using the DT has CONFIG_OF_CONTROL
set in its configuration?  For example, all PPC boards have been using
the DT for years, but count how many of them define CONFIG_OF_CONTROL
...

How many other boards have you actually tested with this
modification applied?

  Set initial value for gd. Powerpc SoCs use locked cache as init RAM.
  
  Well, some of them do, not all.
 
 arch/powerpc/lib/board.c uses this way. I presume it is safe to use for all 
 PPC
 parts.nn

At least your comment is wrong.  And again:  how many other boards
have actually been testes?

And why is this now necessary?  Everything has been working perfectly
fine without that for all other boards - so why do we now need such a
global modification just for this specific board?

  Change return value for mac_read_from_eeprom() when mismatch happens to
  prevent calling hang().
  
  You mean, you just ignore the error?  This is a change of the cpolicy
  that has nothing to do with generic board support, right?  Why should
  this be done now, i. e. why has it been accepted and considered to be
  working before?
 
 This function is helpful but not critical. If reading fails, the board should
 continue to boot then users will have a chance to fix it. The new generic 
 board
 treats this as other functions in board_init_r. Any error will cause hanging.

You repeat yourself, but you do not answer my questions.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
What is mind?  No matter.  What is matter?  Never mind.
  -- Thomas Hewitt Key, 1799-1875
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-26 Thread Simon Glass
Hi York,

On 26 April 2014 10:36, York Sun york...@freescale.com wrote:
 On 04/26/2014 02:22 AM, Wolfgang Denk wrote:
 Dear York Sun,

 In message 1398474623-4709-1-git-send-email-york...@freescale.com you 
 wrote:

 Add #ifdef CONFIG_OF_CONTROL for reserve_fdt(), setup_fdt(), reloc_fdt().

 This looks wrong to me.  This is a global file, and you are affecting
 a ton of unrelated boards.

 Understood it is a global file. These functions deal with FDT. They should not
 be in the path if targets don't use device tree to configure their devices. If
 there is another more appropriate macro to use, please let me know. Take the
 example I used, MPC8536DS, the gd-fdt_blob would have incorrect value because
 it doesn't use device tree.

It should be NULL, so none of this code will do anything.

See the code at the top of board_init_f():

#if !defined(CONFIG_CPM2)  !defined(CONFIG_MPC512X)  \
!defined(CONFIG_MPC83xx)  !defined(CONFIG_MPC85xx)  \
!defined(CONFIG_MPC86xx)  !defined(CONFIG_X86)
   zero_global_data();
#endif

For x86 the data is zeroed for us by previous start-up code. Possibly
the same is true of ARM. For the MPC devices I copied what was there,
but it is possible that it shold be zeroed, or at least fdt_blob
should be zeroed.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


[U-Boot] [RFC] RFC: convert MPC8536DS to use generic board

2014-04-25 Thread York Sun
This patch converts MC8536DS to use generic board. This is for discussion.
Do NOT apply.

To make it work

Add #ifdef CONFIG_OF_CONTROL for reserve_fdt(), setup_fdt(), reloc_fdt().
Set initial value for gd. Powerpc SoCs use locked cache as init RAM.
Change return value for mac_read_from_eeprom() when mismatch happens to
prevent calling hang().

Signed-off-by: York Sun york...@freescale.com
---
 board/freescale/common/sys_eeprom.c |2 +-
 common/board_f.c|   18 +-
 include/configs/MPC8536DS.h |2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/board/freescale/common/sys_eeprom.c 
b/board/freescale/common/sys_eeprom.c
index 9c18dd8..a3c37ff 100644
--- a/board/freescale/common/sys_eeprom.c
+++ b/board/freescale/common/sys_eeprom.c
@@ -447,7 +447,7 @@ int mac_read_from_eeprom(void)
crcp = (void *)e + crc_offset;
if (crc != be32_to_cpu(*crcp)) {
printf(CRC mismatch (%08x != %08x)\n, crc, 
be32_to_cpu(e.crc));
-   return -1;
+   return 0;
}
 
 #ifdef CONFIG_SYS_I2C_EEPROM_NXID
diff --git a/common/board_f.c b/common/board_f.c
index f285bad..b972db8 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -335,6 +335,7 @@ static int setup_ram_buf(void)
 }
 #endif
 
+#ifdef CONFIG_OF_CONTROL
 static int setup_fdt(void)
 {
 #ifdef CONFIG_OF_EMBED
@@ -354,6 +355,7 @@ static int setup_fdt(void)
(uintptr_t)gd-fdt_blob);
return 0;
 }
+#endif
 
 /* Get the top of usable RAM */
 __weak ulong board_get_usable_ram_top(ulong total_size)
@@ -549,6 +551,7 @@ static int reserve_global_data(void)
return 0;
 }
 
+#ifdef CONFIG_OF_CONTROL
 static int reserve_fdt(void)
 {
/*
@@ -567,6 +570,7 @@ static int reserve_fdt(void)
 
return 0;
 }
+#endif
 
 static int reserve_stacks(void)
 {
@@ -584,7 +588,6 @@ static int reserve_stacks(void)
gd-start_addr_sp -= 16;
gd-start_addr_sp = ~0xf;
gd-irq_sp = gd-start_addr_sp;
-
/*
 * Handle architecture-specific things here
 * TODO(s...@chromium.org): Perhaps create arch_reserve_stack()
@@ -724,6 +727,7 @@ static int setup_dram_config(void)
return 0;
 }
 
+#ifdef CONFIG_OF_CONTROL
 static int reloc_fdt(void)
 {
if (gd-new_fdt) {
@@ -733,6 +737,7 @@ static int reloc_fdt(void)
 
return 0;
 }
+#endif
 
 static int setup_reloc(void)
 {
@@ -789,7 +794,9 @@ static init_fnc_t init_sequence_f[] = {
setup_ram_buf,
 #endif
setup_mon_len,
+#ifdef CONFIG_OF_CONTROL
setup_fdt,
+#endif
trace_early_init,
 #if defined(CONFIG_MPC85xx) || defined(CONFIG_MPC86xx)
/* TODO: can this go into arch_cpu_init()? */
@@ -945,7 +952,9 @@ static init_fnc_t init_sequence_f[] = {
 #endif
setup_machine,
reserve_global_data,
+#ifdef CONFIG_OF_CONTROL
reserve_fdt,
+#endif
reserve_stacks,
setup_dram_config,
show_dram_config,
@@ -960,7 +969,9 @@ static init_fnc_t init_sequence_f[] = {
setup_board_extra,
 #endif
INIT_FUNC_WATCHDOG_RESET
+#ifdef CONFIG_OF_CONTROL
reloc_fdt,
+#endif
setup_reloc,
 #if !defined(CONFIG_ARM)  !defined(CONFIG_SANDBOX)
jump_to_copy,
@@ -976,6 +987,11 @@ void board_init_f(ulong boot_flags)
gd = data;
 #endif
 
+#ifdef CONFIG_PPC
+   gd = (gd_t *) (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_GBL_DATA_OFFSET);
+   __asm__ __volatile__(:::memory);
+#endif
+
/*
 * Clear global data before it is accessed at debug print
 * in initcall_run_list. Otherwise the debug print probably
diff --git a/include/configs/MPC8536DS.h b/include/configs/MPC8536DS.h
index f15e162..c241736 100644
--- a/include/configs/MPC8536DS.h
+++ b/include/configs/MPC8536DS.h
@@ -11,6 +11,8 @@
 #ifndef __CONFIG_H
 #define __CONFIG_H
 
+#define CONFIG_SYS_GENERIC_BOARD
+
 #include ../board/freescale/common/ics307_clk.h
 
 #ifdef CONFIG_36BIT
-- 
1.7.9.5

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot