From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Thu, 15 Mar 2012 14:09:00 -0500
Subject: [U-Boot] [PATCH v4 14/27] Introduce generic pre-relocation
	board_f.c
In-Reply-To: <1331777784-8528-15-git-send-email-sjg@chromium.org>
References: <1331777784-8528-1-git-send-email-sjg@chromium.org>
	<1331777784-8528-15-git-send-email-sjg@chromium.org>
Message-ID: <4F623E4C.1080000@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 03/14/2012 09:16 PM, Simon Glass wrote:
> +/*
> + * sjg: IMO this code should be
> + * refactored to a single function, something like:
> + *
> + * void led_set_state(enum led_colour_t colour, int on);
> + */
> +/************************************************************************
> + * Coloured LED functionality
> + ************************************************************************
> + * May be supplied by boards if desired
> + */
> +inline void __coloured_LED_init(void) {}
> +void coloured_LED_init(void)
> +	__attribute__((weak, alias("__coloured_LED_init")));
> +inline void __red_led_on(void) {}
> +void red_led_on(void) __attribute__((weak, alias("__red_led_on")));
> +inline void __red_led_off(void) {}
> +void red_led_off(void) __attribute__((weak, alias("__red_led_off")));
> +inline void __green_led_on(void) {}
> +void green_led_on(void) __attribute__((weak, alias("__green_led_on")));
> +inline void __green_led_off(void) {}
> +void green_led_off(void) __attribute__((weak, alias("__green_led_off")));
> +inline void __yellow_led_on(void) {}
> +void yellow_led_on(void) __attribute__((weak, alias("__yellow_led_on")));
> +inline void __yellow_led_off(void) {}
> +void yellow_led_off(void) __attribute__((weak, alias("__yellow_led_off")));
> +inline void __blue_led_on(void) {}
> +void blue_led_on(void) __attribute__((weak, alias("__blue_led_on")));
> +inline void __blue_led_off(void) {}
> +void blue_led_off(void) __attribute__((weak, alias("__blue_led_off")));

Is this really the right file for this?

> +/*
> + * Why is gd allocated a register? Prior to reloc it might be better to
> + * just pass it around to each function in this file?

You're assuming that this is the only file that needs gd.

> +static int reserve_stacks(void)
> +{
> +	/* setup stack pointer for exceptions */
> +	gd->irq_sp = gd->dest_addr_sp;
> +#ifdef CONFIG_USE_IRQ
> +	gd->dest_addr_sp -= (CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ);
> +	debug("Reserving %zu Bytes for IRQ stack at: %08lx\n",
> +		CONFIG_STACKSIZE_IRQ + CONFIG_STACKSIZE_FIQ, gd->dest_addr_sp);
> +
> +	/* 8-byte alignment for ARM ABI compliance */
> +	gd->dest_addr_sp &= ~0x07;
> +#endif
> +	/* leave 3 words for abort-stack, plus 1 for alignment */
> +	gd->dest_addr_sp -= 16;
> +
> +	return 0;
> +}

What does "leave 3 words for abort-stack, plus 1 for alignment" mean in
a generic context?  Certainly we shouldn't have references to things
like FIQ or ARM ABI.

Do all architectures U-Boot supports have a stack that grows downward?

PowerPC requires 16-byte stack alignment, not 8-byte.

-Scott