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

[Jisheng Zhang]

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.

> >  
> > > "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?

> 
> 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