From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Fri, 10 May 2013 19:45:11 -0500 Subject: [U-Boot] [PATCH] powerpc/p1022ds: boot from SD card/SPI flash with SPL In-Reply-To: (from B40530@freescale.com on Fri May 10 03:44:29 2013) Message-ID: <1368233111.19683.20@snotra> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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