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@xxxxxxxxxxxx> wrote: > > Hi Ben, thank you for reviewing this patch. See below. > > On Mon, Aug 28, 2023 at 6:03 AM Ben Chuang <benchuanggli@xxxxxxxxx> 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: > > 1. 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
