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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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)