* [PATCH edk2-platforms 0/2] Re-enable the second SATA controller on Overdrive
@ 2017-08-19 21:41 Alan Ott
2017-08-19 21:41 ` [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits Alan Ott
2017-08-19 21:41 ` [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller Alan Ott
0 siblings, 2 replies; 8+ messages in thread
From: Alan Ott @ 2017-08-19 21:41 UTC (permalink / raw)
To: leif.lindholm, ard.biesheuvel; +Cc: edk2-devel, linaro-uefi, Alan Ott
Without the PCD for the second SATA Controller being specified, the boot
will hang. These patches fix it.
Alan Ott (2):
Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller
Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 10 +++-------
Silicon/AMD/Styx/AmdStyx.dec | 2 +-
Silicon/AMD/Styx/Drivers/StyxSataPlatformDxe/InitController.c | 4 ++--
3 files changed, 6 insertions(+), 10 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
2017-08-19 21:41 [PATCH edk2-platforms 0/2] Re-enable the second SATA controller on Overdrive Alan Ott
@ 2017-08-19 21:41 ` Alan Ott
2017-08-20 17:46 ` Ard Biesheuvel
2017-08-19 21:41 ` [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller Alan Ott
1 sibling, 1 reply; 8+ messages in thread
From: Alan Ott @ 2017-08-19 21:41 UTC (permalink / raw)
To: leif.lindholm, ard.biesheuvel; +Cc: edk2-devel, linaro-uefi, Alan Ott
Extra bits are needed to accomodate all 14 SATA ports
Signed-off-by: Alan Ott <alan@softiron.com>
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);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller
2017-08-19 21:41 [PATCH edk2-platforms 0/2] Re-enable the second SATA controller on Overdrive Alan Ott
2017-08-19 21:41 ` [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits Alan Ott
@ 2017-08-19 21:41 ` Alan Ott
2017-08-20 17:46 ` Ard Biesheuvel
1 sibling, 1 reply; 8+ messages in thread
From: Alan Ott @ 2017-08-19 21:41 UTC (permalink / raw)
To: leif.lindholm, ard.biesheuvel; +Cc: edk2-devel, linaro-uefi, Alan Ott
The comment indicating that only the first SATA controller is operational
on SoftIron-branded OverDrive 3000 boards is incorrect. Re-enable the
second SATA controller.
Signed-off-by: Alan Ott <alan@softiron.com>
Contributed-under: TianoCore Contribution Agreement 1.0
---
Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
index f256ffb..2881de3 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
@@ -409,14 +409,10 @@ DEFINE DO_FLASHER = FALSE
gArmTokenSpaceGuid.PcdGicDistributorBase|0xE1110000
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xE112F000
- #
- # AMD's B1 based Overdrive has 14 SATA ports across 2 controllers. However,
- # it appears that Softiron's Overdrive 3000, which is also B1 based, does
- # not have the second SATA controller enabled, and any attempts to use it
- # will crash the firmware. So use the first controller only.
- #
+ # SATA Ports
gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8
- gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xffff
+ gAmdStyxTokenSpaceGuid.PcdSata1PortCount|6
+ gAmdStyxTokenSpaceGuid.PcdSataPortMode|0x0fffffff
# PCIe Support
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
--
2.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
2017-08-19 21:41 ` [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits Alan Ott
@ 2017-08-20 17:46 ` Ard Biesheuvel
2017-08-20 18:08 ` Alan Ott
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2017-08-20 17:46 UTC (permalink / raw)
To: Alan Ott; +Cc: Leif Lindholm, edk2-devel@lists.01.org, linaro-uefi
On 19 August 2017 at 22:41, Alan Ott <alan@softiron.com> wrote:
> Extra bits are needed to accomodate all 14 SATA ports
>
> Signed-off-by: Alan Ott <alan@softiron.com>
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller
2017-08-19 21:41 ` [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller Alan Ott
@ 2017-08-20 17:46 ` Ard Biesheuvel
0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2017-08-20 17:46 UTC (permalink / raw)
To: Alan Ott; +Cc: Leif Lindholm, edk2-devel@lists.01.org, linaro-uefi
On 19 August 2017 at 22:41, Alan Ott <alan@softiron.com> wrote:
> The comment indicating that only the first SATA controller is operational
> on SoftIron-branded OverDrive 3000 boards is incorrect. Re-enable the
> second SATA controller.
>
> Signed-off-by: Alan Ott <alan@softiron.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Platform/AMD/OverdriveBoard/OverdriveBoard.dsc | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> index f256ffb..2881de3 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.dsc
> @@ -409,14 +409,10 @@ DEFINE DO_FLASHER = FALSE
> gArmTokenSpaceGuid.PcdGicDistributorBase|0xE1110000
> gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0xE112F000
>
> - #
> - # AMD's B1 based Overdrive has 14 SATA ports across 2 controllers. However,
> - # it appears that Softiron's Overdrive 3000, which is also B1 based, does
> - # not have the second SATA controller enabled, and any attempts to use it
> - # will crash the firmware. So use the first controller only.
> - #
> + # SATA Ports
> gAmdStyxTokenSpaceGuid.PcdSata0PortCount|8
> - gAmdStyxTokenSpaceGuid.PcdSataPortMode|0xffff
> + gAmdStyxTokenSpaceGuid.PcdSata1PortCount|6
> + gAmdStyxTokenSpaceGuid.PcdSataPortMode|0x0fffffff
>
> # PCIe Support
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0xF0000000
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
2017-08-20 17:46 ` Ard Biesheuvel
@ 2017-08-20 18:08 ` Alan Ott
2017-08-20 18:09 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Alan Ott @ 2017-08-20 18:08 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Leif Lindholm, edk2-devel@lists.01.org, linaro-uefi
On 08/20/2017 01:46 PM, Ard Biesheuvel wrote:
> On 19 August 2017 at 22:41, Alan Ott <alan@softiron.com> wrote:
>> Extra bits are needed to accomodate all 14 SATA ports
>>
>> Signed-off-by: Alan Ott <alan@softiron.com>
>> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
2017-08-20 18:08 ` Alan Ott
@ 2017-08-20 18:09 ` Ard Biesheuvel
2017-08-20 18:11 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2017-08-20 18:09 UTC (permalink / raw)
To: Alan Ott; +Cc: Leif Lindholm, edk2-devel@lists.01.org, linaro-uefi
On 20 August 2017 at 19:08, Alan Ott <alan@softiron.com> wrote:
> On 08/20/2017 01:46 PM, Ard Biesheuvel wrote:
>>
>> On 19 August 2017 at 22:41, Alan Ott <alan@softiron.com> wrote:
>>>
>>> Extra bits are needed to accomodate all 14 SATA ports
>>>
>>> Signed-off-by: Alan Ott <alan@softiron.com>
>>> 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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits
2017-08-20 18:09 ` Ard Biesheuvel
@ 2017-08-20 18:11 ` Ard Biesheuvel
0 siblings, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2017-08-20 18:11 UTC (permalink / raw)
To: Alan Ott; +Cc: Leif Lindholm, edk2-devel@lists.01.org, linaro-uefi
On 20 August 2017 at 19:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 20 August 2017 at 19:08, Alan Ott <alan@softiron.com> wrote:
>> On 08/20/2017 01:46 PM, Ard Biesheuvel wrote:
>>>
>>> On 19 August 2017 at 22:41, Alan Ott <alan@softiron.com> wrote:
>>>>
>>>> Extra bits are needed to accomodate all 14 SATA ports
>>>>
>>>> Signed-off-by: Alan Ott <alan@softiron.com>
>>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-20 18:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-19 21:41 [PATCH edk2-platforms 0/2] Re-enable the second SATA controller on Overdrive Alan Ott
2017-08-19 21:41 ` [PATCH edk2-platforms 1/2] Silicon/AMD/Styx: Make PcdSataPortMode 32 bits Alan Ott
2017-08-20 17:46 ` Ard Biesheuvel
2017-08-20 18:08 ` Alan Ott
2017-08-20 18:09 ` Ard Biesheuvel
2017-08-20 18:11 ` Ard Biesheuvel
2017-08-19 21:41 ` [PATCH edk2-platforms 2/2] Platform/AMD/OverdriveBoard: Re-enable the second SATA Controller Alan Ott
2017-08-20 17:46 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox