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

["Pete Batard" <pete@akeo.ie>]

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
>>>
>