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: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)