Re: [PATCH] Revert "mmc: sdhci-xenon: add runtime pm support and reimplement standby"

[Ulf Hansson]

On Fri, 15 May 2020 at 08:00, Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote:
>
> On Thu, 14 May 2020 12:18:58 +0200 Ulf Hansson wrote:
>
> >
> >
> > On Thu, 14 May 2020 at 07:45, Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 13 May 2020 14:15:21 +0200 Ulf Hansson wrote:
> > >
> > > >
> > > >
> > > > On Wed, 13 May 2020 at 11:47, Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > This reverts commit a027b2c5fed78851e69fab395b02d127a7759fc7.
> > > > >
> > > > > The HW supports auto clock gating, so it's useless to do runtime pm
> > > > > in software.
> > > >
> > > > Runtime PM isn't soley about clock gating. Moreover it manages the
> > >
> > > Per my understanding, current xenon rpm implementation is just clock gating.
>
> what's your option about this? My point is the HW can auto clock
> gate, so what's the benefit of current rpm implementation given it only does
> clock gating. FWICT, when submitting the xenon rpm patch, I don't think the
> author  compared the power consumption. If the comparison is done, it's easy
> to find the rpm doesn't bring any power consumption benefit at all.

As I stated, runtime PM isn't solely about clock gating. It depends on the SoC.

For example, if this sdhci device is being powered by a shared voltage
rail through a PM domain, perhaps it's even critical from an energy
efficiency point to manage runtime PM correctly.

By looking at the original commit, that sounds exactly what is
happening as the registers seem to lose their context.

>
> > >
> > > > "pltfm_host->clk", which means even if the controller supports auto
> > > > clock gating, gating/ungating the externally provided clock still
> > > > makes sense.
> > >
> > >        clock -----------  xenon IP
> > >       |___ rpm           |__ HW Auto clock gate
> > >
> > > Per my understanding, with rpm, both clock and IP is clock gated; while with
> > > Auto clock gate, the IP is clock gated. So the only difference is clock itself.
> > > Considering the gain(suspect we have power consumption gain, see below), the
> > > pay -- 56 LoCs and latency doesn't deserve gain.
> > >
> > > Even if considering from power consumption POV, sdhci_runtime_suspend_host(),
> > > sdhci_runtime_resume_host(), and the retune process could be more than the clock
> > > itself.
> >
> > Right.
> >
> > The re-tune may be costly, yes. However, whether the re-tune is
> > *really* needed actually varies depending on the sdhci variant and the
> > SoC. Additionally, re-tune isn't done for all types of (e)MMC/SD/SDIO
> > cards.
> >
> > I see a few options that you can explore.
> >
> > 1. There is no requirement to call sdhci_runtime_suspend|resume_host()
> > from sdhci-xenon's ->runtime_suspend|resume() callbacks - if that's
> > not explicitly needed. The point is, you can do other things there,
> > that suits your variant/SoC better.
>
> Yes, there's no requirement to call sdhci_runtime_suspend|resume_host().
> But simply removing the calls would break system suspend. How to handle
> this situation?

Alright. Then perhaps you need parts of what
sdhci_runtime_suspend|resume() is doing?

Anyway, as I said below. If you really have some good reasons from an
energy efficiency point of view, you can always disable runtime
suspend, along the lines of below.

To put it clear, to me the reasons you are providing to justify the
revert just doesn't make sense. Please try a different option.

>
> >
> > 2. Perhaps for embedded eMMCs, with a non-removable slot, the
> > re-tuning is costly. If you want to prevent the device from entering
> > runtime suspend for that slot, for example, just do an additional
> > pm_runtime_get_noresume() during ->probe().
> >
> > [...]
> >

Kind regards
Uffe