Hi Sven and Stanisław,
Sorry for Stanisław, I have seen your reply, but I only reply to this Regarding the location of the source codes, if the maintainers don't comment. I'll follow them too. :)
On Tue, Aug 29, 2023 at 6:51 AM Sven van Ashbrook svenva@chromium.org wrote:
Hi Ben, thank you for reviewing this patch. See below.
On Mon, Aug 28, 2023 at 6:03 AM Ben Chuang benchuanggli@gmail.com wrote:
There is a situation for your reference. If `allow_runtime_pm' is set to false and the system resumes from suspend, GL9763E LPM negotiation will be always disabled on S0. GL9763E will stay L0 and never enter L1 because GL9763E LPM negotiation is disabled.
This patch enables allow_runtime_pm. The simple flow is gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a) (a) -+--> idle --> gl9763e_runtime_suspend() -> LPM enabled | +--> no idle -> gl9763e_runtime_resume() -> LPM disabled
This patch disables allow_runtime_pm. The simple flow is gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled (no runtime_pm)
Although that may not be the case with the current configuration, it's only a possibility.
Thanks so much for bringing this up. We have discussed internally and as far as we know, the current patch will work correctly in all cases. Could you verify our argument please?
The following assumptions are key:
- If CONFIG_PM is set, the runtime_pm framework is always present, i.e. there
cannot exist a kernel which has PM but lacks runtime_pm. 2. The pm_runtime framework always makes sure the device is runtime active before calling XXX_suspend, waking it up if need be. So when XXX_suspend gets called, the device is always runtime active. 3. if CONFIG_PM is set, runtime_pm can only be disabled via echo on > /sys/devices/.../power/control, and then the runtime_pm framework always keeps the device in runtime active. In such case LPM negotiation is always disabled.
Using these assumptions, we get:
Runtime_pm allowed: —------------------ gl9763e_runtime_resume() -> LPM disabled -> gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled -> (a) (a) -+--> idle --> gl9763e_runtime_suspend() -> LPM enabled | +--> no idle -> nothing - already runtime active -> LPM disabled
Runtime_pm not allowed: —---------------------- gl9763e_runtime_resume() always called -> LPM always disabled gl9763e_suspend() -> LPM enabled -> gl9763e_resume() -> LPM disabled
In all above cases the LPM negotiation flag is correct.
My concern is that when runtime_pm is false, gl9763e is disabled LPM negotiation, gl9763e can't enter L1.x and s0ix may fail. It seems that runtime_pm will always exist and that's ok.
sdhci doesn't know anything about the bus. It is independent of PCI, so I can't see how it would make any difference. One of the people cc'ed might know more. Jason Lai (cc'ed) added it for runtime PM.
As far as I know, when disabling LPM negotiation, the GL9763E will stop entering L1. It doesn't other side effect. Does Jason.Lai and Victor.Shih have any comments or suggestions?
Sounds like everyone assumes that you can freely change LPM negotiation on the PCIe bus after the cqhci_suspend() and sdhci_suspend_host() calls, so we will assume that too.
Ah, I suppose cqhci_suspend() may need to be done first safely, then gl9763e_set_low_power_negotiation(slot, true).
Best regards, Ben Chuang