Hi Prafula,
On Thu, Jan 6, 2011 at 2:23 AM, Prafulla Wadaskar prafulla@marvell.com wrote:
-----Original Message----- From: Lei Wen [mailto:adrian.wenl@gmail.com] Sent: Wednesday, January 05, 2011 8:59 PM To: Prafulla Wadaskar Cc: Lei Wen; u-boot@lists.denx.de; Yu Tang; Ashish Karkare; Prabhanjan Sarnaik Subject: Re: [U-BOOT] [PATCH 2/6] mv: seperate kirkwood and mmp from common setting
...snip...
- NAND configuration
- */
+#ifdef CONFIG_CMD_NAND +#define CONFIG_NAND_KIRKWOOD +#define CONFIG_SYS_MAX_NAND_DEVICE 1 +#define NAND_MAX_CHIPS 1 +#define CONFIG_SYS_NAND_BASE 0xD8000000 /*
MV_DEFADR_NANDF */
+#define NAND_ALLOW_ERASE_ALL 1 +#define CONFIG_SYS_64BIT_VSPRINTF /* needed for nand_util.c */ +#endif
I think split below makes more sense- overall objective is to avoid
code duplication here.
For mv-common.h +/*
- NAND configuration
- */
+#ifdef CONFIG_CMD_NAND +#define CONFIG_SYS_MAX_NAND_DEVICE 1 +#define NAND_MAX_CHIPS 1 +#define CONFIG_SYS_64BIT_VSPRINTF /* needed for nand_util.c */ +#endif
For arch-kirkwood/config.h +/*
- NAND configuration
- */
+#ifdef CONFIG_CMD_NAND +#define CONFIG_NAND_KIRKWOOD +#define CONFIG_SYS_NAND_BASE 0xD8000000 /*
MV_DEFADR_NANDF */
+#define NAND_ALLOW_ERASE_ALL 1 +#endif
+/*
- SPI Flash configuration
- */
+#ifdef CONFIG_CMD_SF +#define CONFIG_SPI_FLASH 1 +#define CONFIG_HARD_SPI 1 +#define CONFIG_KIRKWOOD_SPI 1 +#define CONFIG_SPI_FLASH_MACRONIX 1 +#define CONFIG_ENV_SPI_BUS 0 +#define CONFIG_ENV_SPI_CS 0 +#define CONFIG_ENV_SPI_MAX_HZ 50000000 /*50Mhz
*/
+#endif
Same applies for rest of the definitions. mv-common.h should represent common definitions for
Kirkwood,armada100, and future SoCs based boards so that
Yes, agree... Since current armada100 havn't added nand and spi support, how about move those nand and spi definition to kirkwood and then after armada100 add the nand, we reconsider to move back the common back?
On the other hand, keep your patch small by not moving this driver specific stuff to arch-kirkwood/config.h.
As new common driver will get supported on armada100, we will push them to config.h, this way even future patches will also be smaller.
Especially for those code below, I really believe this should not be put into the common code... #ifndef CONFIG_ARMADA100 /* will be removed latter */-#define CONFIG_CMD_EXT2 #define CONFIG_CMD_JFFS2 #define CONFIG_CMD_FAT #define CONFIG_CMD_UBI #define CONFIG_CMD_UBIFS #define CONFIG_RBTREE #define CONFIG_MTD_DEVICE /* needed for mtdparts commands */ #define CONFIG_MTD_PARTITIONS #define CONFIG_CMD_MTDPARTS #define CONFIG_LZO #endif
Well, we can.. only replace #ifndef CONFIG_ARMADA100 with #ifdef CONFIG_SYS_MVFS and define this macro in arch-kirkwood/config.h, this way in future after enabling ide, nand, spi driver support just defining the same macro in armada100/config.h will enable the file system support for armada100 platforms.
I think not all platform under marvell would need to enable all those configure... What if it only want to enable one setting from this big group, like it only want the CONFIG_CMD_FAT, then you need to it set the CONFIG_SYS_MVFS with other unnessary setting enabled...
It increase the image file size... and I don't think a bootloader should have full function support that uboot can support... Each board should tailor this fs setting by its willing, not set all by default...
For this patch set, if you think it just should keep patch small, I can leave it at this moment. But I think it should be move board specific configure file, like kirkwood/config.h...
Thanks, Lei