| Message ID | 1360961665-10693-13-git-send-email-benoit.thebaudeau@advansee.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Poonam, Andy, On Friday, February 15, 2013 9:54:19 PM, Benoît Thébaudeau wrote: > PAD_TO is not a generic SPL configuration option, so use CONFIG_SPL_MAX_SIZE > instead. > > We want to use --pad-to with a size, but this option expects an address, so > use > u-boot-spl.bin instead of u-boot-spl as the input file in order to get > addresses > starting at 0. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > --- > Changes in v7: > - Use u-boot-spl.bin instead of u-boot-spl in order to avoid having to use > --change-addresses. > > Changes in v6: > - Fix size passed to --pad-to thanks to --change-addresses. > > Changes in v5: None > Changes in v4: > - New patch. > > Changes in v3: None > Changes in v2: None > > Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index a8c7b7b..317dffc 100644 > --- a/Makefile > +++ b/Makefile > @@ -486,7 +486,8 @@ $(obj)u-boot.dis: $(obj)u-boot > $(OBJDUMP) -d $< > $@ > > $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin > - $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(PAD_TO) -O binary $(obj)spl/u-boot-spl > $(obj)spl/u-boot-spl-pad.bin > + $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_MAX_SIZE) \ > + -I binary -O binary $< $(obj)spl/u-boot-spl-pad.bin > cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@ > rm $(obj)spl/u-boot-spl-pad.bin I would like to let you know what is going on, and to get your feedback for this patch. include/configs/p1_p2_rdb_pc.h seems to be the only current user of u-boot-with-spl.bin, triggered for example by the P2020RDB-PC_NAND config. Before this patch, PAD_TO was used, but there is no such definition for this board for generic SPL, so this board seems broken, all the more none of the various values defined for CONFIG_SYS_TEXT_BASE relatively to CONFIG_SPL_TEXT_BASE would be compatible with an image built by appending U-Boot to the generic SPL. Can you confirm? This patch won't fix this board, but I want to make sure that it won't be an issue for you now or later. Best regards, Benoît
On Sunday, February 17, 2013 5:16:49 PM, Benoît Thébaudeau wrote: > Hi Poonam, Andy, > > On Friday, February 15, 2013 9:54:19 PM, Benoît Thébaudeau wrote: > > PAD_TO is not a generic SPL configuration option, so use > > CONFIG_SPL_MAX_SIZE > > instead. > > > > We want to use --pad-to with a size, but this option expects an address, so > > use > > u-boot-spl.bin instead of u-boot-spl as the input file in order to get > > addresses > > starting at 0. > > > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > > --- > > Changes in v7: > > - Use u-boot-spl.bin instead of u-boot-spl in order to avoid having to use > > --change-addresses. > > > > Changes in v6: > > - Fix size passed to --pad-to thanks to --change-addresses. > > > > Changes in v5: None > > Changes in v4: > > - New patch. > > > > Changes in v3: None > > Changes in v2: None > > > > Makefile | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index a8c7b7b..317dffc 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -486,7 +486,8 @@ $(obj)u-boot.dis: $(obj)u-boot > > $(OBJDUMP) -d $< > $@ > > > > $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin > > - $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(PAD_TO) -O binary > > $(obj)spl/u-boot-spl > > $(obj)spl/u-boot-spl-pad.bin > > + $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_MAX_SIZE) \ > > + -I binary -O binary $< $(obj)spl/u-boot-spl-pad.bin > > cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@ > > rm $(obj)spl/u-boot-spl-pad.bin > > I would like to let you know what is going on, and to get your feedback for > this > patch. > > include/configs/p1_p2_rdb_pc.h seems to be the only current user of > u-boot-with-spl.bin, triggered for example by the P2020RDB-PC_NAND config. > > Before this patch, PAD_TO was used, but there is no such definition for this > board for generic SPL, so this board seems broken, all the more none of the > various values defined for CONFIG_SYS_TEXT_BASE relatively to > CONFIG_SPL_TEXT_BASE would be compatible with an image built by appending > U-Boot > to the generic SPL. Can you confirm? > > This patch won't fix this board, but I want to make sure that it won't be an > issue for you now or later. I'm also wondering why there is both generic SPL for NAND and legacy "NAND SPL" for p1_p2_rdb, all the more the "NAND SPL" version does not seem to be used in boards.cfg. Best regards, Benoît
On Sun, Feb 17, 2013 at 05:16:49PM +0100, Beno??t Th??baudeau wrote: > Hi Poonam, Andy, > > On Friday, February 15, 2013 9:54:19 PM, Beno??t Th??baudeau wrote: > > PAD_TO is not a generic SPL configuration option, so use CONFIG_SPL_MAX_SIZE > > instead. > > > > We want to use --pad-to with a size, but this option expects an address, so > > use > > u-boot-spl.bin instead of u-boot-spl as the input file in order to get > > addresses > > starting at 0. > > > > Signed-off-by: Beno??t Th??baudeau <benoit.thebaudeau@advansee.com> > > --- > > Changes in v7: > > - Use u-boot-spl.bin instead of u-boot-spl in order to avoid having to use > > --change-addresses. > > > > Changes in v6: > > - Fix size passed to --pad-to thanks to --change-addresses. > > > > Changes in v5: None > > Changes in v4: > > - New patch. > > > > Changes in v3: None > > Changes in v2: None > > > > Makefile | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index a8c7b7b..317dffc 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -486,7 +486,8 @@ $(obj)u-boot.dis: $(obj)u-boot > > $(OBJDUMP) -d $< > $@ > > > > $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin > > - $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(PAD_TO) -O binary $(obj)spl/u-boot-spl > > $(obj)spl/u-boot-spl-pad.bin > > + $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_MAX_SIZE) \ > > + -I binary -O binary $< $(obj)spl/u-boot-spl-pad.bin > > cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@ > > rm $(obj)spl/u-boot-spl-pad.bin > > I would like to let you know what is going on, and to get your feedback for this > patch. > > include/configs/p1_p2_rdb_pc.h seems to be the only current user of > u-boot-with-spl.bin, triggered for example by the P2020RDB-PC_NAND config. cam_enc_4xx also uses this target. Heiko? It looks like this change should be safe there as well. > Before this patch, PAD_TO was used, but there is no such definition for this > board for generic SPL, so this board seems broken, all the more none of the > various values defined for CONFIG_SYS_TEXT_BASE relatively to > CONFIG_SPL_TEXT_BASE would be compatible with an image built by appending U-Boot > to the generic SPL. Can you confirm? > > This patch won't fix this board, but I want to make sure that it won't be an > issue for you now or later.
On Monday, February 18, 2013 5:50:59 PM, Tom Rini wrote: > On Sun, Feb 17, 2013 at 05:16:49PM +0100, Beno??t Th??baudeau wrote: > > > Hi Poonam, Andy, > > > > On Friday, February 15, 2013 9:54:19 PM, Beno??t Th??baudeau wrote: > > > PAD_TO is not a generic SPL configuration option, so use > > > CONFIG_SPL_MAX_SIZE > > > instead. > > > > > > We want to use --pad-to with a size, but this option expects an address, > > > so > > > use > > > u-boot-spl.bin instead of u-boot-spl as the input file in order to get > > > addresses > > > starting at 0. > > > > > > Signed-off-by: Beno??t Th??baudeau <benoit.thebaudeau@advansee.com> > > > --- > > > Changes in v7: > > > - Use u-boot-spl.bin instead of u-boot-spl in order to avoid having to > > > use > > > --change-addresses. > > > > > > Changes in v6: > > > - Fix size passed to --pad-to thanks to --change-addresses. > > > > > > Changes in v5: None > > > Changes in v4: > > > - New patch. > > > > > > Changes in v3: None > > > Changes in v2: None > > > > > > Makefile | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/Makefile b/Makefile > > > index a8c7b7b..317dffc 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -486,7 +486,8 @@ $(obj)u-boot.dis: $(obj)u-boot > > > $(OBJDUMP) -d $< > $@ > > > > > > $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin > > > - $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(PAD_TO) -O binary > > > $(obj)spl/u-boot-spl > > > $(obj)spl/u-boot-spl-pad.bin > > > + $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_MAX_SIZE) \ > > > + -I binary -O binary $< $(obj)spl/u-boot-spl-pad.bin > > > cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@ > > > rm $(obj)spl/u-boot-spl-pad.bin > > > > I would like to let you know what is going on, and to get your feedback for > > this > > patch. > > > > include/configs/p1_p2_rdb_pc.h seems to be the only current user of > > u-boot-with-spl.bin, triggered for example by the P2020RDB-PC_NAND config. > > cam_enc_4xx also uses this target. Heiko? It looks like this change > should be safe there as well. And MPC8313ERDB too. But I've just seen that commit 74752ba did something for that in u-boot/master, and this commit is not in u-boot-imx/master on which I based this series. Why is u-boot-imx/master not sync'ed with u-boot/master? How am I supposed to handle patch sets depending on several custodian repositories? Commit 74752ba performs a '--pad-to=$(or $(CONFIG_SPL_PAD_TO),0)' on u-boot-spl. I could use this CONFIG_SPL_PAD_TO for this series too, but is it really necessary to have both CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE? In other words, is there any case for which CONFIG_SPL_PAD_TO could be different from CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE for a valid reason? > > Before this patch, PAD_TO was used, but there is no such definition for > > this > > board for generic SPL, so this board seems broken, all the more none of the > > various values defined for CONFIG_SYS_TEXT_BASE relatively to > > CONFIG_SPL_TEXT_BASE would be compatible with an image built by appending > > U-Boot > > to the generic SPL. Can you confirm? > > > > This patch won't fix this board, but I want to make sure that it won't be > > an > > issue for you now or later. Best regards, Benoît
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/18/2013 12:26 PM, Benoît Thébaudeau wrote: [snip] > But I've just seen that commit 74752ba did something for that in > u-boot/master, and this commit is not in u-boot-imx/master on which > I based this series. Why is u-boot-imx/master not sync'ed with > u-boot/master? How am I supposed to handle patch sets depending on > several custodian repositories? I'm not sure why u-boot-imx is out of sync at the moment. My rule of thumb is to start working on u-boot/master and cherry-pick out things I need from other places into a -test branch. I know the kernel folks have to deal with a lot of this as well, but I think it ends up being a developer choice on what best fits their needs when they need N different series to be applied for their starting point. > Commit 74752ba performs a '--pad-to=$(or $(CONFIG_SPL_PAD_TO),0)' > on u-boot-spl. I could use this CONFIG_SPL_PAD_TO for this series > too, but is it really necessary to have both CONFIG_SPL_PAD_TO and > CONFIG_SPL_MAX_SIZE? In other words, is there any case for which > CONFIG_SPL_PAD_TO could be different from CONFIG_SPL_TEXT_BASE + > CONFIG_SPL_MAX_SIZE for a valid reason? I was wondering along those lines. I don't _think_ we need both CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE, but we can't combine CONFIG_SPL_MAX_SIZE and CONFIG_SPL_TEXT_BASE as on TI platforms we start quite well above zero (0x402F0400 on am33xx, etc). That said, I guess we do need CONFIG_SPL_PAD_TO so that some platforms can do: #define CONFIG_SPL_PAD_TO (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE) and others just #define CONFIG_SPL_PAD_TO CONFIG_SPL_MAX_SIZE - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRImSWAAoJENk4IS6UOR1W0DkP/3VBLW0gdRoIfvNHpO3cI0Db FfjKGQoCgAmrlCE4PyGo2SkkBPl/doxLWYCOr8DJ/BDnjF04r/8NMT/wn/a9xrvv d5fCpIxQPme356SfMS23LVnTmJR4mOKLdHjJ2QXxyrFXXTnI7W80iPgfslgCTJje kxbH5chVQq7LYN9ZeynOP9XEZYT9rTcAXG9LVPqStWi73izpMa/D0adc/FqdWfi3 qcstiSxSQyPWmF7O7dRYzs9J4BMcT749NuQmkvbrh64/XL81emynfrYCgBU70QMr uQV9qxq+zALmUuMTdWRD0ATLQiybuN5Mbam7X4ACAmi8THfSEuUWtS3g0PR9+FH+ /HYICi8l2WDONRCWDcj5PLpQNYaNeunSPj+8q4g4i/OiEO+1rWZqtrgXxcH/WTlT dz6C6w/YFhfCakKwB8r2+5H3GyIpoDgbqsCfxI8LTNDA69/zXohbH6uDNMqWDPAV IS0+7Z3iC+MPmfkAXL2vz02CoiUiX2YwgYrhVrmPAbbsHSLndh2oAmGkdl8Hc2BK zhCKW75bv1flXwBIE3skocKZTDTjiMsMKoqosiFtSIaXQsn6Zz7YYh4ZpCNE0U3C P3GRpeotnXk0jRp6DOGKceWfVQLbY3rAZKWoMAjTujGIh3YfXRVU+KWLqi8APjNK a2aPpMFx6Oy72BQvV11W =zJru -----END PGP SIGNATURE-----
On 02/18/2013 06:28:15 AM, Benoît Thébaudeau wrote: > I'm also wondering why there is both generic SPL for NAND and legacy > "NAND SPL" > for p1_p2_rdb, all the more the "NAND SPL" version does not seem to > be used in > boards.cfg. "p1_p2_rdb_pc" and "P1_P2_RDB" are different targets (unfortunately). The former is for newer boards and has been converted to the new SPL. The latter is for older boards, which I do not have access to and have had a hard time getting information about (which would be required to merge the two targets). Perhaps P1_P2_RDB should just be removed. -Scott
On Monday, February 18, 2013 6:27:50 PM, Tom Rini wrote: > > Commit 74752ba performs a '--pad-to=$(or $(CONFIG_SPL_PAD_TO),0)' > > on u-boot-spl. I could use this CONFIG_SPL_PAD_TO for this series > > too, but is it really necessary to have both CONFIG_SPL_PAD_TO and > > CONFIG_SPL_MAX_SIZE? In other words, is there any case for which > > CONFIG_SPL_PAD_TO could be different from CONFIG_SPL_TEXT_BASE + > > CONFIG_SPL_MAX_SIZE for a valid reason? > > I was wondering along those lines. I don't _think_ we need both > CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE, but we can't combine > CONFIG_SPL_MAX_SIZE and CONFIG_SPL_TEXT_BASE as on TI platforms we > start quite well above zero (0x402F0400 on am33xx, etc). That said, I > guess we do need CONFIG_SPL_PAD_TO so that some platforms can do: > #define CONFIG_SPL_PAD_TO (CONFIG_SPL_TEXT_BASE + CONFIG_SPL_MAX_SIZE) > and others just > #define CONFIG_SPL_PAD_TO CONFIG_SPL_MAX_SIZE If we did like my patch here, i.e. use objcopy with u-boot-spl.bin instead of u-boot-spl, objcopy would always get a fake 0x0 address at the beginning of the .bin, so CONFIG_SPL_MAX_SIZE could be used with --pad-to, and CONFIG_SPL_PAD_TO would be useless. The only question is if we may need to have an empty gap between the SPL and U-Boot within the resulting image. I don't think so since that would mean that the target memory device has an area that is not really available at the location of this gap. Best regards, Benoît
On 02/18/2013 12:00:52 PM, Benoît Thébaudeau wrote: > On Monday, February 18, 2013 6:27:50 PM, Tom Rini wrote: > > > Commit 74752ba performs a '--pad-to=$(or $(CONFIG_SPL_PAD_TO),0)' > > > on u-boot-spl. I could use this CONFIG_SPL_PAD_TO for this series > > > too, but is it really necessary to have both CONFIG_SPL_PAD_TO and > > > CONFIG_SPL_MAX_SIZE? In other words, is there any case for which > > > CONFIG_SPL_PAD_TO could be different from CONFIG_SPL_TEXT_BASE + > > > CONFIG_SPL_MAX_SIZE for a valid reason? They're logically different things. > > I was wondering along those lines. I don't _think_ we need both > > CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE, but we can't combine > > CONFIG_SPL_MAX_SIZE and CONFIG_SPL_TEXT_BASE as on TI platforms we > > start quite well above zero (0x402F0400 on am33xx, etc). That > said, I > > guess we do need CONFIG_SPL_PAD_TO so that some platforms can do: > > #define CONFIG_SPL_PAD_TO (CONFIG_SPL_TEXT_BASE + > CONFIG_SPL_MAX_SIZE) > > and others just > > #define CONFIG_SPL_PAD_TO CONFIG_SPL_MAX_SIZE > > If we did like my patch here, i.e. use objcopy with u-boot-spl.bin > instead of > u-boot-spl, objcopy would always get a fake 0x0 address at the > beginning of the > .bin, so CONFIG_SPL_MAX_SIZE could be used with --pad-to, and > CONFIG_SPL_PAD_TO > would be useless. > > The only question is if we may need to have an empty gap between the > SPL and > U-Boot within the resulting image. I don't think so since that would > mean that > the target memory device has an area that is not really available at > the > location of this gap. Why not allow that possibility? Maybe it's easier for the SPL to load from a particular offset (e.g. NAND starting at the beginning of a block)? -Scott
On Monday, February 18, 2013 7:02:49 PM, Scott Wood wrote: > On 02/18/2013 12:00:52 PM, Benoît Thébaudeau wrote: > > On Monday, February 18, 2013 6:27:50 PM, Tom Rini wrote: > > > > Commit 74752ba performs a '--pad-to=$(or $(CONFIG_SPL_PAD_TO),0)' > > > > on u-boot-spl. I could use this CONFIG_SPL_PAD_TO for this series > > > > too, but is it really necessary to have both CONFIG_SPL_PAD_TO and > > > > CONFIG_SPL_MAX_SIZE? In other words, is there any case for which > > > > CONFIG_SPL_PAD_TO could be different from CONFIG_SPL_TEXT_BASE + > > > > CONFIG_SPL_MAX_SIZE for a valid reason? > > They're logically different things. > > > > I was wondering along those lines. I don't _think_ we need both > > > CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE, but we can't combine > > > CONFIG_SPL_MAX_SIZE and CONFIG_SPL_TEXT_BASE as on TI platforms we > > > start quite well above zero (0x402F0400 on am33xx, etc). That > > said, I > > > guess we do need CONFIG_SPL_PAD_TO so that some platforms can do: > > > #define CONFIG_SPL_PAD_TO (CONFIG_SPL_TEXT_BASE + > > CONFIG_SPL_MAX_SIZE) > > > and others just > > > #define CONFIG_SPL_PAD_TO CONFIG_SPL_MAX_SIZE > > > > If we did like my patch here, i.e. use objcopy with u-boot-spl.bin > > instead of > > u-boot-spl, objcopy would always get a fake 0x0 address at the > > beginning of the > > .bin, so CONFIG_SPL_MAX_SIZE could be used with --pad-to, and > > CONFIG_SPL_PAD_TO > > would be useless. > > > > The only question is if we may need to have an empty gap between the > > SPL and > > U-Boot within the resulting image. I don't think so since that would > > mean that > > the target memory device has an area that is not really available at > > the > > location of this gap. > > Why not allow that possibility? To save a config setting (there are already many for SPL) if this is not strictly required, but also for the reason below. > Maybe it's easier for the SPL to load > from a particular offset (e.g. NAND starting at the beginning of a > block)? CONFIG_SPL_MAX_SIZE would be closer to a NAND mapping in that case (e.g. size of 1 NAND Flash block) than CONFIG_SPL_PAD_TO (address within RAM that should be considered relatively to CONFIG_SPL_TEXT_BASE to get the NAND offset). Also, CONFIG_SPL_PAD_TO and CONFIG_SPL_MAX_SIZE depend on each other: If both can be defined, you may change one forgetting the other one, which could e.g. result in an overlapping of SPL and U-Boot that won't show up at build time (with CONFIG_SPL_MAX_SIZE = 0x1000 and CONFIG_SPL_PAD_TO = CONFIG_SPL_TEXT_BASE + 0x800, the SPL would build fine, and objcopy wouldn't complain). Best regards, Benoît
diff --git a/Makefile b/Makefile index a8c7b7b..317dffc 100644 --- a/Makefile +++ b/Makefile @@ -486,7 +486,8 @@ $(obj)u-boot.dis: $(obj)u-boot $(OBJDUMP) -d $< > $@ $(obj)u-boot-with-spl.bin: $(obj)spl/u-boot-spl.bin $(obj)u-boot.bin - $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(PAD_TO) -O binary $(obj)spl/u-boot-spl $(obj)spl/u-boot-spl-pad.bin + $(OBJCOPY) ${OBJCFLAGS} --pad-to=$(CONFIG_SPL_MAX_SIZE) \ + -I binary -O binary $< $(obj)spl/u-boot-spl-pad.bin cat $(obj)spl/u-boot-spl-pad.bin $(obj)u-boot.bin > $@ rm $(obj)spl/u-boot-spl-pad.bin
PAD_TO is not a generic SPL configuration option, so use CONFIG_SPL_MAX_SIZE instead. We want to use --pad-to with a size, but this option expects an address, so use u-boot-spl.bin instead of u-boot-spl as the input file in order to get addresses starting at 0. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> --- Changes in v7: - Use u-boot-spl.bin instead of u-boot-spl in order to avoid having to use --change-addresses. Changes in v6: - Fix size passed to --pad-to thanks to --change-addresses. Changes in v5: None Changes in v4: - New patch. Changes in v3: None Changes in v2: None Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)