Re: [edk2-devel][edk2-platforms][PATCH V1 2/2] WhitleyOpenBoardPkg/PlatformInfo: Add board ID vendor range

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Isaac,

Please see feedback inline below.

Thanks,
Nate

> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Friday, January 21, 2022 5:22 PM
> To: devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>
> Subject: [edk2-devel][edk2-platforms][PATCH V1 2/2]
> WhitleyOpenBoardPkg/PlatformInfo: Add board ID vendor range
> 
> Add a vendor reserved range to avoid collisions with Intel reference board ID
> future use, if any.
> 
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Signed-off-by: Isaac Oram <isaac.w.oram@intel.com>
> ---
> 
> Platform/Intel/WhitleyOpenBoardPkg/Platform/Pei/PlatformInfo/PlatformI
> nfo.c |   3 +-
>  Platform/Intel/WhitleyOpenBoardPkg/Readme.md                                | 118
> ++++++++++++++++++++
>  Silicon/Intel/WhitleySiliconPkg/Include/PlatformInfoTypes.h                 |   8 +-
>  3 files changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Platform/Intel/WhitleyOpenBoardPkg/Platform/Pei/PlatformInfo/Platfor
> mInfo.c
> b/Platform/Intel/WhitleyOpenBoardPkg/Platform/Pei/PlatformInfo/Platfor
> mInfo.c
> index 87b4e57803..0065819d83 100644
> ---
> a/Platform/Intel/WhitleyOpenBoardPkg/Platform/Pei/PlatformInfo/Platfor
> mInfo.c
> +++
> b/Platform/Intel/WhitleyOpenBoardPkg/Platform/Pei/PlatformInfo/Platf
> +++ ormInfo.c
> @@ -413,7 +413,8 @@ PdrGetPlatformInfo (
>      return Status;
>    }
> 
> -  if ((PlatformInfoHob->BoardId >= TypePlatformMin) && (PlatformInfoHob-
> >BoardId <= TypePlatformMax)) {
> +  if ((PlatformInfoHob->BoardId >= TypePlatformMin) &&
> (PlatformInfoHob->BoardId <= TypePlatformMax) ||
> +      (PlatformInfoHob->BoardId >= TypePlatformVendorMin) &&
> + (PlatformInfoHob->BoardId <= TypePlatformVendorMax)) {
>      //
>      // Valid Platform Identified
>      //
> diff --git a/Platform/Intel/WhitleyOpenBoardPkg/Readme.md
> b/Platform/Intel/WhitleyOpenBoardPkg/Readme.md
> new file mode 100644
> index 0000000000..cc9f9a208f
> --- /dev/null
> +++ b/Platform/Intel/WhitleyOpenBoardPkg/Readme.md
> @@ -0,0 +1,118 @@
> +# **Board Porting for Intel&reg; Whitley Platform**
> +
> +## Overview
> +There are currently three board ports:
> +* WilsonCityRvp
> +* CooperCityRvp
> +* JunctionCity
> +
> +There are corresponding binaries in edk2-non-
> osi/WhitleyOpenBoardBinPkg.
> +
> +And there is a template for board porting, BoardPortWhitley.  See below for
> detailed instructions on creating a new board port.
> +
> +## BoardPortTemplate
> +This template profides basic instructions for how to customize the
> WhitleyOpenBoardPkg for a new system board.
> +
> +## Board Naming Convention
> +The current board directories are named arbitrarily and do not conform to
> code.

The above sentence has a somewhat antagonistic tone. Please modify the prose to be a little more impartial. Perhaps something like "The naming of board directories is unaffiliated with naming used in code."

> +Within code, the key is "TypeBoardName" which shows up in code in key
> ways:

Please remove the redundant "in code" from the above sentence. Perhaps something like "Within code, the key is "TypeBoardName" which shows up in the following principal forms:"

> +* EFI_PLATFORM_TYPE enum, e.g. TypeJunctionCity
> +* UBA Protocol, e.g. gEfiPlatformTypeJunctionCityProtocolGuid

You need two new lines here. Otherwise, the sentence beginning with " To facilitate future convention..." gets rendered as part of the "UBA Protocol" bullet point.

> +To facilitate future convention, "BoardPortTemplate" is used to increase
> consistency.
> +
> +## Board Porting Steps
> +It is desirable to pick a fairly unique name as WhitleyOpenBoardPkg UBA
> feature is designed to make it easy to support many boards in a single binary.
> +For the purposes of this example, "MyBoard" is the board name in code and
> filesystem.
> +

The numbing of the steps listed below is off. It goes 1, 2, 5

> +1. Copy WhitleyOpenBoardPkg/BoardPortTemplate to
> +WhitleyOpenBoardPkg/MyBoard 2. Rename
> +WhitleyOpenBoardPkg/MyBoard/Uba/TypeBoardPortTemplate to
> +WhitleyOpenBoardPkg/MyBoard/Uba/TypeMyBoard
> +5. Search and replace BoardPortTemplate with MyBoard in
> WhitleyOpenBoardPkg/MyBoard.  Do not search and replace at a higher
> scope as you will break the template examples.
> +6. Add a new EFI_PLATFORM_TYPE enum in edk2-
> platforms\Silicon\Intel\WhitleySiliconPkg\Include\PlatformInfoTypes.h, e.g.
> +```
> +TypeMyBoard, // 0x80
> +```
> +Please update the comment for TypeBoardPortTemplate to match the new
> maximum used, e.g.
> +```
> +TypeBoardPortTemplate               // 0x81
> +```
> +7. Update the PcdBoardId for your board in the
> WhitleyOpenBoardPkg/MyBoard/PlatformPkg.dsc, e.g.
> +```
> +gPlatformTokenSpaceGuid.PcdBoardId|0x80 # TypeMyBoard ``` 8. Update
> +each INF in WhitleyOpenBoardPkg/MyBoard/Uba with new GUID filename
> 9.
> +Add a DXE UBA protocol GUID to WhitleyOpenBoardPkg/PlatformPkg.dec,
> +*with a new GUID* ```
> +gEfiPlatformTypeMyBoardProtocolGuid       = { 0xa68228c5, 0xc00f, 0x4d9a, {
> 0x8d, 0xed, 0xb9, 0x6b, 0x9e, 0xef, 0xab, 0xca } }
> +```
> +10. Add your board to the switch statement in BoardInitDxeDriverEntry
> +(); in WhitleyOpenBoardPkg/Uba/BoardInit/Dxe/BoardInitDxe.c
> +```
> +   case TypeMyBoard:
> +      Status = gBS->InstallProtocolInterface (
> +        &Handle,
> +        &gEfiPlatformTypeMyBoardProtocolGuid,
> +        EFI_NATIVE_INTERFACE,
> +        NULL
> +        );
> +      ASSERT_EFI_ERROR (Status);
> +      break;
> +```
> +11. Add the gEfiPlatformTypeMyBoardProtocolGuid to the
> +WhitleyOpenBoardPkg/Uba/BoardInit/Dxe/BoardInitDxe.inf
> +12. Add a build option to edk2-platforms/Platform/Intel/build.cfg.  e.g.
> +```
> +MyBoard = WhitleyOpenBoardPkg/MyBoard/build_config.cfg
> +```
> +13. At this point, you can build from edk2-platforms/Platform/Intel, e.g.
> +```
> +build_bios.py -p MyBoard -t VS2015x86 -d ``` 14. At this point,
> +customization is not scripted.  The following are common customization
> areas:
> +MyBoard/Uba/TypeBoardPortTemplate/Pei
> +* GPIO
> +* VR, IMON
> +* SKU info
> +* Board layout, sockets, memory
> +* Soft straps, PCH, config, USB OC
> +* PCI, KTI, IO port bifurcation
> +MyBoard/Uba/TypeBoardPortTemplate/Dxe
> +* IIO config update
> +* Slot config update
> +* USB overcurrent update
> +
> +## Board Builds
> +
> +**Building with the python script**
> +
> +1. Open command window, go to the workspace directory, e.g.
> +c:\Edk2Workspace or ~/Edk2Workspace in the case of a linux OS 2. If using a
> linux OS
> +   * Type "cd edk2"
> +   * Type "source edksetup.sh"
> +   * Type "cd ../" to go back to the workspace directory 3. Type "cd
> +edk2-platforms/Platform/Intel 4. Type "python build_bios.py -p
> +TARGET_BOARD"
> +
> +* build_bios.py arguments:
> +
> +  | Argument              | Function                            |
> +  | ----------------------|-------------------------------------|
> +  | -h, --help            | show this help message and exit     |
> +  | --platform, -p        | the platform to build               |
> +  | --toolchain, -t       | tool Chain to use in build process  |
> +  | --DEBUG, -d           | debug flag                          |
> +  | --RELEASE, -r         | release flag                        |
> +  | --TEST_RELEASE, -tr   | test Release flag                   |
> +  | --RELEASE_PDB, -rp    | release flag                        |
> +  | --list, -l            | lists available platforms           |
> +  | --cleanall            | cleans all                          |
> +  | --clean               | cleans specified platform           |
> +  | --capsule             | capsule build enabled               |
> +  | --silent              | silent build enabled                |
> +  | --performance         | performance build enabled           |
> +  | --fsp                 | fsp wrapper build enabled           |
> +  | --fspapi              | API mode fsp wrapper build enabled  |
> +  | --hash                | Enable hash-based caching           |
> +  | --binary-destination  | create cache in specified directory |
> +  | --binary-source       | Consume cache from directory        |
> +  |                                                             |
> +
> +* For more information on build options
> +  * Type "python build_bios.py -h"
> diff --git a/Silicon/Intel/WhitleySiliconPkg/Include/PlatformInfoTypes.h
> b/Silicon/Intel/WhitleySiliconPkg/Include/PlatformInfoTypes.h
> index fae3101336..bfc6a49138 100644
> --- a/Silicon/Intel/WhitleySiliconPkg/Include/PlatformInfoTypes.h
> +++ b/Silicon/Intel/WhitleySiliconPkg/Include/PlatformInfoTypes.h
> @@ -65,13 +65,19 @@ typedef enum {
>    TypeBigPineKey,
>    TypeExperWorkStationRP,
>    TypeJunctionCity,
> -  EndOfEfiPlatformTypeEnum
> +  EndOfEfiPlatformTypeEnum,
> +  //
> +  // Vendor board range currently starts at 0x80  //
> +  TypeBoardPortTemplate               // 0x80
>  } EFI_PLATFORM_TYPE;
> 
>  #define TypePlatformUnknown       0xFF
>  #define TypePlatformMin           StartOfEfiPlatformTypeEnum + 1
>  #define TypePlatformMax           EndOfEfiPlatformTypeEnum - 1
>  #define TypePlatformDefault       TypeWilsonPointRP
> +#define TypePlatformVendorMin     0x80
> +#define TypePlatformVendorMax     TypeBoardPortTemplate - 1
> 
>  //
>  // CPU type: Standard (no MCP), -F, etc
> --
> 2.27.0.windows.1