Re: [edk2-platforms] [PATCH v1 2/3] Platform/ARM/SgiPkg: Add HMAT ACPI table for RdN1EdgeX2

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Vijay,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com> 
Sent: 28 January 2021 01:43 PM
To: devel@edk2.groups.io; leif@nuviainc.com; ardb+tianocore@kernel.org; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Thomas Abraham <thomas.abraham@arm.com>
Subject: [edk2-platforms] [PATCH v1 2/3] Platform/ARM/SgiPkg: Add HMAT ACPI table for RdN1EdgeX2

Add HMAT table support for RD-N1-Edge Dual-chip platform.

Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
---
 Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf |   1 +
 Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc     | 108 ++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
index d44f02ab0c16..36d41281439d 100644
--- a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2AcpiTables.inf
@@ -22,6 +22,7 @@ [Sources]
   Iort.aslc
   Mcfg.aslc
   RdN1Edge/Dsdt.asl
+  RdN1EdgeX2/Hmat.aslc
   RdN1EdgeX2/Madt.aslc
   RdN1EdgeX2/Srat.aslc
   Spcr.aslc
diff --git a/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc
new file mode 100644
index 000000000000..29d089aed053
--- /dev/null
+++ b/Platform/ARM/SgiPkg/AcpiTables/RdN1EdgeX2/Hmat.aslc
@@ -0,0 +1,108 @@
+/** @file
+*  Heterogeneous Memory Attribute Table (HMAT)
+*
+*  Copyright (c) 2020, ARM Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include "SgiAcpiHeader.h"
+#include "SgiPlatform.h"
+#include <IndustryStandard/Acpi.h>
+#include <Library/AcpiLib.h>
+#include <Library/ArmLib.h>
[SAMI] Use <Sgixx.h> for Sgi headers. See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/55_preprocessor_directives#5-5-1-1-use-the-proper-file-delimiters-when-including-files
Also, it should be possible to move the Sgi headers here.
[/SAMI]
+
+#define CHIP_CNT                      FixedPcdGet32 (PcdChipCount)
+#define INITATOR_PROXIMITY_DOMAIN_CNT 2
+#define TARGET_PROXIMITY_DOMIAIN_CNT  2
+
+//
+// HMAT Table
+//
+#pragma pack (1)
+
+typedef struct {
+  UINT32  InitatorProximityDomain[INITATOR_PROXIMITY_DOMAIN_CNT];
+  UINT32  TargetProximityDomiain[TARGET_PROXIMITY_DOMIAIN_CNT];
+  UINT16  MatrixEntry[INITATOR_PROXIMITY_DOMAIN_CNT * TARGET_PROXIMITY_DOMIAIN_CNT];
+} InitiatorTargetProximityMatrix;
[SAMI] Please follow structure typedef and tag naming convention. 
See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/4_naming_conventions/45_name_space_rules#4-5-1-names-shall-be-used-consistently-within-the-same-type
[/SAMI]
+
+typedef struct {
+  EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_HEADER                Header;
+  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES          Proximity[CHIP_CNT];
+  EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_INFO  LatencyInfo;
+  InitiatorTargetProximityMatrix                                          Matrix;
+  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO                      MemSideCache0;
+  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO                      MemSideCache1;
+} EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE;
+
+#pragma pack ()
+
+#define HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT(           \
+  TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
+    )                                                                          \
[SAMI] Align ). It would be preferable to follow the alignment style of a function. Indeed, a space before the starting parenthesis would not work.
[/SAMI]

+{                                                                              \
+  TotalCacheLevels, CacheLevel, CacheAssociativity, WritePolicy, CacheLineSize \
+}
+
+EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE Hmat = {
+  // Header
+  {
+    ARM_ACPI_HEADER (
+      EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_SIGNATURE,
+      EFI_ACPI_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE,
+      EFI_ACPI_6_3_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_REVISION
+    ),
+    {
+      EFI_ACPI_RESERVED_BYTE,
+      EFI_ACPI_RESERVED_BYTE,
+      EFI_ACPI_RESERVED_BYTE,
+      EFI_ACPI_RESERVED_BYTE
+    },
+  },
+
+  // Memory Proximity Domain
+  {
+    EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES_INIT (
+      1, 0x0, 0x0),
+    EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_PROXIMITY_DOMAIN_ATTRIBUTES_INIT (
+      1, 0x1, 0x1),
+   },
+
+  // Latency Info
+  EFI_ACPI_6_3_HMAT_STRUCTURE_SYSTEM_LOCALITY_LATENCY_AND_BANDWIDTH_INFO_INIT (
+    0, 0, INITATOR_PROXIMITY_DOMAIN_CNT, TARGET_PROXIMITY_DOMIAIN_CNT, 100),
+  {
+    {0, 1}, {0, 1},
+    {
+      10, 20,
+      20, 10,
+    }
+  },
+
+  // Memory Side Cache
+  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_INIT (
+    0x0, SIZE_8MB,
[SAMI] Alignment.  See https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_source_files/52_spacing#5-2-2-4-subsequent-lines-of-multi-line-function-calls-should-line-up-two-spaces-from-the-beginning-of-the-function-name
[/SAMI]
+    HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT (
+      1,
+      1,
+      2,
+      2,
+      64 // 64 bytes cache line length
+    ),
+    0),
+
+  EFI_ACPI_6_3_HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_INIT (
+    0x1, SIZE_8MB,
+    HMAT_STRUCTURE_MEMORY_SIDE_CACHE_INFO_CACHE_ATTRIBUTES_INIT (
+      1,
+      1,
+      2,
+      2,
+      64 // 64 bytes cache line length
+    ),
+    0),
+};
+
+VOID* CONST ReferenceAcpiTable = &Hmat;
[SAMI] Add a comment to indicate that the above line is to prevent the optimiser from discarding the table.
[/SAMI]
-- 
2.17.1