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

[Paul Kocialkowski]

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 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".

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.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150826/f4718bd9/attachment.sig>