Re: [edk2-platforms: PATCH 07/14] Marvell/Armada7k8k: Implement PciHostBridgeLib

["Marcin Wojtas" <mw@semihalf.com>]

Hi Leif,

pt., 10 maj 2019 o 17:50 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Thu, May 09, 2019 at 11:53:35AM +0200, Marcin Wojtas wrote:
> > Add an implementation of the PciHostBridgeLib glue library that
> > describes the PCIe RC on this SoC so that the generic PCI host bridge
> > driver can attach to it.
> >
> > This includes a constructor which performs the SoC specific init and
> > training sequences.
> >
> > This patch is based on work of Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > and Jing Hua <jinghua@marvell.com>/
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.inf          |  52 +++
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h |  95 ++++++
>
> Since you so helpfully gave me a link to your branch, I can tell that
> Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h
> has incorrect line endings. If you could address for v2, that would be
> most appreciated.
>
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.c            | 244 +++++++++++++++
> >  Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c | 330 ++++++++++++++++++++
> >  4 files changed, 721 insertions(+)
> >  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.inf
> >  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h
> >  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.c
> >  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
> >
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.inf
> > new file mode 100644
> > index 0000000..e46f71d
> > --- /dev/null
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.inf
> > @@ -0,0 +1,52 @@
> > +## @file
> > +#  PCI Host Bridge Library instance for Marvell Armada 7k/8k SOC
> > +#
> > +#  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> > +#  Copyright (c) 2019 Marvell International Ltd. All rights reserved.<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                    = 0x0001001B
> > +  BASE_NAME                      = Armada7k8kPciHostBridgeLib
> > +  FILE_GUID                      = 7f989c9d-02a0-4348-8aeb-ab2e1566fb18
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = PciHostBridgeLib|DXE_DRIVER
> > +  CONSTRUCTOR                    = Armada7k8kPciHostBridgeLibConstructor
> > +
> > +[Sources]
> > +  PciHostBridgeLib.c
> > +  PciHostBridgeLibConstructor.c
> > +
> > +[Packages]
> > +  ArmPkg/ArmPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > +  ArmLib
> > +  ArmadaSoCDescLib
> > +  DebugLib
> > +  DevicePathLib
> > +  MemoryAllocationLib
> > +  MvGpioLib
> > +  UefiBootServicesTableLib
> > +
> > +[Protocols]
> > +  gEmbeddedGpioProtocolGuid
> > +  gMarvellBoardDescProtocolGuid
> > +
> > +[Depex]
> > +  gMarvellPlatformInitCompleteProtocolGuid
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h
> > new file mode 100644
> > index 0000000..ff9d919
> > --- /dev/null
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.h
> > @@ -0,0 +1,95 @@
> > +/** @file
> > +  PCI Host Bridge Library instance for Marvell 70x0/80x0
> > +
> > +  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> > +  Copyright (c) 2019 Marvell International Ltd. All rights reserved.<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.
> > +
> > +**/
> > +#ifndef __PCI_HOST_BRIDGE_LIB_CONSTRUCTOR_H__
> > +#define __PCI_HOST_BRIDGE_LIB_CONSTRUCTOR_H__
> > +
> > +#define IATU_VIEWPORT_OFF                                   0x900
> > +#define IATU_VIEWPORT_INBOUND                               BIT31
> > +#define IATU_VIEWPORT_OUTBOUND                              0
> > +#define IATU_VIEWPORT_REGION_INDEX(Idx)                     ((Idx) & 7)
> > +
> > +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0                   0x904
> > +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM          0x0
> > +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_IO           0x2
> > +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG0         0x4
> > +#define IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG1         0x5
> > +
> > +#define IATU_REGION_CTRL_2_OFF_OUTBOUND_0                   0x908
> > +#define IATU_REGION_CTRL_2_OFF_OUTBOUND_0_REGION_EN         BIT31
> > +#define IATU_REGION_CTRL_2_OFF_OUTBOUND_0_CFG_SHIFT_MODE    BIT28
> > +
> > +#define IATU_LWR_BASE_ADDR_OFF_OUTBOUND_0                   0x90C
> > +#define IATU_UPPER_BASE_ADDR_OFF_OUTBOUND_0                 0x910
> > +#define IATU_LIMIT_ADDR_OFF_OUTBOUND_0                      0x914
> > +#define IATU_LWR_TARGET_ADDR_OFF_OUTBOUND_0                 0x918
> > +#define IATU_UPPER_TARGET_ADDR_OFF_OUTBOUND_0               0x91C
> > +
> > +#define PORT_LINK_CTRL_OFF                                  0x710
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_x1                  (0x01 << 16)
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_x2                  (0x03 << 16)
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_x4                  (0x07 << 16)
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_x8                  (0x0f << 16)
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_x16                 (0x1f << 16)
> > +#define PORT_LINK_CTRL_OFF_LINK_CAPABLE_MASK                (0x3f << 16)
> > +
> > +#define GEN2_CTRL_OFF                                       0x80c
> > +#define GEN2_CTRL_OFF_NUM_OF_LANES(n)                       (((n) & 0x1f) << 8)
> > +#define GEN2_CTRL_OFF_NUM_OF_LANES_MASK                     (0x1f << 8)
> > +#define GEN2_CTRL_OFF_DIRECT_SPEED_CHANGE                   BIT17
> > +
> > +#define PCIE_GLOBAL_CTRL_OFFSET                             0x8000
> > +#define PCIE_GLOBAL_APP_LTSSM_EN                            BIT2
> > +#define PCIE_GLOBAL_CTRL_DEVICE_TYPE_RC                     (0x4 << 4)
> > +#define PCIE_GLOBAL_CTRL_DEVICE_TYPE_MASK                   (0xF << 4)
> > +
> > +#define PCIE_GLOBAL_STATUS_REG                              0x8008
> > +#define PCIE_GLOBAL_STATUS_RDLH_LINK_UP                     BIT1
> > +#define PCIE_GLOBAL_STATUS_PHY_LINK_UP                      BIT9
> > +
> > +#define PCIE_PM_STATUS                                      0x8014
> > +#define PCIE_PM_LTSSM_STAT_MASK                             (0x3f << 3)
> > +
> > +#define PCIE_GLOBAL_INT_MASK1_REG                           0x8020
> > +#define PCIE_INT_A_ASSERT_MASK                              BIT9
> > +#define PCIE_INT_B_ASSERT_MASK                              BIT10
> > +#define PCIE_INT_C_ASSERT_MASK                              BIT11
> > +#define PCIE_INT_D_ASSERT_MASK                              BIT12
> > +
> > +#define PCIE_ARCACHE_TRC_REG                                0x8050
> > +#define PCIE_AWCACHE_TRC_REG                                0x8054
> > +#define PCIE_ARUSER_REG                                     0x805C
> > +#define PCIE_AWUSER_REG                                     0x8060
> > +
> > +#define ARCACHE_DEFAULT_VALUE                               0x3511
> > +#define AWCACHE_DEFAULT_VALUE                               0x5311
> > +
> > +#define AX_USER_DOMAIN_INNER_SHAREABLE                      (0x1 << 4)
> > +#define AX_USER_DOMAIN_OUTER_SHAREABLE                      (0x2 << 4)
> > +#define AX_USER_DOMAIN_MASK                                 (0x3 << 4)
> > +
> > +#define PCIE_LINK_CAPABILITY                                0x7C
> > +#define PCIE_LINK_CTL_2                                     0xA0
> > +#define TARGET_LINK_SPEED_MASK                              0xF
> > +#define LINK_SPEED_GEN_1                                    0x1
> > +#define LINK_SPEED_GEN_2                                    0x2
> > +#define LINK_SPEED_GEN_3                                    0x3
> > +
> > +#define PCIE_GEN3_EQU_CTRL                                  0x8A8
> > +#define GEN3_EQU_EVAL_2MS_DISABLE                           BIT5
> > +
> > +#define PCIE_LINK_UP_TIMEOUT_US                             40000
> > +
> > +#endif
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.c
> > new file mode 100644
> > index 0000000..ff6288c
> > --- /dev/null
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLib.c
> > @@ -0,0 +1,244 @@
> > +/** @file
> > +  PCI Host Bridge Library instance for Marvell Armada 70x0/80x0
> > +
> > +  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> > +  Copyright (c) 2019 Marvell International Ltd. All rights reserved.<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.
> > +
> > +**/
> > +#include <PiDxe.h>
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/DevicePathLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/PciHostBridgeLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +
> > +#include <Protocol/BoardDesc.h>
> > +#include <Protocol/PciHostBridgeResourceAllocation.h>
> > +#include <Protocol/PciRootBridgeIo.h>
> > +
> > +#pragma pack(1)
> > +typedef struct {
> > +  ACPI_HID_DEVICE_PATH     AcpiDevicePath;
> > +  EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
> > +} EFI_PCI_ROOT_BRIDGE_DEVICE_PATH;
> > +#pragma pack ()
> > +
> > +STATIC EFI_PCI_ROOT_BRIDGE_DEVICE_PATH mEfiPciRootBridgeDevicePath = {
> > +  {
> > +    {
> > +      ACPI_DEVICE_PATH,
> > +      ACPI_DP,
> > +      {
> > +        (UINT8) (sizeof (ACPI_HID_DEVICE_PATH)),
> > +        (UINT8) ((sizeof (ACPI_HID_DEVICE_PATH)) >> 8)
>
> Drop space after (UINT8), x2.
>
> > +      }
> > +    },
> > +    EISA_PNP_ID(0x0A08), // PCI Express
>
> Space before (.
>
> > +    0
> > +  },
> > +
> > +  {
> > +    END_DEVICE_PATH_TYPE,
> > +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +    {
> > +      END_DEVICE_PATH_LENGTH,
> > +      0
> > +    }
> > +  }
> > +};
> > +
> > +GLOBAL_REMOVE_IF_UNREFERENCED
> > +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> > +  L"Mem", L"I/O", L"Bus"
> > +};
> > +
> > +/**
> > +  Return all the root bridge instances in an array.
> > +
> > +  @param Count  Return the count of root bridge instances.
> > +
> > +  @return All the root bridge instances in an array.
> > +          The array should be passed into PciHostBridgeFreeRootBridges()
> > +          when it's not used.
> > +
> > +**/
> > +PCI_ROOT_BRIDGE *
> > +EFIAPI
> > +PciHostBridgeGetRootBridges (
> > +  UINTN *Count
> > +  )
> > +{
> > +  MARVELL_BOARD_DESC_PROTOCOL *BoardDescriptionProtocol;
> > +  MV_BOARD_PCIE_DESCRIPTION   *BoardPcieDescription;
> > +  MV_PCIE_CONTROLLER          *PcieController;
> > +  PCI_ROOT_BRIDGE             *PciRootBridges;
> > +  PCI_ROOT_BRIDGE             *RootBridge;
> > +  EFI_STATUS                   Status;
> > +  UINTN                        Index;
> > +
> > +  *Count = 0;
> > +
> > +  /* Obtain list of available controllers */
> > +  Status = gBS->LocateProtocol (&gMarvellBoardDescProtocolGuid,
> > +                  NULL,
> > +                  (VOID **)&BoardDescriptionProtocol);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot locate BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    return NULL;
> > +  }
> > +
> > +  Status = BoardDescriptionProtocol->PcieDescriptionGet (
> > +                                       BoardDescriptionProtocol,
> > +                                       &BoardPcieDescription);
> > +  if (Status == EFI_NOT_FOUND) {
> > +    /* No controllers used on the platform, exit silently */
> > +    return NULL;
> > +  } else if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot get Pcie board desc from BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    return NULL;
> > +  }
> > +
> > +  /* Assign return values */
> > +  PciRootBridges = AllocateZeroPool (BoardPcieDescription->PcieControllerCount *
> > +                                     sizeof (PCI_ROOT_BRIDGE));
> > +  if (PciRootBridges == NULL) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Fail to allocate resources\n", __FUNCTION__));
> > +    return NULL;
> > +  }
> > +
> > +  *Count = BoardPcieDescription->PcieControllerCount;
> > +  RootBridge = PciRootBridges;
> > +
> > +  /* Fill information of all root bridge instances */
> > +  for (Index = 0; Index < *Count; Index++, RootBridge++) {
> > +
> > +    PcieController = &(BoardPcieDescription->PcieControllers[Index]);
> > +
> > +    RootBridge->Segment   = 0;
> > +    RootBridge->Supports  = 0;
> > +    RootBridge->Attributes  = RootBridge->Supports;
> > +
> > +    RootBridge->DmaAbove4G  = FALSE;
> > +
> > +    RootBridge->AllocationAttributes  = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM |
> > +                                        EFI_PCI_HOST_BRIDGE_MEM64_DECODE;
> > +
> > +    RootBridge->Bus.Base = PcieController->PcieBusMin;
> > +    RootBridge->Bus.Limit = PcieController->PcieBusMax;
> > +    RootBridge->Io.Base = PcieController->PcieIoWinBase;
> > +    RootBridge->Io.Limit = PcieController->PcieIoWinBase +
> > +                           PcieController->PcieIoWinSize - 1;
> > +    RootBridge->Mem.Base = PcieController->PcieMmio32WinBase;
> > +    RootBridge->Mem.Limit = PcieController->PcieMmio32WinBase +
> > +                            PcieController->PcieMmio32WinSize - 1;
> > +    RootBridge->MemAbove4G.Base = PcieController->PcieMmio64WinBase;
> > +    RootBridge->MemAbove4G.Limit = PcieController->PcieMmio64WinBase +
> > +                                   PcieController->PcieMmio64WinSize - 1;
> > +
> > +    /* No separate ranges for prefetchable and non-prefetchable BARs */
> > +    RootBridge->PMem.Base           = MAX_UINT64;
> > +    RootBridge->PMem.Limit          = 0;
> > +    RootBridge->PMemAbove4G.Base    = MAX_UINT64;
> > +    RootBridge->PMemAbove4G.Limit   = 0;
> > +
> > +    ASSERT (PcieController->PcieMmio64Translation == 0);
> > +    ASSERT (PcieController->PcieMmio32Translation == 0);
> > +
> > +    RootBridge->NoExtendedConfigSpace = FALSE;
> > +
> > +    RootBridge->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath;
> > +  }
> > +
> > +  return PciRootBridges;
> > +}
> > +
> > +/**
> > +  Free the root bridge instances array returned from PciHostBridgeGetRootBridges().
> > +
> > +  @param Bridges The root bridge instances array.
> > +  @param Count   The count of the array.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +PciHostBridgeFreeRootBridges (
> > +  PCI_ROOT_BRIDGE *Bridges,
> > +  UINTN           Count
> > +  )
> > +{
> > +  FreePool (Bridges);
> > +}
> > +
> > +/**
> > +  Inform the platform that the resource conflict happens.
> > +
> > +  @param HostBridgeHandle Handle of the Host Bridge.
> > +  @param Configuration    Pointer to PCI I/O and PCI memory resource
> > +                          descriptors. The Configuration contains the resources
> > +                          for all the root bridges. The resource for each root
> > +                          bridge is terminated with END descriptor and an
> > +                          additional END is appended indicating the end of the
> > +                          entire resources. The resource descriptor field
> > +                          values follow the description in
> > +                          EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL
> > +                          .SubmitResources().
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +PciHostBridgeResourceConflict (
> > +  EFI_HANDLE                        HostBridgeHandle,
> > +  VOID                              *Configuration
> > +  )
> > +{
> > +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> > +  UINTN                              RootBridgeIndex;
> > +
> > +  DEBUG ((DEBUG_ERROR, "PciHostBridge: Resource conflict happens!\n"));
> > +
> > +  RootBridgeIndex = 0;
> > +  Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration;
>
> Drop space after "*)".
>
> > +
> > +  while (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
> > +
> > +    DEBUG ((DEBUG_ERROR, "RootBridge[%d]:\n", RootBridgeIndex++));
> > +
> > +    for (; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> > +      ASSERT (Descriptor->ResType <
> > +              (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) /
> > +               sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0])));
> > +
> > +      DEBUG ((DEBUG_ERROR,
> > +        " %s: Length/Alignment = 0x%lx / 0x%lx\n",
> > +        mPciHostBridgeLibAcpiAddressSpaceTypeStr[Descriptor->ResType],
> > +        Descriptor->AddrLen, Descriptor->AddrRangeMax));
> > +
> > +      if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> > +        DEBUG ((DEBUG_ERROR,
> > +          "     Granularity/SpecificFlag = %ld / %02x%s\n",
> > +          Descriptor->AddrSpaceGranularity, Descriptor->SpecificFlag,
> > +          ((Descriptor->SpecificFlag &
> > +            EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) ?
> > +            L" (Prefetchable)" : L""));
>
> This ternary is quite an effort to parse. I think a temporary
> variable for the retult of the & operation would improve this
> substantially.
>
> Alternatively, the wrapping and indentation used in
> Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLib.c
> is a lot easier to read.
>
> > +      }
> > +    }
> > +    /* Skip the END descriptor for root bridge */
> > +    ASSERT (Descriptor->Desc == ACPI_END_TAG_DESCRIPTOR);
> > +    Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)(
> > +                  (EFI_ACPI_END_TAG_DESCRIPTOR *)Descriptor + 1);
> > +  }
> > +}
> > diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
> > new file mode 100644
> > index 0000000..ced2c12
> > --- /dev/null
> > +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
> > @@ -0,0 +1,330 @@
> > +/** @file
> > +  PCI Host Bridge Library instance for Marvell 70x0/80x0
> > +
> > +  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> > +  Copyright (c) 2019 Marvell International Ltd. All rights reserved.<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.
> > +
> > +**/
> > +
> > +#include <PiDxe.h>
> > +
> > +#include <IndustryStandard/Pci22.h>
> > +
> > +#include <Library/ArmLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/MvGpioLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +
> > +#include <Protocol/BoardDesc.h>
> > +
> > +#include "PciHostBridgeLibConstructor.h"
> > +
> > +/**
> > +  This function configures PCIE controllers IATU windows.
> > +
> > +  @param [in] PcieBaseAddress PCIE controller base address.
> > +  @param [in] Index           IATU window index.
> > +  @param [in] CpuBase         Address from the CPU perspective.
> > +  @param [in] PciBase         Target PCIE address.
> > +  @param [in] Size            IATU window size.
> > +  @param [in] Type            IATU window type.
> > +  @param [in] EnableFlags     Extra configuration flags.
> > +
> > +  @retval none
> > +
> > +**/
> > +STATIC
> > +VOID
> > +ConfigureWindow (
> > +  IN EFI_PHYSICAL_ADDRESS PcieBaseAddress,
> > +  IN UINTN                Index,
> > +  IN UINT64               CpuBase,
> > +  IN UINT64               PciBase,
> > +  IN UINT64               Size,
> > +  IN UINTN                Type,
> > +  IN UINTN                EnableFlags
> > +  )
> > +{
> > +  ArmDataMemoryBarrier ();
> > +
> > +  MmioWrite32 (PcieBaseAddress + IATU_VIEWPORT_OFF,
> > +    IATU_VIEWPORT_OUTBOUND | IATU_VIEWPORT_REGION_INDEX (Index));
> > +
> > +  ArmDataMemoryBarrier ();
> > +
> > +  MmioWrite32 (PcieBaseAddress + IATU_LWR_BASE_ADDR_OFF_OUTBOUND_0,
> > +    (UINT32)(CpuBase & 0xFFFFFFFF));
> > +  MmioWrite32 (PcieBaseAddress + IATU_UPPER_BASE_ADDR_OFF_OUTBOUND_0,
> > +    (UINT32)(CpuBase >> 32));
> > +  MmioWrite32 (PcieBaseAddress + IATU_LIMIT_ADDR_OFF_OUTBOUND_0,
> > +    (UINT32)(CpuBase + Size - 1));
> > +  MmioWrite32 (PcieBaseAddress + IATU_LWR_TARGET_ADDR_OFF_OUTBOUND_0,
> > +    (UINT32)(PciBase & 0xFFFFFFFF));
> > +  MmioWrite32 (PcieBaseAddress + IATU_UPPER_TARGET_ADDR_OFF_OUTBOUND_0,
> > +    (UINT32)(PciBase >> 32));
> > +  MmioWrite32 (PcieBaseAddress + IATU_REGION_CTRL_1_OFF_OUTBOUND_0,
> > +    Type);
> > +  MmioWrite32 (PcieBaseAddress + IATU_REGION_CTRL_2_OFF_OUTBOUND_0,
> > +    IATU_REGION_CTRL_2_OFF_OUTBOUND_0_REGION_EN | EnableFlags);
> > +}
> > +
> > +/**
> > +  Perform PCIE slot reset using external GPIO pin.
> > +
> > +  @param [in] PcieBaseAddress  PCIE controller base address.
> > +
> > +  @retval none
> > +
> > +**/
> > +STATIC
> > +VOID
> > +WaitForLink (
> > +  IN EFI_PHYSICAL_ADDRESS PcieBaseAddress
> > +  )
> > +{
> > +  UINT32 Mask;
> > +  UINT32 Status;
> > +  UINT32 Timeout;
> > +
> > +  if (!(MmioRead32 (PcieBaseAddress + PCIE_PM_STATUS) & PCIE_PM_LTSSM_STAT_MASK)) {
> > +    DEBUG ((DEBUG_INIT, "%a: no PCIE device detected\n", __FUNCTION__));
> > +    return;
> > +  }
> > +
> > +  /* Wait for the link to establish itself */
> > +  DEBUG ((DEBUG_INIT, "%a: waiting for PCIE link\n", __FUNCTION__));
> > +
> > +  Mask = PCIE_GLOBAL_STATUS_RDLH_LINK_UP | PCIE_GLOBAL_STATUS_PHY_LINK_UP;
> > +  Timeout = PCIE_LINK_UP_TIMEOUT_US / 10;
> > +  do {
> > +    Status = MmioRead32 (PcieBaseAddress + PCIE_GLOBAL_STATUS_REG);
> > +    if ((Status & Mask) == Mask) {
> > +      DEBUG ((DEBUG_ERROR, "pcie@0x%x link UP\n", PcieBaseAddress));
> > +      break;
> > +    }
> > +    gBS->Stall (10);
>
> Why this Stall?
> Do we need a MemoryFence ()?

The stall is added in order to not to poll the link status too often
in the loop. I will add a comment.

>
> > +  } while (Timeout--);
> > +}
> > +
> > +/**
> > +  Perform PCIE slot reset using external GPIO pin.
> > +
> > +  @param [in] *PcieResetGpio  GPIO pin description.
> > +
> > +  @retval EFI_SUCEESS         PCIE slot reset succeeded.
> > +  @retval Other               Return error status.
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +ResetPcieSlot (
> > +  IN MV_GPIO_PIN *PcieResetGpio
> > +  )
> > +{
> > +  EMBEDDED_GPIO_MODE   Mode;
> > +  EMBEDDED_GPIO_PIN    GpioPin;
> > +  EMBEDDED_GPIO       *GpioProtocol;
> > +  EFI_STATUS           Status;
> > +
> > +  /* Get GPIO protocol */
> > +  Status = MvGpioGetProtocol (PcieResetGpio->ControllerType, &GpioProtocol);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: Unable to find GPIO protocol\n", __FUNCTION__));
> > +    return Status;
> > +  }
> > +
> > +  GpioPin = GPIO (PcieResetGpio->ControllerId, PcieResetGpio->PinNumber),
> > +
> > +  /* Reset the slot by toggling the GPIO pin */
> > +  Mode = PcieResetGpio->ActiveHigh ? GPIO_MODE_OUTPUT_1 : GPIO_MODE_OUTPUT_0;
> > +  Status = GpioProtocol->Set (GpioProtocol, GpioPin, Mode);
> > +  gBS->Stall (10 * 1000);
>
> Why this Stall?
> Do we need a MemoryFence ()?
>
> > +
> > +  Mode = PcieResetGpio->ActiveHigh ? GPIO_MODE_OUTPUT_0 : GPIO_MODE_OUTPUT_1;
> > +  Status = GpioProtocol->Set (GpioProtocol, GpioPin, Mode);
> > +  gBS->Stall (20 * 1000);
>
> Why this Stall?
> Do we need a MemoryFence ()?
>

I will add memory fence and add comments around stalls for justification.

Best regards,
Marcin

> /
>     Leif
>
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +  Obtain resources and perform a low-level PCIE controllers
> > +  configuration.
> > +
> > +  @param [in]  ImageHandle  The image handle.
> > +  @param [in] *SystemTable  The system table.
> > +
> > +  @retval EFI_SUCEESS       PCIE configuration successful.
> > +  @retval Other             Return error status.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +Armada7k8kPciHostBridgeLibConstructor (
> > +  IN EFI_HANDLE       ImageHandle,
> > +  IN EFI_SYSTEM_TABLE *SystemTable
> > +  )
> > +{
> > +  MARVELL_BOARD_DESC_PROTOCOL *BoardDescriptionProtocol;
> > +  MV_BOARD_PCIE_DESCRIPTION   *BoardPcieDescription;
> > +  MV_PCIE_CONTROLLER          *PcieController;
> > +  EFI_PHYSICAL_ADDRESS         PcieBaseAddress;
> > +  EFI_STATUS                   Status;
> > +  UINTN                        Index;
> > +
> > +  /* Obtain list of available controllers */
> > +  Status = gBS->LocateProtocol (&gMarvellBoardDescProtocolGuid,
> > +                  NULL,
> > +                  (VOID **)&BoardDescriptionProtocol);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot locate BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    return EFI_DEVICE_ERROR;
> > +  }
> > +
> > +  Status = BoardDescriptionProtocol->PcieDescriptionGet (
> > +                                       BoardDescriptionProtocol,
> > +                                       &BoardPcieDescription);
> > +  if (Status == EFI_NOT_FOUND) {
> > +    /* No controllers used on the platform, exit silently */
> > +    return EFI_SUCCESS;
> > +  } else if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot get Pcie board desc from BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    return EFI_DEVICE_ERROR;
> > +  }
> > +
> > +  for (Index = 0; Index < BoardPcieDescription->PcieControllerCount; Index++) {
> > +
> > +    PcieController = &(BoardPcieDescription->PcieControllers[Index]);
> > +
> > +    ASSERT (PcieController->PcieBusMin == 0);
> > +    ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB == 0);
> > +
> > +    if (PcieController->HaveResetGpio == TRUE) {
> > +      /* Reset PCIE slot */
> > +      Status = ResetPcieSlot (&PcieController->PcieResetGpio);
> > +      if (EFI_ERROR (Status)) {
> > +        DEBUG ((DEBUG_ERROR,
> > +          "%a: Cannot reset Pcie Slot\n",
> > +          __FUNCTION__));
> > +        return EFI_DEVICE_ERROR;
> > +      }
> > +    }
> > +
> > +    /* Low level PCIE controller configuration */
> > +    PcieBaseAddress = PcieController->PcieBaseAddress;
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PORT_LINK_CTRL_OFF,
> > +      ~PORT_LINK_CTRL_OFF_LINK_CAPABLE_MASK,
> > +      PORT_LINK_CTRL_OFF_LINK_CAPABLE_x4);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + GEN2_CTRL_OFF,
> > +      ~GEN2_CTRL_OFF_NUM_OF_LANES_MASK,
> > +      GEN2_CTRL_OFF_NUM_OF_LANES(4) | GEN2_CTRL_OFF_DIRECT_SPEED_CHANGE);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PCIE_GLOBAL_CTRL_OFFSET,
> > +      ~(PCIE_GLOBAL_CTRL_DEVICE_TYPE_MASK | PCIE_GLOBAL_APP_LTSSM_EN),
> > +      PCIE_GLOBAL_CTRL_DEVICE_TYPE_RC);
> > +
> > +    MmioWrite32 (PcieBaseAddress + PCIE_ARCACHE_TRC_REG,
> > +      ARCACHE_DEFAULT_VALUE);
> > +
> > +    MmioWrite32 (PcieBaseAddress + PCIE_AWCACHE_TRC_REG,
> > +      AWCACHE_DEFAULT_VALUE);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PCIE_ARUSER_REG,
> > +      ~AX_USER_DOMAIN_MASK,
> > +      AX_USER_DOMAIN_OUTER_SHAREABLE);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PCIE_AWUSER_REG,
> > +      ~AX_USER_DOMAIN_MASK,
> > +      AX_USER_DOMAIN_OUTER_SHAREABLE);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PCIE_LINK_CTL_2,
> > +      ~TARGET_LINK_SPEED_MASK,
> > +      LINK_SPEED_GEN_3);
> > +
> > +    MmioAndThenOr32 (PcieBaseAddress + PCIE_LINK_CAPABILITY,
> > +      ~TARGET_LINK_SPEED_MASK,
> > +      LINK_SPEED_GEN_3);
> > +
> > +    MmioOr32 (PcieBaseAddress + PCIE_GEN3_EQU_CTRL,
> > +      GEN3_EQU_EVAL_2MS_DISABLE);
> > +
> > +    MmioOr32 (PcieBaseAddress + PCIE_GLOBAL_CTRL_OFFSET,
> > +      PCIE_GLOBAL_APP_LTSSM_EN);
> > +
> > +    /* Region 0: MMIO32 range */
> > +    ConfigureWindow (PcieBaseAddress,
> > +      0,
> > +      PcieController->PcieMmio32WinBase,
> > +      PcieController->PcieMmio32WinBase,
> > +      PcieController->PcieMmio32WinSize,
> > +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,
> > +      0);
> > +
> > +    /* Region 1: Type 0 config space */
> > +    ConfigureWindow (PcieBaseAddress,
> > +      1,
> > +      PcieController->ConfigSpaceAddress,
> > +      0x0,
> > +      SIZE_64KB,
> > +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG0,
> > +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_CFG_SHIFT_MODE);
> > +
> > +    /* Region 2: Type 1 config space */
> > +    ConfigureWindow (PcieBaseAddress,
> > +      2,
> > +      PcieController->ConfigSpaceAddress + SIZE_64KB,
> > +      0x0,
> > +      PcieController->PcieBusMax * SIZE_1MB,
> > +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_CFG1,
> > +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_CFG_SHIFT_MODE);
> > +
> > +    /* Region 3: port I/O range */
> > +    ConfigureWindow (PcieBaseAddress,
> > +      3,
> > +      PcieController->PcieIoTranslation,
> > +      PcieController->PcieIoWinBase,
> > +      PcieController->PcieIoWinSize,
> > +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_IO,
> > +      0);
> > +
> > +    /* Region 4: MMIO64 range */
> > +    ConfigureWindow (PcieBaseAddress,
> > +      4,
> > +      PcieController->PcieMmio64WinBase,
> > +      PcieController->PcieMmio64WinBase,
> > +      PcieController->PcieMmio64WinSize,
> > +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,
> > +      0);
> > +
> > +    MmioOr32 (PcieBaseAddress + PCIE_GLOBAL_INT_MASK1_REG,
> > +      PCIE_INT_A_ASSERT_MASK |
> > +      PCIE_INT_B_ASSERT_MASK |
> > +      PCIE_INT_C_ASSERT_MASK |
> > +      PCIE_INT_D_ASSERT_MASK);
> > +
> > +    WaitForLink (PcieBaseAddress);
> > +
> > +    /* Enable the RC */
> > +    MmioOr32 (PcieBaseAddress + PCI_COMMAND_OFFSET,
> > +      EFI_PCI_COMMAND_IO_SPACE |
> > +      EFI_PCI_COMMAND_MEMORY_SPACE |
> > +      EFI_PCI_COMMAND_BUS_MASTER);
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > --
> > 2.7.4
> >