[edk2-platforms][PATCH v3 0/5] Various SMBIOS improvements

[edk2-platforms][PATCH v3 0/5] Various SMBIOS improvements

[Pete Batard]

Changes from v2:
- Consistent use of curly brackets for if statements
- Remove unused variables that may produce an issue with git bisect
- Use gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor to populate vendor

Note that 5/5, which has been R-b, is unchanged.

Pete Batard (5):
  Platform/RPi3/RpiFirmwareDxe: Add more query functions
  Platform/RPi3/RpiFirmwareDxe: Improve serial number population
  Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries
  Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD
  Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 198 ++++++++++++--------
 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |   2 +
 Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c         | 167 ++++++++++++++++-
 Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h                  |  58 ++++--
 Platform/RaspberryPi/RPi3/RPi3.dsc                                        |   4 +-
 5 files changed, 333 insertions(+), 96 deletions(-)

-- 
2.21.0.windows.1

[edk2-platforms][PATCH v3 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions

[Pete Batard]

This patch introduces the capability to also query the Model Name/
Manufacturer Name/CPU Name/Firmware Revision using the RpiFirmware
protocol. This is aims at making the driver more suitable to cater
for platforms other than the Raspberry Pi 3 as well as simplifying
the population of entries in PlatformSmbiosDxe.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 158 +++++++++++++++++++-
 Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h          |  58 +++++--
 2 files changed, 199 insertions(+), 17 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 9b5ee1946279..25d7fa3974c0 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -461,7 +461,7 @@ typedef struct {
   RPI_FW_TAG_HEAD           TagHead;
   RPI_FW_MODEL_REVISION_TAG TagBody;
   UINT32                    EndTag;
-} RPI_FW_GET_MODEL_REVISION_CMD;
+} RPI_FW_GET_REVISION_CMD;
 #pragma pack()
 
 STATIC
@@ -471,7 +471,7 @@ RpiFirmwareGetModelRevision (
   OUT   UINT32 *Revision
   )
 {
-  RPI_FW_GET_MODEL_REVISION_CMD *Cmd;
+  RPI_FW_GET_REVISION_CMD       *Cmd;
   EFI_STATUS                    Status;
   UINT32                        Result;
 
@@ -506,6 +506,156 @@ RpiFirmwareGetModelRevision (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+EFIAPI
+RpiFirmwareGetFirmwareRevision (
+  OUT   UINT32 *Revision
+  )
+{
+  RPI_FW_GET_REVISION_CMD       *Cmd;
+  EFI_STATUS                    Status;
+  UINT32                        Result;
+
+  if (!AcquireSpinLockOrFail (&mMailboxLock)) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__));
+    return EFI_DEVICE_ERROR;
+  }
+
+  Cmd = mDmaBuffer;
+  ZeroMem (Cmd, sizeof (*Cmd));
+
+  Cmd->BufferHead.BufferSize  = sizeof (*Cmd);
+  Cmd->BufferHead.Response    = 0;
+  Cmd->TagHead.TagId          = RPI_MBOX_GET_REVISION;
+  Cmd->TagHead.TagSize        = sizeof (Cmd->TagBody);
+  Cmd->TagHead.TagValueSize   = 0;
+  Cmd->EndTag                 = 0;
+
+  Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result);
+
+  ReleaseSpinLock (&mMailboxLock);
+
+  if (EFI_ERROR (Status) ||
+      Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: mailbox transaction error: Status == %r, Response == 0x%x\n",
+      __FUNCTION__, Status, Cmd->BufferHead.Response));
+    return EFI_DEVICE_ERROR;
+  }
+
+  *Revision = Cmd->TagBody.Revision;
+  return EFI_SUCCESS;
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetModelName (
+  IN INTN ModelId
+  )
+{
+  UINT32  Revision;
+
+  // If a negative ModelId is passed, detect it.
+  if ((ModelId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+    ModelId = (Revision >> 4) & 0xFF;
+  }
+
+  switch (ModelId) {
+  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+  case 0x00:
+    return "Raspberry Pi Model A";
+  case 0x01:
+    return "Raspberry Pi Model B";
+  case 0x02:
+    return "Raspberry Pi Model A+";
+  case 0x03:
+    return "Raspberry Pi Model B+";
+  case 0x04:
+    return "Raspberry Pi 2 Model B";
+  case 0x06:
+    return "Raspberry Pi Compute Module 1";
+  case 0x08:
+    return "Raspberry Pi 3 Model B";
+  case 0x09:
+    return "Raspberry Pi Zero";
+  case 0x0A:
+    return "Raspberry Pi Compute Module 3";
+  case 0x0C:
+    return "Raspberry Pi Zero W";
+  case 0x0D:
+    return "Raspberry Pi 3 Model B+";
+  case 0x0E:
+    return "Raspberry Pi 3 Model A+";
+  case 0x11:
+    return "Raspberry Pi 4 Model B";
+  default:
+    return "Unknown Raspberry Pi Model";
+  }
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetManufacturerName (
+  IN INTN ManufacturerId
+  )
+{
+  UINT32  Revision;
+
+  // If a negative ModelId is passed, detect it.
+  if ((ManufacturerId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+    ManufacturerId = (Revision >> 16) & 0x0F;
+  }
+
+  switch (ManufacturerId) {
+  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+  case 0x00:
+    return "Sony UK";
+  case 0x01:
+    return "Egoman";
+  case 0x02:
+  case 0x04:
+    return "Embest";
+  case 0x03:
+    return "Sony Japan";
+  case 0x05:
+    return "Stadium";
+  default:
+    return "Unknown Manufacturer";
+  }
+}
+
+STATIC
+CHAR8*
+EFIAPI
+RpiFirmwareGetCpuName (
+  IN INTN CpuId
+  )
+{
+  UINT32  Revision;
+
+  // If a negative CpuId is passed, detect it.
+  if ((CpuId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
+    CpuId = (Revision >> 12) & 0x0F;
+  }
+
+  switch (CpuId) {
+  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+  case 0x00:
+    return "BCM2835 (ARM11)";
+  case 0x01:
+    return "BCM2836 (ARM Cortex-A7)";
+  case 0x02:
+    return "BCM2837 (ARM Cortex-A53)";
+  case 0x03:
+    return "BCM2711 (ARM Cortex-A72)";
+  default:
+    return "Unknown CPU Model";
+  }
+}
+
 #pragma pack()
 typedef struct {
   UINT32 Width;
@@ -1009,6 +1159,10 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
   RpiFirmwareGetSerial,
   RpiFirmwareGetModel,
   RpiFirmwareGetModelRevision,
+  RpiFirmwareGetModelName,
+  RpiFirmwareGetFirmwareRevision,
+  RpiFirmwareGetManufacturerName,
+  RpiFirmwareGetCpuName,
   RpiFirmwareGetArmMemory
 };
 
diff --git a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
index ec70f28efe1a..e49d6e6132d9 100644
--- a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
+++ b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
@@ -96,6 +96,30 @@ EFI_STATUS
   UINT32 *Revision
   );
 
+typedef
+CHAR8*
+(EFIAPI *GET_MODEL_NAME) (
+  INTN ModelId
+  );
+
+typedef
+EFI_STATUS
+(EFIAPI *GET_FIRMWARE_REVISION) (
+  UINT32 *Revision
+  );
+
+typedef
+CHAR8*
+(EFIAPI *GET_MANUFACTURER_NAME) (
+  INTN ManufacturerId
+  );
+
+typedef
+CHAR8*
+(EFIAPI *GET_CPU_NAME) (
+  INTN CpuId
+  );
+
 typedef
 EFI_STATUS
 (EFIAPI *GET_ARM_MEM) (
@@ -104,21 +128,25 @@ EFI_STATUS
   );
 
 typedef struct {
-  SET_POWER_STATE    SetPowerState;
-  GET_MAC_ADDRESS    GetMacAddress;
-  GET_COMMAND_LINE   GetCommandLine;
-  GET_CLOCK_RATE     GetClockRate;
-  GET_CLOCK_RATE     GetMaxClockRate;
-  GET_CLOCK_RATE     GetMinClockRate;
-  SET_CLOCK_RATE     SetClockRate;
-  GET_FB             GetFB;
-  FREE_FB            FreeFB;
-  GET_FB_SIZE        GetFBSize;
-  SET_LED            SetLed;
-  GET_SERIAL         GetSerial;
-  GET_MODEL          GetModel;
-  GET_MODEL_REVISION GetModelRevision;
-  GET_ARM_MEM        GetArmMem;
+  SET_POWER_STATE       SetPowerState;
+  GET_MAC_ADDRESS       GetMacAddress;
+  GET_COMMAND_LINE      GetCommandLine;
+  GET_CLOCK_RATE        GetClockRate;
+  GET_CLOCK_RATE        GetMaxClockRate;
+  GET_CLOCK_RATE        GetMinClockRate;
+  SET_CLOCK_RATE        SetClockRate;
+  GET_FB                GetFB;
+  FREE_FB               FreeFB;
+  GET_FB_SIZE           GetFBSize;
+  SET_LED               SetLed;
+  GET_SERIAL            GetSerial;
+  GET_MODEL             GetModel;
+  GET_MODEL_REVISION    GetModelRevision;
+  GET_MODEL_NAME        GetModelName;
+  GET_FIRMWARE_REVISION GetFirmwareRevision;
+  GET_MANUFACTURER_NAME GetManufacturerName;
+  GET_CPU_NAME          GetCpuName;
+  GET_ARM_MEM           GetArmMem;
 } RASPBERRY_PI_FIRMWARE_PROTOCOL;
 
 extern EFI_GUID gRaspberryPiFirmwareProtocolGuid;
-- 
2.21.0.windows.1

[edk2-platforms][PATCH v3 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population

[Pete Batard]

Improve RpiFirmwareGetSerial() to derive a serial number from the
MAC address, in case the platform returns 0 for the serial number.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index 25d7fa3974c0..5a9d4c3f1787 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -393,7 +393,14 @@ RpiFirmwareGetSerial (
   }
 
   *Serial = Cmd->TagBody.Serial;
-  return EFI_SUCCESS;
+  // Some platforms return 0 for serial. For those, try to use the MAC address.
+  if (*Serial == 0) {
+    Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
+    // Convert to a more user-friendly value
+    *Serial = SwapBytes64 (*Serial << 16);
+  }
+
+  return Status;
 }
 
 #pragma pack()
-- 
2.21.0.windows.1

[edk2-platforms][PATCH v3 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries

[Pete Batard]

This patch cleans up the population SMBIOS entries by removing elements
that we don't have data for, as well as properly filling the ones for
which we do, through the newly added queries from RpiFirmwareDxe.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 135 ++++++++++----------
 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |   2 +
 Platform/RaspberryPi/RPi3/RPi3.dsc                                        |   4 +-
 3 files changed, 69 insertions(+), 72 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index bc35175279f2..9150dcfd8c5b 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -35,6 +35,7 @@
 #include <Library/UefiDriverEntryPoint.h>
 #include <Library/UefiLib.h>
 #include <Library/BaseLib.h>
+#include <Library/PcdLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
@@ -104,16 +105,19 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = {
           //  VirtualMachineSupported              :1;
           //  ExtensionByte2Reserved               :3;
   },
-  0xFF,                    // SystemBiosMajorRelease
-  0xFF,                    // SystemBiosMinorRelease
-  0xFF,                    // EmbeddedControllerFirmwareMajorRelease
-  0xFF,                    // EmbeddedControllerFirmwareMinorRelease
+  0,                       // SystemBiosMajorRelease
+  0,                       // SystemBiosMinorRelease
+  0,                       // EmbeddedControllerFirmwareMajorRelease
+  0,                       // EmbeddedControllerFirmwareMinorRelease
 };
 
+CHAR8 mBiosVendor[128]  = "EDK2";
+CHAR8 mBiosVersion[128] = "EDK2-DEV";
+
 CHAR8 *mBIOSInfoType0Strings[] = {
-  "https://github.com/andreiw/RaspberryPiPkg",             // Vendor String
-  "Raspberry Pi 64-bit UEFI (" __DATE__ " " __TIME__ ")",  // BiosVersion String
-  __DATE__,
+  mBiosVendor,              // Vendor
+  mBiosVersion,             // Version
+  __DATE__ " " __TIME__,    // Release Date
   NULL
 };
 
@@ -132,42 +136,19 @@ SMBIOS_TABLE_TYPE1 mSysInfoType1 = {
   6,    // Family String
 };
 
-#define PROD_BASE     8
-#define PROD_KNOWN   13
-#define PROD_UNKNOWN 11
-CHAR8 *ProductNames[] = {
-  /* 8 */ "3",
-  /* 9 */ "Zero",
-  /* 10 */ "CM3",
-  /* 11 */ "???",
-  /* 12 */ "Zero W",
-  /* 13 */ "3B+"
-};
-
-#define MANU_UNKNOWN 0
-#define MANU_KNOWN   4
-#define MANU_BASE    1
-CHAR8 *ManufNames[] = {
-  "???",
-  /* 0 */ "Sony",
-  /* 1 */ "Egoman",
-  /* 2 */ "Embest",
-  /* 3 */ "Sony Japan",
-  /* 4 */ "Embest"
-};
-
-CHAR8 mSysInfoManufName[sizeof ("Sony Japan")];
-CHAR8 mSysInfoProductName[sizeof ("64-bit Raspberry Pi XXXXXX (rev. xxxxxxxx)")];
+CHAR8 mSysInfoManufName[128];
+CHAR8 mSysInfoProductName[128];
+CHAR8 mSysInfoVersionName[128];
 CHAR8 mSysInfoSerial[sizeof (UINT64) * 2 + 1];
 CHAR8 mSysInfoSKU[sizeof (UINT64) * 2 + 1];
 
 CHAR8 *mSysInfoType1Strings[] = {
   mSysInfoManufName,
   mSysInfoProductName,
-  mSysInfoProductName,
+  mSysInfoVersionName,
   mSysInfoSerial,
   mSysInfoSKU,
-  "edk2",
+  "Raspberry Pi",
   NULL
 };
 
@@ -180,7 +161,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
   2,    // ProductName String
   3,    // Version String
   4,    // SerialNumber String
-  5,    // AssetTag String
+  0,    // AssetTag String
   {     // FeatureFlag
     1,    //  Motherboard           :1;
     0,    //  RequiresDaughterCard  :1;
@@ -189,7 +170,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
     0,    //  HotSwappable          :1;
     0,    //  Reserved              :3;
   },
-  6,    // LocationInChassis String
+  0,                        // LocationInChassis String
   0,                        // ChassisHandle;
   BaseBoardTypeMotherBoard, // BoardType;
   0,                        // NumberOfContainedObjectHandles;
@@ -198,10 +179,8 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = {
 CHAR8 *mBoardInfoType2Strings[] = {
   mSysInfoManufName,
   mSysInfoProductName,
-  mSysInfoProductName,
+  mSysInfoVersionName,
   mSysInfoSerial,
-  "None",
-  mSysInfoSKU,
   NULL
 };
 
@@ -214,7 +193,7 @@ SMBIOS_TABLE_TYPE3 mEnclosureInfoType3 = {
   MiscChassisEmbeddedPc,    // Type;
   2,                        // Version String
   3,                        // SerialNumber String
-  4,                        // AssetTag String
+  0,                        // AssetTag String
   ChassisStateSafe,         // BootupState;
   ChassisStateSafe,         // PowerSupplyState;
   ChassisStateSafe,         // ThermalState;
@@ -230,7 +209,6 @@ CHAR8 *mEnclosureInfoType3Strings[] = {
   mSysInfoManufName,
   mSysInfoProductName,
   mSysInfoSerial,
-  "None",
   NULL
 };
 
@@ -306,9 +284,9 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = {
   0,                      // L1CacheHandle;
   0,                      // L2CacheHandle;
   0,                      // L3CacheHandle;
-  4,                      // SerialNumber;
-  5,                      // AssetTag;
-  6,                      // PartNumber;
+  0,                      // SerialNumber;
+  0,                      // AssetTag;
+  0,                      // PartNumber;
   4,                      // CoreCount;
   4,                      // EnabledCoreCount;
   4,                      // ThreadCount;
@@ -316,13 +294,12 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = {
   ProcessorFamilyARM,     // ARM Processor Family;
 };
 
+CHAR8 mCpuName[128] = "Unknown ARM CPU";
+
 CHAR8 *mProcessorInfoType4Strings[] = {
   "Socket",
-  "ARM",
-  "BCM2837 ARMv8",
-  "1.0",
-  "1.0",
-  "1.0",
+  "Broadcom",
+  mCpuName,
   NULL
 };
 
@@ -618,6 +595,29 @@ BIOSInfoUpdateSmbiosType0 (
   VOID
   )
 {
+  UINT32 FirmwareRevision = 0;
+  EFI_STATUS Status = EFI_SUCCESS;
+
+  // Populate the Firmware major and minor.
+  Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to get firmware revision: %r\n", Status));
+  } else {
+    // This expects Broadcom / The Raspberry Pi Foundation to switch to
+    // less nonsensical VideoCore firmware revisions in the future...
+    mBIOSInfoType0.EmbeddedControllerFirmwareMajorRelease =
+      (UINT8)((FirmwareRevision >> 16) & 0xFF);
+    mBIOSInfoType0.EmbeddedControllerFirmwareMinorRelease =
+      (UINT8)(FirmwareRevision & 0xFF);
+  }
+
+  // mBiosVendor and mBiosVersion, which are referenced in mBIOSInfoType0Strings,
+  // are left unchanged if the following calls fail.
+  UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVendor),
+    mBiosVendor, sizeof (mBiosVendor));
+  UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString),
+    mBiosVersion, sizeof (mBiosVersion));
+
   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL);
 }
 
@@ -664,34 +664,25 @@ SysInfoUpdateSmbiosType1 (
   UINT32 BoardRevision = 0;
   EFI_STATUS Status = EFI_SUCCESS;
   UINT64 BoardSerial = 0;
-  UINTN Prod = PROD_UNKNOWN;
-  UINTN Manu = MANU_UNKNOWN;
+  INTN Prod = -1;
+  INTN Manu = -1;
 
   Status = mFwProtocol->GetModelRevision (&BoardRevision);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "Failed to get board model: %r\n", Status));
   } else {
     Prod = (BoardRevision >> 4) & 0xFF;
+    Manu = (BoardRevision >> 16) & 0x0F;
   }
 
-  if (Prod > PROD_KNOWN) {
-    Prod = PROD_UNKNOWN;
-  }
-  Prod -= PROD_BASE;
-  AsciiSPrint (mSysInfoProductName, sizeof (mSysInfoProductName),
-    "64-bit Raspberry Pi %a (rev. %x)", ProductNames[Prod], BoardRevision);
-
-  Manu = (BoardRevision >> 16) & 0xF;
-  if (Manu > MANU_KNOWN) {
-    Manu = MANU_UNKNOWN;
-  } else {
-    Manu += MANU_BASE;
-  }
-  AsciiSPrint (mSysInfoManufName, sizeof (mSysInfoManufName), "%a", ManufNames[Manu]);
+  AsciiStrCpyS (mSysInfoProductName, sizeof (mSysInfoProductName),
+    mFwProtocol->GetModelName (Prod));
+  AsciiStrCpyS (mSysInfoManufName, sizeof (mSysInfoManufName),
+    mFwProtocol->GetManufacturerName (Manu));
+  AsciiSPrint (mSysInfoVersionName, sizeof (mSysInfoVersionName),
+    "%X", BoardRevision);
 
-  I64ToHexString (mSysInfoSKU,
-    sizeof (mSysInfoSKU),
-    BoardRevision);
+  I64ToHexString (mSysInfoSKU, sizeof (mSysInfoSKU), BoardRevision);
 
   Status = mFwProtocol->GetSerial (&BoardSerial);
   if (EFI_ERROR (Status)) {
@@ -702,9 +693,11 @@ SysInfoUpdateSmbiosType1 (
 
   DEBUG ((DEBUG_ERROR, "Board Serial Number: %a\n", mSysInfoSerial));
 
-  mSysInfoType1.Uuid.Data1 = *(UINT32*)"RPi3";
+  mSysInfoType1.Uuid.Data1 = BoardRevision;
   mSysInfoType1.Uuid.Data2 = 0x0;
   mSysInfoType1.Uuid.Data3 = 0x0;
+  // Swap endianness, so that the serial is more user-friendly as a UUID
+  BoardSerial = SwapBytes64 (BoardSerial);
   CopyMem (mSysInfoType1.Uuid.Data4, &BoardSerial, sizeof (BoardSerial));
 
   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mSysInfoType1, mSysInfoType1Strings, NULL);
@@ -763,6 +756,8 @@ ProcessorInfoUpdateSmbiosType4 (
     DEBUG ((DEBUG_INFO, "Current CPU speed: %uHz\n", Rate));
   }
 
+  AsciiStrCpyS (mCpuName, sizeof (mCpuName), mFwProtocol->GetCpuName (-1));
+
   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mProcessorInfoType4, mProcessorInfoType4Strings, NULL);
 }
 
diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
index f7c74f7f5456..485450625b54 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
@@ -48,3 +48,5 @@ [Depex]
 
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemorySize
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
index b37a02e97da7..bc424a06bb45 100644
--- a/Platform/RaspberryPi/RPi3/RPi3.dsc
+++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
@@ -309,7 +309,7 @@ [PcdsFixedAtBuild.common]
 
   gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0xc0000000
 
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"edk2-1.0"
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"EDK2-DEV"
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   # override the default values from SecurityPkg to ensure images from all sources are verified in secure boot
@@ -383,7 +383,7 @@ [PcdsFixedAtBuild.common]
   gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
 
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"Raspberry Pi 3 64-bit UEFI"
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor|L"EDK2"
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
 
 [PcdsDynamicHii.common.DEFAULT]
-- 
2.21.0.windows.1

[edk2-platforms][PATCH v3 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD

[Pete Batard]

String parsing code is added to BIOSInfoUpdateSmbiosType0() so that
any numeric "x.y" value being passed in PcdFirmwareVersionString is
now used to populate the BIOS major and minor.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 45 ++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index 9150dcfd8c5b..b5dcff897a59 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -41,6 +41,8 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/PrintLib.h>
 
+#define SMB_IS_DIGIT(c)  (((c) >= '0') && ((c) <= '9'))
+
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
 
 /***********************************************************************
@@ -597,6 +599,9 @@ BIOSInfoUpdateSmbiosType0 (
 {
   UINT32 FirmwareRevision = 0;
   EFI_STATUS Status = EFI_SUCCESS;
+  INTN   i;
+  INTN   State = 0;
+  INTN   Value[2];
 
   // Populate the Firmware major and minor.
   Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
@@ -618,6 +623,46 @@ BIOSInfoUpdateSmbiosType0 (
   UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString),
     mBiosVersion, sizeof (mBiosVersion));
 
+  // Look for a "x.y" numeric string anywhere in mBiosVersion and
+  // try to parse it to populate the BIOS major and minor.
+  for (i = 0; (i < AsciiStrLen (mBiosVersion)) && (State < 4); i++) {
+    switch (State) {
+    case 0:
+      if (!SMB_IS_DIGIT (mBiosVersion[i]))
+        break;
+      Value[0] = Value[1] = 0;
+      State++;
+      // Fall through
+    case 1:
+    case 3:
+      if (SMB_IS_DIGIT (mBiosVersion[i])) {
+        Value[State / 2] = (Value[State / 2] * 10) + (mBiosVersion[i] - '0');
+        if (Value[State / 2] > 255) {
+          while (SMB_IS_DIGIT (mBiosVersion[i + 1]))
+            i++;
+          // Reset our state (we may have something like "Firmware X83737.1 v1.23")
+          State = 0;
+        }
+      } else {
+        State++;
+      }
+      if (State != 2)
+        break;
+      // Fall through
+    case 2:
+      if ((mBiosVersion[i] == '.') && (SMB_IS_DIGIT (mBiosVersion[i + 1]))) {
+        State++;
+      } else {
+        State = 0;
+      }
+      break;
+    }
+  }
+  if ((State == 3) || (State == 4)) {
+    mBIOSInfoType0.SystemBiosMajorRelease = (UINT8)Value[0];
+    mBIOSInfoType0.SystemBiosMinorRelease = (UINT8)Value[1];
+  }
+
   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL);
 }
 
-- 
2.21.0.windows.1

[edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Pete Batard]

The board revision is the proper channel to use to detect the amount of
RAM available as bits [20-22] report the effective RAM size for the board
starting with 256 MB (000b) and doubling in size for each value.

Signed-off-by: Pete Batard <pete@akeo.ie>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index b5dcff897a59..5abc82b8d363 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
   )
 {
   EFI_STATUS Status;
-  UINT32 Base;
-  UINT32 Size;
+  UINT32 BoardRevision = 0;
 
-  Status = mFwProtocol->GetArmMem (&Base, &Size);
+  // Note: Type 19 addresses are expressed in KB, not bytes
+  mMemArrMapInfoType19.StartingAddress = 0;
+  // The minimum RAM size used on any Raspberry Pi model is 256 MB
+  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
+  Status = mFwProtocol->GetModelRevision (&BoardRevision);
   if (Status != EFI_SUCCESS) {
-    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", Status));
+    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - defaulting to 256 MB: %r\n", Status));
   } else {
-    mMemArrMapInfoType19.StartingAddress = Base / 1024;
-    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
+    // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
+    // Bits [20-22] indicate the amount of memory starting with 256MB (000b)
+    // and doubling in size for each value (001b = 512 MB, 010b = 1GB, etc.)
+    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 0x07;
   }
+  mMemArrMapInfoType19.EndingAddress -= 1;
 
   LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, mMemArrMapInfoType19Strings, NULL);
 }
-- 
2.21.0.windows.1

Re: [edk2-devel] [edk2-platforms][PATCH v3 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions

[Philippe Mathieu-Daudé]

On 10/11/19 1:07 PM, Pete Batard wrote:
> This patch introduces the capability to also query the Model Name/
> Manufacturer Name/CPU Name/Firmware Revision using the RpiFirmware
> protocol. This is aims at making the driver more suitable to cater
> for platforms other than the Raspberry Pi 3 as well as simplifying
> the population of entries in PlatformSmbiosDxe.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>   Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 158 +++++++++++++++++++-
>   Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h          |  58 +++++--
>   2 files changed, 199 insertions(+), 17 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> index 9b5ee1946279..25d7fa3974c0 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> @@ -461,7 +461,7 @@ typedef struct {
>     RPI_FW_TAG_HEAD           TagHead;
>     RPI_FW_MODEL_REVISION_TAG TagBody;
>     UINT32                    EndTag;
> -} RPI_FW_GET_MODEL_REVISION_CMD;
> +} RPI_FW_GET_REVISION_CMD;
>   #pragma pack()
>   
>   STATIC
> @@ -471,7 +471,7 @@ RpiFirmwareGetModelRevision (
>     OUT   UINT32 *Revision
>     )
>   {
> -  RPI_FW_GET_MODEL_REVISION_CMD *Cmd;
> +  RPI_FW_GET_REVISION_CMD       *Cmd;
>     EFI_STATUS                    Status;
>     UINT32                        Result;
>   
> @@ -506,6 +506,156 @@ RpiFirmwareGetModelRevision (
>     return EFI_SUCCESS;
>   }
>   
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +RpiFirmwareGetFirmwareRevision (
> +  OUT   UINT32 *Revision
> +  )
> +{
> +  RPI_FW_GET_REVISION_CMD       *Cmd;
> +  EFI_STATUS                    Status;
> +  UINT32                        Result;
> +
> +  if (!AcquireSpinLockOrFail (&mMailboxLock)) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__));
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  Cmd = mDmaBuffer;
> +  ZeroMem (Cmd, sizeof (*Cmd));
> +
> +  Cmd->BufferHead.BufferSize  = sizeof (*Cmd);
> +  Cmd->BufferHead.Response    = 0;
> +  Cmd->TagHead.TagId          = RPI_MBOX_GET_REVISION;
> +  Cmd->TagHead.TagSize        = sizeof (Cmd->TagBody);
> +  Cmd->TagHead.TagValueSize   = 0;
> +  Cmd->EndTag                 = 0;
> +
> +  Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result);
> +
> +  ReleaseSpinLock (&mMailboxLock);
> +
> +  if (EFI_ERROR (Status) ||
> +      Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: mailbox transaction error: Status == %r, Response == 0x%x\n",
> +      __FUNCTION__, Status, Cmd->BufferHead.Response));
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  *Revision = Cmd->TagBody.Revision;
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +CHAR8*
> +EFIAPI
> +RpiFirmwareGetModelName (
> +  IN INTN ModelId
> +  )
> +{
> +  UINT32  Revision;
> +
> +  // If a negative ModelId is passed, detect it.
> +  if ((ModelId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
> +    ModelId = (Revision >> 4) & 0xFF;
> +  }
> +
> +  switch (ModelId) {
> +  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> +  case 0x00:
> +    return "Raspberry Pi Model A";
> +  case 0x01:
> +    return "Raspberry Pi Model B";
> +  case 0x02:
> +    return "Raspberry Pi Model A+";
> +  case 0x03:
> +    return "Raspberry Pi Model B+";
> +  case 0x04:
> +    return "Raspberry Pi 2 Model B";
> +  case 0x06:
> +    return "Raspberry Pi Compute Module 1";
> +  case 0x08:
> +    return "Raspberry Pi 3 Model B";
> +  case 0x09:
> +    return "Raspberry Pi Zero";
> +  case 0x0A:
> +    return "Raspberry Pi Compute Module 3";
> +  case 0x0C:
> +    return "Raspberry Pi Zero W";
> +  case 0x0D:
> +    return "Raspberry Pi 3 Model B+";
> +  case 0x0E:
> +    return "Raspberry Pi 3 Model A+";
> +  case 0x11:
> +    return "Raspberry Pi 4 Model B";
> +  default:
> +    return "Unknown Raspberry Pi Model";
> +  }
> +}
> +
> +STATIC
> +CHAR8*
> +EFIAPI
> +RpiFirmwareGetManufacturerName (
> +  IN INTN ManufacturerId
> +  )
> +{
> +  UINT32  Revision;
> +
> +  // If a negative ModelId is passed, detect it.
> +  if ((ManufacturerId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
> +    ManufacturerId = (Revision >> 16) & 0x0F;
> +  }
> +
> +  switch (ManufacturerId) {
> +  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> +  case 0x00:
> +    return "Sony UK";
> +  case 0x01:
> +    return "Egoman";
> +  case 0x02:
> +  case 0x04:
> +    return "Embest";
> +  case 0x03:
> +    return "Sony Japan";
> +  case 0x05:
> +    return "Stadium";
> +  default:
> +    return "Unknown Manufacturer";
> +  }
> +}
> +
> +STATIC
> +CHAR8*
> +EFIAPI
> +RpiFirmwareGetCpuName (
> +  IN INTN CpuId
> +  )
> +{
> +  UINT32  Revision;
> +
> +  // If a negative CpuId is passed, detect it.
> +  if ((CpuId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) {
> +    CpuId = (Revision >> 12) & 0x0F;
> +  }
> +
> +  switch (CpuId) {
> +  // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> +  case 0x00:
> +    return "BCM2835 (ARM11)";
> +  case 0x01:
> +    return "BCM2836 (ARM Cortex-A7)";
> +  case 0x02:
> +    return "BCM2837 (ARM Cortex-A53)";
> +  case 0x03:
> +    return "BCM2711 (ARM Cortex-A72)";
> +  default:
> +    return "Unknown CPU Model";
> +  }
> +}
> +
>   #pragma pack()
>   typedef struct {
>     UINT32 Width;
> @@ -1009,6 +1159,10 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
>     RpiFirmwareGetSerial,
>     RpiFirmwareGetModel,
>     RpiFirmwareGetModelRevision,
> +  RpiFirmwareGetModelName,
> +  RpiFirmwareGetFirmwareRevision,
> +  RpiFirmwareGetManufacturerName,
> +  RpiFirmwareGetCpuName,
>     RpiFirmwareGetArmMemory
>   };
>   
> diff --git a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
> index ec70f28efe1a..e49d6e6132d9 100644
> --- a/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
> +++ b/Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h
> @@ -96,6 +96,30 @@ EFI_STATUS
>     UINT32 *Revision
>     );
>   
> +typedef
> +CHAR8*
> +(EFIAPI *GET_MODEL_NAME) (
> +  INTN ModelId
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *GET_FIRMWARE_REVISION) (
> +  UINT32 *Revision
> +  );
> +
> +typedef
> +CHAR8*
> +(EFIAPI *GET_MANUFACTURER_NAME) (
> +  INTN ManufacturerId
> +  );
> +
> +typedef
> +CHAR8*
> +(EFIAPI *GET_CPU_NAME) (
> +  INTN CpuId
> +  );
> +
>   typedef
>   EFI_STATUS
>   (EFIAPI *GET_ARM_MEM) (
> @@ -104,21 +128,25 @@ EFI_STATUS
>     );
>   
>   typedef struct {
> -  SET_POWER_STATE    SetPowerState;
> -  GET_MAC_ADDRESS    GetMacAddress;
> -  GET_COMMAND_LINE   GetCommandLine;
> -  GET_CLOCK_RATE     GetClockRate;
> -  GET_CLOCK_RATE     GetMaxClockRate;
> -  GET_CLOCK_RATE     GetMinClockRate;
> -  SET_CLOCK_RATE     SetClockRate;
> -  GET_FB             GetFB;
> -  FREE_FB            FreeFB;
> -  GET_FB_SIZE        GetFBSize;
> -  SET_LED            SetLed;
> -  GET_SERIAL         GetSerial;
> -  GET_MODEL          GetModel;
> -  GET_MODEL_REVISION GetModelRevision;
> -  GET_ARM_MEM        GetArmMem;
> +  SET_POWER_STATE       SetPowerState;
> +  GET_MAC_ADDRESS       GetMacAddress;
> +  GET_COMMAND_LINE      GetCommandLine;
> +  GET_CLOCK_RATE        GetClockRate;
> +  GET_CLOCK_RATE        GetMaxClockRate;
> +  GET_CLOCK_RATE        GetMinClockRate;
> +  SET_CLOCK_RATE        SetClockRate;
> +  GET_FB                GetFB;
> +  FREE_FB               FreeFB;
> +  GET_FB_SIZE           GetFBSize;
> +  SET_LED               SetLed;
> +  GET_SERIAL            GetSerial;
> +  GET_MODEL             GetModel;
> +  GET_MODEL_REVISION    GetModelRevision;
> +  GET_MODEL_NAME        GetModelName;
> +  GET_FIRMWARE_REVISION GetFirmwareRevision;
> +  GET_MANUFACTURER_NAME GetManufacturerName;
> +  GET_CPU_NAME          GetCpuName;
> +  GET_ARM_MEM           GetArmMem;
>   } RASPBERRY_PI_FIRMWARE_PROTOCOL;
>   
>   extern EFI_GUID gRaspberryPiFirmwareProtocolGuid;
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Philippe Mathieu-Daudé]

Hi Pete,

On 10/11/19 1:07 PM, Pete Batard wrote:
> The board revision is the proper channel to use to detect the amount of
> RAM available as bits [20-22] report the effective RAM size for the board
> starting with 256 MB (000b) and doubling in size for each value.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>   Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> index b5dcff897a59..5abc82b8d363 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>     )
>   {
>     EFI_STATUS Status;
> -  UINT32 Base;
> -  UINT32 Size;
> +  UINT32 BoardRevision = 0;
>   
> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
> +  // Note: Type 19 addresses are expressed in KB, not bytes
> +  mMemArrMapInfoType19.StartingAddress = 0;

Now you assume the ARM base RAM address is always 0, why?

> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>     if (Status != EFI_SUCCESS) {
> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", Status));
> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - defaulting to 256 MB: %r\n", Status));
>     } else {
> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
> +    // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> +    // Bits [20-22] indicate the amount of memory starting with 256MB (000b)
> +    // and doubling in size for each value (001b = 512 MB, 010b = 1GB, etc.)
> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 0x07;
>     }
> +  mMemArrMapInfoType19.EndingAddress -= 1;
>   
>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, mMemArrMapInfoType19Strings, NULL);
>   }
> 

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Pete Batard]

Hi Philippe,

On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
> Hi Pete,
> 
> On 10/11/19 1:07 PM, Pete Batard wrote:
>> The board revision is the proper channel to use to detect the amount of
>> RAM available as bits [20-22] report the effective RAM size for the board
>> starting with 256 MB (000b) and doubling in size for each value.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>> ---
>>   
>> Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 
>> 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git 
>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
>> index b5dcff897a59..5abc82b8d363 100644
>> --- 
>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
>> +++ 
>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
>> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>>     )
>>   {
>>     EFI_STATUS Status;
>> -  UINT32 Base;
>> -  UINT32 Size;
>> +  UINT32 BoardRevision = 0;
>> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
>> +  // Note: Type 19 addresses are expressed in KB, not bytes
>> +  mMemArrMapInfoType19.StartingAddress = 0;
> 
> Now you assume the ARM base RAM address is always 0, why?

Because, in the case that is of interest to us here (Broadcom SoCs used 
for the various Raspberry Pi platforms), this is what the documentation 
says.

If you look at something like 
https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf 
(which, as per the Pi Foundation, also applies to the later models) and 
especially the memory layout graphic that you see early in the document, 
you can find that the ARM base RAM address is indeed set to 0 always.

At this stage, we have no reason to think that we are going to contend 
with a model where the RAM base address isn't 0.

Regards,

/Pete

>> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
>> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
>> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>>     if (Status != EFI_SUCCESS) {
>> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", 
>> Status));
>> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - 
>> defaulting to 256 MB: %r\n", Status));
>>     } else {
>> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
>> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
>> +    // 
>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>
>> +    // Bits [20-22] indicate the amount of memory starting with 256MB 
>> (000b)
>> +    // and doubling in size for each value (001b = 512 MB, 010b = 
>> 1GB, etc.)
>> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 0x07;
>>     }
>> +  mMemArrMapInfoType19.EndingAddress -= 1;
>>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, 
>> mMemArrMapInfoType19Strings, NULL);
>>   }
>>
> 

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Philippe Mathieu-Daudé]

On 10/14/19 1:44 PM, Pete Batard wrote:
> Hi Philippe,
> 
> On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
>> Hi Pete,
>>
>> On 10/11/19 1:07 PM, Pete Batard wrote:
>>> The board revision is the proper channel to use to detect the amount of
>>> RAM available as bits [20-22] report the effective RAM size for the 
>>> board
>>> starting with 256 MB (000b) and doubling in size for each value.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>> Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>> | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git 
>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> index b5dcff897a59..5abc82b8d363 100644
>>> --- 
>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> +++ 
>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>
>>> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>>>     )
>>>   {
>>>     EFI_STATUS Status;
>>> -  UINT32 Base;
>>> -  UINT32 Size;
>>> +  UINT32 BoardRevision = 0;
>>> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
>>> +  // Note: Type 19 addresses are expressed in KB, not bytes
>>> +  mMemArrMapInfoType19.StartingAddress = 0;
>>
>> Now you assume the ARM base RAM address is always 0, why?
> 
> Because, in the case that is of interest to us here (Broadcom SoCs used 
> for the various Raspberry Pi platforms), this is what the documentation 
> says.
> 
> If you look at something like 
> https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf 
> (which, as per the Pi Foundation, also applies to the later models) and 
> especially the memory layout graphic that you see early in the document, 
> you can find that the ARM base RAM address is indeed set to 0 always.
> 
> At this stage, we have no reason to think that we are going to contend 
> with a model where the RAM base address isn't 0.

OK. I have no idea what are Broadcom plans, I read somewhere they were 
going to use tricks to allow more than 4GB of RAM on the Raspberry Pi 4, 
so I was wondering, since they provide a firmware API call to get the 
RAM base address.

Maybe you can add a comment that we expect all following boards to use 
RAM base at 0 (simply replying to this email, and eventually Leif would 
amend it previous to push).

Otherwise your patch looks OK.

Regards,

Phil.

>>> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
>>> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
>>> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>>>     if (Status != EFI_SUCCESS) {
>>> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", 
>>> Status));
>>> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - 
>>> defaulting to 256 MB: %r\n", Status));
>>>     } else {
>>> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
>>> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
>>> +    // 
>>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>>
>>> +    // Bits [20-22] indicate the amount of memory starting with 
>>> 256MB (000b)
>>> +    // and doubling in size for each value (001b = 512 MB, 010b = 
>>> 1GB, etc.)
>>> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 
>>> 0x07;
>>>     }
>>> +  mMemArrMapInfoType19.EndingAddress -= 1;
>>>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, 
>>> mMemArrMapInfoType19Strings, NULL);
>>>   }
>>>
>>
> 
> 
> 
> 

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Pete Batard]

On 2019.10.14 12:53, Philippe Mathieu-Daudé wrote:
> On 10/14/19 1:44 PM, Pete Batard wrote:
>> Hi Philippe,
>>
>> On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
>>> Hi Pete,
>>>
>>> On 10/11/19 1:07 PM, Pete Batard wrote:
>>>> The board revision is the proper channel to use to detect the amount of
>>>> RAM available as bits [20-22] report the effective RAM size for the 
>>>> board
>>>> starting with 256 MB (000b) and doubling in size for each value.
>>>>
>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>> ---
>>>> Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>> | 18 ++++++++++++------
>>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git 
>>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>
>>>> index b5dcff897a59..5abc82b8d363 100644
>>>> --- 
>>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>
>>>> +++ 
>>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>
>>>> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>>>>     )
>>>>   {
>>>>     EFI_STATUS Status;
>>>> -  UINT32 Base;
>>>> -  UINT32 Size;
>>>> +  UINT32 BoardRevision = 0;
>>>> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
>>>> +  // Note: Type 19 addresses are expressed in KB, not bytes

Comment to add:

// The memory layout used in all known Pi SoC's starts at 0

>>>> +  mMemArrMapInfoType19.StartingAddress = 0;
>>>
>>> Now you assume the ARM base RAM address is always 0, why?
>>
>> Because, in the case that is of interest to us here (Broadcom SoCs 
>> used for the various Raspberry Pi platforms), this is what the 
>> documentation says.
>>
>> If you look at something like 
>> https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf 
>> (which, as per the Pi Foundation, also applies to the later models) 
>> and especially the memory layout graphic that you see early in the 
>> document, you can find that the ARM base RAM address is indeed set to 
>> 0 always.
>>
>> At this stage, we have no reason to think that we are going to contend 
>> with a model where the RAM base address isn't 0.
> 
> OK. I have no idea what are Broadcom plans, I read somewhere they were 
> going to use tricks to allow more than 4GB of RAM on the Raspberry Pi 4, 
> so I was wondering, since they provide a firmware API call to get the 
> RAM base address.

Well the thing is, this patch comes straight from work that is being 
carried out to add support for the Pi 4 to edk2-platforms. So we pretty 
much already validated that 0 is what needs to be used for the Pi 4 as 
well...

> Maybe you can add a comment that we expect all following boards to use 
> RAM base at 0 (simply replying to this email, and eventually Leif would 
> amend it previous to push).

Sure. Comment added above.

Regards,

/Pete

> 
> Otherwise your patch looks OK.
> 
> Regards,
> 
> Phil.
> 
>>>> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
>>>> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
>>>> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>>>>     if (Status != EFI_SUCCESS) {
>>>> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", 
>>>> Status));
>>>> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - 
>>>> defaulting to 256 MB: %r\n", Status));
>>>>     } else {
>>>> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
>>>> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
>>>> +    // 
>>>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>>>
>>>> +    // Bits [20-22] indicate the amount of memory starting with 
>>>> 256MB (000b)
>>>> +    // and doubling in size for each value (001b = 512 MB, 010b = 
>>>> 1GB, etc.)
>>>> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 
>>>> 0x07;
>>>>     }
>>>> +  mMemArrMapInfoType19.EndingAddress -= 1;
>>>>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, 
>>>> mMemArrMapInfoType19Strings, NULL);
>>>>   }
>>>>
>>>
>>
>>
>> 
>>
> 

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Philippe Mathieu-Daudé]

On 10/14/19 2:03 PM, Pete Batard wrote:
> On 2019.10.14 12:53, Philippe Mathieu-Daudé wrote:
>> On 10/14/19 1:44 PM, Pete Batard wrote:
>>> Hi Philippe,
>>>
>>> On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
>>>> Hi Pete,
>>>>
>>>> On 10/11/19 1:07 PM, Pete Batard wrote:
>>>>> The board revision is the proper channel to use to detect the 
>>>>> amount of
>>>>> RAM available as bits [20-22] report the effective RAM size for the 
>>>>> board
>>>>> starting with 256 MB (000b) and doubling in size for each value.
>>>>>
>>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>>> ---
>>>>> Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>> | 18 ++++++++++++------
>>>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git 
>>>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>>
>>>>> index b5dcff897a59..5abc82b8d363 100644
>>>>> --- 
>>>>> a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>>
>>>>> +++ 
>>>>> b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c 
>>>>>
>>>>> @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
>>>>>     )
>>>>>   {
>>>>>     EFI_STATUS Status;
>>>>> -  UINT32 Base;
>>>>> -  UINT32 Size;
>>>>> +  UINT32 BoardRevision = 0;
>>>>> -  Status = mFwProtocol->GetArmMem (&Base, &Size);
>>>>> +  // Note: Type 19 addresses are expressed in KB, not bytes
> 
> Comment to add:
> 
> // The memory layout used in all known Pi SoC's starts at 0

Thanks, Leif do you mind amending this comment?

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

>>>>> +  mMemArrMapInfoType19.StartingAddress = 0;
>>>>
>>>> Now you assume the ARM base RAM address is always 0, why?
>>>
>>> Because, in the case that is of interest to us here (Broadcom SoCs 
>>> used for the various Raspberry Pi platforms), this is what the 
>>> documentation says.
>>>
>>> If you look at something like 
>>> https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf 
>>> (which, as per the Pi Foundation, also applies to the later models) 
>>> and especially the memory layout graphic that you see early in the 
>>> document, you can find that the ARM base RAM address is indeed set to 
>>> 0 always.
>>>
>>> At this stage, we have no reason to think that we are going to 
>>> contend with a model where the RAM base address isn't 0.
>>
>> OK. I have no idea what are Broadcom plans, I read somewhere they were 
>> going to use tricks to allow more than 4GB of RAM on the Raspberry Pi 
>> 4, so I was wondering, since they provide a firmware API call to get 
>> the RAM base address.
> 
> Well the thing is, this patch comes straight from work that is being 
> carried out to add support for the Pi 4 to edk2-platforms. So we pretty 
> much already validated that 0 is what needs to be used for the Pi 4 as 
> well...

OK, good news :)

>> Maybe you can add a comment that we expect all following boards to use 
>> RAM base at 0 (simply replying to this email, and eventually Leif 
>> would amend it previous to push).
> 
> Sure. Comment added above.

Thanks!

> Regards,
> 
> /Pete
> 
>>
>> Otherwise your patch looks OK.
>>
>> Regards,
>>
>> Phil.
>>
>>>>> +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
>>>>> +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
>>>>> +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
>>>>>     if (Status != EFI_SUCCESS) {
>>>>> -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory size: %r\n", 
>>>>> Status));
>>>>> +    DEBUG ((DEBUG_WARNING, "Couldn't get the board memory size - 
>>>>> defaulting to 256 MB: %r\n", Status));
>>>>>     } else {
>>>>> -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
>>>>> -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
>>>>> +    // 
>>>>> www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md 
>>>>>
>>>>> +    // Bits [20-22] indicate the amount of memory starting with 
>>>>> 256MB (000b)
>>>>> +    // and doubling in size for each value (001b = 512 MB, 010b = 
>>>>> 1GB, etc.)
>>>>> +    mMemArrMapInfoType19.EndingAddress <<= (BoardRevision >> 20) & 
>>>>> 0x07;
>>>>>     }
>>>>> +  mMemArrMapInfoType19.EndingAddress -= 1;
>>>>>     LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19, 
>>>>> mMemArrMapInfoType19Strings, NULL);
>>>>>   }
>>>>>
>>>>
>>>
>>>
>>> 
>>>
>>
> 

Re: [edk2-devel] [edk2-platforms][PATCH v3 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision

[Leif Lindholm]

On Mon, Oct 14, 2019 at 02:08:10PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/14/19 2:03 PM, Pete Batard wrote:
> > On 2019.10.14 12:53, Philippe Mathieu-Daudé wrote:
> > > On 10/14/19 1:44 PM, Pete Batard wrote:
> > > > Hi Philippe,
> > > > 
> > > > On 2019.10.14 11:01, Philippe Mathieu-Daudé wrote:
> > > > > Hi Pete,
> > > > > 
> > > > > On 10/11/19 1:07 PM, Pete Batard wrote:
> > > > > > The board revision is the proper channel to use to
> > > > > > detect the amount of
> > > > > > RAM available as bits [20-22] report the effective RAM
> > > > > > size for the board
> > > > > > starting with 256 MB (000b) and doubling in size for each value.
> > > > > > 
> > > > > > Signed-off-by: Pete Batard <pete@akeo.ie>
> > > > > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > > ---
> > > > > > Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> > > > > > | 18 ++++++++++++------
> > > > > >   1 file changed, 12 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> > > > > > 
> > > > > > index b5dcff897a59..5abc82b8d363 100644
> > > > > > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> > > > > > 
> > > > > > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> > > > > > 
> > > > > > @@ -866,16 +866,22 @@ MemArrMapInfoUpdateSmbiosType19 (
> > > > > >     )
> > > > > >   {
> > > > > >     EFI_STATUS Status;
> > > > > > -  UINT32 Base;
> > > > > > -  UINT32 Size;
> > > > > > +  UINT32 BoardRevision = 0;
> > > > > > -  Status = mFwProtocol->GetArmMem (&Base, &Size);
> > > > > > +  // Note: Type 19 addresses are expressed in KB, not bytes
> > 
> > Comment to add:
> > 
> > // The memory layout used in all known Pi SoC's starts at 0
> 
> Thanks, Leif do you mind amending this comment?

No issue with that.
Have the rest of today off, but wil get back to this set tomorrow
morning.

/
    Leif

> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
> > > > > > +  mMemArrMapInfoType19.StartingAddress = 0;
> > > > > 
> > > > > Now you assume the ARM base RAM address is always 0, why?
> > > > 
> > > > Because, in the case that is of interest to us here (Broadcom
> > > > SoCs used for the various Raspberry Pi platforms), this is what
> > > > the documentation says.
> > > > 
> > > > If you look at something like https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> > > > (which, as per the Pi Foundation, also applies to the later
> > > > models) and especially the memory layout graphic that you see
> > > > early in the document, you can find that the ARM base RAM
> > > > address is indeed set to 0 always.
> > > > 
> > > > At this stage, we have no reason to think that we are going to
> > > > contend with a model where the RAM base address isn't 0.
> > > 
> > > OK. I have no idea what are Broadcom plans, I read somewhere they
> > > were going to use tricks to allow more than 4GB of RAM on the
> > > Raspberry Pi 4, so I was wondering, since they provide a firmware
> > > API call to get the RAM base address.
> > 
> > Well the thing is, this patch comes straight from work that is being
> > carried out to add support for the Pi 4 to edk2-platforms. So we pretty
> > much already validated that 0 is what needs to be used for the Pi 4 as
> > well...
> 
> OK, good news :)
> 
> > > Maybe you can add a comment that we expect all following boards to
> > > use RAM base at 0 (simply replying to this email, and eventually
> > > Leif would amend it previous to push).
> > 
> > Sure. Comment added above.
> 
> Thanks!
> 
> > Regards,
> > 
> > /Pete
> > 
> > > 
> > > Otherwise your patch looks OK.
> > > 
> > > Regards,
> > > 
> > > Phil.
> > > 
> > > > > > +  // The minimum RAM size used on any Raspberry Pi model is 256 MB
> > > > > > +  mMemArrMapInfoType19.EndingAddress = 256 * 1024;
> > > > > > +  Status = mFwProtocol->GetModelRevision (&BoardRevision);
> > > > > >     if (Status != EFI_SUCCESS) {
> > > > > > -    DEBUG ((DEBUG_ERROR, "Couldn't get the ARM memory
> > > > > > size: %r\n", Status));
> > > > > > +    DEBUG ((DEBUG_WARNING, "Couldn't get the board
> > > > > > memory size - defaulting to 256 MB: %r\n", Status));
> > > > > >     } else {
> > > > > > -    mMemArrMapInfoType19.StartingAddress = Base / 1024;
> > > > > > -    mMemArrMapInfoType19.EndingAddress = (Base + Size - 1) / 1024;
> > > > > > +    // www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> > > > > > 
> > > > > > +    // Bits [20-22] indicate the amount of memory
> > > > > > starting with 256MB (000b)
> > > > > > +    // and doubling in size for each value (001b = 512
> > > > > > MB, 010b = 1GB, etc.)
> > > > > > +    mMemArrMapInfoType19.EndingAddress <<=
> > > > > > (BoardRevision >> 20) & 0x07;
> > > > > >     }
> > > > > > +  mMemArrMapInfoType19.EndingAddress -= 1;
> > > > > >     LogSmbiosData
> > > > > > ((EFI_SMBIOS_TABLE_HEADER*)&mMemArrMapInfoType19,
> > > > > > mMemArrMapInfoType19Strings, NULL);
> > > > > >   }
> > > > > > 
> > > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > 
> 

Re: [edk2-platforms][PATCH v3 0/5] Various SMBIOS improvements

[Leif Lindholm]

On Fri, Oct 11, 2019 at 12:07:41PM +0100, Pete Batard wrote:
> Changes from v2:
> - Consistent use of curly brackets for if statements
> - Remove unused variables that may produce an issue with git bisect
> - Use gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor to populate vendor
> 
> Note that 5/5, which has been R-b, is unchanged.

For the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 48a95de81ede..2a4e369d588e.

Thanks!

/
    Leif

> Pete Batard (5):
>   Platform/RPi3/RpiFirmwareDxe: Add more query functions
>   Platform/RPi3/RpiFirmwareDxe: Improve serial number population
>   Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries
>   Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD
>   Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision
> 
>  Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 198 ++++++++++++--------
>  Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |   2 +
>  Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c         | 167 ++++++++++++++++-
>  Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h                  |  58 ++++--
>  Platform/RaspberryPi/RPi3/RPi3.dsc                                        |   4 +-
>  5 files changed, 333 insertions(+), 96 deletions(-)
> 
> -- 
> 2.21.0.windows.1
>