Re: [PATCH edk2-platforms v4 8/9] Platform/ARM/Sgi: implement PciHostBridgeLib support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 21 May 2018 at 10:25, Thomas Abraham <thomas.abraham@arm.com> wrote:
> SGI platforms include a PCIe root complex to which a AHCI controller
> is attached as an endpoint. So implement the PciHostBridgeLib glue
> layer and enable support for PCIe and AHCI controllers.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Thomas Abraham <thomas.abraham@arm.com>
> ---
>  .../Library/PciHostBridgeLib/PciHostBridgeLib.c    | 189 +++++++++++++++++++++
>  .../Library/PciHostBridgeLib/PciHostBridgeLib.inf  |  63 +++++++
>  .../SgiPkg/Library/PlatformLib/PlatformLibMem.c    |  20 ++-
>  Platform/ARM/SgiPkg/SgiPlatform.dec                |   4 +
>  Platform/ARM/SgiPkg/SgiPlatform.dsc                |  48 ++++++
>  Platform/ARM/SgiPkg/SgiPlatform.fdf                |  20 +++
>  6 files changed, 343 insertions(+), 1 deletion(-)
>  create mode 100644 Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
>  create mode 100644 Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
>
> diff --git a/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> new file mode 100644
> index 0000000..f8bf9fc
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
> @@ -0,0 +1,189 @@
> +/** @file
> +*  PCI Host Bridge Library instance for ARM SGI platforms
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  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 <Protocol/PciHostBridgeResourceAllocation.h>
> +#include <Protocol/PciRootBridgeIo.h>
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED
> +CHAR16 *mPciHostBridgeLibAcpiAddressSpaceTypeStr[] = {
> +  L"Mem", L"I/O", L"Bus"
> +};
> +

You can make this

STATIC CHAR16 CONST * CONST

afaict

> +#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)
> +      }
> +    },
> +    EISA_PNP_ID (0x0A08), // PCIe
> +    0
> +  }, {
> +    END_DEVICE_PATH_TYPE,
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    {
> +      END_DEVICE_PATH_LENGTH,
> +      0
> +    }
> +  }
> +};
> +
> +STATIC PCI_ROOT_BRIDGE mPciRootBridge[] = {
> +  {
> +    0,                                              // Segment
> +    0,                                              // Supports
> +    0,                                              // Attributes
> +    FALSE,                                          // DmaAbove4G

Is 64-bit DMA not supported by this PCIe controller?

> +    FALSE,                                          // NoExtendedConfigSpace
> +    FALSE,                                          // ResourceAssigned
> +    EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM |          // AllocationAttributes
> +    EFI_PCI_HOST_BRIDGE_MEM64_DECODE,
> +    {
> +      // Bus
> +      FixedPcdGet32 (PcdPciBusMin),
> +      FixedPcdGet32 (PcdPciBusMax)
> +    }, {
> +      // Io
> +      FixedPcdGet64 (PcdPciIoBase),
> +      FixedPcdGet64 (PcdPciIoBase) + FixedPcdGet64 (PcdPciIoSize) - 1
> +    }, {
> +      // Mem
> +      FixedPcdGet32 (PcdPciMmio32Base),
> +      FixedPcdGet32 (PcdPciMmio32Base) + FixedPcdGet32 (PcdPciMmio32Size) - 1
> +    }, {
> +      // MemAbove4G
> +      FixedPcdGet64 (PcdPciMmio64Base),
> +      FixedPcdGet64 (PcdPciMmio64Base) + FixedPcdGet64 (PcdPciMmio64Size) - 1
> +    }, {
> +      // PMem
> +      MAX_UINT64,
> +      0
> +    }, {
> +      // PMemAbove4G
> +      MAX_UINT64,
> +      0
> +    },
> +    (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath
> +  },
> +};
> +
> +/**
> +  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
> +  )
> +{
> +  *Count = ARRAY_SIZE (mPciRootBridge);
> +  return mPciRootBridge;
> +}
> +
> +/**
> +  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
> +  )
> +{
> +  return;

Please drop this redundant return

> +}
> +
> +/**
> +  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;
> +  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""
> +                ));
> +      }
> +    }
> +    //
> +    // 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/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> new file mode 100644
> index 0000000..16c2295
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> @@ -0,0 +1,63 @@
> +## @file
> +#  PCI Host Bridge Library instance for ARM SGI platforms.
> +#
> +#  Copyright (c) 2016, Intel Corporation. All rights reserved.
> +#  Copyright (c) 2017, ARM Limited. All rights reserved.
> +#
> +#  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                    = 0x00010005
> +  BASE_NAME                      = PciHostBridgeLib
> +  FILE_GUID                      = 6879CEAD-DC94-42EB-895C-096D36B8083C
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = PciHostBridgeLib|DXE_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +#  VALID_ARCHITECTURES           = AARCH64 ARM
> +#
> +
> +[Sources]
> +  PciHostBridgeLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  ArmPkg/ArmPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  DevicePathLib
> +  IoLib
> +  MemoryAllocationLib
> +  UefiBootServicesTableLib
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdPciBusMin
> +  gArmTokenSpaceGuid.PcdPciBusMax
> +  gArmTokenSpaceGuid.PcdPciIoBase
> +  gArmTokenSpaceGuid.PcdPciIoSize
> +  gArmTokenSpaceGuid.PcdPciMmio32Base
> +  gArmTokenSpaceGuid.PcdPciMmio32Size
> +  gArmTokenSpaceGuid.PcdPciMmio32Translation
> +  gArmTokenSpaceGuid.PcdPciMmio64Base
> +  gArmTokenSpaceGuid.PcdPciMmio64Size
> +
> +[Protocols]
> +  gEfiCpuIo2ProtocolGuid          ## CONSUMES
> +
> +[Depex]
> +  gEfiCpuIo2ProtocolGuid
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> index f038559..e3e4d2a 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
> @@ -22,7 +22,7 @@
>  #include <SgiPlatform.h>
>
>  // Total number of descriptors, including the final "end-of-table" descriptor.
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  9
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS  12
>
>  /**
>    Returns the Virtual Memory Map of the platform.
> @@ -99,6 +99,24 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[Index].Length          = PcdGet64 (PcdSystemMemorySize);
>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>
> +  // PCI Configuration Space
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet32 (PcdPciConfigurationSpaceBaseAddress);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet32 (PcdPciConfigurationSpaceBaseAddress);
> +  VirtualMemoryTable[Index].Length          = PcdGet32 (PcdPciConfigurationSpaceSize);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // PCI Memory Space
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet32 (PcdPciMmio32Base);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet32 (PcdPciMmio32Base);
> +  VirtualMemoryTable[Index].Length          = PcdGet32 (PcdPciMmio32Size);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +
> +  // 64-bit PCI Memory Space
> +  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdPciMmio64Base);
> +  VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdPciMmio64Base);
> +  VirtualMemoryTable[Index].Length          = PcdGet64 (PcdPciMmio64Size);
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +

You can drop the MMIO mappings here: the PCI host bridge driver will
take care of this (but keep the one for the config space)

>    // End of Table
>    VirtualMemoryTable[++Index].PhysicalBase  = 0;
>    VirtualMemoryTable[Index].VirtualBase     = 0;
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec
> index d995937..0772c0b 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> @@ -35,3 +35,7 @@
>  [PcdsFeatureFlag.common]
>    # Set this PCD to TRUE to enable virtio support.
>    gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x00000001
> +
> +[PcdsFixedAtBuild.common]
> +  gArmSgiTokenSpaceGuid.PcdPciConfigurationSpaceBaseAddress|0x60000000|UINT32|0x00000011
> +  gArmSgiTokenSpaceGuid.PcdPciConfigurationSpaceSize|0x01FF0000|UINT32|0x00000012

Please drop these PCDs. You already have
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress for the former, and
for the latter, you can use

(PcdPciBusMax - PcdPciBusMin + 1) * SIZE_1MB


> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index f11c1f3..06c4c3d 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -76,6 +76,10 @@
>
>  [LibraryClasses.common.DXE_DRIVER]
>    FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
> +  PciHostBridgeLib|Platform/ARM/SgiPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
> +  PciSegmentLib|MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
> +  PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
> +  PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
>
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
> @@ -120,6 +124,24 @@
>    gArmTokenSpaceGuid.PcdGicDistributorBase|0x30000000
>    gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x300C0000
>
> +  #
> +  # PCIe
> +  #
> +  gArmTokenSpaceGuid.PcdPciBusMin|0
> +  gArmTokenSpaceGuid.PcdPciBusMax|255
> +  gArmTokenSpaceGuid.PcdPciIoBase|0x0
> +  gArmTokenSpaceGuid.PcdPciIoSize|0x00800000
> +  gArmTokenSpaceGuid.PcdPciIoTranslation|0x77800000
> +  gArmTokenSpaceGuid.PcdPciMmio32Base|0x70000000
> +  gArmTokenSpaceGuid.PcdPciMmio32Size|0x07800000
> +  gArmTokenSpaceGuid.PcdPciMmio32Translation|0x0
> +  gArmTokenSpaceGuid.PcdPciMmio64Base|0x500000000
> +  gArmTokenSpaceGuid.PcdPciMmio64Size|0x300000000
> +  gArmTokenSpaceGuid.PcdPciMmio64Translation|0x0
> +  gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x60000000
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|24
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
> +
>    ## PL011 - Serial Terminal
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x7FF80000
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200
> @@ -237,6 +259,7 @@
>    MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
>    MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
>    MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
> +  FatPkg/EnhancedFatDxe/Fat.inf
>
>    #
>    # Bds
> @@ -255,3 +278,28 @@
>
>    # SMSC LAN 91C111
>    EmbeddedPkg/Drivers/Lan91xDxe/Lan91xDxe.inf
> +
> +  #
> +  # Required by PCI
> +  #
> +  ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
> +
> +  #
> +  # PCI Support
> +  #
> +  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf {
> +    <PcdsFixedAtBuild>
> +      gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8010004F
> +  }
> +
> +  #
> +  # AHCI Support
> +  #
> +  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> +  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
> +
> +  #
> +  # SATA Controller
> +  #
> +  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> index 6f6e6aa..17cdf48 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> @@ -108,6 +108,26 @@ READ_LOCK_STATUS   = TRUE
>    INF RuleOverride=ACPITABLE Platform/ARM/SgiPkg/AcpiTables/Sgi575/AcpiTables.inf
>    INF MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
>
> +  # Required by PCI
> +  INF ArmPkg/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
> +
> +  #
> +  # PCI Support
> +  #
> +  INF MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +  INF MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf
> +
> +  #
> +  # AHCI Support
> +  #
> +  INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> +  INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
> +
> +  #
> +  # SATA Controller
> +  #
> +  INF MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> +
>    #
>    # Multiple Console IO support
>    #
> --
> 2.7.4
>