Re: [PATCH edk2-platforms v2] Silicon/Socionext/SynQuacer: implement menu option to set max PCIe speed

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

On Thu, Jan 25, 2018 at 03:31:19PM +0000, Ard Biesheuvel wrote:
> Add menu options to the SynQuacer Platform menu screen to limit the
> maximum PCIe link speed for each slot individually. This may be useful
> to work around potential PCIe issues.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2:
> Make the speed limit per-slot instead of per-RC. That does make this
> implementation more specific to DeveloperBox than it was before, but
> given the special knowledge about the on-board ASM1184e switch and the
> need for enabling spread-spectrum on the ASM1061, we're already past the
> point where PlatformDxe is a generic SynQuacer driver, and we may need to
> move it into Platform/Socionext (and clone it for each platform) in the
> future.
> 
>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc                                                     |   2 +
>  Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc                                         |   2 +
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pci.c                                                |  24 ++++-
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c                                        | 107 ++++++++++++++++++++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h                                        |  13 +++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf                                      |  11 ++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.uni                                   |  29 ++++++
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxeHii.vfr                                   |  69 +++++++++++++
>  Silicon/Socionext/SynQuacer/Include/Guid/SynQuacerPlatformFormSet.h                                  |  23 +++++
>  Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  |   5 +
>  Silicon/Socionext/SynQuacer/Include/Platform/VarStore.h                                              |  29 ++++++
>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLib.inf          |   2 +
>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  35 ++++++-
>  Silicon/Socionext/SynQuacer/SynQuacer.dec                                                            |   5 +
>  14 files changed, 349 insertions(+), 7 deletions(-)
> 
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 86685d1dec3b..2d46b4515749 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -405,6 +405,8 @@ [PcdsDynamicExDefault.common.DEFAULT]
>  [PcdsDynamicHii]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|30
>  
> +  gSynQuacerTokenSpaceGuid.PcdPlatformSettings|L"SynQuacerPlatformSettings"|gSynQuacerPlatformFormSetGuid|0x0|0x0|NV,BS
> +
>  [PcdsDynamicDefault]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0x0000000000000000
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0xFFFFFFFFFFFFFFFF
> diff --git a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> index b4b9239143bc..263b6454ff72 100644
> --- a/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> +++ b/Platform/Socionext/SynQuacerEvalBoard/SynQuacerEvalBoard.dsc
> @@ -397,6 +397,8 @@ [PcdsDynamicExDefault.common.DEFAULT]
>  [PcdsDynamicHii]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|L"Timeout"|gEfiGlobalVariableGuid|0x0|30
>  
> +  gSynQuacerTokenSpaceGuid.PcdPlatformSettings|L"SynQuacerPlatformSettings"|gSynQuacerPlatformFormSetGuid|0x0|0x0|NV,BS
> +
>  [PcdsDynamicDefault]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0x0000000000000000
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0xFFFFFFFFFFFFFFFF
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pci.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pci.c
> index 9af3dd942cdd..4e1b4a6f9080 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pci.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pci.c
> @@ -45,6 +45,11 @@ RetrainAsm1184eDownstreamPort (
>    EFI_STATUS                Status;
>    PCIE_CAP                  Cap;
>    PCI_REG_PCIE_LINK_CONTROL LinkControl;
> +  UINTN                     SegmentNumber;
> +  UINTN                     BusNumber;
> +  UINTN                     DeviceNumber;
> +  UINTN                     FunctionNumber;
> +  UINTN                     Location;
>  
>    //
>    // The upstream and downstream ports share the same PID/VID, so check
> @@ -64,8 +69,23 @@ RetrainAsm1184eDownstreamPort (
>      return;
>    }
>  
> -  DEBUG ((DEBUG_INFO, "%a: retraining ASM118x downstream PCIe port\n",
> -    __FUNCTION__));
> +  Status = PciIo->GetLocation (PciIo, &SegmentNumber, &BusNumber, &DeviceNumber,
> +                    &FunctionNumber);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Location = SYNQUACER_PCI_LOCATION (SegmentNumber, BusNumber, DeviceNumber);
> +  if ((Location == SYNQUACER_PCI_SLOT0_LOCATION &&
> +       mHiiSettings->PcieSlot0MaxSpeed == PCIE_MAX_SPEED_GEN1) ||
> +      (Location == SYNQUACER_PCI_SLOT1_LOCATION &&
> +       mHiiSettings->PcieSlot1MaxSpeed == PCIE_MAX_SPEED_GEN1) ||
> +      (Location == SYNQUACER_PCI_SLOT2_LOCATION &&
> +       mHiiSettings->PcieSlot2MaxSpeed == PCIE_MAX_SPEED_GEN1)) {

Having discussed with Ard on IRC, this looks to me like exactly the
kind of thing you would want to be able to have a static inline
function in a header file, due to it being a SoC-specific operation
also needed in a different module further down in this patch. (And
turning it into a library would be substantial overkill.)

Since that would currently be banned by the coding style, I have asked
Ard to replace this pattern with a switch statement instead, to
improve readability.

But I think I will bring this topic up for discussion at the spring
plugfest.

/
    Leif