| Message ID | 1440517253-18394-1-git-send-email-contact@paulk.fr |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Aug 25, 2015 at 05:40:53PM +0200, Paul Kocialkowski wrote: > The SYS_BOOT-based fallback shouldn't only check for one of the conditions of > use and then let the switch/case handle each boot device without enforcing the > conditions for each type of boot device again. > > For instance, this behaviour would trigger the fallback for UART when > BOOT_DEVICE_UART is defined, CONFIG_SPL_YMODEM_SUPPORT is enabled (which should > be a show-stopper) and e.g. BOOT_DEVICE_USB is enabled and not > CONFIG_SPL_USB_SUPPORT. > Separating the logic for USB and UART is a first step to solve this. > > In addition, a similar problematic behaviour took place when BOOT_DEVICE_USBETH, > BOOT_DEVICE_USB and CONFIG_SPL_USBETH_SUPPORT were enabled and not > CONFIG_SPL_USB_SUPPORT. > > Since BOOT_DEVICE_USBETH is only a problem when it's defined to the same value > as BOOT_DEVICE_USB, we can check that BOOT_DEVICE_USBETH and BOOT_DEVICE_USB are > different and if not, that CONFIG_SPL_USBETH_SUPPORT is not enabled to enable > the SYS_BOOT-based fallback mechanism. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> Reviewed-by: Tom Rini <trini@konsulko.com> But I'm going to wait until Hannes can report back with a Tested-by too, thanks!
Hi Paul, thanks for sending this fix. Basically i can now bring up my board with UART. Further i want to discuss the whole thing a bit, before we can finish. On 25.08.2015 17:40, Paul Kocialkowski wrote: > The SYS_BOOT-based fallback shouldn't only check for one of the conditions of > use and then let the switch/case handle each boot device without enforcing the > conditions for each type of boot device again. > > For instance, this behaviour would trigger the fallback for UART when > BOOT_DEVICE_UART is defined, CONFIG_SPL_YMODEM_SUPPORT is enabled (which should > be a show-stopper) and e.g. BOOT_DEVICE_USB is enabled and not > CONFIG_SPL_USB_SUPPORT. > Separating the logic for USB and UART is a first step to solve this. > > In addition, a similar problematic behaviour took place when BOOT_DEVICE_USBETH, > BOOT_DEVICE_USB and CONFIG_SPL_USBETH_SUPPORT were enabled and not > CONFIG_SPL_USB_SUPPORT. > > Since BOOT_DEVICE_USBETH is only a problem when it's defined to the same value > as BOOT_DEVICE_USB, we can check that BOOT_DEVICE_USBETH and BOOT_DEVICE_USB are > different and if not, that CONFIG_SPL_USBETH_SUPPORT is not enabled to enable > the SYS_BOOT-based fallback mechanism. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > --- > arch/arm/cpu/armv7/omap-common/boot-common.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/arch/arm/cpu/armv7/omap-common/boot-common.c b/arch/arm/cpu/armv7/omap-common/boot-common.c > index 5ec46fa..41f65c0 100644 > --- a/arch/arm/cpu/armv7/omap-common/boot-common.c > +++ b/arch/arm/cpu/armv7/omap-common/boot-common.c > @@ -30,6 +30,7 @@ void save_omap_boot_params(void) > { > u32 boot_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS); > struct omap_boot_parameters *omap_boot_params; > + int sys_boot_device = 0; the name of sys_boot_device variable is a bit confusing to me. It would be more readable if you name it for example "boot_device_invalid". > u32 boot_device; > u32 boot_mode; > > @@ -64,31 +65,31 @@ void save_omap_boot_params(void) > if (boot_device == BOOT_DEVICE_QSPI_4) > boot_device = BOOT_DEVICE_SPI; > #endif > -#if (defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT)) || \ > - (defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT)) || \ > - (defined(BOOT_DEVICE_USBETH) && !defined(CONFIG_SPL_USBETH_SUPPORT)) > /* > * When booting from peripheral booting, the boot device is not usable > * as-is (unless there is support for it), so the boot device is instead > * figured out using the SYS_BOOT pins. > */ > - switch (boot_device) { > -#ifdef BOOT_DEVICE_UART > - case BOOT_DEVICE_UART: > +#if defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT) > + if (boot_device == BOOT_DEVICE_UART) > + sys_boot_device = 1; > #endif A more readable approach could be: /* detect a inoperable bootdevice passed from ROM-code */ int boot_device_invalid = 0; switch (boot_device) { #if !defined(CONFIG_SPL_YMODEM_SUPPORT) && defined(BOOT_DEVICE_UART) case BOOT_DEVICE_UART: boot_device_invalid = 1; break; #endif #if !defined(CONFIG_SPL_USBETH_SUPPORT) && defined(BOOT_DEVICE_USBETH) case BOOT_DEVICE_USBETH: boot_device_invalid = 1; break; #endif #if !defined(CONFIG_SPL_USB_SUPPORT) && defined(BOOT_DEVICE_USB) case BOOT_DEVICE_USB: boot_device_invalid = 1; break; #endif } if (boot_device_invalid) boot_device = omap_sys_boot_device(); > -#ifdef BOOT_DEVICE_USB > - case BOOT_DEVICE_USB: > +#if defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT) && \ > + (!defined(BOOT_DEVICE_USBETH) || \ > + ((BOOT_DEVICE_USBETH != BOOT_DEVICE_USB) || \ > + !defined(CONFIG_SPL_USBETH_SUPPORT))) > + if (boot_device == BOOT_DEVICE_USB) > + sys_boot_device = 1; > #endif I don't see the need of testing "BOOT_DEVICE_USBETH != BOOT_DEVICE_USB", because they are always different defined in spl.h. BOOT_DEVICE_USBETH = 0x44 BOOT_DEVICE_USB = 0x45 maybe i'm missing something here. > + > + if (sys_boot_device) { > boot_device = omap_sys_boot_device(); would it be a good idea to pass the current boot_device to the fallback function omap_sys_boot_device. So the plattform fallback could figure out "the next best". best regards, Hannes
Le mercredi 26 août 2015 à 08:24 +0200, Hannes Schmelzer a écrit : > Hi Paul, > > thanks for sending this fix. > Basically i can now bring up my board with UART. > > Further i want to discuss the whole thing a bit, before we can finish. > > On 25.08.2015 17:40, Paul Kocialkowski wrote: > > The SYS_BOOT-based fallback shouldn't only check for one of the conditions of > > use and then let the switch/case handle each boot device without enforcing the > > conditions for each type of boot device again. > > > > For instance, this behaviour would trigger the fallback for UART when > > BOOT_DEVICE_UART is defined, CONFIG_SPL_YMODEM_SUPPORT is enabled (which should > > be a show-stopper) and e.g. BOOT_DEVICE_USB is enabled and not > > CONFIG_SPL_USB_SUPPORT. > > Separating the logic for USB and UART is a first step to solve this. > > > > In addition, a similar problematic behaviour took place when BOOT_DEVICE_USBETH, > > BOOT_DEVICE_USB and CONFIG_SPL_USBETH_SUPPORT were enabled and not > > CONFIG_SPL_USB_SUPPORT. > > > > Since BOOT_DEVICE_USBETH is only a problem when it's defined to the same value > > as BOOT_DEVICE_USB, we can check that BOOT_DEVICE_USBETH and BOOT_DEVICE_USB are > > different and if not, that CONFIG_SPL_USBETH_SUPPORT is not enabled to enable > > the SYS_BOOT-based fallback mechanism. > > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > > --- > > arch/arm/cpu/armv7/omap-common/boot-common.c | 23 ++++++++++++----------- > > 1 file changed, 12 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm/cpu/armv7/omap-common/boot-common.c b/arch/arm/cpu/armv7/omap-common/boot-common.c > > index 5ec46fa..41f65c0 100644 > > --- a/arch/arm/cpu/armv7/omap-common/boot-common.c > > +++ b/arch/arm/cpu/armv7/omap-common/boot-common.c > > @@ -30,6 +30,7 @@ void save_omap_boot_params(void) > > { > > u32 boot_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS); > > struct omap_boot_parameters *omap_boot_params; > > + int sys_boot_device = 0; > the name of sys_boot_device variable is a bit confusing to me. > It would be more readable if you name it for example "boot_device_invalid". sys_boot_device makes sense as in "take the boot device from sys_boot". Like salted_chips, to indicate that the chips are salted. The point is not to say that the boot device is "invalid", it's to say that we have no support for it and better try something else instead. > > u32 boot_device; > > u32 boot_mode; > > > > @@ -64,31 +65,31 @@ void save_omap_boot_params(void) > > if (boot_device == BOOT_DEVICE_QSPI_4) > > boot_device = BOOT_DEVICE_SPI; > > #endif > > -#if (defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT)) || \ > > - (defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT)) || \ > > - (defined(BOOT_DEVICE_USBETH) && !defined(CONFIG_SPL_USBETH_SUPPORT)) > > /* > > * When booting from peripheral booting, the boot device is not usable > > * as-is (unless there is support for it), so the boot device is instead > > * figured out using the SYS_BOOT pins. > > */ > > - switch (boot_device) { > > -#ifdef BOOT_DEVICE_UART > > - case BOOT_DEVICE_UART: > > +#if defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT) > > + if (boot_device == BOOT_DEVICE_UART) > > + sys_boot_device = 1; > > #endif > A more readable approach could be: > > /* detect a inoperable bootdevice passed from ROM-code */ > int boot_device_invalid = 0; > switch (boot_device) { > #if !defined(CONFIG_SPL_YMODEM_SUPPORT) && defined(BOOT_DEVICE_UART) > case BOOT_DEVICE_UART: > boot_device_invalid = 1; > break; > #endif > #if !defined(CONFIG_SPL_USBETH_SUPPORT) && defined(BOOT_DEVICE_USBETH) > case BOOT_DEVICE_USBETH: > boot_device_invalid = 1; > break; > #endif On all OMAP platforms, USB peripheral loading is done via bulk USB and I suspect it is the same on am33xx platforms. However, people did define that as USBETH to allow loading the next stage via USB ethernet, but it could just as well (and more rightfully) have been SPL_USB_SUPPORT. The idea is that the platform's spl.h could define both BOOT_DEVICE_USB and BOOT_DEVICE_USBETH to the same value and let each board define whether to go with USBETH or simple USB given what is enabled among CONFIG_SPL_USBETH_SUPPORT and CONFIG_SPL_USB_SUPPORT. That still sounds like a relevant approach to me, even though it wasn't implemented that way yet. Currently, am33xx either defines BOOT_DEVICE_USB or BOOT_DEVICE_USBETH (but not both at the same time). The point is, we have to be careful about the case where BOOT_DEVICE_USB and BOOT_DEVICE_USBETH are defined to the same value, because one could enable the fallback while the other one would still be valid. And in that case, the fallback would take over, which is not what we want. Since USBETH is never reported as such by the bootrom, I thought it would make more sense to have platforms that want the fallback active to define a proper BOOT_DEVICE_USB and have the boot-common code double-check that it can legitimately be enabled. It doesn't make such a big difference to also include USBETH as a proper boot device in that logic, provided that we keep making sure it won't override USB when both are defined to the same value (and vice-versa). On the other hand CPGMAC is indeed network loading, which is peripheral booting as well. If we agree that the fallback is designed to replace unsupported peripheral booting devices by the requested *memory* booting device from SYS_BOOT, it makes sense to add support for it, too. > #if !defined(CONFIG_SPL_USB_SUPPORT) && defined(BOOT_DEVICE_USB) > case BOOT_DEVICE_USB: > boot_device_invalid = 1; > break; > #endif > } > if (boot_device_invalid) > boot_device = omap_sys_boot_device(); > > -#ifdef BOOT_DEVICE_USB > > - case BOOT_DEVICE_USB: > > +#if defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT) && \ > > + (!defined(BOOT_DEVICE_USBETH) || \ > > + ((BOOT_DEVICE_USBETH != BOOT_DEVICE_USB) || \ > > + !defined(CONFIG_SPL_USBETH_SUPPORT))) > > + if (boot_device == BOOT_DEVICE_USB) > > + sys_boot_device = 1; > > #endif > I don't see the need of testing "BOOT_DEVICE_USBETH != BOOT_DEVICE_USB", > because they are always different defined in spl.h. > BOOT_DEVICE_USBETH = 0x44 > BOOT_DEVICE_USB = 0x45 > > maybe i'm missing something here. See above. > > + > > + if (sys_boot_device) { > > boot_device = omap_sys_boot_device(); > > would it be a good idea to pass the current boot_device to the fallback > function omap_sys_boot_device. > So the plattform fallback could figure out "the next best". No, the idea here is purely to read from the SYS_BOOT pins and take the first memory booting device, nothing more. It seems like the most reliable way to do things, because what's in the SYS_BOOT pins is supposed to be a working setup.
By the way, would you like to add am33xx support to the SYS_BOOT-based fallback interface? There is already get_sysboot_value on am33xx code (that is unused), I suggest that you use that function to write a proper omap_sys_boot_device in boot.c, as is done on omap platforms (and get rid of get_sysboot_value afterwards). Most of the work is to fill up a boot_devices array to match up each first *memory* booting devices based on the SYS_BOOT value, for each supported platform. Currently, am33xx supports: * AM33xx * AM43xx * TI81xx: TI816x (DM816x/AM389x), TI814x (DM814x/AM387x) That's 4 platforms and 4 TRMs to get the data from. The list of proper CONFIG defines to each for each platform can be found in arch/arm/includes/asm/arch-am33xx/spl.h Let me know if you're interested, I didn't do it at first because I have no device to test this with.
On Wed, Aug 26, 2015 at 12:51:07PM +0200, Paul Kocialkowski wrote: [snip] > On all OMAP platforms, USB peripheral loading is done via bulk USB and I > suspect it is the same on am33xx platforms. Actually no! :( On OMAP3/4/5 (so dra7xx/am57xx) it's the same bulk USB. On AM33xx/AM43xx it's (in ROM) USB RNDIS. That's why on am33xx/am43xx we continue on with USB RNDIS (which is what SPL_USBETH_SUPPORT implements, not the other USB "ethernet" class).
Le mercredi 26 août 2015 à 09:23 -0400, Tom Rini a écrit : > On Wed, Aug 26, 2015 at 12:51:07PM +0200, Paul Kocialkowski wrote: > > [snip] > > On all OMAP platforms, USB peripheral loading is done via bulk USB and I > > suspect it is the same on am33xx platforms. > > Actually no! :( > > On OMAP3/4/5 (so dra7xx/am57xx) it's the same bulk USB. On > AM33xx/AM43xx it's (in ROM) USB RNDIS. That's why on am33xx/am43xx we > continue on with USB RNDIS (which is what SPL_USBETH_SUPPORT implements, > not the other USB "ethernet" class). Interesting. So with this in mind, would it be acceptable to assume that BOOT_DEVICE_USB and BOOT_DEVICE_USBETH are always two distinct boot devices and thus not have to care about whether they're equal or not and just assume they're not, like any other BOOT_DEVICE? This would greatly simplify the logic and make everything more readable. I can submit a new patch set in that direction tomorrow.
On Thu, Aug 27, 2015 at 12:59:31AM +0200, Paul Kocialkowski wrote: > Le mercredi 26 août 2015 à 09:23 -0400, Tom Rini a écrit : > > On Wed, Aug 26, 2015 at 12:51:07PM +0200, Paul Kocialkowski wrote: > > > > [snip] > > > On all OMAP platforms, USB peripheral loading is done via bulk USB and I > > > suspect it is the same on am33xx platforms. > > > > Actually no! :( > > > > On OMAP3/4/5 (so dra7xx/am57xx) it's the same bulk USB. On > > AM33xx/AM43xx it's (in ROM) USB RNDIS. That's why on am33xx/am43xx we > > continue on with USB RNDIS (which is what SPL_USBETH_SUPPORT implements, > > not the other USB "ethernet" class). > > Interesting. So with this in mind, would it be acceptable to assume that > BOOT_DEVICE_USB and BOOT_DEVICE_USBETH are always two distinct boot > devices and thus not have to care about whether they're equal or not and > just assume they're not, like any other BOOT_DEVICE? > > This would greatly simplify the logic and make everything more readable. > > I can submit a new patch set in that direction tomorrow. Yes, lets go with that.
On 2015-08-27 00:59, Paul Kocialkowski wrote: > Le mercredi 26 août 2015 à 09:23 -0400, Tom Rini a écrit : >> On Wed, Aug 26, 2015 at 12:51:07PM +0200, Paul Kocialkowski wrote: >> >> [snip] >>> On all OMAP platforms, USB peripheral loading is done via bulk USB and I >>> suspect it is the same on am33xx platforms. >> Actually no! :( >> >> On OMAP3/4/5 (so dra7xx/am57xx) it's the same bulk USB. On >> AM33xx/AM43xx it's (in ROM) USB RNDIS. That's why on am33xx/am43xx we >> continue on with USB RNDIS (which is what SPL_USBETH_SUPPORT implements, >> not the other USB "ethernet" class). > Interesting. So with this in mind, would it be acceptable to assume that > BOOT_DEVICE_USB and BOOT_DEVICE_USBETH are always two distinct boot > devices and thus not have to care about whether they're equal or not and > just assume they're not, like any other BOOT_DEVICE? > > This would greatly simplify the logic and make everything more readable. > > I can submit a new patch set in that direction tomorrow. Hi Paul, agree with that, looking forward your new patch. best regards, Hannes
diff --git a/arch/arm/cpu/armv7/omap-common/boot-common.c b/arch/arm/cpu/armv7/omap-common/boot-common.c index 5ec46fa..41f65c0 100644 --- a/arch/arm/cpu/armv7/omap-common/boot-common.c +++ b/arch/arm/cpu/armv7/omap-common/boot-common.c @@ -30,6 +30,7 @@ void save_omap_boot_params(void) { u32 boot_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS); struct omap_boot_parameters *omap_boot_params; + int sys_boot_device = 0; u32 boot_device; u32 boot_mode; @@ -64,31 +65,31 @@ void save_omap_boot_params(void) if (boot_device == BOOT_DEVICE_QSPI_4) boot_device = BOOT_DEVICE_SPI; #endif -#if (defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT)) || \ - (defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT)) || \ - (defined(BOOT_DEVICE_USBETH) && !defined(CONFIG_SPL_USBETH_SUPPORT)) /* * When booting from peripheral booting, the boot device is not usable * as-is (unless there is support for it), so the boot device is instead * figured out using the SYS_BOOT pins. */ - switch (boot_device) { -#ifdef BOOT_DEVICE_UART - case BOOT_DEVICE_UART: +#if defined(BOOT_DEVICE_UART) && !defined(CONFIG_SPL_YMODEM_SUPPORT) + if (boot_device == BOOT_DEVICE_UART) + sys_boot_device = 1; #endif -#ifdef BOOT_DEVICE_USB - case BOOT_DEVICE_USB: +#if defined(BOOT_DEVICE_USB) && !defined(CONFIG_SPL_USB_SUPPORT) && \ + (!defined(BOOT_DEVICE_USBETH) || \ + ((BOOT_DEVICE_USBETH != BOOT_DEVICE_USB) || \ + !defined(CONFIG_SPL_USBETH_SUPPORT))) + if (boot_device == BOOT_DEVICE_USB) + sys_boot_device = 1; #endif + + if (sys_boot_device) { boot_device = omap_sys_boot_device(); /* MMC raw mode will fallback to FS mode. */ if ((boot_device >= MMC_BOOT_DEVICES_START) && (boot_device <= MMC_BOOT_DEVICES_END)) boot_mode = MMCSD_MODE_RAW; - - break; } -#endif gd->arch.omap_boot_device = boot_device;
The SYS_BOOT-based fallback shouldn't only check for one of the conditions of use and then let the switch/case handle each boot device without enforcing the conditions for each type of boot device again. For instance, this behaviour would trigger the fallback for UART when BOOT_DEVICE_UART is defined, CONFIG_SPL_YMODEM_SUPPORT is enabled (which should be a show-stopper) and e.g. BOOT_DEVICE_USB is enabled and not CONFIG_SPL_USB_SUPPORT. Separating the logic for USB and UART is a first step to solve this. In addition, a similar problematic behaviour took place when BOOT_DEVICE_USBETH, BOOT_DEVICE_USB and CONFIG_SPL_USBETH_SUPPORT were enabled and not CONFIG_SPL_USB_SUPPORT. Since BOOT_DEVICE_USBETH is only a problem when it's defined to the same value as BOOT_DEVICE_USB, we can check that BOOT_DEVICE_USBETH and BOOT_DEVICE_USB are different and if not, that CONFIG_SPL_USBETH_SUPPORT is not enabled to enable the SYS_BOOT-based fallback mechanism. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> --- arch/arm/cpu/armv7/omap-common/boot-common.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)