From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=45.249.212.191; helo=huawei.com; envelope-from=huangming23@huawei.com; receiver=edk2-devel@lists.01.org Received: from huawei.com (szxga05-in.huawei.com [45.249.212.191]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1B39422333794 for ; Mon, 22 Jan 2018 21:55:24 -0800 (PST) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id 57D31B74C2BE4; Tue, 23 Jan 2018 14:00:35 +0800 (CST) Received: from [127.0.0.1] (10.61.17.224) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.361.1; Tue, 23 Jan 2018 14:00:27 +0800 To: Ard Biesheuvel , Ming Huang References: <1516287703-35516-1-git-send-email-huangming23@huawei.com> <1516287703-35516-2-git-send-email-huangming23@huawei.com> CC: Leif Lindholm , linaro-uefi , "edk2-devel@lists.01.org" , Graeme Gregory , , wanghuiqiang , Jason Zhang , Mengfanrong , From: "Huangming (Mark)" Message-ID: Date: Tue, 23 Jan 2018 14:00:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.61.17.224] X-CFilter-Loop: Reflected Subject: Re: [PATCH edk2-platforms v1 01/14] Hisilicon/D05: Add PPTT support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Jan 2018 05:55:26 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit On 2018/1/20 18:16, Ard Biesheuvel wrote: > On 18 January 2018 at 15:01, Ming Huang wrote: >> From: Jason Zhang >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Jason Zhang >> Signed-off-by: Ming Huang >> Signed-off-by: Heyi Guo >> --- >> Platform/Hisilicon/D05/D05.dsc | 1 + >> Platform/Hisilicon/D05/D05.fdf | 1 + >> Silicon/Hisilicon/Hi1616/D05AcpiTables/Hi1616Platform.h | 27 ++ >> Silicon/Hisilicon/Hi1616/D05AcpiTables/MadtHi1616.aslc | 31 +- >> Silicon/Hisilicon/Hi1616/Pptt/Pptt.c | 447 ++++++++++++++++++++ >> Silicon/Hisilicon/Hi1616/Pptt/Pptt.h | 142 +++++++ >> Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf | 55 +++ >> 7 files changed, 677 insertions(+), 27 deletions(-) >> >> diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc >> index 77a89fd..710339c 100644 >> --- a/Platform/Hisilicon/D05/D05.dsc >> +++ b/Platform/Hisilicon/D05/D05.dsc >> @@ -506,6 +506,7 @@ >> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf >> >> Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf >> + Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf >> Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf >> >> # >> diff --git a/Platform/Hisilicon/D05/D05.fdf b/Platform/Hisilicon/D05/D05.fdf >> index 78ab0c8..97de4d2 100644 >> --- a/Platform/Hisilicon/D05/D05.fdf >> +++ b/Platform/Hisilicon/D05/D05.fdf >> @@ -241,6 +241,7 @@ READ_LOCK_STATUS = TRUE >> INF Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf >> >> INF RuleOverride=ACPITABLE Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf >> + INF Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf >> INF Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf >> >> # >> diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Hi1616Platform.h b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Hi1616Platform.h >> index 808219a..f1927e8 100644 >> --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Hi1616Platform.h >> +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Hi1616Platform.h >> @@ -19,6 +19,7 @@ >> >> #ifndef _HI1610_PLATFORM_H_ >> #define _HI1610_PLATFORM_H_ >> +#include >> > > Empty line before ^^^ please > >> // >> // ACPI table information used to initialize tables. >> @@ -44,5 +45,31 @@ >> } >> >> #define HI1616_WATCHDOG_COUNT 2 >> +#define HI1616_GIC_STRUCTURE_COUNT 64 >> + >> +#define HI1616_MPID_TA_BASE 0x10000 >> +#define HI1616_MPID_TB_BASE 0x30000 >> +#define HI1616_MPID_TA_2_BASE 0x50000 >> +#define HI1616_MPID_TB_2_BASE 0x70000 >> + >> +// Differs from Juno, we have another affinity level beyond cluster and core >> +#define PLATFORM_GET_MPID_TA(ClusterId, CoreId) (HI1616_MPID_TA_BASE | ((ClusterId) << 8) | (CoreId)) >> +#define PLATFORM_GET_MPID_TB(ClusterId, CoreId) (HI1616_MPID_TB_BASE | ((ClusterId) << 8) | (CoreId)) >> +#define PLATFORM_GET_MPID_TA_2(ClusterId, CoreId) (HI1616_MPID_TA_2_BASE | ((ClusterId) << 8) | (CoreId)) >> +#define PLATFORM_GET_MPID_TB_2(ClusterId, CoreId) (HI1616_MPID_TB_2_BASE | ((ClusterId) << 8) | (CoreId)) >> + >> +// >> +// Multiple APIC Description Table >> +// >> +#pragma pack (1) >> + >> +typedef struct { >> + EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER Header; >> + EFI_ACPI_6_1_GIC_STRUCTURE GicInterfaces[HI1616_GIC_STRUCTURE_COUNT]; >> + EFI_ACPI_6_1_GIC_DISTRIBUTOR_STRUCTURE GicDistributor; >> + EFI_ACPI_6_1_GIC_ITS_STRUCTURE GicITS[8]; >> +} EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE; >> + >> +#pragma pack () >> >> #endif >> diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/MadtHi1616.aslc b/Silicon/Hisilicon/Hi1616/D05AcpiTables/MadtHi1616.aslc >> index 169ee72..33dca03 100644 >> --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/MadtHi1616.aslc >> +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/MadtHi1616.aslc >> @@ -1,9 +1,9 @@ >> /** @file >> * Multiple APIC Description Table (MADT) >> * >> -* Copyright (c) 2012 - 2014, ARM Limited. All rights reserved. >> -* Copyright (c) 2015 - 2016, Hisilicon Limited. All rights reserved. >> -* Copyright (c) 2015 - 2016, Linaro Limited. All rights reserved. >> +* Copyright (c) 2012 - 2018, ARM Limited. All rights reserved. >> +* Copyright (c) 2015 - 2018, Hisilicon Limited. All rights reserved. >> +* Copyright (c) 2015 - 2018, Linaro Limited. All rights reserved. > > Please don't touch the copyright statements belonging to other companies > >> * >> * This program and the accompanying materials >> * >> @@ -19,34 +19,11 @@ >> * >> **/ >> >> - >> -#include >> +#include "Hi1616Platform.h" >> #include >> #include >> #include >> #include >> -#include "Hi1616Platform.h" >> - >> -// Differs from Juno, we have another affinity level beyond cluster and core >> -// 0x20000 is only for socket 0 >> -#define PLATFORM_GET_MPID_TA(ClusterId, CoreId) (0x10000 | ((ClusterId) << 8) | (CoreId)) >> -#define PLATFORM_GET_MPID_TB(ClusterId, CoreId) (0x30000 | ((ClusterId) << 8) | (CoreId)) >> -#define PLATFORM_GET_MPID_TA_2(ClusterId, CoreId) (0x50000 | ((ClusterId) << 8) | (CoreId)) >> -#define PLATFORM_GET_MPID_TB_2(ClusterId, CoreId) (0x70000 | ((ClusterId) << 8) | (CoreId)) >> - >> -// >> -// Multiple APIC Description Table >> -// >> -#pragma pack (1) >> - >> -typedef struct { >> - EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER Header; >> - EFI_ACPI_6_1_GIC_STRUCTURE GicInterfaces[64]; >> - EFI_ACPI_6_1_GIC_DISTRIBUTOR_STRUCTURE GicDistributor; >> - EFI_ACPI_6_1_GIC_ITS_STRUCTURE GicITS[8]; >> -} EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE; >> - >> -#pragma pack () >> >> EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE Madt = { >> { >> diff --git a/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c >> new file mode 100644 >> index 0000000..eac4736 >> --- /dev/null >> +++ b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c >> @@ -0,0 +1,447 @@ >> +/** @file >> +* >> +* Copyright (c) 2017, Hisilicon Limited. All rights reserved. >> +* Copyright (c) 2017, 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. >> +* >> +* Based on the files under ArmPlatformPkg/ArmJunoPkg/AcpiTables/ >> +* >> +**/ >> + >> +#include "Pptt.h" >> + >> +EFI_ACPI_TABLE_PROTOCOL *mAcpiTableProtocol = NULL; >> +EFI_ACPI_SDT_PROTOCOL *mAcpiSdtProtocol = NULL; >> + >> +EFI_ACPI_DESCRIPTION_HEADER mPpttHeader = >> + ARM_ACPI_HEADER ( >> + EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_SIGNATURE, >> + EFI_ACPI_DESCRIPTION_HEADER, >> + EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_REVISION >> + ); >> + >> +EFI_ACPI_6_2_PPTT_TYPE2 mPpttSocketType2[PPTT_SOCKET_COMPONENT_NO] = >> +{ >> + {2, sizeof(EFI_ACPI_6_2_PPTT_TYPE2), 0, 0, 0, 0, 0, 0, 0} >> +}; >> + >> +EFI_ACPI_6_2_PPTT_TYPE1 mPpttCacheType1[PPTT_CACHE_NO] = >> +{ >> + {1, sizeof(EFI_ACPI_6_2_PPTT_TYPE1), 0, 0, 0, 0, 0, 0, 0, 0}, //L1I 48K 0xC000 CacheAssociativity8Way >> + {1, sizeof(EFI_ACPI_6_2_PPTT_TYPE1), 0, 0, 0, 0, 0, 0, 0, 0}, //L1D 32k 0x8000 CacheAssociativity8Way >> + {1, sizeof(EFI_ACPI_6_2_PPTT_TYPE1), 0, 0, 0, 0, 0, 0, 0, 0}, //L2 1M 0x100000 CacheAssociativity8Way >> + {1, sizeof(EFI_ACPI_6_2_PPTT_TYPE1), 0, 0, 0, 0x1000000, 0x2000, 0x10, 0x0A, 0x80} //L3 16M 0x1000000 CacheAssociativity16Way Linesize-128byte >> +}; >> + > > Please make all of these STATIC ^^^ > > And functions below as well > >> +EFI_STATUS >> +InitCacheInfo( >> + ) >> +{ >> + UINT8 Index; >> + PPTT_TYPE1_ATTRIBUTES Type1Attributes; >> + CSSELR_DATA CsselrData; >> + CCSIDR_DATA CcsidrData; >> + >> + for (Index = 0; Index < PPTT_CACHE_NO - 1; Index++) { >> + CsselrData.Data = 0; >> + CcsidrData.Data = 0; >> + Type1Attributes.Data = 0; >> + >> + if (Index == 0) { //L1I > > space after // > >> + CsselrData.Bits.InD = 1; >> + CsselrData.Bits.Level = 0; >> + Type1Attributes.Bits.CacheType = 1; >> + } else if (Index == 1) { >> + Type1Attributes.Bits.CacheType = 0; >> + CsselrData.Bits.Level = Index -1; > > space after - > >> + } else { >> + Type1Attributes.Bits.CacheType = 2; >> + CsselrData.Bits.Level = Index -1; > > and here > >> + } >> + >> + CcsidrData.Data = ReadCCSIDR (CsselrData.Data); >> + >> + if (CcsidrData.Bits.Wa == 1) { >> + Type1Attributes.Bits.AllocateType = 1; >> + if (CcsidrData.Bits.Ra == 1) { >> + Type1Attributes.Bits.AllocateType++; > > Just assign '2' here. BTW don't we have #defines for these constants? > >> + } >> + } >> + >> + if (CcsidrData.Bits.Wt == 1) { >> + Type1Attributes.Bits.WritePolicy = 1; >> + } >> + DEBUG ((DEBUG_INFO, "[Acpi PPTT] Level = %x!CcsidrData = %x!\n",CsselrData.Bits.Level, CcsidrData.Data)); >> + >> + mPpttCacheType1[Index].NumberOfSets = (UINT16)CcsidrData.Bits.NumSets + 1; >> + mPpttCacheType1[Index].Associativity = (UINT16)CcsidrData.Bits.Associativity + 1; >> + mPpttCacheType1[Index].LineSize = (UINT16)( 1 << (CcsidrData.Bits.LineSize + 4)); >> + mPpttCacheType1[Index].Size = mPpttCacheType1[Index].LineSize * \ >> + mPpttCacheType1[Index].Associativity * \ >> + mPpttCacheType1[Index].NumberOfSets; >> + mPpttCacheType1[Index].Attributes = Type1Attributes.Data; >> + mPpttCacheType1[Index].Flags = PPTT_TYPE1_SIZE_VALID | PPTT_TYPE1_NUMBER_OF_SETS_VALID | PPTT_TYPE1_ASSOCIATIVITY_VALID | \ >> + PPTT_TYPE1_ALLOCATION_TYPE_VALID | PPTT_TYPE1_CACHE_TYPE_VALID | PPTT_TYPE1_WRITE_POLICY_VALID | \ >> + PPTT_TYPE1_LINE_SIZE_VALID; >> + >> + } >> + >> + // L3 >> + mPpttCacheType1[3].Flags = PPTT_TYPE1_SIZE_VALID | PPTT_TYPE1_NUMBER_OF_SETS_VALID | PPTT_TYPE1_ASSOCIATIVITY_VALID | \ >> + PPTT_TYPE1_ALLOCATION_TYPE_VALID | PPTT_TYPE1_CACHE_TYPE_VALID | PPTT_TYPE1_WRITE_POLICY_VALID | \ >> + PPTT_TYPE1_LINE_SIZE_VALID; >> + > > Where do you assign mPpttCacheType1[3].Size/Attributes/... ? > >> + return EFI_SUCCESS; >> +} >> + > > STATIC > >> +EFI_STATUS >> +AddCoreTable( >> + IN VOID *PpttTable, >> + IN OUT VOID *PpttTableLengthRemain, >> + IN UINT32 Flags, >> + IN UINT32 Parent, >> + IN UINT32 ResourceNo, >> + IN UINT32 ProcessorId > > Please align like > > IN VOID *PpttTable, > IN OUT VOID *PpttTableLengthRemain, > IN UINT32 Flags, > >> + ) >> +{ >> + EFI_ACPI_6_2_PPTT_TYPE0 *PpttType0; >> + EFI_ACPI_6_2_PPTT_TYPE1 *PpttType1; >> + UINT32 *PrivateResource; >> + UINT8 Index; >> + >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4) { > > If *PpttTableLengthRemain is a UINT32 then use a UINT32* as the > function parameter not VOID*, and drop the cast here. > >> + return EFI_OUT_OF_RESOURCES; >> + } >> + PpttType0 = (EFI_ACPI_6_2_PPTT_TYPE0 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); > > Please cast PpttTable to UINT8* before adding to it. I will add UINT8 * to it, and I will define PpttTable to EFI_ACPI_DESCRIPTION_HEADER * to avoid cast repeatedly. > >> + PpttType0->Type = 0; >> + PpttType0->Flags = Flags; >> + PpttType0->Parent= Parent; >> + PpttType0->AcpiProcessorId = ProcessorId; >> + PpttType0->PrivateResourceNo = ResourceNo; >> + PpttType0->Length = sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4; > > Space after sizeof > >> + >> + *(UINT32 *)PpttTableLengthRemain -= (UINTN)PpttType0->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType0->Length; >> + PrivateResource = (UINT32 *)((UINT8 *)PpttType0 + sizeof(EFI_ACPI_6_2_PPTT_TYPE0)); >> + >> + // Add cache type structure >> + for (Index = 0; Index < ResourceNo; Index++, PrivateResource++) { >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE1)) { > > space after sizeof > >> + return EFI_OUT_OF_RESOURCES; >> + } >> + *PrivateResource = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length; >> + PpttType1 = (EFI_ACPI_6_2_PPTT_TYPE1 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + gBS->CopyMem (PpttType1, &mPpttCacheType1[Index], sizeof(EFI_ACPI_6_2_PPTT_TYPE1)); >> + *(UINT32 *)PpttTableLengthRemain -= PpttType1->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType1->Length; >> + } >> + >> + return EFI_SUCCESS; >> +} >> + > > STATIC > >> +EFI_STATUS >> +AddClusterTable ( >> + IN VOID *PpttTable, >> + IN OUT VOID *PpttTableLengthRemain, >> + IN UINT32 Flags, >> + IN UINT32 Parent, >> + IN UINT32 ResourceNo >> + ) >> +{ > > Alignment as above > >> + EFI_ACPI_6_2_PPTT_TYPE0 *PpttType0; >> + EFI_ACPI_6_2_PPTT_TYPE1 *PpttType1; >> + UINT32 *PrivateResource; >> + >> + if ((*(UINT32 *)PpttTableLengthRemain) < (sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4)) { > > Use a UINT32* type for PpttTableLengthRemain > >> + return EFI_OUT_OF_RESOURCES; >> + } >> + PpttType0 = (EFI_ACPI_6_2_PPTT_TYPE0 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); > > Use UINT8* cast for PpttTable > >> + PpttType0->Type = 0; >> + PpttType0->Flags = Flags; >> + PpttType0->Parent= Parent; >> + PpttType0->PrivateResourceNo = ResourceNo; >> + PpttType0->Length = sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4; >> + >> + *(UINT32 *)PpttTableLengthRemain -= PpttType0->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType0->Length; >> + PrivateResource = (UINT32 *)((UINT8 *)PpttType0 + sizeof(EFI_ACPI_6_2_PPTT_TYPE0)); >> + >> + // Add cache type structure >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE1)) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + *PrivateResource = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length; >> + PpttType1 = (EFI_ACPI_6_2_PPTT_TYPE1 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + gBS->CopyMem (PpttType1, &mPpttCacheType1[2], sizeof(EFI_ACPI_6_2_PPTT_TYPE1)); >> + *(UINT32 *)PpttTableLengthRemain -= PpttType1->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType1->Length; >> + >> + return EFI_SUCCESS; >> +} >> + > > Same comments apply to AddScclTable() > >> +EFI_STATUS >> +AddScclTable( >> + IN VOID *PpttTable, >> + IN OUT VOID *PpttTableLengthRemain, >> + IN UINT32 Flags, >> + IN UINT32 Parent, >> + IN UINT32 ResourceNo >> + ) >> +{ >> + EFI_ACPI_6_2_PPTT_TYPE0 *PpttType0; >> + EFI_ACPI_6_2_PPTT_TYPE1 *PpttType1; >> + UINT32 *PrivateResource; >> + >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + PpttType0 = (EFI_ACPI_6_2_PPTT_TYPE0 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + PpttType0->Type = 0; >> + PpttType0->Flags = Flags; >> + PpttType0->Parent= Parent; >> + PpttType0->PrivateResourceNo = ResourceNo; >> + PpttType0->Length = sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4; >> + >> + *(UINT32 *)PpttTableLengthRemain -= PpttType0->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType0->Length; >> + PrivateResource = (UINT32 *)((UINT8 *)PpttType0 + sizeof(EFI_ACPI_6_2_PPTT_TYPE0)); >> + >> + // Add cache type structure >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE1)) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + *PrivateResource = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length; >> + PpttType1 = (EFI_ACPI_6_2_PPTT_TYPE1 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + gBS->CopyMem (PpttType1, &mPpttCacheType1[3], sizeof(EFI_ACPI_6_2_PPTT_TYPE1)); >> + *(UINT32 *)PpttTableLengthRemain -= PpttType1->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType1->Length; >> + >> + return EFI_SUCCESS; >> +} >> + >> +EFI_STATUS >> +AddSocketTable( >> + IN VOID *PpttTable, >> + IN OUT VOID *PpttTableLengthRemain, >> + IN UINT32 Flags, >> + IN UINT32 Parent, >> + IN UINT32 ResourceNo >> + ) >> +{ >> + EFI_ACPI_6_2_PPTT_TYPE0 *PpttType0; >> + EFI_ACPI_6_2_PPTT_TYPE2 *PpttType2; >> + UINT32 *PrivateResource; >> + UINT8 Index; >> + >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE0)) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + PpttType0 = (EFI_ACPI_6_2_PPTT_TYPE0 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + PpttType0->Type = 0; >> + PpttType0->Flags = Flags; >> + PpttType0->Parent= Parent; >> + PpttType0->PrivateResourceNo = ResourceNo; >> + PpttType0->Length = sizeof(EFI_ACPI_6_2_PPTT_TYPE0) + ResourceNo * 4; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType0->Length; >> + >> + *(UINT32 *)PpttTableLengthRemain -= PpttType0->Length; >> + if (*(UINT32 *)PpttTableLengthRemain < ResourceNo * 4) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + PrivateResource = (UINT32 *)((UINT8 *)PpttType0 + sizeof(EFI_ACPI_6_2_PPTT_TYPE0)); >> + DEBUG ((DEBUG_INFO, "[Acpi PPTT] sizeof(EFI_ACPI_6_2_PPTT_TYPE2) = %x!\n", sizeof(EFI_ACPI_6_2_PPTT_TYPE2))); >> + >> + for (Index = 0; Index < ResourceNo; Index++, PrivateResource++) { >> + if (*(UINT32 *)PpttTableLengthRemain < sizeof(EFI_ACPI_6_2_PPTT_TYPE2)) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + *PrivateResource = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length; >> + PpttType2 = (EFI_ACPI_6_2_PPTT_TYPE2 *)(PpttTable + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); >> + gBS->CopyMem (PpttType2, &mPpttSocketType2[Index], sizeof(EFI_ACPI_6_2_PPTT_TYPE2)); >> + *(UINT32 *)PpttTableLengthRemain -= PpttType2->Length; >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length += PpttType2->Length; >> + } >> + >> + return EFI_SUCCESS; >> +} >> + > > The functions above look very similar. Would it be possible to merge them? > > STATIC > >> +VOID >> +GetApic( >> +EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE *ApicTable, >> +VOID *PpttTable, >> +IN UINT32 PpttTableLengthRemain, >> +IN UINT32 Index1 >> +) >> +{ >> + UINT32 IndexSocket, IndexSccl, IndexCulster, IndexCore; > > cluster not culster > >> + UINT32 SocketOffset, ScclOffset, ClusterOffset; >> + UINT32 Parent = 0; >> + UINT32 Flags = 0; >> + UINT32 ResourceNo = 0; > > Empty line > >> + //Get APIC data > > Space after // > >> + for (IndexSocket = 0; IndexSocket < PPTT_SOCKET_NO; IndexSocket++) { >> + SocketOffset = 0; >> + for (IndexSccl = 0; IndexSccl < PPTT_DIE_NO; IndexSccl++) { >> + ScclOffset = 0; >> + for (IndexCulster = 0; IndexCulster < PPTT_CULSTER_NO; IndexCulster++) { >> + ClusterOffset = 0; >> + for (IndexCore = 0; IndexCore < PPTT_CORE_NO; IndexCore++) { >> + >> + DEBUG ((DEBUG_INFO, "[Acpi PPTT] IndexSocket:%x, IndexSccl:%x, IndexCulster:%x, IndexCore:%x!\n",IndexSocket,IndexSccl ,IndexCulster,IndexCore)); >> + >> + if (ApicTable->GicInterfaces[Index1].AcpiProcessorUid != Index1) { >> + //This processor is unusable > > Space after // > >> + DEBUG ((DEBUG_ERROR, "[Acpi PPTT] Please check MADT table for UID!\n")); >> + return; >> + } >> + if ((ApicTable->GicInterfaces[Index1].Flags & BIT0) == 0 ) { >> + //This processor is unusable > > and here > >> + Index1++; >> + continue; >> + } >> + >> + if (SocketOffset == 0) { >> + //Add socket0 for type0 table > > and here, plus indentation > >> + ResourceNo = PPTT_SOCKET_COMPONENT_NO; >> + SocketOffset = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length; >> + Parent = 0; >> + Flags = PPTT_TYPE0_SOCKET_FLAG; >> + AddSocketTable (PpttTable, &PpttTableLengthRemain, Flags, Parent, ResourceNo); >> + } >> + if (ScclOffset == 0) { >> + //Add socket0die0 for type0 table > > and here > >> + ResourceNo = 1; >> + ScclOffset = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length ; > > No space before ; > >> + Parent = SocketOffset; >> + Flags = PPTT_TYPE0_DIE_FLAG; >> + AddScclTable (PpttTable, &PpttTableLengthRemain, Flags, Parent, ResourceNo); >> + } >> + if (ClusterOffset == 0) { >> + //Add socket0die0ClusterId for type0 table > > Space after // and indentation > > >> + ResourceNo = 1; >> + ClusterOffset = ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length ; >> + Parent = ScclOffset; >> + Flags = PPTT_TYPE0_CLUSTER_FLAG; >> + AddClusterTable (PpttTable, &PpttTableLengthRemain, Flags, Parent, ResourceNo); >> + } >> + >> + //Add socket0die0ClusterIdCoreId for type0 table > > and here > >> + ResourceNo = 2; >> + Parent = ClusterOffset; >> + Flags = PPTT_TYPE0_CORE_FLAG; >> + AddCoreTable (PpttTable, &PpttTableLengthRemain, Flags, Parent, ResourceNo, Index1); >> + >> + Index1++; >> + } >> + } >> + } >> + } >> + return ; >> +} >> + > > STATIC > >> +VOID >> +PpttSetAcpiTable( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + UINTN AcpiTableHandle; >> + EFI_STATUS Status; >> + UINT8 Checksum; >> + EFI_ACPI_SDT_HEADER *Table; >> + EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE *ApicTable; >> + EFI_ACPI_TABLE_VERSION TableVersion; >> + VOID *PpttTable; >> + UINTN TableKey; >> + UINT32 Index0, Index1; >> + UINT32 PpttTableLengthRemain = 0; >> + >> + gBS->CloseEvent (Event); >> + >> + InitCacheInfo (); >> + >> + PpttTable = AllocateZeroPool (PPTT_TABLE_MAX_LEN); >> + gBS->CopyMem (PpttTable, &mPpttHeader, sizeof(EFI_ACPI_DESCRIPTION_HEADER)); >> + PpttTableLengthRemain = PPTT_TABLE_MAX_LEN - sizeof(EFI_ACPI_DESCRIPTION_HEADER); >> + >> + for (Index0 = 0; Index0 < EFI_ACPI_MAX_NUM_TABLES; Index0++) { >> + Status = mAcpiSdtProtocol->GetAcpiTable (Index0, &Table, &TableVersion, &TableKey); >> + if (EFI_ERROR (Status)) { >> + break; >> + } >> + //Find APIC table > > Space after // > >> + if (Table->Signature != EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE) { >> + continue; >> + } >> + >> + ApicTable = (EFI_ACPI_6_1_MULTIPLE_APIC_DESCRIPTION_TABLE *)Table; >> + Index1 = 0; >> + >> + GetApic (ApicTable, PpttTable, PpttTableLengthRemain, Index1); >> + break; >> + } >> + >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR,"%a:%d Status=%r\n",__FILE__,__LINE__,Status)); > > Does it make sense to proceed here? > >> + } >> + >> + Checksum = CalculateCheckSum8 ((UINT8 *)(PpttTable), ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length); > > No () around PpttTable > Line length (please check throughout) > >> + ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Checksum= Checksum; > > Space before = > >> + >> + AcpiTableHandle = 0; >> + Status = mAcpiTableProtocol->InstallAcpiTable (mAcpiTableProtocol, PpttTable, ((EFI_ACPI_DESCRIPTION_HEADER *)PpttTable)->Length, &AcpiTableHandle); >> + > > Line length? > >> + FreePool (PpttTable); >> + return ; >> +} >> + > > STATIC > >> +EFI_STATUS >> +InitPpttTable( > > Space before ( > Missing VOID > >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_EVENT ReadyToBootEvent; >> + >> + Status = EfiCreateEventReadyToBootEx ( >> + TPL_NOTIFY, >> + PpttSetAcpiTable, >> + NULL, >> + &ReadyToBootEvent >> + ); > > Indentation > > Also, can you just move this call to EfiCreateEventReadyToBootEx() > into the function below? > >> + ASSERT_EFI_ERROR (Status); >> + >> + return Status; >> +} >> + >> +EFI_STATUS >> +EFIAPI >> +PpttEntryPoint( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + Status = gBS->LocateProtocol (&gEfiAcpiTableProtocolGuid, NULL, (VOID**)&mAcpiTableProtocol); > > Space between VOID and ** > Line length > >> + if (EFI_ERROR (Status)) { >> + return EFI_ABORTED; >> + } >> + >> + Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID**) &mAcpiSdtProtocol); > > No space after (VOID **) > > Also, you have a DEPEX on both protocols, so it is sufficient to use > ASSERT_EFI_ERROR() here > >> + if (EFI_ERROR (Status)) { >> + return EFI_ABORTED; >> + } >> + >> + InitPpttTable (); >> + >> + DEBUG ((DEBUG_INFO, "Acpi Pptt init done.\n")); >> + >> + return EFI_SUCCESS; >> +} >> diff --git a/Silicon/Hisilicon/Hi1616/Pptt/Pptt.h b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.h >> new file mode 100644 >> index 0000000..5dc635f >> --- /dev/null >> +++ b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.h >> @@ -0,0 +1,142 @@ >> +/** @file >> +* >> +* Copyright (c) 2017, Hisilicon Limited. All rights reserved. >> +* Copyright (c) 2017, 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. >> +* >> +* Based on the files under ArmPlatformPkg/ArmJunoPkg/AcpiTables/ >> +* >> +**/ >> + >> +#ifndef _PPTT_H_ >> +#define _PPTT_H_ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "../D05AcpiTables/Hi1616Platform.h" >> + >> +/// >> +/// "PPTT" Processor Properties Topology Table >> +/// >> +#define EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_SIGNATURE SIGNATURE_32('P', 'P', 'T', 'T') >> +#define EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_REVISION 0x01 >> +#define EFI_ACPI_MAX_NUM_TABLES 20 >> + >> +#define PPTT_TABLE_MAX_LEN 0x6000 >> +#define PPTT_SOCKET_NO 0x2 >> +#define PPTT_DIE_NO 0x2 >> +#define PPTT_CULSTER_NO 0x4 >> +#define PPTT_CORE_NO 0x4 >> +#define PPTT_SOCKET_COMPONENT_NO 0x1 >> +#define PPTT_CACHE_NO 0x4 >> + >> +#define PPTT_TYPE0_PHYSICAL_PKG BIT0 >> +#define PPTT_TYPE0_PROCESSORID_VALID BIT1 >> +#define PPTT_TYPE0_SOCKET_FLAG PPTT_TYPE0_PHYSICAL_PKG >> +#define PPTT_TYPE0_DIE_FLAG PPTT_TYPE0_PHYSICAL_PKG >> +#define PPTT_TYPE0_CLUSTER_FLAG 0 >> +#define PPTT_TYPE0_CORE_FLAG PPTT_TYPE0_PROCESSORID_VALID >> + >> +#define PPTT_TYPE1_SIZE_VALID BIT0 >> +#define PPTT_TYPE1_NUMBER_OF_SETS_VALID BIT1 >> +#define PPTT_TYPE1_ASSOCIATIVITY_VALID BIT2 >> +#define PPTT_TYPE1_ALLOCATION_TYPE_VALID BIT3 >> +#define PPTT_TYPE1_CACHE_TYPE_VALID BIT4 >> +#define PPTT_TYPE1_WRITE_POLICY_VALID BIT5 >> +#define PPTT_TYPE1_LINE_SIZE_VALID BIT6 >> + >> +typedef union { >> + struct { >> + UINT32 InD :1; >> + UINT32 Level :3; >> + UINT32 Reserved :28; >> + } Bits; >> + UINT32 Data; >> +}CSSELR_DATA; > > Space after } > >> + >> +typedef union { >> + struct { >> + UINT32 LineSize :3; >> + UINT32 Associativity :10; >> + UINT32 NumSets :15; >> + UINT32 Wa :1; >> + UINT32 Ra :1; >> + UINT32 Wb :1; >> + UINT32 Wt :1; >> + } Bits; >> + UINT32 Data; >> +}CCSIDR_DATA; > > and here > >> + >> +// >> +// Processor Hierarchy Node Structure >> +// >> +typedef struct { >> + UINT8 Type; >> + UINT8 Length; >> + UINT16 Reserved; >> + UINT32 Flags; >> + UINT32 Parent; >> + UINT32 AcpiProcessorId; >> + UINT32 PrivateResourceNo; >> +} EFI_ACPI_6_2_PPTT_TYPE0; >> + >> +// >> +// Cache Configuration >> +// >> +typedef union { >> + struct { >> + UINT8 AllocateType :2; >> + UINT8 CacheType :2; >> + UINT8 WritePolicy :1; >> + UINT8 Reserved :3; >> + } Bits; >> + UINT8 Data; >> +}PPTT_TYPE1_ATTRIBUTES; > > and here > >> + >> +// >> +// Cache Type Structure >> +// >> +typedef struct { >> + UINT8 Type; >> + UINT8 Length; >> + UINT16 Reserved; >> + UINT32 Flags; >> + UINT32 NextLevelOfCache; >> + UINT32 Size; >> + UINT32 NumberOfSets; >> + UINT8 Associativity; >> + UINT8 Attributes; >> + UINT16 LineSize; >> +} EFI_ACPI_6_2_PPTT_TYPE1; >> + >> +// >> +// ID Structure >> +// >> +typedef struct { >> + UINT8 Type; >> + UINT8 Length; >> + UINT16 Reserved; >> + UINT32 VendorId; >> + UINT64 Level1Id; >> + UINT64 Level2Id; >> + UINT16 MajorRev; >> + UINT16 MinorRev; >> + UINT16 SpinRev; >> +} EFI_ACPI_6_2_PPTT_TYPE2; >> + >> +#endif // _PPTT_H_ >> + >> diff --git a/Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf >> new file mode 100644 >> index 0000000..ce26b97 >> --- /dev/null >> +++ b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf >> @@ -0,0 +1,55 @@ >> +/** @file >> +* >> +* Copyright (c) 2017, Hisilicon Limited. All rights reserved. >> +* Copyright (c) 2017, Linaro Limited. All rights reserved. >> +* > > You should probably update these now > >> +* 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. >> +* >> +* Based on the files under ArmPlatformPkg/ArmJunoPkg/AcpiTables/ >> +* >> +**/ >> + >> +[Defines] >> + INF_VERSION = 0x00010005 > > 0x0000001A > >> + BASE_NAME = AcpiPptt >> + FILE_GUID = AAB14F90-DC2E-4f33-A594-C7894A5B412D >> + MODULE_TYPE = DXE_DRIVER >> + VERSION_STRING = 1.0 >> + ENTRY_POINT = PpttEntryPoint >> + >> +[Sources.common] >> + Pptt.c >> + Pptt.h >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + edk2-platforms/Silicon/Hisilicon/HisiPkg.dec >> + ArmPkg/ArmPkg.dec >> + >> +[LibraryClasses] >> + ArmLib >> + HobLib >> + UefiRuntimeServicesTableLib >> + UefiDriverEntryPoint >> + BaseMemoryLib >> + DebugLib >> + >> +[Guids] >> + > > Please remove empty sections > >> + >> +[Protocols] >> + gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED >> + gEfiAcpiSdtProtocolGuid > > Please use the annotation consistently: > use ## not # > annotate all protocols > >> + >> +[Pcd] >> + >> + >> +[Depex] >> + gEfiAcpiTableProtocolGuid AND gEfiAcpiSdtProtocolGuid >> + >> -- >> 1.9.1 >> > > . > -- Best Regards, Ming