From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (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 66E4521C9127B for ; Sun, 20 Aug 2017 11:08:39 -0700 (PDT) Received: by mail-io0-x231.google.com with SMTP id g71so46079031ioe.5 for ; Sun, 20 Aug 2017 11:11:10 -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=bsJWXyxpJ7ItrSUPpkHzrkTU68lOSg42D1O9QqKhP24=; b=eglbfFVAFV8ZWMsuo4c8xeubcfDp8fbAfRrUTGe46ESdaXyroDQLTYpX5Ahc+1J9tL JJDKxSGYj7HLbsx5QaXgMTap2IgUiNe6TXEPhSd7ZaAnI9IWshX7yFXDFq5bcYyB//Yx gA0zkmfAVUafr+VoZbKm1hSg1/5MfHfG7mbdk= 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=bsJWXyxpJ7ItrSUPpkHzrkTU68lOSg42D1O9QqKhP24=; b=dXFpdNB4mcBeHZtWINAV5LXHEtVXWZqVTpjAXr3+fPu+mEtcctiJIlb4XVYZhiUOO/ HtrvZBIN0KF+JxkJnYtqBdh+ylM0Lga+A29HlIDZ3YdzrwgJEnm4fAq5OHcjnZet981S xXIrF30nojWFKUKDLCixxgy8pzqo5CrndASpmyg6hrv5sHvh4W2rDuD/Nzm7uW09giKu htmlIwezp2Sckcu+rOaYsv6YlTtai2mI7C6zbRR2bx/CxjxLNUEPtLUZaMj+RUyu1XhF 55EHA7+DloOV3i5m2F0O06XvTBF5vvnnQrPksotmxfJtibuLh3VxTturoyzCqKKBsBM/ 9d6A== X-Gm-Message-State: AHYfb5h9567oVck6IS9StmEWQrqTrCF9JAIC2sx3ZJWCVBU4CKX0mxLc 0ReZRDxoCN4/jHOOjzRMpZfjYxu3wczx X-Received: by 10.107.43.131 with SMTP id r125mr14081657ior.76.1503252669508; Sun, 20 Aug 2017 11:11:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Sun, 20 Aug 2017 11:11:09 -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:11:09 +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:08:39 -0000 Content-Type: text/plain; charset="UTF-8" On 20 August 2017 at 19:09, Ard Biesheuvel wrote: > 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. >> Thanks