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

["Huangming (Mark)" <huangming23@huawei.com>]

On 2018/1/20 18:16, Ard Biesheuvel wrote:
> 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/... ?
Other fields are used with initial value.
> 
>> +  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
>>
> 
> .
> 

-- 
Best Regards,

Ming