From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::241; helo=mail-pl0-x241.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x241.google.com (mail-pl0-x241.google.com [IPv6:2607:f8b0:400e:c01::241]) (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 CA1E7210EE4FB for ; Mon, 13 Aug 2018 19:31:16 -0700 (PDT) Received: by mail-pl0-x241.google.com with SMTP id ba4-v6so7686578plb.11 for ; Mon, 13 Aug 2018 19:31:16 -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=NetMzm+PjbPs3PSUekUNFZlF65Q+EE8kSXsADSsfuws=; b=T4T2i/f1oQ4Oq6Qu7OVcsjetEL6pnu792y2/23snsaitljOaoCLLBTFjeHaNi1vP6I vxKyyibkbDSQeVsdJ/d7kAuRlEynUzHm1spZLE2kxjQohqQ6oiJsfJbSztP9cl69nU97 WlKbQRQ0xX7J+Eet8HF0T7V1vOfkLDlxw1FII= 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=NetMzm+PjbPs3PSUekUNFZlF65Q+EE8kSXsADSsfuws=; b=GcoVOACqdMiwSGFJQp/jXmbwPxdBMbN1nka24SOIZs+6SYo7hIpqace/GyYvcbVCUQ dJFsuKOaqXtUvU/XMQf3IjcDXa3BI3xkJxhff8t792ANMT0Or0MzjneJBqZuwC6+T3In GCTFrIn5dHf+TIPzu04dpew5m3sMQKgT/+4+AsuOV9ZBaw9gaXQa2mgAe3h4kdvzHfyF rB6UrbEh3MibUsz9A6Xzb8ZwwinRK0vM75OZQ45UX9Pflxb37lALTBCsHhTz9dftKDlK gXS0HlwR9gpjphXsleqt/jPtGjYxPFCqLBrktI2T8QcVeh7aSTxqME2EH1fW7dT8kxwv 6+8g== X-Gm-Message-State: AOUpUlHgN7fQQT+/wN/0YmnYK82xwJLxfSOE/unClCJzAUw1OqKvUeFE 0qFxiF6g8bY5ImP37jAAD0QOVQ== X-Google-Smtp-Source: AA+uWPy24UusdzKPSlslkVlCXH+W8AG5fyaAc9VQp7nLxtRATEy9ebghvRQGSO1uSYzNRQXj7JLZFA== X-Received: by 2002:a17:902:4203:: with SMTP id g3-v6mr19039568pld.30.1534213876190; Mon, 13 Aug 2018 19:31:16 -0700 (PDT) Received: from [10.196.1.150] ([64.64.108.142]) by smtp.gmail.com with ESMTPSA id r22-v6sm33582075pfl.112.2018.08.13.19.31.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Aug 2018 19:31:15 -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: <37acfb11-2df0-73f7-53f5-d2dbc2f9255d@linaro.org> Date: Tue, 14 Aug 2018 10:31:02 +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: Tue, 14 Aug 2018 02:31:17 -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? :) > >>>> 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. >> As this function is use in several platforms, if I rename this function, D03/D05 should be rename in edk2-platforms and many functions in HwPkg should be rename. How about use the original name? >> >>> >>>> + } >>>> +} >>>> + >>>> + >>>> +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 >