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

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

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