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

[Pali Rohár]

On Tuesday 30 August 2022 13:15:26 Stefan Roese wrote:
> On 30.08.22 11:19, Pali Rohár wrote:
> > On Tuesday 30 August 2022 10:04:51 Maciej W. Rozycki wrote:
> > > On Sat, 27 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 have to agree with Pali here. We need to be careful with size increase
> in the SPL images, as some of the build targets are very limited here.
> So making this workaround configurable is definitely a good idea.
> 
> The question remains, at least for me, if the Kconfig option should be
> enable per default or not. For SPL my suggestions is to disable is per
> default because of the size remarks above. For U-Boot proper I'm not so
> sure. Please see below...
> 
> > > 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.
> > 
> > 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.
> 
> All this makes sense to me. You both have good points AFAICT. Please
> see below...
> 
> > >   I'd say in 99% of cases this will only cause frustration and they won't
> > > bother.  They will just conclude that either piece of hardware involved is
> > > broken and will throw it away.  Just as I almost did.  The seller has
> > > offered me a refund, which seems thought to be a universal solution
> > > nowadays (but I need to do what I meant to and getting money back doesn't
> > > solve it).
> > > 
> > >   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. 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.
> > 
> > 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)?
> 
> Agreed. But I also understand the reasoning from Maciej, at least in
> parts. Thinking a bit more about this, my preference would be to still
> include this workaround per default in U-Boot proper though. To not
> make things too complicated here.
> 
> Just my 0.02$.

I understand it.

Anyway, I would really to know where is the issue (in which part of PCIe
hierarchy) and what exactly is affected.

I think that deep understanding of the issue is important or at least
confirmation from the vendor (which we know that it would not come).

> > >   NB I'm slowly getting fed up with the amount of non-working stuff piling
> > > up around.  Every other piece of equipment I try doesn't work for one
> > > reason or another and I need to either chase bugs myself or to spend days
> > > and weeks to persuade someone at least to believe a problem is there to
> > > get that sorted.  All in my free time I'd rather spend on something else.
> > > I'd welcome things working automagically for a change so that I could
> > > focus on what I mean to be doing, and therefore I take breaking things
> > > deliberately as a major offence.
> > > 
> > >   FWIW,
> > > 
> > >    Maciej
> > 
> > What other U-Boot developers think?
> 
> Please see above.
> 
> Thanks,
> Stefan