| Message ID | 20180530181929.5066-2-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | More SynQuacer updates | expand |
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
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
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
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
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