[U-Boot] [PATCH] omap-common: SYS_BOOT fallback logic correction

[Hannes Schmelzer <hannes@schmelzer.or.at>]

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