| Message ID | 1398474623-4709-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | RFC |
| Headers | show |
Dear York Sun, In message <1398474623-4709-1-git-send-email-yorksun@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
On 04/26/2014 02:22 AM, Wolfgang Denk wrote: > Dear York Sun, > > In message <1398474623-4709-1-git-send-email-yorksun@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
Dear York Sun, In message <535BE09F.80005@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
Hi York, On 26 April 2014 10:36, York Sun <yorksun@freescale.com> wrote: > On 04/26/2014 02:22 AM, Wolfgang Denk wrote: >> Dear York Sun, >> >> In message <1398474623-4709-1-git-send-email-yorksun@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
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
On Sat, 2014-04-26 at 21:36 +0200, Wolfgang Denk wrote: > Dear York Sun, > > In message <535BE09F.80005@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
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
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(sjg@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
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 <yorksun@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(-)