Re: [PATCH v7 1/2] PCI: iproc: Retry request when CRS returned from EP

[Oza Oza]

On Wed, Aug 23, 2017 at 11:30 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Wed, Aug 23, 2017 at 09:32:06PM +0530, Oza Oza wrote:
>> On Wed, Aug 23, 2017 at 9:22 PM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
>> > Hi Oza,
>> >
>> >> In working Enumuration case I get following:
>> >> [    9.125976] pci 0000:00:00.0: bridge configuration invalid ([bus
>> >> 00-00]), re-configuring
>> >> [    9.134267] where=0x0 val=0xffff0001
>> >> [    9.146946] where=0x0 val=0xffff0001
>> >> [    9.158943] where=0x0 val=0xffff0001
>> >> [    9.170945] where=0x0 val=0xffff0001
>> >> [    9.186945] where=0x0 val=0xffff0001
>> >> [    9.210944] where=0x0 val=0xffff0001
>> >> [    9.250943] where=0x0 val=0xffff0001
>> >> [    9.322942] where=0x0 val=0xffff0001
>> >> [    9.458943] where=0x0 val=0xffff0001
>> >> [    9.726942] where=0x0 val=0x9538086    >> actual vendor and device id.
>> >>
>> >> so I think I have to retry in RC driver, so the old code still holds good.
>> >> except that I have to do factoring out
>> > You need to return 0xFFFF0001 for vendor ID register and return 0xFFFFFFFF for
>> > other registers like COMMAND register during the CRS period.
>
> The proposal we're trying to implement is to handle this controller as
> an RP that does not support CRS SV.  Such an RP would never return
> 0xffff0001 for the vendor/device ID.  If it never got a successful
> completion, it would return 0xffffffff.
>
> So I think you do have to either retry (as in your v7 patch) or add a
> delay before we start enumeration.
>
> It looks like we waited somewhere between 320ms and 590ms before this
> device became ready.
>
> The PCI specs do require a delay after a reset (PCIe r3.1, sec 6.6.1)
> before software sends a config request.  I don't think there's
> anywhere in the PCI core where we delay before we first enumerate
> devices under a host bridge.  I'm not sure we'd *want* such a delay in
> the PCI core, because on many systems the BIOS has already initialized
> the PCI controller, and an additional delay would be unnecessary and
> would only slow down boot.
>
> But it might make sense to add a delay in the PCI controller driver --
> it knows when the reset occurs and probably knows more about the
> firmware environment.  I haven't tried to analyze all of sec 6.6.1,
> but this section:
>
>   Unless Readiness Notifications mechanisms are used (see Section
>   6.23), the Root Complex and/or system software must allow at least
>   1.0 s after a Conventional Reset of a device, before it may
>   determine that a device which fails to return a Successful
>   Completion status for a valid Configuration Request is a broken
>   device. This period is independent of how quickly Link training
>   completes.
>
>   Note: This delay is analogous to the Trhfa parameter specified for
>   PCI/PCI-X, and is intended to allow an adequate amount of time for
>   devices which require self initialization.
>
> makes it sound like a 1sec delay might be needed.  That sounds like an
> awful lot, but this device did take close to that amount of time.
>
> I don't care which way you go.  You've already implemented the delay
> in the v7 patch, and that's fine with me.
>

Thanks for your inputs Bjorn.
I will have v8 patch which will have;
factored out separate patch + retry implementation of v7 patch.

> Bjorn