[PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Maciej W. Rozycki]

On Tue, 30 Aug 2022, Pali Rohár wrote:

> > > Moreover this workaround is enabled for all existing hardware and also all
> > > future PCIe hardware, which opens a hole that other PCIe vendors may
> > > introduce same HW issue as on systems where this workaround is required and
> > > nobody would notice it because U-Boot automatically apply workaround for it.
> > 
> >  Why is it a problem?
> 
> I think I wrote it. One issue is that it is increasing size of SPL image
> and we really should not include into SPL things which are not required
> for all target platforms. Lot of boards have size constrained memory
> requirements and unnecessary features should not be automatically
> enabled.

 I agree on the SPL side.

> > Is the intent to cause hassle to end users and 
> > force them to take action when they have a non-working piece of hardware?  
> 
> Vendors should really test their hardware without any automatic
> workaround. Otherwise we are going into the hell if some workarounds are
> automatically enabled and nobody notice broken behavior. And vendors
> really build software with default options and do not care about it if
> default options are suitable.

 I suspect that most vendors of PCIe option cards do not verify them with 
U-Boot at all.

> There is already direction to make all workaround targeted and enabled
> only for platforms / hardware which really need it.
> 
> So enabling some workaround for all platforms which are produced and
> will be produced in the future is step backward.

 Well workarounds for option cards do necessarily have to be included with 
all hardware that has external PCI/e connectivity I'm afraid.

 The ASMedia ASM2824 switch is used with the StarTech PEX8M2E2 dual M.2
M-Key adapter.  Also sold under the Ableconn brand as PEXM2-130.  And M.2
M-Key to PCIe slot adapters are widely available.

 The Pericom PI7C9X2G304 switch is used with the Delock 41433 dual PCIe
slot adapter.  Also sold under the SYBA IOCrest brand as SI-PEX60016.
They have both been withdrawn AFAICT now, so availability may vary though.

 You can plug these option cards into any system.

> >  And at least I know what U-boot (or indeed firmware) is and have a 
> > general understanding of how computers work.  Most people just want to 
> > plug stuff in and use it for whatever their need is.  Expecting them to 
> > take action to get things working is wasting their time (which BTW seems 
> > to have been a growing trend in last ~30 years: putting burden on the end 
> > user to get our problems solved, which saves our time and money at the 
> > expense of end user's).
> 
> The another issue here is that it was not fully investigated where the
> issue is.

 It was and the outcome detailed along with the original submission.

> If it is processor specific, PCIe switch specific or endpoint
> specific, or combination of these options. It was just observed that
> proposed workaround fix this issue on one specific combination. And you
> confirmed this in previous post, that you are unsure if it is not
> specific to downstream ports of the switch.

 It was determined that the two generic PCIe switches named above fail to 
negotiate link.  I would expect other parts from the same vendors to show 
the same problem as IP blocks get reused between silicon.

> We really should not include such "bloatware" code to be enabled for
> everything, on every one board. You are the first who reported this
> issue. Nobody else complained about it, so I really do not see reason
> why all other users and developers must be forced to have it in their
> U-Boot binaries. Moreover lot of boards on which is U-Boot running do
> not have PCIe bus exported on the slot and have PCIe devices integrated
> and soldered on the board. Why on the earth I have to need this
> workaround also on these boards (which are moreover sized constrained)?

 You need to have a way to specify the absence of slot connectivity in 
board configuration then.  This will allow one to say:

	depends on PCI_SLOT || BADLY_BROKEN_BOARD

etc.

  Maciej