From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-03.1984.is (mail-03.1984.is [93.95.224.70]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A39FD21D2E63C for ; Sun, 20 Aug 2017 11:05:51 -0700 (PDT) Received: from localhost by mail-03.1984.is with esmtpsa (TLSv1.2:DHE-RSA-AES128-SHA:128) (Exim 4.84_2) (envelope-from ) id 1djUe0-0001xv-L7; Sun, 20 Aug 2017 18:08:16 +0000 To: Ard Biesheuvel References: <20170819214114.8140-1-alan@softiron.com> <20170819214114.8140-2-alan@softiron.com> Cc: Leif Lindholm , "edk2-devel@lists.01.org" , linaro-uefi From: Alan Ott Message-ID: Date: Sun, 20 Aug 2017 14:08:13 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: 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:05:52 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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.