[platforms: PATCH 0/6] Armada7k8k ICU support

[platforms: PATCH 0/6] Armada7k8k ICU support

[Marcin Wojtas]

Hi,

This patchset introduces support for ICU (Interrupt Consolidation
Unit) of Armada7k8k SoC family. This unit allows to send a
message-based interrupts from the CP110 unit (South Bridge)
to the Application Processor hardware block. After dispatching
the interrupts in the GIC are generated. 

A basic version of the library is introduced, that
allows to configure a static mapping of the interrupt lanes
between CP110 interfaces and GIC. It is required for
the cases, where the OS does not support the ICU
controller on its own (e.g. ACPI boot).

The patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/icu-upstream-r20180712

I'm looking forward to review and any comments/remarks.

Best regards,
Marcin


Marcin Wojtas (6):
  Marvell/Armada70x0Db: Set correct CP110 count
  Marvell/Library: Introduce ArmadaIcuLib class
  Marvell/Library: Armada7k8kSoCDescLib: Enable getting CP base address
  Marvell/Library: Armada7k8kSoCDescLib: Introduce ICU information
  Marvell/Library: Implement common ArmadaIcuLib
  Marvell/Armada7k8k: Enable ICU configuration

 Silicon/Marvell/Marvell.dec                                                    |   1 +
 Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                                  |   1 +
 Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc                                 |   7 +-
 Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf                 |   1 +
 Silicon/Marvell/Library/IcuLib/IcuLib.inf                                      |  38 +++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h |  12 +
 Silicon/Marvell/Include/Library/ArmadaIcuLib.h                                 |  45 +++
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             |  36 +++
 Silicon/Marvell/Library/IcuLib/IcuLib.h                                        |  46 +++
 Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c                   |   2 +
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c |  50 ++++
 Silicon/Marvell/Library/IcuLib/IcuLib.c                                        | 315 ++++++++++++++++++++
 12 files changed, 552 insertions(+), 2 deletions(-)
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.inf
 create mode 100644 Silicon/Marvell/Include/Library/ArmadaIcuLib.h
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.h
 create mode 100644 Silicon/Marvell/Library/IcuLib/IcuLib.c

-- 
2.7.4

[platforms: PATCH 1/6] Marvell/Armada70x0Db: Set correct CP110 count

[Marcin Wojtas]

As a preparation for adding the ICU (Interrupt Consolidation
Unit) library implementation a correct CP110 count is required.
Do it for Armada70x0Db and fix depending XHCI/AHCI PCD's accordingly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
index 5ccee1b..2240a57 100644
--- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
+++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
@@ -53,6 +53,9 @@
 #
 ################################################################################
 [PcdsFixedAtBuild.common]
+  #CP110 count
+  gMarvellTokenSpaceGuid.PcdMaxCpCount|1
+
   #MPP
   gMarvellTokenSpaceGuid.PcdMppChipCount|2
 
@@ -129,8 +132,8 @@
   gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1 }
 
   #PciEmulation
-  gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1, 0x0, 0x0 }
-  gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x0 }
+  gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1 }
+  gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1 }
   gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
 
   #RTC
-- 
2.7.4

[platforms: PATCH 2/6] Marvell/Library: Introduce ArmadaIcuLib class

[Marcin Wojtas]

ICU (Interrupt Consolidation Unit) is a mechanism,
that allows to send a 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 GIC. It is required for the cases, where
the OS does not support the ICU controller on its own
(e.g. ACPI boot).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Marvell.dec                    |  1 +
 Silicon/Marvell/Include/Library/ArmadaIcuLib.h | 45 ++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 Silicon/Marvell/Include/Library/ArmadaIcuLib.h

diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
index 4def897..616624e 100644
--- a/Silicon/Marvell/Marvell.dec
+++ b/Silicon/Marvell/Marvell.dec
@@ -61,6 +61,7 @@
 
 [LibraryClasses]
   ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h
+  ArmadaIcuLib|Include/Library/ArmadaIcuLib.h
   ArmadaSoCDescLib|Include/Library/ArmadaSoCDescLib.h
   SampleAtResetLib|Include/Library/SampleAtResetLib.h
 
diff --git a/Silicon/Marvell/Include/Library/ArmadaIcuLib.h b/Silicon/Marvell/Include/Library/ArmadaIcuLib.h
new file mode 100644
index 0000000..9b68934
--- /dev/null
+++ b/Silicon/Marvell/Include/Library/ArmadaIcuLib.h
@@ -0,0 +1,45 @@
+/**
+*
+*  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.
+*
+**/
+#ifndef __ARMADA_ICU_LIB_H__
+#define __ARMADA_ICU_LIB_H__
+
+typedef enum {
+  Level = 0,
+  Edge = 1
+} ICU_IRQ_TYPE;
+
+typedef struct {
+  UINTN IcuId;
+  UINTN SpiId;
+  ICU_IRQ_TYPE IrqType;
+} ICU_IRQ;
+
+typedef struct {
+  const ICU_IRQ  *Map;
+  UINTN           Size;
+} ICU_CONFIG_ENTRY;
+
+typedef struct {
+  ICU_CONFIG_ENTRY NonSecure;
+  ICU_CONFIG_ENTRY Sei;
+  ICU_CONFIG_ENTRY Rei;
+} ICU_CONFIG;
+
+EFI_STATUS
+EFIAPI
+ArmadaIcuInitialize (
+  VOID
+  );
+
+#endif /* __ARMADA_ICU_LIB_H__ */
-- 
2.7.4

[platforms: PATCH 3/6] Marvell/Library: Armada7k8kSoCDescLib: Enable getting CP base address

[Marcin Wojtas]

For upcoming patches there is a need to get the CP110 base address,
introduce according getter function for it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             |  6 ++++++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
index d2bcf2a..56efdbe 100644
--- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
+++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
@@ -36,6 +36,12 @@ ArmadaSoCDescComPhyGet (
   IN OUT UINTN                *DescCount
   );
 
+UINTN
+EFIAPI
+ArmadaSoCDescCpBaseGet (
+  IN UINTN  CpIndex
+  );
+
 //
 // I2C
 //
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
index 6ce6bad..c7c9c13 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
@@ -61,6 +61,17 @@ ArmadaSoCDescComPhyGet (
   return EFI_SUCCESS;
 }
 
+UINTN
+EFIAPI
+ArmadaSoCDescCpBaseGet (
+  IN UINTN  CpIndex
+  )
+{
+  ASSERT (CpIndex < FixedPcdGet8 (PcdMaxCpCount));
+
+  return MV_SOC_CP_BASE (CpIndex);
+}
+
 EFI_STATUS
 EFIAPI
 ArmadaSoCDescI2cGet (
-- 
2.7.4

[platforms: PATCH 4/6] Marvell/Library: Armada7k8kSoCDescLib: Introduce ICU information

[Marcin Wojtas]

This patch introduces new library callback (ArmadaSoCDescIcuGet ()),
which dynamically allocates and fills MV_SOC_ICU_DESC structure with
the SoC description of ICU (Interrupt Consolidation Unit).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h | 12 ++++++
 Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 30 +++++++++++++++
 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 39 ++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
index 3072883..c14b985 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
@@ -44,6 +44,18 @@
 #define MV_SOC_I2C_BASE(I2c)             (0x701000 + ((I2c) * 0x100))
 
 //
+// Platform description of ICU (Interrupt Consolidation Unit) controllers
+//
+#define ICU_GIC_MAPPING_OFFSET           0
+#define ICU_NSR_SET_SPI_BASE             0xf03f0040
+#define ICU_NSR_CLEAR_SPI_BASE           0xf03f0048
+#define ICU_SEI_SET_SPI_BASE             0xf03f0230
+#define ICU_SEI_CLEAR_SPI_BASE           0xf03f0230
+#define ICU_REI_SET_SPI_BASE             0xf03f0270
+#define ICU_REI_CLEAR_SPI_BASE           0xf03f0270
+#define ICU_GROUP_UNSUPPORTED            0x0
+
+//
 // Platform description of MDIO controllers
 //
 #define MV_SOC_MDIO_BASE(Cp)             (MV_SOC_CP_BASE (Cp) + 0x12A200)
diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
index 56efdbe..4d2a85f 100644
--- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
+++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
@@ -58,6 +58,36 @@ ArmadaSoCDescI2cGet (
   );
 
 //
+// ICU (Interrupt Consolidation Unit)
+//
+typedef enum {
+  ICU_GROUP_NSR  = 0,
+  ICU_GROUP_SR   = 1,
+  ICU_GROUP_LPI  = 2,
+  ICU_GROUP_VLPI = 3,
+  ICU_GROUP_SEI  = 4,
+  ICU_GROUP_REI  = 5,
+  ICU_GROUP_MAX,
+} ICU_GROUP;
+
+typedef struct {
+  ICU_GROUP Group;
+  UINTN     SetSpiAddr;
+  UINTN     ClrSpiAddr;
+} ICU_MSI;
+
+typedef struct {
+  UINTN    IcuSpiBase;
+  ICU_MSI  IcuMsi[ICU_GROUP_MAX];
+} MV_SOC_ICU_DESC;
+
+EFI_STATUS
+EFIAPI
+ArmadaSoCDescIcuGet (
+  IN OUT MV_SOC_ICU_DESC  **IcuDesc
+  );
+
+//
 // MDIO
 //
 typedef struct {
diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
index c7c9c13..8383206 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
@@ -103,6 +103,45 @@ ArmadaSoCDescI2cGet (
   return EFI_SUCCESS;
 }
 
+//
+// Allocate the MSI address per interrupt Group,
+// unsupported Groups get NULL address.
+//
+STATIC
+MV_SOC_ICU_DESC mA7k8kIcuDescTemplate = {
+  ICU_GIC_MAPPING_OFFSET,
+  {
+    /* Non secure interrupts */
+    {ICU_GROUP_NSR,  ICU_NSR_SET_SPI_BASE,  ICU_NSR_CLEAR_SPI_BASE},
+    /* Secure interrupts */
+    {ICU_GROUP_SR,   ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
+    /* LPI interrupts */
+    {ICU_GROUP_LPI,  ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
+    /* Virtual LPI interrupts */
+    {ICU_GROUP_VLPI, ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
+    /* System error interrupts */
+    {ICU_GROUP_SEI,  ICU_SEI_SET_SPI_BASE,  ICU_SEI_CLEAR_SPI_BASE},
+    /* RAM error interrupts */
+    {ICU_GROUP_REI,  ICU_REI_SET_SPI_BASE,  ICU_REI_CLEAR_SPI_BASE},
+  }
+};
+
+EFI_STATUS
+EFIAPI
+ArmadaSoCDescIcuGet (
+  IN OUT MV_SOC_ICU_DESC  **IcuDesc
+  )
+{
+  *IcuDesc = AllocateCopyPool (sizeof (mA7k8kIcuDescTemplate),
+               &mA7k8kIcuDescTemplate);
+  if (*IcuDesc == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  return EFI_SUCCESS;
+}
+
 EFI_STATUS
 EFIAPI
 ArmadaSoCDescMdioGet (
-- 
2.7.4

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

[Marcin Wojtas]

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

[platforms: PATCH 6/6] Marvell/Armada7k8k: Enable ICU configuration

[Marcin Wojtas]

This patch enables the ICU (Interrupt Consolidation Unit)
configuration in the common platform initialization driver.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                  | 1 +
 Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf | 1 +
 Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c   | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
index a9d67a2..27b14ed 100644
--- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
+++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
@@ -32,6 +32,7 @@
 #SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #
 [LibraryClasses.common]
+  ArmadaIcuLib|Silicon/Marvell/Library/IcuLib/IcuLib.inf
   ArmadaSoCDescLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf
   ArmPlatformLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
   ComPhyLib|Silicon/Marvell/Library/ComPhyLib/ComPhyLib.inf
diff --git a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
index 803dc6e..5503463 100644
--- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
+++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
@@ -30,6 +30,7 @@
   Silicon/Marvell/Marvell.dec
 
 [LibraryClasses]
+  ArmadaIcuLib
   ComPhyLib
   DebugLib
   MppLib
diff --git a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
index 1efad77..18b6783 100644
--- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
+++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
@@ -12,6 +12,7 @@
 
 **/
 
+#include <Library/ArmadaIcuLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MppLib.h>
 #include <Library/MvComPhyLib.h>
@@ -40,6 +41,7 @@ ArmadaPlatInitDxeEntryPoint (
   MvComPhyInit ();
   UtmiPhyInit ();
   MppInitialize ();
+  ArmadaIcuInitialize ();
 
   return EFI_SUCCESS;
 }
-- 
2.7.4

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

[Ard Biesheuvel]

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
>

Re: [platforms: PATCH 1/6] Marvell/Armada70x0Db: Set correct CP110 count

[Ard Biesheuvel]

On 12 July 2018 at 09:39, Marcin Wojtas <mw@semihalf.com> wrote:
> As a preparation for adding the ICU (Interrupt Consolidation
> Unit) library implementation a correct CP110 count is required.
> Do it for Armada70x0Db and fix depending XHCI/AHCI PCD's accordingly.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

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

> ---
>  Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> index 5ccee1b..2240a57 100644
> --- a/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> +++ b/Platform/Marvell/Armada70x0Db/Armada70x0Db.dsc
> @@ -53,6 +53,9 @@
>  #
>  ################################################################################
>  [PcdsFixedAtBuild.common]
> +  #CP110 count
> +  gMarvellTokenSpaceGuid.PcdMaxCpCount|1
> +
>    #MPP
>    gMarvellTokenSpaceGuid.PcdMppChipCount|2
>
> @@ -129,8 +132,8 @@
>    gMarvellTokenSpaceGuid.PcdPp2Controllers|{ 0x1 }
>
>    #PciEmulation
> -  gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1, 0x0, 0x0 }
> -  gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1, 0x0 }
> +  gMarvellTokenSpaceGuid.PcdPciEXhci|{ 0x1, 0x1 }
> +  gMarvellTokenSpaceGuid.PcdPciEAhci|{ 0x1 }
>    gMarvellTokenSpaceGuid.PcdPciESdhci|{ 0x1, 0x1 }
>
>    #RTC
> --
> 2.7.4
>

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

[Marcin Wojtas]

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
> >

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

[Leif Lindholm]

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
> 

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

[Marcin Wojtas]

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
>>

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

[Leif Lindholm]

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
> >>

Re: [platforms: PATCH 2/6] Marvell/Library: Introduce ArmadaIcuLib class

[Ard Biesheuvel]

On 12 July 2018 at 09:39, Marcin Wojtas <mw@semihalf.com> wrote:
> ICU (Interrupt Consolidation Unit) is a mechanism,
> that allows to send a 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 GIC. It is required for the cases, where
> the OS does not support the ICU controller on its own
> (e.g. ACPI boot).
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Marvell.dec                    |  1 +
>  Silicon/Marvell/Include/Library/ArmadaIcuLib.h | 45 ++++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 Silicon/Marvell/Include/Library/ArmadaIcuLib.h
>
> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> index 4def897..616624e 100644
> --- a/Silicon/Marvell/Marvell.dec
> +++ b/Silicon/Marvell/Marvell.dec
> @@ -61,6 +61,7 @@
>
>  [LibraryClasses]
>    ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h
> +  ArmadaIcuLib|Include/Library/ArmadaIcuLib.h
>    ArmadaSoCDescLib|Include/Library/ArmadaSoCDescLib.h
>    SampleAtResetLib|Include/Library/SampleAtResetLib.h
>
> diff --git a/Silicon/Marvell/Include/Library/ArmadaIcuLib.h b/Silicon/Marvell/Include/Library/ArmadaIcuLib.h
> new file mode 100644
> index 0000000..9b68934
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Library/ArmadaIcuLib.h
> @@ -0,0 +1,45 @@
> +/**
> +*
> +*  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.
> +*
> +**/
> +#ifndef __ARMADA_ICU_LIB_H__
> +#define __ARMADA_ICU_LIB_H__
> +
> +typedef enum {
> +  Level = 0,
> +  Edge = 1

Could we have more discriminating identifiers here please?
IcuIrqTypeLevel and IcuIrqTypeEdge perhaps?

> +} ICU_IRQ_TYPE;
> +
> +typedef struct {
> +  UINTN IcuId;
> +  UINTN SpiId;
> +  ICU_IRQ_TYPE IrqType;
> +} ICU_IRQ;
> +
> +typedef struct {
> +  const ICU_IRQ  *Map;
> +  UINTN           Size;
> +} ICU_CONFIG_ENTRY;
> +
> +typedef struct {
> +  ICU_CONFIG_ENTRY NonSecure;
> +  ICU_CONFIG_ENTRY Sei;
> +  ICU_CONFIG_ENTRY Rei;
> +} ICU_CONFIG;
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaIcuInitialize (
> +  VOID
> +  );
> +
> +#endif /* __ARMADA_ICU_LIB_H__ */
> --
> 2.7.4
>

Re: [platforms: PATCH 3/6] Marvell/Library: Armada7k8kSoCDescLib: Enable getting CP base address

[Ard Biesheuvel]

On 12 July 2018 at 09:39, Marcin Wojtas <mw@semihalf.com> wrote:
> For upcoming patches there is a need to get the CP110 base address,
> introduce according getter function for it.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             |  6 ++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 11 +++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> index d2bcf2a..56efdbe 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> @@ -36,6 +36,12 @@ ArmadaSoCDescComPhyGet (
>    IN OUT UINTN                *DescCount
>    );
>
> +UINTN

If this is a memory address, you should use EFI_PHYSICAL_ADDRESS here.

> +EFIAPI
> +ArmadaSoCDescCpBaseGet (
> +  IN UINTN  CpIndex
> +  );
> +
>  //
>  // I2C
>  //
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> index 6ce6bad..c7c9c13 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> @@ -61,6 +61,17 @@ ArmadaSoCDescComPhyGet (
>    return EFI_SUCCESS;
>  }
>
> +UINTN
> +EFIAPI
> +ArmadaSoCDescCpBaseGet (
> +  IN UINTN  CpIndex
> +  )
> +{
> +  ASSERT (CpIndex < FixedPcdGet8 (PcdMaxCpCount));
> +
> +  return MV_SOC_CP_BASE (CpIndex);
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  ArmadaSoCDescI2cGet (
> --
> 2.7.4
>

Re: [platforms: PATCH 4/6] Marvell/Library: Armada7k8kSoCDescLib: Introduce ICU information

[Ard Biesheuvel]

On 12 July 2018 at 09:39, Marcin Wojtas <mw@semihalf.com> wrote:
> This patch introduces new library callback (ArmadaSoCDescIcuGet ()),
> which dynamically allocates and fills MV_SOC_ICU_DESC structure with
> the SoC description of ICU (Interrupt Consolidation Unit).
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h | 12 ++++++
>  Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h                             | 30 +++++++++++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c | 39 ++++++++++++++++++++
>  3 files changed, 81 insertions(+)
>
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> index 3072883..c14b985 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.h
> @@ -44,6 +44,18 @@
>  #define MV_SOC_I2C_BASE(I2c)             (0x701000 + ((I2c) * 0x100))
>
>  //
> +// Platform description of ICU (Interrupt Consolidation Unit) controllers
> +//
> +#define ICU_GIC_MAPPING_OFFSET           0
> +#define ICU_NSR_SET_SPI_BASE             0xf03f0040
> +#define ICU_NSR_CLEAR_SPI_BASE           0xf03f0048
> +#define ICU_SEI_SET_SPI_BASE             0xf03f0230
> +#define ICU_SEI_CLEAR_SPI_BASE           0xf03f0230
> +#define ICU_REI_SET_SPI_BASE             0xf03f0270
> +#define ICU_REI_CLEAR_SPI_BASE           0xf03f0270
> +#define ICU_GROUP_UNSUPPORTED            0x0
> +
> +//
>  // Platform description of MDIO controllers
>  //
>  #define MV_SOC_MDIO_BASE(Cp)             (MV_SOC_CP_BASE (Cp) + 0x12A200)
> diff --git a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> index 56efdbe..4d2a85f 100644
> --- a/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> +++ b/Silicon/Marvell/Include/Library/ArmadaSoCDescLib.h
> @@ -58,6 +58,36 @@ ArmadaSoCDescI2cGet (
>    );
>
>  //
> +// ICU (Interrupt Consolidation Unit)
> +//
> +typedef enum {
> +  ICU_GROUP_NSR  = 0,
> +  ICU_GROUP_SR   = 1,
> +  ICU_GROUP_LPI  = 2,
> +  ICU_GROUP_VLPI = 3,
> +  ICU_GROUP_SEI  = 4,
> +  ICU_GROUP_REI  = 5,

Are these identifiers defined externally anywhere? If not, it is
better to use IcuGroupXxx, which is more idiomatic in UEFI/EDK2.

> +  ICU_GROUP_MAX,
> +} ICU_GROUP;
> +
> +typedef struct {
> +  ICU_GROUP Group;
> +  UINTN     SetSpiAddr;
> +  UINTN     ClrSpiAddr;
> +} ICU_MSI;
> +
> +typedef struct {
> +  UINTN    IcuSpiBase;
> +  ICU_MSI  IcuMsi[ICU_GROUP_MAX];
> +} MV_SOC_ICU_DESC;
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCDescIcuGet (
> +  IN OUT MV_SOC_ICU_DESC  **IcuDesc
> +  );
> +
> +//
>  // MDIO
>  //
>  typedef struct {
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> index c7c9c13..8383206 100644
> --- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.c
> @@ -103,6 +103,45 @@ ArmadaSoCDescI2cGet (
>    return EFI_SUCCESS;
>  }
>
> +//
> +// Allocate the MSI address per interrupt Group,
> +// unsupported Groups get NULL address.
> +//
> +STATIC
> +MV_SOC_ICU_DESC mA7k8kIcuDescTemplate = {
> +  ICU_GIC_MAPPING_OFFSET,
> +  {
> +    /* Non secure interrupts */
> +    {ICU_GROUP_NSR,  ICU_NSR_SET_SPI_BASE,  ICU_NSR_CLEAR_SPI_BASE},

Please put a space after { and before }

> +    /* Secure interrupts */
> +    {ICU_GROUP_SR,   ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
> +    /* LPI interrupts */
> +    {ICU_GROUP_LPI,  ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
> +    /* Virtual LPI interrupts */
> +    {ICU_GROUP_VLPI, ICU_GROUP_UNSUPPORTED, ICU_GROUP_UNSUPPORTED},
> +    /* System error interrupts */
> +    {ICU_GROUP_SEI,  ICU_SEI_SET_SPI_BASE,  ICU_SEI_CLEAR_SPI_BASE},
> +    /* RAM error interrupts */
> +    {ICU_GROUP_REI,  ICU_REI_SET_SPI_BASE,  ICU_REI_CLEAR_SPI_BASE},
> +  }
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaSoCDescIcuGet (
> +  IN OUT MV_SOC_ICU_DESC  **IcuDesc
> +  )
> +{
> +  *IcuDesc = AllocateCopyPool (sizeof (mA7k8kIcuDescTemplate),
> +               &mA7k8kIcuDescTemplate);
> +  if (*IcuDesc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  EFI_STATUS
>  EFIAPI
>  ArmadaSoCDescMdioGet (
> --
> 2.7.4
>

Re: [platforms: PATCH 6/6] Marvell/Armada7k8k: Enable ICU configuration

[Ard Biesheuvel]

On 12 July 2018 at 09:40, Marcin Wojtas <mw@semihalf.com> wrote:
> This patch enables the ICU (Interrupt Consolidation Unit)
> configuration in the common platform initialization driver.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

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

> ---
>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                  | 1 +
>  Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf | 1 +
>  Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c   | 2 ++
>  3 files changed, 4 insertions(+)
>
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index a9d67a2..27b14ed 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -32,6 +32,7 @@
>  #SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #
>  [LibraryClasses.common]
> +  ArmadaIcuLib|Silicon/Marvell/Library/IcuLib/IcuLib.inf
>    ArmadaSoCDescLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kSoCDescLib/Armada7k8kSoCDescLib.inf
>    ArmPlatformLib|Silicon/Marvell/Armada7k8k/Library/Armada7k8kLib/Armada7k8kLib.inf
>    ComPhyLib|Silicon/Marvell/Library/ComPhyLib/ComPhyLib.inf
> diff --git a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> index 803dc6e..5503463 100644
> --- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> +++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> @@ -30,6 +30,7 @@
>    Silicon/Marvell/Marvell.dec
>
>  [LibraryClasses]
> +  ArmadaIcuLib
>    ComPhyLib
>    DebugLib
>    MppLib
> diff --git a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> index 1efad77..18b6783 100644
> --- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> +++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> @@ -12,6 +12,7 @@
>
>  **/
>
> +#include <Library/ArmadaIcuLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MppLib.h>
>  #include <Library/MvComPhyLib.h>
> @@ -40,6 +41,7 @@ ArmadaPlatInitDxeEntryPoint (
>    MvComPhyInit ();
>    UtmiPhyInit ();
>    MppInitialize ();
> +  ArmadaIcuInitialize ();
>
>    return EFI_SUCCESS;
>  }
> --
> 2.7.4
>