From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=p2SKhSg/; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: leif.lindholm@linaro.org) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by groups.io with SMTP; Fri, 27 Sep 2019 11:34:46 -0700 Received: by mail-wr1-f68.google.com with SMTP id o18so4256699wrv.13 for ; Fri, 27 Sep 2019 11:34:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=NI0L3SMwTYfLaG6rQc6hxGVo2wzskWE979p7tFEzslk=; b=p2SKhSg/z+10CBMwThFoxHhWptMYFPuqhNkLUxxP6CEFPMQSIKdzNzdd3Uf8T98zLC aqrnRHrmD8FwJTZeyBPZkE0r43+dAdXRFiHpbm/TyXxfTv/uZTuGWqGR16SHybwzwpV5 hFL70PxWicsy2JpO43Zs4vU+a9DlaE+hAQA4qhPADvTRkhR8jITxY5eUTrHZxR62rsBE HBEhZfyalIxC6+EDk6p7RZ3BfxcOdv7fNEQksDqh3sxbJxvz7cbZ+4DWjh5COSJ5WzsY Cz/pVD9mU+ETI4bMjoDQ/LN1eYCKCBQcizpR9x3XS8U3mhh9Aehq3VEFHgmv8TKI8uGK S0qg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=NI0L3SMwTYfLaG6rQc6hxGVo2wzskWE979p7tFEzslk=; b=EnGoWoAGi7bi6P0+np/x/WU9slpMnZKeBZacgpOXkj4rE3C8v3dBgobexFwdmBxtCf +BiGLn7kiUJM3pcWrSidzN1RmklNqxgIt4gZkq+cOKqjl3EKlDnL0ifN67wLaDNZzumm ZIGRRied4ESBU2ZXCt2G66FFMy5MCEHxWRBn3CbCb/Jf6VpGIvd/xwNtNj+MeKpIapAe YhJVjXpltaqUAlI3yzUzFi+haDYfdRXt9I1YbbdTVRdXHDAtCg6VI6ctsn2DPlkNEzZK YfwQbNOY/fdrUlkRLCpllgCj2g0NogClUETGcnMs28k3aDXSWcARt9sNRp/0SLsRK6Fo fPPA== X-Gm-Message-State: APjAAAX4xWeACjElquz6HJDa6kCsGrq/a0hgjFmMDdB7Q6C6b0zNqER1 FHynDdmU/X2JkWBnDENCaTZERA== X-Google-Smtp-Source: APXvYqyUJ92dBqTX5RS5LgsF4nWJbhosEEg6Q4SgOGOFLFJW4G2gOtg5Ntp0TDeQTpCuxv+AbqLopA== X-Received: by 2002:adf:a415:: with SMTP id d21mr3856485wra.94.1569609284519; Fri, 27 Sep 2019 11:34:44 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id x6sm12175722wmf.38.2019.09.27.11.34.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Sep 2019 11:34:43 -0700 (PDT) Date: Fri, 27 Sep 2019 19:34:42 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org Subject: Re: [edk2-platforms PATCH 1/2] Platform/RPi3: Add more query functions in RpiFirmwareDxe Message-ID: <20190927183442.GR25504@bivouac.eciton.net> References: <20190904121418.3700-1-pete@akeo.ie> <20190904121418.3700-2-pete@akeo.ie> MIME-Version: 1.0 In-Reply-To: <20190904121418.3700-2-pete@akeo.ie> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 >