[v1] Armada7k8k ICU support

[edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Marcin Wojtas 7 years, 7 months ago

ICU (Interrupt Consolidation Unit) is a mechanism,
that allows to send-message based interrupts from the
CP110 unit (South Bridge) to the Application Processor
hardware block. After dispatching the interrupts in the
GIC are generated.

This patch adds a basic version of the library, that
allows to configure a static mapping between CP110
interfaces and GICv2 of the Armada7k8k SoC family.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
 Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
 Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++
 3 files changed, 399 insertions(+)
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c

diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
new file mode 100644
index 0000000..0010141
--- /dev/null
+++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
@@ -0,0 +1,38 @@
+## @file
+#
+#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
+#
+#  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.
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = IcuLib
+  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmadaIcuLib
+
+[Sources]
+  IcuLib.c
+
+[Packages]
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+  Silicon/Marvell/Marvell.dec
+
+[LibraryClasses]
+  ArmadaSoCDescLib
+  DebugLib
+  IoLib
+  PcdLib
+
+[FixedPcd]
+  gMarvellTokenSpaceGuid.PcdMaxCpCount
diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
new file mode 100644
index 0000000..4bf2298
--- /dev/null
+++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
@@ -0,0 +1,46 @@
+/**
+*
+*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
+*
+*  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.
+*
+*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
+*  ICU - Interrupt Consolidation Unit
+*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
+*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/ArmadaIcuLib.h>
+#include <Library/ArmadaSoCDescLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000
+
+#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
+#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
+#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
+#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))
+#define ICU_INT_CFG(x)          (0x100 + 4 * x)
+
+#define ICU_INT_ENABLE_OFFSET    24
+#define ICU_IS_EDGE_OFFSET       28
+#define ICU_GROUP_OFFSET         29
+
+#define ICU_MAX_SUPPORTED_UNITS  2
+#define ICU_MAX_IRQS_PER_CP      64
+
+#define MAX_ICU_IRQS             207
diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
new file mode 100644
index 0000000..4ac98aa
--- /dev/null
+++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
@@ -0,0 +1,315 @@
+/**
+*
+*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
+*
+*  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.
+*
+*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
+*  ICU - Interrupt Consolidation Unit
+*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
+*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
+*
+**/
+
+#include "IcuLib.h"
+
+EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
+
+STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
+  {22,   0, Level}, /* PCIx4 INT A interrupt */
+  {23,   1, Level}, /* PCIx1 INT A interrupt */
+  {24,   2, Level}, /* PCIx1 INT A interrupt */
+  {27,   3, Level}, /* SD/MMC */
+  {33,   4, Level}, /* PPv2 DBG AXI monitor */
+  {34,   4, Level}, /* HB1      AXI monitor */
+  {35,   4, Level}, /* AP       AXI monitor */
+  {36,   4, Level}, /* PPv2     AXI monitor */
+  {39,   5, Level}, /* PPv2 Irq */
+  {40,   6, Level}, /* PPv2 Irq */
+  {41,   7, Level}, /* PPv2 Irq */
+  {43,   8, Level}, /* PPv2 Irq */
+  {44,   9, Level}, /* PPv2 Irq */
+  {45,  10, Level}, /* PPv2 Irq */
+  {47,  11, Level}, /* PPv2 Irq */
+  {48,  12, Level}, /* PPv2 Irq */
+  {49,  13, Level}, /* PPv2 Irq */
+  {51,  14, Level}, /* PPv2 Irq */
+  {52,  15, Level}, /* PPv2 Irq */
+  {53,  16, Level}, /* PPv2 Irq */
+  {55,  17, Level}, /* PPv2 Irq */
+  {56,  18, Level}, /* PPv2 Irq */
+  {57,  19, Level}, /* PPv2 Irq */
+  {59,  20, Level}, /* PPv2 Irq */
+  {60,  21, Level}, /* PPv2 Irq */
+  {61,  22, Level}, /* PPv2 Irq */
+  {63,  23, Level}, /* PPv2 Irq */
+  {64,  24, Level}, /* PPv2 Irq */
+  {65,  25, Level}, /* PPv2 Irq */
+  {67,  26, Level}, /* PPv2 Irq */
+  {68,  27, Level}, /* PPv2 Irq */
+  {69,  28, Level}, /* PPv2 Irq */
+  {71,  29, Level}, /* PPv2 Irq */
+  {72,  30, Level}, /* PPv2 Irq */
+  {73,  31, Level}, /* PPv2 Irq */
+  {78,  32, Level}, /* MG Irq */
+  {79,  33, Level}, /* GPIO 56-63 */
+  {80,  34, Level}, /* GPIO 48-55 */
+  {81,  35, Level}, /* GPIO 40-47 */
+  {82,  36, Level}, /* GPIO 32-39 */
+  {83,  37, Level}, /* GPIO 24-31 */
+  {84,  38, Level}, /* GPIO 16-23 */
+  {85,  39, Level}, /* GPIO  8-15 */
+  {86,  40, Level}, /* GPIO  0-7  */
+  {88,  41, Level}, /* EIP-197 ring-0 */
+  {89,  42, Level}, /* EIP-197 ring-1 */
+  {90,  43, Level}, /* EIP-197 ring-2 */
+  {91,  44, Level}, /* EIP-197 ring-3 */
+  {92,  45, Level}, /* EIP-197 int */
+  {95,  46, Level}, /* EIP-150 Irq */
+  {102, 47, Level}, /* USB3 Device Irq */
+  {105, 48, Level}, /* USB3 Host-1 Irq */
+  {106, 49, Level}, /* USB3 Host-0 Irq */
+  {107, 50, Level}, /* SATA Host-1 Irq */
+  {109, 50, Level}, /* SATA Host-0 Irq */
+  {115, 52, Level}, /* NAND Irq */
+  {117, 53, Level}, /* SPI-1 Irq */
+  {118, 54, Level}, /* SPI-0 Irq */
+  {120, 55, Level}, /* I2C 0 Irq */
+  {121, 56, Level}, /* I2C 1 Irq */
+  {122, 57, Level}, /* UART 0 Irq */
+  {123, 58, Level}, /* UART 1 Irq */
+  {124, 59, Level}, /* UART 2 Irq */
+  {125, 60, Level}, /* UART 3 Irq */
+  {127, 61, Level}, /* GOP-3 Irq */
+  {128, 62, Level}, /* GOP-2 Irq */
+  {129, 63, Level}, /* GOP-0 Irq */
+};
+
+/*
+ * SEI - System Error Interrupts
+ * Note: SPI ID 0-20 are reserved for North-Bridge
+ */
+STATIC ICU_IRQ IrqMapSei[] = {
+  {11,  21, Level}, /* SEI error CP-2-CP */
+  {15,  22, Level}, /* PIDI-64 SOC */
+  {16,  23, Level}, /* D2D error Irq */
+  {17,  24, Level}, /* D2D Irq */
+  {18,  25, Level}, /* NAND error */
+  {19,  26, Level}, /* PCIx4 error */
+  {20,  27, Level}, /* PCIx1_0 error */
+  {21,  28, Level}, /* PCIx1_1 error */
+  {25,  29, Level}, /* SDIO reg error */
+  {75,  30, Level}, /* IOB error */
+  {94,  31, Level}, /* EIP150 error */
+  {97,  32, Level}, /* XOR-1 system error */
+  {99,  33, Level}, /* XOR-0 system error */
+  {108, 34, Level}, /* SATA-1 error */
+  {110, 35, Level}, /* SATA-0 error */
+  {114, 36, Level}, /* TDM-MC error */
+  {116, 37, Level}, /* DFX server Irq */
+  {117, 38, Level}, /* Device bus error */
+  {147, 39, Level}, /* Audio error */
+  {171, 40, Level}, /* PIDI Sync error */
+};
+
+/* REI - RAM Error Interrupts */
+STATIC CONST ICU_IRQ IrqMapRei[] = {
+  {12,  0, Level}, /* REI error CP-2-CP */
+  {26,  1, Level}, /* SDIO memory error */
+  {87,  2, Level}, /* EIP-197 ECC error */
+  {93,  3, Edge},  /* EIP-150 RAM error */
+  {96,  4, Level}, /* XOR-1 memory Irq */
+  {98,  5, Level}, /* XOR-0 memory Irq */
+  {100, 6, Edge},  /* USB3 device tx parity */
+  {101, 7, Edge},  /* USB3 device rq parity */
+  {103, 8, Edge},  /* USB3H-1 RAM error */
+  {104, 9, Edge},  /* USB3H-0 RAM error */
+};
+
+STATIC CONST ICU_CONFIG IcuConfigDefault = {
+  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
+  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
+  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
+};
+
+STATIC
+VOID
+IcuClearIrq (
+  IN UINTN IcuBase,
+  IN UINTN Nr
+)
+{
+  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
+}
+
+STATIC
+VOID
+IcuSetIrq (
+  IN UINTN           IcuBase,
+  IN CONST ICU_IRQ  *Irq,
+  IN UINTN           SpiBase,
+  IN ICU_GROUP       Group
+  )
+{
+  UINT32 IcuInt;
+
+  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
+  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
+  IcuInt |= Group << ICU_GROUP_OFFSET;
+
+  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
+}
+
+STATIC
+VOID
+IcuConfigure (
+  IN UINTN             CpIndex,
+  IN MV_SOC_ICU_DESC  *IcuDesc,
+  IN CONST ICU_CONFIG *Config
+  )
+{
+  UINTN IcuBase, Index, SpiOffset, SpiBase;
+  CONST ICU_IRQ *Irq;
+  ICU_MSI *Msi;
+
+  /* Get ICU registers base address */
+  IcuBase = ICU_REG_BASE (CpIndex);
+  /* Get the base of the GIC SPI ID in the MSI message */
+  SpiBase = IcuDesc->IcuSpiBase;
+  /* Get multiple CP110 instances SPI ID shift */
+  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
+  /* Get MSI addresses per interrupt group */
+  Msi = IcuDesc->IcuMsi;
+
+  /* Set the address for SET_SPI and CLR_SPI registers in AP */
+  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
+    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
+    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
+    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
+    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
+  }
+
+  /* Mask all ICU interrupts */
+  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
+    IcuClearIrq (IcuBase, Index);
+  }
+
+  /* Configure the ICU interrupt lines */
+  Irq = Config->NonSecure.Map;
+  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
+    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
+  }
+
+  Irq = Config->Sei.Map;
+  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
+    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
+  }
+
+  Irq = Config->Rei.Map;
+  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
+    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
+  }
+}
+
+STATIC
+VOID
+IcuClearGicSpi (
+  IN UINTN             CpIndex,
+  IN MV_SOC_ICU_DESC  *IcuDesc
+  )
+{
+  CONST ICU_CONFIG *Config;
+  UINTN Index, SpiOffset, SpiBase;
+  CONST ICU_IRQ *Irq;
+  ICU_MSI *Msi;
+
+  Config = &IcuConfigDefault;
+
+  /* Get the base of the GIC SPI ID in the MSI message */
+  SpiBase = IcuDesc->IcuSpiBase;
+  /* Get multiple CP110 instances SPI ID shift */
+  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
+  /* Get MSI addresses per interrupt group */
+  Msi = IcuDesc->IcuMsi;
+
+  /* Clear ICU-generated GIC SPI interrupts */
+  Irq = Config->NonSecure.Map;
+  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
+    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
+  }
+}
+
+VOID
+EFIAPI
+IcuCleanUp (
+  IN EFI_EVENT  Event,
+  IN VOID      *Context
+  )
+{
+  MV_SOC_ICU_DESC *IcuDesc;
+  UINTN CpCount, CpIndex;
+
+  IcuDesc = Context;
+
+  CpCount = FixedPcdGet8 (PcdMaxCpCount);
+  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
+    CpCount = ICU_MAX_SUPPORTED_UNITS;
+  }
+
+  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
+    IcuClearGicSpi (CpIndex, IcuDesc);
+  }
+}
+
+EFI_STATUS
+EFIAPI
+ArmadaIcuInitialize (
+  )
+{
+  MV_SOC_ICU_DESC *IcuDesc;
+  UINTN CpCount, CpIndex;
+  EFI_STATUS Status;
+
+  /*
+   * Due to limited amount of interrupt lanes, only 2 units can be
+   * wired to the GIC.
+   */
+  CpCount = FixedPcdGet8 (PcdMaxCpCount);
+  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
+      ICU_MAX_SUPPORTED_UNITS,
+      __FUNCTION__));
+    CpCount = ICU_MAX_SUPPORTED_UNITS;
+  }
+
+  /* Obtain SoC description of the ICU */
+  Status = ArmadaSoCDescIcuGet (&IcuDesc);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  /* Configure default ICU to GIC interrupt mapping for each CP110 */
+  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
+    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
+  }
+
+  /*
+   * In order to be immune to the OS capability of clearing ICU-generated
+   * GIC interrupts, register ExitBootServices event, that will
+   * make sure they remain disabled during OS boot.
+   */
+  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
+                  TPL_NOTIFY,
+                  IcuCleanUp,
+                  IcuDesc,
+                  &EfiExitBootServicesEvent);
+
+  return Status;
+}
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Ard Biesheuvel 7 years, 7 months ago

On 12 July 2018 at 09:40, Marcin Wojtas <mw@semihalf.com> wrote:
> ICU (Interrupt Consolidation Unit) is a mechanism,
> that allows to send-message based interrupts from the
> CP110 unit (South Bridge) to the Application Processor
> hardware block. After dispatching the interrupts in the
> GIC are generated.
>
> This patch adds a basic version of the library, that
> allows to configure a static mapping between CP110
> interfaces and GICv2 of the Armada7k8k SoC family.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
>  Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
>  Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++

Does it make sense for a library at this level in the hierarchy (i.e.,
generic Marvell) to carry all the default mappings for all the
interrupts you have below? Doesn't those make this library specific to
7k8k ?

Perhaps you could add a separate library class to obtain those
defaults, and have special implementations for different SoC types.


>  3 files changed, 399 insertions(+)
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c
>
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> new file mode 100644
> index 0000000..0010141
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> @@ -0,0 +1,38 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
> +#
> +#  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.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = IcuLib
> +  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmadaIcuLib
> +
> +[Sources]
> +  IcuLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmadaSoCDescLib
> +  DebugLib
> +  IoLib
> +  PcdLib
> +
> +[FixedPcd]
> +  gMarvellTokenSpaceGuid.PcdMaxCpCount
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> new file mode 100644
> index 0000000..4bf2298
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> @@ -0,0 +1,46 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  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.
> +*
> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> +*  ICU - Interrupt Consolidation Unit
> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> +*
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Library/ArmadaIcuLib.h>
> +#include <Library/ArmadaSoCDescLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000
> +
> +#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
> +#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
> +#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
> +#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))
> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
> +
> +#define ICU_INT_ENABLE_OFFSET    24
> +#define ICU_IS_EDGE_OFFSET       28
> +#define ICU_GROUP_OFFSET         29
> +
> +#define ICU_MAX_SUPPORTED_UNITS  2
> +#define ICU_MAX_IRQS_PER_CP      64
> +
> +#define MAX_ICU_IRQS             207
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> new file mode 100644
> index 0000000..4ac98aa
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> @@ -0,0 +1,315 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  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.
> +*
> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> +*  ICU - Interrupt Consolidation Unit
> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> +*
> +**/
> +
> +#include "IcuLib.h"
> +
> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> +
> +STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
> +  {22,   0, Level}, /* PCIx4 INT A interrupt */
> +  {23,   1, Level}, /* PCIx1 INT A interrupt */
> +  {24,   2, Level}, /* PCIx1 INT A interrupt */
> +  {27,   3, Level}, /* SD/MMC */
> +  {33,   4, Level}, /* PPv2 DBG AXI monitor */
> +  {34,   4, Level}, /* HB1      AXI monitor */
> +  {35,   4, Level}, /* AP       AXI monitor */
> +  {36,   4, Level}, /* PPv2     AXI monitor */
> +  {39,   5, Level}, /* PPv2 Irq */
> +  {40,   6, Level}, /* PPv2 Irq */
> +  {41,   7, Level}, /* PPv2 Irq */
> +  {43,   8, Level}, /* PPv2 Irq */
> +  {44,   9, Level}, /* PPv2 Irq */
> +  {45,  10, Level}, /* PPv2 Irq */
> +  {47,  11, Level}, /* PPv2 Irq */
> +  {48,  12, Level}, /* PPv2 Irq */
> +  {49,  13, Level}, /* PPv2 Irq */
> +  {51,  14, Level}, /* PPv2 Irq */
> +  {52,  15, Level}, /* PPv2 Irq */
> +  {53,  16, Level}, /* PPv2 Irq */
> +  {55,  17, Level}, /* PPv2 Irq */
> +  {56,  18, Level}, /* PPv2 Irq */
> +  {57,  19, Level}, /* PPv2 Irq */
> +  {59,  20, Level}, /* PPv2 Irq */
> +  {60,  21, Level}, /* PPv2 Irq */
> +  {61,  22, Level}, /* PPv2 Irq */
> +  {63,  23, Level}, /* PPv2 Irq */
> +  {64,  24, Level}, /* PPv2 Irq */
> +  {65,  25, Level}, /* PPv2 Irq */
> +  {67,  26, Level}, /* PPv2 Irq */
> +  {68,  27, Level}, /* PPv2 Irq */
> +  {69,  28, Level}, /* PPv2 Irq */
> +  {71,  29, Level}, /* PPv2 Irq */
> +  {72,  30, Level}, /* PPv2 Irq */
> +  {73,  31, Level}, /* PPv2 Irq */
> +  {78,  32, Level}, /* MG Irq */
> +  {79,  33, Level}, /* GPIO 56-63 */
> +  {80,  34, Level}, /* GPIO 48-55 */
> +  {81,  35, Level}, /* GPIO 40-47 */
> +  {82,  36, Level}, /* GPIO 32-39 */
> +  {83,  37, Level}, /* GPIO 24-31 */
> +  {84,  38, Level}, /* GPIO 16-23 */
> +  {85,  39, Level}, /* GPIO  8-15 */
> +  {86,  40, Level}, /* GPIO  0-7  */
> +  {88,  41, Level}, /* EIP-197 ring-0 */
> +  {89,  42, Level}, /* EIP-197 ring-1 */
> +  {90,  43, Level}, /* EIP-197 ring-2 */
> +  {91,  44, Level}, /* EIP-197 ring-3 */
> +  {92,  45, Level}, /* EIP-197 int */
> +  {95,  46, Level}, /* EIP-150 Irq */
> +  {102, 47, Level}, /* USB3 Device Irq */
> +  {105, 48, Level}, /* USB3 Host-1 Irq */
> +  {106, 49, Level}, /* USB3 Host-0 Irq */
> +  {107, 50, Level}, /* SATA Host-1 Irq */
> +  {109, 50, Level}, /* SATA Host-0 Irq */
> +  {115, 52, Level}, /* NAND Irq */
> +  {117, 53, Level}, /* SPI-1 Irq */
> +  {118, 54, Level}, /* SPI-0 Irq */
> +  {120, 55, Level}, /* I2C 0 Irq */
> +  {121, 56, Level}, /* I2C 1 Irq */
> +  {122, 57, Level}, /* UART 0 Irq */
> +  {123, 58, Level}, /* UART 1 Irq */
> +  {124, 59, Level}, /* UART 2 Irq */
> +  {125, 60, Level}, /* UART 3 Irq */
> +  {127, 61, Level}, /* GOP-3 Irq */
> +  {128, 62, Level}, /* GOP-2 Irq */
> +  {129, 63, Level}, /* GOP-0 Irq */
> +};
> +
> +/*
> + * SEI - System Error Interrupts
> + * Note: SPI ID 0-20 are reserved for North-Bridge
> + */
> +STATIC ICU_IRQ IrqMapSei[] = {
> +  {11,  21, Level}, /* SEI error CP-2-CP */
> +  {15,  22, Level}, /* PIDI-64 SOC */
> +  {16,  23, Level}, /* D2D error Irq */
> +  {17,  24, Level}, /* D2D Irq */
> +  {18,  25, Level}, /* NAND error */
> +  {19,  26, Level}, /* PCIx4 error */
> +  {20,  27, Level}, /* PCIx1_0 error */
> +  {21,  28, Level}, /* PCIx1_1 error */
> +  {25,  29, Level}, /* SDIO reg error */
> +  {75,  30, Level}, /* IOB error */
> +  {94,  31, Level}, /* EIP150 error */
> +  {97,  32, Level}, /* XOR-1 system error */
> +  {99,  33, Level}, /* XOR-0 system error */
> +  {108, 34, Level}, /* SATA-1 error */
> +  {110, 35, Level}, /* SATA-0 error */
> +  {114, 36, Level}, /* TDM-MC error */
> +  {116, 37, Level}, /* DFX server Irq */
> +  {117, 38, Level}, /* Device bus error */
> +  {147, 39, Level}, /* Audio error */
> +  {171, 40, Level}, /* PIDI Sync error */
> +};
> +
> +/* REI - RAM Error Interrupts */
> +STATIC CONST ICU_IRQ IrqMapRei[] = {
> +  {12,  0, Level}, /* REI error CP-2-CP */
> +  {26,  1, Level}, /* SDIO memory error */
> +  {87,  2, Level}, /* EIP-197 ECC error */
> +  {93,  3, Edge},  /* EIP-150 RAM error */
> +  {96,  4, Level}, /* XOR-1 memory Irq */
> +  {98,  5, Level}, /* XOR-0 memory Irq */
> +  {100, 6, Edge},  /* USB3 device tx parity */
> +  {101, 7, Edge},  /* USB3 device rq parity */
> +  {103, 8, Edge},  /* USB3H-1 RAM error */
> +  {104, 9, Edge},  /* USB3H-0 RAM error */
> +};
> +
> +STATIC CONST ICU_CONFIG IcuConfigDefault = {
> +  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
> +  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
> +  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
> +};
> +
> +STATIC
> +VOID
> +IcuClearIrq (
> +  IN UINTN IcuBase,
> +  IN UINTN Nr
> +)
> +{
> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
> +}
> +
> +STATIC
> +VOID
> +IcuSetIrq (
> +  IN UINTN           IcuBase,
> +  IN CONST ICU_IRQ  *Irq,
> +  IN UINTN           SpiBase,
> +  IN ICU_GROUP       Group
> +  )
> +{
> +  UINT32 IcuInt;
> +
> +  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
> +  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
> +  IcuInt |= Group << ICU_GROUP_OFFSET;
> +
> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
> +}
> +
> +STATIC
> +VOID
> +IcuConfigure (
> +  IN UINTN             CpIndex,
> +  IN MV_SOC_ICU_DESC  *IcuDesc,
> +  IN CONST ICU_CONFIG *Config
> +  )
> +{
> +  UINTN IcuBase, Index, SpiOffset, SpiBase;
> +  CONST ICU_IRQ *Irq;
> +  ICU_MSI *Msi;
> +
> +  /* Get ICU registers base address */
> +  IcuBase = ICU_REG_BASE (CpIndex);
> +  /* Get the base of the GIC SPI ID in the MSI message */
> +  SpiBase = IcuDesc->IcuSpiBase;
> +  /* Get multiple CP110 instances SPI ID shift */
> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> +  /* Get MSI addresses per interrupt group */
> +  Msi = IcuDesc->IcuMsi;
> +
> +  /* Set the address for SET_SPI and CLR_SPI registers in AP */
> +  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
> +  }
> +
> +  /* Mask all ICU interrupts */
> +  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
> +    IcuClearIrq (IcuBase, Index);
> +  }
> +
> +  /* Configure the ICU interrupt lines */
> +  Irq = Config->NonSecure.Map;
> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
> +  }
> +
> +  Irq = Config->Sei.Map;
> +  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
> +  }
> +
> +  Irq = Config->Rei.Map;
> +  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
> +  }
> +}
> +
> +STATIC
> +VOID
> +IcuClearGicSpi (
> +  IN UINTN             CpIndex,
> +  IN MV_SOC_ICU_DESC  *IcuDesc
> +  )
> +{
> +  CONST ICU_CONFIG *Config;
> +  UINTN Index, SpiOffset, SpiBase;
> +  CONST ICU_IRQ *Irq;
> +  ICU_MSI *Msi;
> +
> +  Config = &IcuConfigDefault;
> +
> +  /* Get the base of the GIC SPI ID in the MSI message */
> +  SpiBase = IcuDesc->IcuSpiBase;
> +  /* Get multiple CP110 instances SPI ID shift */
> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> +  /* Get MSI addresses per interrupt group */
> +  Msi = IcuDesc->IcuMsi;
> +
> +  /* Clear ICU-generated GIC SPI interrupts */
> +  Irq = Config->NonSecure.Map;
> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> +    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
> +  }
> +}
> +
> +VOID
> +EFIAPI
> +IcuCleanUp (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  MV_SOC_ICU_DESC *IcuDesc;
> +  UINTN CpCount, CpIndex;
> +
> +  IcuDesc = Context;
> +
> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> +  }
> +
> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> +    IcuClearGicSpi (CpIndex, IcuDesc);
> +  }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaIcuInitialize (
> +  )
> +{
> +  MV_SOC_ICU_DESC *IcuDesc;
> +  UINTN CpCount, CpIndex;
> +  EFI_STATUS Status;
> +
> +  /*
> +   * Due to limited amount of interrupt lanes, only 2 units can be
> +   * wired to the GIC.
> +   */
> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
> +      ICU_MAX_SUPPORTED_UNITS,
> +      __FUNCTION__));
> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> +  }
> +
> +  /* Obtain SoC description of the ICU */
> +  Status = ArmadaSoCDescIcuGet (&IcuDesc);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Configure default ICU to GIC interrupt mapping for each CP110 */
> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> +    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
> +  }
> +
> +  /*
> +   * In order to be immune to the OS capability of clearing ICU-generated
> +   * GIC interrupts, register ExitBootServices event, that will
> +   * make sure they remain disabled during OS boot.
> +   */
> +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                  TPL_NOTIFY,
> +                  IcuCleanUp,
> +                  IcuDesc,
> +                  &EfiExitBootServicesEvent);
> +
> +  return Status;
> +}
> --
> 2.7.4
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Marcin Wojtas 7 years, 7 months ago

Hi Ard,

2018-07-12 9:57 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>
> On 12 July 2018 at 09:40, Marcin Wojtas <mw@semihalf.com> wrote:
> > ICU (Interrupt Consolidation Unit) is a mechanism,
> > that allows to send-message based interrupts from the
> > CP110 unit (South Bridge) to the Application Processor
> > hardware block. After dispatching the interrupts in the
> > GIC are generated.
> >
> > This patch adds a basic version of the library, that
> > allows to configure a static mapping between CP110
> > interfaces and GICv2 of the Armada7k8k SoC family.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
> >  Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
> >  Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++
>
> Does it make sense for a library at this level in the hierarchy (i.e.,
> generic Marvell) to carry all the default mappings for all the
> interrupts you have below? Doesn't those make this library specific to
> 7k8k ?
>
> Perhaps you could add a separate library class to obtain those
> defaults, and have special implementations for different SoC types.


In the local tree I have another SoC family, which is using same CP110
and different Application Processor unit. Initially I did exactly what
you described, but it turned out, the only difference needed for
providing the default mapping was the ICU_MSI structure and CP110 base
addresses.

So finally there was big, unnecessary duplication, which I removed by
putting the SoC-specific description part to the appropriate library.
Would it be ok, if we leave it as-is in such circumstances?

Best regards,
Marcin

>
>
>
> >  3 files changed, 399 insertions(+)
> >  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
> >  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
> >  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c
> >
> > diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> > new file mode 100644
> > index 0000000..0010141
> > --- /dev/null
> > +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> > @@ -0,0 +1,38 @@
> > +## @file
> > +#
> > +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
> > +#
> > +#  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.
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001A
> > +  BASE_NAME                      = IcuLib
> > +  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = ArmadaIcuLib
> > +
> > +[Sources]
> > +  IcuLib.c
> > +
> > +[Packages]
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > +  ArmadaSoCDescLib
> > +  DebugLib
> > +  IoLib
> > +  PcdLib
> > +
> > +[FixedPcd]
> > +  gMarvellTokenSpaceGuid.PcdMaxCpCount
> > diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> > new file mode 100644
> > index 0000000..4bf2298
> > --- /dev/null
> > +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> > @@ -0,0 +1,46 @@
> > +/**
> > +*
> > +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> > +*
> > +*  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.
> > +*
> > +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> > +*  ICU - Interrupt Consolidation Unit
> > +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> > +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> > +*
> > +**/
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Library/ArmadaIcuLib.h>
> > +#include <Library/ArmadaSoCDescLib.h>
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +
> > +#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000
> > +
> > +#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
> > +#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
> > +#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
> > +#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))
> > +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
> > +
> > +#define ICU_INT_ENABLE_OFFSET    24
> > +#define ICU_IS_EDGE_OFFSET       28
> > +#define ICU_GROUP_OFFSET         29
> > +
> > +#define ICU_MAX_SUPPORTED_UNITS  2
> > +#define ICU_MAX_IRQS_PER_CP      64
> > +
> > +#define MAX_ICU_IRQS             207
> > diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> > new file mode 100644
> > index 0000000..4ac98aa
> > --- /dev/null
> > +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> > @@ -0,0 +1,315 @@
> > +/**
> > +*
> > +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> > +*
> > +*  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.
> > +*
> > +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> > +*  ICU - Interrupt Consolidation Unit
> > +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> > +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> > +*
> > +**/
> > +
> > +#include "IcuLib.h"
> > +
> > +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> > +
> > +STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
> > +  {22,   0, Level}, /* PCIx4 INT A interrupt */
> > +  {23,   1, Level}, /* PCIx1 INT A interrupt */
> > +  {24,   2, Level}, /* PCIx1 INT A interrupt */
> > +  {27,   3, Level}, /* SD/MMC */
> > +  {33,   4, Level}, /* PPv2 DBG AXI monitor */
> > +  {34,   4, Level}, /* HB1      AXI monitor */
> > +  {35,   4, Level}, /* AP       AXI monitor */
> > +  {36,   4, Level}, /* PPv2     AXI monitor */
> > +  {39,   5, Level}, /* PPv2 Irq */
> > +  {40,   6, Level}, /* PPv2 Irq */
> > +  {41,   7, Level}, /* PPv2 Irq */
> > +  {43,   8, Level}, /* PPv2 Irq */
> > +  {44,   9, Level}, /* PPv2 Irq */
> > +  {45,  10, Level}, /* PPv2 Irq */
> > +  {47,  11, Level}, /* PPv2 Irq */
> > +  {48,  12, Level}, /* PPv2 Irq */
> > +  {49,  13, Level}, /* PPv2 Irq */
> > +  {51,  14, Level}, /* PPv2 Irq */
> > +  {52,  15, Level}, /* PPv2 Irq */
> > +  {53,  16, Level}, /* PPv2 Irq */
> > +  {55,  17, Level}, /* PPv2 Irq */
> > +  {56,  18, Level}, /* PPv2 Irq */
> > +  {57,  19, Level}, /* PPv2 Irq */
> > +  {59,  20, Level}, /* PPv2 Irq */
> > +  {60,  21, Level}, /* PPv2 Irq */
> > +  {61,  22, Level}, /* PPv2 Irq */
> > +  {63,  23, Level}, /* PPv2 Irq */
> > +  {64,  24, Level}, /* PPv2 Irq */
> > +  {65,  25, Level}, /* PPv2 Irq */
> > +  {67,  26, Level}, /* PPv2 Irq */
> > +  {68,  27, Level}, /* PPv2 Irq */
> > +  {69,  28, Level}, /* PPv2 Irq */
> > +  {71,  29, Level}, /* PPv2 Irq */
> > +  {72,  30, Level}, /* PPv2 Irq */
> > +  {73,  31, Level}, /* PPv2 Irq */
> > +  {78,  32, Level}, /* MG Irq */
> > +  {79,  33, Level}, /* GPIO 56-63 */
> > +  {80,  34, Level}, /* GPIO 48-55 */
> > +  {81,  35, Level}, /* GPIO 40-47 */
> > +  {82,  36, Level}, /* GPIO 32-39 */
> > +  {83,  37, Level}, /* GPIO 24-31 */
> > +  {84,  38, Level}, /* GPIO 16-23 */
> > +  {85,  39, Level}, /* GPIO  8-15 */
> > +  {86,  40, Level}, /* GPIO  0-7  */
> > +  {88,  41, Level}, /* EIP-197 ring-0 */
> > +  {89,  42, Level}, /* EIP-197 ring-1 */
> > +  {90,  43, Level}, /* EIP-197 ring-2 */
> > +  {91,  44, Level}, /* EIP-197 ring-3 */
> > +  {92,  45, Level}, /* EIP-197 int */
> > +  {95,  46, Level}, /* EIP-150 Irq */
> > +  {102, 47, Level}, /* USB3 Device Irq */
> > +  {105, 48, Level}, /* USB3 Host-1 Irq */
> > +  {106, 49, Level}, /* USB3 Host-0 Irq */
> > +  {107, 50, Level}, /* SATA Host-1 Irq */
> > +  {109, 50, Level}, /* SATA Host-0 Irq */
> > +  {115, 52, Level}, /* NAND Irq */
> > +  {117, 53, Level}, /* SPI-1 Irq */
> > +  {118, 54, Level}, /* SPI-0 Irq */
> > +  {120, 55, Level}, /* I2C 0 Irq */
> > +  {121, 56, Level}, /* I2C 1 Irq */
> > +  {122, 57, Level}, /* UART 0 Irq */
> > +  {123, 58, Level}, /* UART 1 Irq */
> > +  {124, 59, Level}, /* UART 2 Irq */
> > +  {125, 60, Level}, /* UART 3 Irq */
> > +  {127, 61, Level}, /* GOP-3 Irq */
> > +  {128, 62, Level}, /* GOP-2 Irq */
> > +  {129, 63, Level}, /* GOP-0 Irq */
> > +};
> > +
> > +/*
> > + * SEI - System Error Interrupts
> > + * Note: SPI ID 0-20 are reserved for North-Bridge
> > + */
> > +STATIC ICU_IRQ IrqMapSei[] = {
> > +  {11,  21, Level}, /* SEI error CP-2-CP */
> > +  {15,  22, Level}, /* PIDI-64 SOC */
> > +  {16,  23, Level}, /* D2D error Irq */
> > +  {17,  24, Level}, /* D2D Irq */
> > +  {18,  25, Level}, /* NAND error */
> > +  {19,  26, Level}, /* PCIx4 error */
> > +  {20,  27, Level}, /* PCIx1_0 error */
> > +  {21,  28, Level}, /* PCIx1_1 error */
> > +  {25,  29, Level}, /* SDIO reg error */
> > +  {75,  30, Level}, /* IOB error */
> > +  {94,  31, Level}, /* EIP150 error */
> > +  {97,  32, Level}, /* XOR-1 system error */
> > +  {99,  33, Level}, /* XOR-0 system error */
> > +  {108, 34, Level}, /* SATA-1 error */
> > +  {110, 35, Level}, /* SATA-0 error */
> > +  {114, 36, Level}, /* TDM-MC error */
> > +  {116, 37, Level}, /* DFX server Irq */
> > +  {117, 38, Level}, /* Device bus error */
> > +  {147, 39, Level}, /* Audio error */
> > +  {171, 40, Level}, /* PIDI Sync error */
> > +};
> > +
> > +/* REI - RAM Error Interrupts */
> > +STATIC CONST ICU_IRQ IrqMapRei[] = {
> > +  {12,  0, Level}, /* REI error CP-2-CP */
> > +  {26,  1, Level}, /* SDIO memory error */
> > +  {87,  2, Level}, /* EIP-197 ECC error */
> > +  {93,  3, Edge},  /* EIP-150 RAM error */
> > +  {96,  4, Level}, /* XOR-1 memory Irq */
> > +  {98,  5, Level}, /* XOR-0 memory Irq */
> > +  {100, 6, Edge},  /* USB3 device tx parity */
> > +  {101, 7, Edge},  /* USB3 device rq parity */
> > +  {103, 8, Edge},  /* USB3H-1 RAM error */
> > +  {104, 9, Edge},  /* USB3H-0 RAM error */
> > +};
> > +
> > +STATIC CONST ICU_CONFIG IcuConfigDefault = {
> > +  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
> > +  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
> > +  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
> > +};
> > +
> > +STATIC
> > +VOID
> > +IcuClearIrq (
> > +  IN UINTN IcuBase,
> > +  IN UINTN Nr
> > +)
> > +{
> > +  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
> > +}
> > +
> > +STATIC
> > +VOID
> > +IcuSetIrq (
> > +  IN UINTN           IcuBase,
> > +  IN CONST ICU_IRQ  *Irq,
> > +  IN UINTN           SpiBase,
> > +  IN ICU_GROUP       Group
> > +  )
> > +{
> > +  UINT32 IcuInt;
> > +
> > +  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
> > +  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
> > +  IcuInt |= Group << ICU_GROUP_OFFSET;
> > +
> > +  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
> > +}
> > +
> > +STATIC
> > +VOID
> > +IcuConfigure (
> > +  IN UINTN             CpIndex,
> > +  IN MV_SOC_ICU_DESC  *IcuDesc,
> > +  IN CONST ICU_CONFIG *Config
> > +  )
> > +{
> > +  UINTN IcuBase, Index, SpiOffset, SpiBase;
> > +  CONST ICU_IRQ *Irq;
> > +  ICU_MSI *Msi;
> > +
> > +  /* Get ICU registers base address */
> > +  IcuBase = ICU_REG_BASE (CpIndex);
> > +  /* Get the base of the GIC SPI ID in the MSI message */
> > +  SpiBase = IcuDesc->IcuSpiBase;
> > +  /* Get multiple CP110 instances SPI ID shift */
> > +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> > +  /* Get MSI addresses per interrupt group */
> > +  Msi = IcuDesc->IcuMsi;
> > +
> > +  /* Set the address for SET_SPI and CLR_SPI registers in AP */
> > +  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
> > +    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
> > +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
> > +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
> > +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
> > +  }
> > +
> > +  /* Mask all ICU interrupts */
> > +  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
> > +    IcuClearIrq (IcuBase, Index);
> > +  }
> > +
> > +  /* Configure the ICU interrupt lines */
> > +  Irq = Config->NonSecure.Map;
> > +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> > +    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
> > +  }
> > +
> > +  Irq = Config->Sei.Map;
> > +  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
> > +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
> > +  }
> > +
> > +  Irq = Config->Rei.Map;
> > +  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
> > +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
> > +  }
> > +}
> > +
> > +STATIC
> > +VOID
> > +IcuClearGicSpi (
> > +  IN UINTN             CpIndex,
> > +  IN MV_SOC_ICU_DESC  *IcuDesc
> > +  )
> > +{
> > +  CONST ICU_CONFIG *Config;
> > +  UINTN Index, SpiOffset, SpiBase;
> > +  CONST ICU_IRQ *Irq;
> > +  ICU_MSI *Msi;
> > +
> > +  Config = &IcuConfigDefault;
> > +
> > +  /* Get the base of the GIC SPI ID in the MSI message */
> > +  SpiBase = IcuDesc->IcuSpiBase;
> > +  /* Get multiple CP110 instances SPI ID shift */
> > +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> > +  /* Get MSI addresses per interrupt group */
> > +  Msi = IcuDesc->IcuMsi;
> > +
> > +  /* Clear ICU-generated GIC SPI interrupts */
> > +  Irq = Config->NonSecure.Map;
> > +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> > +    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
> > +  }
> > +}
> > +
> > +VOID
> > +EFIAPI
> > +IcuCleanUp (
> > +  IN EFI_EVENT  Event,
> > +  IN VOID      *Context
> > +  )
> > +{
> > +  MV_SOC_ICU_DESC *IcuDesc;
> > +  UINTN CpCount, CpIndex;
> > +
> > +  IcuDesc = Context;
> > +
> > +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> > +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> > +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> > +  }
> > +
> > +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> > +    IcuClearGicSpi (CpIndex, IcuDesc);
> > +  }
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +ArmadaIcuInitialize (
> > +  )
> > +{
> > +  MV_SOC_ICU_DESC *IcuDesc;
> > +  UINTN CpCount, CpIndex;
> > +  EFI_STATUS Status;
> > +
> > +  /*
> > +   * Due to limited amount of interrupt lanes, only 2 units can be
> > +   * wired to the GIC.
> > +   */
> > +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> > +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
> > +      ICU_MAX_SUPPORTED_UNITS,
> > +      __FUNCTION__));
> > +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> > +  }
> > +
> > +  /* Obtain SoC description of the ICU */
> > +  Status = ArmadaSoCDescIcuGet (&IcuDesc);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  /* Configure default ICU to GIC interrupt mapping for each CP110 */
> > +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> > +    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
> > +  }
> > +
> > +  /*
> > +   * In order to be immune to the OS capability of clearing ICU-generated
> > +   * GIC interrupts, register ExitBootServices event, that will
> > +   * make sure they remain disabled during OS boot.
> > +   */
> > +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> > +                  TPL_NOTIFY,
> > +                  IcuCleanUp,
> > +                  IcuDesc,
> > +                  &EfiExitBootServicesEvent);
> > +
> > +  return Status;
> > +}
> > --
> > 2.7.4
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Leif Lindholm 7 years, 7 months ago

On Thu, Jul 12, 2018 at 09:40:00AM +0200, Marcin Wojtas wrote:
> ICU (Interrupt Consolidation Unit) is a mechanism,
> that allows to send-message based interrupts from the
> CP110 unit (South Bridge) to the Application Processor
> hardware block. After dispatching the interrupts in the
> GIC are generated.
> 
> This patch adds a basic version of the library, that
> allows to configure a static mapping between CP110
> interfaces and GICv2 of the Armada7k8k SoC family.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
>  Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
>  Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++
>  3 files changed, 399 insertions(+)
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c
>
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> new file mode 100644
> index 0000000..0010141
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> @@ -0,0 +1,38 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
> +#
> +#  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.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = IcuLib
> +  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmadaIcuLib
> +
> +[Sources]
> +  IcuLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmadaSoCDescLib
> +  DebugLib
> +  IoLib
> +  PcdLib
> +
> +[FixedPcd]
> +  gMarvellTokenSpaceGuid.PcdMaxCpCount
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> new file mode 100644
> index 0000000..4bf2298
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> @@ -0,0 +1,46 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  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.
> +*
> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> +*  ICU - Interrupt Consolidation Unit
> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> +*
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Library/ArmadaIcuLib.h>
> +#include <Library/ArmadaSoCDescLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000

Macro missing parentheses.

> +
> +#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
> +#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
> +#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
> +#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))

Can that 0x10 be given a descriptive #define?

> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)

Does this 4 signify sizeof (UINT32) - i.e. does it get the offset into
a series of 32-bit registers? If so, please use that instead.
Also, please use parentheses around multiplication consistently
(either not at all or everywhere).

> +
> +#define ICU_INT_ENABLE_OFFSET    24
> +#define ICU_IS_EDGE_OFFSET       28
> +#define ICU_GROUP_OFFSET         29
> +
> +#define ICU_MAX_SUPPORTED_UNITS  2
> +#define ICU_MAX_IRQS_PER_CP      64
> +
> +#define MAX_ICU_IRQS             207
> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> new file mode 100644
> index 0000000..4ac98aa
> --- /dev/null
> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> @@ -0,0 +1,315 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  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.
> +*
> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> +*  ICU - Interrupt Consolidation Unit
> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> +*
> +**/
> +
> +#include "IcuLib.h"
> +
> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;

STATIC?
and mExitBootServicesEvent?
And no need to initialise it.

(Yes, I see you've looked at existing templates, and I'm sure we'll
get around to cleaning those up at some point.)

Digging into the origin of this antipattern briefly made me question
my knowledge of the C language, until I realised EFI_EVENT is a
typedef for VOID *.

> +
> +STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
> +  {22,   0, Level}, /* PCIx4 INT A interrupt */
> +  {23,   1, Level}, /* PCIx1 INT A interrupt */
> +  {24,   2, Level}, /* PCIx1 INT A interrupt */
> +  {27,   3, Level}, /* SD/MMC */
> +  {33,   4, Level}, /* PPv2 DBG AXI monitor */
> +  {34,   4, Level}, /* HB1      AXI monitor */
> +  {35,   4, Level}, /* AP       AXI monitor */
> +  {36,   4, Level}, /* PPv2     AXI monitor */
> +  {39,   5, Level}, /* PPv2 Irq */
> +  {40,   6, Level}, /* PPv2 Irq */
> +  {41,   7, Level}, /* PPv2 Irq */
> +  {43,   8, Level}, /* PPv2 Irq */
> +  {44,   9, Level}, /* PPv2 Irq */
> +  {45,  10, Level}, /* PPv2 Irq */
> +  {47,  11, Level}, /* PPv2 Irq */
> +  {48,  12, Level}, /* PPv2 Irq */
> +  {49,  13, Level}, /* PPv2 Irq */
> +  {51,  14, Level}, /* PPv2 Irq */
> +  {52,  15, Level}, /* PPv2 Irq */
> +  {53,  16, Level}, /* PPv2 Irq */
> +  {55,  17, Level}, /* PPv2 Irq */
> +  {56,  18, Level}, /* PPv2 Irq */
> +  {57,  19, Level}, /* PPv2 Irq */
> +  {59,  20, Level}, /* PPv2 Irq */
> +  {60,  21, Level}, /* PPv2 Irq */
> +  {61,  22, Level}, /* PPv2 Irq */
> +  {63,  23, Level}, /* PPv2 Irq */
> +  {64,  24, Level}, /* PPv2 Irq */
> +  {65,  25, Level}, /* PPv2 Irq */
> +  {67,  26, Level}, /* PPv2 Irq */
> +  {68,  27, Level}, /* PPv2 Irq */
> +  {69,  28, Level}, /* PPv2 Irq */
> +  {71,  29, Level}, /* PPv2 Irq */
> +  {72,  30, Level}, /* PPv2 Irq */
> +  {73,  31, Level}, /* PPv2 Irq */
> +  {78,  32, Level}, /* MG Irq */
> +  {79,  33, Level}, /* GPIO 56-63 */
> +  {80,  34, Level}, /* GPIO 48-55 */
> +  {81,  35, Level}, /* GPIO 40-47 */
> +  {82,  36, Level}, /* GPIO 32-39 */
> +  {83,  37, Level}, /* GPIO 24-31 */
> +  {84,  38, Level}, /* GPIO 16-23 */
> +  {85,  39, Level}, /* GPIO  8-15 */
> +  {86,  40, Level}, /* GPIO  0-7  */
> +  {88,  41, Level}, /* EIP-197 ring-0 */
> +  {89,  42, Level}, /* EIP-197 ring-1 */
> +  {90,  43, Level}, /* EIP-197 ring-2 */
> +  {91,  44, Level}, /* EIP-197 ring-3 */
> +  {92,  45, Level}, /* EIP-197 int */
> +  {95,  46, Level}, /* EIP-150 Irq */
> +  {102, 47, Level}, /* USB3 Device Irq */
> +  {105, 48, Level}, /* USB3 Host-1 Irq */
> +  {106, 49, Level}, /* USB3 Host-0 Irq */
> +  {107, 50, Level}, /* SATA Host-1 Irq */
> +  {109, 50, Level}, /* SATA Host-0 Irq */
> +  {115, 52, Level}, /* NAND Irq */
> +  {117, 53, Level}, /* SPI-1 Irq */
> +  {118, 54, Level}, /* SPI-0 Irq */
> +  {120, 55, Level}, /* I2C 0 Irq */
> +  {121, 56, Level}, /* I2C 1 Irq */
> +  {122, 57, Level}, /* UART 0 Irq */
> +  {123, 58, Level}, /* UART 1 Irq */
> +  {124, 59, Level}, /* UART 2 Irq */
> +  {125, 60, Level}, /* UART 3 Irq */
> +  {127, 61, Level}, /* GOP-3 Irq */
> +  {128, 62, Level}, /* GOP-2 Irq */
> +  {129, 63, Level}, /* GOP-0 Irq */
> +};
> +
> +/*
> + * SEI - System Error Interrupts
> + * Note: SPI ID 0-20 are reserved for North-Bridge
> + */
> +STATIC ICU_IRQ IrqMapSei[] = {
> +  {11,  21, Level}, /* SEI error CP-2-CP */
> +  {15,  22, Level}, /* PIDI-64 SOC */
> +  {16,  23, Level}, /* D2D error Irq */
> +  {17,  24, Level}, /* D2D Irq */
> +  {18,  25, Level}, /* NAND error */
> +  {19,  26, Level}, /* PCIx4 error */
> +  {20,  27, Level}, /* PCIx1_0 error */
> +  {21,  28, Level}, /* PCIx1_1 error */
> +  {25,  29, Level}, /* SDIO reg error */
> +  {75,  30, Level}, /* IOB error */
> +  {94,  31, Level}, /* EIP150 error */
> +  {97,  32, Level}, /* XOR-1 system error */
> +  {99,  33, Level}, /* XOR-0 system error */
> +  {108, 34, Level}, /* SATA-1 error */
> +  {110, 35, Level}, /* SATA-0 error */
> +  {114, 36, Level}, /* TDM-MC error */
> +  {116, 37, Level}, /* DFX server Irq */
> +  {117, 38, Level}, /* Device bus error */
> +  {147, 39, Level}, /* Audio error */
> +  {171, 40, Level}, /* PIDI Sync error */
> +};
> +
> +/* REI - RAM Error Interrupts */
> +STATIC CONST ICU_IRQ IrqMapRei[] = {
> +  {12,  0, Level}, /* REI error CP-2-CP */
> +  {26,  1, Level}, /* SDIO memory error */
> +  {87,  2, Level}, /* EIP-197 ECC error */
> +  {93,  3, Edge},  /* EIP-150 RAM error */
> +  {96,  4, Level}, /* XOR-1 memory Irq */
> +  {98,  5, Level}, /* XOR-0 memory Irq */
> +  {100, 6, Edge},  /* USB3 device tx parity */
> +  {101, 7, Edge},  /* USB3 device rq parity */
> +  {103, 8, Edge},  /* USB3H-1 RAM error */
> +  {104, 9, Edge},  /* USB3H-0 RAM error */
> +};
> +
> +STATIC CONST ICU_CONFIG IcuConfigDefault = {

The combination of CONST and Default confuses me slightly, as it
mildly implies there can be other config objects floating around -
whereas this just appears to refer to what the hardware is configured
to on every boot.
.
Would IcuInitialConfig work as well?

> +  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
> +  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
> +  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
> +};
> +
> +STATIC
> +VOID
> +IcuClearIrq (
> +  IN UINTN IcuBase,
> +  IN UINTN Nr
> +)
> +{
> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
> +}
> +
> +STATIC
> +VOID
> +IcuSetIrq (
> +  IN UINTN           IcuBase,
> +  IN CONST ICU_IRQ  *Irq,
> +  IN UINTN           SpiBase,
> +  IN ICU_GROUP       Group
> +  )
> +{
> +  UINT32 IcuInt;
> +
> +  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
> +  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
> +  IcuInt |= Group << ICU_GROUP_OFFSET;
> +
> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
> +}
> +
> +STATIC
> +VOID
> +IcuConfigure (
> +  IN UINTN             CpIndex,
> +  IN MV_SOC_ICU_DESC  *IcuDesc,
> +  IN CONST ICU_CONFIG *Config
> +  )
> +{
> +  UINTN IcuBase, Index, SpiOffset, SpiBase;
> +  CONST ICU_IRQ *Irq;
> +  ICU_MSI *Msi;
> +
> +  /* Get ICU registers base address */
> +  IcuBase = ICU_REG_BASE (CpIndex);
> +  /* Get the base of the GIC SPI ID in the MSI message */
> +  SpiBase = IcuDesc->IcuSpiBase;
> +  /* Get multiple CP110 instances SPI ID shift */
> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> +  /* Get MSI addresses per interrupt group */
> +  Msi = IcuDesc->IcuMsi;
> +
> +  /* Set the address for SET_SPI and CLR_SPI registers in AP */
> +  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);

Hmm, this common operation (split 64 into 2x32) feels like something
EDK2 would have a macro for, but I can't find one. I.e. POINTER_HIGH32
and POINTER_LOW32.
I see it would be useful in other platforms' PCI code as well.

Anyway - the above 4 lines are all too long.

/
    Leif

> +  }
> +
> +  /* Mask all ICU interrupts */
> +  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
> +    IcuClearIrq (IcuBase, Index);
> +  }
> +
> +  /* Configure the ICU interrupt lines */
> +  Irq = Config->NonSecure.Map;
> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
> +  }
> +
> +  Irq = Config->Sei.Map;
> +  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
> +  }
> +
> +  Irq = Config->Rei.Map;
> +  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
> +  }
> +}
> +
> +STATIC
> +VOID
> +IcuClearGicSpi (
> +  IN UINTN             CpIndex,
> +  IN MV_SOC_ICU_DESC  *IcuDesc
> +  )
> +{
> +  CONST ICU_CONFIG *Config;
> +  UINTN Index, SpiOffset, SpiBase;
> +  CONST ICU_IRQ *Irq;
> +  ICU_MSI *Msi;
> +
> +  Config = &IcuConfigDefault;
> +
> +  /* Get the base of the GIC SPI ID in the MSI message */
> +  SpiBase = IcuDesc->IcuSpiBase;
> +  /* Get multiple CP110 instances SPI ID shift */
> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> +  /* Get MSI addresses per interrupt group */
> +  Msi = IcuDesc->IcuMsi;
> +
> +  /* Clear ICU-generated GIC SPI interrupts */
> +  Irq = Config->NonSecure.Map;
> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> +    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
> +  }
> +}
> +
> +VOID
> +EFIAPI
> +IcuCleanUp (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  MV_SOC_ICU_DESC *IcuDesc;
> +  UINTN CpCount, CpIndex;
> +
> +  IcuDesc = Context;
> +
> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> +  }
> +
> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> +    IcuClearGicSpi (CpIndex, IcuDesc);
> +  }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaIcuInitialize (
> +  )
> +{
> +  MV_SOC_ICU_DESC *IcuDesc;
> +  UINTN CpCount, CpIndex;
> +  EFI_STATUS Status;
> +
> +  /*
> +   * Due to limited amount of interrupt lanes, only 2 units can be
> +   * wired to the GIC.
> +   */
> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
> +      ICU_MAX_SUPPORTED_UNITS,
> +      __FUNCTION__));
> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> +  }
> +
> +  /* Obtain SoC description of the ICU */
> +  Status = ArmadaSoCDescIcuGet (&IcuDesc);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Configure default ICU to GIC interrupt mapping for each CP110 */
> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> +    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
> +  }
> +
> +  /*
> +   * In order to be immune to the OS capability of clearing ICU-generated
> +   * GIC interrupts, register ExitBootServices event, that will
> +   * make sure they remain disabled during OS boot.
> +   */
> +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                  TPL_NOTIFY,
> +                  IcuCleanUp,
> +                  IcuDesc,
> +                  &EfiExitBootServicesEvent);
> +
> +  return Status;
> +}
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Marcin Wojtas 7 years, 7 months ago

Hi Leif,



2018-07-12 12:35 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Jul 12, 2018 at 09:40:00AM +0200, Marcin Wojtas wrote:
>> ICU (Interrupt Consolidation Unit) is a mechanism,
>> that allows to send-message based interrupts from the
>> CP110 unit (South Bridge) to the Application Processor
>> hardware block. After dispatching the interrupts in the
>> GIC are generated.
>>
>> This patch adds a basic version of the library, that
>> allows to configure a static mapping between CP110
>> interfaces and GICv2 of the Armada7k8k SoC family.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
>>  Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
>>  Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++
>>  3 files changed, 399 insertions(+)
>>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
>>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
>>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c
>>
>> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
>> new file mode 100644
>> index 0000000..0010141
>> --- /dev/null
>> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
>> @@ -0,0 +1,38 @@
>> +## @file
>> +#
>> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
>> +#
>> +#  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.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001A
>> +  BASE_NAME                      = IcuLib
>> +  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = ArmadaIcuLib
>> +
>> +[Sources]
>> +  IcuLib.c
>> +
>> +[Packages]
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  MdePkg/MdePkg.dec
>> +  Silicon/Marvell/Marvell.dec
>> +
>> +[LibraryClasses]
>> +  ArmadaSoCDescLib
>> +  DebugLib
>> +  IoLib
>> +  PcdLib
>> +
>> +[FixedPcd]
>> +  gMarvellTokenSpaceGuid.PcdMaxCpCount
>> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
>> new file mode 100644
>> index 0000000..4bf2298
>> --- /dev/null
>> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
>> @@ -0,0 +1,46 @@
>> +/**
>> +*
>> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
>> +*
>> +*  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.
>> +*
>> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
>> +*  ICU - Interrupt Consolidation Unit
>> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
>> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
>> +*
>> +**/
>> +
>> +#include <Uefi.h>
>> +
>> +#include <Library/ArmadaIcuLib.h>
>> +#include <Library/ArmadaSoCDescLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000
>
> Macro missing parentheses.
>
>> +
>> +#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
>> +#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
>> +#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
>> +#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))
>
> Can that 0x10 be given a descriptive #define?
>

0x10 is just a part of register offset calculation formula. If you
wish to replace it with macro, how about:
#define ICU_GROUP_REGISTER_BASE_SHIFT    0x10
?


>> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
>
> Does this 4 signify sizeof (UINT32) - i.e. does it get the offset into
> a series of 32-bit registers? If so, please use that instead.

Ok.

> Also, please use parentheses around multiplication consistently
> (either not at all or everywhere).
>

In previous series (board description) you recommended using the
parentheses, so I'll follow this guideline consistently.

>> +
>> +#define ICU_INT_ENABLE_OFFSET    24
>> +#define ICU_IS_EDGE_OFFSET       28
>> +#define ICU_GROUP_OFFSET         29
>> +
>> +#define ICU_MAX_SUPPORTED_UNITS  2
>> +#define ICU_MAX_IRQS_PER_CP      64
>> +
>> +#define MAX_ICU_IRQS             207
>> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
>> new file mode 100644
>> index 0000000..4ac98aa
>> --- /dev/null
>> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
>> @@ -0,0 +1,315 @@
>> +/**
>> +*
>> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
>> +*
>> +*  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.
>> +*
>> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
>> +*  ICU - Interrupt Consolidation Unit
>> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
>> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
>> +*
>> +**/
>> +
>> +#include "IcuLib.h"
>> +
>> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
>
> STATIC?
> and mExitBootServicesEvent?
> And no need to initialise it.
>

2x Ok.

> (Yes, I see you've looked at existing templates, and I'm sure we'll
> get around to cleaning those up at some point.)
>
> Digging into the origin of this antipattern briefly made me question
> my knowledge of the C language, until I realised EFI_EVENT is a
> typedef for VOID *.
>
>> +
>> +STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
>> +  {22,   0, Level}, /* PCIx4 INT A interrupt */
>> +  {23,   1, Level}, /* PCIx1 INT A interrupt */
>> +  {24,   2, Level}, /* PCIx1 INT A interrupt */
>> +  {27,   3, Level}, /* SD/MMC */
>> +  {33,   4, Level}, /* PPv2 DBG AXI monitor */
>> +  {34,   4, Level}, /* HB1      AXI monitor */
>> +  {35,   4, Level}, /* AP       AXI monitor */
>> +  {36,   4, Level}, /* PPv2     AXI monitor */
>> +  {39,   5, Level}, /* PPv2 Irq */
>> +  {40,   6, Level}, /* PPv2 Irq */
>> +  {41,   7, Level}, /* PPv2 Irq */
>> +  {43,   8, Level}, /* PPv2 Irq */
>> +  {44,   9, Level}, /* PPv2 Irq */
>> +  {45,  10, Level}, /* PPv2 Irq */
>> +  {47,  11, Level}, /* PPv2 Irq */
>> +  {48,  12, Level}, /* PPv2 Irq */
>> +  {49,  13, Level}, /* PPv2 Irq */
>> +  {51,  14, Level}, /* PPv2 Irq */
>> +  {52,  15, Level}, /* PPv2 Irq */
>> +  {53,  16, Level}, /* PPv2 Irq */
>> +  {55,  17, Level}, /* PPv2 Irq */
>> +  {56,  18, Level}, /* PPv2 Irq */
>> +  {57,  19, Level}, /* PPv2 Irq */
>> +  {59,  20, Level}, /* PPv2 Irq */
>> +  {60,  21, Level}, /* PPv2 Irq */
>> +  {61,  22, Level}, /* PPv2 Irq */
>> +  {63,  23, Level}, /* PPv2 Irq */
>> +  {64,  24, Level}, /* PPv2 Irq */
>> +  {65,  25, Level}, /* PPv2 Irq */
>> +  {67,  26, Level}, /* PPv2 Irq */
>> +  {68,  27, Level}, /* PPv2 Irq */
>> +  {69,  28, Level}, /* PPv2 Irq */
>> +  {71,  29, Level}, /* PPv2 Irq */
>> +  {72,  30, Level}, /* PPv2 Irq */
>> +  {73,  31, Level}, /* PPv2 Irq */
>> +  {78,  32, Level}, /* MG Irq */
>> +  {79,  33, Level}, /* GPIO 56-63 */
>> +  {80,  34, Level}, /* GPIO 48-55 */
>> +  {81,  35, Level}, /* GPIO 40-47 */
>> +  {82,  36, Level}, /* GPIO 32-39 */
>> +  {83,  37, Level}, /* GPIO 24-31 */
>> +  {84,  38, Level}, /* GPIO 16-23 */
>> +  {85,  39, Level}, /* GPIO  8-15 */
>> +  {86,  40, Level}, /* GPIO  0-7  */
>> +  {88,  41, Level}, /* EIP-197 ring-0 */
>> +  {89,  42, Level}, /* EIP-197 ring-1 */
>> +  {90,  43, Level}, /* EIP-197 ring-2 */
>> +  {91,  44, Level}, /* EIP-197 ring-3 */
>> +  {92,  45, Level}, /* EIP-197 int */
>> +  {95,  46, Level}, /* EIP-150 Irq */
>> +  {102, 47, Level}, /* USB3 Device Irq */
>> +  {105, 48, Level}, /* USB3 Host-1 Irq */
>> +  {106, 49, Level}, /* USB3 Host-0 Irq */
>> +  {107, 50, Level}, /* SATA Host-1 Irq */
>> +  {109, 50, Level}, /* SATA Host-0 Irq */
>> +  {115, 52, Level}, /* NAND Irq */
>> +  {117, 53, Level}, /* SPI-1 Irq */
>> +  {118, 54, Level}, /* SPI-0 Irq */
>> +  {120, 55, Level}, /* I2C 0 Irq */
>> +  {121, 56, Level}, /* I2C 1 Irq */
>> +  {122, 57, Level}, /* UART 0 Irq */
>> +  {123, 58, Level}, /* UART 1 Irq */
>> +  {124, 59, Level}, /* UART 2 Irq */
>> +  {125, 60, Level}, /* UART 3 Irq */
>> +  {127, 61, Level}, /* GOP-3 Irq */
>> +  {128, 62, Level}, /* GOP-2 Irq */
>> +  {129, 63, Level}, /* GOP-0 Irq */
>> +};
>> +
>> +/*
>> + * SEI - System Error Interrupts
>> + * Note: SPI ID 0-20 are reserved for North-Bridge
>> + */
>> +STATIC ICU_IRQ IrqMapSei[] = {
>> +  {11,  21, Level}, /* SEI error CP-2-CP */
>> +  {15,  22, Level}, /* PIDI-64 SOC */
>> +  {16,  23, Level}, /* D2D error Irq */
>> +  {17,  24, Level}, /* D2D Irq */
>> +  {18,  25, Level}, /* NAND error */
>> +  {19,  26, Level}, /* PCIx4 error */
>> +  {20,  27, Level}, /* PCIx1_0 error */
>> +  {21,  28, Level}, /* PCIx1_1 error */
>> +  {25,  29, Level}, /* SDIO reg error */
>> +  {75,  30, Level}, /* IOB error */
>> +  {94,  31, Level}, /* EIP150 error */
>> +  {97,  32, Level}, /* XOR-1 system error */
>> +  {99,  33, Level}, /* XOR-0 system error */
>> +  {108, 34, Level}, /* SATA-1 error */
>> +  {110, 35, Level}, /* SATA-0 error */
>> +  {114, 36, Level}, /* TDM-MC error */
>> +  {116, 37, Level}, /* DFX server Irq */
>> +  {117, 38, Level}, /* Device bus error */
>> +  {147, 39, Level}, /* Audio error */
>> +  {171, 40, Level}, /* PIDI Sync error */
>> +};
>> +
>> +/* REI - RAM Error Interrupts */
>> +STATIC CONST ICU_IRQ IrqMapRei[] = {
>> +  {12,  0, Level}, /* REI error CP-2-CP */
>> +  {26,  1, Level}, /* SDIO memory error */
>> +  {87,  2, Level}, /* EIP-197 ECC error */
>> +  {93,  3, Edge},  /* EIP-150 RAM error */
>> +  {96,  4, Level}, /* XOR-1 memory Irq */
>> +  {98,  5, Level}, /* XOR-0 memory Irq */
>> +  {100, 6, Edge},  /* USB3 device tx parity */
>> +  {101, 7, Edge},  /* USB3 device rq parity */
>> +  {103, 8, Edge},  /* USB3H-1 RAM error */
>> +  {104, 9, Edge},  /* USB3H-0 RAM error */
>> +};
>> +
>> +STATIC CONST ICU_CONFIG IcuConfigDefault = {
>
> The combination of CONST and Default confuses me slightly, as it
> mildly implies there can be other config objects floating around -
> whereas this just appears to refer to what the hardware is configured
> to on every boot.
> .
> Would IcuInitialConfig work as well?

Ok, will rename.

>
>> +  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
>> +  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
>> +  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
>> +};
>> +
>> +STATIC
>> +VOID
>> +IcuClearIrq (
>> +  IN UINTN IcuBase,
>> +  IN UINTN Nr
>> +)
>> +{
>> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
>> +}
>> +
>> +STATIC
>> +VOID
>> +IcuSetIrq (
>> +  IN UINTN           IcuBase,
>> +  IN CONST ICU_IRQ  *Irq,
>> +  IN UINTN           SpiBase,
>> +  IN ICU_GROUP       Group
>> +  )
>> +{
>> +  UINT32 IcuInt;
>> +
>> +  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
>> +  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
>> +  IcuInt |= Group << ICU_GROUP_OFFSET;
>> +
>> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
>> +}
>> +
>> +STATIC
>> +VOID
>> +IcuConfigure (
>> +  IN UINTN             CpIndex,
>> +  IN MV_SOC_ICU_DESC  *IcuDesc,
>> +  IN CONST ICU_CONFIG *Config
>> +  )
>> +{
>> +  UINTN IcuBase, Index, SpiOffset, SpiBase;
>> +  CONST ICU_IRQ *Irq;
>> +  ICU_MSI *Msi;
>> +
>> +  /* Get ICU registers base address */
>> +  IcuBase = ICU_REG_BASE (CpIndex);
>> +  /* Get the base of the GIC SPI ID in the MSI message */
>> +  SpiBase = IcuDesc->IcuSpiBase;
>> +  /* Get multiple CP110 instances SPI ID shift */
>> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
>> +  /* Get MSI addresses per interrupt group */
>> +  Msi = IcuDesc->IcuMsi;
>> +
>> +  /* Set the address for SET_SPI and CLR_SPI registers in AP */
>> +  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
>> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
>> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
>> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
>> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
>
> Hmm, this common operation (split 64 into 2x32) feels like something
> EDK2 would have a macro for, but I can't find one. I.e. POINTER_HIGH32
> and POINTER_LOW32.
> I see it would be useful in other platforms' PCI code as well.
>
> Anyway - the above 4 lines are all too long.
>

Lines are now 86 - 79 - 86 - 79 signs long - I didn't break them for
the sake of readability, but I'll do it then.

> /
>     Leif
>
>> +  }
>> +
>> +  /* Mask all ICU interrupts */
>> +  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
>> +    IcuClearIrq (IcuBase, Index);
>> +  }
>> +
>> +  /* Configure the ICU interrupt lines */
>> +  Irq = Config->NonSecure.Map;
>> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
>> +    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
>> +  }
>> +
>> +  Irq = Config->Sei.Map;
>> +  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
>> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
>> +  }
>> +
>> +  Irq = Config->Rei.Map;
>> +  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
>> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
>> +  }
>> +}
>> +
>> +STATIC
>> +VOID
>> +IcuClearGicSpi (
>> +  IN UINTN             CpIndex,
>> +  IN MV_SOC_ICU_DESC  *IcuDesc
>> +  )
>> +{
>> +  CONST ICU_CONFIG *Config;
>> +  UINTN Index, SpiOffset, SpiBase;
>> +  CONST ICU_IRQ *Irq;
>> +  ICU_MSI *Msi;
>> +
>> +  Config = &IcuConfigDefault;
>> +
>> +  /* Get the base of the GIC SPI ID in the MSI message */
>> +  SpiBase = IcuDesc->IcuSpiBase;
>> +  /* Get multiple CP110 instances SPI ID shift */
>> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
>> +  /* Get MSI addresses per interrupt group */
>> +  Msi = IcuDesc->IcuMsi;
>> +
>> +  /* Clear ICU-generated GIC SPI interrupts */
>> +  Irq = Config->NonSecure.Map;
>> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
>> +    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
>> +  }
>> +}
>> +
>> +VOID
>> +EFIAPI
>> +IcuCleanUp (
>> +  IN EFI_EVENT  Event,
>> +  IN VOID      *Context
>> +  )
>> +{
>> +  MV_SOC_ICU_DESC *IcuDesc;
>> +  UINTN CpCount, CpIndex;
>> +
>> +  IcuDesc = Context;
>> +
>> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
>> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
>> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
>> +  }
>> +
>> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
>> +    IcuClearGicSpi (CpIndex, IcuDesc);
>> +  }
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +ArmadaIcuInitialize (
>> +  )
>> +{
>> +  MV_SOC_ICU_DESC *IcuDesc;
>> +  UINTN CpCount, CpIndex;
>> +  EFI_STATUS Status;
>> +
>> +  /*
>> +   * Due to limited amount of interrupt lanes, only 2 units can be
>> +   * wired to the GIC.
>> +   */
>> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
>> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
>> +      ICU_MAX_SUPPORTED_UNITS,
>> +      __FUNCTION__));
>> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
>> +  }
>> +
>> +  /* Obtain SoC description of the ICU */
>> +  Status = ArmadaSoCDescIcuGet (&IcuDesc);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  /* Configure default ICU to GIC interrupt mapping for each CP110 */
>> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
>> +    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
>> +  }
>> +
>> +  /*
>> +   * In order to be immune to the OS capability of clearing ICU-generated
>> +   * GIC interrupts, register ExitBootServices event, that will
>> +   * make sure they remain disabled during OS boot.
>> +   */
>> +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
>> +                  TPL_NOTIFY,
>> +                  IcuCleanUp,
>> +                  IcuDesc,
>> +                  &EfiExitBootServicesEvent);
>> +
>> +  return Status;
>> +}
>> --
>> 2.7.4
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib

Posted by Leif Lindholm 7 years, 7 months ago

On Thu, Jul 12, 2018 at 12:59:16PM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 
> 
> 2018-07-12 12:35 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Jul 12, 2018 at 09:40:00AM +0200, Marcin Wojtas wrote:
> >> ICU (Interrupt Consolidation Unit) is a mechanism,
> >> that allows to send-message based interrupts from the
> >> CP110 unit (South Bridge) to the Application Processor
> >> hardware block. After dispatching the interrupts in the
> >> GIC are generated.
> >>
> >> This patch adds a basic version of the library, that
> >> allows to configure a static mapping between CP110
> >> interfaces and GICv2 of the Armada7k8k SoC family.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Silicon/Marvell/Library/IcuLib/IcuLib.inf |  38 +++
> >>  Silicon/Marvell/Library/IcuLib/IcuLib.h   |  46 +++
> >>  Silicon/Marvell/Library/IcuLib/IcuLib.c   | 315 ++++++++++++++++++++
> >>  3 files changed, 399 insertions(+)
> >>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
> >>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
> >>  create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c
> >>
> >> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.inf b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> >> new file mode 100644
> >> index 0000000..0010141
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.inf
> >> @@ -0,0 +1,38 @@
> >> +## @file
> >> +#
> >> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
> >> +#
> >> +#  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.
> >> +#
> >> +##
> >> +
> >> +[Defines]
> >> +  INF_VERSION                    = 0x0001001A
> >> +  BASE_NAME                      = IcuLib
> >> +  FILE_GUID                      = 0301c9cb-43e6-40a8-96bf-41bd0501e86d
> >> +  MODULE_TYPE                    = BASE
> >> +  VERSION_STRING                 = 1.0
> >> +  LIBRARY_CLASS                  = ArmadaIcuLib
> >> +
> >> +[Sources]
> >> +  IcuLib.c
> >> +
> >> +[Packages]
> >> +  MdeModulePkg/MdeModulePkg.dec
> >> +  MdePkg/MdePkg.dec
> >> +  Silicon/Marvell/Marvell.dec
> >> +
> >> +[LibraryClasses]
> >> +  ArmadaSoCDescLib
> >> +  DebugLib
> >> +  IoLib
> >> +  PcdLib
> >> +
> >> +[FixedPcd]
> >> +  gMarvellTokenSpaceGuid.PcdMaxCpCount
> >> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.h b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> >> new file mode 100644
> >> index 0000000..4bf2298
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.h
> >> @@ -0,0 +1,46 @@
> >> +/**
> >> +*
> >> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> >> +*
> >> +*  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.
> >> +*
> >> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> >> +*  ICU - Interrupt Consolidation Unit
> >> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> >> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> >> +*
> >> +**/
> >> +
> >> +#include <Uefi.h>
> >> +
> >> +#include <Library/ArmadaIcuLib.h>
> >> +#include <Library/ArmadaSoCDescLib.h>
> >> +#include <Library/BaseMemoryLib.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/IoLib.h>
> >> +#include <Library/MemoryAllocationLib.h>
> >> +#include <Library/PcdLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +
> >> +#define ICU_REG_BASE(Cp)        ArmadaSoCDescCpBaseGet (CpIndex) + 0x1E0000
> >
> > Macro missing parentheses.
> >
> >> +
> >> +#define ICU_SET_SPI_AL(x)       (0x10 + (0x10 * x))
> >> +#define ICU_SET_SPI_AH(x)       (0x14 + (0x10 * x))
> >> +#define ICU_CLR_SPI_AL(x)       (0x18 + (0x10 * x))
> >> +#define ICU_CLR_SPI_AH(x)       (0x1c + (0x10 * x))
> >
> > Can that 0x10 be given a descriptive #define?
> >
> 
> 0x10 is just a part of register offset calculation formula. If you
> wish to replace it with macro, how about:
> #define ICU_GROUP_REGISTER_BASE_SHIFT    0x10
> ?

Not SHIFT (which suggests << or >> to me), but OFFSET would work.

> >> +#define ICU_INT_CFG(x)          (0x100 + 4 * x)
> >
> > Does this 4 signify sizeof (UINT32) - i.e. does it get the offset into
> > a series of 32-bit registers? If so, please use that instead.
> 
> Ok.
> 
> > Also, please use parentheses around multiplication consistently
> > (either not at all or everywhere).
> >
> 
> In previous series (board description) you recommended using the
> parentheses, so I'll follow this guideline consistently.

There were parentheses around (0x10 * x) but not around 4 * x.
This was the reason for my comment.
Since it's relegated to a non-complicated compiler macro, I don't
really care whether they're there or not - but I want them consistent.

> >> +
> >> +#define ICU_INT_ENABLE_OFFSET    24
> >> +#define ICU_IS_EDGE_OFFSET       28
> >> +#define ICU_GROUP_OFFSET         29
> >> +
> >> +#define ICU_MAX_SUPPORTED_UNITS  2
> >> +#define ICU_MAX_IRQS_PER_CP      64
> >> +
> >> +#define MAX_ICU_IRQS             207
> >> diff --git a/Silicon/Marvell/Library/IcuLib/IcuLib.c b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> >> new file mode 100644
> >> index 0000000..4ac98aa
> >> --- /dev/null
> >> +++ b/Silicon/Marvell/Library/IcuLib/IcuLib.c
> >> @@ -0,0 +1,315 @@
> >> +/**
> >> +*
> >> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> >> +*
> >> +*  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.
> >> +*
> >> +*  Glossary - abbreviations used in Marvell SampleAtReset library implementation:
> >> +*  ICU - Interrupt Consolidation Unit
> >> +*  AP - Application Processor hardware block (Armada 7k8k incorporates AP806)
> >> +*  CP - South Bridge hardware blocks (Armada 7k8k incorporates CP110)
> >> +*
> >> +**/
> >> +
> >> +#include "IcuLib.h"
> >> +
> >> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> >
> > STATIC?
> > and mExitBootServicesEvent?
> > And no need to initialise it.
> >
> 
> 2x Ok.
> 
> > (Yes, I see you've looked at existing templates, and I'm sure we'll
> > get around to cleaning those up at some point.)
> >
> > Digging into the origin of this antipattern briefly made me question
> > my knowledge of the C language, until I realised EFI_EVENT is a
> > typedef for VOID *.
> >
> >> +
> >> +STATIC CONST ICU_IRQ IrqMapNonSecure[] = {
> >> +  {22,   0, Level}, /* PCIx4 INT A interrupt */
> >> +  {23,   1, Level}, /* PCIx1 INT A interrupt */
> >> +  {24,   2, Level}, /* PCIx1 INT A interrupt */
> >> +  {27,   3, Level}, /* SD/MMC */
> >> +  {33,   4, Level}, /* PPv2 DBG AXI monitor */
> >> +  {34,   4, Level}, /* HB1      AXI monitor */
> >> +  {35,   4, Level}, /* AP       AXI monitor */
> >> +  {36,   4, Level}, /* PPv2     AXI monitor */
> >> +  {39,   5, Level}, /* PPv2 Irq */
> >> +  {40,   6, Level}, /* PPv2 Irq */
> >> +  {41,   7, Level}, /* PPv2 Irq */
> >> +  {43,   8, Level}, /* PPv2 Irq */
> >> +  {44,   9, Level}, /* PPv2 Irq */
> >> +  {45,  10, Level}, /* PPv2 Irq */
> >> +  {47,  11, Level}, /* PPv2 Irq */
> >> +  {48,  12, Level}, /* PPv2 Irq */
> >> +  {49,  13, Level}, /* PPv2 Irq */
> >> +  {51,  14, Level}, /* PPv2 Irq */
> >> +  {52,  15, Level}, /* PPv2 Irq */
> >> +  {53,  16, Level}, /* PPv2 Irq */
> >> +  {55,  17, Level}, /* PPv2 Irq */
> >> +  {56,  18, Level}, /* PPv2 Irq */
> >> +  {57,  19, Level}, /* PPv2 Irq */
> >> +  {59,  20, Level}, /* PPv2 Irq */
> >> +  {60,  21, Level}, /* PPv2 Irq */
> >> +  {61,  22, Level}, /* PPv2 Irq */
> >> +  {63,  23, Level}, /* PPv2 Irq */
> >> +  {64,  24, Level}, /* PPv2 Irq */
> >> +  {65,  25, Level}, /* PPv2 Irq */
> >> +  {67,  26, Level}, /* PPv2 Irq */
> >> +  {68,  27, Level}, /* PPv2 Irq */
> >> +  {69,  28, Level}, /* PPv2 Irq */
> >> +  {71,  29, Level}, /* PPv2 Irq */
> >> +  {72,  30, Level}, /* PPv2 Irq */
> >> +  {73,  31, Level}, /* PPv2 Irq */
> >> +  {78,  32, Level}, /* MG Irq */
> >> +  {79,  33, Level}, /* GPIO 56-63 */
> >> +  {80,  34, Level}, /* GPIO 48-55 */
> >> +  {81,  35, Level}, /* GPIO 40-47 */
> >> +  {82,  36, Level}, /* GPIO 32-39 */
> >> +  {83,  37, Level}, /* GPIO 24-31 */
> >> +  {84,  38, Level}, /* GPIO 16-23 */
> >> +  {85,  39, Level}, /* GPIO  8-15 */
> >> +  {86,  40, Level}, /* GPIO  0-7  */
> >> +  {88,  41, Level}, /* EIP-197 ring-0 */
> >> +  {89,  42, Level}, /* EIP-197 ring-1 */
> >> +  {90,  43, Level}, /* EIP-197 ring-2 */
> >> +  {91,  44, Level}, /* EIP-197 ring-3 */
> >> +  {92,  45, Level}, /* EIP-197 int */
> >> +  {95,  46, Level}, /* EIP-150 Irq */
> >> +  {102, 47, Level}, /* USB3 Device Irq */
> >> +  {105, 48, Level}, /* USB3 Host-1 Irq */
> >> +  {106, 49, Level}, /* USB3 Host-0 Irq */
> >> +  {107, 50, Level}, /* SATA Host-1 Irq */
> >> +  {109, 50, Level}, /* SATA Host-0 Irq */
> >> +  {115, 52, Level}, /* NAND Irq */
> >> +  {117, 53, Level}, /* SPI-1 Irq */
> >> +  {118, 54, Level}, /* SPI-0 Irq */
> >> +  {120, 55, Level}, /* I2C 0 Irq */
> >> +  {121, 56, Level}, /* I2C 1 Irq */
> >> +  {122, 57, Level}, /* UART 0 Irq */
> >> +  {123, 58, Level}, /* UART 1 Irq */
> >> +  {124, 59, Level}, /* UART 2 Irq */
> >> +  {125, 60, Level}, /* UART 3 Irq */
> >> +  {127, 61, Level}, /* GOP-3 Irq */
> >> +  {128, 62, Level}, /* GOP-2 Irq */
> >> +  {129, 63, Level}, /* GOP-0 Irq */
> >> +};
> >> +
> >> +/*
> >> + * SEI - System Error Interrupts
> >> + * Note: SPI ID 0-20 are reserved for North-Bridge
> >> + */
> >> +STATIC ICU_IRQ IrqMapSei[] = {
> >> +  {11,  21, Level}, /* SEI error CP-2-CP */
> >> +  {15,  22, Level}, /* PIDI-64 SOC */
> >> +  {16,  23, Level}, /* D2D error Irq */
> >> +  {17,  24, Level}, /* D2D Irq */
> >> +  {18,  25, Level}, /* NAND error */
> >> +  {19,  26, Level}, /* PCIx4 error */
> >> +  {20,  27, Level}, /* PCIx1_0 error */
> >> +  {21,  28, Level}, /* PCIx1_1 error */
> >> +  {25,  29, Level}, /* SDIO reg error */
> >> +  {75,  30, Level}, /* IOB error */
> >> +  {94,  31, Level}, /* EIP150 error */
> >> +  {97,  32, Level}, /* XOR-1 system error */
> >> +  {99,  33, Level}, /* XOR-0 system error */
> >> +  {108, 34, Level}, /* SATA-1 error */
> >> +  {110, 35, Level}, /* SATA-0 error */
> >> +  {114, 36, Level}, /* TDM-MC error */
> >> +  {116, 37, Level}, /* DFX server Irq */
> >> +  {117, 38, Level}, /* Device bus error */
> >> +  {147, 39, Level}, /* Audio error */
> >> +  {171, 40, Level}, /* PIDI Sync error */
> >> +};
> >> +
> >> +/* REI - RAM Error Interrupts */
> >> +STATIC CONST ICU_IRQ IrqMapRei[] = {
> >> +  {12,  0, Level}, /* REI error CP-2-CP */
> >> +  {26,  1, Level}, /* SDIO memory error */
> >> +  {87,  2, Level}, /* EIP-197 ECC error */
> >> +  {93,  3, Edge},  /* EIP-150 RAM error */
> >> +  {96,  4, Level}, /* XOR-1 memory Irq */
> >> +  {98,  5, Level}, /* XOR-0 memory Irq */
> >> +  {100, 6, Edge},  /* USB3 device tx parity */
> >> +  {101, 7, Edge},  /* USB3 device rq parity */
> >> +  {103, 8, Edge},  /* USB3H-1 RAM error */
> >> +  {104, 9, Edge},  /* USB3H-0 RAM error */
> >> +};
> >> +
> >> +STATIC CONST ICU_CONFIG IcuConfigDefault = {
> >
> > The combination of CONST and Default confuses me slightly, as it
> > mildly implies there can be other config objects floating around -
> > whereas this just appears to refer to what the hardware is configured
> > to on every boot.
> > .
> > Would IcuInitialConfig work as well?
> 
> Ok, will rename.
> 
> >
> >> +  .NonSecure =  { IrqMapNonSecure, ARRAY_SIZE (IrqMapNonSecure) },
> >> +  .Sei =        { IrqMapSei, ARRAY_SIZE (IrqMapSei) },
> >> +  .Rei =        { IrqMapRei, ARRAY_SIZE (IrqMapRei) },
> >> +};
> >> +
> >> +STATIC
> >> +VOID
> >> +IcuClearIrq (
> >> +  IN UINTN IcuBase,
> >> +  IN UINTN Nr
> >> +)
> >> +{
> >> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Nr), 0);
> >> +}
> >> +
> >> +STATIC
> >> +VOID
> >> +IcuSetIrq (
> >> +  IN UINTN           IcuBase,
> >> +  IN CONST ICU_IRQ  *Irq,
> >> +  IN UINTN           SpiBase,
> >> +  IN ICU_GROUP       Group
> >> +  )
> >> +{
> >> +  UINT32 IcuInt;
> >> +
> >> +  IcuInt  = (Irq->SpiId + SpiBase) | (1 << ICU_INT_ENABLE_OFFSET);
> >> +  IcuInt |= Irq->IrqType << ICU_IS_EDGE_OFFSET;
> >> +  IcuInt |= Group << ICU_GROUP_OFFSET;
> >> +
> >> +  MmioWrite32 (IcuBase + ICU_INT_CFG (Irq->IcuId), IcuInt);
> >> +}
> >> +
> >> +STATIC
> >> +VOID
> >> +IcuConfigure (
> >> +  IN UINTN             CpIndex,
> >> +  IN MV_SOC_ICU_DESC  *IcuDesc,
> >> +  IN CONST ICU_CONFIG *Config
> >> +  )
> >> +{
> >> +  UINTN IcuBase, Index, SpiOffset, SpiBase;
> >> +  CONST ICU_IRQ *Irq;
> >> +  ICU_MSI *Msi;
> >> +
> >> +  /* Get ICU registers base address */
> >> +  IcuBase = ICU_REG_BASE (CpIndex);
> >> +  /* Get the base of the GIC SPI ID in the MSI message */
> >> +  SpiBase = IcuDesc->IcuSpiBase;
> >> +  /* Get multiple CP110 instances SPI ID shift */
> >> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> >> +  /* Get MSI addresses per interrupt group */
> >> +  Msi = IcuDesc->IcuMsi;
> >> +
> >> +  /* Set the address for SET_SPI and CLR_SPI registers in AP */
> >> +  for (Index = 0; Index < ICU_GROUP_MAX; Index++, Msi++) {
> >> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AL (Msi->Group), Msi->SetSpiAddr & 0xFFFFFFFF);
> >> +    MmioWrite32 (IcuBase + ICU_SET_SPI_AH (Msi->Group), Msi->SetSpiAddr >> 32);
> >> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AL (Msi->Group), Msi->ClrSpiAddr & 0xFFFFFFFF);
> >> +    MmioWrite32 (IcuBase + ICU_CLR_SPI_AH (Msi->Group), Msi->ClrSpiAddr >> 32);
> >
> > Hmm, this common operation (split 64 into 2x32) feels like something
> > EDK2 would have a macro for, but I can't find one. I.e. POINTER_HIGH32
> > and POINTER_LOW32.
> > I see it would be useful in other platforms' PCI code as well.
> >
> > Anyway - the above 4 lines are all too long.
> >
> 
> Lines are now 86 - 79 - 86 - 79 signs long - I didn't break them for
> the sake of readability, but I'll do it then.

Right, sorry - two of them are fine.

I don't complain about line lengths where they don't hide information,
but these ones end up getting cut off in the middle of a mask.

/
    Leif

> > /
> >     Leif
> >
> >> +  }
> >> +
> >> +  /* Mask all ICU interrupts */
> >> +  for (Index = 0; Index < MAX_ICU_IRQS; Index++) {
> >> +    IcuClearIrq (IcuBase, Index);
> >> +  }
> >> +
> >> +  /* Configure the ICU interrupt lines */
> >> +  Irq = Config->NonSecure.Map;
> >> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> >> +    IcuSetIrq (IcuBase, Irq, SpiBase + SpiOffset, ICU_GROUP_NSR);
> >> +  }
> >> +
> >> +  Irq = Config->Sei.Map;
> >> +  for (Index = 0; Index < Config->Sei.Size; Index++, Irq++) {
> >> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_SEI);
> >> +  }
> >> +
> >> +  Irq = Config->Rei.Map;
> >> +  for (Index = 0; Index < Config->Rei.Size; Index++, Irq++) {
> >> +    IcuSetIrq (IcuBase, Irq, SpiBase, ICU_GROUP_REI);
> >> +  }
> >> +}
> >> +
> >> +STATIC
> >> +VOID
> >> +IcuClearGicSpi (
> >> +  IN UINTN             CpIndex,
> >> +  IN MV_SOC_ICU_DESC  *IcuDesc
> >> +  )
> >> +{
> >> +  CONST ICU_CONFIG *Config;
> >> +  UINTN Index, SpiOffset, SpiBase;
> >> +  CONST ICU_IRQ *Irq;
> >> +  ICU_MSI *Msi;
> >> +
> >> +  Config = &IcuConfigDefault;
> >> +
> >> +  /* Get the base of the GIC SPI ID in the MSI message */
> >> +  SpiBase = IcuDesc->IcuSpiBase;
> >> +  /* Get multiple CP110 instances SPI ID shift */
> >> +  SpiOffset = CpIndex * ICU_MAX_IRQS_PER_CP;
> >> +  /* Get MSI addresses per interrupt group */
> >> +  Msi = IcuDesc->IcuMsi;
> >> +
> >> +  /* Clear ICU-generated GIC SPI interrupts */
> >> +  Irq = Config->NonSecure.Map;
> >> +  for (Index = 0; Index < Config->NonSecure.Size; Index++, Irq++) {
> >> +    MmioWrite32 (Msi->ClrSpiAddr, Irq->SpiId + SpiBase + SpiOffset);
> >> +  }
> >> +}
> >> +
> >> +VOID
> >> +EFIAPI
> >> +IcuCleanUp (
> >> +  IN EFI_EVENT  Event,
> >> +  IN VOID      *Context
> >> +  )
> >> +{
> >> +  MV_SOC_ICU_DESC *IcuDesc;
> >> +  UINTN CpCount, CpIndex;
> >> +
> >> +  IcuDesc = Context;
> >> +
> >> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> >> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> >> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> >> +  }
> >> +
> >> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> >> +    IcuClearGicSpi (CpIndex, IcuDesc);
> >> +  }
> >> +}
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +ArmadaIcuInitialize (
> >> +  )
> >> +{
> >> +  MV_SOC_ICU_DESC *IcuDesc;
> >> +  UINTN CpCount, CpIndex;
> >> +  EFI_STATUS Status;
> >> +
> >> +  /*
> >> +   * Due to limited amount of interrupt lanes, only 2 units can be
> >> +   * wired to the GIC.
> >> +   */
> >> +  CpCount = FixedPcdGet8 (PcdMaxCpCount);
> >> +  if (CpCount > ICU_MAX_SUPPORTED_UNITS) {
> >> +    DEBUG ((DEBUG_ERROR,
> >> +      "%a: Default ICU to GIC mapping is available for maximum %d CP110 units",
> >> +      ICU_MAX_SUPPORTED_UNITS,
> >> +      __FUNCTION__));
> >> +    CpCount = ICU_MAX_SUPPORTED_UNITS;
> >> +  }
> >> +
> >> +  /* Obtain SoC description of the ICU */
> >> +  Status = ArmadaSoCDescIcuGet (&IcuDesc);
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  /* Configure default ICU to GIC interrupt mapping for each CP110 */
> >> +  for (CpIndex = 0; CpIndex < CpCount; CpIndex++) {
> >> +    IcuConfigure (CpIndex, IcuDesc, &IcuConfigDefault);
> >> +  }
> >> +
> >> +  /*
> >> +   * In order to be immune to the OS capability of clearing ICU-generated
> >> +   * GIC interrupts, register ExitBootServices event, that will
> >> +   * make sure they remain disabled during OS boot.
> >> +   */
> >> +  Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> >> +                  TPL_NOTIFY,
> >> +                  IcuCleanUp,
> >> +                  IcuDesc,
> >> +                  &EfiExitBootServicesEvent);
> >> +
> >> +  return Status;
> >> +}
> >> --
> >> 2.7.4
> >>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [platforms: PATCH 1/6] Marvell/Armada70x0Db: Set correct CP110 count
[edk2] [platforms: PATCH 2/6] Marvell/Library: Introduce ArmadaIcuLib class
[edk2] [platforms: PATCH 3/6] Marvell/Library: Armada7k8kSoCDescLib: Enable getting CP base address
[edk2] [platforms: PATCH 4/6] Marvell/Library: Armada7k8kSoCDescLib: Introduce ICU information
[edk2] [platforms: PATCH 5/6] Marvell/Library: Implement common ArmadaIcuLib
[edk2] [platforms: PATCH 6/6] Marvell/Armada7k8k: Enable ICU configuration