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