diff mbox series

[2/2] OPP: Disallow "opp-hz" property without a corresponding clk

Message ID c03c4f2b9d4dcc3264d1902606c6c5c464b4b043.1669012140.git.viresh.kumar@linaro.org (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series OPP: Disallow "opp-hz" property without a corresponding clk | expand

Commit Message

Viresh Kumar Nov. 21, 2022, 6:57 a.m. UTC
This removes the special code added by the commit 2083da24eb56 ("OPP:
Allow multiple clocks for a device"), to make it work for Qcom SoC.

In qcom-cpufreq-hw driver, we want to skip clk configuration that
happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
the "opp-hz" property so we can use the freq based OPP helpers.

The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
no longer need the special handling of the same in the OPP core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c    | 14 --------------
 drivers/opp/debugfs.c |  2 +-
 2 files changed, 1 insertion(+), 15 deletions(-)

Comments

Johan Hovold Nov. 21, 2022, 7:22 a.m. UTC | #1
On Mon, Nov 21, 2022 at 12:27:48PM +0530, Viresh Kumar wrote:
> This removes the special code added by the commit 2083da24eb56 ("OPP:
> Allow multiple clocks for a device"), to make it work for Qcom SoC.
> 
> In qcom-cpufreq-hw driver, we want to skip clk configuration that
> happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
> the "opp-hz" property so we can use the freq based OPP helpers.
> 
> The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
> no longer need the special handling of the same in the OPP core.

Didn't this affect also sc8280xp? Perhaps you can hold off with applying
this one for a bit until the needed devicetree changes are in linux-next
for all the affected platforms.

(It looks like Mani's series only updated sm8450 and I guess Bjorn
hasn't picked up that one up yet either.)

Johan
Viresh Kumar Nov. 21, 2022, 7:38 a.m. UTC | #2
On 21-11-22, 08:22, Johan Hovold wrote:
> On Mon, Nov 21, 2022 at 12:27:48PM +0530, Viresh Kumar wrote:
> > This removes the special code added by the commit 2083da24eb56 ("OPP:
> > Allow multiple clocks for a device"), to make it work for Qcom SoC.
> > 
> > In qcom-cpufreq-hw driver, we want to skip clk configuration that
> > happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
> > the "opp-hz" property so we can use the freq based OPP helpers.
> > 
> > The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
> > no longer need the special handling of the same in the OPP core.
> 
> Didn't this affect also sc8280xp?

I assumed Mani fixed all affected Qcom SoCs :(

> Perhaps you can hold off with applying
> this one for a bit until the needed devicetree changes are in linux-next
> for all the affected platforms.

Sure.

> (It looks like Mani's series only updated sm8450 and I guess Bjorn
> hasn't picked up that one up yet either.)

I applied that series today to my cpufreq/arm/linux-next, since it had
cpufreq updates too and these patches are rebased on top of them.
Manivannan Sadhasivam Nov. 21, 2022, 7:39 a.m. UTC | #3
On Mon, Nov 21, 2022 at 08:22:01AM +0100, Johan Hovold wrote:
> On Mon, Nov 21, 2022 at 12:27:48PM +0530, Viresh Kumar wrote:
> > This removes the special code added by the commit 2083da24eb56 ("OPP:
> > Allow multiple clocks for a device"), to make it work for Qcom SoC.
> > 
> > In qcom-cpufreq-hw driver, we want to skip clk configuration that
> > happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
> > the "opp-hz" property so we can use the freq based OPP helpers.
> > 
> > The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
> > no longer need the special handling of the same in the OPP core.
> 
> Didn't this affect also sc8280xp? Perhaps you can hold off with applying
> this one for a bit until the needed devicetree changes are in linux-next
> for all the affected platforms.
> 
> (It looks like Mani's series only updated sm8450 and I guess Bjorn
> hasn't picked up that one up yet either.)
> 

That's right. I have proposed to do the similar change to other SoCs as well
once the series was completely merged. I thought of doing so for 6.3.

Btw, there seems to be one more candidate:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sm8250.dtsi#n2537

Looks like newer SoCs that has the GMU within the GPU block doesn't have clock
property. This is because, GMU is the one supplying clocks to the GPU unlike the
old SoCs where the clocks used to come from GCC itself.

But we do have a GMU devicetree node, so it should be a matter of adding the
clock provider support as done for cpufreq and represent it in devicetree.

I'll ping Rob Clark and see how to get it done.

Thanks,
Mani

> Johan
Johan Hovold Nov. 21, 2022, 7:42 a.m. UTC | #4
On Mon, Nov 21, 2022 at 01:08:17PM +0530, Viresh Kumar wrote:
> On 21-11-22, 08:22, Johan Hovold wrote:
> > On Mon, Nov 21, 2022 at 12:27:48PM +0530, Viresh Kumar wrote:
> > > This removes the special code added by the commit 2083da24eb56 ("OPP:
> > > Allow multiple clocks for a device"), to make it work for Qcom SoC.
> > > 
> > > In qcom-cpufreq-hw driver, we want to skip clk configuration that
> > > happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
> > > the "opp-hz" property so we can use the freq based OPP helpers.
> > > 
> > > The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
> > > no longer need the special handling of the same in the OPP core.
> > 
> > Didn't this affect also sc8280xp?
> 
> I assumed Mani fixed all affected Qcom SoCs :(

I think he may have been waiting for his suggested solution to be
accepted before updating all the affected dtsi:s.
 
> > Perhaps you can hold off with applying
> > this one for a bit until the needed devicetree changes are in linux-next
> > for all the affected platforms.
> 
> Sure.

Thanks.

> > (It looks like Mani's series only updated sm8450 and I guess Bjorn
> > hasn't picked up that one up yet either.)
> 
> I applied that series today to my cpufreq/arm/linux-next, since it had
> cpufreq updates too and these patches are rebased on top of them.

Ok, I was expected the devicetree update to through Bjorn's tree as I
imagine there may be conflicts otherwise.

What was the intention here, Mani?

Johan
Manivannan Sadhasivam Nov. 21, 2022, 7:44 a.m. UTC | #5
On Mon, Nov 21, 2022 at 01:08:17PM +0530, Viresh Kumar wrote:
> On 21-11-22, 08:22, Johan Hovold wrote:
> > On Mon, Nov 21, 2022 at 12:27:48PM +0530, Viresh Kumar wrote:
> > > This removes the special code added by the commit 2083da24eb56 ("OPP:
> > > Allow multiple clocks for a device"), to make it work for Qcom SoC.
> > > 
> > > In qcom-cpufreq-hw driver, we want to skip clk configuration that
> > > happens via dev_pm_opp_set_opp(), but still want the OPP core to parse
> > > the "opp-hz" property so we can use the freq based OPP helpers.
> > > 
> > > The DT for Qcom SoCs is fixed now and contain a valid clk entry, and we
> > > no longer need the special handling of the same in the OPP core.
> > 
> > Didn't this affect also sc8280xp?
> 
> I assumed Mani fixed all affected Qcom SoCs :(
> 
> > Perhaps you can hold off with applying
> > this one for a bit until the needed devicetree changes are in linux-next
> > for all the affected platforms.
> 
> Sure.
> 
> > (It looks like Mani's series only updated sm8450 and I guess Bjorn
> > hasn't picked up that one up yet either.)
> 
> I applied that series today to my cpufreq/arm/linux-next, since it had
> cpufreq updates too and these patches are rebased on top of them.
> 

Ah, I thought you applied only cpufreq patches. DTS and bindings patches are
supposed to go through Bjorn's tree. Can you please drop them?

Thanks,
Mani

> -- 
> viresh
Viresh Kumar Nov. 21, 2022, 8:30 a.m. UTC | #6
On 21-11-22, 08:42, Johan Hovold wrote:
> Ok, I was expected the devicetree update to through Bjorn's tree as I
> imagine there may be conflicts otherwise.

Dropped the DT patch now.

Mani, I believe the first patch in this series is still required.
Without that, when the cpufreq driver calls dev_pm_opp_set_opp(), the
OPP core may end up calling clk_set_rate(), which shouldn't be done in
case of Qcom SoCs.

I am not sure though, how it will work with sc8280xp. Can you please
check ?
Manivannan Sadhasivam Nov. 22, 2022, 1:26 p.m. UTC | #7
On Mon, Nov 21, 2022 at 02:00:36PM +0530, Viresh Kumar wrote:
> On 21-11-22, 08:42, Johan Hovold wrote:
> > Ok, I was expected the devicetree update to through Bjorn's tree as I
> > imagine there may be conflicts otherwise.
> 
> Dropped the DT patch now.
> 
> Mani, I believe the first patch in this series is still required.
> Without that, when the cpufreq driver calls dev_pm_opp_set_opp(), the
> OPP core may end up calling clk_set_rate(), which shouldn't be done in
> case of Qcom SoCs.
> 

If there is no .set_rate() callback implemented by the clock provider, it won't
hurt, right?

Thanks,
Mani

> I am not sure though, how it will work with sc8280xp. Can you please
> check ?
> 
> -- 
> viresh
Viresh Kumar Nov. 24, 2022, 4:23 a.m. UTC | #8
On 22-11-22, 18:56, Manivannan Sadhasivam wrote:
> If there is no .set_rate() callback implemented by the clock provider, it won't
> hurt, right?

It shouldn't, I guess. Well, in that case, is the first patch even
required ? Maybe we should keep it, this makes clear that we won't
even call set_rate(), irrespective of the face that it is implemented
or not.

Also, the clk provider may not be part of this file later on, for
other SoC versions, and it is better in that case too.
Manivannan Sadhasivam Nov. 24, 2022, 5:24 a.m. UTC | #9
On Thu, Nov 24, 2022 at 09:53:04AM +0530, Viresh Kumar wrote:
> On 22-11-22, 18:56, Manivannan Sadhasivam wrote:
> > If there is no .set_rate() callback implemented by the clock provider, it won't
> > hurt, right?
> 
> It shouldn't, I guess. Well, in that case, is the first patch even
> required ? Maybe we should keep it, this makes clear that we won't
> even call set_rate(), irrespective of the face that it is implemented
> or not.
> 

I don't think that detail is required to be made explicit. If someone cares,
they can easlily find out by glancing through the OPP code.

So IMO, we don't need patch 1/2.

> Also, the clk provider may not be part of this file later on, for
> other SoC versions, and it is better in that case too.
> 

We cannot predict what the HW guys will come up with ;) But as said above, I
don't think it is necessary to to make it explicit.

Thanks,
Mani

> -- 
> viresh
Viresh Kumar Jan. 25, 2023, 4:21 a.m. UTC | #10
On 21-11-22, 13:09, Manivannan Sadhasivam wrote:
> That's right. I have proposed to do the similar change to other SoCs as well
> once the series was completely merged. I thought of doing so for 6.3.
> 
> Btw, there seems to be one more candidate:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sm8250.dtsi#n2537
> 
> Looks like newer SoCs that has the GMU within the GPU block doesn't have clock
> property. This is because, GMU is the one supplying clocks to the GPU unlike the
> old SoCs where the clocks used to come from GCC itself.
> 
> But we do have a GMU devicetree node, so it should be a matter of adding the
> clock provider support as done for cpufreq and represent it in devicetree.
> 
> I'll ping Rob Clark and see how to get it done.

Any update on this Mani ? I want to get the hack removed if possible.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e87567dbe99f..b7158d33c13d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1384,20 +1384,6 @@  static struct opp_table *_update_opp_table_clk(struct device *dev,
 	}
 
 	if (ret == -ENOENT) {
-		/*
-		 * There are few platforms which don't want the OPP core to
-		 * manage device's clock settings. In such cases neither the
-		 * platform provides the clks explicitly to us, nor the DT
-		 * contains a valid clk entry. The OPP nodes in DT may still
-		 * contain "opp-hz" property though, which we need to parse and
-		 * allow the platform to find an OPP based on freq later on.
-		 *
-		 * This is a simple solution to take care of such corner cases,
-		 * i.e. make the clk_count 1, which lets us allocate space for
-		 * frequency in opp->rates and also parse the entries in DT.
-		 */
-		opp_table->clk_count = 1;
-
 		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
 		return opp_table;
 	}
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 96a30a032c5f..402c507edac7 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -138,7 +138,7 @@  void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
 	 * - For some devices rate isn't available or there are multiple, use
 	 *   index instead for them.
 	 */
-	if (likely(opp_table->clk_count == 1 && opp->rates[0]))
+	if (likely(opp_table->clk_count == 1))
 		id = opp->rates[0];
 	else
 		id = _get_opp_count(opp_table);