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

[Stefan Roese]

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$.

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