Qcom clock performance votes on mainline

Qcom clock performance votes on mainline

[Stephan Gerhold]

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?

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?

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

Thanks in advance!
Stephan

[1]: https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/clk/qcom/clock-gcc-8916.c?h=LA.BR.1.2.9.1-02310-8x16.0
[2]: https://lore.kernel.org/linux-arm-msm/20190129015547.213276-1-swboyd@chromium.org/
[3]: https://lore.kernel.org/linux-pm/20200826093328.88268-1-stephan@gerhold.net/
[4]: https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/clk/qcom/clock-gcc-8916.c?h=LA.BR.1.2.9.1-02310-8x16.0#n354

Re: Qcom clock performance votes on mainline

[Stephen Boyd]

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.

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

Re: Qcom clock performance votes on mainline

[Stephan Gerhold]

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.

I don't think it's possible at the moment, but actually Viresh mentioned
that use case (scaling a power domain only for some of the OPPs) when we
discussed where to enable the power domains listed in the OPP table [1].

Problem back then was that we didn't have a real example where this
would be needed. It seems like such an example exists now, so I will
discuss ways to implement that with Viresh.

I just wanted to be sure that adding the additional power domain to the
CPU OPP table is the right approach.

Thanks again!
Stephan

[1]: https://lore.kernel.org/linux-pm/20200828063511.y47ofywtu5qo57bq@vireshk-i7/

Re: Qcom clock performance votes on mainline

[Viresh Kumar]

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 ?

> I don't think it's possible at the moment, but actually Viresh mentioned
> that use case (scaling a power domain only for some of the OPPs) when we
> discussed where to enable the power domains listed in the OPP table [1].
> 
> Problem back then was that we didn't have a real example where this
> would be needed. It seems like such an example exists now, so I will
> discuss ways to implement that with Viresh.
> 
> I just wanted to be sure that adding the additional power domain to the
> CPU OPP table is the right approach.
> 
> Thanks again!
> Stephan
> 
> [1]: https://lore.kernel.org/linux-pm/20200828063511.y47ofywtu5qo57bq@vireshk-i7/

-- 
viresh

Re: Qcom clock performance votes on mainline

[Stephan Gerhold]

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/