public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: Pete Batard <pete@akeo.ie>
Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org
Subject: Re: [edk2-platforms PATCH 1/2] Platform/RPi3: Add more query functions in RpiFirmwareDxe
Date: Fri, 27 Sep 2019 19:34:42 +0100	[thread overview]
Message-ID: <20190927183442.GR25504@bivouac.eciton.net> (raw)
In-Reply-To: <20190904121418.3700-2-pete@akeo.ie>

On Wed, Sep 04, 2019 at 01:14:17PM +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.
> 
> Also fixes a typo where "%s" was used instead of "%a" and improves
> RpiFirmwareGetSerial() to derive a serial from the the MAC address
> in case the platform returns 0 for the serial.

Can you please break those bits out into separate patches?
(The word "also" in a commit message is a good harbinger of commit
splitting.)
Also (sorry), could you spell out "serial number" in the commit
message?

No other issues with the code.

Best Regards,

Leif

> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 166 +++++++++++++++++++-
>  Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h          |  58 +++++--
>  2 files changed, 205 insertions(+), 19 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
> index 9b5ee1946279..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()
> @@ -461,7 +468,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 +478,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 +513,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 +1163,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
> 

  reply	other threads:[~2019-09-27 18:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 12:14 [edk2-platforms PATCH 0/2] Platform/RPi3: PlatformSmbios improvements Pete Batard
2019-09-04 12:14 ` [edk2-platforms PATCH 1/2] Platform/RPi3: Add more query functions in RpiFirmwareDxe Pete Batard
2019-09-27 18:34   ` Leif Lindholm [this message]
2019-09-04 12:14 ` [edk2-platforms PATCH 2/2] Platform/RPi3: Improve the population of PlatformSmbiosDxe elements Pete Batard
2019-09-27 19:03   ` Leif Lindholm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190927183442.GR25504@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox