| Message ID | 1441289520-22749-3-git-send-email-bernhard.nortmann@web.de |
|---|---|
| State | Superseded |
| Delegated to: | Hans de Goede |
| Headers | show |
Hi, On 03-09-15 16:12, Bernhard Nortmann wrote: > This patch makes use of the previous changes to add a new "fel" boot > target for sunxi boards. > > When booting via FEL, it's often desirable to work around the absence > of other (usable) boot devices - or to be able to override them, > deviating from the standard boot sequence. To achieve this, the "fel" > boot target gets the highest priority, but won't actually do anything > unless certain criteria are met. > > The "bootcmd_fel" implementation proposed here first tests if an actual > FEL boot takes place (using the "fel_booted" env var), and secondly > checks that "fel_data_addr" was set (originating from the 'loader', > i.e. the sunxi-tools fel utility). If both checks pass, then it will > try to execute a boot script (boot.scr) at the given address. In case > of an error (e.g. an invalid image), the source command might return > "false", causing "distro_bootcmd" to proceed with the next boot target. > > Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> > > --- > > include/configs/sunxi-common.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h > index 48cc4ed..750355d 100644 > --- a/include/configs/sunxi-common.h > +++ b/include/configs/sunxi-common.h > @@ -423,7 +423,20 @@ extern int soft_i2c_gpio_scl; > #define BOOT_TARGET_DEVICES_USB(func) > #endif > > +/* FEL boot support, auto-execute boot.scr if a script address was provided */ > +#define BOOTENV_DEV_FEL(devtypeu, devtypel, instance) \ > + "bootcmd_fel=" \ > + "if test -n ${fel_booted}; then " \ > + "echo '(FEL boot)';" \ > + "if test -n ${fel_data_addr}; then " \ > + "source ${fel_data_addr}; " \ > + "fi; " \ > + "fi\0" I would prefer to have this like this: "bootcmd_fel=" \ "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \ "echo '(FEL boot)';" \ "source ${fel_data_addr}; " \ "fi\0" Also if we are not using fel_data_size, then why do we even have it ? Regards, Hans > +#define BOOTENV_DEV_NAME_FEL(devtypeu, devtypel, instance) \ > + "fel " > + > #define BOOT_TARGET_DEVICES(func) \ > + func(FEL, fel, na) \ > BOOT_TARGET_DEVICES_MMC(func) \ > BOOT_TARGET_DEVICES_SCSI(func) \ > BOOT_TARGET_DEVICES_USB(func) \ >
Hi! Am 10.09.2015 um 20:36 schrieb Hans de Goede: > Hi, > > I would prefer to have this like this: > > "bootcmd_fel=" \ > "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \ > "echo '(FEL boot)';" \ > "source ${fel_data_addr}; " \ > "fi\0" > Sure, we could do that. I wanted to make clear that ${fel_booted} is independent of a script being present (and thus ${fel_data_addr} set). If the user feels inclined to do so, he might e.g. tweak bootcmd_fel to override some defaults even with no boot.scr involved. > Also if we are not using fel_data_size, then why do we even > have it ? > I thought it unnecessary to restrict ourselves to not being able to pass the size information, and kept it optional deliberately. Admittedly it's pointless in the "standard" case of boot.scr, as that is expected to be an image with a well-defined header (including data size). I could imagine other uses, e.g. a customized fel utility passing uEnv.txt-style data, and integrating that via bootcmd_fel "import -t ${fel_data_addr} ${fel_data_size}". Personally I like to do this when testing; I find it easier to simply edit a text file (without having to go through a mkimage .scr on each cycle). Regards, B. Nortmann
Hi, On 11-09-15 11:31, Bernhard Nortmann wrote: > Hi! > > Am 10.09.2015 um 20:36 schrieb Hans de Goede: >> Hi, >> >> I would prefer to have this like this: >> >> "bootcmd_fel=" \ >> "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \ >> "echo '(FEL boot)';" \ >> "source ${fel_data_addr}; " \ >> "fi\0" >> > > Sure, we could do that. I wanted to make clear that ${fel_booted} is > independent of a script being present (and thus ${fel_data_addr} set). > If the user feels inclined to do so, he might e.g. tweak bootcmd_fel > to override some defaults even with no boot.scr involved. I think that an user advanced enough to do this can figure this out anyways and the above is cleaner, so please switch to the above construct for the next version. >> Also if we are not using fel_data_size, then why do we even >> have it ? >> > > I thought it unnecessary to restrict ourselves to not being able to > pass the size information, and kept it optional deliberately. Right, but by doing so you are taking our last reserved uint32, making it unavailable for future use, I'm not sure if that is a good idea. > Admittedly it's pointless in the "standard" case of boot.scr, as that > is expected to be an image with a well-defined header (including data > size). I could imagine other uses, e.g. a customized fel utility > passing uEnv.txt-style data, and integrating that via bootcmd_fel > "import -t ${fel_data_addr} ${fel_data_size}". Personally I like to > do this when testing; I find it easier to simply edit a text file > (without having to go through a mkimage .scr on each cycle). But your sunxi-tools patches depends on the header to detect that a boot.scr is being loaded and set ${fel_data_addr} and ${fel_data_size} in the spl header, so this requires hacking up things in both u-boot and sunxi-tools. Given that this is already a pretty exotic use-case I believe that the user having to have mkimage available (the calling of it can be scripted away) is not a big deal. I would rather see an as clean as possible solution focussing on just the boot.scr use-case, as said that is exotic enough already to not complicate things further. Regards, Hans
On Sat, 2015-09-12 at 14:48 +0200, Hans de Goede wrote: > I believe that the user having to have mkimage available (the calling > of it can be scripted away) is not a big deal. The fel tool could even call it automatically, couldn't it?
On Fri, 11 Sep 2015 11:31:50 +0200 Bernhard Nortmann <bernhard.nortmann@web.de> wrote: > Hi! > > Am 10.09.2015 um 20:36 schrieb Hans de Goede: > > Hi, > > > > I would prefer to have this like this: > > > > "bootcmd_fel=" \ > > "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \ > > "echo '(FEL boot)';" \ > > "source ${fel_data_addr}; " \ > > "fi\0" > > > > Sure, we could do that. I wanted to make clear that ${fel_booted} is > independent of a script being present (and thus ${fel_data_addr} set). > If the user feels inclined to do so, he might e.g. tweak bootcmd_fel > to override some defaults even with no boot.scr involved. > > > Also if we are not using fel_data_size, then why do we even > > have it ? > > > > I thought it unnecessary to restrict ourselves to not being able to > pass the size information, and kept it optional deliberately. > > Admittedly it's pointless in the "standard" case of boot.scr, as that > is expected to be an image with a well-defined header (including data > size). I could imagine other uses, e.g. a customized fel utility > passing uEnv.txt-style data, and integrating that via bootcmd_fel > "import -t ${fel_data_addr} ${fel_data_size}". We could probably have this data represented in the following way in the SPL header: union { struct { uint32_t fel_boot_script_address; uint32_t must_be_zero; }; struct { uint32_t fel_uenv_txt_address; uint32_t fel_uenv_txt_size; }; }; So that if 'fel_uenv_txt_size' is non-zero, then we can do "import -t ${fel_uenv_txt_address} ${fel_uenv_txt_size}". And if it is zero, then treat this pointer as boot.scr data. And we don't even need to use the union if we don't want to. This all data could be also stored in a straightforward way (at the expense of using additional 4 bytes in the SPL header): uint32_t fel_boot_script_address; uint32_t fel_uenv_txt_address; uint32_t fel_uenv_txt_size; And because it takes more storage space, we would need to increase the SPL header size. But this can be easily done. > Personally I like to do this when testing; I find it easier to > simply edit a text file (without having to go through a mkimage > .scr on each cycle). Supporting both boot.scr and uEnv.txt for FEL boot seems to be reasonably simple to me. You can even do it in a single patch series. As Hans suggests, please take care of the boot.scr case first. Then maybe introduce uEnv.txt support with an additional patch.
Hi, On 14-09-15 12:33, Siarhei Siamashka wrote: > On Fri, 11 Sep 2015 11:31:50 +0200 > Bernhard Nortmann <bernhard.nortmann@web.de> wrote: > >> Hi! >> >> Am 10.09.2015 um 20:36 schrieb Hans de Goede: >>> Hi, >>> >>> I would prefer to have this like this: >>> >>> "bootcmd_fel=" \ >>> "if test -n ${fel_booted} && test -n ${fel_data_addr}; then " \ >>> "echo '(FEL boot)';" \ >>> "source ${fel_data_addr}; " \ >>> "fi\0" >>> >> >> Sure, we could do that. I wanted to make clear that ${fel_booted} is >> independent of a script being present (and thus ${fel_data_addr} set). >> If the user feels inclined to do so, he might e.g. tweak bootcmd_fel >> to override some defaults even with no boot.scr involved. >> >>> Also if we are not using fel_data_size, then why do we even >>> have it ? >>> >> >> I thought it unnecessary to restrict ourselves to not being able to >> pass the size information, and kept it optional deliberately. >> >> Admittedly it's pointless in the "standard" case of boot.scr, as that >> is expected to be an image with a well-defined header (including data >> size). I could imagine other uses, e.g. a customized fel utility >> passing uEnv.txt-style data, and integrating that via bootcmd_fel >> "import -t ${fel_data_addr} ${fel_data_size}". > > We could probably have this data represented in the following way > in the SPL header: > > union { > struct { > uint32_t fel_boot_script_address; > uint32_t must_be_zero; > }; > struct { > uint32_t fel_uenv_txt_address; > uint32_t fel_uenv_txt_size; > }; > }; > > So that if 'fel_uenv_txt_size' is non-zero, then we can do > "import -t ${fel_uenv_txt_address} ${fel_uenv_txt_size}". > And if it is zero, then treat this pointer as boot.scr data. > > And we don't even need to use the union if we don't want to. This all > data could be also stored in a straightforward way (at the expense > of using additional 4 bytes in the SPL header): > > uint32_t fel_boot_script_address; > uint32_t fel_uenv_txt_address; > uint32_t fel_uenv_txt_size; > > And because it takes more storage space, we would need to increase > the SPL header size. But this can be easily done. > >> Personally I like to do this when testing; I find it easier to >> simply edit a text file (without having to go through a mkimage >> .scr on each cycle). > > Supporting both boot.scr and uEnv.txt for FEL boot seems to be > reasonably simple to me. You can even do it in a single patch > series. As Hans suggests, please take care of the boot.scr case > first. Then maybe introduce uEnv.txt support with an additional > patch. I'm still not convinced that support uEnv.txt is necessary at all, but I agree that if we this having this done through extra fields, rather then through a union would be better. Regards, Hans
Hi, On 14-09-15 13:42, Hans de Goede wrote: >> Supporting both boot.scr and uEnv.txt for FEL boot seems to be >> reasonably simple to me. You can even do it in a single patch >> series. As Hans suggests, please take care of the boot.scr case >> first. Then maybe introduce uEnv.txt support with an additional >> patch. > > I'm still not convinced that support uEnv.txt is necessary at all, > but I agree that if we this having this done through extra fields, > rather then through a union would be better. p.s. As for the version byte vs minor/major scheme discussion. Why not simply declare that we will always be backwards compatible, and only ever add new fields ? Then we can simply add version checks around those new fields, and have both back and forward compatibility. In this case the single byte should suffice nicely. Regards, Hans
On Mon, 14 Sep 2015 13:46:57 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 14-09-15 13:42, Hans de Goede wrote: > > >> Supporting both boot.scr and uEnv.txt for FEL boot seems to be > >> reasonably simple to me. You can even do it in a single patch > >> series. As Hans suggests, please take care of the boot.scr case > >> first. Then maybe introduce uEnv.txt support with an additional > >> patch. > > > > I'm still not convinced that support uEnv.txt is necessary at all, > > but I agree that if we this having this done through extra fields, > > rather then through a union would be better. > > p.s. > > As for the version byte vs minor/major scheme discussion. Why not > simply declare that we will always be backwards compatible, and > only ever add new fields ? Then we can simply add version checks > around those new fields, and have both back and forward compatibility. > > In this case the single byte should suffice nicely. Regarding "Why not simply declare that we will always be backwards compatible". Certain things are not always in our perfect control. It's better not to make unnecessary promises, otherwise they can become a liability :-) For example, let's suppose that we have added data fields for both "uEnv.txt" and "boot.scr" in the SPL header. Then suppose that later U-Boot maintainers decide to deprecate and drop support of one of these methods in favour of the other. This situation can be handled by doing the major version increase and removing the obsolete field from the SPL header. The "fel" tool would notice that the U-Boot binary is incompatible and complain, providing the user with all the necessary hints. But if we don't have the concept of major/minor versions, then the "fel" tool would be just assuming full backwards compatibility and happily trying to pass the obsolete data to U-Boot. Yes, we can also always change the SPL header signature in the case of introducing incompatible changes. But the down side is that the "fel" tool would treat this situation as "unknown bootloader" instead of "incompatible U-Boot". Anyway, nothing really needs to be changed on the U-Boot side at this moment and the Bernhard's patches don't need any modifications. It's up to the "fel" tool how to interpret the version byte. For example, we can arbitrarily decide that the top 4 bits are used for the major version part and low 4 bits are used for the minor version part. If only the minor version number is changed, then assume backwards compatibility. If the major version number has changed, then complain to the user.
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 48cc4ed..750355d 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -423,7 +423,20 @@ extern int soft_i2c_gpio_scl; #define BOOT_TARGET_DEVICES_USB(func) #endif +/* FEL boot support, auto-execute boot.scr if a script address was provided */ +#define BOOTENV_DEV_FEL(devtypeu, devtypel, instance) \ + "bootcmd_fel=" \ + "if test -n ${fel_booted}; then " \ + "echo '(FEL boot)';" \ + "if test -n ${fel_data_addr}; then " \ + "source ${fel_data_addr}; " \ + "fi; " \ + "fi\0" +#define BOOTENV_DEV_NAME_FEL(devtypeu, devtypel, instance) \ + "fel " + #define BOOT_TARGET_DEVICES(func) \ + func(FEL, fel, na) \ BOOT_TARGET_DEVICES_MMC(func) \ BOOT_TARGET_DEVICES_SCSI(func) \ BOOT_TARGET_DEVICES_USB(func) \
This patch makes use of the previous changes to add a new "fel" boot target for sunxi boards. When booting via FEL, it's often desirable to work around the absence of other (usable) boot devices - or to be able to override them, deviating from the standard boot sequence. To achieve this, the "fel" boot target gets the highest priority, but won't actually do anything unless certain criteria are met. The "bootcmd_fel" implementation proposed here first tests if an actual FEL boot takes place (using the "fel_booted" env var), and secondly checks that "fel_data_addr" was set (originating from the 'loader', i.e. the sunxi-tools fel utility). If both checks pass, then it will try to execute a boot script (boot.scr) at the given address. In case of an error (e.g. an invalid image), the source command might return "false", causing "distro_bootcmd" to proceed with the next boot target. Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> --- include/configs/sunxi-common.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)