[edk2-platforms][PATCH 0/2] Platform/RPi: Smbios reporting improvements

[edk2-platforms][PATCH 0/2] Platform/RPi: Smbios reporting improvements

[Pete Batard]

This series of patches improves the reporting of the firmware version
and platform serial number for the Raspberry Pi platforms.

Since there is only so much we can do with 16 bits and the firmware revision
reported by the VideoCore mailbox is really a timestamp of when the VC
firmware was build, we convert it to a YY.MM revision, which should be good
enough for our purpose, which is indicational and end-user readable.

A serial number fix is also needed as RPi4 platforms appear to report
0x0000000010000000 as the board serial, so the case where we fell back to
the MAC address when the board serial is 0 needs to be amended.

Pete Batard (2):
  Platform/RPi/SmbiosDxe: Report a more human readable firmware revision
  Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4

 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 20 ++++++++++++--------
 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |  2 ++
 Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c         |  5 +++--
 3 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.21.0.windows.1

[edk2-platforms][PATCH 1/2] Platform/RPi/SmbiosDxe: Report a more human readable firmware revision

[Pete Batard]

The firmware revision that is queried through the VideoCore mailbox
is really the 32-bit timestamp of when the VideoCore firmware was
generated.

To make this more palatable for human reporting, convert it to a
YY.MM firmware revision, so that end-users can get an approximative
idea of how old their VideoCore firmware is.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 20 ++++++++++++--------
 Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |  2 ++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
index 6aa68a0925ba..06c73d691f9d 100644
--- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
+++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
@@ -38,6 +38,7 @@
 #include <Library/PcdLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/TimeBaseLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/PrintLib.h>
 
@@ -597,23 +598,26 @@ BIOSInfoUpdateSmbiosType0 (
   VOID
   )
 {
-  UINT32 FirmwareRevision = 0;
+  UINT32 EpochSeconds = 0;
   EFI_STATUS Status = EFI_SUCCESS;
+  EFI_TIME Time;
   INTN   i;
   INTN   State = 0;
   INTN   Value[2];
 
   // Populate the Firmware major and minor.
-  Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
+  Status = mFwProtocol->GetFirmwareRevision (&EpochSeconds);
   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);
+    // The firmware revision is really an epoch time which we convert to a
+    // YY.MM major.minor. This is good enough for our purpose, where this
+    // revision is merely provided as a loose indicator of when the
+    // VideoCore firmware was generated.
+    EpochToEfiTime (EpochSeconds, &Time);
+    ASSERT (Time.Year >= 2000 && Time.Year <= 2255);
+    mBIOSInfoType0.EmbeddedControllerFirmwareMajorRelease = (UINT8)(Time.Year - 2000);
+    mBIOSInfoType0.EmbeddedControllerFirmwareMinorRelease = Time.Month;
   }
 
   // mBiosVendor and mBiosVersion, which are referenced in mBIOSInfoType0Strings,
diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
index 0bd72c3ba6a1..9554c2e998f1 100644
--- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
+++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
@@ -27,6 +27,7 @@ [Packages]
   ArmPlatformPkg/ArmPlatformPkg.dec
   ArmPkg/ArmPkg.dec
   Platform/RaspberryPi/RaspberryPi.dec
+  EmbeddedPkg/EmbeddedPkg.dec
 
 [LibraryClasses]
   UefiBootServicesTableLib
@@ -37,6 +38,7 @@ [LibraryClasses]
   UefiDriverEntryPoint
   DebugLib
   PrintLib
+  TimeBaseLib
 
 [Protocols]
   gEfiSmbiosProtocolGuid           # PROTOCOL SOMETIMES_CONSUMED
-- 
2.21.0.windows.1

[edk2-platforms][PATCH 2/2] Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4

[Pete Batard]

Some (all?) Raspbery Pi 4 platforms report 0x0000000010000000 as their
board serial when queried through the VideoCore mailbox.

Fix this by using the MAC address then.

Signed-off-by: Pete Batard <pete@akeo.ie>
---
 Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index dd61ef089ca7..75826fdc0e53 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -394,8 +394,9 @@ RpiFirmwareGetSerial (
   }
 
   *Serial = Cmd->TagBody.Serial;
-  // Some platforms return 0 for serial. For those, try to use the MAC address.
-  if (*Serial == 0) {
+  // Some platforms return 0 or 0x0000000010000000 for serial.
+  // For those, try to use the MAC address.
+  if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {
     Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
     // Convert to a more user-friendly value
     *Serial = SwapBytes64 (*Serial << 16);
-- 
2.21.0.windows.1

Re: [edk2-platforms][PATCH 2/2] Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4

[Ard Biesheuvel]

On Wed, 8 Jan 2020 at 18:00, Pete Batard <pete@akeo.ie> wrote:
>
> Some (all?) Raspbery Pi 4 platforms report 0x0000000010000000 as their
> board serial when queried through the VideoCore mailbox.
>
> Fix this by using the MAC address then.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> index dd61ef089ca7..75826fdc0e53 100644
> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> @@ -394,8 +394,9 @@ RpiFirmwareGetSerial (
>    }
>
>    *Serial = Cmd->TagBody.Serial;
> -  // Some platforms return 0 for serial. For those, try to use the MAC address.
> -  if (*Serial == 0) {
> +  // Some platforms return 0 or 0x0000000010000000 for serial.
> +  // For those, try to use the MAC address.
> +  if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {

What is the point of using a mask here? Is it deliberately matching
0x0000000020000000 or 0x00000000F0000000 as well?

>      Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
>      // Convert to a more user-friendly value
>      *Serial = SwapBytes64 (*Serial << 16);
> --
> 2.21.0.windows.1
>

Re: [edk2-platforms][PATCH 2/2] Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4

[Pete Batard]

Hi Ard,

On 2020.01.09 14:44, Ard Biesheuvel wrote:
> On Wed, 8 Jan 2020 at 18:00, Pete Batard <pete@akeo.ie> wrote:
>>
>> Some (all?) Raspbery Pi 4 platforms report 0x0000000010000000 as their
>> board serial when queried through the VideoCore mailbox.
>>
>> Fix this by using the MAC address then.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> index dd61ef089ca7..75826fdc0e53 100644
>> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>> @@ -394,8 +394,9 @@ RpiFirmwareGetSerial (
>>     }
>>
>>     *Serial = Cmd->TagBody.Serial;
>> -  // Some platforms return 0 for serial. For those, try to use the MAC address.
>> -  if (*Serial == 0) {
>> +  // Some platforms return 0 or 0x0000000010000000 for serial.
>> +  // For those, try to use the MAC address.
>> +  if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {
> 
> What is the point of using a mask here? Is it deliberately matching
> 0x0000000020000000 or 0x00000000F0000000 as well?

My expectation is that if we're seeing 0x0000000010000000 as a board 
serial, there's not so insignificant chance are future boards may use 
0x0000000010000000, 0x0000000030000000 and so on.

In other words, when someone starts counting from 0 to 1 somewhere, it's 
not unreasonable to also expect them to reach 2 at some stage...

Now, if anything, the one change we could apply here is remove the 
(*Serial == 0) check, which of course is redundant. But I fail to see 
why we'd want to restrict ourselves to checking only for 
0x0000000010000000 here, when we have no clear idea how the value we 
read may evolve in the future, as anything that the mask condition 
matches is clearly not a serial we want to use anyway...

Regards,

/Pete


> 
>>       Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
>>       // Convert to a more user-friendly value
>>       *Serial = SwapBytes64 (*Serial << 16);
>> --
>> 2.21.0.windows.1
>>

Re: [edk2-platforms][PATCH 2/2] Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4

[Pete Batard]

Could I have a ping on this, as well as 
https://edk2.groups.io/g/devel/topic/69331625#52611?

It looks to me like the latest RPi related patches have reached some 
kind of integration limbo, since I'm not seeing any of those being 
requested a v2.

Thanks,

/Pete

On 2020.01.09 15:02, Pete Batard wrote:
> Hi Ard,
> 
> On 2020.01.09 14:44, Ard Biesheuvel wrote:
>> On Wed, 8 Jan 2020 at 18:00, Pete Batard <pete@akeo.ie> wrote:
>>>
>>> Some (all?) Raspbery Pi 4 platforms report 0x0000000010000000 as their
>>> board serial when queried through the VideoCore mailbox.
>>>
>>> Fix this by using the MAC address then.
>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> ---
>>>   Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git 
>>> a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c 
>>> b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> index dd61ef089ca7..75826fdc0e53 100644
>>> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
>>> @@ -394,8 +394,9 @@ RpiFirmwareGetSerial (
>>>     }
>>>
>>>     *Serial = Cmd->TagBody.Serial;
>>> -  // Some platforms return 0 for serial. For those, try to use the 
>>> MAC address.
>>> -  if (*Serial == 0) {
>>> +  // Some platforms return 0 or 0x0000000010000000 for serial.
>>> +  // For those, try to use the MAC address.
>>> +  if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {
>>
>> What is the point of using a mask here? Is it deliberately matching
>> 0x0000000020000000 or 0x00000000F0000000 as well?
> 
> My expectation is that if we're seeing 0x0000000010000000 as a board 
> serial, there's not so insignificant chance are future boards may use 
> 0x0000000010000000, 0x0000000030000000 and so on.
> 
> In other words, when someone starts counting from 0 to 1 somewhere, it's 
> not unreasonable to also expect them to reach 2 at some stage...
> 
> Now, if anything, the one change we could apply here is remove the 
> (*Serial == 0) check, which of course is redundant. But I fail to see 
> why we'd want to restrict ourselves to checking only for 
> 0x0000000010000000 here, when we have no clear idea how the value we 
> read may evolve in the future, as anything that the mask condition 
> matches is clearly not a serial we want to use anyway...
> 
> Regards,
> 
> /Pete
> 
> 
>>
>>>       Status = RpiFirmwareGetMacAddress ((UINT8*) Serial);
>>>       // Convert to a more user-friendly value
>>>       *Serial = SwapBytes64 (*Serial << 16);
>>> -- 
>>> 2.21.0.windows.1
>>>
> 

Re: [edk2-platforms][PATCH 1/2] Platform/RPi/SmbiosDxe: Report a more human readable firmware revision

[Philippe Mathieu-Daudé]

On 1/8/20 6:00 PM, Pete Batard wrote:
> The firmware revision that is queried through the VideoCore mailbox
> is really the 32-bit timestamp of when the VideoCore firmware was
> generated.
> 
> To make this more palatable for human reporting, convert it to a
> YY.MM firmware revision, so that end-users can get an approximative
> idea of how old their VideoCore firmware is.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 20 ++++++++++++--------
>   Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |  2 ++
>   2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> index 6aa68a0925ba..06c73d691f9d 100644
> --- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> +++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c
> @@ -38,6 +38,7 @@
>   #include <Library/PcdLib.h>
>   #include <Library/BaseMemoryLib.h>
>   #include <Library/MemoryAllocationLib.h>
> +#include <Library/TimeBaseLib.h>
>   #include <Library/UefiBootServicesTableLib.h>
>   #include <Library/PrintLib.h>
>   
> @@ -597,23 +598,26 @@ BIOSInfoUpdateSmbiosType0 (
>     VOID
>     )
>   {
> -  UINT32 FirmwareRevision = 0;
> +  UINT32 EpochSeconds = 0;
>     EFI_STATUS Status = EFI_SUCCESS;
> +  EFI_TIME Time;
>     INTN   i;
>     INTN   State = 0;
>     INTN   Value[2];
>   
>     // Populate the Firmware major and minor.
> -  Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision);
> +  Status = mFwProtocol->GetFirmwareRevision (&EpochSeconds);
>     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);
> +    // The firmware revision is really an epoch time which we convert to a
> +    // YY.MM major.minor. This is good enough for our purpose, where this
> +    // revision is merely provided as a loose indicator of when the
> +    // VideoCore firmware was generated.
> +    EpochToEfiTime (EpochSeconds, &Time);
> +    ASSERT (Time.Year >= 2000 && Time.Year <= 2255);
> +    mBIOSInfoType0.EmbeddedControllerFirmwareMajorRelease = (UINT8)(Time.Year - 2000);
> +    mBIOSInfoType0.EmbeddedControllerFirmwareMinorRelease = Time.Month;
>     }
>   
>     // mBiosVendor and mBiosVersion, which are referenced in mBIOSInfoType0Strings,
> diff --git a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> index 0bd72c3ba6a1..9554c2e998f1 100644
> --- a/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf
> @@ -27,6 +27,7 @@ [Packages]
>     ArmPlatformPkg/ArmPlatformPkg.dec
>     ArmPkg/ArmPkg.dec
>     Platform/RaspberryPi/RaspberryPi.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
>   
>   [LibraryClasses]
>     UefiBootServicesTableLib
> @@ -37,6 +38,7 @@ [LibraryClasses]
>     UefiDriverEntryPoint
>     DebugLib
>     PrintLib
> +  TimeBaseLib
>   
>   [Protocols]
>     gEfiSmbiosProtocolGuid           # PROTOCOL SOMETIMES_CONSUMED
> 

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

Re: [edk2-platforms][PATCH 0/2] Platform/RPi: Smbios reporting improvements

[Ard Biesheuvel]

On Wed, 8 Jan 2020 at 18:00, Pete Batard <pete@akeo.ie> wrote:
>
> This series of patches improves the reporting of the firmware version
> and platform serial number for the Raspberry Pi platforms.
>
> Since there is only so much we can do with 16 bits and the firmware revision
> reported by the VideoCore mailbox is really a timestamp of when the VC
> firmware was build, we convert it to a YY.MM revision, which should be good
> enough for our purpose, which is indicational and end-user readable.
>
> A serial number fix is also needed as RPi4 platforms appear to report
> 0x0000000010000000 as the board serial, so the case where we fell back to
> the MAC address when the board serial is 0 needs to be amended.
>
> Pete Batard (2):
>   Platform/RPi/SmbiosDxe: Report a more human readable firmware revision
>   Platform/RPi/RPiFirmwareDxe: Fix serial number population for RPi4
>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 314c45bc620b..9369b01e86ad

Thanks!


>  Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c   | 20 ++++++++++++--------
>  Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf |  2 ++
>  Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c         |  5 +++--
>  3 files changed, 17 insertions(+), 10 deletions(-)
>
> --
> 2.21.0.windows.1
>