| Message ID | cover.1657695140.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: qcom-hw: Move clocks to CPU node | expand |
On Wed, Jul 13, 2022 at 12:22:55PM +0530, Viresh Kumar wrote: > Hi, > > A recent patch series, targeting enhancements in the OPP core, ended up breaking > cpufreq on some of the Qualcomm platforms [1]. Necessary adjustments are made in > the OPP core, a bit hacky though, to get it working for now but it would be > better to solve the problem at hand in a cleaner way. And this patchset is an > attempt towards the same. > > cpufreq-hw is a hardware engine, which takes care of frequency > management for CPUs. The engine manages the clocks for CPU devices, but > it isn't the end consumer of the clocks, which are the CPUs in this > case. > > For this reason, it looks incorrect to keep the clock related properties > in the cpufreq-hw node. They should really be present at the end user, > i.e. the CPUs. > > The case was simple currently as all the devices, i.e. the CPUs, that > the engine manages share the same clock names. What if the clock names > are different for different CPUs or clusters ? How will keeping the > clock properties in the cpufreq-hw node work in that case ? > > This design creates further problems for frameworks like OPP, which > expects all such details (clocks) to be present in the end device node > itself, instead of another related node. > > This patchset moves the clock properties to the node that uses them instead, > i.e. the CPU nodes and makes necessary adjustments at other places. > > After this is applied, I can drop the unnecessary change from the OPP core, but > I wanted to discuss if this is a step in the right direction or not first and so > the RFC. > The clocks defined in the devicetree currently (CXO, GPLL) are the source clocks of the EPSS block (cpufreq-hw). And EPSS will supply clock and voltage through other blocks to the CPU domains. Even though the end consumer of the source clocks are the CPUs, those clocks are not directly reachign the CPUs but instead through some other blocks in EPSS. Initially I was temped to add cpufreq-hw as the clock provider and have it source clocks to the individual CPUs. This somehow models the clock topology also, but after having a discussion with Bjorn we concluded that it is best to leave it as it is. The main issue that Bjorn pointed out was the fact that the clocks coming out of EPSS are not exactly of the same frequency that was requested. EPSS will do its own logic to generate the clocks to the CPUs based on the input frequency vote and limits. Thanks, Mani > -- > Viresh > > [1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/ > > Viresh Kumar (4): > dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes > arm64: dts: qcom: Move clocks to CPU nodes > cpufreq: qcom-cpufreq-hw: Clocks are moved to CPU nodes > cpufreq: qcom-cpufreq-hw: Register config_clks helper > > .../bindings/cpufreq/cpufreq-qcom-hw.yaml | 31 ++++---- > arch/arm64/boot/dts/qcom/sc7180.dtsi | 19 ++++- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 18 ++++- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 19 ++++- > arch/arm64/boot/dts/qcom/sm6350.dtsi | 18 ++++- > arch/arm64/boot/dts/qcom/sm8150.dtsi | 19 ++++- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 18 ++++- > arch/arm64/boot/dts/qcom/sm8350.dtsi | 19 ++++- > arch/arm64/boot/dts/qcom/sm8450.dtsi | 18 ++++- > drivers/cpufreq/qcom-cpufreq-hw.c | 75 ++++++++++++++----- > 10 files changed, 199 insertions(+), 55 deletions(-) > > -- > 2.31.1.272.g89b43f80a514 >
On 15-07-22, 21:39, Manivannan Sadhasivam wrote: > The clocks defined in the devicetree currently (CXO, GPLL) are the source > clocks of the EPSS block (cpufreq-hw). And EPSS will supply clock and > voltage through other blocks to the CPU domains. Even though the end > consumer of the source clocks are the CPUs, those clocks are not > directly reachign the CPUs but instead through some other blocks in EPSS. Fair enough, o these clocks should be present in the cpufreq-hw node, as there were. > Initially I was temped to add cpufreq-hw as the clock provider and have > it source clocks to the individual CPUs. This somehow models the clock > topology also Right. > , but after having a discussion with Bjorn we concluded that > it is best to leave it as it is. > > The main issue that Bjorn pointed out was the fact that the clocks coming > out of EPSS are not exactly of the same frequency that was requested. > EPSS will do its own logic to generate the clocks to the CPUs based on > the input frequency vote and limits. The OPP tables, which are part of the CPU nodes, mentions clock rates. Are these values for the cxo/gpll clocks or the clock that reaches the CPUs? I believe the latter. The DT is not really complete if the CPU node mentions the frequency, but not the source clock. It works for you because you don't want to do clk_set_rate() in this case, but then it leaves other frameworks, like OPP, confused and rightly so. Normally, there is always a difference in what the OPP table contains as frequency value and what the hardware programs, mostly it is small though. It shouldn't prevent us from having the hierarchy clearly defined in the DT. Based on your description, I think it would be better to make cpufreq-hw a clock provider and CPUs the consumer of it. It would then allow the OPP core to not carry the hack to make it all work.
On Mon, Aug 01, 2022 at 08:07:56AM +0530, Viresh Kumar wrote: > On 18-07-22, 07:27, Viresh Kumar wrote: > > The OPP tables, which are part of the CPU nodes, mentions clock rates. > > Are these values for the cxo/gpll clocks or the clock that reaches the > > CPUs? I believe the latter. The DT is not really complete if the CPU > > node mentions the frequency, but not the source clock. It works for > > you because you don't want to do clk_set_rate() in this case, but then > > it leaves other frameworks, like OPP, confused and rightly so. > > > > Normally, there is always a difference in what the OPP table contains > > as frequency value and what the hardware programs, mostly it is small > > though. It shouldn't prevent us from having the hierarchy clearly > > defined in the DT. > > > > Based on your description, I think it would be better to make > > cpufreq-hw a clock provider and CPUs the consumer of it. It would then > > allow the OPP core to not carry the hack to make it all work. > > Bjorn / Mani, > > Can we please get this sorted out ? I don't want to carry an unnecessary hack in > the OPP core for this. > Conceptually, it sounds like a good idea to express the clock feeding the CPU clusters, which is controlled by the OSM/EPSS. But do you expect the OPP framework to actually do something with the clock, or just to ensure that the relationship is properly described? FWIW, the possible discrepancy between the requested frequency and the actual frequency comes from the fact that OSM/EPSS throttles the cluster frequency based on a number of different factors (thermal, voltages ...). This is reported back to the kernel using the thermal pressure interface. It would be quite interesting to see some investigation in how efficient the kernel is at making use of this feedback. Regards, Bjorn
On 29-08-22, 22:24, Bjorn Andersson wrote: > Conceptually, it sounds like a good idea to express the clock feeding > the CPU clusters, which is controlled by the OSM/EPSS. But do you > expect the OPP framework to actually do something with the clock, or > just to ensure that the relationship is properly described? No, the OPP core will never try to set the clock rate in your case, though it will do clk_get(). > FWIW, the possible discrepancy between the requested frequency and the > actual frequency comes from the fact that OSM/EPSS throttles the cluster > frequency based on a number of different factors (thermal, voltages > ...). > This is reported back to the kernel using the thermal pressure > interface. It would be quite interesting to see some investigation in > how efficient the kernel is at making use of this feedback.
On Tue, Aug 30, 2022 at 11:10:42AM +0530, Viresh Kumar wrote: > On 29-08-22, 22:24, Bjorn Andersson wrote: > > Conceptually, it sounds like a good idea to express the clock feeding > > the CPU clusters, which is controlled by the OSM/EPSS. But do you > > expect the OPP framework to actually do something with the clock, or > > just to ensure that the relationship is properly described? > > No, the OPP core will never try to set the clock rate in your case, > though it will do clk_get(). > Okay. Then I think it is a fair argument to make qcom-cpufreq-hw as the clock provider for CPUs. I will send the RFC soon. Thanks, Mani > > FWIW, the possible discrepancy between the requested frequency and the > > actual frequency comes from the fact that OSM/EPSS throttles the cluster > > frequency based on a number of different factors (thermal, voltages > > ...). > > This is reported back to the kernel using the thermal pressure > > interface. It would be quite interesting to see some investigation in > > how efficient the kernel is at making use of this feedback. > > -- > viresh
On 30-08-22, 11:50, Manivannan Sadhasivam wrote: > On Tue, Aug 30, 2022 at 11:10:42AM +0530, Viresh Kumar wrote: > > On 29-08-22, 22:24, Bjorn Andersson wrote: > > > Conceptually, it sounds like a good idea to express the clock feeding > > > the CPU clusters, which is controlled by the OSM/EPSS. But do you > > > expect the OPP framework to actually do something with the clock, or > > > just to ensure that the relationship is properly described? > > > > No, the OPP core will never try to set the clock rate in your case, > > though it will do clk_get(). > > > > Okay. Then I think it is a fair argument to make qcom-cpufreq-hw as the > clock provider for CPUs. > > I will send the RFC soon. Ping.