Re: [PATCH] Silicon/SynQuacer/PlatformDxe: enable spread spectrum mode for ASM1061 SATA

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

On Wed, Jan 24, 2018 at 10:40:52AM +0000, Ard Biesheuvel wrote:
> The ASM1061 SATA controller integrated into the DeveloperBox board
> emits too much electromagnetic radiation, so it needs spread spectrum
> mode enabled.

I presume the spectrum mode is on the PCIe side rather than the SATA
side? Can this be explicitly mentioned?

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/{Asmedia118x.c => Asmedia.c} | 79 +++++++++++++++-----

Since you started renaming things, I invoke the right of bikeshedding.

The SATA controller is only Asmedia as well due to happenstance - if
this file is becoming "various PCI tweaks", can we just call it Pci.c
(or PciTweaks.c by all means)?

>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf              |  2 +-
>  2 files changed, 60 insertions(+), 21 deletions(-)
> 
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c
> similarity index 64%
> rename from Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c
> rename to Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c
> index c4cbacd3dff9..6c289fa1892e 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia118x.c
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Asmedia.c
> @@ -15,9 +15,12 @@
>  #include "PlatformDxe.h"
>  
>  #define ASMEDIA_VID                         0x1b21
> +#define ASM1061_PID                         0x0612
>  #define ASM1182E_PID                        0x1182
>  #define ASM1184E_PID                        0x1184
>  
> +#define ASM1061_SSC_OFFSET                  0xA10
> +
>  #define ASM118x_PCIE_CAPABILITY_OFFSET      0x80
>  #define ASM118x_PCIE_LINK_CONTROL_OFFSET    (ASM118x_PCIE_CAPABILITY_OFFSET + \
>                                               OFFSET_OF (PCI_CAPABILITY_PCIEXP, \
> @@ -39,24 +42,10 @@ 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
> @@ -91,6 +80,34 @@ RetrainAsm1184eDownstreamPort (
>  
>  STATIC
>  VOID
> +EnableAsm1061SpreadSpectrum (
> +  IN  EFI_PCI_IO_PROTOCOL   *PciIo
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT8       SscVal;
> +
> +  DEBUG ((DEBUG_INFO, "%a: enabling spread spectrum mode 0 for ASM1061\n",
> +    __FUNCTION__));
> +
> +  // SSC mode 0~-4000 ppm, 1:1 modulation
> +
> +  SscVal = 0;
> +  Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1,
> +                        &SscVal);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  MemoryFence ();
> +  gBS->Stall (1); // delay at least 100 ns between writes of the same register
> +
> +  SscVal = 1;
> +  Status = PciIo->Pci.Write (PciIo, EfiPciIoWidthUint8, ASM1061_SSC_OFFSET, 1,
> +                        &SscVal);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +STATIC
> +VOID
>  EFIAPI
>  OnPciIoProtocolNotify (
>    IN EFI_EVENT      Event,
> @@ -101,6 +118,7 @@ OnPciIoProtocolNotify (
>    EFI_STATUS                Status;
>    EFI_HANDLE                HandleBuffer;
>    UINTN                     BufferSize;
> +  UINT16                    PciVidPid[2];
>  
>    while (TRUE) {
>      BufferSize = sizeof (EFI_HANDLE);
> @@ -114,12 +132,33 @@ OnPciIoProtocolNotify (
>                      (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);
> +    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));
> +      continue;
> +    }
> +
> +    if (PciVidPid[0] != ASMEDIA_VID) {
> +      continue;
> +    }
> +
> +    if (PciVidPid[1] == ASM1061_PID) {
> +      //
> +      // The ASM1061 SATA controller as integrated into the DeveloperBox design
> +      // emits too much electromagnetic radiation. So enable spread spectrum
> +      // mode.
> +      //
> +      EnableAsm1061SpreadSpectrum (PciIo);
> +    } else if (PciVidPid[1] == ASM1182E_PID || PciVidPid[1] == ASM1184E_PID) {

Does this "else if" prevent the SATA controller from training GEN2?
Could it be just another if?

/
    Leif

> +      //
> +      // 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);
> +    }
>    }
>  }
>  
> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> index 16412b999a40..64c90ac74e8e 100644
> --- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -23,7 +23,7 @@ [Defines]
>    ENTRY_POINT                    = PlatformDxeEntryPoint
>  
>  [Sources]
> -  Asmedia118x.c
> +  Asmedia.c
>    Emmc.c
>    PlatformDxe.c
>    PlatformDxeHii.uni
> -- 
> 2.11.0
>