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

[Leif Lindholm <leif.lindholm@linaro.org>]

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