On 05/10/2013 03:44:29 AM, Zhang Ying-B40530 wrote:
This patch needs to be broken into several patches that each take
care of one logical problem; it's too hard to properly review (and
have the right people pay attention to certain parts) in its current
state.
[Zhang Ying]
It only can be broken to two patches, one for SD boot, another for
SPI boot.
You can also separate out logical changes in support of your eventual
goal (as it looks like you already started doing with the
CONFIG_SPL_ENV_SUPPORT patch and such).
> @@ -83,5 +107,6 @@ SECTIONS
> *(.sbss*)
> *(.bss*)
> }
> + . = ALIGN(4);
> __bss_end = .;
> }
This seems unrelated.
[Zhang Ying]
It is necessary. In the function clear_bss(), the address of bss is
on the basis of the __bss_start, and then to four bytes of
incremental growth, finally the last stop is based on the address and
__bss_end is equal or not.
It may be necessary, but I don't see what it has to do with SD/SPI boot
other than by chance. Make this a separate patch with a changelog that
explains the problem.
> diff --git a/board/freescale/common/sdhc_boot.c
> b/board/freescale/common/sdhc_boot.c
> index e432318..96b0680 100644
> --- a/board/freescale/common/sdhc_boot.c
> +++ b/board/freescale/common/sdhc_boot.c
> + val = *(u16 *)(tmp_buf + ESDHC_BOOT_IMAGE_SIGN_ADDR);
> + if ((u16)ESDHC_BOOT_IMAGE_SIGN != val) {
> + free(tmp_buf);
> + return;
> + }
Why do you need this cast?
[Zhang Ying]
The offset 0x1FE of the config data sector should contain the value
0x55AA. If the value in this location doesn't match 0x55AA, it means
that the SD/MMC card doesn't contain a valid user code.
But why do you need to cast ESDHC_BOOT_IMAGE_SIGN to (u16)?
And the other cast probably violates C99 aliasing rules.
> + *
> + * You should have received a copy of the GNU General Public
License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef __SDHC_BOOT_H_
> +#define __SDHC_BOOT_H_ 1
> +
> +
> +int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr); void
> +mmc_get_env(void); void mmc_boot(void);
> +
> +#endif /* __SDHC_BOOT_H_ */
Does this stuff really belong in board/freescale? Should probably at
least be in arch/powerpc/cpu/mpc85xx, if not more generic.
[Zhang Ying]
Ok, whether we can handle like this: sdhc_boot.c and spi_boot.c will
be deleted. All this stuff in the sdhc_boot.c will be moved to
drivers/mmc/fsl_esdhc.c, and the functions in the spi_boot.c will be
moved to drivers/mmc/fsl_espi.c.
Maybe drivers/mmc/fsl_espi_spl.c and such?
> +void hang(void)
> +{
> + puts("### ERROR ### Please RESET the board ###\n");
> + for (;;)
> + ;
> +}
Whitespace
[Zhang Ying]
?
I'm not sure what I meant here. :-P
Maybe I meant to put this comment elsewhere, or trimmed the wrong
context...
> diff --git a/common/Makefile b/common/Makefile index
0e0fff1..bc80414
> 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -225,6 +225,11 @@ COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_common.o
> COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_flags.o
> COBJS-$(CONFIG_SPL_NET_SUPPORT) += env_nowhere.o
> COBJS-$(CONFIG_SPL_NET_SUPPORT) += miiphyutil.o
> +COBJS-$(CONFIG_SPL_HWCONFIG_SUPPORT) += hwconfig.o
> +COBJS-$(CONFIG_SPL_INIT_DDR_SUPPORT) += ddr_spd.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_attr.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_flags.o
> +COBJS-$(CONFIG_SPL_ENV_SUPPORT) += env_callback.o
CONFIG_SPL_ENV_SUPPORT should replace CONFIG_SPL_NET_SUPPORT here
(and add it to the boards that already have CONFIG_SPL_NET_SUPPORT).
This sort of refactoring needs to be a separate patch, BTW.
Can ddr_spd.o and hwconfig just use their normal CONFIG symbols (i.e.
move the existing makefile line out of the !SPL ifdef)? It's getting
a bit excessive with all the new SPL symbols.
[Zhang Ying]
If do it, the SPL's size will increase. I fear this will affect the
other SPL no CONFIG_SPL_NET_SUPPORT is defined.
Which SPL in particular are you concerned about, that uses
common/Makefile at all, and have CONFIG_SPD and/or CONFIG_HWCONFIG?
How much will they grow, after gc-sections (mainly all that's left is
anonymous strings)?
> +Where $file is the target file. Also keep in mind the u-boot.bin
> file needs
> +to be the u-boot built for your particular platform and target
media.
> +
> +Hint: To generate a u-boot.bin for a P1022DS booting from SD I
would
> run the
> +following in the u-boot repository:
> +
> + $ make P1022DS_SDCARD
s/Hint/Example/ and s/from SD I would run/from SD, run/
[Zhang Ying]
run bootsd? This is another independent requirement. If we want, we
can do it in another project.
Hmm? I was just fixing up the wording -- change "Hint" to "Example"
and change "from SD I would run" to "from SD, run".
That said, yes I would like to see "run bootsd". :-)
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
