[v5,0/3] qcom-cpufreq-hw: Add CPU clock provider support

Hello,

This series adds clock provider support to the Qcom CPUFreq driver for
supplying the clocks to the CPU cores in Qcom SoCs.

The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
clocks to the CPU cores. But this is not represented clearly in devicetree.
There is no clock coming out of the CPUFreq HW node to the CPU. This created
an issue [1] with the OPP core when a recent enhancement series was submitted.
Eventhough the issue got fixed in the OPP framework in the meantime, that's
not a proper solution and this series aims to fix it properly.

There was also an attempt made by Viresh [2] to fix the issue by moving the
clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
since those clocks belong to the CPUFreq HW node only.

The proposal here is to add clock provider support to the Qcom CPUFreq HW
driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
This correctly reflects the hardware implementation.

The clock provider is a simple one that just provides the frequency of the
clocks supplied to each frequency domain in the SoC using .recalc_rate()
callback. The frequency supplied by the driver will be the actual frequency
that comes out of the EPSS/OSM block after the DCVS operation. This frequency
is not same as what the CPUFreq framework has set but it is the one that gets
supplied to the CPUs after throttling by LMh.

This series has been tested on SM8450 based dev board with the OPP hack removed
and hence there is a DTS change only for that platform. Once this series gets
accepted, rest of the platform DTS can also be modified and finally the hack on
the OPP core can be dropped.

Thanks,
Mani

[1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/
[2] https://lore.kernel.org/lkml/20220801054255.GA12039@thinkpad/t/

Changes in v5:

* Switched to Hz unit for the CPU clocks

Changes in v4:

* Rebased on top of cpufreq/arm/linux-next branch

Changes in v3:

* Submitted the cpufreq driver cleanup patches as a separate series as
  suggested by Viresh
* Removed static keyword from clk_init_data declaration

Changes in v2:

* Moved the qcom_cpufreq_data allocation to probe
* Added single clock provider with multiple clks for each freq domain
* Moved soc_data to qcom_cpufreq struct
* Added Rob's review for binding

Manivannan Sadhasivam (3):
  dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
  arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
  cpufreq: qcom-hw: Add CPU clock provider support

 .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 12 ++++++
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |  9 ++++
 drivers/cpufreq/qcom-cpufreq-hw.c             | 43 +++++++++++++++++++
 3 files changed, 64 insertions(+)

On Wed, Nov 09, 2022 at 01:19:08PM +0530, Manivannan Sadhasivam wrote:

[...]

> qcom_cpufreq_hw_get() returns the frequency that got programmed by the cpufreq
> core. But that frequency is not necessarily the one that gets delivered to the
> CPU cores because the EPSS/OSM hardware block may vary the frequency after the
> DCVS operation.
> 
> So this frequency is the final one that gets delivered to the CPU cores.
> 

OK, thanks for the info. Just wondering if there is any issue making
qcom_cpufreq_hw_get() return this value instead of all these complexity.
I think the DT binding is too confusing as cpufreq-dt uses that to manage
DVFS which this one uses it.

If possible we should just make cpufreq_get(cpu) return the value you need
and use the same where ever you need. Sorry if I am missing something obvious
but I am struggling to see that.

On Wed, Nov 09, 2022 at 11:08:31AM +0000, Sudeep Holla wrote:
> On Wed, Nov 09, 2022 at 01:19:08PM +0530, Manivannan Sadhasivam wrote:
> 
> [...]
> 
> > qcom_cpufreq_hw_get() returns the frequency that got programmed by the cpufreq
> > core. But that frequency is not necessarily the one that gets delivered to the
> > CPU cores because the EPSS/OSM hardware block may vary the frequency after the
> > DCVS operation.
> > 
> > So this frequency is the final one that gets delivered to the CPU cores.
> > 
> 
> OK, thanks for the info. Just wondering if there is any issue making
> qcom_cpufreq_hw_get() return this value instead of all these complexity.
> I think the DT binding is too confusing as cpufreq-dt uses that to manage
> DVFS which this one uses it.
> 
> If possible we should just make cpufreq_get(cpu) return the value you need
> and use the same where ever you need. Sorry if I am missing something obvious
> but I am struggling to see that.
> 

I don't think using the final DCVS frequency would be applicable for cpufreq
core.

cpufreq core sets the desired frequency in the form of index using the
target_index() callback and the qcom-cpufreq-hw driver uses that index directly
to select the specific entry in the hardware LUT (Look Up Table).

Then with get() callback, the frequency will be returned based on the LUT index
read from the hardware. In this case, the frequency is going to be static
(i.e, what gets set by the cpufreq core will be the same). I believe this is
what the API also expects.

In the case of qcom_lmh_get_throttle_freq(), the frequency is going to be
dynamic (i.e changes with every internal DCVS operation). But this is exactly
what the OPP core expects with clk_get_rate() of CPU clock, so using
qcom_lmh_get_throttle_freq() makes sense there.

Thanks,
Mani

> -- 
> Regards,
> Sudeep

On Wed, Nov 09, 2022 at 06:05:26PM +0530, Manivannan Sadhasivam wrote:
> I don't think using the final DCVS frequency would be applicable for cpufreq
> core.
> 
> cpufreq core sets the desired frequency in the form of index using the
> target_index() callback and the qcom-cpufreq-hw driver uses that index directly
> to select the specific entry in the hardware LUT (Look Up Table).
> 
> Then with get() callback, the frequency will be returned based on the LUT index
> read from the hardware. In this case, the frequency is going to be static
> (i.e, what gets set by the cpufreq core will be the same). I believe this is
> what the API also expects.
>

I guessed so and hence thought of asking. Is the cpufreq_get() API expected
to return something close to what was set or is it expected to return the
real set h/w value if and when possible. I wanted to confirm if that is the
expectation from the cpufreq core or is it just the way qcom cpufreq-hw
driver(probably many others too) work today.

> In the case of qcom_lmh_get_throttle_freq(), the frequency is going to be
> dynamic (i.e changes with every internal DCVS operation). But this is exactly
> what the OPP core expects with clk_get_rate() of CPU clock, so using
> qcom_lmh_get_throttle_freq() makes sense there.
>

OK, the reason I ask is that IIRC the ACPI CPPC driver might get the exact
delivered frequency rather than something based on the set value, so it
shouldn't be a requirement but I may be wrong.

Viresh, thoughts ?

--
Regards,
Sudeep

On Mon, Nov 14, 2022 at 08:46:58AM +0530, Viresh Kumar wrote:
> On 09-11-22, 16:47, Sudeep Holla wrote:
> > Is the cpufreq_get() API expected
> > to return something close to what was set or is it expected to return the
> > real set h/w value if and when possible.
> 
> The real frequency the hardware is running at.
> 

Oh... In that case, qcom-cpufreq-hw driver is not returning the real frequency
but instead whatever set by the cpufreq core previously using target_index().

Should I fix it too in the next version of this series?

Thanks,
Mani

> -- 
> viresh

On 14-11-22, 11:55, Manivannan Sadhasivam wrote:
> On Mon, Nov 14, 2022 at 08:46:58AM +0530, Viresh Kumar wrote:
> > On 09-11-22, 16:47, Sudeep Holla wrote:
> > > Is the cpufreq_get() API expected
> > > to return something close to what was set or is it expected to return the
> > > real set h/w value if and when possible.
> > 
> > The real frequency the hardware is running at.
> > 
> 
> Oh... In that case, qcom-cpufreq-hw driver is not returning the real frequency
> but instead whatever set by the cpufreq core previously using target_index().
> 
> Should I fix it too in the next version of this series?

Yes.