Re: Qcom clock performance votes on mainline

[Stephan Gerhold <stephan@gerhold.net>]

On Fri, Sep 11, 2020 at 11:35:56AM +0530, Viresh Kumar wrote:
> On 11-09-20, 07:48, Stephan Gerhold wrote:
> > On Thu, Sep 10, 2020 at 02:28:01PM -0700, Stephen Boyd wrote:
> > > Quoting Stephan Gerhold (2020-09-10 09:26:10)
> > > > Hi Stephen, Hi Rajendra,
> > > > 
> > > > while working on some MSM8916 things I've been staring at the downstream
> > > > clock-gcc-8916.c [1] driver a bit. One thing that confuses me are the
> > > > voltage/performance state votes that are made for certain clocks within
> > > > the driver. Specifically lines like
> > > > 
> > > >     VDD_DIG_FMAX_MAP2(LOW, 32000000, NOMINAL, 64000000),
> > > > 
> > > > on certain clocks like UART, I2C or SPI. There does not seem to be
> > > > anything equivalent in the mainline clock driver at the moment.
> > > > 
> > > > As far as I understand from related discussions on mailing lists [2],
> > > > these performance votes are not supposed to be added to the clock
> > > > driver(s), but rather as required-opps within OPP tables of all the
> > > > consumers. Is that correct?
> > > 
> > > Yes.
> > > 
> > > > 
> > > > As a second question, I'm wondering about one particular case:
> > > > I've been trying to get CPR / all the CPU frequencies working on MSM8916.
> > > > For that, I already added performance state votes for VDDMX and CPR as
> > > > required-opps to the CPU OPP table.
> > > > 
> > > > After a recent discussion [3] with Viresh about where to enable power
> > > > domains managed by the OPP core, I've been looking at all the
> > > > performance state votes made in the downstream kernel again.
> > > > 
> > > > Actually, the A53 PLL used for the higher CPU frequencies also has such
> > > > voltage/performance state votes. The downstream driver declares the
> > > > clock like [4]:
> > > > 
> > > >                 .vdd_class = &vdd_sr2_pll,
> > > >                 .fmax = (unsigned long [VDD_SR2_PLL_NUM]) {
> > > >                         [VDD_SR2_PLL_SVS] = 1000000000,
> > > >                         [VDD_SR2_PLL_NOM] = 1900000000,
> > > >                 },
> > > >                 .num_fmax = VDD_SR2_PLL_NUM,
> > > > 
> > > > which ends up as votes for the VDDCX power domain.
> > > > 
> > > > Now I'm wondering: Where should I make these votes on mainline?
> > > > Should I add it as yet another required-opps to the CPU OPP table?
> > > 
> > > Sounds like the right approach.
> > > 
> > 
> > Thanks for the quick reply!
> > 
> > > > 
> > > > It would be a bit of a special case because these votes are only done
> > > > for the A53 PLL (which is only used for the higher CPU frequencies, not
> > > > the lower ones)...
> > > 
> > > Can that be put into the OPP table somehow for only the high
> > > frequencies? The OPP tables for CPUs sometimes cover the CPU PLL voltage
> > > requirements too so it doesn't seem like a totally bad idea.
> 
> Maybe we can allow the vote value to be 0 somehow ?
> 

Yep, I tried using an extra opp-level = <0> OPP for that in [1].
It does not work at the moment but maybe we can (somehow) add support
for that. I have some more thoughts about where/how to enable the power
domains from the OPP core in this case. The discussion probably fits
better to the other mail thread so I will reply there later.

Thanks!
Stephan

[1]: https://lore.kernel.org/linux-pm/20200828095706.GA1865@gerhold.net/