From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 19238222A336F for ; Wed, 24 Jan 2018 03:07:50 -0800 (PST) Received: by mail-it0-x242.google.com with SMTP id p124so4772739ite.1 for ; Wed, 24 Jan 2018 03:13:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fqT0/nye7Gyo9pYOd5KoB1wbn8HdWHQcMDJUXODtx90=; b=WletSuJgLzlxCVc7NlqWhjU2NN3CGyCszSmfcQcOAvwzM53yOOmyvdoG6n4xvrhTiV z8P3dl+7So2MoWY7Rj9w6CiogvLwnYKDvWvm5SF2N93W7nOz92nH2dONksBM/Soq4wO8 XB1yyh2EPl/Iy8MtVuMTkRq/8dwUwrUc4FTSM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fqT0/nye7Gyo9pYOd5KoB1wbn8HdWHQcMDJUXODtx90=; b=YIYLNMbPeOkycID/KGz5tYf+PuZh5/l2uV4oCdmcivhKMxl6ATGhJ63hGbYerRJU97 FGfwYNTWfuXBWS4Ss3Q8CrvNlx4hSW//69AZV3nf8yKHxnk/n1Bosg87+/mbKRhM+Xii l3HHL/PXBy/XsLsDCBkbzCl95us92Q33yjhc4PCdeanjdKZcHIVGSkl0SDmu1Y2o1wG7 RPuxbgHsItfv15lLi4YyCR/urjVpGxdk4kMF8zzv5dVjUzJiswUsrJwXuWpQ1B8q/VGi CPUQtcC7uNmHoLrl5XdIToGCp+IrHZ2PdOLnLqJq0YGm+Zc8eKmiZsTpMlSc3NRn6uuz J8XQ== X-Gm-Message-State: AKwxyteRFHDiylAL9jAe/DfiAL3223wX/Ei8B5fYh9mCNhemv/jSZ+TV zLAk5vAWTW7zSXg0udIsivbP3Awl84tTbjSyudHp+w== X-Google-Smtp-Source: AH8x227nnN5eySAh0mFIIKUdeRGXHPWSzuQrU6snyWM3ExjFhtu6X1irwwIxWJY9sgklstn3Yl6A5gW3GZ/GDAFA9c0= X-Received: by 10.36.128.5 with SMTP id g5mr8051037itd.17.1516792398357; Wed, 24 Jan 2018 03:13:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Wed, 24 Jan 2018 03:13:17 -0800 (PST) In-Reply-To: <20180124111010.2u7e44zs3whxzeb7@bivouac.eciton.net> References: <20180124104052.9920-1-ard.biesheuvel@linaro.org> <20180124111010.2u7e44zs3whxzeb7@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 24 Jan 2018 11:13:17 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH] Silicon/SynQuacer/PlatformDxe: enable spread spectrum mode for ASM1061 SATA X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 11:07:51 -0000 Content-Type: text/plain; charset="UTF-8" On 24 January 2018 at 11:10, Leif Lindholm wrote: > 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 >> --- >> 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)? > Sure >> 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? > Only if PciVidPid[1] equals ASM1061_PID and ASM1182E_PID/ASM1184E_PID at the same time. > >> + // >> + // 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 >>