Re: [PATCH edk2-platforms 8/8] Silicon/SynQuacer/PlatformDxe: retrain PCIe switch links to Gen2 speed

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

On 12 December 2017 at 17:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Dec 12, 2017 at 10:38:07AM +0000, Ard Biesheuvel wrote:
>> For some reason, the Asmedia 118x PCIe switch needs a little help to
>> make sure that the downstream links train at Gen2 speed. So add a
>> PCI I/O protocol notifier that implements this for each PCIe downstream
>> port that is present on the system.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c          | 140 ++++++++++++++++++++
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c   |  13 +-
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h   |  37 ++++++
>>  Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf |   3 +
>>  4 files changed, 184 insertions(+), 9 deletions(-)
>>
>> diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c
>> new file mode 100644
>> index 000000000000..b069b42d0a42
>> --- /dev/null
>> +++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Pcie.c
>
> Bikeshedding time:
> This driver would likely be needed for any other platform including
> this switch as well, right?
>
> While it may be premature to create a standalone driver under
> Silicon/Asmedia ... how about calling this file something to make it
> clear that it is specifically intended to handle Asmedia 118x devices,
> to make it easier* to do so in the future? I.e. Asmedia118x.c?
>
> * by avoiding accruing other random bits of platform-specific PCI
>   hackery in the same file.
>

To be honest, I am not entirely sure. I need this hack for the
standalone card as well as the onboard switch, so it is not related to
a board level defect on developerbox. However, it could be related to
how the Synopsys IP manages the reset and training etc.

But I agree, let's move this to Asmedia118x.c and not create a generic
looking file that invites more PCI quirks to be parked there