[U-Boot] [U-BOOT] [PATCH 2/6] mv: seperate kirkwood and mmp from common setting

[Lei Wen]

Hi Prafula,

On Thu, Jan 6, 2011 at 2:23 AM, Prafulla Wadaskar <prafulla at marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:adrian.wenl at gmail.com]
>> Sent: Wednesday, January 05, 2011 8:59 PM
>> To: Prafulla Wadaskar
>> Cc: Lei Wen; u-boot at 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