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;
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;
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