Re: [PATCH] PCI: increase D3 delay for AMD Ryzen5/7 XHCI controllers

[Daniel Drake <drake@endlessm.com>]

On Fri, Nov 22, 2019 at 7:15 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > But then when pci_update_current_state() is called, it reads pmcsr as
> > 3 (D3hot). That's not what I would expect. I guess this means that
> > this platform's _PR3/_PS3 do not actually allow us to put the device
> > into D3cold,
>
> That you can't really say.
>
> Anyway, it is not guaranteed to do that.  For example, the power
> resource(s) listed by _PR3 for the device may be referenced by
> something else too which prevents them from being turned off.
>
> > and/or the _PR0/_PS0 transition does not actually transition the device to D0.
>
> Yes.
>
> Which may be the case if the power resource(s) in _PR3 have not been
> turned off really.
>
> [To debug this a bit more, you can enable dynamic debug in
> drivers/acpi/device_pm.c.]

We checked in an earlier thread before I figured out the timing detail
- these power resources are being turned off at this point.

> > While there is some ACPI strangeness here, the D3hot vs D3cold thing
> > is perhaps not the most relevant point. If I hack the code to avoid
> > D3cold altogether, just trying to do D0->D3hot->D0, it fails in the
> > same way.
>
> OK, but then you don't really flip the power resource(s), so that only
> means that _PS0 does not restore D0, but in general it only is valid
> to execute _PS0 after _PS3 (if both are present which is the case
> here), so this is not conclusive again.

_PS0 is called after _PS3 in the above case.

My feeling is that on this platform, we are not actually entering
D3cold at any point. Linux appears to be powering off the specified
ACPI power domains, but after turning them back on and executing _PS0
to move to D0initialized, the pmcsr still reporting D3 state seems
highly suspicious to me.

Also, I just experimented adding a pmscr register read to the end of
pci_set_power_state() , after pci_platform_power_transition() has been
called. If the power was truly cut and we're in D3cold then I would
expect this to fail. However the register read succeeds and returns
the same value 0x103.

During resume, Linux seems to have accurately detected this failure to
transition to D3cold in pci_update_current_state() by reading pmcsr
and setting dev->current_state to D3hot accordingly. We then deal with
what looks like a D3hot->D0 transition, which suffers the same failure
as seen when I force Linux to avoid D3cold and actually do a "real"
D0->D3hot->D0 cycle.

Presumably on a platform where D3cold actually works, after the device
has then been moved to D0uninitialized via ACPI _PS0 and _PR0,
pci_update_current_state() would then read pmcsr and update
dev->current_state to have value D0?

So in terms of the review comment questioning if the function name
quirk_d3_delay() and accompanying message "extending delay after
power-on from D3 to %d msec\n" is good (or whether it should say D3hot
or D3cold), maybe it should say D3hot. Plus a comment noting that
D3cold doesn't actually seem to be fully cold on this platform, so
we're actually dealing with a D3hot -> D0 transition?

Daniel