Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support

From: Sudeep Holla <sudeep.holla@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	andersson@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	rafael@kernel.org, robh+dt@kernel.org, johan@kernel.org,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
Date: Fri, 18 Nov 2022 11:54:15 +0000	[thread overview]
Message-ID: <20221118115415.gr2crvyhs7rcfbir@bogus> (raw)
In-Reply-To: <20221118055730.yrzpuih3zfko5c2q@vireshk-i7>

On Fri, Nov 18, 2022 at 11:27:30AM +0530, Viresh Kumar wrote:
> On 17-11-22, 12:01, Sudeep Holla wrote:
> > Thanks for the link. Sorry I still don't get the complete picture. Who are
> > the consumers of these clock nodes if not cpufreq itself.
> > 
> > I am going to guess, so other device(like inter-connect) with phandle into
> > CPU device perhaps ? Also I assume it will have phandle to non-CPU device
> > and hence we need generic device clock solution. Sorry for the noise, but
> > I still find having both clocks and qcom,freq-domain property is quite
> > confusing but I am fine as I understand it bit better now.
> 
> Lemme try to explain what the initial problem was, because of which I suggested
> the DT to be fixed, even if no one is going to use it as a client.
> 
> The OPP core provides two features:
> 
> - Parsing of the OPP table and provide the data to the client.
> - Ability to switch the OPPs, i.e. configuring all resources.
> 
> qcom-cpufreq-hw driver uses both of these, but in a tricky way (like Tegra30).
> It used the OPP core to parse the data, along with "opp-hz" property and switch
> the OPPs by calling dev_pm_opp_set_opp(). But it doesn't want
> dev_pm_opp_set_opp() to change the clock rate, but configure everything else.
> 
> Now the OPP core needs to distinguish platforms for valid and invalid
> configurations, to make sure something isn't broken. For example a developer
> wants to change the OPP along with frequency and passes a valid OPP table. But
> forgets to set the clock entry in device's node. This is an error and the OPP
> core needs to report it. There can be more of such issues with different
> configurations.
> 
> Also, as Mani explained, if the OPP core is required to switch the OPPs, then it
> needs to know the initial frequency of the device to see if we are going up or
> down the frequency graph. And so it will do a clk_get_rate() if there is
> "opp-hz" available.
> 
> 
> What we did in case of Tegra30 (commit 1b195626) is provide a .config_clks
> helper, which does nothing. So the OPP core doesn't need to know if frequency is
> programmed or not.
> 
> The same can not be done for Qcom right now as the CPU node doesn't have the clk
> property though it has "opp-hz".
> 
> Weather we have a user in kernel (OS) or not, shouldn't decide how the DT looks
> like. The DT should clearly define what the hardware looks like, irrespective of
> the users. The CPU has a clock and it should be mentioned. If the OPP core
> chooses to use that information, then it is a fine expectation to have.
> 
> And so we are here. Most likely no one will ever do clk_set_rate() on this new
> clock, which is fine, though OPP core will likely do clk_get_rate() here.
> 
> Hope I was able to clarify few things here.
> 

Thanks a ton for such detailed explanation. Ulf was trying to integrate
genpd + performance domains with SCMI. That is the reason for my interest
in this topic. Sorry for all the trouble.

-- 
Regards,
Sudeep