* [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements
@ 2019-10-08 12:38 Pete Batard
2019-10-08 12:38 ` [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Pete Batard
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw)
To: devel; +Cc: ard.biesheuvel, leif.lindholm
v1 can be found at https://edk2.groups.io/g/devel/message/46806 - 46808
Changes for v2:
- Split *functional* changes into multipe commits
- Spell out serial number
- *NEW* Derive Type 19 addresses from board revision (RPi4 improvement)
Also, with regards to further splitting that was requested and while I
appreciate that I already got some leniency in that respect, I must state
that my time is limited, therefore, unless I suspect a typo or style fix,
that got corrected in a "while I'm here" fashion, to result in a potential
CVE or make people browsing the codebase genuinely puzzled, I'm afraid that
I will continue to shove those as Also's into the commits I submit, so that
I can at least try to reclaim some valuable time...
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 | 211 ++++++++++++--------
Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 1 +
Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 166 ++++++++++++++-
Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++--
Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +-
5 files changed, 334 insertions(+), 104 deletions(-)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard @ 2019-10-08 12:38 ` Pete Batard 2019-10-10 8:39 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population Pete Batard ` (3 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm 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 | 155 +++++++++++++++++++- Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- 2 files changed, 196 insertions(+), 17 deletions(-) diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c index 9b5ee1946279..378c99bcba05 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,153 @@ 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 +1156,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Pete Batard @ 2019-10-10 8:39 ` Leif Lindholm 2019-10-10 12:41 ` Pete Batard 0 siblings, 1 reply; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 8:39 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Tue, Oct 08, 2019 at 01:38:37PM +0100, 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 | 155 +++++++++++++++++++- > Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- > 2 files changed, 196 insertions(+), 17 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > index 9b5ee1946279..378c99bcba05 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,153 @@ 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)) Style-wise, please always use {} with if-statements. Beyond that, this pattern repeats identcally in three functions in this file. Meanwhile, there is never any error handling of RpiFirmwareGetModelRevision other than if not successful, we leave it as negative. Are there other intended uses of that function, or could we move this logic there? / Leif > + 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 +1156,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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions 2019-10-10 8:39 ` Leif Lindholm @ 2019-10-10 12:41 ` Pete Batard 2019-10-10 16:43 ` Leif Lindholm 0 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-10 12:41 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel Hi Leif, On 2019.10.10 09:39, Leif Lindholm wrote: > On Tue, Oct 08, 2019 at 01:38:37PM +0100, 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 | 155 +++++++++++++++++++- >> Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- >> 2 files changed, 196 insertions(+), 17 deletions(-) >> >> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> index 9b5ee1946279..378c99bcba05 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,153 @@ 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)) > > Style-wise, please always use {} with if-statements. Will do. > Beyond that, this pattern repeats identcally in three functions in > this file. Meanwhile, there is never any error handling of > RpiFirmwareGetModelRevision other than if not successful, we leave it > as negative. Yes, because then we'll get the default case (that returns "Unknown ..."), which is precisely what we want if we can't access the model revision for any reason. > Are there other intended uses of that function, or could > we move this logic there? I'm not sure how that would work, since ModelId, ManufacturerId, and CpuId are for different parts of the bit fields, so, if I understand what you're suggesting correctly (but I'm not really sure I do) trying to move the check for negative value into RpiFirmwareGetModelRevision () wouldn't work that great. Plus then the function name would become a misnommer. Are you really seeing a functional issue with the current code, or something that would make a possible contributor wanting to modify this section puzzled as to what we're trying to accomplish here (which I think is pretty clear, but then again, I wrote the code so maybe I'm not the most impartial? Because if not, I'd rather save my limited supply of time, and just go with a v3 that only adds the {} suggested. Regards, /Pete > > / > Leif > >> + 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 +1156,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 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions 2019-10-10 12:41 ` Pete Batard @ 2019-10-10 16:43 ` Leif Lindholm 2019-10-10 17:27 ` Pete Batard 0 siblings, 1 reply; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 16:43 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Thu, Oct 10, 2019 at 01:41:06PM +0100, Pete Batard wrote: > Hi Leif, > > On 2019.10.10 09:39, Leif Lindholm wrote: > > On Tue, Oct 08, 2019 at 01:38:37PM +0100, 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 | 155 +++++++++++++++++++- > > > Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- > > > 2 files changed, 196 insertions(+), 17 deletions(-) > > > > > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > index 9b5ee1946279..378c99bcba05 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,153 @@ 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)) > > > > Style-wise, please always use {} with if-statements. > > Will do. > > > Beyond that, this pattern repeats identcally in three functions in > > this file. Meanwhile, there is never any error handling of > > RpiFirmwareGetModelRevision other than if not successful, we leave it > > as negative. > > Yes, because then we'll get the default case (that returns "Unknown ..."), > which is precisely what we want if we can't access the model revision for > any reason. > > > Are there other intended uses of that function, or could > > we move this logic there? > > I'm not sure how that would work, since ModelId, ManufacturerId, and CpuId > are for different parts of the bit fields, so, if I understand what you're > suggesting correctly (but I'm not really sure I do) trying to move the check > for negative value into RpiFirmwareGetModelRevision () wouldn't work that > great. Plus then the function name would become a misnommer. > > Are you really seeing a functional issue with the current code, or something > that would make a possible contributor wanting to modify this section > puzzled as to what we're trying to accomplish here (which I think is pretty > clear, but then again, I wrote the code so maybe I'm not the most impartial? > Because if not, I'd rather save my limited supply of time, and just go with > a v3 that only adds the {} suggested. I see no functional issues with the code, but with my own limited supply of time I try to ensure the code stays as simple as possible so I don't need to stop and think where there isn't actually anything complicated going on (like here). I am after all a bear of very little brain. I also will *always* trigger on seeing a near-identical pattern repeated many times in the same file. In this instance, I find myself spending way too much time untangling what is actually going on. I see - SysInfoUpdateSmbiosType1 () calling mFwProtocol->GetModelRevision (), and then using the result from that to determine which parameters to pass to mFwProtocol->GetModelName() and mFwProtocol->GetManufacturerName(). - Both of which if the GetModelRevision () call was unsuccessful get passed -1. - Now, when the parameter is -1, they both call straight into GetModelRevision (). Why are we expecting the second call to succeed when the first one fails? Am I completely misreading what this code does? Does GetModelRevision() return different values depending on when you call it? Best Regards, Leif ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions 2019-10-10 16:43 ` Leif Lindholm @ 2019-10-10 17:27 ` Pete Batard 0 siblings, 0 replies; 17+ messages in thread From: Pete Batard @ 2019-10-10 17:27 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel On 2019.10.10 17:43, Leif Lindholm wrote: > On Thu, Oct 10, 2019 at 01:41:06PM +0100, Pete Batard wrote: >> Hi Leif, >> >> On 2019.10.10 09:39, Leif Lindholm wrote: >>> On Tue, Oct 08, 2019 at 01:38:37PM +0100, 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 | 155 +++++++++++++++++++- >>>> Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- >>>> 2 files changed, 196 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> index 9b5ee1946279..378c99bcba05 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,153 @@ 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)) >>> >>> Style-wise, please always use {} with if-statements. >> >> Will do. >> >>> Beyond that, this pattern repeats identcally in three functions in >>> this file. Meanwhile, there is never any error handling of >>> RpiFirmwareGetModelRevision other than if not successful, we leave it >>> as negative. >> >> Yes, because then we'll get the default case (that returns "Unknown ..."), >> which is precisely what we want if we can't access the model revision for >> any reason. >> >>> Are there other intended uses of that function, or could >>> we move this logic there? >> >> I'm not sure how that would work, since ModelId, ManufacturerId, and CpuId >> are for different parts of the bit fields, so, if I understand what you're >> suggesting correctly (but I'm not really sure I do) trying to move the check >> for negative value into RpiFirmwareGetModelRevision () wouldn't work that >> great. Plus then the function name would become a misnommer. >> >> Are you really seeing a functional issue with the current code, or something >> that would make a possible contributor wanting to modify this section >> puzzled as to what we're trying to accomplish here (which I think is pretty >> clear, but then again, I wrote the code so maybe I'm not the most impartial? >> Because if not, I'd rather save my limited supply of time, and just go with >> a v3 that only adds the {} suggested. > > I see no functional issues with the code, but with my own limited > supply of time I try to ensure the code stays as simple as possible so > I don't need to stop and think where there isn't actually anything > complicated going on (like here). I am after all a bear of very little > brain. > > I also will *always* trigger on seeing a near-identical pattern > repeated many times in the same file. > > In this instance, I find myself spending way too much time untangling > what is actually going on. I see > - SysInfoUpdateSmbiosType1 () calling mFwProtocol->GetModelRevision (), > and then using the result from that to determine which parameters to > pass to mFwProtocol->GetModelName() and > mFwProtocol->GetManufacturerName(). > - Both of which if the GetModelRevision () call was unsuccessful get > passed -1. The whole point is to create new generic calls that may be used by any driver that needs them, and not only PlatformSmbiosDxe. That's our starting point. And the idea is that, unlike what currently happens in SysInfoUpdateSmbiosType1 (), not all of these calls may also happen to be calling GetModelRevision (). As such, I believe it makes a lot of sense to have the new calls designed as they are, else we find ourselves trying to cover the case of minimizing calls to GetModelRevision (), either in PlatformSmbiosDxe or RpiFirmwareDxe, which seems an unwarranted micro optimization goal to me. > - Now, when the parameter is -1, they both call straight into > GetModelRevision (). Why are we expecting the second call to succeed > when the first one fails? We're not expecting anything. We're receiving a call from a driver, of which we have no idea what actions it performed beforehand, that may happens to ask us to replicate some of these actions. And, in a generic manner, and regardless of a state we should not have any idea about due to encapsulation, we do issue a call to GetModelRevision (). Or, let me put it this way: From PlatformSmbiosDxe:SysInfoUpdateSmbiosType1 ()'s perspective, the call to RpiFirmwareDxe:GetModelName () is supposed to be a black box that returns a result it can use. And from RpiFirmwareDxe:GetModelName ()'s perspective, what potentially duplicated actions the caller did beforehand is also a black box. Ideally, this is intended to make both drivers, with their better abstracted goals, easier to understand and maintain, so I can't help but feel quite dismayed that it's the very opposite that appears to be occurring here. Still, if anything, I would assert that any action that aims at reducing calls to GetModelRevision () should be reported against PlatformSmbiosDxe rather than the lower level (and therefore more generic) RpiFirmwareDxe. Regards, /Pete > Am I completely misreading what this code does? > > Does GetModelRevision() return different values depending on when you > call it? > > Best Regards, > > Leif > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Pete Batard @ 2019-10-08 12:38 ` Pete Batard 2019-10-10 8:43 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries Pete Batard ` (2 subsequent siblings) 4 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm Improve RpiFirmwareGetSerial() to derive a serial number from the MAC address, in case the platform returns 0 for the serial number. Also fix a typo where "%s" was used instead of "%a". Signed-off-by: Pete Batard <pete@akeo.ie> --- Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c index 378c99bcba05..c2344252d2c0 100644 --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c @@ -219,7 +219,7 @@ RpiFirmwareSetPowerState ( if (!EFI_ERROR (Status) && PowerState ^ (Cmd->TagBody.PowerState & RPI_MBOX_POWER_STATE_ENABLE)) { - DEBUG ((DEBUG_ERROR, "%a: failed to %sable power for device %d\n", + DEBUG ((DEBUG_ERROR, "%a: failed to %aable power for device %d\n", __FUNCTION__, PowerState ? "en" : "dis", DeviceId)); Status = EFI_DEVICE_ERROR; } @@ -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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population Pete Batard @ 2019-10-10 8:43 ` Leif Lindholm 2019-10-10 12:41 ` Pete Batard 0 siblings, 1 reply; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 8:43 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Tue, Oct 08, 2019 at 01:38:38PM +0100, Pete Batard wrote: > Improve RpiFirmwareGetSerial() to derive a serial number from the > MAC address, in case the platform returns 0 for the serial number. > > Also fix a typo where "%s" was used instead of "%a". I did not see a reply to my feedback on the previous round, so I'm unsure if this is a mistake or a disagreement? > Signed-off-by: Pete Batard <pete@akeo.ie> > --- > Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > index 378c99bcba05..c2344252d2c0 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > @@ -219,7 +219,7 @@ RpiFirmwareSetPowerState ( > > if (!EFI_ERROR (Status) && > PowerState ^ (Cmd->TagBody.PowerState & RPI_MBOX_POWER_STATE_ENABLE)) { > - DEBUG ((DEBUG_ERROR, "%a: failed to %sable power for device %d\n", > + DEBUG ((DEBUG_ERROR, "%a: failed to %aable power for device %d\n", This is clearly a bugfix, but I don't see it as being part of "Improve serial number population", which is what it claims to be if I view it wih git blame. / Leif > __FUNCTION__, PowerState ? "en" : "dis", DeviceId)); > Status = EFI_DEVICE_ERROR; > } > @@ -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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population 2019-10-10 8:43 ` Leif Lindholm @ 2019-10-10 12:41 ` Pete Batard 2019-10-10 16:49 ` Leif Lindholm 0 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-10 12:41 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel Hi Leif, It's a disagreement. And the same goes for 3/5 & 4/5. Please see the note I wrote in 0/5 for the v2, because the cover letter this is usually the place I try to clarify elements that may throw off a maintainer, and that don't belong in a commit message. Not so sound flippant here, but as long as there isn't an MTV award for "Most atomic codebase ever", I just don't have the time to split what I consider to be frivolous commits. The reasoning behind that is that I realistically don't consider that people are actually going to be thrown off by a "while I was here I also fixed an obvious typo" that got added into an existing commit or, most important, that even if they do, the amount of time that is going to be collectively wasted by people who might be thrown of by not having uber atomicity is not going to exceed the amount of time it will cost *me* to split it. Therefore, while I do understand the desire to have an atomic commit history, I'm afraid that if we can't strike a balance between how much extra time contributors are expected to waste vs how atomic a *real-life* codebase is enforced to be, if I have to split every little typo and stylistic fix into yet another commit, I'm simply not going to bother fixing typos or low hanging fruits I see any more. Regards, /Pete On 2019.10.10 09:43, Leif Lindholm wrote: > On Tue, Oct 08, 2019 at 01:38:38PM +0100, Pete Batard wrote: >> Improve RpiFirmwareGetSerial() to derive a serial number from the >> MAC address, in case the platform returns 0 for the serial number. >> >> Also fix a typo where "%s" was used instead of "%a". > > I did not see a reply to my feedback on the previous round, so I'm > unsure if this is a mistake or a disagreement? > >> Signed-off-by: Pete Batard <pete@akeo.ie> >> --- >> Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> index 378c99bcba05..c2344252d2c0 100644 >> --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> @@ -219,7 +219,7 @@ RpiFirmwareSetPowerState ( >> >> if (!EFI_ERROR (Status) && >> PowerState ^ (Cmd->TagBody.PowerState & RPI_MBOX_POWER_STATE_ENABLE)) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to %sable power for device %d\n", >> + DEBUG ((DEBUG_ERROR, "%a: failed to %aable power for device %d\n", > > This is clearly a bugfix, but I don't see it as being part of "Improve > serial number population", which is what it claims to be if I view it > wih git blame. > > / > Leif > >> __FUNCTION__, PowerState ? "en" : "dis", DeviceId)); >> Status = EFI_DEVICE_ERROR; >> } >> @@ -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 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population 2019-10-10 12:41 ` Pete Batard @ 2019-10-10 16:49 ` Leif Lindholm 2019-10-10 17:28 ` Pete Batard 0 siblings, 1 reply; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 16:49 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Thu, Oct 10, 2019 at 01:41:20PM +0100, Pete Batard wrote: > It's a disagreement. And the same goes for 3/5 & 4/5. Please see the note I > wrote in 0/5 for the v2, because the cover letter this is usually the place > I try to clarify elements that may throw off a maintainer, and that don't > belong in a commit message. > > Not so sound flippant here, but as long as there isn't an MTV award for > "Most atomic codebase ever", I just don't have the time to split what I > consider to be frivolous commits. The reasoning behind that is that I > realistically don't consider that people are actually going to be thrown off > by a "while I was here I also fixed an obvious typo" that got added into an > existing commit or, most important, that even if they do, the amount of time > that is going to be collectively wasted by people who might be thrown of by > not having uber atomicity is not going to exceed the amount of time it will > cost *me* to split it. > > Therefore, while I do understand the desire to have an atomic commit > history, I'm afraid that if we can't strike a balance between how much extra > time contributors are expected to waste vs how atomic a *real-life* > codebase is enforced to be, if I have to split every little typo and > stylistic fix into yet another commit, I'm simply not going to bother fixing > typos or low hanging fruits I see any more. I understand, we all have a limited supply of time. Since I don't wish to start implementing different rules for different contributors, could I ask you to stop contributing typo and low hanging fruit fixes? Best Regards, Leif ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population 2019-10-10 16:49 ` Leif Lindholm @ 2019-10-10 17:28 ` Pete Batard 0 siblings, 0 replies; 17+ messages in thread From: Pete Batard @ 2019-10-10 17:28 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel On 2019.10.10 17:49, Leif Lindholm wrote: > On Thu, Oct 10, 2019 at 01:41:20PM +0100, Pete Batard wrote: >> It's a disagreement. And the same goes for 3/5 & 4/5. Please see the note I >> wrote in 0/5 for the v2, because the cover letter this is usually the place >> I try to clarify elements that may throw off a maintainer, and that don't >> belong in a commit message. >> >> Not so sound flippant here, but as long as there isn't an MTV award for >> "Most atomic codebase ever", I just don't have the time to split what I >> consider to be frivolous commits. The reasoning behind that is that I >> realistically don't consider that people are actually going to be thrown off >> by a "while I was here I also fixed an obvious typo" that got added into an >> existing commit or, most important, that even if they do, the amount of time >> that is going to be collectively wasted by people who might be thrown of by >> not having uber atomicity is not going to exceed the amount of time it will >> cost *me* to split it. >> >> Therefore, while I do understand the desire to have an atomic commit >> history, I'm afraid that if we can't strike a balance between how much extra >> time contributors are expected to waste vs how atomic a *real-life* >> codebase is enforced to be, if I have to split every little typo and >> stylistic fix into yet another commit, I'm simply not going to bother fixing >> typos or low hanging fruits I see any more. > > I understand, we all have a limited supply of time. > > Since I don't wish to start implementing different rules for different > contributors, could I ask you to stop contributing typo and low > hanging fruit fixes? I'm fine with that. Regards, /Pete > > Best Regards, > > Leif > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population Pete Batard @ 2019-10-08 12:38 ` Pete Batard 2019-10-10 8:59 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision Pete Batard 4 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm 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. Additional minor improvements are also applied, such as consistent use of uppercase values. Signed-off-by: Pete Batard <pete@akeo.ie> --- Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 153 ++++++++++---------- Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 1 + Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +- 3 files changed, 76 insertions(+), 80 deletions(-) diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c index bc35175279f2..8a4840267780 100644 --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c @@ -35,12 +35,13 @@ #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> #include <Library/PrintLib.h> -STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; +static RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; /*********************************************************************** SMBIOS data definition TYPE0 BIOS Information @@ -49,7 +50,7 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { { EFI_SMBIOS_TYPE_BIOS_INFORMATION, sizeof (SMBIOS_TABLE_TYPE0), 0 }, 1, // Vendor String 2, // BiosVersion String - 0x0, // BiosSegment + 0, // BiosSegment 3, // BiosReleaseDate String 0x1F, // BiosSize { // BiosCharacteristics @@ -97,23 +98,25 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { // Boot1394IsSupported :1; // SmartBatteryIsSupported :1; // BIOSCharacteristicsExtensionBytes[1] - 0x0e, // BiosBootSpecIsSupported :1; + 0x0E, // BiosBootSpecIsSupported :1; // FunctionKeyNetworkBootIsSupported :1; // TargetContentDistributionEnabled :1; // UefiSpecificationSupported :1; // VirtualMachineSupported :1; // ExtensionByte2Reserved :3; }, - 0xFF, // SystemBiosMajorRelease - 0xFF, // SystemBiosMinorRelease - 0xFF, // EmbeddedControllerFirmwareMajorRelease - 0xFF, // EmbeddedControllerFirmwareMinorRelease + 0, // SystemBiosMajorRelease + 0, // SystemBiosMinorRelease + 0, // EmbeddedControllerFirmwareMajorRelease + 0, // EmbeddedControllerFirmwareMinorRelease }; +CHAR8 mBiosVersion[128] = "EDK2-DEV"; + CHAR8 *mBIOSInfoType0Strings[] = { - "https://github.com/andreiw/RaspberryPiPkg", // Vendor String - "Raspberry Pi 64-bit UEFI (" __DATE__ " " __TIME__ ")", // BiosVersion String - __DATE__, + "TianoCore", // Vendor + mBiosVersion, // Version + __DATE__ " " __TIME__, // Release Date NULL }; @@ -132,42 +135,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 +160,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 +169,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 +178,8 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = { CHAR8 *mBoardInfoType2Strings[] = { mSysInfoManufName, mSysInfoProductName, - mSysInfoProductName, + mSysInfoVersionName, mSysInfoSerial, - "None", - mSysInfoSKU, NULL }; @@ -214,7 +192,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 +208,6 @@ CHAR8 *mEnclosureInfoType3Strings[] = { mSysInfoManufName, mSysInfoProductName, mSysInfoSerial, - "None", NULL }; @@ -240,9 +217,9 @@ CHAR8 *mEnclosureInfoType3Strings[] = { SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = { { EFI_SMBIOS_TYPE_PROCESSOR_INFORMATION, sizeof (SMBIOS_TABLE_TYPE4), 0}, 1, // Socket String - CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. + CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. ProcessorFamilyIndicatorFamily2, // ProcessorFamily; ///< The enumeration value from PROCESSOR_FAMILY2_DATA. - 2, // ProcessorManufacture String; + 2, // ProcessorManufacturer String; { // ProcessorId; { // PROCESSOR_SIGNATURE 0, // ProcessorSteppingId:4; @@ -306,9 +283,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 +293,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 }; @@ -430,7 +406,7 @@ SMBIOS_TABLE_TYPE17 mMemDevInfoType17 = { 0x0400, // Size; // When bit 15 is 0: Size in MB // When bit 15 is 1: Size in KB, and continues in ExtendedSize MemoryFormFactorUnknown, // FormFactor; ///< The enumeration value from MEMORY_FORM_FACTOR. - 0xff, // DeviceSet; + 0xFF, // DeviceSet; 1, // DeviceLocator String 2, // BankLocator String MemoryTypeDram, // MemoryType; ///< The enumeration value from MEMORY_DEVICE_TYPE. @@ -618,6 +594,30 @@ BIOSInfoUpdateSmbiosType0 ( VOID ) { + 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); + 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); + } + + // mBiosVersion, which is referenced in mBIOSInfoType0Strings, + // is not modified if the following call fails. + UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString), + mBiosVersion, sizeof (mBiosVersion)); + LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL); } @@ -631,7 +631,7 @@ I64ToHexString ( IN UINT64 Value ) { - static CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; + STATIC CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; UINT8 StringInx; INT8 NibbleInx; @@ -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.Data2 = 0x0; - mSysInfoType1.Uuid.Data3 = 0x0; + mSysInfoType1.Uuid.Data1 = BoardRevision; + mSysInfoType1.Uuid.Data2 = 0; + mSysInfoType1.Uuid.Data3 = 0; + // 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..fde194ea5d90 100644 --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf @@ -48,3 +48,4 @@ [Depex] [Pcd] gArmTokenSpaceGuid.PcdSystemMemorySize + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc index b37a02e97da7..6adc21bcc370 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 -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries Pete Batard @ 2019-10-10 8:59 ` Leif Lindholm 0 siblings, 0 replies; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 8:59 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Tue, Oct 08, 2019 at 01:38:39PM +0100, Pete Batard wrote: > 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. > > Additional minor improvements are also applied, such as consistent use > of uppercase values. Like for 2/5: mistake, misunderstanding, or disagreement? I appreciate style fixes, but I appreciate a consistent git history more. > Signed-off-by: Pete Batard <pete@akeo.ie> > --- > Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 153 ++++++++++---------- > Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 1 + > Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +- > 3 files changed, 76 insertions(+), 80 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > index bc35175279f2..8a4840267780 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > @@ -35,12 +35,13 @@ > #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> > #include <Library/PrintLib.h> > > -STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; > +static RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; Style fix only. > > /*********************************************************************** > SMBIOS data definition TYPE0 BIOS Information > @@ -49,7 +50,7 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { > { EFI_SMBIOS_TYPE_BIOS_INFORMATION, sizeof (SMBIOS_TABLE_TYPE0), 0 }, > 1, // Vendor String > 2, // BiosVersion String > - 0x0, // BiosSegment > + 0, // BiosSegment Style change only. > 3, // BiosReleaseDate String > 0x1F, // BiosSize > { // BiosCharacteristics > @@ -97,23 +98,25 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { > // Boot1394IsSupported :1; > // SmartBatteryIsSupported :1; > // BIOSCharacteristicsExtensionBytes[1] > - 0x0e, // BiosBootSpecIsSupported :1; > + 0x0E, // BiosBootSpecIsSupported :1; Case change only. > // FunctionKeyNetworkBootIsSupported :1; > // TargetContentDistributionEnabled :1; > // UefiSpecificationSupported :1; > // VirtualMachineSupported :1; > // ExtensionByte2Reserved :3; > }, > - 0xFF, // SystemBiosMajorRelease > - 0xFF, // SystemBiosMinorRelease > - 0xFF, // EmbeddedControllerFirmwareMajorRelease > - 0xFF, // EmbeddedControllerFirmwareMinorRelease > + 0, // SystemBiosMajorRelease > + 0, // SystemBiosMinorRelease > + 0, // EmbeddedControllerFirmwareMajorRelease > + 0, // EmbeddedControllerFirmwareMinorRelease > }; > > +CHAR8 mBiosVersion[128] = "EDK2-DEV"; > + > CHAR8 *mBIOSInfoType0Strings[] = { > - "https://github.com/andreiw/RaspberryPiPkg", // Vendor String > - "Raspberry Pi 64-bit UEFI (" __DATE__ " " __TIME__ ")", // BiosVersion String > - __DATE__, > + "TianoCore", // Vendor Could alternatively use gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor here? I'm not sure "EDK II" (the default value) is a more useful description, but it would make this platform more consistent with others. > + mBiosVersion, // Version > + __DATE__ " " __TIME__, // Release Date > NULL > }; > > @@ -132,42 +135,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 +160,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 +169,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 +178,8 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = { > CHAR8 *mBoardInfoType2Strings[] = { > mSysInfoManufName, > mSysInfoProductName, > - mSysInfoProductName, > + mSysInfoVersionName, > mSysInfoSerial, > - "None", > - mSysInfoSKU, > NULL > }; > > @@ -214,7 +192,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 +208,6 @@ CHAR8 *mEnclosureInfoType3Strings[] = { > mSysInfoManufName, > mSysInfoProductName, > mSysInfoSerial, > - "None", > NULL > }; > > @@ -240,9 +217,9 @@ CHAR8 *mEnclosureInfoType3Strings[] = { > SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = { > { EFI_SMBIOS_TYPE_PROCESSOR_INFORMATION, sizeof (SMBIOS_TABLE_TYPE4), 0}, > 1, // Socket String > - CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. > + CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. Whitespace change only. > ProcessorFamilyIndicatorFamily2, // ProcessorFamily; ///< The enumeration value from PROCESSOR_FAMILY2_DATA. > - 2, // ProcessorManufacture String; > + 2, // ProcessorManufacturer String; Comment typo fix only. > { // ProcessorId; > { // PROCESSOR_SIGNATURE > 0, // ProcessorSteppingId:4; > @@ -306,9 +283,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 +293,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 > }; > > @@ -430,7 +406,7 @@ SMBIOS_TABLE_TYPE17 mMemDevInfoType17 = { > 0x0400, // Size; // When bit 15 is 0: Size in MB > // When bit 15 is 1: Size in KB, and continues in ExtendedSize > MemoryFormFactorUnknown, // FormFactor; ///< The enumeration value from MEMORY_FORM_FACTOR. > - 0xff, // DeviceSet; > + 0xFF, // DeviceSet; Style fix only. > 1, // DeviceLocator String > 2, // BankLocator String > MemoryTypeDram, // MemoryType; ///< The enumeration value from MEMORY_DEVICE_TYPE. > @@ -618,6 +594,30 @@ BIOSInfoUpdateSmbiosType0 ( > VOID > ) > { > + UINT32 FirmwareRevision = 0; > + EFI_STATUS Status = EFI_SUCCESS; > + INTN i; > + INTN State = 0; > + INTN Value[2]; The three last variables defined above are only used by code added by the next patch, meaning this patch would break git bisect with many toolchains (defined but not used/set but not used). > + > + // 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); > + } > + > + // mBiosVersion, which is referenced in mBIOSInfoType0Strings, > + // is not modified if the following call fails. > + UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString), > + mBiosVersion, sizeof (mBiosVersion)); > + > LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL); > } > > @@ -631,7 +631,7 @@ I64ToHexString ( > IN UINT64 Value > ) > { > - static CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; > + STATIC CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; Style fix only. / Leif > UINT8 StringInx; > INT8 NibbleInx; > > @@ -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.Data2 = 0x0; > - mSysInfoType1.Uuid.Data3 = 0x0; > + mSysInfoType1.Uuid.Data1 = BoardRevision; > + mSysInfoType1.Uuid.Data2 = 0; > + mSysInfoType1.Uuid.Data3 = 0; > + // 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..fde194ea5d90 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf > @@ -48,3 +48,4 @@ [Depex] > > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemorySize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString > diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc > index b37a02e97da7..6adc21bcc370 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 > -- > 2.21.0.windows.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard ` (2 preceding siblings ...) 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries Pete Batard @ 2019-10-08 12:38 ` Pete Batard 2019-10-10 9:01 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision Pete Batard 4 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm 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 | 44 +++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c index 8a4840267780..66ffadd0cade 100644 --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c @@ -41,7 +41,9 @@ #include <Library/UefiBootServicesTableLib.h> #include <Library/PrintLib.h> -static RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; +#define SMB_IS_DIGIT(c) (((c) >= '0') && ((c) <= '9')) + +STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; /*********************************************************************** SMBIOS data definition TYPE0 BIOS Information @@ -618,6 +620,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD Pete Batard @ 2019-10-10 9:01 ` Leif Lindholm 0 siblings, 0 replies; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 9:01 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Tue, Oct 08, 2019 at 01:38:40PM +0100, Pete Batard wrote: > 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 | 44 +++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > index 8a4840267780..66ffadd0cade 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > @@ -41,7 +41,9 @@ > #include <Library/UefiBootServicesTableLib.h> > #include <Library/PrintLib.h> > > -static RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; > +#define SMB_IS_DIGIT(c) (((c) >= '0') && ((c) <= '9')) > + > +STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; Style fix only. / Leif > > /*********************************************************************** > SMBIOS data definition TYPE0 BIOS Information > @@ -618,6 +620,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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard ` (3 preceding siblings ...) 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD Pete Batard @ 2019-10-08 12:38 ` Pete Batard 2019-10-10 9:03 ` Leif Lindholm 4 siblings, 1 reply; 17+ messages in thread From: Pete Batard @ 2019-10-08 12:38 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm 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> --- 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 66ffadd0cade..540e3fd61f25 100644 --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c @@ -863,16 +863,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision Pete Batard @ 2019-10-10 9:03 ` Leif Lindholm 0 siblings, 0 replies; 17+ messages in thread From: Leif Lindholm @ 2019-10-10 9:03 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel On Tue, Oct 08, 2019 at 01:38:41PM +0100, 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 66ffadd0cade..540e3fd61f25 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > @@ -863,16 +863,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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-10-10 17:28 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-08 12:38 [edk2-platforms][PATCH v2 0/5] Platform/RPi3: Various SMBIOS improvements Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Pete Batard 2019-10-10 8:39 ` Leif Lindholm 2019-10-10 12:41 ` Pete Batard 2019-10-10 16:43 ` Leif Lindholm 2019-10-10 17:27 ` Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 2/5] Platform/RPi3/RpiFirmwareDxe: Improve serial number population Pete Batard 2019-10-10 8:43 ` Leif Lindholm 2019-10-10 12:41 ` Pete Batard 2019-10-10 16:49 ` Leif Lindholm 2019-10-10 17:28 ` Pete Batard 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries Pete Batard 2019-10-10 8:59 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 4/5] Platform/RPi3/PlatformSmbiosDxe: Populate BIOS major/minor from PCD Pete Batard 2019-10-10 9:01 ` Leif Lindholm 2019-10-08 12:38 ` [edk2-platforms][PATCH v2 5/5] Platform/RPi3/PlatformSmbiosDxe: Derive RAM size from board revision Pete Batard 2019-10-10 9:03 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox