From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web12.6374.1592483758802779938 for ; Thu, 18 Jun 2020 05:35:59 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=zQmEOPCi; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id u26so6578151wmn.1 for ; Thu, 18 Jun 2020 05:35:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=eav1aCNinwclyDv2mC2PCUdpEJQKiZTvUp+RUlWljS8=; b=zQmEOPCivRDhUf/tydQ7Sgu6IxDJod3A+di3JUTqTxoEvojt7A+OkrRniW7UhJKDPs sgNI155sk+ROCCFtgMoENu+PZo4GhuopfaJSXEE8KYd59V/swsrQ0jB1PuzKt8ihQD3Q lQfR7g/snLDGnZqT68z9LOCr4lefcORy3VqtOAl84aHcVvwGCQPgaxse6Dy0IJhHZjQF X/nt4JMP776dlywPGNL/Nv5ZQ4ZlrvPvyzF/t2Cx6R7iIZJ9YmJVjoQumBL5tsOStuiv 19Ki4DOGYIuoT6XcL4McPCkUIGTQEPkKYmLS2iZyIHkU+AZonPHIqL5suwUJRcvWQHmL v5iQ== 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=eav1aCNinwclyDv2mC2PCUdpEJQKiZTvUp+RUlWljS8=; b=j38rvgBh/cF+kK0B4KBmPVdHwIXX0OD/1UCeBtY2Y6cU2PFd5N0d4hBIQpGSnoIkr8 Al0tiXfJLm1MWoaUBraP9CrgnmGuYOq85YObeCMGiw//CtYjaKm+gLPVrJWjnQhytPSO OhXsrvaevW+pPLxkOQWU9VtD0A5am5qkf+U4XCWFbyAspW4+XEUtwCtQy7oNrtzq9N8N l/9YWrrUQJtzpAXzzKWLv+8ErRSTcM6nx4LG7VC3w26UBnIwFkjoJh6PnwiKKLfygbR5 RjkKYUNyBsxlIPP2oN7r+XsZ7uW9bwnPDsK5qGQKNWKyeSySSoEXbazO4bFI5plF0ros Fnow== X-Gm-Message-State: AOAM531nPmAR09yjpBUpE5rDN59fpmn9+DGfjtE8w61RLs7gBTeyx4SH gnVFVPgg6UdP8MaPOZsE6r5KhA== X-Google-Smtp-Source: ABdhPJzG4zuxep4xyWsvgbFSKRPdPjwKIM9HPh2eyNrd9gN+8UNNxY4Fng/3PyiWhDbz541iVMwI8A== X-Received: by 2002:a7b:c456:: with SMTP id l22mr3767692wmi.120.1592483757189; Thu, 18 Jun 2020 05:35:57 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id d5sm3657749wrb.14.2020.06.18.05.35.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Jun 2020 05:35:56 -0700 (PDT) Date: Thu, 18 Jun 2020 13:35:54 +0100 From: "Leif Lindholm" To: Wasim Khan Cc: devel@edk2.groups.io, meenakshi.aggarwal@nxp.com, V.Sethi@nxp.com, ard.biesheuvel@arm.com, Wasim Khan Subject: Re: [PATCH edk2-platforms 3/7] Platform/NXP: LX2160aRdbPkg: Add PlatformDxe driver Message-ID: <20200618123554.GW6739@vanye> References: <1591741050-11645-1-git-send-email-wasim.khan@oss.nxp.com> <1591741050-11645-4-git-send-email-wasim.khan@oss.nxp.com> MIME-Version: 1.0 In-Reply-To: <1591741050-11645-4-git-send-email-wasim.khan@oss.nxp.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jun 10, 2020 at 03:47:26 +0530, Wasim Khan wrote: > From: Wasim Khan > > Add PlatformDxe to do platform specific work. > At present it perform platform specific Pci initialization. > Signed-off-by: Wasim Khan > --- > Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf | 35 +++++++++ > Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c | 78 ++++++++++++++++++++ > 2 files changed, 113 insertions(+) > > diff --git a/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf > new file mode 100644 > index 000000000000..2514adf1d69d > --- /dev/null > +++ b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf > @@ -0,0 +1,35 @@ > +## @file > +# > +# Copyright 2020 NXP > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010019 > + BASE_NAME = PlatformDxe > + FILE_GUID = C4063EBA-7729-11EA-BC55-0232AC130003 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = PlatformDxeEntryPoint > + > +[Sources] > + PlatformDxe.c > + > +[Packages] > + MdePkg/MdePkg.dec > + Silicon/NXP/Chassis3V2/Chassis3V2.dec > + Silicon/NXP/LX2160A/LX2160A.dec > + Silicon/NXP/NxpQoriqLs.dec > + > +[LibraryClasses] > + PcdLib > + UefiDriverEntryPoint > + > +[Pcd] > + gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable > + gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl > + > +[Depex] > + TRUE > diff --git a/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c > new file mode 100644 > index 000000000000..73599aaeb7bf > --- /dev/null > +++ b/Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c > @@ -0,0 +1,78 @@ > +/** @file > +* > +* Copyright 2020 NXP > +* > +* SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > +#include > +#include > +#include > + > +/** > + Enable PciCfgShift feature for LX2160-Rev2 ... which means what? > + > +**/ > +VOID > +EnableCfgShift ( But even more importantly, that isn't what this function does: > + VOID > + ) > +{ > + UINT32 Svr; > + > + Svr = SocGetSvr (); > + if ((SVR_SOC_VER(Svr) == SVR_LX2160A) && (SVR_MAJOR(Svr) == 0x2)) { ... it performs variant/version detection and sets feature flags based on it. The fact that it currently happens to be setting only a single flag shouldn't mean that in order to understand what PlatformPciInit() does even at a high level, I need to know what an "EnableCfgShift" is. The function should be called something like DetectSocVersion, EnableSocRevisionWorkarounds, or something other descriptive. *Then* there should be a comment here describing what PcdPciCfgShiftEnable is and what it does. > + PcdSetBoolS (PcdPciCfgShiftEnable, TRUE); > + } > +} > + > +/** > + Enable Layerscape Gen4 controller for LX2160A-Rev1 > + > +**/ > +VOID > +EnablePciController ( > + VOID > + ) > +{ > + UINT32 Svr; > + > + Svr = SocGetSvr (); > + if ((SVR_SOC_VER(Svr) == SVR_LX2160A) && (SVR_MAJOR(Svr) == 0x1)) { > + PcdSetBoolS (PcdPciLsGen4Ctrl, TRUE); Again, this doesn't *enable* a controller, which is what the function name says it does. It sets a Pcd, which is then used by subsequent code. I don't know if this function should be smashed together with the previous one under an even more generic name, but this too motivates a comment on what a PcdPciLsGen4Ctrl is. > + } > +} > + > +/** > + Platfrom Specific PCI Initialization Platform / Leif > + > +**/ > +VOID > +PlatformPciInit ( > + VOID > + ) > +{ > + EnableCfgShift (); > + EnablePciController (); > +} > + > +/** > + The entry point for PlatformDxe driver. This driver > + intends to perform platform specific initialization. > + > + @param[in] ImageHandle The image handle of the driver. > + @param[in] SystemTable The system table. > + > + @retval EFI_SUCCESS Driver initialization success. > + > +**/ > +EFI_STATUS > +EFIAPI > +PlatformDxeEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + // Platfrom Specific PCI Initialization > + PlatformPciInit (); > + return EFI_SUCCESS; > +} > -- > 2.7.4 >