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.