From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x235.google.com (mail-io0-x235.google.com [IPv6:2607:f8b0:4001:c06::235]) (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 5300621C9127B for ; Sun, 20 Aug 2017 10:43:43 -0700 (PDT) Received: by mail-io0-x235.google.com with SMTP id p141so4205537iop.3 for ; Sun, 20 Aug 2017 10:46:14 -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=DFqfzWCRcR4rJHbzYTjnVW6NPnvnKiOeLlbHwta9oWk=; b=Sg7Duejf1u3Gko7A/7FQMeI9YpBIAaLQcYVx6x/8pVHdFPoiy1kb3ZBemzx6xmWtFh djZ13aP8SIDHtd3Q5Gh+wvIrtRhS5KcBQc/F6FPqgX0MqWC3QVgScqkgSkQqWSCG/AD5 NNdelYCZybT9pHbnzPp4N3FeGI1CZePCJWros= 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=DFqfzWCRcR4rJHbzYTjnVW6NPnvnKiOeLlbHwta9oWk=; b=RTCdrX/cq7UFc2uGKxicsIkmopIr4r9V1Nq4i9WqgKsLlbeGSQmNOyNbk5lncUmgPj fB6qZDtvl0Gj+pU2GqkKS7SJEUELVkGCGcXJN6BNkRS9mXk56hKu/64SHZcCvDEDsmCC KOXDKV2f0krEq3x1mCyWGMSu2vL/bDIKkDMtMKm69Bc0YcULdqcvYHUNM/m1lPjztF06 AHSDrQREbbjywkddCUW2tX0m2pFf+d157FfTPOt6DVDIuu1aBCGrwekGH37RxE/c1Qq7 VAxbAkyyzEvPdXFRJ9FlHhT7fZb+yB7Aa8lfuFnQ0OmZnTOD2S4swWQCQ/xJl2xTs5qq YRjQ== X-Gm-Message-State: AHYfb5gNzznDKqqff0Zgfiag2q+pxGdRjveHwQUpOSVBk5+1+V6/ABtr E7wmFDIlpqHFucS1GwJZZDUyGzQnY7xR X-Received: by 10.107.162.21 with SMTP id l21mr13661736ioe.154.1503251173377; Sun, 20 Aug 2017 10:46:13 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Sun, 20 Aug 2017 10:46:12 -0700 (PDT) In-Reply-To: <20170819214114.8140-2-alan@softiron.com> References: <20170819214114.8140-1-alan@softiron.com> <20170819214114.8140-2-alan@softiron.com> From: Ard Biesheuvel Date: Sun, 20 Aug 2017 18:46:12 +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 17:43:43 -0000 Content-Type: text/plain; charset="UTF-8" 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.. Also, this code relies rather heavily on SataChPerSerdes == 2, so we might want to add an ASSERT () for that.