From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::441; helo=mail-pf1-x441.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf1-x441.google.com (mail-pf1-x441.google.com [IPv6:2607:f8b0: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 8477B210E3DCE for ; Thu, 9 Aug 2018 04:55:04 -0700 (PDT) Received: by mail-pf1-x441.google.com with SMTP id a26-v6so2747400pfo.4 for ; Thu, 09 Aug 2018 04:55:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=JFpAyRXsZv1stm8id7yOxMzoEoydIRwVlGADaba/zWE=; b=iTHeY/4v3XgEyE7noSN/Nd/IPoU+rxenI/yJLjXz7dTTs68UXpILg4aAtt4bPDOBbe 7W4q6/9URM/uYganLXkDhoipz7Brpz0HbI1Y2F1HRKB3qRdJnqoquejxeb0+PRxwaAB+ MR3em/ZJ7jPoYFmEd6Ad+L099GWjwJAuhOu4k= 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-transfer-encoding; bh=JFpAyRXsZv1stm8id7yOxMzoEoydIRwVlGADaba/zWE=; b=aOdT9CZR+Hc2OGIFyJHMJfKFIJshaRrI0skhTrpXU2mytzc74Sl8i/3chZUO+fVjBC 9ar5WFYLpa+xsvA/76YDw/UVV8A2R1sw9WE0BaOswvMS3fL5i2rYjAxOAmdjjaqx7Txe 8iEUcJYqF9fkv6qcsFZXvcnwCGvUOQeBtGuS4Tx2UUGzGsDIcHPLvIXNDiYXx9rYgbGF GVvu08iFfOtyWxY5yfvBrLYd/qJvsuss7ybXnzrvq4ig2u3B147qRDTMTE71RhNo2uf7 NbnE/Gb0AJuTbpv7qapvXL684FUgqcDILslPq/re0BcWMU3ks1gKTS0atrjyK9qWCeHs PXeg== X-Gm-Message-State: AOUpUlGxMSoGc9ku4qtNH9pWX8dfnlthuMIXo53rgK/7w56Iu4TFg/LM 1wLJs10uI3rD3XnxLjQqnUbUg8RTRfw= X-Google-Smtp-Source: AA+uWPxhESbwo30xgKerPpECyWmvtZeKxKwofiI7GEMTF2WLT7PxPvP0qiGSZl6HnzOAgKzvyO0o2w== X-Received: by 2002:a62:57dc:: with SMTP id i89-v6mr2048333pfj.65.1533815704138; Thu, 09 Aug 2018 04:55:04 -0700 (PDT) Received: from [10.199.0.182] ([64.64.108.224]) by smtp.gmail.com with ESMTPSA id l3-v6sm8496989pff.8.2018.08.09.04.54.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 09 Aug 2018 04:55:03 -0700 (PDT) To: Leif Lindholm 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 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> <20180809102754.nzvblo5s3lqtmxqz@bivouac.eciton.net> From: Ming Message-ID: Date: Thu, 9 Aug 2018 19:54:47 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180809102754.nzvblo5s3lqtmxqz@bivouac.eciton.net> 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 11:55:04 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 在 8/9/2018 6:27 PM, Leif Lindholm 写道: > 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? :) MN is not a processor. > >>>> 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. As your suggestion, after adding PlatformArch.h to OemAddressMapLib.h, this line can be delete. > >>> >>>> +#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 :) OK > >> >>> >>>> + 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 >