From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::241; helo=mail-wm0-x241.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (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 2DF94222A336A for ; Wed, 24 Jan 2018 03:04:46 -0800 (PST) Received: by mail-wm0-x241.google.com with SMTP id r78so7836353wme.0 for ; Wed, 24 Jan 2018 03:10:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YTqsIBJz68+Aebxeln8veJbYSTApk/Y0ByKNKkhwuVU=; b=DoyvoMtNlJOAOAnAoPrwseFFhZI8RrWc5Llav9bEwkBCTxUczFOjj8yc1k2bAMtdPK Eyj/hvZHeotlEoJEhfJTTm7kiwL1pbiJbAWGQYOY5VxTfXH5o53JxT7nXM6ZnfeknIDh zhf8Vu4kBneib54ox14UCNdyExNCb7gRyKjas= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=YTqsIBJz68+Aebxeln8veJbYSTApk/Y0ByKNKkhwuVU=; b=aLP2CjzBX5axj80co6p16AGR/OPksbBfwxTLJOqVdGFkMU/+ZFO5Z9sWXwXhN1so2J VlhpDbUfhFY35TlaFnvZaqDtS7MmIJdpkx2CEGTZvMjDEZq88i3GLzDrLxgBWXcJiIdw Oi0/0AerBx1EGM2YvE04WrC30oP0PWJYkmOxpX6qIeVjP4SQYHButG7WBPYZFX6VonTH M3PAtAZ5kidAET6REMegQ1AtB9KU6buV0rCPCuo3xB9ez4CWlaImCk4ex9pTj5flXh1g gkVdnNg/EemnQWGDrNnfx9NQ6EEOieeMSRhUnsRHKDDV6/pn2fUExCWnmewSYM5bVMNO u8ZQ== X-Gm-Message-State: AKwxytfgnIx/TqPj+r/02Il5O//bz+fvrsJY+njtSQdI31+H3eDgGlIx 2zy6/l/tvLk43VEtwmf1ZVAsaYnacfA= X-Google-Smtp-Source: AH8x225clM+c35Ns01oinPiW2y7dwtMDjrTK9CKJB9iyHEGY0ceiNZJvSfaFGK8vBoMr9w8CDl/tEA== X-Received: by 10.28.120.19 with SMTP id t19mr4514778wmc.53.1516792213354; Wed, 24 Jan 2018 03:10:13 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 5sm2118741wre.35.2018.01.24.03.10.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 24 Jan 2018 03:10:12 -0800 (PST) Date: Wed, 24 Jan 2018 11:10:10 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <20180124111010.2u7e44zs3whxzeb7@bivouac.eciton.net> References: <20180124104052.9920-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20180124104052.9920-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) 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:04:47 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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)? > 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 >