Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add Hummingboard SmBios

[Chris Co <Christopher.Co@microsoft.com>]

Hi Leif,

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Tuesday, August 7, 2018 5:50 AM
> To: Chris Co <Christopher.Co@microsoft.com>
> Cc: edk2-devel@lists.01.org; Michael D Kinney
> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add
> Hummingboard SmBios
> 
> I'll start trickling through some generic comments on the code, apart from
> the general ones I made before.
> 

Thank you for these comments and continuing with the review.  I do sincerely apologize about all the code style issues.  I should have been more rigorous about enforcing code style before sending it up (It was a lot worse before :-( )
I am working on generating the new patch set.  Currently about halfway though.  I will incorporate these comments into it.  Should have it ready by early next week.  That said, if you do have more feedback on other patches, I would be glad to incorporate them in as soon as they are available.

> On Sun, Jul 22, 2018 at 01:30:34AM +0000, Chris Co wrote:
> > This adds SMBIOS support for SolidRun's i.MX6Q Hummingboard Edge.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >
> > +  {       // BIOSCharacteristicsExtensionBytes[]
> > +    0x81, //  AcpiIsSupported                   :1;
> > +          //  UsbLegacyIsSupported              :1;
> > +          //  AgpIsSupported                    :1;
> > +          //  I2OBootIsSupported                :1;
> > +          //  Ls120BootIsSupported              :1;
> > +          //  AtapiZipDriveBootIsSupported      :1;
> > +          //  Boot1394IsSupported               :1;
> > +          //  SmartBatteryIsSupported           :1;
> > +    0x0e, //  BiosBootSpecIsSupported           :1;
> > +          //  FunctionKeyNetworkBootIsSupported :1;
> > +          //  TargetContentDistributionEnabled  :1;
> > +          //  UefiSpecificationSupported        :1;
> > +          //  VirtualMachineSupported           :1;
> > +          //  ExtensionByte2Reserved            :3;
> > +  },
> > +  0x00,                    // SystemBiosMajorRelease
> > +  0x01,                    // SystemBiosMinorRelease
> > +  0xFF,                    // EmbeddedControllerFirmwareMajorRelease
> > +  0xFF,                    // EmbeddedControllerFirmwareMinorRelease
> > +};
> > +
> > +CHAR8 *mBIOSInfoType0Strings[] = {
> > +  "To be filled by O.E.M.", // Vendor String
> > +  "0.1",                    // BiosVersion String
> > +  __DATE__,                 // BiosReleaseDate String
> > +  NULL
> > +};
> 
> Could we actually fill this in instead?
> Vendor String is arguably TianoCore.
> We could do something similar to the Hisilicon platforms and use
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor and
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString,
> then tweaked in the platform .dsc.
> 

I will make this change and use PCDs.  Much cleaner with PCDs.

> > +
> > +SMBIOS_TABLE_TYPE1 mSysInfoType1 = {
> > +  { EFI_SMBIOS_TYPE_SYSTEM_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE1), 0 },
> > +  1,    // Manufacturer String
> > +  2,    // ProductName String
> > +  3,    // Version String
> > +  4,    // SerialNumber String
> > +  { 0x25EF0280, 0xEC82, 0x42B0, { 0x8F, 0xB6, 0x10, 0xAD, 0xCC, 0xC6,
> > +0x7C, 0x02 } },
> > +  SystemWakeupTypePowerSwitch,
> > +  5,    // SKUNumber String
> > +  6,    // Family String
> > +};
> > +CHAR8  *mSysInfoType1Strings[] = {
> > +  "SolidRun",
> > +  "HummingBoard-Edge i4Pro",
> > +  "2.0",
> > +  "System Serial#",
> > +  "hb04w-e-110-00-004-0",
> 
> Do we have any way of obtaining the three above programmatically?
> 

This is the default template for Sysinfo, which gets overwritten at runtime.  Given that we are overwriting this table at runtime, I'm thinking it would be better to just generate the table runtime and avoid confusion by having this template.

> > +  "edk2",
> 
> Family as 'edk2'?
> 

Definitely should not be edk2.  Will fix this.

> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE2  mBoardInfoType2 = {
> > +  { EFI_SMBIOS_TYPE_BASEBOARD_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE2), 0 },
> > +  1,    // Manufacturer String
> > +  2,    // ProductName String
> > +  3,    // Version String
> > +  4,    // SerialNumber String
> > +  5,    // AssetTag String
> > +  {     // FeatureFlag
> > +    1,    //  Motherboard           :1;
> > +    0,    //  RequiresDaughterCard  :1;
> > +    0,    //  Removable             :1;
> > +    0,    //  Replaceable           :1;
> > +    0,    //  HotSwappable          :1;
> > +    0,    //  Reserved              :3;
> > +  },
> > +  6,                        // LocationInChassis String
> > +  0,                        // ChassisHandle;
> > +  BaseBoardTypeMotherBoard, // BoardType;
> > +  0,                        // NumberOfContainedObjectHandles;
> > +  { 0 }                     // ContainedObjectHandles[1];
> > +};
> > +CHAR8  *mBoardInfoType2Strings[] = {
> > +  "SolidRun",
> > +  "HummingBoard-Edge i4Pro",
> > +  "2.0",
> > +  "Base Board Serial#",
> > +  "Base Board Asset Tag#",
> > +  "hb04w-e-110-00-004-0",
> 
> Do we have any way of obtaining the four above programmatically?
> 
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE3  mEnclosureInfoType3 = {
> > +  { EFI_SMBIOS_TYPE_SYSTEM_ENCLOSURE, sizeof
> (SMBIOS_TABLE_TYPE3), 0 },
> > +  1,                          // Manufacturer String
> > +  MiscChassisTypeOther,       // Type;
> > +  2,                          // Version String
> > +  3,                          // SerialNumber String
> > +  4,                          // AssetTag String
> > +  ChassisStateSafe,           // BootupState;
> > +  ChassisStateSafe,           // PowerSupplyState;
> > +  ChassisStateSafe,           // ThermalState;
> > +  ChassisSecurityStatusNone,  // SecurityStatus;
> > +  { 0, 0, 0, 0 },             // OemDefined[4];
> > +  0,                          // Height;
> > +  0,                          // NumberofPowerCords;
> > +  0,                          // ContainedElementCount;
> > +  0,                          // ContainedElementRecordLength;
> > +  { { 0 } },                  // ContainedElements[1];
> > +};
> > +CHAR8  *mEnclosureInfoType3Strings[] = {
> > +  "SolidRun",
> > +  "2.0",
> > +  "Chassis Board Serial#",
> > +  "Chassis Board Asset Tag#",
> 
> Do we have any way of obtaining the three above programmatically?
> 
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = {
> > +  { EFI_SMBIOS_TYPE_PROCESSOR_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE4), 0},
> > +  1,                                // Socket String;
> > +  CentralProcessor,                 // ProcessorType;
> > +  ProcessorFamilyIndicatorFamily2,  // ProcessorFamily;
> > +  2,                                // ProcessorManufacture String;
> > +  {                                 // ProcessorId;
> > +    {     // PROCESSOR_SIGNATURE
> > +      0,  //  ProcessorSteppingId:4;
> > +      0,  //  ProcessorModel:     4;
> > +      0,  //  ProcessorFamily:    4;
> > +      0,  //  ProcessorType:      2;
> > +      0,  //  ProcessorReserved1: 2;
> > +      0,  //  ProcessorXModel:    4;
> > +      0,  //  ProcessorXFamily:   8;
> > +      0,  //  ProcessorReserved2: 4;
> > +    },
> > +    {     // PROCESSOR_FEATURE_FLAGS
> > +      0,  //  ProcessorFpu       :1;
> > +      0,  //  ProcessorVme       :1;
> > +      0,  //  ProcessorDe        :1;
> > +      0,  //  ProcessorPse       :1;
> > +      0,  //  ProcessorTsc       :1;
> > +      0,  //  ProcessorMsr       :1;
> > +      0,  //  ProcessorPae       :1;
> > +      0,  //  ProcessorMce       :1;
> > +      0,  //  ProcessorCx8       :1;
> > +      0,  //  ProcessorApic      :1;
> > +      0,  //  ProcessorReserved1 :1;
> > +      0,  //  ProcessorSep       :1;
> > +      0,  //  ProcessorMtrr      :1;
> > +      0,  //  ProcessorPge       :1;
> > +      0,  //  ProcessorMca       :1;
> > +      0,  //  ProcessorCmov      :1;
> > +      0,  //  ProcessorPat       :1;
> > +      0,  //  ProcessorPse36     :1;
> > +      0,  //  ProcessorPsn       :1;
> > +      0,  //  ProcessorClfsh     :1;
> > +      0,  //  ProcessorReserved2 :1;
> > +      0,  //  ProcessorDs        :1;
> > +      0,  //  ProcessorAcpi      :1;
> > +      0,  //  ProcessorMmx       :1;
> > +      0,  //  ProcessorFxsr      :1;
> > +      0,  //  ProcessorSse       :1;
> > +      0,  //  ProcessorSse2      :1;
> > +      0,  //  ProcessorSs        :1;
> > +      0,  //  ProcessorReserved3 :1;
> > +      0,  //  ProcessorTm        :1;
> > +      0,  //  ProcessorReserved4 :2;
> > +    }
> > +  },
> > +  3,    // ProcessorVersion String;
> > +  {     // Voltage;
> > +    1,  // ProcessorVoltageCapability5V        :1;
> > +    1,  // ProcessorVoltageCapability3_3V      :1;
> > +    1,  // ProcessorVoltageCapability2_9V      :1;
> > +    0,  // ProcessorVoltageCapabilityReserved  :1;
> > +    0,  // ProcessorVoltageReserved            :3;
> > +    0   // ProcessorVoltageIndicateLegacy      :1;
> > +  },
> > +  0,                      // ExternalClock;
> > +  1200,                   // MaxSpeed;
> > +  792,                    // CurrentSpeed;
> > +  0x41,                   // Status;
> > +  ProcessorUpgradeOther,  // ProcessorUpgrade;
> > +  0,                      // L1CacheHandle;
> > +  0,                      // L2CacheHandle;
> > +  0,                      // L3CacheHandle;
> > +  4,                      // SerialNumber;
> > +  5,                      // AssetTag;
> > +  6,                      // PartNumber;
> > +  0,                      // CoreCount;
> > +  0,                      // EnabledCoreCount;
> > +  0,                      // ThreadCount;
> > +  0,                      // ProcessorCharacteristics;
> > +  ProcessorFamilyARMv7,   // ProcessorFamily2;
> > +};
> > +
> > +CHAR8 *mProcessorInfoType4Strings[] = {
> > +  "SoM",
> > +  "NXP",
> > +  "i.MX 6Quad",
> 
> i.MX6 Quad?
>

Yep.  Will fix.
 
> > +  "1.0",
> > +  "1.0",
> > +  "1.0",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE7  mCacheInfoType7I1 = {
> > +  { EFI_SMBIOS_TYPE_CACHE_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE7), 0 },
> > +  1,      // SocketDesignation String
> > +  0x018A, // Cache Configuration
> > +  0x0020, // Maximum Size 32k
> > +  0x0020, // Install Size 32k
> > +  {       // Supported SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  {       // Current SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  0,                      // Cache Speed unknown
> > +  CacheErrorMultiBit,     // Error Correction Multi
> > +  CacheTypeInstruction,   // System Cache Type
> > +  CacheAssociativity2Way  // Associativity };
> > +CHAR8  *mCacheInfoType7I1Strings[] = {
> > +  "L1 ICache",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE7  mCacheInfoType7D1 = {
> > +  { EFI_SMBIOS_TYPE_CACHE_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE7), 0 },
> > +  1,      // SocketDesignation String
> > +  0x018A, // Cache Configuration
> > +  0x0020, // Maximum Size 32k
> > +  0x0020, // Install Size 32k
> > +  {       // Supported SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  {       // Current SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  0,                      // Cache Speed unknown
> > +  CacheErrorMultiBit,     // Error Correction Multi
> > +  CacheTypeData,          // System Cache Type
> > +  CacheAssociativity2Way  // Associativity };
> > +CHAR8  *mCacheInfoType7D1Strings[] = {
> > +  "L1 DCache",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE7  mCacheInfoType7U2 = {
> > +  { EFI_SMBIOS_TYPE_CACHE_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE7), 0 },
> > +  1,      // SocketDesignation String
> > +  0x018A, // Cache Configuration
> > +  0x0400, // Maximum Size 1M
> > +  0x0400, // Install Size 1M
> > +  {       // Supported SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  {       // Current SRAM Type
> > +    0,    //Other             :1
> > +    0,    //Unknown           :1
> > +    0,    //NonBurst          :1
> > +    1,    //Burst             :1
> > +    0,    //PiplelineBurst    :1
> > +    1,    //Synchronous       :1
> > +    0,    //Asynchronous      :1
> > +    0     //Reserved          :9
> > +  },
> > +  0,                      // Cache Speed unknown
> > +  CacheErrorMultiBit,     // Error Correction Multi
> > +  CacheTypeUnified,       // System Cache Type
> > +  CacheAssociativity2Way  // Associativity };
> > +CHAR8  *mCacheInfoType7U2Strings[] = {
> > +  "L2 UCache (PL310)",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE9  mSysSlotInfoType9 = {
> > +  { EFI_SMBIOS_TYPE_SYSTEM_SLOTS, sizeof (SMBIOS_TABLE_TYPE9), 0 },
> > +  1,                      // SlotDesignation String
> > +  SlotTypeOther,          // SlotType;
> > +  SlotDataBusWidthOther,  // SlotDataBusWidth;
> > +  SlotUsageAvailable,     // CurrentUsage;
> > +  SlotLengthOther,        // SlotLength;
> > +  0,    // SlotID;
> > +  {     // SlotCharacteristics1;
> > +    1,  // CharacteristicsUnknown  :1;
> > +    0,  // Provides50Volts         :1;
> > +    0,  // Provides33Volts         :1;
> > +    0,  // SharedSlot              :1;
> > +    0,  // PcCard16Supported       :1;
> > +    0,  // CardBusSupported        :1;
> > +    0,  // ZoomVideoSupported      :1;
> > +    0,  // ModemRingResumeSupported:1;
> > +  },
> > +  {     // SlotCharacteristics2;
> > +    0,  // PmeSignalSupported      :1;
> > +    0,  // HotPlugDevicesSupported :1;
> > +    0,  // SmbusSignalSupported    :1;
> > +    0,  // Reserved                :5;
> > +  },
> > +  0,    // SegmentGroupNum;
> > +  0,    // BusNum;
> > +  0,    // DevFuncNum;
> > +};
> > +CHAR8  *mSysSlotInfoType9Strings[] = {
> > +  "SD Card",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE16 mPhyMemArrayInfoType16 = {
> > +  { EFI_SMBIOS_TYPE_PHYSICAL_MEMORY_ARRAY, sizeof
> > +(SMBIOS_TABLE_TYPE16), 0 },
> > +  MemoryArrayLocationSystemBoard, // Location;
> > +  MemoryArrayUseSystemMemory,     // Use;
> > +  MemoryErrorCorrectionUnknown,   // MemoryErrorCorrection;
> > +  0x80000000,                     // MaximumCapacity (2 GB);
> > +  0xFFFE,                         // MemoryErrorInformationHandle;
> > +  1,                              // NumberOfMemoryDevices;
> > +  0x80000000ULL,                  // ExtendedMaximumCapacity (2 GB)
> > +};
> > +CHAR8 *mPhyMemArrayInfoType16Strings[] = {
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE17 mMemDevInfoType17 = {
> > +  { EFI_SMBIOS_TYPE_MEMORY_DEVICE, sizeof (SMBIOS_TABLE_TYPE17),
> 0 },
> > +  0,          // MemoryArrayHandle; Matches SMBIOS_TABLE_TYPE16.Handle,
> see PhyMemArrayInfoUpdateSmbiosType16()
> > +  0xFFFE,     // MemoryErrorInformationHandle;
> > +  0xFFFF,     // TotalWidth;
> > +  0xFFFF,     // DataWidth;
> > +  0x0800,     // Size; // When bit 15 is 0: Size in MB
> > +                       // When bit 15 is 1: Size in KB, and continues
> > +in ExtendedSize
> > +  MemoryFormFactorUnknown,  // FormFactor;
> > +  0xff,                     // DeviceSet;
> > +  1,                        // DeviceLocator String
> > +  2,                        // BankLocator String
> > +  MemoryTypeDram,           // MemoryType;
> > +  {     // TypeDetail;
> > +    0,  // Reserved        :1;
> > +    0,  // Other           :1;
> > +    1,  // Unknown         :1;
> > +    0,  // FastPaged       :1;
> > +    0,  // StaticColumn    :1;
> > +    0,  // PseudoStatic    :1;
> > +    0,  // Rambus          :1;
> > +    0,  // Synchronous     :1;
> > +    0,  // Cmos            :1;
> > +    0,  // Edo             :1;
> > +    0,  // WindowDram      :1;
> > +    0,  // CacheDram       :1;
> > +    0,  // Nonvolatile     :1;
> > +    0,  // Registered      :1;
> > +    0,  // Unbuffered      :1;
> > +    0,  // Reserved1       :1;
> > +  },
> > +  0,          // Speed;
> > +  3,          // Manufacturer String
> > +  0,          // SerialNumber String
> > +  0,          // AssetTag String
> > +  0,          // PartNumber String
> > +  0,          // Attributes;
> > +  0,          // ExtendedSize;
> > +  0,          // ConfiguredMemoryClockSpeed;
> > +};
> > +CHAR8 *mMemDevInfoType17Strings[] = {
> > +  "OS Virtual Memory",
> > +  "malloc",
> > +  "OSV",
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE19 mMemArrMapInfoType19 = {
> > +  { EFI_SMBIOS_TYPE_MEMORY_ARRAY_MAPPED_ADDRESS, sizeof
> > +(SMBIOS_TABLE_TYPE19), 0 },
> > +  0x10000000, // StartingAddress;
> > +  0x8fffffff, // EndingAddress;
> > +  0,          // MemoryArrayHandle;
> > +  1,          // PartitionWidth;
> > +  0,          // ExtendedStartingAddress;
> > +  0,          // ExtendedEndingAddress;
> > +};
> > +CHAR8 *mMemArrMapInfoType19Strings[] = {
> > +  NULL
> > +};
> > +
> > +SMBIOS_TABLE_TYPE32 mBootInfoType32 = {
> > +  { EFI_SMBIOS_TYPE_SYSTEM_BOOT_INFORMATION, sizeof
> (SMBIOS_TABLE_TYPE32), 0 },
> > +  { 0, 0, 0, 0, 0, 0 },         // Reserved[6];
> > +  BootInformationStatusNoError  // BootStatus };
> > +
> > +CHAR8 *mBootInfoType32Strings[] = {
> > +  NULL
> > +};
> > +
> > +/**
> > +
> > +  Create SMBIOS record.
> > +
> > +  Converts a fixed SMBIOS structure and an array of pointers to
> > + strings into  an SMBIOS record where the strings are cat'ed on the
> > + end of the fixed record  and terminated via a double NULL and add to
> SMBIOS table.
> > +
> > +  @param  Template    Fixed SMBIOS structure, required.
> > +  @param  StringPack  Array of strings to convert to an SMBIOS string pack.
> > +                      NULL is OK.
> > +  @param  DataSmbiosHandle  The new SMBIOS record handle .
> > +                      NULL is OK.
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +LogSmbiosData (
> > +  IN  EFI_SMBIOS_TABLE_HEADER *Template,
> > +  IN  CHAR8                   **StringPack,
> > +  OUT EFI_SMBIOS_HANDLE       *DataSmbiosHandle
> > +  )
> > +{
> > +  EFI_STATUS                Status;
> > +  EFI_SMBIOS_PROTOCOL       *Smbios;
> > +  EFI_SMBIOS_HANDLE         SmbiosHandle;
> > +  EFI_SMBIOS_TABLE_HEADER   *Record;
> > +  UINTN                     Index;
> > +  UINTN                     StringSize;
> > +  UINTN                     Size;
> > +  CHAR8                     *Str;
> > +
> > +  Status = gBS->LocateProtocol (&gEfiSmbiosProtocolGuid, NULL, (VOID
> > + **)&Smbios);  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  // Calculate the size of the fixed record and optional string pack
> > + Size = Template->Length;  if (StringPack == NULL) {
> > +    // At least a double null is required
> > +    Size += 2;
> > +  } else {
> > +    for (Index = 0; StringPack[Index] != NULL; Index++) {
> > +      StringSize = AsciiStrSize (StringPack[Index]);
> > +      Size += StringSize;
> > +    }
> > +  if (StringPack[0] == NULL) {
> > +    // At least a double null is required
> > +    Size += 1;
> > +    }
> > +
> > +    // Don't forget the terminating double null
> > +    Size += 1;
> > +  }
> > +
> > +  // Copy over Template
> > +  Record = (EFI_SMBIOS_TABLE_HEADER *)AllocateZeroPool (Size);  if
> > + (Record == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +  CopyMem (Record, Template, Template->Length);
> > +
> > +  // Append string pack
> > +  Str = ((CHAR8 *)Record) + Record->Length;
> > +
> > +  for (Index = 0; StringPack[Index] != NULL; Index++) {
> > +    StringSize = AsciiStrSize (StringPack[Index]);
> > +    CopyMem (Str, StringPack[Index], StringSize);
> > +    Str += StringSize;
> > +  }
> > +
> > +  *Str = 0;
> > +  SmbiosHandle = SMBIOS_HANDLE_PI_RESERVED;  Status = Smbios->Add
> (
> > +                     Smbios,
> > +                     gImageHandle,
> > +                     &SmbiosHandle,
> > +                     Record
> > +                     );
> > +  if ((Status == EFI_SUCCESS) && (DataSmbiosHandle != NULL)) {
> > +      *DataSmbiosHandle = SmbiosHandle;  }
> > +
> > +  ASSERT_EFI_ERROR (Status);
> > +  FreePool (Record);
> > +  return Status;
> > +}
> > +
> > +VOID
> > +BIOSInfoUpdateSmbiosType0 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mBIOSInfoType0,
> > +mBIOSInfoType0Strings, NULL); }
> > +
> > +VOID
> > +I64ToHexString(
> 
> Hmm. We already have several copies of functions doing exactly this in EDK2.
> Just no centrally reusable one. Maybe worth adding to BaseLib?
> 
> Regardless, it should probablt be called UINT64ToHexString or suchlike.
> 

I'll rename it to UINT64ToHexString for the next patch set, then will look into making a centrally reusable version.

> And a space before (.
> 

Autoformatter (AStyle) does catch and fix this scenario.

> > +    IN OUT CHAR8* TargetStringSz,
> > +    IN UINT32 TargetStringSize,
> 
> One parameter called Sz, and another called Size?
> And the first one is where the string goes?
>

Will rename Sz, Inx, etc to be standard names.  Not exactly sure why one of our devs decided to use this notation...

> > +    IN UINT64 Value
> > +    )
> > +{
> > +    static CHAR8 ItoH[] = {
> > +'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' };
> 
> STATIC
> ItoH is what I often see this funcion named.
> HexCharacters would be a more suitable CamelCase name.
> 

Will change to HexCharacters.

> > +    UINT8 StringInx;
> > +    INT8 NibbleInx;
> 
> Inx->Index.
> 
> > +
> > +    ZeroMem((void*)TargetStringSz, TargetStringSize);
> 
> Space before (.
> (VOID *)
> 

AStyle fixes the spacing before ( and around operators.  I'll also change void -> VOID

> > +
> > +    //
> > +    // Convert each nibble to hex string, starting from
> > +    // the highest non-zero nibble.
> > +    //
> > +    StringInx = 0;
> > +    for (NibbleInx = sizeof(UINT64) * 2; NibbleInx > 0; --NibbleInx) {
> > +        UINT64 NibbleMask = (((UINT64)0xF) << ((NibbleInx - 1) * 4));
> > +        UINT8 Nibble = (UINT8)((Value & NibbleMask) >> ((NibbleInx -
> > + 1) * 4));
> > +
> > +        ASSERT(Nibble <= 0xF);
> 
> This assert checks only whether the algorithm is correct or whether the
> compiler generates code correctly. If the former is a problem, I recommend a
> simpler algorithm.
> Meanwhile, I don't see an ASSERT verifying that the output buffer is large
> enough.
> 

Will fix this implementation and add the ASSERT output buffer size.

> > +
> > +        if (StringInx < (TargetStringSize-1)) {
> 
> Spaces around '-'.
> 
> > +            TargetStringSz[StringInx++] = ItoH[Nibble];
> > +        } else {
> > +            break;
> > +        }
> > +    }
> > +}
> > +
> > +UINT64 ObtainPlatformId(VOID)
> 
> Format like BIOSInfoUpdateSmbiosType0 above.
> 

Will fix.

> > +{
> > +   UINT64 BoardSerial=(UINT64)0;
> 
> Spaces around '='. (If we don't bring up the "no assignment on definition"
> rule from the coding style.
> 

The next patch set will follow the "no assignment on definition" rule.

> Why cast that zero?
> 

Will fix.

> > +
> > +   // see iMX6 reference manual section 46.5.10 for OTP CFG registers
> description
> > +   const UINT32 RegOCOTP_CFG0Addr=0x021BC410; // OTP Bank0 Word1
> contains lower 32 bits of the Unique ID and SJC_CHALLENGE field
> > +   const UINT32 RegOCOTP_CFG1Addr=0x021BC420; // OTP Bank0 Word2
> > + contains upper 32 bits of the Unique ID and SJC_CHALLENGE field
> 
> CONST.
> Spaces around '='.
> Lines too long. (Move Comments to separate lines.)
> 
> Please put these addresses as #defines in a local .h file instead (unless
> there's a global i.MX6 .h file they'd fit better in).
> 

Will move the register defines into our global i.MX6.h in our silicon package.

> > +   BoardSerial = ((UINT64)(MmioRead32(RegOCOTP_CFG1Addr))) << 32;
> > +   BoardSerial = BoardSerial |
> > + ((UINT64)(MmioRead32(RegOCOTP_CFG0Addr)));
> 
> MmioRead32 (
> A bit heavy on the parantheses.
> 

Will fix.

> > +
> > +   DEBUG((DEBUG_INFO, "Hummingboard Edge ser no %08X%08Xh \r\n",
> > + (UINT32)(BoardSerial>>32), (UINT32)BoardSerial));
> 
> Spaces around '<<'.
> Wrap long line.
> 

AStyle also helps identify long line situations.  Will fix it.

> > +   return BoardSerial;
> > +}
> > +
> > +VOID
> > +SysInfoUpdateSmbiosType1 (
> > +  VOID
> > +  )
> > +{
> > +   UINT64 BoardSerial=(UINT64)0;
> 
> Spaces around '=' (please address throughout).
> 
> > +   UINT32 Bank0Word3=0;
> > +   static CHAR8 BoardSerialString[sizeof(BoardSerial) * 2 +
> > + 1]={0x00};
> 
> STATIC.
> 
> > +   int k=0;
> 
> I?
> 

Will fix and use INT32.

> > +
> > +   const UINT32 RegOCOTP_CFG2Addr=0x021BC430;
> 
> Please put these addresses as #defines in a local .h file instead (unless
> there's a global i.MX6 .h file they'd fit better in).
> 
> > +   BoardSerial = ObtainPlatformId();
> 
> Space before (.
> 
> > +
> > +   // Update the Smbios Type1 information with the board serial string
> > +   I64ToHexString(&BoardSerialString[0], sizeof(BoardSerialString),
> > + BoardSerial);
> 
> Space before (.
> 
> > +   mSysInfoType1Strings[mSysInfoType1.SerialNumber - 1] =
> > + &BoardSerialString[0];
> > +
> > +   Bank0Word3 = MmioRead32(RegOCOTP_CFG2Addr);
> > +   // Construct string to make UUID: fixed prefix + board serial number
> printed in hex
> > +   mSysInfoType1.Uuid.Data1=((UINT32)'H'<<24) | ((UINT32)'M'<<16) |
> ((UINT32)'B'<<8) | (UINT32)'E' ;
> > +   mSysInfoType1.Uuid.Data2=(UINT16)Bank0Word3;
> > +   mSysInfoType1.Uuid.Data3=(UINT16)(Bank0Word3>>16);
> > +
> > +   for(k=7; k>=0; k--) {
> > +      mSysInfoType1.Uuid.Data4[7-k]=(UINT8)(BoardSerial>>(k*8));
> 
> Spaces around '>=', '-', '>>', '*'.
> 
> The SMBIOS specification defines the format for this field, and this is not it.
> Only the last 6 bytes should be node-specific - the rest should be generated
> once and reused for all Hummingboard systems. (For example by using
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> guidgenerator.com%2Fonline-guid-
> generator.aspx&amp;data=02%7C01%7CChristopher.Co%40microsoft.com%
> 7C635c700b11db4c8dcfd708d5fc6446fa%7C72f988bf86f141af91ab2d7cd011db
> 47%7C1%7C0%7C636692429993081378&amp;sdata=wudEX5bOKI37TvKL49%2
> BQiksD9vUbfRIqX1oFXAhUluo%3D&amp;reserved=0.)
> 
> RFC4122, section 4.1.6, also mentions if you're _not_ using a MAC address for
> this, you should set the multicast bit to avoid clashes.
> (But basically, my suggestion would be to use the MAC address if you have
> access to it from here.)
> 

Not sure where the previous dev came up with this.  I'll switch it to use the MAC for the last 6 bytes since we do have access to it at this stage.

Thanks,
Chris

> > +   }
> > +
> > +   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mSysInfoType1,
> > + mSysInfoType1Strings, NULL);
> > +
> > +}
> > +
> > +VOID
> > +BoardInfoUpdateSmbiosType2 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mBoardInfoType2,
> > +mBoardInfoType2Strings, NULL); }
> > +
> > +VOID
> > +EnclosureInfoUpdateSmbiosType3 (
> > +  VOID
> > +  )
> > +{
> > +  UINT8 ChassisSerialNo=0xFF;
> > +  static CHAR8 ChassisBoardSerialString[sizeof(ChassisSerialNo) * 2 +
> > +1]={0x00};
> 
> STATIC. (Please address throughout.)
> S[ace before (.
> 
> > +
> > +  // Generate chassis number from same platform id.
> > +  ChassisSerialNo = (UINT8)(ObtainPlatformId() >> 40);
> > + DEBUG((DEBUG_INFO, "Chassis Board Serial %02Xh \r\n",
> > + ChassisSerialNo));
> > +
> > +  I64ToHexString(&ChassisBoardSerialString[0],
> > + sizeof(ChassisBoardSerialString), ChassisSerialNo);
> 
> Space after (.
> Wrap long line.
> 
> > +  mEnclosureInfoType3Strings[mEnclosureInfoType3.SerialNumber - 1] =
> > + &ChassisBoardSerialString[0];
> > +
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER
> *)&mEnclosureInfoType3,
> > + mEnclosureInfoType3Strings, NULL);
> 
> Please wrap long lines.
> 
> > +}
> > +
> > +VOID
> > +ProcessorInfoUpdateSmbiosType4 (
> > +  IN UINTN MaxCpus
> > +  )
> > +{
> > +  mProcessorInfoType4.CoreCount        = (UINT8) MaxCpus;
> > +  mProcessorInfoType4.EnabledCoreCount = (UINT8) MaxCpus;
> > +  mProcessorInfoType4.ThreadCount      = (UINT8) MaxCpus;
> > +
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER
> *)&mProcessorInfoType4,
> > + mProcessorInfoType4Strings, NULL);
> 
> Wrap long line.
> 
> > +}
> > +
> > +VOID
> > +CacheInfoUpdateSmbiosType7 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mCacheInfoType7I1,
> > +mCacheInfoType7I1Strings, NULL);
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mCacheInfoType7D1,
> > +mCacheInfoType7D1Strings, NULL);
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mCacheInfoType7U2,
> > +mCacheInfoType7U2Strings, NULL);
> 
> Please wrap long lines.
> 
> > +}
> > +
> > +VOID
> > +SysSlotInfoUpdateSmbiosType9 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mSysSlotInfoType9,
> > +mSysSlotInfoType9Strings, NULL);
> 
> Wrap long line.
> 
> > +}
> > +
> > +VOID
> > +PhyMemArrayInfoUpdateSmbiosType16 (
> > +  VOID
> > +  )
> > +{
> > +  EFI_SMBIOS_HANDLE MemArraySmbiosHandle;
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER
> *)&mPhyMemArrayInfoType16,
> > +mPhyMemArrayInfoType16Strings, &MemArraySmbiosHandle);
> 
> Wrap long line.
> 
> > +
> > +  // Update the memory device information
> > +  mMemDevInfoType17.MemoryArrayHandle = MemArraySmbiosHandle;
> }
> > +
> > +VOID
> > +MemDevInfoUpdateSmbiosType17 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER
> *)&mMemDevInfoType17,
> > +mMemDevInfoType17Strings, NULL);
> 
> Wrap long line.
> 
> > +}
> > +
> > +VOID
> > +MemArrMapInfoUpdateSmbiosType19 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER
> *)&mMemArrMapInfoType19,
> > +mMemArrMapInfoType19Strings, NULL);
> 
> Wrap long line.
> 
> > +}
> > +
> > +VOID
> > +BootInfoUpdateSmbiosType32 (
> > +  VOID
> > +  )
> > +{
> > +  LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mBootInfoType32,
> > +mBootInfoType32Strings, NULL);
> 
> Wrap long line.
> 
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +PlatformSmbiosDriverEntryPoint (
> > +  IN EFI_HANDLE        ImageHandle,
> > +  IN EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  BIOSInfoUpdateSmbiosType0();
> > +
> > +  SysInfoUpdateSmbiosType1();
> > +
> > +  BoardInfoUpdateSmbiosType2();
> > +
> > +  EnclosureInfoUpdateSmbiosType3();
> > +
> > +  ProcessorInfoUpdateSmbiosType4 (FixedPcdGet32(PcdCoreCount));
> > +
> > +  CacheInfoUpdateSmbiosType7();
> > +
> > +  SysSlotInfoUpdateSmbiosType9();
> > +
> > +  PhyMemArrayInfoUpdateSmbiosType16();
> > +
> > +  MemDevInfoUpdateSmbiosType17();
> > +
> > +  MemArrMapInfoUpdateSmbiosType19();
> > +
> > +  BootInfoUpdateSmbiosType32();
> > +
> > +  return EFI_SUCCESS;
> > +}
> > diff --git
> >
> a/Platform/SolidRun/HummingboardEdge_iMX6Q_2GB/Drivers/PlatformSm
> biosD
> > xe/PlatformSmbiosDxe.inf
> >
> b/Platform/SolidRun/HummingboardEdge_iMX6Q_2GB/Drivers/PlatformSm
> biosD
> > xe/PlatformSmbiosDxe.inf
> > new file mode 100644
> > index 000000000000..4efcd76ffb47
> > --- /dev/null
> > +++
> b/Platform/SolidRun/HummingboardEdge_iMX6Q_2GB/Drivers/PlatformSm
> b
> > +++ iosDxe/PlatformSmbiosDxe.inf
> > @@ -0,0 +1,54 @@
> > +#/** @file
> > +#
> > +#  Copyright (c) 2013 Linaro.org
> > +#  Copyright (c), Microsoft Corporation. All rights reserved.
> > +#
> > +#  This program and the accompanying materials #  are licensed and
> > +made available under the terms and conditions of the BSD License #
> > +which accompanies this distribution.  The full text of the license
> > +may be found at #
> >
> +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C635c700b11db4c8dcfd708d5fc6446fa%7C72f988bf
> 86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636692429993081378&amp;sdata=sS
> W6z4b4V
> > +RI%2BGGnJ0lDyDx3AXHOH0Sr64IISp8G3QXU%3D&amp;reserved=0
> > +#
> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS, #  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +#
> > +#**/
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
> 
> 001a
> 
> > +  BASE_NAME                      = PlatformSmbiosDxe
> > +  FILE_GUID                      = 3847D23F-1D95-4772-B60C-4BBFBC4D532F
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = PlatformSmbiosDriverEntryPoint
> > +
> > +[Sources]
> > +  PlatformSmbiosDxe.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  ArmPkg/ArmPkg.dec
> > +  ArmPlatformPkg/ArmPlatformPkg.dec
> > +  Silicon/NXP/iMX6Pkg/iMX6Pkg.dec
> 
> Please sort alphabetically.
> 
> > +
> > +[LibraryClasses]
> > +  UefiBootServicesTableLib
> > +  MemoryAllocationLib
> > +  BaseMemoryLib
> > +  BaseLib
> > +  UefiLib
> > +  UefiDriverEntryPoint
> > +  IoLib
> > +  DebugLib
> > +  IoLib
> 
> Please sort alphabetically.
> I think one IoLib is enough.
> 
> /
>     Leif
> 
> > +
> > +[Protocols]
> > +  gEfiSmbiosProtocolGuid                        # PROTOCOL
> SOMETIMES_CONSUMED
> > +
> > +[Guids]
> > +
> > +[FixedPcd]
> > + gArmPlatformTokenSpaceGuid.PcdCoreCount
> > +
> > +[Depex]
> > +  gEfiSmbiosProtocolGuid
> > --
> > 2.16.2.gvfs.1.33.gf5370f1
> >