From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::441; helo=mail-wr1-x441.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) (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 4B6FB210E3DC2 for ; Thu, 9 Aug 2018 03:27:59 -0700 (PDT) Received: by mail-wr1-x441.google.com with SMTP id g1-v6so4702360wru.2 for ; Thu, 09 Aug 2018 03:27:59 -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:content-transfer-encoding:in-reply-to :user-agent; bh=TmyvTMmc1/PE3yxj6LjRZ8rxdDVAHbGtZ92xdPaENxg=; b=I+YYnX8W7+eSiThDGtVfhj53XGaXgFGVF0WMc1xf4IDdBkLe7mdKA/sbLASceN04F6 Gwl9wF/W5J1YTgjpEQ7xdRK69xYM2AyQIID6UUxXlVXzv2pn64j+z7+yPvZiIuTzSgDI goaroB0Adv1O4d5+2EfzGz+qSq0k66wxIjjls= 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:content-transfer-encoding :in-reply-to:user-agent; bh=TmyvTMmc1/PE3yxj6LjRZ8rxdDVAHbGtZ92xdPaENxg=; b=XgGnqv2x9oKANc8H1Tl2FL/NRhGeVyXX4f0zIh/bWA54Na3GBskgUfoEsco3iRpcJU gEgxNE75nuSGDqk0r1Kc6Xzr/1YbLw6oBPIVFoMvx2iVsrb4L6ZwS7pvKrjH9GXISaTj MP/0Tw/oPkkPl2bxL8Fjcsb46hQqGfKiVwhxIf8SpMteFLhPLKwsMPOH2vH/GMVNXMAf 1iunDvNsntDBxvxnE071PcgYAVqKlV0ynTS0TVIzR5o4d7tVaEWFSWf+JoCBgVubjlDK pMr7RVdF1R8IcdzXHwBvF6ByUSfdeddrUNMk9OZsKIJR2Ol6x6lN73IZeK87xNT3K4sg AI8g== X-Gm-Message-State: AOUpUlF4mmk0o85dcO1AZlDIeAALp3N9eu9C0pI7VF7iQSlv17tU3brU RmBWDeHCB3tCG54uEnIV0pH4quzWAJU= X-Google-Smtp-Source: AA+uWPz+uCl87SSE9A/Xfxstns2H3SxjJUFYAtVVb31UsJ2ybV2Gct6izw97gON07G5qJKGWQ/fL4Q== X-Received: by 2002:adf:9c93:: with SMTP id d19-v6mr1022522wre.11.1533810477879; Thu, 09 Aug 2018 03:27:57 -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 p11-v6sm4561073wrs.51.2018.08.09.03.27.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 09 Aug 2018 03:27:56 -0700 (PDT) Date: Thu, 9 Aug 2018 11:27:54 +0100 From: Leif Lindholm To: Ming Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org, graeme.gregory@linaro.org, ard.biesheuvel@linaro.org, guoheyi@huawei.com, wanghuiqiang@huawei.com, huangming23@huawei.com, zhangjinsong2@huawei.com, huangdaode@hisilicon.com, john.garry@huawei.com, xinliang.liu@linaro.org, Heyi Guo Message-ID: <20180809102754.nzvblo5s3lqtmxqz@bivouac.eciton.net> References: <20180724070922.63362-1-ming.huang@linaro.org> <20180724070922.63362-28-ming.huang@linaro.org> <20180804095934.7dgxsy6ne7squzz7@bivouac.eciton.net> <5989d59b-204a-d220-19bc-2aa83dd1a1bc@linaro.org> MIME-Version: 1.0 In-Reply-To: <5989d59b-204a-d220-19bc-2aa83dd1a1bc@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 27/38] Platform/Hisilicon/D06: Add EarlyConfigPeim peim X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 10:28:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Aug 09, 2018 at 03:07:07PM +0800, Ming wrote: > 在 8/4/2018 5:59 PM, Leif Lindholm 写道: > > On Tue, Jul 24, 2018 at 03:09:11PM +0800, Ming Huang wrote: > >> This peim configuare SMMU,AP,MN. > > > > configuare -> configure > > > > I know SMMU and AP, but I don't know MN. > > MN: Miscellaneous Node > > > > > Hmm, also, 'AP' is a bit unfortunate to use in EDK2 context. > > PI specifies 'BSP' for Boot-strap Processor, as the one executing all > > of the EDK2 code. It then uses 'AP' to refer to Additional Processors, > > which can be assigned tasks using the EFI_MP_SERVICES_PROTOCOL. > > > > So I think in a TianoCore context, this should be 'BSP'. > > Agree, change it to BSP. Does that mean MN becomes AP? :) > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Ming Huang > >> Signed-off-by: Heyi Guo > >> --- > >> Platform/Hisilicon/D06/D06.dsc | 1 + > >> Platform/Hisilicon/D06/D06.fdf | 1 + > >> Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.c | 108 ++++++++++++++++++++ > >> Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.inf | 50 +++++++++ > >> 4 files changed, 160 insertions(+) > >> > >> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc > >> index 49322f8304..9e4f961116 100644 > >> --- a/Platform/Hisilicon/D06/D06.dsc > >> +++ b/Platform/Hisilicon/D06/D06.dsc > >> @@ -267,6 +267,7 @@ > >> MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf > >> MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > >> > >> + Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.inf > >> Silicon/Hisilicon/Drivers/VersionInfoPeim/VersionInfoPeim.inf > >> > >> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf { > >> diff --git a/Platform/Hisilicon/D06/D06.fdf b/Platform/Hisilicon/D06/D06.fdf > >> index e65dddd4e9..ec424d49ed 100644 > >> --- a/Platform/Hisilicon/D06/D06.fdf > >> +++ b/Platform/Hisilicon/D06/D06.fdf > >> @@ -359,6 +359,7 @@ READ_LOCK_STATUS = TRUE > >> INF ArmPkg/Drivers/CpuPei/CpuPei.inf > >> INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf > >> INF IntelFrameworkModulePkg/Universal/StatusCode/Pei/StatusCodePei.inf > >> + INF Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.inf > >> > >> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >> > >> diff --git a/Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.c b/Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.c > >> new file mode 100644 > >> index 0000000000..606cdf926a > >> --- /dev/null > >> +++ b/Platform/Hisilicon/D06/EarlyConfigPeim/EarlyConfigPeimD06.c > >> @@ -0,0 +1,108 @@ > >> +/** @file > >> +* > >> +* Copyright (c) 2018, Hisilicon Limited. All rights reserved. > >> +* Copyright (c) 2018, Linaro Limited. All rights reserved. > >> +* > >> +* This program and the accompanying materials > >> +* are licensed and made available under the terms and conditions of the BSD License > >> +* which accompanies this distribution. The full text of the license may be found at > >> +* http://opensource.org/licenses/bsd-license.php > >> +* > >> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > >> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > >> +* > >> +**/ > >> + > >> + > >> +#include > >> +#include // This header file should be on ahead > > > > Then please move it down instead of leaving a comment :) > > Maybe our comment is not accuracy, we want the PlatformArch.h should > be in front of the below Library/*.h, not in top. Why? I am happy for .h files to include other .h files that they need for their internal definitions - so that we don't need to worry about functional needs for include order. I would prefer for this to be resolved so that the comment is simply not needed. > > > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#define PERI_SUBCTRL_BASE (0x40000000) > >> +#define MDIO_SUBCTRL_BASE (0x60000000) > >> +#define PCIE2_SUBCTRL_BASE (0xA0000000) > >> +#define PCIE0_SUBCTRL_BASE (0xB0000000) > >> +#define ALG_BASE (0xD0000000) > >> + > >> +#define SC_BROADCAST_EN_REG (0x16220) > >> +#define SC_BROADCAST_SCL1_ADDR0_REG (0x16230) > >> +#define SC_BROADCAST_SCL1_ADDR1_REG (0x16234) > >> +#define SC_BROADCAST_SCL2_ADDR0_REG (0x16238) > >> +#define SC_BROADCAST_SCL2_ADDR1_REG (0x1623C) > >> +#define SC_BROADCAST_SCL3_ADDR0_REG (0x16240) > >> +#define SC_BROADCAST_SCL3_ADDR1_REG (0x16244) > >> +#define PCIE_SUBCTRL_SC_DISP_DAW_EN_REG (0x1000) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY3_REG (0x1010) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY4_REG (0x1014) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY5_REG (0x1018) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY6_REG (0x101C) > >> +#define PCIE_SUBCTRL_SC_REMAP_CTRL_REG (0x1200) > >> +#define SC_ITS_M3_INT_MUX_SEL_REG (0x21F0) > >> +#define SC_TM_CLKEN0_REG (0x2050) > >> + > >> +#define SC_TM_CLKEN0_REG_VALUE (0x3) > >> +#define SC_BROADCAST_EN_REG_VALUE (0x7) > >> +#define SC_BROADCAST_SCLx_ADDRx_REG_VALUE0 (0x0) > >> +#define SC_BROADCAST_SCLx_ADDRx_REG_VALUE1 (0x40016260) > >> +#define SC_BROADCAST_SCLx_ADDRx_REG_VALUE2 (0x60016260) > >> +#define SC_BROADCAST_SCLx_ADDRx_REG_VALUE3 (0x400) > >> +#define SC_ITS_M3_INT_MUX_SEL_REG_VALUE (0x7) > >> +#define PCIE_SUBCTRL_SC_REMAP_CTRL_REG_VALUE0 (0x0) > >> +#define PCIE_SUBCTRL_SC_DISP_DAW_EN_REG_VALUE0 (0x27) > >> +#define PCIE_SUBCTRL_SC_DISP_DAW_EN_REG_VALUE1 (0x2F) > >> +#define PCIE_SUBCTRL_SC_DISP_DAW_EN_REG_VALUE2 (0x77) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY3_REG_VALUE0 (0x178033) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY4_REG_VALUE0 (0x17003c) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY5_REG_VALUE0 (0x15003d) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY5_REG_VALUE1 (0x170035) > >> +#define PCIE_SUBCTRL_SC_DISPATCH_DAW_ARRAY6_REG_VALUE0 (0x16003e) > >> + > >> +VOID > >> +QResetAp ( > > > > Hmm. No function definitions in header files please. > > Move to .c file (and make STATIC). > > Sorry, I don't really understand here. This function is in .c > file already. Ah, so it is. My apologies. In that case, please move these #defines to a .h file :) > > > > >> + VOID > >> + ) > >> +{ > >> + MmioWrite64 (FixedPcdGet64 (PcdMailBoxAddress), 0x0); > >> + (void)WriteBackInvalidateDataCacheRange ( > > > > VOID > > > >> + (VOID *)FixedPcdGet64 (PcdMailBoxAddress), > >> + 8 > > > > sizeof (UINT64)? > > Yes > > > > >> + ); > >> + > >> + //SCCL A > >> + if (!PcdGet64 (PcdTrustedFirmwareEnable)) > >> + { > > > > That { goes at the end of the previous line. > > > >> + StartupAp (); > > > > Hmm. At some point, we want to rename that function, to align with my > > comments above. > > OK > > > > >> + } > >> +} > >> + > >> + > >> +EFI_STATUS > >> +EFIAPI > >> +EarlyConfigEntry ( > > > > No definitions in .h files. Move to .c file and make STATIC. > > Do you suggest add STATIC here? If it is only used in this file, yes please. / Leif