Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Wolfgang Denk 
Dear haiying.w...@freescale.com,

In message 1296499317-26616-4-git-send-email-haiying.w...@freescale.com you 
wrote:
 From: Haiying Wang haiying.w...@freescale.com
 
 Support P1021MDS board to boot from NAND flash (No NOR flash on this
 board). And because P1021 only has 256K L2 SRAM, which can not used for final
 uboot image, this patch also enables the TPL BOOT on P1021MDS so that DDR can
 be initialized in L2 SRAM through SPD code. So there are three stage uboot
 images:
 * nand_spl, pad from 4KB size to 16KB, load tpl_boot from offset 16KB in NAND.
 * tpl_boot, 112KB size. The env variables are copied to offset 128KB
   in L2 SRAM, so that ddr spd code can get the interleaving mode setting in 
 env.
   It loads final uboot image from offset 128KB in NAND.
 * final uboot image, size is variable depends on the functions enabled.


 diff --git a/board/freescale/p1021mds/config.mk 
 b/board/freescale/p1021mds/config.mk
 new file mode 100644
 index 000..3888f61
 --- /dev/null
 +++ b/board/freescale/p1021mds/config.mk
...
 +ifndef NAND_SPL
 +ifndef IN_TPL
 +ifeq ($(CONFIG_NAND), y)
 +LDSCRIPT := $(TOPDIR)/$(CPUDIR)/u-boot-nand.lds
 +endif
 +endif
 +endif

Why is this config.mk needed?  Can you not do all this in the board
config file instead?

 diff --git a/board/freescale/p1021mds/ddr.c b/board/freescale/p1021mds/ddr.c
 new file mode 100644
 index 000..594a4a8
 --- /dev/null
 +++ b/board/freescale/p1021mds/ddr.c

It seems there are a number of functions here which ar actually shared
with other files, for example board/freescale/p1022ds/ddr.c.

I wonder if it is not possible to use more common code here - especially
given the fact that we already have a nice collection of such files:

board/freescale/corenet_ds/ddr.c
board/freescale/mpc8536ds/ddr.c
board/freescale/mpc8540ads/ddr.c
board/freescale/mpc8541cds/ddr.c
board/freescale/mpc8544ds/ddr.c
board/freescale/mpc8548cds/ddr.c
board/freescale/mpc8555cds/ddr.c
board/freescale/mpc8560ads/ddr.c
board/freescale/mpc8568mds/ddr.c
board/freescale/mpc8569mds/ddr.c
board/freescale/mpc8572ds/ddr.c
board/freescale/mpc8610hpcd/ddr.c
board/freescale/mpc8641hpcn/ddr.c
board/freescale/p1022ds/ddr.c
board/freescale/p1_p2_rdb/ddr.c
board/freescale/p2020ds/ddr.c

 diff --git a/board/freescale/p1021mds/p1021mds.c 
 b/board/freescale/p1021mds/p1021mds.c
 new file mode 100644
 index 000..c7a7e57
 --- /dev/null
 +++ b/board/freescale/p1021mds/p1021mds.c
...
 +extern void cpu_mp_lmb_reserve(struct lmb *lmb);

Please move prototypes to header file.

 +void board_lmb_reserve(struct lmb *lmb)
 +{
 + cpu_mp_lmb_reserve(lmb);
 +}

How many board/freescale/name/name.c file share this same code?


 diff --git a/board/freescale/p1021mds/tlb.c b/board/freescale/p1021mds/tlb.c
 new file mode 100644
 index 000..30af6dd
 --- /dev/null
 +++ b/board/freescale/p1021mds/tlb.c

How much of this is actually different from - say -
board/freescale/p1022ds/tlb.c ?


...
 +/*
 + * Environment Configuration
 + */
 +#define CONFIG_HOSTNAME  p1021mds
 +#define CONFIG_ROOTPATH  /nfsroot
 +#define CONFIG_BOOTFILE  your.uImage

Please rather omit the setting instead of using fillers that are of no
practical value.

 +#define CONFIG_LOADADDR  100   /*default location for tftp and 
 bootm*/
 +
 +#define CONFIG_BOOTDELAY 10   /* -1 disables auto-boot */
 +#undef  CONFIG_BOOTARGS   /* the boot command will set bootargs*/
 +
 +#define  CONFIG_EXTRA_ENV_SETTINGS   
 \
 + netdev=eth0\0 \
 + consoledev=ttyS0\0\
 + ramdiskaddr=200\0 \
 + ramdiskfile=your.ramdisk.u-boot\0 \

Ditto. [BTW: why ramdisk.u-boot? U-Boot does not use ramdisks.
The ramdisk is only used for some OS, so that should probably be
...ramdisk.linux instead?]

 + fdtaddr=c0\0  \
 + fdtfile=your.fdt.dtb\0\

Ditto. [Are fdt and dtb not redundant?]

 diff --git a/tpl/board/freescale/p1021mds/tpl_boot.c 
 b/tpl/board/freescale/p1021mds/tpl_boot.c
 new file mode 100644
 index 000..386d76c
 --- /dev/null
 +++ b/tpl/board/freescale/p1021mds/tpl_boot.c
...
 +extern void nand_load(unsigned int offs, int uboot_size, uchar *dst);
 +extern phys_size_t init_ddr_dram(void);

Please move prototypes to header files.


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
Any sufficiently advanced bug is indistinguishable from a feature.

Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Scott Wood 
On Mon, 31 Jan 2011 21:18:35 +0100
Wolfgang Denk w...@denx.de wrote:

 Dear Scott Wood,
 
 In message 20110131140801.33609...@udp111988uds.am.freescale.net you wrote:
 
   Please rather omit the setting instead of using fillers that are of no
   practical value.
  
  Well, they do make it easier for a user to quickly see what the names
  are that U-Boot expects for such commonly used things, rather than
  having to scan the manual.
 
 I doubt both the quicly see part (in such a long list of settings)
 and the rather than having to scan the manual part.

I've found it convenient, along with the more meaningful error
messages if I forget to replace one of them.  YMMV.

+   ramdiskfile=your.ramdisk.u-boot\0 
\
   
   Ditto. [BTW: why ramdisk.u-boot? U-Boot does not use ramdisks.
   The ramdisk is only used for some OS, so that should probably be
   ...ramdisk.linux instead?]
  
  We often use the .u-boot suffix on ramdisks that have been wrapped
  with a uImage header.
 
 That would be a uRamdisk then (similar to uImage).

Is anyone actually calling it that?  What if I have multiple ramdisk
images that I want to call different names?

FWIW, for non-Linux OS images we sometimes use .uImage as a suffix.

-Scott

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

Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Kumar Gala 
 It seems there are a number of functions here which ar actually shared
 with other files, for example board/freescale/p1022ds/ddr.c.
 
 I wonder if it is not possible to use more common code here - especially
 given the fact that we already have a nice collection of such files:
 
   board/freescale/corenet_ds/ddr.c
   board/freescale/mpc8536ds/ddr.c
   board/freescale/mpc8540ads/ddr.c
   board/freescale/mpc8541cds/ddr.c
   board/freescale/mpc8544ds/ddr.c
   board/freescale/mpc8548cds/ddr.c
   board/freescale/mpc8555cds/ddr.c
   board/freescale/mpc8560ads/ddr.c
   board/freescale/mpc8568mds/ddr.c
   board/freescale/mpc8569mds/ddr.c
   board/freescale/mpc8572ds/ddr.c
   board/freescale/mpc8610hpcd/ddr.c
   board/freescale/mpc8641hpcn/ddr.c
   board/freescale/p1022ds/ddr.c
   board/freescale/p1_p2_rdb/ddr.c
   board/freescale/p2020ds/ddr.c

We've already done that, the code in these files is board specific 
params/tuning of DDR params.


   
 diff --git a/board/freescale/p1021mds/p1021mds.c 
 b/board/freescale/p1021mds/p1021mds.c
 new file mode 100644
 index 000..c7a7e57
 --- /dev/null
 +++ b/board/freescale/p1021mds/p1021mds.c
 ...
 +extern void cpu_mp_lmb_reserve(struct lmb *lmb);

We have this in asm/mp.h already.

Will cleanup the other guys

 Please move prototypes to header file.
 
 +void board_lmb_reserve(struct lmb *lmb)
 +{
 +cpu_mp_lmb_reserve(lmb);
 +}
 
 How many board/freescale/name/name.c file share this same code?

All of our multicore parts do this, we could move this into other places like 
arch_lmb_reserve().

 diff --git a/board/freescale/p1021mds/tlb.c b/board/freescale/p1021mds/tlb.c
 new file mode 100644
 index 000..30af6dd
 --- /dev/null
 +++ b/board/freescale/p1021mds/tlb.c
 
 How much of this is actually different from - say -
 board/freescale/p1022ds/tlb.c ?

Its mostly board specific.

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

Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Kumar Gala 

On Jan 31, 2011, at 3:39 PM, Haiying Wang wrote:

 On Mon, 2011-01-31 at 21:03 +0100, Wolfgang Denk wrote:
 Dear haiying.w...@freescale.com,
 diff --git a/board/freescale/p1021mds/config.mk 
 b/board/freescale/p1021mds/config.mk
 new file mode 100644
 index 000..3888f61
 --- /dev/null
 +++ b/board/freescale/p1021mds/config.mk
 ...
 +ifndef NAND_SPL
 +ifndef IN_TPL
 +ifeq ($(CONFIG_NAND), y)
 +LDSCRIPT := $(TOPDIR)/$(CPUDIR)/u-boot-nand.lds
 +endif
 +endif
 +endif
 
 Why is this config.mk needed?  Can you not do all this in the board
 config file instead?
 Do you mean the board header file or arch/powerpc/config.mk? I did not see 
 any LDSCRIPT defined in Freescale board header file.

I think something like:

diff --git a/include/configs/MPC8572DS.h b/include/configs/MPC8572DS.h
index e6b60cf..f2d6cdb 100644
--- a/include/configs/MPC8572DS.h
+++ b/include/configs/MPC8572DS.h
@@ -37,6 +37,7 @@
 #define CONFIG_NAND_U_BOOT
 #define CONFIG_RAMBOOT_NAND
 #ifdef CONFIG_NAND_SPL
+#define CONFIG_SYS_LDSCRIPT arch/powerpc/cpu/mpc85xx/u-boot-nand.lds
 #define CONFIG_SYS_TEXT_BASE_SPL 0xfff0
 #define CONFIG_SYS_MONITOR_BASECONFIG_SYS_TEXT_BASE_SPL /* start of mon

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

Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Wolfgang Denk 
Dear Haiying Wang,

In message 1296509955.2049.543.camel@haiying-laptop you wrote:

  Why is this config.mk needed?  Can you not do all this in the board
  config file instead?
 Do you mean the board header file or arch/powerpc/config.mk? I did not see 
 any LDSCRIPT defined in Freescale board header file.

I mean the board config header file, include/configs/name.h

   diff --git a/board/freescale/p1021mds/ddr.c 
   b/board/freescale/p1021mds/ddr.c
   new file mode 100644
   index 000..594a4a8
   --- /dev/null
   +++ b/board/freescale/p1021mds/ddr.c
  
  It seems there are a number of functions here which ar actually shared
  with other files, for example board/freescale/p1022ds/ddr.c.
 Every boards has its board specific ddr parameters which are defined the its 
 own board ddr.c. The common code for ddr has been defined in 
 arch/powerpc/cpu/mpc8xxx/ddr/.

Well, but there is tons of common code. For example, all of

board/freescale/corenet_ds/ddr.c
board/freescale/mpc8536ds/ddr.c
board/freescale/mpc8540ads/ddr.c
board/freescale/mpc8541cds/ddr.c
board/freescale/mpc8544ds/ddr.c
board/freescale/mpc8548cds/ddr.c
board/freescale/mpc8555cds/ddr.c
board/freescale/mpc8560ads/ddr.c
board/freescale/mpc8568mds/ddr.c
board/freescale/mpc8569mds/ddr.c
board/freescale/mpc8572ds/ddr.c
board/freescale/mpc8610hpcd/ddr.c
board/freescale/mpc8641hpcn/ddr.c
board/freescale/p1021mds/ddr.c
board/freescale/p1022ds/ddr.c
board/freescale/p2020ds/ddr.c

share the same function

unsigned int fsl_ddr_get_mem_data_rate(void)
{
return get_ddr_freq(0);
}

And

board/freescale/p1021mds/ddr.c
board/freescale/p1022ds/ddr.c

share (except for the comment) the same

void fsl_ddr_get_spd(ddr3_spd_eeprom_t *ctrl_dimms_spd, unsigned int 
ctrl_num)

while
board/freescale/corenet_ds/ddr.c
board/freescale/mpc8569mds/ddr.c

use another variant, but again both boards the same one.


 If you go to see each ddr.c, you can find there is
 fsl_ddr_board_options() which defines the different values for each
 board. Also fsl_ddr_get_spd() is also highly dependent on board, like
 ddr type(ddr2 or ddr3), i2c spd eeprom address, ddr controller# etc.

Actually this is not quite true. See examples above.

   +void board_lmb_reserve(struct lmb *lmb)
   +{
   + cpu_mp_lmb_reserve(lmb);
   +}
  
  How many board/freescale/name/name.c file share this same code?
 There are some, but I don't know whether there will be difference coming in 
 later.

Then we can use a common implementation for all where it fits, and
use board specific code only where needed.

   diff --git a/board/freescale/p1021mds/tlb.c 
   b/board/freescale/p1021mds/tlb.c
   new file mode 100644
   index 000..30af6dd
   --- /dev/null
   +++ b/board/freescale/p1021mds/tlb.c
  
  How much of this is actually different from - say -
  board/freescale/p1022ds/tlb.c ?
 The tlb.c is also a highly board dependent file. Different boards have 
 different supported peripherals. If you look at p1021 and p1022's tlb.c 
 files, you can see p1022ds has 3 PCIE, P1021 has 2, P1022ds has NOR flash, 
 P1021MDS only has NAND flash... etc

Yes, there are differences. But it seems there is more common code
than differing one?

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
A penny saved is a penny to squander.
- Ambrose Bierce
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/6] P1021: add P1021MDS board support

2011-01-31 Thread Wolfgang Denk 
Dear Kumar Gala,

In message ae2e740c-25bb-48db-bb6f-a92b91d57...@kernel.crashing.org you wrote:
 
  +LDSCRIPT := $(TOPDIR)/$(CPUDIR)/u-boot-nand.lds
  +endif
  +endif
  +endif
  
  Why is this config.mk needed?  Can you not do all this in the board
  config file instead?
  Do you mean the board header file or arch/powerpc/config.mk? I did not
 see any LDSCRIPT defined in Freescale board header file.
 
 I think something like:
 
 diff --git a/include/configs/MPC8572DS.h b/include/configs/MPC8572DS.h
 index e6b60cf..f2d6cdb 100644
 --- a/include/configs/MPC8572DS.h
 +++ b/include/configs/MPC8572DS.h
 @@ -37,6 +37,7 @@
  #define CONFIG_NAND_U_BOOT
  #define CONFIG_RAMBOOT_NAND
  #ifdef CONFIG_NAND_SPL
 +#define CONFIG_SYS_LDSCRIPT arch/powerpc/cpu/mpc85xx/u-boot-nand.lds

This will eventually break with out of tree builds.

Rather use

#define CONFIG_SYS_LDSCRIPT $(TOPDIR)/$(CPUDIR)/u-boot-nand.lds

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
There's a way out of any cage.
-- Captain Christopher Pike, The Menagerie (The Cage),
   stardate unknown.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot