Re: [edk2-devel] [edk2-platforms][PATCH V1 1/8] Platform/Sgi: Helper macros for PPTT Table

["PierreGondois" <pierre.gondois@arm.com>]

Hi Pranav,

> diff --git a/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h 
> b/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> index 8d715de173c9..7ceb090a78e9 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2018-2020, ARM Limited. All rights reserved.
> +*  Copyright (c) 2018-2021, ARM Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -20,6 +20,132 @@
>  #define EFI_ACPI_ARM_CREATOR_ID SIGNATURE_32('A','R','M',' ')
>  #define EFI_ACPI_ARM_CREATOR_REVISION 0x00000099
>
> +#define CORE_COUNT      FixedPcdGet32 (PcdCoreCount)
> +#define CLUSTER_COUNT   FixedPcdGet32 (PcdClusterCount)
> +
> +#pragma pack(1)
> +// PPTT processor core structure
> +typedef struct {
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR  Core;
> +  UINT32                                 Offset[2];

I think there should be 3 entries (DCache, ICache, L2Cache). Updating 
this will require updating the other PPTT tables written.

Would it be also possible to rename the field 'PrivateResources' as in 
the spec ?
Another question: what does 'RD_' stands for ?

> +  EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE      DCache;
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE      ICache;
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE      L2Cache;
> +} RD_PPTT_CORE;
> +
> +// PPTT processor cluster structure
> +typedef struct {
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR  Cluster;
> +  UINT32                                 Offset;
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE      L3Cache;
> +  RD_PPTT_CORE Core[CORE_COUNT];
> +} RD_PPTT_CLUSTER;
> +
> +// PPTT processor cluster structure without cache
> +typedef struct {
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR  Cluster;
> +  UINT32                                 Offset;
I think there is no need for an offset here. Updating this will require 
updating the other PPTT tables written.
> +  RD_PPTT_CORE Core[CORE_COUNT];
> +} RD_PPTT_MINIMAL_CLUSTER;
> +
> +// PPTT processor package structure
> +typedef struct {
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR  Package;
> +  UINT32                                 Offset;
> +  EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE      Slc;
> +  RD_PPTT_MINIMAL_CLUSTER Cluster[CLUSTER_COUNT];
> +} RD_PPTT_SLC_PACKAGE;
> +#pragma pack ()
> +
> +//
> +// PPTT processor structure flags for different SoC components as 
> defined in
> +// ACPI 6.3 specification
> +//
> +
[...]
>
> +// EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR
> +#define EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR_INIT(Length, Flag, 
> Parent,       \
> +  ACPIProcessorID, NumberOfPrivateResource) \

I think it should be possible to remove the 'Length' parameter and 
compute it as:
sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_PROCESSOR) +
NumberOfPrivateResource * sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE) +
NumberOfPrivateResource * sizeof (UINT32)

> + { \
> +    EFI_ACPI_6_3_PPTT_TYPE_PROCESSOR,                 /* Type 0 
> */             \
> +    Length,                                           /* Length 
> */             \
> + { \
> + EFI_ACPI_RESERVED_BYTE, \
> + EFI_ACPI_RESERVED_BYTE, \
> + }, \
> +    Flag,                                             /* Processor 
> flags */    \
> +    Parent,                                           /* Ref to 
> parent node */ \
> +    ACPIProcessorID,                                  /* UID, as per 
> MADT */   \
> +    NumberOfPrivateResource                           /* Resource 
> count */     \
> +  }
> +
> +// EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE
> +#define EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE_INIT(Flag, NextLevelCache, 
> Size,     \
> +  NoOfSets, Associativity, Attributes, 
> LineSize)                               \
> + { \
> +    EFI_ACPI_6_3_PPTT_TYPE_CACHE,                     /* Type 1 
> */             \
> +    sizeof (EFI_ACPI_6_3_PPTT_STRUCTURE_CACHE),       /* Length 
> */             \
> + { \
> + EFI_ACPI_RESERVED_BYTE, \
> + EFI_ACPI_RESERVED_BYTE, \
> + }, \
> +    Flag,                                             /* Cache flags 
> */        \
> +    NextLevelCache,                                   /* Ref to next 
> level */  \
> +    Size,                                             /* Size in 
> bytes */      \
> +    NoOfSets,                                         /* Num of sets 
> */        \
> +    Associativity,                                    /* Num of ways 
> */        \
> +    Attributes,                                       /* Cache 
> attributes */   \
> +    LineSize                                          /* Line size in 
> bytes */ \
> +  }
> +
>  #endif /* __SGI_ACPI_HEADER__ */
> -- 
> 2.17.1

Regards,

Pierre