[U-Boot-Users] [PATCH 2/2] Add board specific files for BUBBATWO

[Kim Phillips]

On Mon,  2 Jun 2008 15:16:02 +0200
Tor Krill <tor at excito.com> wrote:

> +U_BOOT_CMD (bubbacmd, 3, 1, do_bubbacmd,
> +		"bubbacmd- Board specific commands for Bubba TWO\n",
> +		"spindown [dev] - spin down disk dev\n"
> +		"bubbacmd spinup [dev] - spin up disk dev\n"
> +		"bubbacmd button - read button status\n");
> +#endif

I the spinup/spindown commands should be part of cmd_ide, no?  Or
rather, should they be eliminated in favour of "Das U-Boot way" of only
powering up devices when accesses are requested?  i.e, power up on a
disk read command and power down after it's done?

btw, this also prevents me from applying this board support patch
because of the sata_sil3114 driver dependency here.

> +int getboardversion (void)
> +{
> +	volatile immap_t *im = (immap_t *) CFG_IMMR;
> +	int rev = im->gpio[0].dat & (HW_ID0 | HW_ID1 | HW_ID2);

looks like the and op is unnecessary here..plus we're not doing
volatile i/o accesses any more - use {in,out}_be32(), {set,clr}bits32()
etc., this occurs many other places in this patch..

> +
> +	rev =
> +		((rev & HW_ID0) >> 3) | ((rev & HW_ID1) >> 2) | ((rev & HW_ID2) <<
> +								1);
> +
> +	return rev;
> +}

return ((rev & HW_ID0) >> 3) | ((rev & HW_ID1) >> 2) |
       ((rev & HW_ID2) << 1);


> +ulong board_flash_get_legacy (ulong base, int banknum, flash_info_t * info)
> +{
> +	if (banknum == 0) {	/* non-CFI boot flash */
> +		info->portwidth = FLASH_CFI_16BIT;
> +		info->chipwidth = FLASH_CFI_BY16;
> +		info->interface = FLASH_CFI_X16;
> +		return 1;
> +	} else {
> +		return 0;
> +	}

else is superfluous

> +/* Lookup table for the different boardversions */
> +static struct {
> +	u32 memsize;
> +	u32 config;
> +} meminfo[] = {
> +	/* 0 - revA  prototypes */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 1 - revB */
> +	{
> +		memsize: 256, config:CFG_DDR_CONFIG_256},
> +	/* 2 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 3 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 4 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 5 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 6 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128},
> +	/* 7 - empty */
> +	{
> +		memsize: 128, config:CFG_DDR_CONFIG_128}
> +};

would something like this be neater:

	{ 128, CFG_DDR_CONFIG_128 }, /* 7 - empty */

?

Kim