[Linaro-uefi,v2,03/10] Platforms/AMD/Styx: set SATA port mode to Gen3 on all ports

Message ID 20170627132145.28159-4-ard.biesheuvel@linaro.org
State Accepted
Commit dc07fa34c77cddeed81ae0871c4bc55d8d060580
Headers show
Series
  • Platforms/AMD/Styx: various Cello related fixes
Related show

Commit Message

Ard Biesheuvel June 27, 2017, 1:21 p.m.
The SATA related PCDs consumed by AmdSataInitLib contain a PcdSataPortMode
PCD that sets the port mode to all ports. This PCD defaults to zero, while
the code in question has no default, i.e., the value 0 is not treated as
either 1, 2 or 3, and so the init sequence is not carried out correctly.

While observed SATA link detection issues on CelloBoard appear to be
unrelated to this (i.e., this change did not improve the situation),
let's set the correct values nonetheless.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 1 +
 Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 2 ++
 Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 2 +-
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Leif Lindholm June 27, 2017, 4:03 p.m. | #1
On Tue, Jun 27, 2017 at 01:21:38PM +0000, Ard Biesheuvel wrote:
> The SATA related PCDs consumed by AmdSataInitLib contain a PcdSataPortMode
> PCD that sets the port mode to all ports. This PCD defaults to zero, while
> the code in question has no default, i.e., the value 0 is not treated as
> either 1, 2 or 3, and so the init sequence is not carried out correctly.

Could you slip a "Gen" in there somewhere before pushing?
(I know it is in subject line as well.)

> While observed SATA link detection issues on CelloBoard appear to be
> unrelated to this (i.e., this change did not improve the situation),
> let's set the correct values nonetheless.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 1 +
>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 2 ++
>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 2 +-
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> index ddb944d0beb4..d10c0901c811 100644
> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
> @@ -404,6 +404,7 @@ DEFINE DO_KCS    = 0
>    #
>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
>    gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
>  
>    # PCIe Support
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> index f6d2d37014dd..298cf3eb1c28 100644
> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
> @@ -406,6 +406,8 @@ DEFINE DO_KCS       = 1
>    #
>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
>    gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
> +
>  
>    # PCIe Support
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> index 9d533149af07..6c284fb3b7db 100644
> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
> @@ -415,7 +415,7 @@ DEFINE DO_KCS       = 1
>    # will crash the firmware. So use the first controller only.
>    #
>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8
> -  gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xffff
>  
>    # PCIe Support
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
> -- 
> 2.9.3

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Ard Biesheuvel June 27, 2017, 4:04 p.m. | #2
On 27 June 2017 at 16:03, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Jun 27, 2017 at 01:21:38PM +0000, Ard Biesheuvel wrote:
>> The SATA related PCDs consumed by AmdSataInitLib contain a PcdSataPortMode
>> PCD that sets the port mode to all ports. This PCD defaults to zero, while
>> the code in question has no default, i.e., the value 0 is not treated as
>> either 1, 2 or 3, and so the init sequence is not carried out correctly.
>
> Could you slip a "Gen" in there somewhere before pushing?
> (I know it is in subject line as well.)
>

Sure.

>> While observed SATA link detection issues on CelloBoard appear to be
>> unrelated to this (i.e., this change did not improve the situation),
>> let's set the correct values nonetheless.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc                 | 1 +
>>  Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc | 2 ++
>>  Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc         | 2 +-
>>  3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> index ddb944d0beb4..d10c0901c811 100644
>> --- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> +++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
>> @@ -404,6 +404,7 @@ DEFINE DO_KCS    = 0
>>    #
>>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
>>    gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
>> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
>>
>>    # PCIe Support
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
>> diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> index f6d2d37014dd..298cf3eb1c28 100644
>> --- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> +++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
>> @@ -406,6 +406,8 @@ DEFINE DO_KCS       = 1
>>    #
>>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
>>    gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
>> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
>> +
>>
>>    # PCIe Support
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
>> diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> index 9d533149af07..6c284fb3b7db 100644
>> --- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> +++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
>> @@ -415,7 +415,7 @@ DEFINE DO_KCS       = 1
>>    # will crash the firmware. So use the first controller only.
>>    #
>>    gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8
>> -  gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
>> +  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xffff
>>
>>    # PCIe Support
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
>> --
>> 2.9.3
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Patch

diff --git a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
index ddb944d0beb4..d10c0901c811 100644
--- a/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
+++ b/Platforms/AMD/Styx/CelloBoard/CelloBoard.dsc
@@ -404,6 +404,7 @@  DEFINE DO_KCS    = 0
   #
   gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
   gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
+  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
 
   # PCIe Support
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
diff --git a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
index f6d2d37014dd..298cf3eb1c28 100644
--- a/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
+++ b/Platforms/AMD/Styx/Overdrive1000Board/Overdrive1000Board.dsc
@@ -406,6 +406,8 @@  DEFINE DO_KCS       = 1
   #
   gAmdStyxTokenSpaceGuid.PcdSata0PortCount|2
   gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
+  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xf
+
 
   # PCIe Support
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
diff --git a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
index 9d533149af07..6c284fb3b7db 100644
--- a/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platforms/AMD/Styx/OverdriveBoard/OverdriveBoard.dsc
@@ -415,7 +415,7 @@  DEFINE DO_KCS       = 1
   # will crash the firmware. So use the first controller only.
   #
   gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8
-  gAmdStyxTokenSpaceGuid.PcdSata1PortCount|0
+  gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xffff
 
   # PCIe Support
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000