[U-Boot] [PATCH v5 1/8] x86: Add new slimbootloader CPU type

[Park, Aiden]

Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
> Sent: Monday, July 22, 2019 8:14 AM
> To: Park, Aiden <aiden.park at intel.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Simon Glass
> <sjg at chromium.org>; Bin Meng <bmeng.cn at gmail.com>
> Subject: Re: [PATCH v5 1/8] x86: Add new slimbootloader CPU type
> 
> On Wed, Jul 17, 2019 at 7:41 AM Park, Aiden <aiden.park at intel.com> wrote:
> >
> > This slimbootloader cpu type is to enable U-Boot as a payload which
> 
> cpu -> CPU
> 
Let me change this.

> > runs on top of Slim Bootloader(https://github.com/slimbootloader).
> > The Slim Bootloader is designed with multi-stage architecture for the
> > execution from reset vector to OS booting, and supports qemu,
> 
> qemu -> QEMU
> 
Let me change this.

> > Apollolake, Whiskeylake and Coffeelake platforms consuming Intel FSP
> > (https://github.com/IntelFsp) for silicon initialization including CAR
> > and memory initialization.
> > The Slim Bootloader generates new HOB(Hand Off Block) which are serial
> > port info, memory map info, performance data info and so on, and
> > passes it to a Payload. U-Boot as a payload will use these HOB
> > information for basic initialization such as serial console.
> 
> > +config SYS_SLIMBOOTLOADER
> 
> > +       bool
> > +       default y
> 
> def_bool y ?
>
Thanks. Let me fix this and select SYS_SLIMBOOTLOADER in board Kconfig.
 
> > +       imply SYS_NS16550
> > +       imply AHCI_PCI
> > +       imply SCSI
> > +       imply SCSI_AHCI
> > +       imply MMC
> > +       imply MMC_PCI
> > +       imply MMC_SDHCI
> > +       imply MMC_SDHCI_SDMA
> > +       imply USB
> > +       imply USB_EHCI_HCD
> > +       imply USB_XHCI_HCD
> > +       imply USB_STORAGE
> > +       imply USB_KEYBOARD
> > +       imply E1000
> 
> > +       imply RTL8169
> 
> Is it part of SoC? I dunno we have Realtek inside, usually either Intel or Synopsys.
>
Yes on silicon Slim Bootloader supports, but not verified with this series.
I think removing this one would be better until this is really required.

Hi Bin, RTL8169 has been added as your recommended, but it hasn't been verified.
Is it okay to skip adding RTL8169 in this series? Let me add this later if it is really required.   

> > -#ifndef CONFIG_HAVE_FSP
> > +#if !defined(CONFIG_HAVE_FSP)
> && !defined(CONFIG_SYS_SLIMBOOTLOADER)
> 
> > -#ifdef CONFIG_HAVE_FSP
> > +#if defined(CONFIG_HAVE_FSP) || defined(CONFIG_SYS_SLIMBOOTLOADER)
> 
> Hmm... Maybe reasonable to have an additional option to tell something
> CONFIG_WE_HAVE_HOB_BUT_FSP.
>
Okay, let me add a new option - CONFIG_USE_HOB.
This will be selected by CONFIG_HAVE_FSP or CONFIG_SYS_SLIMBOOTLOADER.
 
> >         /* Store the HOB list if we have one */
> >         test    %esi, %esi
> >         jz      skip_hob
> >         movl    %esi, GD_HOB_LIST(%edx)
> >
> > +#ifdef CONFIG_HAVE_FSP
> 
> > +#endif
> 
> > +#ifndef __SLIMBOOTLOADER_ARCH_H__
> > +#define __SLIMBOOTLOADER_ARCH_H__
> > +
> > +#include <common.h>
> 
> Is it going to be expanded later?
> Otherwise I do not really see a point.
>
Yes, this is expanded in next patches. To avoid this confusion, let me remove this in 1st patch.

> > +#endif
> 
> > -#ifdef CONFIG_HAVE_FSP
> > +#if defined(CONFIG_HAVE_FSP) || defined(CONFIG_SYS_SLIMBOOTLOADER)
> 
> > -#ifdef CONFIG_HAVE_FSP
> > +#if defined(CONFIG_HAVE_FSP) || defined(CONFIG_SYS_SLIMBOOTLOADER)
> 
> Same as above.
> 
> --
> With Best Regards,
> Andy Shevchenko