[edk2,edk2-platforms,1/3] Silicon/SynQuacerPciHostBridgeLib: add workaround for PCIe MMIO64

Message ID 20180530181929.5066-2-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • More SynQuacer updates
Related show

Commit Message

Ard Biesheuvel May 30, 2018, 6:19 p.m.
From: Masahisa KOJIMA <masahisa.kojima@linaro.org>


The current revision of SC2A11 contains PCIe bus issue.
In MRd transaction, 1st/Last DW BE fields are not correctly set
by hardware.

As a workaround, set TH bit and specify MSG_CODE in iATU.
With this setup, the value specified as MSG_CODE is set to the
1st/Last DW BE fields and PCIe controller can emit the correct
MRd TLP header.
Same workaround was already included for MMIO32 region,
MMIO64 region also requires this workaround.
Some deivices, such as Samsong SSD 970 EVO, do not work
without this modification.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Masahisa KOJIMA <masahisa.kojima@linaro.org>

---
 Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm May 31, 2018, 9:11 a.m. | #1
On Wed, May 30, 2018 at 08:19:27PM +0200, Ard Biesheuvel wrote:
> From: Masahisa KOJIMA <masahisa.kojima@linaro.org>

> 

> The current revision of SC2A11 contains PCIe bus issue.

> In MRd transaction, 1st/Last DW BE fields are not correctly set

> by hardware.

> 

> As a workaround, set TH bit and specify MSG_CODE in iATU.

> With this setup, the value specified as MSG_CODE is set to the

> 1st/Last DW BE fields and PCIe controller can emit the correct

> MRd TLP header.

> Same workaround was already included for MMIO32 region,

> MMIO64 region also requires this workaround.

> Some deivices, such as Samsong SSD 970 EVO, do not work

> without this modification.

>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Masahisa KOJIMA <masahisa.kojima@linaro.org>


Please add own S-o-b.

> ---

>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c | 5 +++--

>  1 file changed, 3 insertions(+), 2 deletions(-)

> 

> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> index e4679543cc66..227f9a725ce8 100644

> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> @@ -359,8 +359,9 @@ PciInitControllerPost (

>        RootBridge->MemAbove4G.Base,

>        RootBridge->MemAbove4G.Base,

>        RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Base + 1,

> -      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,

> -      0);

> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM |

> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TH,

> +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_MSG_CODE_32BIT);


Hmm ...
This fix clearly needs to go in. But since this is working around a
bug in first-revision silicon, should we not have something
conditional here?

/
    Leif

>    }

>  

>    // enable link

> -- 

> 2.17.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel May 31, 2018, 9:17 a.m. | #2
On 31 May 2018 at 11:11, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, May 30, 2018 at 08:19:27PM +0200, Ard Biesheuvel wrote:

>> From: Masahisa KOJIMA <masahisa.kojima@linaro.org>

>>

>> The current revision of SC2A11 contains PCIe bus issue.

>> In MRd transaction, 1st/Last DW BE fields are not correctly set

>> by hardware.

>>

>> As a workaround, set TH bit and specify MSG_CODE in iATU.

>> With this setup, the value specified as MSG_CODE is set to the

>> 1st/Last DW BE fields and PCIe controller can emit the correct

>> MRd TLP header.

>> Same workaround was already included for MMIO32 region,

>> MMIO64 region also requires this workaround.

>> Some deivices, such as Samsong SSD 970 EVO, do not work

>> without this modification.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Masahisa KOJIMA <masahisa.kojima@linaro.org>

>

> Please add own S-o-b.

>

>> ---

>>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c | 5 +++--

>>  1 file changed, 3 insertions(+), 2 deletions(-)

>>

>> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> index e4679543cc66..227f9a725ce8 100644

>> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> @@ -359,8 +359,9 @@ PciInitControllerPost (

>>        RootBridge->MemAbove4G.Base,

>>        RootBridge->MemAbove4G.Base,

>>        RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Base + 1,

>> -      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,

>> -      0);

>> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM |

>> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TH,

>> +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_MSG_CODE_32BIT);

>

> Hmm ...

> This fix clearly needs to go in. But since this is working around a

> bug in first-revision silicon, should we not have something

> conditional here?

>


In theory, yes. In practice, we have no idea yet whether a fixed
revision will ever materialize (the limited respin for the next
revision does not address this issue afaik), nor do we have any idea
how to distinguish them at runtime. Also, it is unlikely that we will
need to run older firmware builds on these new chips. So for the time
being, I think it is reasonable to apply this unconditionally (like we
do for the MMIO32 region already)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm May 31, 2018, 9:46 a.m. | #3
On Thu, May 31, 2018 at 11:17:47AM +0200, Ard Biesheuvel wrote:
> On 31 May 2018 at 11:11, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Wed, May 30, 2018 at 08:19:27PM +0200, Ard Biesheuvel wrote:

> >> From: Masahisa KOJIMA <masahisa.kojima@linaro.org>

> >>

> >> The current revision of SC2A11 contains PCIe bus issue.

> >> In MRd transaction, 1st/Last DW BE fields are not correctly set

> >> by hardware.

> >>

> >> As a workaround, set TH bit and specify MSG_CODE in iATU.

> >> With this setup, the value specified as MSG_CODE is set to the

> >> 1st/Last DW BE fields and PCIe controller can emit the correct

> >> MRd TLP header.

> >> Same workaround was already included for MMIO32 region,

> >> MMIO64 region also requires this workaround.

> >> Some deivices, such as Samsong SSD 970 EVO, do not work

> >> without this modification.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.1

> >> Signed-off-by: Masahisa KOJIMA <masahisa.kojima@linaro.org>

> >

> > Please add own S-o-b.

> >

> >> ---

> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c | 5 +++--

> >>  1 file changed, 3 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> >> index e4679543cc66..227f9a725ce8 100644

> >> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> >> @@ -359,8 +359,9 @@ PciInitControllerPost (

> >>        RootBridge->MemAbove4G.Base,

> >>        RootBridge->MemAbove4G.Base,

> >>        RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Base + 1,

> >> -      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,

> >> -      0);

> >> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM |

> >> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TH,

> >> +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_MSG_CODE_32BIT);

> >

> > Hmm ...

> > This fix clearly needs to go in. But since this is working around a

> > bug in first-revision silicon, should we not have something

> > conditional here?

> 

> In theory, yes. In practice, we have no idea yet whether a fixed

> revision will ever materialize (the limited respin for the next

> revision does not address this issue afaik), nor do we have any idea

> how to distinguish them at runtime.


This is clearly a problem, but even if we can't distinguish them, that
could be made a menu setting (since getting it wrong won't prevent you
from going into the menu and changing it back).

> Also, it is unlikely that we will

> need to run older firmware builds on these new chips. So for the time

> being, I think it is reasonable to apply this unconditionally (like we

> do for the MMIO32 region already)


My opinion stands. But if we already haven't conditionalised the
MMIO32 workaround, then I guess we don't need to tear up the world at
this point.

But I'll just file this forward request that workarounds for hardware
bugs are always conditionalised in future, so that
1) it's crystal clear what bits are workarounds
2) they can more easily be disabled when functional hardware appears.

Anyway
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel May 31, 2018, 10:49 a.m. | #4
On 31 May 2018 at 11:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, May 31, 2018 at 11:17:47AM +0200, Ard Biesheuvel wrote:

>> On 31 May 2018 at 11:11, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> > On Wed, May 30, 2018 at 08:19:27PM +0200, Ard Biesheuvel wrote:

>> >> From: Masahisa KOJIMA <masahisa.kojima@linaro.org>

>> >>

>> >> The current revision of SC2A11 contains PCIe bus issue.

>> >> In MRd transaction, 1st/Last DW BE fields are not correctly set

>> >> by hardware.

>> >>

>> >> As a workaround, set TH bit and specify MSG_CODE in iATU.

>> >> With this setup, the value specified as MSG_CODE is set to the

>> >> 1st/Last DW BE fields and PCIe controller can emit the correct

>> >> MRd TLP header.

>> >> Same workaround was already included for MMIO32 region,

>> >> MMIO64 region also requires this workaround.

>> >> Some deivices, such as Samsong SSD 970 EVO, do not work

>> >> without this modification.

>> >>

>> >> Contributed-under: TianoCore Contribution Agreement 1.1

>> >> Signed-off-by: Masahisa KOJIMA <masahisa.kojima@linaro.org>

>> >

>> > Please add own S-o-b.

>> >

>> >> ---

>> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c | 5 +++--

>> >>  1 file changed, 3 insertions(+), 2 deletions(-)

>> >>

>> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> >> index e4679543cc66..227f9a725ce8 100644

>> >> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

>> >> @@ -359,8 +359,9 @@ PciInitControllerPost (

>> >>        RootBridge->MemAbove4G.Base,

>> >>        RootBridge->MemAbove4G.Base,

>> >>        RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Base + 1,

>> >> -      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,

>> >> -      0);

>> >> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM |

>> >> +      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TH,

>> >> +      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_MSG_CODE_32BIT);

>> >

>> > Hmm ...

>> > This fix clearly needs to go in. But since this is working around a

>> > bug in first-revision silicon, should we not have something

>> > conditional here?

>>

>> In theory, yes. In practice, we have no idea yet whether a fixed

>> revision will ever materialize (the limited respin for the next

>> revision does not address this issue afaik), nor do we have any idea

>> how to distinguish them at runtime.

>

> This is clearly a problem, but even if we can't distinguish them, that

> could be made a menu setting (since getting it wrong won't prevent you

> from going into the menu and changing it back).

>

>> Also, it is unlikely that we will

>> need to run older firmware builds on these new chips. So for the time

>> being, I think it is reasonable to apply this unconditionally (like we

>> do for the MMIO32 region already)

>

> My opinion stands. But if we already haven't conditionalised the

> MMIO32 workaround, then I guess we don't need to tear up the world at

> this point.

>

> But I'll just file this forward request that workarounds for hardware

> bugs are always conditionalised in future, so that

> 1) it's crystal clear what bits are workarounds

> 2) they can more easily be disabled when functional hardware appears.

>

> Anyway

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>


Thanks

Pushed as c9be7b11ea10 (with my S-o-b added)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
index e4679543cc66..227f9a725ce8 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
@@ -359,8 +359,9 @@  PciInitControllerPost (
       RootBridge->MemAbove4G.Base,
       RootBridge->MemAbove4G.Base,
       RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Base + 1,
-      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM,
-      0);
+      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_MEM |
+      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TH,
+      IATU_REGION_CTRL_2_OFF_OUTBOUND_0_MSG_CODE_32BIT);
   }
 
   // enable link