From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (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 0B468221F93D8 for ; Sat, 20 Jan 2018 02:10:46 -0800 (PST) Received: by mail-it0-x243.google.com with SMTP id x42so4859034ita.4 for ; Sat, 20 Jan 2018 02:16:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=QSfabFETJ1vwpiClhSY+TStydIaIuin+COCGV3vaAak=; b=Y+eNjlwKfAIJqCssESuAY8UuVMd7yL6CYrzy1UyY+9oZCDyLvw9Q/LRI8FKWduW9kD LRGpnpcmyX65LL3fjZuh/NLjt4ujxWoJIf7lKwOYojdGgRObOsxjr1TC6OSfLs4oMfV9 +Yda5z0M05TuHhhm+SBjFUyqZf7GRXobYCqDg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=QSfabFETJ1vwpiClhSY+TStydIaIuin+COCGV3vaAak=; b=GXYdU/KMFwKOIDNcq7cl02DYh6EW7W/HPOR+4Pi+vC2z28KJEf+snUhH4oLxXOmwrR UQqUUbx+nM1r9ZoEOWyH/P4XAo3V9BzNliJtMBzkSX/PMZ0I587Ke3aA2nnNlPEzgUlT 5TVuY9/wysir2zDrkZP01N8to9EEA1QSjMOThsOqapCrcheFYUbZ9QrQ34hFtVI1IfKO x61gChdBbYpsw7e7lHVFUmB7JUA7FyUK7hAzi2eIb7kPXJA/EYQWeLm1UyLaWoaAKUbQ hHoxSuPOTEFNKoQcbTFez+OhK0sjzQplrtqHaLwMa4dcVhK2bZIakSKC78e8pQ+JSgx9 A55A== X-Gm-Message-State: AKwxytfB26vhNPs+hvi916iUo9DhyCgQp2qgNvV3Er9xnjXo+euB8Ru9 iYLJjCqfLM9Eh+2E8aU12OD9MBqY/1FSOeQ+RO1ICQ== X-Google-Smtp-Source: AH8x226pgULFsTnimXzPmiPKaF0trDFpaalQvyjhkNYhlcH51leY4ssi/Cn09QOYAzaZppfZpicv4unTJgoi8gs3h2k= X-Received: by 10.36.207.87 with SMTP id y84mr1091839itf.59.1516443369036; Sat, 20 Jan 2018 02:16:09 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Sat, 20 Jan 2018 02:16:08 -0800 (PST) In-Reply-To: <1516287703-35516-2-git-send-email-huangming23@huawei.com> References: <1516287703-35516-1-git-send-email-huangming23@huawei.com> <1516287703-35516-2-git-send-email-huangming23@huawei.com> From: Ard Biesheuvel Date: Sat, 20 Jan 2018 10:16:08 +0000 Message-ID: To: Ming Huang Cc: Leif Lindholm , linaro-uefi , "edk2-devel@lists.01.org" , Graeme Gregory , guoheyi@huawei.com, wanghuiqiang , huangming , Jason Zhang , Mengfanrong , waip23@126.com 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: Sat, 20 Jan 2018 10:10:48 -0000 Content-Type: text/plain; charset="UTF-8" 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. > + 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 >