From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web10.5166.1570711271300772796 for ; Thu, 10 Oct 2019 05:41:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=aQU/VB4/; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.128.67, mailfrom: pete@akeo.ie) Received: by mail-wm1-f67.google.com with SMTP id 5so6790567wmg.0 for ; Thu, 10 Oct 2019 05:41:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Wg3iBd0PeN/lyBIvchpNnD2Pf54QmBRQTosp+2qVOnE=; b=aQU/VB4/ozzfSYXIE3lLrIbhLtEN8/hsFzn/S8VcDq2BpaRDCN0fHd2qTgzwAhMNMI iTBMk25uN6OiG79zLVX4AtFC5GqRjTLD9boUT3a12FVYjTpNpwqCxomb+T/B5N4/kcp1 5IhDOLpHUVpMu7VsxWTleCJcnw655P4XUltU+v4lAdhq37l6fbEgn99B1+dMjNx0Hgod SB3ou7g5G/629yK1T/zFomOEEmLqlGzo0jiIxgfGfjwHX1lTTL9/By+Yn0xnIlsv+pcS Pc9O2Y/9g9Baj6F2giC/QpiiRG6/KGtmZ15hiHzDp++MT7I3I7oHV6nNZLTux/MJ5ajp A7Sw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Wg3iBd0PeN/lyBIvchpNnD2Pf54QmBRQTosp+2qVOnE=; b=dMLxqxOvaQXWTiiBaupIPf5pt0W4mJPSg/FaP+D8QiYegl/5x7fAFbA/DihuU0nr7R gpUgq+NMcGz7ZhzCPhkI/YcYR0aUW9ensxrb2Vyc5My7h5FKD+0vqamjvGacBS9iFE1P 6sU913g/wVYDbRZgQmEfMJtw3/ic/olfPQgJg2BrqMB/WHGFKpuykVnNZzZzdjqzQC4t Lw/BTp87Xa9qtlwlB8lmfpguA5r5EtXq8P0vtFsv/DzFEXL9hyhdxkUr3wcLqjlArvyh B15zLDdaFZKnIvOP9a2ZKvZMbHa3XecUJZtUcCLbdL2AbMGicYu0b0j8PzJxScMnv9Zz o9Rw== X-Gm-Message-State: APjAAAVgXZX9DiSKR77Vbd8mw6JTkA6WAz5ZimLYoOWi5WneiTy3vJ8p +j66+0eQ+E4Ne9OAKeIUVxkjOQ== X-Google-Smtp-Source: APXvYqzSsoqC79QxbZsW9hIs2aOdUxXYukok53//+2Lz/L7Rjpi0cAa2w0iYnDscF0/jHHzipUv6VA== X-Received: by 2002:a1c:f401:: with SMTP id z1mr7287822wma.66.1570711269753; Thu, 10 Oct 2019 05:41:09 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.40.122]) by smtp.googlemail.com with ESMTPSA id c9sm5125912wrt.7.2019.10.10.05.41.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Oct 2019 05:41:07 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions To: Leif Lindholm Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20191008123841.12952-1-pete@akeo.ie> <20191008123841.12952-2-pete@akeo.ie> <20191010083948.GS25504@bivouac.eciton.net> From: "Pete Batard" Message-ID: Date: Thu, 10 Oct 2019 13:41:06 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191010083948.GS25504@bivouac.eciton.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 >> --- >> 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 >>