Re: [PATCH edk2-platforms 8/8] Silicon/SynQuacer/PlatformDxe: retrain PCIe switch links to Gen2 speed

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Dec 12, 2017 at 10:38:07AM +0000, Ard Biesheuvel wrote:
> For some reason, the Asmedia 118x PCIe switch needs a little help to
> make sure that the downstream links train at Gen2 speed. So add a
> PCI I/O protocol notifier that implements this for each PCIe downstream
> port that is present on the system.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c          | 140 ++++++++++++++++++++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c   |  13 +-
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h   |  37 ++++++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf |   3 +
>  4 files changed, 184 insertions(+), 9 deletions(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c
> new file mode 100644
> index 000000000000..b069b42d0a42
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c

Bikeshedding time:
This driver would likely be needed for any other platform including
this switch as well, right?

While it may be premature to create a standalone driver under
Silicon/Asmedia ... how about calling this file something to make it
clear that it is specifically intended to handle Asmedia 118x devices,
to make it easier* to do so in the future? I.e. Asmedia118x.c?

* by avoiding accruing other random bits of platform-specific PCI
  hackery in the same file.

/
    Leif

> @@ -0,0 +1,140 @@
> + /** @file
> +  SynQuacer DXE platform driver - PCIe support
> +
> +  Copyright (c) 2017, Linaro, 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 "PlatformDxe.h"
> +
> +#define ASMEDIA_VID                         0x1b21
> +#define ASM1182E_PID                        0x1182
> +#define ASM1184E_PID                        0x1184
> +
> +#define ASM118x_PCIE_CAPABILITY_OFFSET      0x80
> +#define ASM118x_PCIE_LINK_CONTROL_OFFSET    (ASM118x_PCIE_CAPABILITY_OFFSET + \
> +                                             OFFSET_OF (PCI_CAPABILITY_PCIEXP, \
> +                                                        LinkControl))
> +
> +STATIC VOID         *mPciProtocolNotifyRegistration;
> +STATIC EFI_EVENT    mPciProtocolNotifyEvent;
> +
> +#pragma pack(1)
> +typedef struct {
> +  EFI_PCI_CAPABILITY_HDR    CapHdr;
> +  PCI_REG_PCIE_CAPABILITY   Pcie;
> +} PCIE_CAP;
> +#pragma pack()
> +
> +STATIC
> +VOID
> +RetrainAsm1184eDownstreamPort (
> +  IN  EFI_PCI_IO_PROTOCOL   *PciIo
> +  )
> +{
> +  UINT16                    PciVidPid[2];
> +  EFI_STATUS                Status;
> +  PCIE_CAP                  Cap;
> +  PCI_REG_PCIE_LINK_CONTROL LinkControl;
> +
> +  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16, PCI_VENDOR_ID_OFFSET,
> +                        ARRAY_SIZE (PciVidPid), &PciVidPid);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_WARN, "%a: failed to read PCI vendor/product ID - %r\n",
> +      __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  if (PciVidPid[0] != ASMEDIA_VID ||
> +      (PciVidPid[1] != ASM1182E_PID && PciVidPid[1] != ASM1184E_PID)) {
> +    return;
> +  }
> +
> +  //
> +  // The upstream and downstream ports share the same PID/VID, so check
> +  // the port type. This assumes the PCIe Express capability block lives
> +  // at offset 0x80 in the port's config space, which is known to be the
> +  // case for these particular chips.
> +  //
> +  ASSERT (sizeof (Cap) == sizeof (UINT32));
> +  ASSERT (sizeof (LinkControl) == sizeof (UINT16));
> +
> +  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32,
> +                        ASM118x_PCIE_CAPABILITY_OFFSET, 1, &Cap);
> +  ASSERT_EFI_ERROR (Status);
> +  ASSERT (Cap.CapHdr.CapabilityID == EFI_PCI_CAPABILITY_ID_PCIEXP);
> +
> +  if (Cap.Pcie.Bits.DevicePortType != PCIE_DEVICE_PORT_TYPE_DOWNSTREAM_PORT) {
> +    return;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: retraining ASM1184x downstream PCIe port\n",
> +    __FUNCTION__));
> +
> +  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint16,
> +                        ASM118x_PCIE_LINK_CONTROL_OFFSET, 1, &LinkControl);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  LinkControl.Bits.RetrainLink = 1;
> +
> +  Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint16,
> +                        ASM118x_PCIE_LINK_CONTROL_OFFSET, 1, &LinkControl);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +STATIC
> +VOID
> +EFIAPI
> +OnPciIoProtocolNotify (
> +  IN EFI_EVENT      Event,
> +  IN VOID           *Context
> +  )
> +{
> +  EFI_PCI_IO_PROTOCOL       *PciIo;
> +  EFI_STATUS                Status;
> +  EFI_HANDLE                HandleBuffer;
> +  UINTN                     BufferSize;
> +
> +  while (TRUE) {
> +    BufferSize = sizeof (EFI_HANDLE);
> +    Status = gBS->LocateHandle (ByRegisterNotify, NULL,
> +                    mPciProtocolNotifyRegistration, &BufferSize, &HandleBuffer);
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    Status = gBS->HandleProtocol (HandleBuffer, &gEfiPciIoProtocolGuid,
> +                    (VOID **)&PciIo);
> +    ASSERT_EFI_ERROR (Status);
> +
> +    //
> +    // The ASM1184E 4-port PCIe switch on the DeveloperBox board (and its
> +    // 2-port sibling of which samples were used in development) needs a
> +    // little nudge to get it to train the downstream links at Gen2 speed.
> +    //
> +    RetrainAsm1184eDownstreamPort (PciIo);
> +  }
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +RegisterPcieNotifier (
> +  VOID
> +  )
> +{
> +  mPciProtocolNotifyEvent = EfiCreateProtocolNotifyEvent (
> +                              &gEfiPciIoProtocolGuid,
> +                              TPL_CALLBACK,
> +                              OnPciIoProtocolNotify,
> +                              NULL,
> +                              &mPciProtocolNotifyRegistration);
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> index e58a2093eb49..098a4dbd324e 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
> @@ -12,15 +12,7 @@
>    WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>  
> -#include <PiDxe.h>
> -#include <Library/BaseMemoryLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/DtPlatformDtbLoaderLib.h>
> -#include <Library/IoLib.h>
> -#include <Library/MemoryAllocationLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> -#include <Platform/MemoryMap.h>
> -#include <Protocol/NonDiscoverableDevice.h>
> +#include "PlatformDxe.h"
>  
>  STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mNetsecDesc[] = {
>    {
> @@ -202,5 +194,8 @@ PlatformDxeEntryPoint (
>  
>    SmmuEnableCoherentDma ();
>  
> +  Status = RegisterPcieNotifier ();
> +  ASSERT_EFI_ERROR (Status);
> +
>    return EFI_SUCCESS;
>  }
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
> new file mode 100644
> index 000000000000..d1dad2a3eace
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
> @@ -0,0 +1,37 @@
> +/** @file
> +  SynQuacer DXE platform driver.
> +
> +  Copyright (c) 2017, Linaro, 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 __PLATFORM_DXE_H__
> +#define __PLATFORM_DXE_H__
> +
> +#include <PiDxe.h>
> +#include <IndustryStandard/Pci.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DtPlatformDtbLoaderLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Platform/MemoryMap.h>
> +#include <Protocol/NonDiscoverableDevice.h>
> +#include <Protocol/PciIo.h>
> +
> +EFI_STATUS
> +EFIAPI
> +RegisterPcieNotifier (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> index 00c1130906c4..84498eaddcef 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -23,6 +23,7 @@ [Defines]
>    ENTRY_POINT                    = PlatformDxeEntryPoint
>  
>  [Sources]
> +  Pcie.c
>    PlatformDxe.c
>  
>  [Packages]
> @@ -41,6 +42,7 @@ [LibraryClasses]
>    MemoryAllocationLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  UefiLib
>  
>  [Guids]
>    gFdtTableGuid
> @@ -50,6 +52,7 @@ [Guids]
>  
>  [Protocols]
>    gEdkiiNonDiscoverableDeviceProtocolGuid         ## PRODUCES
> +  gEfiPciIoProtocolGuid                           ## CONSUMES
>    gPcf8563RealTimeClockLibI2cMasterProtolGuid     ## PRODUCES
>  
>  [FixedPcd]
> -- 
> 2.11.0
>