[PATCH edk2-platforms v2 0/8] fixes and updates for AMD OverDrive

[PATCH edk2-platforms v2 0/8] fixes and updates for AMD OverDrive

[Ard Biesheuvel]

Fix some issues in the ACPI and DT descriptions of the SMMU routing,
in particular the routing of the CCP crypto accelerator, which sits
behind an SMMU as well on B1 silicon (but not on B0, strangely enough)

Changes since v1:
- add Leif's ack to patches #1, #2 and #4
- add patches to fix some errors and inaccuracies in the DT
- update the DT generation code to emit interrupt affinity for the PMU node
- update the DT generation code to emit a description of the cache topology
- stop emitting the obsolete linux,phandle properties

Ard Biesheuvel (8):
  Platform/Overdrive: add missing resolution for FileHandleLib
  Platform/Overdrive: clean up stream ID descriptions in DT
  Platform/Overdrive: fix a typo in the DT
  Silicon/AMD/Styx: clean up stream ID mappings for SMMU
  Silicon/AMD/StyxDtbLoaderLib: add interrupt-affinity property to PMU
    node
  Silicon/AMD/StyxDtbLoaderLib: add description of the cache topology
  Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic
    ARMv8
  Silicon/AMD/StyxDtbLoaderLib: omit linux,phandle properties

 .../DeviceTree/OverdriveBoard.dts             |  25 ++--
 .../AMD/OverdriveBoard/OverdriveBoard.dsc     |   2 +-
 .../Drivers/AcpiPlatformDxe/AcpiPlatform.c    |   5 -
 .../Styx/Drivers/AcpiPlatformDxe/Iort.aslc    | 117 ++++--------------
 .../StyxDtbLoaderLib/StyxDtbLoaderLib.c       | 113 ++++++++++++-----
 5 files changed, 120 insertions(+), 142 deletions(-)

-- 
2.17.1

[PATCH edk2-platforms v2 1/8] Platform/Overdrive: add missing resolution for FileHandleLib

[Ard Biesheuvel]

Changes to the core EDK2 repository have caused the build for
Overdrive to break. Move the existing FileHandleLib resolution
to global scope to get things working again.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
index 7369173cc125..5a38b9dd96ae 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
@@ -142,6 +142,7 @@ [LibraryClasses.common]
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
   FileExplorerLib|MdeModulePkg/Library/FileExplorerLib/FileExplorerLib.inf
+  FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
 
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
@@ -697,7 +698,6 @@ [Components.common]
       PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
       BcfgCommandLib|ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.inf
 
-      FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
       ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
 
     <PcdsFixedAtBuild>
-- 
2.17.1

[PATCH edk2-platforms v2 2/8] Platform/Overdrive: clean up stream ID descriptions in DT

[Ard Biesheuvel]

Align the DT description of the SMMU topology and stream IDs with the
actual routing of the SoC. As with the preceding IORT change, this is
mostly a cleanup exercise, but it does actually fix an issue with the
CCP crypto accelerator on B1 silicon.

Since the CCP shares its SMMU with the second SATA controller, which
is only enabled on B1 silicon, we can drop the logic that disables
this SMMU on B0 silicon or on platforms that do not expose any SATA
ports on the second controller (such as the Cello).

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts    | 23 +++++++++++++++-----
 Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c |  6 +----
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
index 1ba0d403eaf0..1d8a6caafd82 100644
--- a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
+++ b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
@@ -86,7 +86,7 @@
 				       */
 				      <0 332 4>,
 				      <0 332 4>;
-			#iommu-cells = <2>;
+			#iommu-cells = <1>;
 			dma-coherent;
 		};
 
@@ -99,7 +99,7 @@
 				       */
 				      <0 331 4>,
 				      <0 331 4>;
-			#iommu-cells = <2>;
+			#iommu-cells = <1>;
 			dma-coherent;
 		};
 
@@ -109,7 +109,12 @@
 			interrupts = <0x0 0x163 0x4>;
 			clocks = <&sata_clk>;
 			dma-coherent;
-			iommus = <&sata0_smmu 0x00 0x1f>; /* 0-31 */
+			iommus = <&sata0_smmu 0x0a>,
+				 <&sata0_smmu 0x0b>,
+				 <&sata0_smmu 0x0e>,
+				 <&sata0_smmu 0x0f>,
+				 <&sata0_smmu 0x1a>,
+				 <&sata0_smmu 0x1e>;
 		};
 
 		sata@e0d00000 {
@@ -119,7 +124,9 @@
 			interrupts = <0x0 0x162 0x4>;
 			clocks = <&sata_clk>;
 			dma-coherent;
-			iommus = <&sata1_smmu 0x00 0x1f>; /* 0-31 */
+			iommus = <&sata1_smmu 0x0e>,
+				 <&sata1_smmu 0x0f>,
+				 <&sata1_smmu 0x1e>;
 		};
 
 		i2c@e1000000 {
@@ -233,6 +240,10 @@
 			interrupts = <0x0 0x3 0x4>;
 			dma-coherent;
 			amd,zlib-support = <0x1>;
+			iommus = <&sata1_smmu 0x00>,
+				 <&sata1_smmu 0x02>,
+				 <&sata1_smmu 0x40>,
+				 <&sata1_smmu 0x42>;
 		};
 
 		pcie: pcie@f0000000 {
@@ -409,7 +420,7 @@
 			phy-handle = <&xgmac0_phy>;
 			phy-mode = "xgmii";
 			dma-coherent;
-			iommus = <&xgmac0_smmu 0x00 0x1f>; /* 0-31 */
+			iommus = <&xgmac0_smmu 0x00 0x17>; /* 0-7, 16-23 */
 		};
 
 		xgmac@e0900000 {
@@ -428,7 +439,7 @@
 			phy-handle = <&xgmac1_phy>;
 			phy-mode = "xgmii";
 			dma-coherent;
-			iommus = <&xgmac1_smmu 0x00 0x1f>; /* 0-31 */
+			iommus = <&xgmac1_smmu 0x00 0x17>; /* 0-7, 16-23 */
 		};
 	};
 
diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index c84c1a81c3ec..261b5f59c8df 100644
--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -212,8 +212,6 @@ DisableSmmu (
 
   Node = fdt_path_offset (Fdt, SmmuNodeName);
   if (Node <= 0) {
-    DEBUG ((DEBUG_WARN, "%a: Failed to find path %s: %a\n",
-      __FUNCTION__, SmmuNodeName, fdt_strerror (Node)));
     return;
   }
 
@@ -251,9 +249,7 @@ SetSocIdStatus (
   if (!PcdGetBool (PcdEnableSmmus)) {
     DisableSmmu (Fdt, "iommu-map", "/smb/smmu@e0a00000", "/smb/pcie@f0000000");
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0200000", "/smb/sata@e0300000");
-  }
-
-  if (!PcdGetBool (PcdEnableSmmus) || !IsRevB1 || FixedPcdGet8 (PcdSata1PortCount) == 0) {
+    DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/ccp@e0100000");
     DisableSmmu (Fdt, "iommus", "/smb/smmu@e0c00000", "/smb/sata@e0d00000");
   }
 
-- 
2.17.1

[PATCH edk2-platforms v2 3/8] Platform/Overdrive: fix a typo in the DT

[Ard Biesheuvel]

DT unit addresses are hex quantities but they should not include
the 0x prefix.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
index 1d8a6caafd82..a92ab695fb2e 100644
--- a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
+++ b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
@@ -291,7 +291,7 @@
 			dma-coherent;
 		};
 
-		ccn@0xe8000000 {
+		ccn@e8000000 {
 			compatible = "arm,ccn-504";
 			reg = <0x0 0xe8000000 0x0 0x1000000>;
 			interrupts = <0x0 0x17c 0x4>;
-- 
2.17.1

[PATCH edk2-platforms v2 4/8] Silicon/AMD/Styx: clean up stream ID mappings for SMMU

[Ard Biesheuvel]

Tighten the stream ID mappings for the SMMU to only cover the stream IDs
that are actually being issued by the respective masters. This is
mostly just a cleanup exercise, since specifying unused stream IDs does
not typically create any problems. However, the CCP crypto accelerator
on B1 silicon actually uses stream IDs that we assigned to the second
SATA controller, so there this actually fixes a problem.

Since the crypto accelerator shares its SMMU with the second AHCI
controller, we can drop the logic that hides the associated IORT
node when running on B0 silicon.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/AcpiPlatform.c |   5 -
 Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc      | 117 ++++----------------
 2 files changed, 22 insertions(+), 100 deletions(-)

diff --git a/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/AcpiPlatform.c b/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/AcpiPlatform.c
index 743ef0f65523..7c267542db19 100644
--- a/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/AcpiPlatform.c
+++ b/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/AcpiPlatform.c
@@ -123,7 +123,6 @@ InstallSystemDescriptionTables (
   UINTN                                               TableSize;
   UINTN                                               TableHandle;
   EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE        *Gtdt;
-  EFI_ACPI_6_0_IO_REMAPPING_TABLE                     *Iort;
   EFI_ACPI_5_1_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER *Madt;
   EFI_ACPI_5_1_GIC_STRUCTURE                          *GicC;
   UINT8                         MacPackage[sizeof(mDefaultMacPackageA)];
@@ -177,10 +176,6 @@ InstallSystemDescriptionTables (
         if (!PcdGetBool (PcdEnableSmmus)) {
           continue;
         }
-        if ((CpuId & STYX_SOC_VERSION_MASK) < STYX_SOC_VERSION_B1) {
-          Iort = (EFI_ACPI_6_0_IO_REMAPPING_TABLE *)Table;
-          Iort->NumNodes -= 2;
-        }
         break;
 
       case EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
diff --git a/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc b/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc
index d46be49f0318..7bdc34f6737e 100644
--- a/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc
+++ b/Silicon/AMD/Styx/Drivers/AcpiPlatformDxe/Iort.aslc
@@ -46,7 +46,7 @@ typedef struct {
 typedef struct {
   EFI_ACPI_6_0_IO_REMAPPING_NAMED_COMP_NODE Node;
   CONST CHAR8                               Name[11];
-  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE        RcIdMapping[32];
+  EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE        RcIdMapping[16];
 } STYX_NC_NODE;
 
 typedef struct {
@@ -63,6 +63,7 @@ typedef struct {
   STYX_NC_NODE                              Sata0NamedNode;
   STYX_SMMU_NODE                            Sata1SmmuNode;
   STYX_NC_NODE                              Sata1NamedNode;
+  STYX_NC_NODE                              CcpNamedNode;
 } STYX_IO_REMAPPING_STRUCTURE;
 
 #define __STYX_SMMU_NODE(Base, Size, Irq)                   \
@@ -114,14 +115,18 @@ typedef struct {
     EFI_ACPI_IORT_ID_MAPPING_FLAGS_SINGLE                   \
   }
 
-#define __STYX_NAMED_COMPONENT_NODE(Name)                   \
+#define __STYX_NUM_NODES(Type)                              \
+  ((sizeof(Type) - FIELD_OFFSET(Type, RcIdMapping)) /       \
+              sizeof(EFI_ACPI_6_0_IO_REMAPPING_ID_TABLE))
+
+#define __STYX_NAMED_COMPONENT_NODE(Name, Num)              \
     {                                                       \
       {                                                     \
         EFI_ACPI_IORT_TYPE_NAMED_COMP,                      \
         sizeof(STYX_NC_NODE),                               \
         0x0,                                                \
         0x0,                                                \
-        0x20,                                               \
+        Num,                                                \
         FIELD_OFFSET(STYX_NC_NODE, RcIdMapping),            \
       },                                                    \
       0x0,                                                  \
@@ -139,7 +144,7 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
     AMD_ACPI_HEADER(EFI_ACPI_6_0_IO_REMAPPING_TABLE_SIGNATURE,
                     STYX_IO_REMAPPING_STRUCTURE,
                     EFI_ACPI_IO_REMAPPING_TABLE_REVISION),
-    10,                                             // NumNodes
+    11,                                             // NumNodes
     sizeof(EFI_ACPI_6_0_IO_REMAPPING_TABLE),        // NodeOffset
     0                                               // Reserved
   }, {
@@ -175,7 +180,7 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
                      STYX_ETH0_SMMU_INTERRUPT)
   }, {
     // Eth0NamedNode
-    __STYX_NAMED_COMPONENT_NODE("\\_SB_.ETH0"),
+    __STYX_NAMED_COMPONENT_NODE("\\_SB_.ETH0", 16),
     {
       __STYX_ID_MAPPING_SINGLE(0x00, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x01, Eth0SmmuNode),
@@ -185,14 +190,6 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
       __STYX_ID_MAPPING_SINGLE(0x05, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x06, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x07, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x08, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x09, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0A, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0B, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0C, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0D, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0E, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0F, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x10, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x11, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x12, Eth0SmmuNode),
@@ -201,14 +198,6 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
       __STYX_ID_MAPPING_SINGLE(0x15, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x16, Eth0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x17, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x18, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x19, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1A, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1B, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1C, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1D, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1E, Eth0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1F, Eth0SmmuNode),
     }
   }, {
     // Eth1SmmuNode
@@ -217,7 +206,7 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
                      STYX_ETH1_SMMU_INTERRUPT)
   }, {
     // Eth1NamedNode
-    __STYX_NAMED_COMPONENT_NODE("\\_SB_.ETH1"),
+    __STYX_NAMED_COMPONENT_NODE("\\_SB_.ETH1", 16),
     {
       __STYX_ID_MAPPING_SINGLE(0x00, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x01, Eth1SmmuNode),
@@ -227,14 +216,6 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
       __STYX_ID_MAPPING_SINGLE(0x05, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x06, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x07, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x08, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x09, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0A, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0B, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0C, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0D, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0E, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0F, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x10, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x11, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x12, Eth1SmmuNode),
@@ -243,14 +224,6 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
       __STYX_ID_MAPPING_SINGLE(0x15, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x16, Eth1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x17, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x18, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x19, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1A, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1B, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1C, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1D, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1E, Eth1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1F, Eth1SmmuNode),
     }
   }, {
     // Sata0SmmuNode
@@ -259,40 +232,14 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
                      STYX_SATA0_SMMU_INTERRUPT)
   }, {
     // Sata0NamedNode
-    __STYX_NAMED_COMPONENT_NODE("\\_SB_.AHC0"),
+    __STYX_NAMED_COMPONENT_NODE("\\_SB_.AHC0", 6),
     {
-      __STYX_ID_MAPPING_SINGLE(0x00, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x01, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x02, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x03, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x04, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x05, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x06, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x07, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x08, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x09, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0A, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0B, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0C, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0D, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0E, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0F, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x10, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x11, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x12, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x13, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x14, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x15, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x16, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x17, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x18, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x19, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x1A, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1B, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1C, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1D, Sata0SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x1E, Sata0SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1F, Sata0SmmuNode),
     }
   }, {
     // Sata1SmmuNode
@@ -301,40 +248,20 @@ STATIC STYX_IO_REMAPPING_STRUCTURE AcpiIort = {
                      STYX_SATA1_SMMU_INTERRUPT)
   }, {
     // Sata1NamedNode
-    __STYX_NAMED_COMPONENT_NODE("\\_SB_.AHC1"),
+    __STYX_NAMED_COMPONENT_NODE("\\_SB_.AHC1", 3),
     {
-      __STYX_ID_MAPPING_SINGLE(0x00, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x01, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x02, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x03, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x04, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x05, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x06, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x07, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x08, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x09, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0A, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0B, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0C, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x0D, Sata1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0E, Sata1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x0F, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x10, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x11, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x12, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x13, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x14, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x15, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x16, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x17, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x18, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x19, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1A, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1B, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1C, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1D, Sata1SmmuNode),
       __STYX_ID_MAPPING_SINGLE(0x1E, Sata1SmmuNode),
-      __STYX_ID_MAPPING_SINGLE(0x1F, Sata1SmmuNode),
+    }
+  }, {
+    // CcpNamedNode
+    __STYX_NAMED_COMPONENT_NODE("\\_SB_.CCP0", 4),
+    {
+      __STYX_ID_MAPPING_SINGLE(0x00, Sata1SmmuNode),
+      __STYX_ID_MAPPING_SINGLE(0x02, Sata1SmmuNode),
+      __STYX_ID_MAPPING_SINGLE(0x40, Sata1SmmuNode),
+      __STYX_ID_MAPPING_SINGLE(0x42, Sata1SmmuNode),
     }
   }
 };
-- 
2.17.1

[PATCH edk2-platforms v2 5/8] Silicon/AMD/StyxDtbLoaderLib: add interrupt-affinity property to PMU node

[Ard Biesheuvel]

AMD Seattle uses a range of SPIs to signal PMU events, and this requires
a description in the DT which SPI maps to which CPU. This requires us to
defer the generation of the PMU node to a point where the CPU phandles
have been allocated.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 55 ++++++++++----------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index 261b5f59c8df..2f7b5e2a7b25 100644
--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -331,33 +331,6 @@ PrepareFdt (
   // Get Id from primary CPU
   MpId = (UINTN)ArmReadMpidr ();
 
-  // Create /pmu node
-  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
-  if (PmuNode >= 0) {
-    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
-
-    // append PMU interrupts
-    for (Index = 0; Index < ArmCoreCount; Index++) {
-      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
-                              ArmCoreInfoTable[Index].CoreId);
-
-      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
-      if (EFI_ERROR (Status)) {
-        DEBUG ((DEBUG_ERROR,
-          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
-        return Status;
-      }
-
-      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
-      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
-      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
-      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
-    }
-  } else {
-    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
-    return EFI_INVALID_PARAMETER;
-  }
-
   // Create /cpus noide
   Node = fdt_add_subnode (Fdt, 0, "cpus");
   if (Node >= 0) {
@@ -449,6 +422,34 @@ PrepareFdt (
     return EFI_INVALID_PARAMETER;
   }
 
+  // Create /pmu node
+  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
+  if (PmuNode >= 0) {
+    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
+
+    // append PMU interrupts
+    for (Index = 0; Index < ArmCoreCount; Index++) {
+      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
+                              ArmCoreInfoTable[Index].CoreId);
+
+      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR,
+          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
+        return Status;
+      }
+
+      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
+      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
+      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
+      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
+      fdt_appendprop_cell (Fdt, PmuNode, "interrupt-affinity", Phandle[Index]);
+    }
+  } else {
+    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
   SetSocIdStatus (Fdt);
   SetXgbeStatus (Fdt);
 
-- 
2.17.1

[PATCH edk2-platforms v2 6/8] Silicon/AMD/StyxDtbLoaderLib: add description of the cache topology

[Ard Biesheuvel]

Emit the cache topology into the device tree too when generating the
CPU nodes and the cpu-map. Note that the cache geometries are all
fixed and thus hardcoded - the only runtime variable aspect is how
many L2 nodes to generate (one per detected cluster)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 49 ++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index 2f7b5e2a7b25..e723e77c7965 100644
--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -305,6 +305,10 @@ PrepareFdt (
   UINT32                      ClusterCount;
   UINT32                      CoresInCluster;
   UINT32                      ClusterId;
+  INT32                       L2Node;
+  INT32                       L3Node;
+  INT32                       L2Phandle[NUM_CORES / 2];
+  INT32                       L3Phandle;
   UINTN                       MpId;
   CHAR8                       Name[10];
   AMD_MP_CORE_INFO_PROTOCOL   *AmdMpCoreInfoProtocol;
@@ -328,6 +332,42 @@ PrepareFdt (
   ASSERT (ArmCoreInfoTable != NULL);
   ASSERT (ArmCoreCount <= NUM_CORES);
 
+  // Create the L3 cache node
+  L3Node = fdt_add_subnode (Fdt, 0, "l3cache");
+  if (L3Node < 0) {
+    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'l3cache' node\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  L3Phandle = fdt_alloc_phandle (Fdt);
+  fdt_setprop_cell (Fdt, L3Node, "cache-level", 3);
+  fdt_setprop_cell (Fdt, L3Node, "cache-size", SIZE_8MB);
+  fdt_setprop_cell (Fdt, L3Node, "cache-line-size", 64);
+  fdt_setprop_cell (Fdt, L3Node, "cache-sets", 8192);
+  fdt_setprop_empty (Fdt, L3Node, "cache-unified");
+  fdt_setprop_cell (Fdt, L3Node, "phandle", L3Phandle);
+
+  ClusterCount = NumberOfClustersInTable (ArmCoreInfoTable, ArmCoreCount);
+  ASSERT (ClusterCount <= ARRAY_SIZE (L2Phandle));
+
+  for (Index = 0; Index < ClusterCount; Index++) {
+    AsciiSPrint (Name, sizeof (Name), "l2cache%d", Index);
+
+    L2Node = fdt_add_subnode (Fdt, 0, Name);
+    if (L2Node < 0) {
+      DEBUG ((DEBUG_ERROR, "FDT: Error creating '%a' node\n", Name));
+      return EFI_INVALID_PARAMETER;
+    }
+
+    L2Phandle[Index] = fdt_alloc_phandle (Fdt);
+    fdt_setprop_cell (Fdt, L2Node, "cache-size", SIZE_1MB);
+    fdt_setprop_cell (Fdt, L2Node, "cache-line-size", 64);
+    fdt_setprop_cell (Fdt, L2Node, "cache-sets", 1024);
+    fdt_setprop_empty (Fdt, L2Node, "cache-unified");
+    fdt_setprop_cell (Fdt, L2Node, "next-level-cache", L3Phandle);
+    fdt_setprop_cell (Fdt, L2Node, "phandle", L2Phandle[Index]);
+  }
+
   // Get Id from primary CPU
   MpId = (UINTN)ArmReadMpidr ();
 
@@ -367,6 +407,15 @@ PrepareFdt (
     fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
     fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
     fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
+
+    fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
+    fdt_setprop_cell (Fdt, CpuNode, "i-cache-line-size", 64);
+    fdt_setprop_cell (Fdt, CpuNode, "i-cache-sets", 256);
+    fdt_setprop_cell (Fdt, CpuNode, "d-cache-size", 2 * SIZE_16KB);
+    fdt_setprop_cell (Fdt, CpuNode, "d-cache-line-size", 64);
+    fdt_setprop_cell (Fdt, CpuNode, "d-cache-sets", 256);
+    fdt_setprop_cell (Fdt, CpuNode, "l2-cache",
+      L2Phandle[ArmCoreInfoTable[Index].ClusterId]);
   }
 
   // Create /cpu-map node
-- 
2.17.1

[PATCH edk2-platforms v2 7/8] Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic ARMv8

[Ard Biesheuvel]

Use the more precise Cortex-A57 based compatible strings to describe
the CPUs and the PMUs in the device tree.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index e723e77c7965..091d151ac722 100644
--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -405,7 +405,7 @@ PrepareFdt (
                             ArmCoreInfoTable[Index].CoreId);
     MpId = cpu_to_fdt64 (MpId);
     fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
-    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
+    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
     fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
 
     fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
@@ -474,7 +474,7 @@ PrepareFdt (
   // Create /pmu node
   PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
   if (PmuNode >= 0) {
-    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
+    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");
 
     // append PMU interrupts
     for (Index = 0; Index < ArmCoreCount; Index++) {
-- 
2.17.1

[PATCH edk2-platforms v2 8/8] Silicon/AMD/StyxDtbLoaderLib: omit linux,phandle properties

[Ard Biesheuvel]

The linux,phandle property is a deprecated alias for the phandle property
which was standardized long ago, so don't bother emitting it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
index 091d151ac722..6ce694573f9f 100644
--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -397,7 +397,6 @@ PrepareFdt (
     }
     Phandle[Index] = fdt_alloc_phandle (Fdt);
     fdt_setprop_cell (Fdt, CpuNode, "phandle", Phandle[Index]);
-    fdt_setprop_cell (Fdt, CpuNode, "linux,phandle", Phandle[Index]);
 
     fdt_setprop_string (Fdt, CpuNode, "enable-method", "psci");
 
-- 
2.17.1

Re: [PATCH edk2-platforms v2 3/8] Platform/Overdrive: fix a typo in the DT

[Leif Lindholm]

On Wed, Nov 27, 2019 at 19:44:34 +0100, Ard Biesheuvel wrote:
> DT unit addresses are hex quantities but they should not include
> the 0x prefix.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

But can you clarify in commit message that this is a cosmetic/style
change only?

(And maybe mirror this cleanup in
Platform/SoftIron/Overdrive1000Board/FdtBlob/styx-overdrive1000.dts
? These appear to be the only two instances in the repo.)

/
    Leif

> ---
>  Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> index 1d8a6caafd82..a92ab695fb2e 100644
> --- a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> +++ b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> @@ -291,7 +291,7 @@
>  			dma-coherent;
>  		};
>  
> -		ccn@0xe8000000 {
> +		ccn@e8000000 {
>  			compatible = "arm,ccn-504";
>  			reg = <0x0 0xe8000000 0x0 0x1000000>;
>  			interrupts = <0x0 0x17c 0x4>;
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms v2 5/8] Silicon/AMD/StyxDtbLoaderLib: add interrupt-affinity property to PMU node

[Leif Lindholm]

On Wed, Nov 27, 2019 at 19:44:36 +0100, Ard Biesheuvel wrote:
> AMD Seattle uses a range of SPIs to signal PMU events, and this requires
> a description in the DT which SPI maps to which CPU. This requires us to
> defer the generation of the PMU node to a point where the CPU phandles
> have been allocated.

Ah, so these were previously populated with garbage? Oops.

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 55 ++++++++++----------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> index 261b5f59c8df..2f7b5e2a7b25 100644
> --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> @@ -331,33 +331,6 @@ PrepareFdt (
>    // Get Id from primary CPU
>    MpId = (UINTN)ArmReadMpidr ();
>  
> -  // Create /pmu node
> -  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> -  if (PmuNode >= 0) {
> -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> -
> -    // append PMU interrupts
> -    for (Index = 0; Index < ArmCoreCount; Index++) {
> -      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
> -                              ArmCoreInfoTable[Index].CoreId);
> -
> -      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
> -      if (EFI_ERROR (Status)) {
> -        DEBUG ((DEBUG_ERROR,
> -          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
> -        return Status;
> -      }
> -
> -      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
> -      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
> -      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
> -      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
> -    }
> -  } else {
> -    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
>    // Create /cpus noide
>    Node = fdt_add_subnode (Fdt, 0, "cpus");
>    if (Node >= 0) {
> @@ -449,6 +422,34 @@ PrepareFdt (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  // Create /pmu node
> +  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> +  if (PmuNode >= 0) {
> +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> +
> +    // append PMU interrupts
> +    for (Index = 0; Index < ArmCoreCount; Index++) {
> +      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
> +                              ArmCoreInfoTable[Index].CoreId);
> +
> +      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_ERROR,
> +          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
> +        return Status;
> +      }
> +
> +      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
> +      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
> +      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
> +      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
> +      fdt_appendprop_cell (Fdt, PmuNode, "interrupt-affinity", Phandle[Index]);
> +    }
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    SetSocIdStatus (Fdt);
>    SetXgbeStatus (Fdt);
>  
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms v2 6/8] Silicon/AMD/StyxDtbLoaderLib: add description of the cache topology

[Leif Lindholm]

On Wed, Nov 27, 2019 at 19:44:37 +0100, Ard Biesheuvel wrote:
> Emit the cache topology into the device tree too when generating the
> CPU nodes and the cpu-map. Note that the cache geometries are all
> fixed and thus hardcoded - the only runtime variable aspect is how
> many L2 nodes to generate (one per detected cluster)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 49 ++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> index 2f7b5e2a7b25..e723e77c7965 100644
> --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> @@ -305,6 +305,10 @@ PrepareFdt (
>    UINT32                      ClusterCount;
>    UINT32                      CoresInCluster;
>    UINT32                      ClusterId;
> +  INT32                       L2Node;
> +  INT32                       L3Node;
> +  INT32                       L2Phandle[NUM_CORES / 2];
> +  INT32                       L3Phandle;
>    UINTN                       MpId;
>    CHAR8                       Name[10];
>    AMD_MP_CORE_INFO_PROTOCOL   *AmdMpCoreInfoProtocol;
> @@ -328,6 +332,42 @@ PrepareFdt (
>    ASSERT (ArmCoreInfoTable != NULL);
>    ASSERT (ArmCoreCount <= NUM_CORES);
>  
> +  // Create the L3 cache node
> +  L3Node = fdt_add_subnode (Fdt, 0, "l3cache");
> +  if (L3Node < 0) {
> +    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'l3cache' node\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  L3Phandle = fdt_alloc_phandle (Fdt);
> +  fdt_setprop_cell (Fdt, L3Node, "cache-level", 3);
> +  fdt_setprop_cell (Fdt, L3Node, "cache-size", SIZE_8MB);
> +  fdt_setprop_cell (Fdt, L3Node, "cache-line-size", 64);
> +  fdt_setprop_cell (Fdt, L3Node, "cache-sets", 8192);
> +  fdt_setprop_empty (Fdt, L3Node, "cache-unified");
> +  fdt_setprop_cell (Fdt, L3Node, "phandle", L3Phandle);
> +
> +  ClusterCount = NumberOfClustersInTable (ArmCoreInfoTable, ArmCoreCount);
> +  ASSERT (ClusterCount <= ARRAY_SIZE (L2Phandle));
> +
> +  for (Index = 0; Index < ClusterCount; Index++) {
> +    AsciiSPrint (Name, sizeof (Name), "l2cache%d", Index);
> +
> +    L2Node = fdt_add_subnode (Fdt, 0, Name);
> +    if (L2Node < 0) {
> +      DEBUG ((DEBUG_ERROR, "FDT: Error creating '%a' node\n", Name));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
> +    L2Phandle[Index] = fdt_alloc_phandle (Fdt);
> +    fdt_setprop_cell (Fdt, L2Node, "cache-size", SIZE_1MB);
> +    fdt_setprop_cell (Fdt, L2Node, "cache-line-size", 64);
> +    fdt_setprop_cell (Fdt, L2Node, "cache-sets", 1024);
> +    fdt_setprop_empty (Fdt, L2Node, "cache-unified");
> +    fdt_setprop_cell (Fdt, L2Node, "next-level-cache", L3Phandle);
> +    fdt_setprop_cell (Fdt, L2Node, "phandle", L2Phandle[Index]);
> +  }
> +
>    // Get Id from primary CPU
>    MpId = (UINTN)ArmReadMpidr ();
>  
> @@ -367,6 +407,15 @@ PrepareFdt (
>      fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
>      fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
>      fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
> +
> +    fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
> +    fdt_setprop_cell (Fdt, CpuNode, "i-cache-line-size", 64);
> +    fdt_setprop_cell (Fdt, CpuNode, "i-cache-sets", 256);
> +    fdt_setprop_cell (Fdt, CpuNode, "d-cache-size", 2 * SIZE_16KB);
> +    fdt_setprop_cell (Fdt, CpuNode, "d-cache-line-size", 64);
> +    fdt_setprop_cell (Fdt, CpuNode, "d-cache-sets", 256);
> +    fdt_setprop_cell (Fdt, CpuNode, "l2-cache",
> +      L2Phandle[ArmCoreInfoTable[Index].ClusterId]);
>    }
>  
>    // Create /cpu-map node
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms v2 5/8] Silicon/AMD/StyxDtbLoaderLib: add interrupt-affinity property to PMU node

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 14:22, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Nov 27, 2019 at 19:44:36 +0100, Ard Biesheuvel wrote:
> > AMD Seattle uses a range of SPIs to signal PMU events, and this requires
> > a description in the DT which SPI maps to which CPU. This requires us to
> > defer the generation of the PMU node to a point where the CPU phandles
> > have been allocated.
>
> Ah, so these were previously populated with garbage? Oops.
>

No, not really. Moving the code obfuscates it a bit, but the new code
has an additional fdt_appendprop_cell() to set "interrupt-affinity"
which was missing entirely before.

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks,

> > ---
> >  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 55 ++++++++++----------
> >  1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > index 261b5f59c8df..2f7b5e2a7b25 100644
> > --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > @@ -331,33 +331,6 @@ PrepareFdt (
> >    // Get Id from primary CPU
> >    MpId = (UINTN)ArmReadMpidr ();
> >
> > -  // Create /pmu node
> > -  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> > -  if (PmuNode >= 0) {
> > -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> > -
> > -    // append PMU interrupts
> > -    for (Index = 0; Index < ArmCoreCount; Index++) {
> > -      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
> > -                              ArmCoreInfoTable[Index].CoreId);
> > -
> > -      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
> > -      if (EFI_ERROR (Status)) {
> > -        DEBUG ((DEBUG_ERROR,
> > -          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
> > -        return Status;
> > -      }
> > -
> > -      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
> > -      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
> > -      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
> > -      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
> > -    }
> > -  } else {
> > -    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
> > -    return EFI_INVALID_PARAMETER;
> > -  }
> > -
> >    // Create /cpus noide
> >    Node = fdt_add_subnode (Fdt, 0, "cpus");
> >    if (Node >= 0) {
> > @@ -449,6 +422,34 @@ PrepareFdt (
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > +  // Create /pmu node
> > +  PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> > +  if (PmuNode >= 0) {
> > +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> > +
> > +    // append PMU interrupts
> > +    for (Index = 0; Index < ArmCoreCount; Index++) {
> > +      MpId = (UINTN)GET_MPID (ArmCoreInfoTable[Index].ClusterId,
> > +                              ArmCoreInfoTable[Index].CoreId);
> > +
> > +      Status = AmdMpCoreInfoProtocol->GetPmuSpiFromMpId (MpId, &PmuInt.IntId);
> > +      if (EFI_ERROR (Status)) {
> > +        DEBUG ((DEBUG_ERROR,
> > +          "FDT: Error getting PMU interrupt for MpId '0x%x'\n", MpId));
> > +        return Status;
> > +      }
> > +
> > +      PmuInt.Flag = cpu_to_fdt32 (PMU_INT_FLAG_SPI);
> > +      PmuInt.IntId = cpu_to_fdt32 (PmuInt.IntId);
> > +      PmuInt.Type = cpu_to_fdt32 (PMU_INT_TYPE_HIGH_LEVEL);
> > +      fdt_appendprop (Fdt, PmuNode, "interrupts", &PmuInt, sizeof(PmuInt));
> > +      fdt_appendprop_cell (Fdt, PmuNode, "interrupt-affinity", Phandle[Index]);
> > +    }
> > +  } else {
> > +    DEBUG ((DEBUG_ERROR, "FDT: Error creating 'pmu' node\n"));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> >    SetSocIdStatus (Fdt);
> >    SetXgbeStatus (Fdt);
> >
> > --
> > 2.17.1
> >

Re: [PATCH edk2-platforms v2 3/8] Platform/Overdrive: fix a typo in the DT

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 13:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Nov 27, 2019 at 19:44:34 +0100, Ard Biesheuvel wrote:
> > DT unit addresses are hex quantities but they should not include
> > the 0x prefix.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> But can you clarify in commit message that this is a cosmetic/style
> change only?
>

Sure.


> (And maybe mirror this cleanup in
> Platform/SoftIron/Overdrive1000Board/FdtBlob/styx-overdrive1000.dts
> ? These appear to be the only two instances in the repo.)
>
> /
>     Leif
>
> > ---
> >  Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> > index 1d8a6caafd82..a92ab695fb2e 100644
> > --- a/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> > +++ b/Platform/AMD/OverdriveBoard/DeviceTree/OverdriveBoard.dts
> > @@ -291,7 +291,7 @@
> >                       dma-coherent;
> >               };
> >
> > -             ccn@0xe8000000 {
> > +             ccn@e8000000 {
> >                       compatible = "arm,ccn-504";
> >                       reg = <0x0 0xe8000000 0x0 0x1000000>;
> >                       interrupts = <0x0 0x17c 0x4>;
> > --
> > 2.17.1
> >

Re: [PATCH edk2-platforms v2 7/8] Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic ARMv8

[Leif Lindholm]

On Wed, Nov 27, 2019 at 19:44:38 +0100, Ard Biesheuvel wrote:
> Use the more precise Cortex-A57 based compatible strings to describe
> the CPUs and the PMUs in the device tree.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> index e723e77c7965..091d151ac722 100644
> --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> @@ -405,7 +405,7 @@ PrepareFdt (
>                              ArmCoreInfoTable[Index].CoreId);
>      MpId = cpu_to_fdt64 (MpId);
>      fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
> -    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
> +    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
>      fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
>  
>      fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
> @@ -474,7 +474,7 @@ PrepareFdt (
>    // Create /pmu node
>    PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
>    if (PmuNode >= 0) {
> -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");

Since we've always only published "arm,armv8-pmuv3" before, is it
worth keeping that around as a secondary compatible string rather
than replacing it outright?

/
    Leif

>      // append PMU interrupts
>      for (Index = 0; Index < ArmCoreCount; Index++) {
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms v2 8/8] Silicon/AMD/StyxDtbLoaderLib: omit linux,phandle properties

[Leif Lindholm]

On Wed, Nov 27, 2019 at 19:44:39 +0100, Ard Biesheuvel wrote:
> The linux,phandle property is a deprecated alias for the phandle property
> which was standardized long ago, so don't bother emitting it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> index 091d151ac722..6ce694573f9f 100644
> --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> @@ -397,7 +397,6 @@ PrepareFdt (
>      }
>      Phandle[Index] = fdt_alloc_phandle (Fdt);
>      fdt_setprop_cell (Fdt, CpuNode, "phandle", Phandle[Index]);
> -    fdt_setprop_cell (Fdt, CpuNode, "linux,phandle", Phandle[Index]);
>  
>      fdt_setprop_string (Fdt, CpuNode, "enable-method", "psci");
>  
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms v2 7/8] Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic ARMv8

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 14:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Nov 27, 2019 at 19:44:38 +0100, Ard Biesheuvel wrote:
> > Use the more precise Cortex-A57 based compatible strings to describe
> > the CPUs and the PMUs in the device tree.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > index e723e77c7965..091d151ac722 100644
> > --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > @@ -405,7 +405,7 @@ PrepareFdt (
> >                              ArmCoreInfoTable[Index].CoreId);
> >      MpId = cpu_to_fdt64 (MpId);
> >      fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
> > -    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
> > +    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
> >      fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
> >
> >      fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
> > @@ -474,7 +474,7 @@ PrepareFdt (
> >    // Create /pmu node
> >    PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> >    if (PmuNode >= 0) {
> > -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> > +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");
>
> Since we've always only published "arm,armv8-pmuv3" before, is it
> worth keeping that around as a secondary compatible string rather
> than replacing it outright?
>

Yeah, good point. I'll change that.

Re: [PATCH edk2-platforms v2 7/8] Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic ARMv8

[Leif Lindholm]

On Thu, Nov 28, 2019 at 14:39:23 +0100, Ard Biesheuvel wrote:
> On Thu, 28 Nov 2019 at 14:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Wed, Nov 27, 2019 at 19:44:38 +0100, Ard Biesheuvel wrote:
> > > Use the more precise Cortex-A57 based compatible strings to describe
> > > the CPUs and the PMUs in the device tree.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > index e723e77c7965..091d151ac722 100644
> > > --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > @@ -405,7 +405,7 @@ PrepareFdt (
> > >                              ArmCoreInfoTable[Index].CoreId);
> > >      MpId = cpu_to_fdt64 (MpId);
> > >      fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
> > > -    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
> > > +    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
> > >      fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
> > >
> > >      fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
> > > @@ -474,7 +474,7 @@ PrepareFdt (
> > >    // Create /pmu node
> > >    PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> > >    if (PmuNode >= 0) {
> > > -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> > > +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");
> >
> > Since we've always only published "arm,armv8-pmuv3" before, is it
> > worth keeping that around as a secondary compatible string rather
> > than replacing it outright?
> 
> Yeah, good point. I'll change that.

Thanks. With that:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH edk2-platforms v2 7/8] Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic ARMv8

[Ard Biesheuvel]

On Thu, 28 Nov 2019 at 14:40, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 28, 2019 at 14:39:23 +0100, Ard Biesheuvel wrote:
> > On Thu, 28 Nov 2019 at 14:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Wed, Nov 27, 2019 at 19:44:38 +0100, Ard Biesheuvel wrote:
> > > > Use the more precise Cortex-A57 based compatible strings to describe
> > > > the CPUs and the PMUs in the device tree.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > ---
> > > >  Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > > index e723e77c7965..091d151ac722 100644
> > > > --- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > > +++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
> > > > @@ -405,7 +405,7 @@ PrepareFdt (
> > > >                              ArmCoreInfoTable[Index].CoreId);
> > > >      MpId = cpu_to_fdt64 (MpId);
> > > >      fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
> > > > -    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,armv8");
> > > > +    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
> > > >      fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");
> > > >
> > > >      fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
> > > > @@ -474,7 +474,7 @@ PrepareFdt (
> > > >    // Create /pmu node
> > > >    PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
> > > >    if (PmuNode >= 0) {
> > > > -    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,armv8-pmuv3");
> > > > +    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");
> > >
> > > Since we've always only published "arm,armv8-pmuv3" before, is it
> > > worth keeping that around as a secondary compatible string rather
> > > than replacing it outright?
> >
> > Yeah, good point. I'll change that.
>
> Thanks. With that:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Cheers. For the record, I'll need to apply the following on top:

--- a/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
+++ b/Silicon/AMD/Styx/Library/StyxDtbLoaderLib/StyxDtbLoaderLib.c
@@ -281,6 +281,8 @@ SetXgbeStatus (
   }
 }

+STATIC CONST CHAR8 mCpuCompatible[] = "arm,cortex-a57\0arm,armv8";
+STATIC CONST CHAR8 mPmuCompatible[] = "arm,cortex-a57-pmu\0arm,armv8-pmuv3";

 STATIC
 EFI_STATUS
@@ -405,7 +407,8 @@ PrepareFdt (
                             ArmCoreInfoTable[Index].CoreId);
     MpId = cpu_to_fdt64 (MpId);
     fdt_setprop (Fdt, CpuNode, "reg", &MpId, sizeof (MpId));
-    fdt_setprop_string (Fdt, CpuNode, "compatible", "arm,cortex-a57");
+    fdt_setprop (Fdt, CpuNode, "compatible", mCpuCompatible,
+      sizeof (mCpuCompatible));
     fdt_setprop_string (Fdt, CpuNode, "device_type", "cpu");

     fdt_setprop_cell (Fdt, CpuNode, "i-cache-size", 3 * SIZE_16KB);
@@ -474,7 +477,8 @@ PrepareFdt (
   // Create /pmu node
   PmuNode = fdt_add_subnode(Fdt, 0, "pmu");
   if (PmuNode >= 0) {
-    fdt_setprop_string (Fdt, PmuNode, "compatible", "arm,cortex-a57-pmu");
+    fdt_setprop (Fdt, PmuNode, "compatible", mPmuCompatible,
+      sizeof (mPmuCompatible));

     // append PMU interrupts
     for (Index = 0; Index < ArmCoreCount; Index++) {

Re: [PATCH edk2-platforms v2 0/8] fixes and updates for AMD OverDrive

[Ard Biesheuvel]

On Wed, 27 Nov 2019 at 19:44, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> Fix some issues in the ACPI and DT descriptions of the SMMU routing,
> in particular the routing of the CCP crypto accelerator, which sits
> behind an SMMU as well on B1 silicon (but not on B0, strangely enough)
>
> Changes since v1:
> - add Leif's ack to patches #1, #2 and #4
> - add patches to fix some errors and inaccuracies in the DT
> - update the DT generation code to emit interrupt affinity for the PMU node
> - update the DT generation code to emit a description of the cache topology
> - stop emitting the obsolete linux,phandle properties
>
> Ard Biesheuvel (8):
>   Platform/Overdrive: add missing resolution for FileHandleLib
>   Platform/Overdrive: clean up stream ID descriptions in DT
>   Platform/Overdrive: fix a typo in the DT
>   Silicon/AMD/Styx: clean up stream ID mappings for SMMU
>   Silicon/AMD/StyxDtbLoaderLib: add interrupt-affinity property to PMU
>     node
>   Silicon/AMD/StyxDtbLoaderLib: add description of the cache topology
>   Silicon/AMD/StyxDtbLoaderLib: use Cortex-A57 IDs instead of generic
>     ARMv8
>   Silicon/AMD/StyxDtbLoaderLib: omit linux,phandle properties
>

Series pushed as 04889ec1198b..6bde2876c3aa

Thanks,