From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::242; helo=mail-wm0-x242.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 23628212AAB6D for ; Tue, 12 Jun 2018 09:00:51 -0700 (PDT) Received: by mail-wm0-x242.google.com with SMTP id r15-v6so21492239wmc.1 for ; Tue, 12 Jun 2018 09:00:51 -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=TKboDOlfgD0qvox/m/h3WIdRQghFqpJUmFT+7H+2hl0=; b=gIXahTvfw8oJGdIllvA1tbg7Tz06Xck25cHjMWX9viBIFq1H1hUpueFgVLRx9Ibcnu oEta0JH6i6h4o9XZlllCeRKAxcoSu3lra/MBKMFWSbAZlGEOoq5OF1utx6uS4LR7z/h0 GQ/PhYmXx6Fuf0ryqASFTUmUkUBxRIR67i5dM= 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=TKboDOlfgD0qvox/m/h3WIdRQghFqpJUmFT+7H+2hl0=; b=eL+9UDAiVJY/Jk8/c9mSPWyG7dMWdVxXh3b626FI13l653KYSle5nMQFmaX5d+F5+u WEPxsDMG5BplCx1/pTEutPKKmEbZgFTklC0JMXc5eX//lRQsDjmfG7OmBibgcycGztI3 O5I4ex2mS/sGgsuBgN1DQue/8kpJda7nHZsCIxmuMFRDfWvIcZzJUd02fSbcy1J9tAPh nJjxQCaix2wCPy/yORxvacwECSDfYUWOtwwqHKBjuMo5Fucj6a/0z5caYX29+NZ6ZY3c lw/moJdOp/kaYmkDJyRM54O1zXdHQindx389ZbEU/vtoST6fvYUdQk6PIHJGjwI7sjJ6 aMsw== X-Gm-Message-State: APt69E21UzSqyxrI+8dDvdRVLvRItUmjNERD6uI88e1a/bJGSXOtpa0t vQh5OjHlS+oCNAFVInthbp+jsg== X-Google-Smtp-Source: ADUXVKJWJ2IGRmJ0LexgDvo+/qtdYhtY1m5kyT698JYIJdn6Q094fd2hBIqmSnTAQ7zc7e8Y/1+nSw== X-Received: by 2002:a1c:b443:: with SMTP id d64-v6mr661343wmf.44.1528819250425; Tue, 12 Jun 2018 09:00:50 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id a8-v6sm729750wrc.18.2018.06.12.09.00.48 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Jun 2018 09:00:48 -0700 (PDT) Date: Tue, 12 Jun 2018 17:00:47 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, jinghua@marvell.com, jsd@semihalf.com, jaz@semihalf.com Message-ID: <20180612160047.a7klyc6lkwbryqcw@bivouac.eciton.net> References: <1528472063-1660-1-git-send-email-mw@semihalf.com> <1528472063-1660-5-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1528472063-1660-5-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms PATCH 04/25] Marvell/Drivers: MvBoardDescDxe: Introduce board description driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jun 2018 16:00:52 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jun 08, 2018 at 05:34:02PM +0200, Marcin Wojtas wrote: > From: jinghua > > This patch introduces a producer of MARVELL_BOARD_DESC_PROTOCOL, which > gets SoC description from ArmadaSoCDescLib, then based on dsc file, > provide only enabled hardware module controllers for the consumers, > which are typically controllers' drivers. Thanks to that > there is a separation between obtaining the platform description and > the drivers. A first example of the board description callback > is information about UTMI controllers and type. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: jinghua > Signed-off-by: Marcin Wojtas > --- > Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c | 174 ++++++++++++++++++++ > Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h | 59 +++++++ > Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf | 65 ++++++++ > 3 files changed, 298 insertions(+) > > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c > new file mode 100644 > index 0000000..c220e58 > --- /dev/null > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c > @@ -0,0 +1,174 @@ > +/******************************************************************************* > +Copyright (C) 2018 Marvell International Ltd. > + > +Marvell BSD License Option > + > +If you received this File from Marvell, you may opt to use, redistribute and/or > +modify this File under the following licensing terms. > +Redistribution and use in source and binary forms, with or without modification, > +are permitted provided that the following conditions are met: > + > +* Redistributions of source code must retain the above copyright notice, > + this list of conditions and the following disclaimer. > + > +* Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + > +* Neither the name of Marvell nor the names of its contributors may be > + used to endorse or promote products derived from this software without > + specific prior written permission. > + > +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND > +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR > +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON > +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > +*******************************************************************************/ > +#include "MvBoardDescDxe.h" > + > +MV_BOARD_DESC *mBoardDescInstance; > + > +STATIC > +EFI_STATUS > +MvBoardDescUtmiGet ( > + IN MARVELL_BOARD_DESC_PROTOCOL *This, > + IN OUT MV_BOARD_UTMI_DESC **UtmiDesc > + ) > +{ > + UINT8 *UtmiDeviceTable, *XhciDeviceTable, *UtmiPortType, UtmiCount; Neither of these pointers sound like they're pointing to character/byte strings - why are they UINT8*? (My guess is that these should all be VOID*.) UtmiCount should preferably be UINTN. > + UINTN UtmiDeviceTableSize, UtmiIndex, Index; > + MV_BOARD_UTMI_DESC *BoardDesc; > + MV_SOC_UTMI_DESC *SoCDesc; > + EFI_STATUS Status; > + > + /* Get SoC data about all available UTMI controllers */ > + Status = ArmadaSoCDescUtmiGet (&SoCDesc, &UtmiCount); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + /* Obtain table with enabled Utmi PHY's */ > + UtmiDeviceTable = (UINT8 *)PcdGetPtr (PcdUtmiControllersEnabled); Will this (and subsequent casts) be required if the pointers are VOID*? > + if (UtmiDeviceTable == NULL) { > + /* No UTMI PHY on platform */ > + return EFI_SUCCESS; > + } > + > + /* Make sure XHCI controllers table is present */ > + XhciDeviceTable = (UINT8 *)PcdGetPtr (PcdPciEXhci); > + if (XhciDeviceTable == NULL) { > + DEBUG ((DEBUG_ERROR, "%a: Missing PcdPciEXhci\n", __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + UtmiDeviceTableSize = PcdGetSize (PcdUtmiControllersEnabled); > + > + /* Check if PCD with UTMI PHYs is correctly defined */ > + if (UtmiDeviceTableSize > UtmiCount || > + UtmiDeviceTableSize > PcdGetSize (PcdPciEXhci)) { Please add some parentheses. > + DEBUG ((DEBUG_ERROR, > + "%a: Wrong PcdUtmiControllersEnabled format\n", > + __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + /* Obtain port type table */ > + UtmiPortType = (UINT8 *)PcdGetPtr (PcdUtmiPortType); > + if (UtmiPortType == NULL || > + PcdGetSize (PcdUtmiPortType) != UtmiDeviceTableSize) { Please add some parentheses. > + DEBUG ((DEBUG_ERROR, "%a: Wrong PcdUtmiPortType format\n", __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + /* Allocate and fill board description */ > + BoardDesc = AllocateZeroPool (UtmiDeviceTableSize * sizeof (MV_BOARD_UTMI_DESC)); > + if (BoardDesc == NULL) { > + DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__)); > + return EFI_OUT_OF_RESOURCES; > + } > + > + UtmiIndex = 0; > + for (Index = 0; Index < UtmiDeviceTableSize; Index++) { > + if (!MVHW_DEV_ENABLED (Utmi, Index)) { if (UtmiDeviceTable[Index] != MVHW_DEV_ENABLED) would read a lot better. > + continue; > + } > + > + /* UTMI PHY without enabled XHCI controller is useless */ > + if (!MVHW_DEV_ENABLED (Xhci, Index)) { Similarly, if (XhciDeviceTable[Index] != MVHW_DEV_ENABLED) > + DEBUG ((DEBUG_ERROR, > + "%a: Disabled Xhci controller %d\n", > + Index, > + __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + BoardDesc[UtmiIndex].SoC = &SoCDesc[Index]; > + BoardDesc[UtmiIndex].UtmiPortType = UtmiPortType[Index]; > + UtmiIndex++; > + } > + > + BoardDesc->UtmiDevCount = UtmiIndex; > + > + *UtmiDesc = BoardDesc; > + > + return EFI_SUCCESS; > +} > + > +STATIC > +VOID > +MvBoardDescFree ( > + IN VOID *BoardDesc > + ) > +{ > + if (BoardDesc != NULL) { Do we really want this test? What it ends up doing is getting rid of an assert if the function is called with a NULL pointer. And if so, why did we call it? > + FreePool (BoardDesc); > + } > +} > + > +STATIC > +EFI_STATUS > +MvBoardDescInitProtocol ( > + IN MARVELL_BOARD_DESC_PROTOCOL *BoardDescProtocol > + ) > +{ > + BoardDescProtocol->BoardDescUtmiGet = MvBoardDescUtmiGet; > + BoardDescProtocol->BoardDescFree = MvBoardDescFree; > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +EFIAPI > +MvBoardDescEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + mBoardDescInstance = AllocateZeroPool (sizeof (MV_BOARD_DESC)); > + if (mBoardDescInstance == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + MvBoardDescInitProtocol (&mBoardDescInstance->BoardDescProtocol); > + > + mBoardDescInstance->Signature = BOARD_DESC_SIGNATURE; > + > + Status = gBS->InstallMultipleProtocolInterfaces (&(mBoardDescInstance->Handle), > + &gMarvellBoardDescProtocolGuid, > + &(mBoardDescInstance->BoardDescProtocol)); > + if (EFI_ERROR (Status)) { > + FreePool (mBoardDescInstance); > + return Status; > + } > + > + return EFI_SUCCESS; > +} > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h > new file mode 100644 > index 0000000..47d9a72 > --- /dev/null > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.h > @@ -0,0 +1,59 @@ > +/******************************************************************************* > +Copyright (C) 2018 Marvell International Ltd. > +Marvell BSD License Option > + > +If you received this File from Marvell, you may opt to use, redistribute and/or > +modify this File under the following licensing terms. > +Redistribution and use in source and binary forms, with or without modification, > +are permitted provided that the following conditions are met: > + > +* Redistributions of source code must retain the above copyright notice, > + this list of conditions and the following disclaimer. > + > +* Redistributions in binary form must reproduce the above copyright > + notice, this list of conditions and the following disclaimer in the > + documentation and/or other materials provided with the distribution. > + > +* Neither the name of Marvell nor the names of its contributors may be > + used to endorse or promote products derived from this software without > + specific prior written permission. > + > +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND > +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR > +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON > +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > +*******************************************************************************/ > +#ifndef __MV_BOARD_DESC_H__ > +#define __MV_BOARD_DESC_H__ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > + > +#define BOARD_DESC_SIGNATURE SIGNATURE_64 ('B', 'O', 'A', 'R', 'D', 'D', 'S', 'C') Very generic sig for something that is still very specific. I'd be happier of the first two letters were 'M' and 'V'. > + > +typedef struct { > + MARVELL_BOARD_DESC_PROTOCOL BoardDescProtocol; > + UINTN Signature; > + EFI_HANDLE Handle; > + EFI_LOCK Lock; > +} MV_BOARD_DESC; > + > +#define MVHW_DEV_ENABLED(type, index) (type ## DeviceTable[index]) This macro obscures what's going on. Please get rid of it as per earlier comments (added after I got here). > + > +#endif // __MV_BOARD_DESC_H__ > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf > new file mode 100644 > index 0000000..9367833 > --- /dev/null > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf > @@ -0,0 +1,65 @@ > +# > +# Marvell BSD License Option > +# > +# If you received this File from Marvell, you may opt to use, redistribute > +# and/or modify this File under the following licensing terms. > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions are met: > +# > +# * Redistributions of source code must retain the above copyright notice, > +# this list of conditions and the following disclaimer. > +# > +# * Redistributions in binary form must reproduce the above copyright > +# notice, this list of conditions and the following disclaimer in the > +# documentation and/or other materials provided with the distribution. > +# > +# * Neither the name of Marvell nor the names of its contributors may be > +# used to endorse or promote products derived from this software without > +# specific prior written permission. > +# > +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" > +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE > +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR > +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER > +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, > +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > +# > + > +[Defines] > + INF_VERSION = 0x00010019 ...001A (just apply throughout before reposting). > + BASE_NAME = BoardDescDxe > + FILE_GUID = 4ed385f9-5d2c-4774-95c5-d5d9d70b3c37 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = MvBoardDescEntryPoint > + > +[Sources] > + MvBoardDescDxe.c > + MvBoardDescDxe.h > + > +[Packages] > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + Silicon/Marvell/Marvell.dec > + > +[LibraryClasses] > + ArmadaSoCDescLib > + DebugLib > + MemoryAllocationLib > + UefiDriverEntryPoint > + UefiLib > + > +[Protocols] > + gMarvellBoardDescProtocolGuid > + > +[Pcd] > + gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled > + gMarvellTokenSpaceGuid.PcdUtmiPortType > + gMarvellTokenSpaceGuid.PcdPciEXhci PcdP before PcdU. / Leif > + > +[Depex] > + TRUE > -- > 2.7.4 >