From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (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 8953B21D2E63C for ; Sun, 20 Aug 2017 11:07:10 -0700 (PDT) Received: by mail-it0-x233.google.com with SMTP id x187so2290579ite.1 for ; Sun, 20 Aug 2017 11:09:41 -0700 (PDT) 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=NCSrETQ5XFYiOjpN+PSJpmSsjpQw9qVdGKM/DK2sVKk=; b=WwaeR/HucSMlNtrezabt1ZxorpO0C7NnxNLs6rPsTN6t54D3mZ//a0g4nGWGgn12ME WYzLMnLuVyn8GXiZWL8M9mFTOynYwRLLHINkiR4a7xtBqxkj6RV1yZbd4wGag5HAK19s Yfj/IQVSA/A2irj1a3fyI0T0NCpOsl1uWGHVc= 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=NCSrETQ5XFYiOjpN+PSJpmSsjpQw9qVdGKM/DK2sVKk=; b=Y/J47TPEw6EUFZP1FIPQ8gdZfQIwkgaQ3BxBTZema1+WCNGUIbA8eRRCHGm6DFJ5uf qAESoK6hINkLDEc4iGtByTGuGuFVUmsyzYKPlo98OWVNS/qx/8gpFqmUsKYKex0BEspq gMNnhDCC1OO0WfzCshv/6Qf4tBCpYPnFSq924vO72T7cu95FcOfCgmorREPV5KMzEVcr dPfe0Vevy+sObV4divXKXSrZ4HJZFYEI9POiBiDqBCEe5l5VdKEf36RAfADmUia7NVh2 ZT9IZhJPdxa0ktYSPgS5xzxmS/OdexWCu2vLQBMHJx42UAZ+p276/aLPxwXeUpdpZP5b 1K5w== X-Gm-Message-State: AHYfb5idAwq9AcraL0fKWkeGewE5Cede22yaHmeMreG5x8wdbr8zGMoL a22L4yi2fUPH+1YTBTBUEeQnz7piFyQaGm4= X-Received: by 10.36.41.143 with SMTP id p137mr3691466itp.98.1503252580655; Sun, 20 Aug 2017 11:09:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Sun, 20 Aug 2017 11:09:40 -0700 (PDT) In-Reply-To: References: <20170819214114.8140-1-alan@softiron.com> <20170819214114.8140-2-alan@softiron.com> From: Ard Biesheuvel Date: Sun, 20 Aug 2017 19:09:40 +0100 Message-ID: To: Alan Ott Cc: Leif Lindholm , "edk2-devel@lists.01.org" , linaro-uefi Subject: Re: [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 20 Aug 2017 18:07:10 -0000 Content-Type: text/plain; charset="UTF-8" On 20 August 2017 at 19:08, Alan Ott wrote: > On 08/20/2017 01:46 PM, Ard Biesheuvel wrote: >> >> On 19 August 2017 at 22:41, Alan Ott wrote: >>> >>> Extra bits are needed to accomodate all 14 SATA ports >>> >>> Signed-off-by: Alan Ott >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> --- >>> Silicon/AMD/Styx/AmdStyx.dec | 2 +- >>> Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c | 4 ++-- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/Silicon/AMD/Styx/AmdStyx.dec b/Silicon/AMD/Styx/AmdStyx.dec >>> index ddd5bf4..c6eebe6 100644 >>> --- a/Silicon/AMD/Styx/AmdStyx.dec >>> +++ b/Silicon/AMD/Styx/AmdStyx.dec >>> @@ -54,7 +54,7 @@ >>> >>> gAmdStyxTokenSpaceGuid.PcdSata0CtrlAxiSlvPort|0xE0300000|UINT32|0x00020000 >>> gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8|UINT8|0x00020001 >>> gAmdStyxTokenSpaceGuid.PcdSataPi|0xFF|UINT32|0x00020002 >>> - gAmdStyxTokenSpaceGuid.PcdSataPortMode|0|UINT16|0x00020003 >>> + gAmdStyxTokenSpaceGuid.PcdSataPortMode|0|UINT32|0x00020003 >>> gAmdStyxTokenSpaceGuid.PcdSataPortMpsp|TRUE|BOOLEAN|0x00020004 >>> gAmdStyxTokenSpaceGuid.PcdSataSmpsSupport|FALSE|BOOLEAN|0x00020005 >>> gAmdStyxTokenSpaceGuid.PcdSataSssSupport|TRUE|BOOLEAN|0x00020006 >>> diff --git >>> a/Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c >>> b/Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c >>> index 1958d91..78c6819 100644 >>> --- a/Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c >>> +++ b/Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c >>> @@ -110,8 +110,8 @@ InitializeSataController ( >>> SataChPerSerdes = FixedPcdGet8 (PcdSataNumChPerSerdes); >>> >>> for (PortNum = 0; PortNum < SataPortCount; PortNum += SataChPerSerdes) >>> { >>> - EvenPort = (UINT32)(FixedPcdGet16 (PcdSataPortMode) >> (PortNum * >>> 2)) & 3; >>> - OddPort = (UINT32)(FixedPcdGet16 (PcdSataPortMode) >> ((PortNum+1) * >>> 2)) & 3; >>> + EvenPort = (UINT32)(FixedPcdGet32 (PcdSataPortMode) >> (PortNum * >>> 2)) & 3; >>> + OddPort = (UINT32)(FixedPcdGet32 (PcdSataPortMode) >> ((PortNum+1) * >>> 2)) & 3; >>> SataPhyInit ((StartPort + PortNum) / SataChPerSerdes, EvenPort, >>> OddPort); >>> } >>> >> >> This doesn't look right to me. PortNum will always be in the set { 0, >> 2, 4, 6 } due to SataPortCount being equal to either the port count of >> sata 0 or of sata 1. So the top 16 bits of the new PCD are never >> referenced, and the port mode for sata 0 ends up being applied to sata >> 1. >> >> Better use >> >> for (PortNum = StartPort; PortNum < StartPort + SataPortCount; ...) >> ..etc.. >> > > I agree. This has apparently been broken all along. I'll fix and resubmit. > >> >> Also, this code relies rather heavily on SataChPerSerdes == 2, so we >> might want to add an ASSERT () for that. >> > > I can do that too. > > Alan. >