[U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL
Scott Wood
scottwood at freescale.com
Sat May 11 02:45:11 CEST 2013
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
More information about the U-Boot
mailing list