Re: [PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix SoCs can suspend

[Ben Chuang]

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