Re: [Linaro-uefi] [PATCH edk2-platforms v1 01/14] Hisilicon/D05: Add PPTT support

On 18 January 2018 at 15:01, Ming Huang heyi.guo@linaro.org wrote:

From: Jason Zhang zhangjinsong2@huawei.com

Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jason Zhang zhangjinsong2@huawei.com Signed-off-by: Ming Huang huangming23@huawei.com Signed-off-by: Heyi Guo heyi.guo@linaro.org

---

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 <IndustryStandard/Acpi.h>

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 <IndustryStandard/Acpi.h> +#include "Hi1616Platform.h" #include <Library/AcpiLib.h> #include <Library/AcpiNextLib.h> #include <Library/ArmLib.h> #include <Library/PcdLib.h> -#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 <IndustryStandard/Acpi.h> +#include <Library/ArmLib/ArmLibPrivate.h> +#include <Library/BaseMemoryLib.h> +#include <Library/DebugLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/UefiLib.h> +#include <Protocol/AcpiSystemDescriptionTable.h> +#include <Protocol/AcpiTable.h> +#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