From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web10.3612.1570696792469220798 for ; Thu, 10 Oct 2019 01:39:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=wiUfauuO; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id j18so6727563wrq.10 for ; Thu, 10 Oct 2019 01:39:52 -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=kYDKWd9IdqQVaX4goR8W4ODUN1GC64ZuLcQVreKgC9o=; b=wiUfauuO4BbovtnrYoGis6dOkYafUpS6eH0bmaqjIn6vhG7wmm+MdPTQS2Dotj5l8p 116FlOeWWqJsVizz/Y32+f9vzJpZ2fy+BKzZXpbCvcwKh77zA9lkBlBSvxRRYJlslSR2 Bj7Cz9CEVG3f2KaRbmBUxpYk/5nga7FHB3rsI5/lVj8fl9ca6ugC0170rib6HDl3gGfP CFapJQnA9pMlwBfPx2czPmkwU5yBPn8r+CQO2VJ/65rECSirRH5CmmtgzPAauP5PW/zH pWAeDrF6whO1/l+/TW30xEzcfvJ+PYuFwmjhLWMGzwtIUsShpm0PhDQ3spPnQJGm8K0L /CnA== 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=kYDKWd9IdqQVaX4goR8W4ODUN1GC64ZuLcQVreKgC9o=; b=jC13zFSZIQ1NiXUROIbHoyYAFbxRYqfmhCsq2MP/Vjr+Dp7lqyw2mEcTDx8VXWu8TE F6C7o4t94Hivz+2wTfKFFnBHjbsJKeOTXJ5NO4jYwSTaKte4r41jNXCA1vpBvHqCupXn seP22smEqNE6RkEXat6WFHOpj+W+gn8KGlaBWRFNo2SUbd1adowYFwYHUNfhrMFtHatN E15CLQ+hXB79ZEwmq2lBAaGhWcHdOVRtW6pjm5UPNkVEsxCaOx9RYg5FAuXtRWv/JtQZ vkoVC0xvrNKpCe4RzE//dDzgllvEw5UZLL4g5fjXx5nukj3HmiKPaOi/sUtB3FQ6GyzL Y+nA== X-Gm-Message-State: APjAAAUOpuUR+xe39O40zoDw+Fwvbr+UB9Njx8o6/Ciftqu3hclOH6tp mvmAnmjAInnhnxQJeljSIuSGNw== X-Google-Smtp-Source: APXvYqwX/c9nX6d7ZXZWDdUy7cxkAdRZQprFG7XiO1rpUvcebSN54FsNbIEUm4rDa5AAOOpR9dGK1w== X-Received: by 2002:a5d:43c9:: with SMTP id v9mr7198924wrr.200.1570696790847; Thu, 10 Oct 2019 01:39:50 -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 f18sm3576048wrv.38.2019.10.10.01.39.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Oct 2019 01:39:50 -0700 (PDT) Date: Thu, 10 Oct 2019 09:39:48 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org Subject: Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Message-ID: <20191010083948.GS25504@bivouac.eciton.net> References: <20191008123841.12952-1-pete@akeo.ie> <20191008123841.12952-2-pete@akeo.ie> MIME-Version: 1.0 In-Reply-To: <20191008123841.12952-2-pete@akeo.ie> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. 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. Are there other intended uses of that function, or could we move this logic there? / 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 >