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::244; helo=mail-wm0-x244.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com [IPv6:2a00:1450:400c:c09::244]) (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 62A0521E2568D for ; Thu, 25 Jan 2018 08:59:30 -0800 (PST) Received: by mail-wm0-x244.google.com with SMTP id f3so16240969wmc.1 for ; Thu, 25 Jan 2018 09:04:59 -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=qX55oervDR6WLrEWwp6f8JYcZxi0zmS9nMcZgwJWku0=; b=hK/VeL/DSuFScK16l6mAFpjVUerGz6xDmMWCFG71pjG4mukIr0SgHPiRzfurK8cfDZ 6gmjhJ1XlVpho1IbnyqVI3qonQFKApKaoZm2abCH+GQ5BxT8D1G1Ffgw7686mB98tdug H0WTL1q+oqrbpASX3n6dE9ZCVisHKNBMAaMgo= 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=qX55oervDR6WLrEWwp6f8JYcZxi0zmS9nMcZgwJWku0=; b=iMfUsbEY0Romc9uhskaQz486X8SwV2BBYcEqRHSVTnSiRaIytB0o6IxNX59H+AXP1T MVwbIzonHau75vFPpOJ4ZsFjNHb9QXoSJkMcw9xok/vWAZXl8YhIe9/lDSsvskk8gip6 5jcNcRPStMZQyDgQkDnupP7Z8dZryFvgJTFNjALyUcvk433qVbeSjxxXXBTFEY4OGt2t Kryf8Nrx5SQfTppXjEmFgwoZawsdq4krNBYZypXt+kAqOm6ItJIWfU0BFBxfj7eoWJUw QGl0L+j5XIh+CcyS3O+Boc/Y4GB4aq4tHA7iQ2CLw6fG36SzmjDiM6Etpwzc8+IoNht9 h9IA== X-Gm-Message-State: AKwxyteXZg7qQ0yeKDnWO408YZAgzA2i8CyPInk3z1YHWG24XsiS6jP2 YxnKraQw02gTlr3f6rmIoC7KN2YT5fk= X-Google-Smtp-Source: AH8x2277uaoDJ4hzh+pVGYb4z8PNakvG+CBbMENrUKaqjYha4e1MqQQvTnoirhZKgwjTndG43g9ctQ== X-Received: by 10.28.5.10 with SMTP id 10mr8172627wmf.126.1516899898237; Thu, 25 Jan 2018 09:04:58 -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 198sm2036462wmo.36.2018.01.25.09.04.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 25 Jan 2018 09:04:57 -0800 (PST) Date: Thu, 25 Jan 2018 17:04:55 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Message-ID: <20180125170455.ddqoyvdxn5qvk7eu@bivouac.eciton.net> References: <20180125153119.27596-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20180125153119.27596-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v2] Silicon/Socionext/SynQuacer: implement menu option to set max PCIe speed 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: Thu, 25 Jan 2018 16:59:31 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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