[PATCH V2 00/15] cpufreq: mediatek: Cleanup and support MT8183 and MT8186

[PATCH V2 00/15] cpufreq: mediatek: Cleanup and support MT8183 and MT8186

[Rex-BC Chen]

Cpufreq is a DVFS driver used for power saving to scale the clock frequency
and supply the voltage for CPUs. This series do some cleanup for MediaTek
cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI
devfreq[3] which are supported in MT8183 and MT8186.

Changes for V2:
1. Drop the modification of transforming cpufreq-mediatek into yaml and
   only add the MediaTek CCI property for MediaTek cpufreq.
2. Split the original patches into several patches.

Reference series:
[1]: V1 of this series is present by Jia-Wei Chang.
     message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com

[2]: The MediaTek CCI devfreq driver is introduced in another series.
     message-id:20220307122513.11822-1-jia-wei.chang@mediatek.com

[3]: The MediaTek SVS driver is introduced in another series.
     message-id:20220221063939.14969-1-roger.lu@mediatek.com

Andrew-sh.Cheng (1):
  cpufreq: mediatek: Add opp notification for SVS support

Jia-Wei Chang (13):
  dt-bindings: cpufreq: mediatek: Add MediaTek CCI property
  cpufreq: mediatek: Use module_init and add module_exit
  cpufreq: mediatek: Cleanup variables and error handling in mtk_cpu_dvfs_info_init()
  cpufreq: mediatek: Remove unused headers
  cpufreq: mediatek: Enable clocks and regulators
  cpufreq: mediatek: Record previous target vproc value
  cpufreq: mediatek: Move voltage limits to platform data
  cpufreq: mediatek: Add .get function
  cpufreq: mediatek: Make sram regulator optional
  cpufreq: mediatek: Update logic of voltage_tracking()
  cpufreq: mediatek: Use maximum voltage in init stage
  cpufreq: mediatek: Link CCI device to CPU
  cpufreq: mediatek: Add support for MT8186

Rex-BC Chen (1):
  cpufreq: mediatek: Use device print to show logs

 .../bindings/cpufreq/cpufreq-mediatek.txt     |   4 +
 drivers/cpufreq/mediatek-cpufreq.c            | 609 ++++++++++++------
 2 files changed, 401 insertions(+), 212 deletions(-)

-- 
2.18.0

[PATCH V2 01/15] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

MediaTek Cache Coherent Interconnect (CCI) uses software devfreq module
for scaling clock frequency and adjust voltage.
The phandle could be linked between CPU and MediaTek CCI for some
MediaTek SoCs, like MT8183 and MT8186.
Therefore, we add this property in cpufreq-mediatek.txt.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 .../devicetree/bindings/cpufreq/cpufreq-mediatek.txt          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
index b8233ec91d3d..d1b3d430c25c 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
@@ -20,6 +20,10 @@ Optional properties:
 	       Vsram to fit SoC specific needs. When absent, the voltage scaling
 	       flow is handled by hardware, hence no software "voltage tracking" is
 	       needed.
+- cci: MediaTek Cache Coherent Interconnect uses software devfreq module for scaling
+       clock frequency and adjust voltage.
+       For details, please refer to
+       Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
 - #cooling-cells:
 	For details, please refer to
 	Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml
-- 
2.18.0

Re: [PATCH V2 01/15] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Krzysztof Kozlowski]

On 08/04/2022 06:58, Rex-BC Chen wrote:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> MediaTek Cache Coherent Interconnect (CCI) uses software devfreq module
> for scaling clock frequency and adjust voltage.
> The phandle could be linked between CPU and MediaTek CCI for some
> MediaTek SoCs, like MT8183 and MT8186.
> Therefore, we add this property in cpufreq-mediatek.txt.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-mediatek.txt          | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> index b8233ec91d3d..d1b3d430c25c 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
> @@ -20,6 +20,10 @@ Optional properties:
>  	       Vsram to fit SoC specific needs. When absent, the voltage scaling
>  	       flow is handled by hardware, hence no software "voltage tracking" is
>  	       needed.
> +- cci: MediaTek Cache Coherent Interconnect uses software devfreq module for scaling
> +       clock frequency and adjust voltage.

You need to describe the type. I am a bit confused whether this is a cci
(so cci-control-port property?) or an interconnect (so interconnect
property)... It does not look like a generic property, so you need
vendor prefix.

> +       For details, please refer to
> +       Documentation/devicetree/bindings/devfreq/mtk-cci.yaml

Such file does not exist.


Best regards,
Krzysztof

Re: [PATCH V2 01/15] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Rex-BC Chen]

On Fri, 2022-04-08 at 10:10 +0200, Krzysztof Kozlowski wrote:
> On 08/04/2022 06:58, Rex-BC Chen wrote:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > MediaTek Cache Coherent Interconnect (CCI) uses software devfreq
> > module
> > for scaling clock frequency and adjust voltage.
> > The phandle could be linked between CPU and MediaTek CCI for some
> > MediaTek SoCs, like MT8183 and MT8186.
> > Therefore, we add this property in cpufreq-mediatek.txt.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >  .../devicetree/bindings/cpufreq/cpufreq-mediatek.txt          | 4
> > ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.txt
> > index b8233ec91d3d..d1b3d430c25c 100644
> > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.txt
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > mediatek.txt
> > @@ -20,6 +20,10 @@ Optional properties:
> >  	       Vsram to fit SoC specific needs. When absent, the
> > voltage scaling
> >  	       flow is handled by hardware, hence no software "voltage
> > tracking" is
> >  	       needed.
> > +- cci: MediaTek Cache Coherent Interconnect uses software devfreq
> > module for scaling
> > +       clock frequency and adjust voltage.
> 
> You need to describe the type. I am a bit confused whether this is a
> cci
> (so cci-control-port property?) or an interconnect (so interconnect
> property)... It does not look like a generic property, so you need
> vendor prefix.

Hello Krzysztof,

Thanks for your review.

Yes, this cci is not arm's cci (cci-control-port property), and it's
mediatek's cci. I will revise this name to "mtk-cci" in next version.

> 
> > +       For details, please refer to
> > +       Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> 
> Such file does not exist.

This mediatek cci is still upstreaming in this patch:
message-id:20220408052150.22536-2-johnson.wang@mediatek.com

Do you have suggestion that I should put this reference?
Or I just remove it and describe the mediatek cci in detail?

BRs,
Rex
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH V2 01/15] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Krzysztof Kozlowski]

On 08/04/2022 12:24, Rex-BC Chen wrote:
>>
>> You need to describe the type. I am a bit confused whether this is a
>> cci
>> (so cci-control-port property?) or an interconnect (so interconnect
>> property)... It does not look like a generic property, so you need
>> vendor prefix.
> 
> Hello Krzysztof,
> 
> Thanks for your review.
> 
> Yes, this cci is not arm's cci (cci-control-port property), and it's
> mediatek's cci. I will revise this name to "mtk-cci" in next version.

Vendor is "mediatek" and comma comes after it. See devicetree spec
paragraph 2.3.1.

> 
>>
>>> +       For details, please refer to
>>> +       Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
>>
>> Such file does not exist.
> 
> This mediatek cci is still upstreaming in this patch:
> message-id:20220408052150.22536-2-johnson.wang@mediatek.com
> 
> Do you have suggestion that I should put this reference?

> Or I just remove it and describe the mediatek cci in detail?

It's ok, but you need to keep path/filename updated.


Best regards,
Krzysztof

Re: [PATCH V2 01/15] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Rex-BC Chen]

On Fri, 2022-04-08 at 13:49 +0200, Krzysztof Kozlowski wrote:
> On 08/04/2022 12:24, Rex-BC Chen wrote:
> > > 
> > > You need to describe the type. I am a bit confused whether this
> > > is a
> > > cci
> > > (so cci-control-port property?) or an interconnect (so
> > > interconnect
> > > property)... It does not look like a generic property, so you
> > > need
> > > vendor prefix.
> > 
> > Hello Krzysztof,
> > 
> > Thanks for your review.
> > 
> > Yes, this cci is not arm's cci (cci-control-port property), and
> > it's
> > mediatek's cci. I will revise this name to "mtk-cci" in next
> > version.
> 
> Vendor is "mediatek" and comma comes after it. See devicetree spec
> paragraph 2.3.1.
> 

Hello Krzysztof,

OK, I will revise as "mediatek,cci" in next version.

> > 
> > > 
> > > > +       For details, please refer to
> > > > +       Documentation/devicetree/bindings/devfreq/mtk-cci.yaml
> > > 
> > > Such file does not exist.
> > 
> > This mediatek cci is still upstreaming in this patch:
> > message-id:20220408052150.22536-2-johnson.wang@mediatek.com
> > 
> > Do you have suggestion that I should put this reference?
> > Or I just remove it and describe the mediatek cci in detail?
> 
> It's ok, but you need to keep path/filename updated.
> 
> 
> Best regards,
> Krzysztof

From your comment of cci series.
I will write is to be
"Documentation/devicetree/bindings/devfreq/mediatek,cci.yaml"

Thanks!

BRs,
Rex

[PATCH V2 02/15] cpufreq: mediatek: Use module_init and add module_exit

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

- Use module_init instead of device_initcall.
- Add a function for module_exit to unregister driver.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 866163883b48..9d7d9c8dc184 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -580,7 +580,13 @@ static int __init mtk_cpufreq_driver_init(void)
 
 	return 0;
 }
-device_initcall(mtk_cpufreq_driver_init);
+module_init(mtk_cpufreq_driver_init)
+
+static void __exit mtk_cpufreq_driver_exit(void)
+{
+	platform_driver_unregister(&mtk_cpufreq_platdrv);
+}
+module_exit(mtk_cpufreq_driver_exit)
 
 MODULE_DESCRIPTION("MediaTek CPUFreq driver");
 MODULE_AUTHOR("Pi-Cheng Chen <pi-cheng.chen@linaro.org>");
-- 
2.18.0

Re: [PATCH V2 02/15] cpufreq: mediatek: Use module_init and add module_exit

[AngeloGioacchino Del Regno]

Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> - Use module_init instead of device_initcall.
> - Add a function for module_exit to unregister driver.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 02/15] cpufreq: mediatek: Use module_init and add module_exit

[Viresh Kumar]

On 08-04-22, 15:36, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > - Use module_init instead of device_initcall.
> > - Add a function for module_exit to unregister driver.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied. Thanks.

-- 
viresh

[PATCH V2 03/15] cpufreq: mediatek: Cleanup variables and error handling in mtk_cpu_dvfs_info_init()

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

- Remove several unnecessary varaibles in mtk_cpu_dvfs_info_init().
- Unify error message format and use dev_err_probe() if possible.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 89 ++++++++++++------------------
 1 file changed, 34 insertions(+), 55 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 9d7d9c8dc184..e0545aca7e41 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -300,96 +300,75 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 {
 	struct device *cpu_dev;
-	struct regulator *proc_reg = ERR_PTR(-ENODEV);
-	struct regulator *sram_reg = ERR_PTR(-ENODEV);
-	struct clk *cpu_clk = ERR_PTR(-ENODEV);
-	struct clk *inter_clk = ERR_PTR(-ENODEV);
 	struct dev_pm_opp *opp;
 	unsigned long rate;
 	int ret;
 
 	cpu_dev = get_cpu_device(cpu);
 	if (!cpu_dev) {
-		pr_err("failed to get cpu%d device\n", cpu);
+		dev_err(cpu_dev, "failed to get cpu%d device\n", cpu);
 		return -ENODEV;
 	}
+	info->cpu_dev = cpu_dev;
 
-	cpu_clk = clk_get(cpu_dev, "cpu");
-	if (IS_ERR(cpu_clk)) {
-		if (PTR_ERR(cpu_clk) == -EPROBE_DEFER)
-			pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu);
-		else
-			pr_err("failed to get cpu clk for cpu%d\n", cpu);
-
-		ret = PTR_ERR(cpu_clk);
-		return ret;
+	info->cpu_clk = clk_get(cpu_dev, "cpu");
+	if (IS_ERR(info->cpu_clk)) {
+		ret = PTR_ERR(info->cpu_clk);
+		return dev_err_probe(cpu_dev, ret,
+				     "cpu%d: failed to get cpu clk\n", cpu);
 	}
 
-	inter_clk = clk_get(cpu_dev, "intermediate");
-	if (IS_ERR(inter_clk)) {
-		if (PTR_ERR(inter_clk) == -EPROBE_DEFER)
-			pr_warn("intermediate clk for cpu%d not ready, retry.\n",
-				cpu);
-		else
-			pr_err("failed to get intermediate clk for cpu%d\n",
-			       cpu);
-
-		ret = PTR_ERR(inter_clk);
+	info->inter_clk = clk_get(cpu_dev, "intermediate");
+	if (IS_ERR(info->inter_clk)) {
+		ret = PTR_ERR(info->inter_clk);
+		dev_err_probe(cpu_dev, ret,
+			      "cpu%d: failed to get intermediate clk\n", cpu);
 		goto out_free_resources;
 	}
 
-	proc_reg = regulator_get_optional(cpu_dev, "proc");
-	if (IS_ERR(proc_reg)) {
-		if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
-			pr_warn("proc regulator for cpu%d not ready, retry.\n",
-				cpu);
-		else
-			pr_err("failed to get proc regulator for cpu%d\n",
-			       cpu);
-
-		ret = PTR_ERR(proc_reg);
+	info->proc_reg = regulator_get_optional(cpu_dev, "proc");
+	if (IS_ERR(info->proc_reg)) {
+		ret = PTR_ERR(info->proc_reg);
+		dev_err_probe(cpu_dev, ret,
+			      "cpu%d: failed to get proc regulator\n", cpu);
 		goto out_free_resources;
 	}
 
 	/* Both presence and absence of sram regulator are valid cases. */
-	sram_reg = regulator_get_exclusive(cpu_dev, "sram");
+	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
+	if (IS_ERR(info->sram_reg))
+		info->sram_reg = NULL;
 
 	/* Get OPP-sharing information from "operating-points-v2" bindings */
 	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &info->cpus);
 	if (ret) {
-		pr_err("failed to get OPP-sharing information for cpu%d\n",
-		       cpu);
+		dev_err(cpu_dev,
+			"cpu%d: failed to get OPP-sharing information\n", cpu);
 		goto out_free_resources;
 	}
 
 	ret = dev_pm_opp_of_cpumask_add_table(&info->cpus);
 	if (ret) {
-		pr_warn("no OPP table for cpu%d\n", cpu);
+		dev_warn(cpu_dev, "cpu%d: no OPP table\n", cpu);
 		goto out_free_resources;
 	}
 
 	/* Search a safe voltage for intermediate frequency. */
-	rate = clk_get_rate(inter_clk);
+	rate = clk_get_rate(info->inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
-		pr_err("failed to get intermediate opp for cpu%d\n", cpu);
+		dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n", cpu);
 		ret = PTR_ERR(opp);
 		goto out_free_opp_table;
 	}
 	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
-	info->cpu_dev = cpu_dev;
-	info->proc_reg = proc_reg;
-	info->sram_reg = IS_ERR(sram_reg) ? NULL : sram_reg;
-	info->cpu_clk = cpu_clk;
-	info->inter_clk = inter_clk;
-
 	/*
 	 * If SRAM regulator is present, software "voltage tracking" is needed
 	 * for this CPU power domain.
 	 */
-	info->need_voltage_tracking = !IS_ERR(sram_reg);
+	info->need_voltage_tracking = (info->sram_reg != NULL);
 
 	return 0;
 
@@ -397,14 +376,14 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
 
 out_free_resources:
-	if (!IS_ERR(proc_reg))
-		regulator_put(proc_reg);
-	if (!IS_ERR(sram_reg))
-		regulator_put(sram_reg);
-	if (!IS_ERR(cpu_clk))
-		clk_put(cpu_clk);
-	if (!IS_ERR(inter_clk))
-		clk_put(inter_clk);
+	if (!IS_ERR(info->proc_reg))
+		regulator_put(info->proc_reg);
+	if (!IS_ERR(info->sram_reg))
+		regulator_put(info->sram_reg);
+	if (!IS_ERR(info->cpu_clk))
+		clk_put(info->cpu_clk);
+	if (!IS_ERR(info->inter_clk))
+		clk_put(info->inter_clk);
 
 	return ret;
 }
-- 
2.18.0

Re: [PATCH V2 03/15] cpufreq: mediatek: Cleanup variables and error handling in mtk_cpu_dvfs_info_init()

[AngeloGioacchino Del Regno]

Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> - Remove several unnecessary varaibles in mtk_cpu_dvfs_info_init().
> - Unify error message format and use dev_err_probe() if possible.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 03/15] cpufreq: mediatek: Cleanup variables and error handling in mtk_cpu_dvfs_info_init()

[Viresh Kumar]

On 08-04-22, 15:36, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > - Remove several unnecessary varaibles in mtk_cpu_dvfs_info_init().
> > - Unify error message format and use dev_err_probe() if possible.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied. Thanks.

-- 
viresh

[PATCH V2 04/15] cpufreq: mediatek: Remove unused headers

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Remove unused headers.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index e0545aca7e41..2a2859dbc5e0 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -13,8 +13,6 @@
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/regulator/consumer.h>
-#include <linux/slab.h>
-#include <linux/thermal.h>
 
 #define MIN_VOLT_SHIFT		(100000)
 #define MAX_VOLT_SHIFT		(200000)
-- 
2.18.0

Re: [PATCH V2 04/15] cpufreq: mediatek: Remove unused headers

[AngeloGioacchino Del Regno]

Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Remove unused headers.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 04/15] cpufreq: mediatek: Remove unused headers

[Viresh Kumar]

On 08-04-22, 15:36, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > Remove unused headers.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied. Thanks.

-- 
viresh

[PATCH V2 05/15] cpufreq: mediatek: Enable clocks and regulators

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

We need to enable regulators so that the max and min requested values will
be recorded.
The intermediate clock is not always enabled by CCF in different projects,
so we should enable it in the cpufreq driver.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 50 +++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 2a2859dbc5e0..dc4a87e68940 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -332,10 +332,23 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
+	ret = regulator_enable(info->proc_reg);
+	if (ret) {
+		dev_warn(cpu_dev, "cpu%d: failed to enable vproc\n", cpu);
+		goto out_free_resources;
+	}
+
 	/* Both presence and absence of sram regulator are valid cases. */
 	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
 	if (IS_ERR(info->sram_reg))
 		info->sram_reg = NULL;
+	else {
+		ret = regulator_enable(info->sram_reg);
+		if (ret) {
+			dev_warn(cpu_dev, "cpu%d: failed to enable vsram\n", cpu);
+			goto out_free_resources;
+		}
+	}
 
 	/* Get OPP-sharing information from "operating-points-v2" bindings */
 	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, &info->cpus);
@@ -351,13 +364,21 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
+	ret = clk_prepare_enable(info->cpu_clk);
+	if (ret)
+		goto out_free_opp_table;
+
+	ret = clk_prepare_enable(info->inter_clk);
+	if (ret)
+		goto out_disable_mux_clock;
+
 	/* Search a safe voltage for intermediate frequency. */
 	rate = clk_get_rate(info->inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
 		dev_err(cpu_dev, "cpu%d: failed to get intermediate opp\n", cpu);
 		ret = PTR_ERR(opp);
-		goto out_free_opp_table;
+		goto out_disable_inter_clock;
 	}
 	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
@@ -370,10 +391,21 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 
 	return 0;
 
+out_disable_inter_clock:
+	clk_disable_unprepare(info->inter_clk);
+
+out_disable_mux_clock:
+	clk_disable_unprepare(info->cpu_clk);
+
 out_free_opp_table:
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
 
 out_free_resources:
+	if (regulator_is_enabled(info->proc_reg))
+		regulator_disable(info->proc_reg);
+	if (info->sram_reg && regulator_is_enabled(info->sram_reg))
+		regulator_disable(info->sram_reg);
+
 	if (!IS_ERR(info->proc_reg))
 		regulator_put(info->proc_reg);
 	if (!IS_ERR(info->sram_reg))
@@ -388,14 +420,22 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 
 static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info)
 {
-	if (!IS_ERR(info->proc_reg))
+	if (!IS_ERR(info->proc_reg)) {
+		regulator_disable(info->proc_reg);
 		regulator_put(info->proc_reg);
-	if (!IS_ERR(info->sram_reg))
+	}
+	if (!IS_ERR(info->sram_reg)) {
+		regulator_disable(info->sram_reg);
 		regulator_put(info->sram_reg);
-	if (!IS_ERR(info->cpu_clk))
+	}
+	if (!IS_ERR(info->cpu_clk)) {
+		clk_disable_unprepare(info->cpu_clk);
 		clk_put(info->cpu_clk);
-	if (!IS_ERR(info->inter_clk))
+	}
+	if (!IS_ERR(info->inter_clk)) {
+		clk_disable_unprepare(info->inter_clk);
 		clk_put(info->inter_clk);
+	}
 
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
 }
-- 
2.18.0

Re: [PATCH V2 05/15] cpufreq: mediatek: Enable clocks and regulators

[AngeloGioacchino Del Regno]

Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> We need to enable regulators so that the max and min requested values will
> be recorded.
> The intermediate clock is not always enabled by CCF in different projects,
> so we should enable it in the cpufreq driver.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 05/15] cpufreq: mediatek: Enable clocks and regulators

[Viresh Kumar]

On 08-04-22, 15:36, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > We need to enable regulators so that the max and min requested values will
> > be recorded.
> > The intermediate clock is not always enabled by CCF in different projects,
> > so we should enable it in the cpufreq driver.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied. Thanks.

-- 
viresh

[PATCH V2 06/15] cpufreq: mediatek: Record previous target vproc value

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

We found the buck voltage may not be exactly the same with what we set
because CPU may share the same buck with other module.
Therefore, we need to record the previous desired value instead of reading
it from regulators.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index dc4a87e68940..472f4de29e5f 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info {
 	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
+	int old_vproc;
 };
 
 static LIST_HEAD(dvfs_info_list);
@@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 
 static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
 {
+	int ret;
+
 	if (info->need_voltage_tracking)
-		return mtk_cpufreq_voltage_tracking(info, vproc);
+		ret = mtk_cpufreq_voltage_tracking(info, vproc);
 	else
-		return regulator_set_voltage(info->proc_reg, vproc,
-					     vproc + VOLT_TOL);
+		ret = regulator_set_voltage(info->proc_reg, vproc,
+					    MAX_VOLT_LIMIT);
+	if (!ret)
+		info->old_vproc = vproc;
+
+	return ret;
 }
 
 static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
@@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 
 	inter_vproc = info->intermediate_voltage;
 
-	old_freq_hz = clk_get_rate(cpu_clk);
-	old_vproc = regulator_get_voltage(info->proc_reg);
-	if (old_vproc < 0) {
-		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
-		return old_vproc;
-	}
-
 	freq_hz = freq_table[index].frequency * 1000;
-
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
 	if (IS_ERR(opp)) {
 		pr_err("cpu%d: failed to find OPP for %ld\n",
@@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	vproc = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
+	old_freq_hz = clk_get_rate(cpu_clk);
+	old_vproc = info->old_vproc;
+	if (old_vproc == 0)
+		old_vproc = regulator_get_voltage(info->proc_reg);
+	if (old_vproc < 0) {
+		dev_err(cpu_dev, "%s: invalid Vproc value: %d\n",
+			__func__, old_vproc);
+		return old_vproc;
+	}
+
 	/*
 	 * If the new voltage or the intermediate voltage is higher than the
 	 * current voltage, scale up voltage first.
-- 
2.18.0

Re: [PATCH V2 06/15] cpufreq: mediatek: Record previous target vproc value

[AngeloGioacchino Del Regno]

Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> We found the buck voltage may not be exactly the same with what we set
> because CPU may share the same buck with other module.
> Therefore, we need to record the previous desired value instead of reading
> it from regulators.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++-----------
>   1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index dc4a87e68940..472f4de29e5f 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info {
>   	struct list_head list_head;
>   	int intermediate_voltage;
>   	bool need_voltage_tracking;
> +	int old_vproc;
>   };
>   
>   static LIST_HEAD(dvfs_info_list);
> @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>   
>   static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
>   {
> +	int ret;
> +
>   	if (info->need_voltage_tracking)
> -		return mtk_cpufreq_voltage_tracking(info, vproc);
> +		ret = mtk_cpufreq_voltage_tracking(info, vproc);
>   	else
> -		return regulator_set_voltage(info->proc_reg, vproc,
> -					     vproc + VOLT_TOL);
> +		ret = regulator_set_voltage(info->proc_reg, vproc,
> +					    MAX_VOLT_LIMIT);
> +	if (!ret)
> +		info->old_vproc = vproc;
> +
> +	return ret;
>   }
>   
>   static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   
>   	inter_vproc = info->intermediate_voltage;
>   
> -	old_freq_hz = clk_get_rate(cpu_clk);
> -	old_vproc = regulator_get_voltage(info->proc_reg);
> -	if (old_vproc < 0) {
> -		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> -		return old_vproc;
> -	}
> -
>   	freq_hz = freq_table[index].frequency * 1000;
> -
>   	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
>   	if (IS_ERR(opp)) {
>   		pr_err("cpu%d: failed to find OPP for %ld\n",
> @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   	vproc = dev_pm_opp_get_voltage(opp);
>   	dev_pm_opp_put(opp);
>   
> +	old_freq_hz = clk_get_rate(cpu_clk);
> +	old_vproc = info->old_vproc;
> +	if (old_vproc == 0)
> +		old_vproc = regulator_get_voltage(info->proc_reg);
> +	if (old_vproc < 0) {
> +		dev_err(cpu_dev, "%s: invalid Vproc value: %d\n",
> +			__func__, old_vproc);
> +		return old_vproc;
> +	}

 From my understandment, if this fails once, it fails forever!

info->old_vproc is set only if info->need_voltage_tracking is true, and only
in mtk_cpufreq_set_voltage(): this function is called only after the checks
that you've introduced there, and that's on previously stored values.
While this was fine in the previous version, because it was always calling
regulator_get_voltage(), here it's not.

I think that a good option here is to:

old_vproc = info->old_vproc;
if (old_vproc <= 0)
	old_vproc = regulator_get_voltage(info->proc_reg);
if (old_vproc < 0) {
	dev_err and return
}

...or, if this is not applicable, we should still find another way to not
let this driver to simply fail forever in case anything goes wrong.

Regards,
Angelo

Re: [PATCH V2 06/15] cpufreq: mediatek: Record previous target vproc value

[Rex-BC Chen]

On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:58, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > We found the buck voltage may not be exactly the same with what we
> > set
> > because CPU may share the same buck with other module.
> > Therefore, we need to record the previous desired value instead of
> > reading
> > it from regulators.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > ---
> >   drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++----
> > -------
> >   1 file changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index dc4a87e68940..472f4de29e5f 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info {
> >   	struct list_head list_head;
> >   	int intermediate_voltage;
> >   	bool need_voltage_tracking;
> > +	int old_vproc;
> >   };
> >   
> >   static LIST_HEAD(dvfs_info_list);
> > @@ -190,11 +191,17 @@ static int
> > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> >   
> >   static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info
> > *info, int vproc)
> >   {
> > +	int ret;
> > +
> >   	if (info->need_voltage_tracking)
> > -		return mtk_cpufreq_voltage_tracking(info, vproc);
> > +		ret = mtk_cpufreq_voltage_tracking(info, vproc);
> >   	else
> > -		return regulator_set_voltage(info->proc_reg, vproc,
> > -					     vproc + VOLT_TOL);
> > +		ret = regulator_set_voltage(info->proc_reg, vproc,
> > +					    MAX_VOLT_LIMIT);
> > +	if (!ret)
> > +		info->old_vproc = vproc;
> > +
> > +	return ret;
> >   }
> >   
> >   static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >   
> >   	inter_vproc = info->intermediate_voltage;
> >   
> > -	old_freq_hz = clk_get_rate(cpu_clk);
> > -	old_vproc = regulator_get_voltage(info->proc_reg);
> > -	if (old_vproc < 0) {
> > -		pr_err("%s: invalid Vproc value: %d\n", __func__,
> > old_vproc);
> > -		return old_vproc;
> > -	}
> > -
> >   	freq_hz = freq_table[index].frequency * 1000;
> > -
> >   	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
> >   	if (IS_ERR(opp)) {
> >   		pr_err("cpu%d: failed to find OPP for %ld\n",
> > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >   	vproc = dev_pm_opp_get_voltage(opp);
> >   	dev_pm_opp_put(opp);
> >   
> > +	old_freq_hz = clk_get_rate(cpu_clk);
> > +	old_vproc = info->old_vproc;
> > +	if (old_vproc == 0)
> > +		old_vproc = regulator_get_voltage(info->proc_reg);
> > +	if (old_vproc < 0) {
> > +		dev_err(cpu_dev, "%s: invalid Vproc value: %d\n",
> > +			__func__, old_vproc);
> > +		return old_vproc;
> > +	}
> 
>  From my understandment, if this fails once, it fails forever!
> 
> info->old_vproc is set only if info->need_voltage_tracking is true,
> and only
> in mtk_cpufreq_set_voltage(): this function is called only after the
> checks
> that you've introduced there, and that's on previously stored values.
> While this was fine in the previous version, because it was always
> calling
> regulator_get_voltage(), here it's not.
> 
> I think that a good option here is to:
> 
> old_vproc = info->old_vproc;
> if (old_vproc <= 0)
> 	old_vproc = regulator_get_voltage(info->proc_reg);
> if (old_vproc < 0) {
> 	dev_err and return
> }
> 
> ...or, if this is not applicable, we should still find another way to
> not
> let this driver to simply fail forever in case anything goes wrong.
> 
> Regards,
> Angelo

Hello Angelo,

Yes, your concern is right.
I will add this in next version.

if (old_vproc <= 0)
	old_vproc = regulator_get_voltage(info->proc_reg);
if (old_vproc < 0) {
	dev_err and return
}

BRs,
Rex

Re: [PATCH V2 06/15] cpufreq: mediatek: Record previous target vproc value

[Viresh Kumar]

On 08-04-22, 12:58, Rex-BC Chen wrote:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> We found the buck voltage may not be exactly the same with what we set
> because CPU may share the same buck with other module.
> Therefore, we need to record the previous desired value instead of reading
> it from regulators.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>  drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index dc4a87e68940..472f4de29e5f 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info {
>  	struct list_head list_head;
>  	int intermediate_voltage;
>  	bool need_voltage_tracking;
> +	int old_vproc;

I like prev_vproc better somehow, but it is up to you to name it :)

>  };
>  
>  static LIST_HEAD(dvfs_info_list);
> @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  
>  static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
>  {
> +	int ret;
> +
>  	if (info->need_voltage_tracking)
> -		return mtk_cpufreq_voltage_tracking(info, vproc);
> +		ret = mtk_cpufreq_voltage_tracking(info, vproc);
>  	else
> -		return regulator_set_voltage(info->proc_reg, vproc,
> -					     vproc + VOLT_TOL);
> +		ret = regulator_set_voltage(info->proc_reg, vproc,
> +					    MAX_VOLT_LIMIT);
> +	if (!ret)
> +		info->old_vproc = vproc;
> +
> +	return ret;
>  }
>  
>  static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>  
>  	inter_vproc = info->intermediate_voltage;
>  
> -	old_freq_hz = clk_get_rate(cpu_clk);
> -	old_vproc = regulator_get_voltage(info->proc_reg);
> -	if (old_vproc < 0) {
> -		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
> -		return old_vproc;
> -	}
> -

Why did you move it down from here? I think it was fine to error out
early if voltage isn't available.

>  	freq_hz = freq_table[index].frequency * 1000;
> -
>  	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
>  	if (IS_ERR(opp)) {
>  		pr_err("cpu%d: failed to find OPP for %ld\n",
> @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>  	vproc = dev_pm_opp_get_voltage(opp);
>  	dev_pm_opp_put(opp);
>  
> +	old_freq_hz = clk_get_rate(cpu_clk);
> +	old_vproc = info->old_vproc;
> +	if (old_vproc == 0)
> +		old_vproc = regulator_get_voltage(info->proc_reg);
> +	if (old_vproc < 0) {
> +		dev_err(cpu_dev, "%s: invalid Vproc value: %d\n",
> +			__func__, old_vproc);
> +		return old_vproc;
> +	}
> +
>  	/*
>  	 * If the new voltage or the intermediate voltage is higher than the
>  	 * current voltage, scale up voltage first.
> -- 
> 2.18.0

-- 
viresh

Re: [PATCH V2 06/15] cpufreq: mediatek: Record previous target vproc value

[Rex-BC Chen]

On Mon, 2022-04-11 at 08:56 +0530, Viresh Kumar wrote:
> On 08-04-22, 12:58, Rex-BC Chen wrote:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > We found the buck voltage may not be exactly the same with what we
> > set
> > because CPU may share the same buck with other module.
> > Therefore, we need to record the previous desired value instead of
> > reading
> > it from regulators.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > ---
> >  drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++-------
> > ----
> >  1 file changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index dc4a87e68940..472f4de29e5f 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info {
> >  	struct list_head list_head;
> >  	int intermediate_voltage;
> >  	bool need_voltage_tracking;
> > +	int old_vproc;
> 
> I like prev_vproc better somehow, but it is up to you to name it :)

Hello Viresh,

Thanks for your review.
I will modify this as prev_vproc in next version.

> 
> >  };
> >  
> >  static LIST_HEAD(dvfs_info_list);
> > @@ -190,11 +191,17 @@ static int
> > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> >  
> >  static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info,
> > int vproc)
> >  {
> > +	int ret;
> > +
> >  	if (info->need_voltage_tracking)
> > -		return mtk_cpufreq_voltage_tracking(info, vproc);
> > +		ret = mtk_cpufreq_voltage_tracking(info, vproc);
> >  	else
> > -		return regulator_set_voltage(info->proc_reg, vproc,
> > -					     vproc + VOLT_TOL);
> > +		ret = regulator_set_voltage(info->proc_reg, vproc,
> > +					    MAX_VOLT_LIMIT);
> > +	if (!ret)
> > +		info->old_vproc = vproc;
> > +
> > +	return ret;
> >  }
> >  
> >  static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >  
> >  	inter_vproc = info->intermediate_voltage;
> >  
> > -	old_freq_hz = clk_get_rate(cpu_clk);
> > -	old_vproc = regulator_get_voltage(info->proc_reg);
> > -	if (old_vproc < 0) {
> > -		pr_err("%s: invalid Vproc value: %d\n", __func__,
> > old_vproc);
> > -		return old_vproc;
> > -	}
> > -
> 
> Why did you move it down from here? I think it was fine to error out
> early if voltage isn't available.

I will move them to original position in next version.

Thanks!

BRs,
Rex

> 
> >  	freq_hz = freq_table[index].frequency * 1000;
> > -
> >  	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
> >  	if (IS_ERR(opp)) {
> >  		pr_err("cpu%d: failed to find OPP for %ld\n",
> > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >  	vproc = dev_pm_opp_get_voltage(opp);
> >  	dev_pm_opp_put(opp);
> >  
> > +	old_freq_hz = clk_get_rate(cpu_clk);
> > +	old_vproc = info->old_vproc;
> > +	if (old_vproc == 0)
> > +		old_vproc = regulator_get_voltage(info->proc_reg);
> > +	if (old_vproc < 0) {
> > +		dev_err(cpu_dev, "%s: invalid Vproc value: %d\n",
> > +			__func__, old_vproc);
> > +		return old_vproc;
> > +	}
> > +
> >  	/*
> >  	 * If the new voltage or the intermediate voltage is higher
> > than the
> >  	 * current voltage, scale up voltage first.
> > -- 
> > 2.18.0
> 
> 

[PATCH V2 08/15] cpufreq: mediatek: Move voltage limits to platform data

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Voltages and shifts are defined as macros originally.
There are different requirements of these values for each MediaTek SoCs.
Therefore, we add the platform data and move these values into it.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 90 ++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 1369da62780a..0be5609ee82e 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -10,15 +10,21 @@
 #include <linux/cpumask.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/regulator/consumer.h>
 
-#define MIN_VOLT_SHIFT		(100000)
-#define MAX_VOLT_SHIFT		(200000)
-#define MAX_VOLT_LIMIT		(1150000)
 #define VOLT_TOL		(10000)
 
+struct mtk_cpufreq_platform_data {
+	int min_volt_shift;
+	int max_volt_shift;
+	int proc_max_volt;
+	int sram_min_volt;
+	int sram_max_volt;
+};
+
 /*
  * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
  * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
@@ -46,8 +52,11 @@ struct mtk_cpu_dvfs_info {
 	struct notifier_block opp_nb;
 	int opp_cpu;
 	unsigned long opp_freq;
+	const struct mtk_cpufreq_platform_data *soc_data;
 };
 
+static struct platform_device *cpufreq_pdev;
+
 static LIST_HEAD(dvfs_info_list);
 
 static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
@@ -65,6 +74,7 @@ static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
+	const struct mtk_cpufreq_platform_data *soc_data = info->soc_data;
 	struct regulator *proc_reg = info->proc_reg;
 	struct regulator *sram_reg = info->sram_reg;
 	int old_vproc, old_vsram, new_vsram, vsram, vproc, ret;
@@ -75,7 +85,8 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		return old_vproc;
 	}
 	/* Vsram should not exceed the maximum allowed voltage of SoC. */
-	new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT);
+	new_vsram = min(new_vproc + soc_data->min_volt_shift,
+			soc_data->sram_max_volt);
 
 	if (old_vproc < new_vproc) {
 		/*
@@ -98,10 +109,11 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 				return old_vproc;
 			}
 
-			vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT);
+			vsram = min(new_vsram,
+				    old_vproc + soc_data->min_volt_shift);
 
-			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
-				vsram = MAX_VOLT_LIMIT;
+			if (vsram + VOLT_TOL >= soc_data->sram_max_volt) {
+				vsram = soc_data->sram_max_volt;
 
 				/*
 				 * If the target Vsram hits the maximum voltage,
@@ -119,7 +131,7 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 				ret = regulator_set_voltage(sram_reg, vsram,
 							    vsram + VOLT_TOL);
 
-				vproc = vsram - MIN_VOLT_SHIFT;
+				vproc = vsram - soc_data->min_volt_shift;
 			}
 			if (ret)
 				return ret;
@@ -153,7 +165,8 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 				return old_vsram;
 			}
 
-			vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT);
+			vproc = max(new_vproc,
+				    old_vsram - soc_data->max_volt_shift);
 			ret = regulator_set_voltage(proc_reg, vproc,
 						    vproc + VOLT_TOL);
 			if (ret)
@@ -162,10 +175,11 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 			if (vproc == new_vproc)
 				vsram = new_vsram;
 			else
-				vsram = max(new_vsram, vproc + MIN_VOLT_SHIFT);
+				vsram = max(new_vsram,
+					    vproc + soc_data->min_volt_shift);
 
-			if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) {
-				vsram = MAX_VOLT_LIMIT;
+			if (vsram + VOLT_TOL >= soc_data->sram_max_volt) {
+				vsram = soc_data->sram_max_volt;
 
 				/*
 				 * If the target Vsram hits the maximum voltage,
@@ -196,13 +210,14 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 
 static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
 {
+	const struct mtk_cpufreq_platform_data *soc_data = info->soc_data;
 	int ret;
 
 	if (info->need_voltage_tracking)
 		ret = mtk_cpufreq_voltage_tracking(info, vproc);
 	else
 		ret = regulator_set_voltage(info->proc_reg, vproc,
-					    MAX_VOLT_LIMIT);
+					    soc_data->proc_max_volt);
 	if (!ret)
 		info->old_vproc = vproc;
 
@@ -576,9 +591,17 @@ static struct cpufreq_driver mtk_cpufreq_driver = {
 
 static int mtk_cpufreq_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
 	struct mtk_cpu_dvfs_info *info, *tmp;
 	int cpu, ret;
 
+	match = dev_get_platdata(&pdev->dev);
+	if (!match || !match->data) {
+		dev_err(&pdev->dev,
+			"failed to get mtk cpufreq platform data\n");
+		return -ENODEV;
+	}
+
 	for_each_possible_cpu(cpu) {
 		info = mtk_cpu_dvfs_info_lookup(cpu);
 		if (info)
@@ -590,6 +613,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
 			goto release_dvfs_info_list;
 		}
 
+		info->soc_data = match->data;
 		ret = mtk_cpu_dvfs_info_init(info, cpu);
 		if (ret) {
 			dev_err(&pdev->dev,
@@ -625,20 +649,27 @@ static struct platform_driver mtk_cpufreq_platdrv = {
 	.probe		= mtk_cpufreq_probe,
 };
 
+static const struct mtk_cpufreq_platform_data mtk_platform_data = {
+	.min_volt_shift = 100000,
+	.max_volt_shift = 200000,
+	.proc_max_volt = 1150000,
+	.sram_min_volt = 0,
+	.sram_max_volt = 1150000,
+};
+
 /* List of machines supported by this driver */
 static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
-	{ .compatible = "mediatek,mt2701", },
-	{ .compatible = "mediatek,mt2712", },
-	{ .compatible = "mediatek,mt7622", },
-	{ .compatible = "mediatek,mt7623", },
-	{ .compatible = "mediatek,mt8167", },
-	{ .compatible = "mediatek,mt817x", },
-	{ .compatible = "mediatek,mt8173", },
-	{ .compatible = "mediatek,mt8176", },
-	{ .compatible = "mediatek,mt8183", },
-	{ .compatible = "mediatek,mt8365", },
-	{ .compatible = "mediatek,mt8516", },
-
+	{ .compatible = "mediatek,mt2701", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt2712", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt7622", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt7623", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8167", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt817x", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8173", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8176", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8183", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8365", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8516", .data = &mtk_platform_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mtk_cpufreq_machines);
@@ -647,7 +678,6 @@ static int __init mtk_cpufreq_driver_init(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
-	struct platform_device *pdev;
 	int err;
 
 	np = of_find_node_by_path("/");
@@ -671,11 +701,12 @@ static int __init mtk_cpufreq_driver_init(void)
 	 * and the device registration codes are put here to handle defer
 	 * probing.
 	 */
-	pdev = platform_device_register_simple("mtk-cpufreq", -1, NULL, 0);
-	if (IS_ERR(pdev)) {
+	cpufreq_pdev = platform_device_register_data(NULL, "mtk-cpufreq", -1,
+						     match, sizeof(*match));
+	if (IS_ERR(cpufreq_pdev)) {
 		pr_err("failed to register mtk-cpufreq platform device\n");
 		platform_driver_unregister(&mtk_cpufreq_platdrv);
-		return PTR_ERR(pdev);
+		return PTR_ERR(cpufreq_pdev);
 	}
 
 	return 0;
@@ -684,6 +715,7 @@ module_init(mtk_cpufreq_driver_init)
 
 static void __exit mtk_cpufreq_driver_exit(void)
 {
+	platform_device_unregister(cpufreq_pdev);
 	platform_driver_unregister(&mtk_cpufreq_platdrv);
 }
 module_exit(mtk_cpufreq_driver_exit)
-- 
2.18.0

Re: [PATCH V2 08/15] cpufreq: mediatek: Move voltage limits to platform data

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Voltages and shifts are defined as macros originally.
> There are different requirements of these values for each MediaTek SoCs.
> Therefore, we add the platform data and move these values into it.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 90 ++++++++++++++++++++----------
>   1 file changed, 61 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 1369da62780a..0be5609ee82e 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c

..snip..

> @@ -625,20 +649,27 @@ static struct platform_driver mtk_cpufreq_platdrv = {
>   	.probe		= mtk_cpufreq_probe,
>   };
>   
> +static const struct mtk_cpufreq_platform_data mtk_platform_data = {

I would prefer if you name this after the oldest SoC that is supported in this
driver, or after the IP version.

Example: if the oldest one is mt7622, then this should be mt7622_platform_data;
          if newer SoCs have got a different cpu frequency control IP, then we
          should have here something like
          mtk_v1_platform_data, mtk_v1_3_platform_data.

Please note that I didn't check release dates, so my reference to mt7622 is
purely casual.

Regards,
Angelo

Re: [PATCH V2 08/15] cpufreq: mediatek: Move voltage limits to platform data

[Rex-BC Chen]

On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > Voltages and shifts are defined as macros originally.
> > There are different requirements of these values for each MediaTek
> > SoCs.
> > Therefore, we add the platform data and move these values into it.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >   drivers/cpufreq/mediatek-cpufreq.c | 90 ++++++++++++++++++++-----
> > -----
> >   1 file changed, 61 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index 1369da62780a..0be5609ee82e 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> 
> ..snip..
> 
> > @@ -625,20 +649,27 @@ static struct platform_driver
> > mtk_cpufreq_platdrv = {
> >   	.probe		= mtk_cpufreq_probe,
> >   };
> >   
> > +static const struct mtk_cpufreq_platform_data mtk_platform_data =
> > {
> 
> I would prefer if you name this after the oldest SoC that is
> supported in this
> driver, or after the IP version.
> 
> Example: if the oldest one is mt7622, then this should be
> mt7622_platform_data;
>           if newer SoCs have got a different cpu frequency control
> IP, then we
>           should have here something like
>           mtk_v1_platform_data, mtk_v1_3_platform_data.
> 
> Please note that I didn't check release dates, so my reference to
> mt7622 is
> purely casual.
> 
> Regards,
> Angelo

Hello Angelo,

Thanks for your review.
I will rename it as mt2701_platform_data.

BRs,
Rex

[PATCH V2 09/15] cpufreq: mediatek: Add .get function

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

We want to get opp frequency via opp table. Therefore, we add the function
"mtk_cpufreq_get()" to do this.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 0be5609ee82e..9e9bce0ff235 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -71,6 +71,15 @@ static struct mtk_cpu_dvfs_info *mtk_cpu_dvfs_info_lookup(int cpu)
 	return NULL;
 }
 
+static unsigned int mtk_cpufreq_get(unsigned int cpu)
+{
+	struct mtk_cpu_dvfs_info *info;
+
+	info = mtk_cpu_dvfs_info_lookup(cpu);
+
+	return !info ? 0 : (info->opp_freq / 1000);
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -581,7 +590,7 @@ static struct cpufreq_driver mtk_cpufreq_driver = {
 		 CPUFREQ_IS_COOLING_DEV,
 	.verify = cpufreq_generic_frequency_table_verify,
 	.target_index = mtk_cpufreq_set_target,
-	.get = cpufreq_generic_get,
+	.get = mtk_cpufreq_get,
 	.init = mtk_cpufreq_init,
 	.exit = mtk_cpufreq_exit,
 	.register_em = cpufreq_register_em_with_opp,
-- 
2.18.0

[PATCH V2 10/15] cpufreq: mediatek: Make sram regulator optional

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

For some MediaTek SoCs, like MT8186, it's possible that the sram regulator
is shared between CPU and CCI.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 9e9bce0ff235..8f688d47e64b 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -435,7 +435,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	}
 
 	/* Both presence and absence of sram regulator are valid cases. */
-	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
+	info->sram_reg = regulator_get_optional(cpu_dev, "sram");
 	if (IS_ERR(info->sram_reg))
 		info->sram_reg = NULL;
 	else {
-- 
2.18.0

Re: [PATCH V2 10/15] cpufreq: mediatek: Make sram regulator optional

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> For some MediaTek SoCs, like MT8186, it's possible that the sram regulator
> is shared between CPU and CCI.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 10/15] cpufreq: mediatek: Make sram regulator optional

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>
> For some MediaTek SoCs, like MT8186, it's possible that the sram regulator
> is shared between CPU and CCI.
>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

nit: missing your sign-off.

> ---
>  drivers/cpufreq/mediatek-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 9e9bce0ff235..8f688d47e64b 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -435,7 +435,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  	}
>  
>  	/* Both presence and absence of sram regulator are valid cases. */
> -	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
> +	info->sram_reg = regulator_get_optional(cpu_dev, "sram");

The changelog says that this regulator may be shared with CCI, so I
understand it's no longer exclusive.  But here you make it optional,
which should be explained in the changelog.  If it's not actually
optional, then it should just be normal "get".

Kevin

Re: [PATCH V2 10/15] cpufreq: mediatek: Make sram regulator optional

[Rex-BC Chen]

On Fri, 2022-04-08 at 13:32 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > For some MediaTek SoCs, like MT8186, it's possible that the sram
> > regulator
> > is shared between CPU and CCI.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> nit: missing your sign-off.
> 
> > ---
> >  drivers/cpufreq/mediatek-cpufreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index 9e9bce0ff235..8f688d47e64b 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -435,7 +435,7 @@ static int mtk_cpu_dvfs_info_init(struct
> > mtk_cpu_dvfs_info *info, int cpu)
> >  	}
> >  
> >  	/* Both presence and absence of sram regulator are valid cases.
> > */
> > -	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
> > +	info->sram_reg = regulator_get_optional(cpu_dev, "sram");
> 
> The changelog says that this regulator may be shared with CCI, so I
> understand it's no longer exclusive.  But here you make it optional,
> which should be explained in the changelog.  If it's not actually
> optional, then it should just be normal "get".
> 
> Kevin

Hello Kevin,

Since cpufreq and cci devfreq might share the same sram regulator in
MediaTek SoC, it is no longer exclusive as you mentioned.

The reason to use regulator_get_optional is we hope regulator framework
can return error for error handling rather than a dummy handler from
regulator_get api.

I will add this to commit message in next version.

Thanks.

BRs,
Rex

Re: [PATCH V2 10/15] cpufreq: mediatek: Make sram regulator optional

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Fri, 2022-04-08 at 13:32 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> > 
>> > For some MediaTek SoCs, like MT8186, it's possible that the sram
>> > regulator
>> > is shared between CPU and CCI.
>> > 
>> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> 
>> nit: missing your sign-off.
>> 
>> > ---
>> >  drivers/cpufreq/mediatek-cpufreq.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
>> > b/drivers/cpufreq/mediatek-cpufreq.c
>> > index 9e9bce0ff235..8f688d47e64b 100644
>> > --- a/drivers/cpufreq/mediatek-cpufreq.c
>> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
>> > @@ -435,7 +435,7 @@ static int mtk_cpu_dvfs_info_init(struct
>> > mtk_cpu_dvfs_info *info, int cpu)
>> >  	}
>> >  
>> >  	/* Both presence and absence of sram regulator are valid cases.
>> > */
>> > -	info->sram_reg = regulator_get_exclusive(cpu_dev, "sram");
>> > +	info->sram_reg = regulator_get_optional(cpu_dev, "sram");
>> 
>> The changelog says that this regulator may be shared with CCI, so I
>> understand it's no longer exclusive.  But here you make it optional,
>> which should be explained in the changelog.  If it's not actually
>> optional, then it should just be normal "get".
>> 
>> Kevin
>
> Hello Kevin,
>
> Since cpufreq and cci devfreq might share the same sram regulator in
> MediaTek SoC, it is no longer exclusive as you mentioned.
>
> The reason to use regulator_get_optional is we hope regulator framework
> can return error for error handling rather than a dummy handler from
> regulator_get api.
>
> I will add this to commit message in next version.

OK, sounds good.

Thanks,

Kevin

[PATCH V2 11/15] cpufreq: mediatek: Update logic of voltage_tracking()

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

- Remove VOLT_TOL because CCI may share the same sram and vproc
  regulators with CPU. Therefore, set the max voltage in
  regulator_set_voltage() to the proc{sram}_max_volt.

- Move comparison of new and old voltages to
  mtk_cpufreq_voltage_tracking().

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 130 ++++++++---------------------
 1 file changed, 34 insertions(+), 96 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 8f688d47e64b..e69b16a6541e 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -8,6 +8,7 @@
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -15,8 +16,6 @@
 #include <linux/pm_opp.h>
 #include <linux/regulator/consumer.h>
 
-#define VOLT_TOL		(10000)
-
 struct mtk_cpufreq_platform_data {
 	int min_volt_shift;
 	int max_volt_shift;
@@ -93,91 +92,44 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
 		return old_vproc;
 	}
-	/* Vsram should not exceed the maximum allowed voltage of SoC. */
-	new_vsram = min(new_vproc + soc_data->min_volt_shift,
-			soc_data->sram_max_volt);
-
-	if (old_vproc < new_vproc) {
-		/*
-		 * When scaling up voltages, Vsram and Vproc scale up step
-		 * by step. At each step, set Vsram to (Vproc + 200mV) first,
-		 * then set Vproc to (Vsram - 100mV).
-		 * Keep doing it until Vsram and Vproc hit target voltages.
-		 */
-		do {
-			old_vsram = regulator_get_voltage(sram_reg);
-			if (old_vsram < 0) {
-				pr_err("%s: invalid Vsram value: %d\n",
-				       __func__, old_vsram);
-				return old_vsram;
-			}
-			old_vproc = regulator_get_voltage(proc_reg);
-			if (old_vproc < 0) {
-				pr_err("%s: invalid Vproc value: %d\n",
-				       __func__, old_vproc);
-				return old_vproc;
-			}
 
-			vsram = min(new_vsram,
-				    old_vproc + soc_data->min_volt_shift);
-
-			if (vsram + VOLT_TOL >= soc_data->sram_max_volt) {
-				vsram = soc_data->sram_max_volt;
+	old_vsram = regulator_get_voltage(sram_reg);
+	if (old_vsram < 0) {
+		pr_err("%s: invalid Vsram value: %d\n", __func__, old_vsram);
+		return old_vsram;
+	}
 
-				/*
-				 * If the target Vsram hits the maximum voltage,
-				 * try to set the exact voltage value first.
-				 */
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram);
-				if (ret)
-					ret = regulator_set_voltage(sram_reg,
-							vsram - VOLT_TOL,
-							vsram);
+	new_vsram = clamp(new_vproc + soc_data->min_volt_shift,
+			  soc_data->sram_min_volt, soc_data->sram_max_volt);
 
-				vproc = new_vproc;
-			} else {
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram + VOLT_TOL);
+	do {
+		if (old_vproc <= new_vproc) {
+			vsram = clamp(old_vproc + soc_data->max_volt_shift,
+				      soc_data->sram_min_volt, new_vsram);
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
 
-				vproc = vsram - soc_data->min_volt_shift;
-			}
 			if (ret)
 				return ret;
 
+			if (vsram == soc_data->sram_max_volt ||
+			    new_vsram == soc_data->sram_min_volt)
+				vproc = new_vproc;
+			else
+				vproc = vsram - soc_data->min_volt_shift;
+
 			ret = regulator_set_voltage(proc_reg, vproc,
-						    vproc + VOLT_TOL);
+						    soc_data->proc_max_volt);
 			if (ret) {
 				regulator_set_voltage(sram_reg, old_vsram,
-						      old_vsram);
+						      soc_data->sram_max_volt);
 				return ret;
 			}
-		} while (vproc < new_vproc || vsram < new_vsram);
-	} else if (old_vproc > new_vproc) {
-		/*
-		 * When scaling down voltages, Vsram and Vproc scale down step
-		 * by step. At each step, set Vproc to (Vsram - 200mV) first,
-		 * then set Vproc to (Vproc + 100mV).
-		 * Keep doing it until Vsram and Vproc hit target voltages.
-		 */
-		do {
-			old_vproc = regulator_get_voltage(proc_reg);
-			if (old_vproc < 0) {
-				pr_err("%s: invalid Vproc value: %d\n",
-				       __func__, old_vproc);
-				return old_vproc;
-			}
-			old_vsram = regulator_get_voltage(sram_reg);
-			if (old_vsram < 0) {
-				pr_err("%s: invalid Vsram value: %d\n",
-				       __func__, old_vsram);
-				return old_vsram;
-			}
-
+		} else if (old_vproc > new_vproc) {
 			vproc = max(new_vproc,
 				    old_vsram - soc_data->max_volt_shift);
 			ret = regulator_set_voltage(proc_reg, vproc,
-						    vproc + VOLT_TOL);
+						    soc_data->proc_max_volt);
 			if (ret)
 				return ret;
 
@@ -187,32 +139,18 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 				vsram = max(new_vsram,
 					    vproc + soc_data->min_volt_shift);
 
-			if (vsram + VOLT_TOL >= soc_data->sram_max_volt) {
-				vsram = soc_data->sram_max_volt;
-
-				/*
-				 * If the target Vsram hits the maximum voltage,
-				 * try to set the exact voltage value first.
-				 */
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram);
-				if (ret)
-					ret = regulator_set_voltage(sram_reg,
-							vsram - VOLT_TOL,
-							vsram);
-			} else {
-				ret = regulator_set_voltage(sram_reg, vsram,
-							    vsram + VOLT_TOL);
-			}
-
+			ret = regulator_set_voltage(sram_reg, vsram,
+						    soc_data->sram_max_volt);
 			if (ret) {
 				regulator_set_voltage(proc_reg, old_vproc,
-						      old_vproc);
+						      soc_data->proc_max_volt);
 				return ret;
 			}
-		} while (vproc > new_vproc + VOLT_TOL ||
-			 vsram > new_vsram + VOLT_TOL);
-	}
+		}
+
+		old_vproc = vproc;
+		old_vsram = vsram;
+	} while (vproc != new_vproc || vsram != new_vsram);
 
 	return 0;
 }
@@ -272,8 +210,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	 * If the new voltage or the intermediate voltage is higher than the
 	 * current voltage, scale up voltage first.
 	 */
-	target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc;
-	if (old_vproc < target_vproc) {
+	target_vproc = max(inter_vproc, vproc);
+	if (old_vproc <= target_vproc) {
 		ret = mtk_cpufreq_set_voltage(info, target_vproc);
 		if (ret) {
 			pr_err("cpu%d: failed to scale up voltage!\n",
-- 
2.18.0

Re: [PATCH V2 11/15] cpufreq: mediatek: Update logic of voltage_tracking()

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>
> - Remove VOLT_TOL because CCI may share the same sram and vproc
>   regulators with CPU. Therefore, set the max voltage in
>   regulator_set_voltage() to the proc{sram}_max_volt.

This could you a bit more detailed explanation.  Why does VOLT_TOL get
in the way when regulators are shared between CPU & CCI?

> - Move comparison of new and old voltages to
>   mtk_cpufreq_voltage_tracking().

Why?  And how is this related to the above change?  Seems to me that it
belongs in a separate patch.

Kevin

Re: [PATCH V2 11/15] cpufreq: mediatek: Update logic of voltage_tracking()

[Rex-BC Chen]

On Fri, 2022-04-08 at 14:08 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > - Remove VOLT_TOL because CCI may share the same sram and vproc
> >   regulators with CPU. Therefore, set the max voltage in
> >   regulator_set_voltage() to the proc{sram}_max_volt.
> 
> This could you a bit more detailed explanation.  Why does VOLT_TOL
> get
> in the way when regulators are shared between CPU & CCI?

Hello Kevin,

Here we use 'sram_min_volt' and 'sram_max_volt' to determine the
voltage boundary of sram regulator.
And use (sram_min_volt - min_volt_shift) and 'proc_max_volt' to
determine the voltage boundary of vproc regulator.
We use them as platform data to replace VOLT_TOL (voltage tolerance)
when determing the voltage boundary and invoking regulator_set_voltage.

I will add this to commit message in next version.

> 
> > - Move comparison of new and old voltages to
> >   mtk_cpufreq_voltage_tracking().
> 
> Why?  And how is this related to the above change?  Seems to me that
> it
> belongs in a separate patch.
> 
> Kevin

I think there are some mistake for this.
I will remove this commit message in next version.

BRs,
Rex

[PATCH V2 12/15] cpufreq: mediatek: Use maximum voltage in init stage

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Two or more clients may use the same regulator, and it could cause the
issue of high-freqeuncy-low-voltage.
To prevent this, we use maximum voltage in mtk_cpu_dvfs_info_init().

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index e69b16a6541e..b08ab7c14818 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -333,7 +333,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 {
 	struct device *cpu_dev;
 	struct dev_pm_opp *opp;
-	unsigned long rate;
+	unsigned long rate, opp_volt;
 	int ret;
 
 	cpu_dev = get_cpu_device(cpu);
@@ -417,6 +417,24 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
+	/* Use highest opp voltage in the init stage */
+	rate = U32_MAX;
+	opp = dev_pm_opp_find_freq_floor(info->cpu_dev, &rate);
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		dev_err(cpu_dev, "cpu%d: failed to get opp\n", info->opp_cpu);
+		goto out_disable_inter_clock;
+	}
+
+	opp_volt = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+	ret = mtk_cpufreq_set_voltage(info, opp_volt);
+	if (ret) {
+		dev_err(cpu_dev, "cpu%d: failed to scale to highest voltage %lu in proc_reg\n",
+			info->opp_cpu, opp_volt);
+		goto out_disable_inter_clock;
+	}
+
 	info->opp_cpu = cpu;
 	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
 	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
-- 
2.18.0

Re: [PATCH V2 12/15] cpufreq: mediatek: Use maximum voltage in init stage

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Two or more clients may use the same regulator, and it could cause the
> issue of high-freqeuncy-low-voltage.
> To prevent this, we use maximum voltage in mtk_cpu_dvfs_info_init().
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

Is this happening for proc-supply (proc_reg)?
...because it looks like it is, so you should send this commit separately
and with an appropriate Fixes: tag.

Re: [PATCH V2 12/15] cpufreq: mediatek: Use maximum voltage in init stage

[Rex-BC Chen]

On Fri, 2022-04-08 at 15:37 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > Two or more clients may use the same regulator, and it could cause
> > the
> > issue of high-freqeuncy-low-voltage.
> > To prevent this, we use maximum voltage in
> > mtk_cpu_dvfs_info_init().
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Is this happening for proc-supply (proc_reg)?
> ...because it looks like it is, so you should send this commit
> separately
> and with an appropriate Fixes: tag.

Hello Angelo,

The commit message is not clear.
The clients are cpufreq and mediatek cci.
I think there is no any error before introducing mediatek cci into
cpufreq.

The order of this patch should be after "cpufreq: mediatek: Link CCI
device to CPU".

If so, I will also correct the commit message in next version.

BRs,
Rex

Re: [PATCH V2 12/15] cpufreq: mediatek: Use maximum voltage in init stage

[Rex-BC Chen]

On Fri, 2022-04-08 at 15:37 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > Two or more clients may use the same regulator, and it could cause
> > the
> > issue of high-freqeuncy-low-voltage.
> > To prevent this, we use maximum voltage in
> > mtk_cpu_dvfs_info_init().
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> Is this happening for proc-supply (proc_reg)?
> ...because it looks like it is, so you should send this commit
> separately
> and with an appropriate Fixes: tag.

Hello all,

If we use the value of max(booting voltage, target cpufreq voltage)
when cci is not ready, I think we don't need this patch to prevent high
frequenc low voltage issue.
(mentioned in [1])

I will drop this patch in next version.

Thanks.

[1]:https://patchwork.kernel.org/comment/24816091/

BRs,
Rex

[PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

In some MediaTek SoCs, like MT8183, CPU and CCI share the same power
supplies. Cpufreq needs to check if CCI devfreq exists and wait until
CCI devfreq ready before scaling frequency.

- Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start
  DVFS when CCI is ready.
- Add platform data for MT8183.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 69 +++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index b08ab7c14818..cebe5af2ef5d 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -22,6 +22,7 @@ struct mtk_cpufreq_platform_data {
 	int proc_max_volt;
 	int sram_min_volt;
 	int sram_max_volt;
+	bool is_ccifreq_support;
 };
 
 /*
@@ -38,6 +39,7 @@ struct mtk_cpufreq_platform_data {
 struct mtk_cpu_dvfs_info {
 	struct cpumask cpus;
 	struct device *cpu_dev;
+	struct device *cci_dev;
 	struct regulator *proc_reg;
 	struct regulator *sram_reg;
 	struct clk *cpu_clk;
@@ -52,6 +54,7 @@ struct mtk_cpu_dvfs_info {
 	int opp_cpu;
 	unsigned long opp_freq;
 	const struct mtk_cpufreq_platform_data *soc_data;
+	bool is_ccifreq_bounded;
 };
 
 static struct platform_device *cpufreq_pdev;
@@ -171,6 +174,29 @@ static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
 	return ret;
 }
 
+static bool is_ccifreq_ready(struct mtk_cpu_dvfs_info *info)
+{
+	struct device_link *sup_link;
+
+	if (info->is_ccifreq_bounded)
+		return true;
+
+	sup_link = device_link_add(info->cpu_dev, info->cci_dev,
+				   DL_FLAG_AUTOREMOVE_CONSUMER);
+	if (!sup_link) {
+		dev_err(info->cpu_dev, "cpu%d: sup_link is NULL\n",
+			info->opp_cpu);
+		return false;
+	}
+
+	if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND)
+		return false;
+
+	info->is_ccifreq_bounded = true;
+
+	return true;
+}
+
 static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 				  unsigned int index)
 {
@@ -183,6 +209,9 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	long freq_hz, old_freq_hz;
 	int vproc, old_vproc, inter_vproc, target_vproc, ret;
 
+	if (info->soc_data->is_ccifreq_support && !is_ccifreq_ready(info))
+		return 0;
+
 	inter_vproc = info->intermediate_voltage;
 
 	freq_hz = freq_table[index].frequency * 1000;
@@ -329,6 +358,23 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb,
 	return notifier_from_errno(ret);
 }
 
+static struct device *of_get_cci(struct device *cpu_dev)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+
+	np = of_parse_phandle(cpu_dev->of_node, "cci", 0);
+	if (IS_ERR(np))
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+	if (IS_ERR(pdev))
+		return NULL;
+
+	return &pdev->dev;
+}
+
 static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 {
 	struct device *cpu_dev;
@@ -343,6 +389,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	}
 	info->cpu_dev = cpu_dev;
 
+	info->is_ccifreq_bounded = false;
+	if (info->soc_data->is_ccifreq_support) {
+		info->cci_dev = of_get_cci(info->cpu_dev);
+		if (IS_ERR_OR_NULL(info->cci_dev)) {
+			ret = PTR_ERR(info->cci_dev);
+			dev_err(cpu_dev, "cpu%d: failed to get cci device\n",
+				cpu);
+			return -ENODEV;
+		}
+	}
+
 	info->cpu_clk = clk_get(cpu_dev, "cpu");
 	if (IS_ERR(info->cpu_clk)) {
 		ret = PTR_ERR(info->cpu_clk);
@@ -620,6 +677,16 @@ static const struct mtk_cpufreq_platform_data mtk_platform_data = {
 	.proc_max_volt = 1150000,
 	.sram_min_volt = 0,
 	.sram_max_volt = 1150000,
+	.is_ccifreq_support = false,
+};
+
+static const struct mtk_cpufreq_platform_data mt8183_platform_data = {
+	.min_volt_shift = 100000,
+	.max_volt_shift = 200000,
+	.proc_max_volt = 1150000,
+	.sram_min_volt = 0,
+	.sram_max_volt = 1150000,
+	.is_ccifreq_support = true,
 };
 
 /* List of machines supported by this driver */
@@ -632,7 +699,7 @@ static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt817x", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8173", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8176", .data = &mtk_platform_data },
-	{ .compatible = "mediatek,mt8183", .data = &mtk_platform_data },
+	{ .compatible = "mediatek,mt8183", .data = &mt8183_platform_data },
 	{ .compatible = "mediatek,mt8365", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8516", .data = &mtk_platform_data },
 	{ }
-- 
2.18.0

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> In some MediaTek SoCs, like MT8183, CPU and CCI share the same power
> supplies. Cpufreq needs to check if CCI devfreq exists and wait until
> CCI devfreq ready before scaling frequency.
> 
> - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start
>    DVFS when CCI is ready.
> - Add platform data for MT8183.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 69 +++++++++++++++++++++++++++++-
>   1 file changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index b08ab7c14818..cebe5af2ef5d 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -22,6 +22,7 @@ struct mtk_cpufreq_platform_data {
>   	int proc_max_volt;
>   	int sram_min_volt;
>   	int sram_max_volt;
> +	bool is_ccifreq_support;

bool ccifreq_supported; looks better.

>   };
>   
>   /*
> @@ -38,6 +39,7 @@ struct mtk_cpufreq_platform_data {
>   struct mtk_cpu_dvfs_info {
>   	struct cpumask cpus;
>   	struct device *cpu_dev;
> +	struct device *cci_dev;
>   	struct regulator *proc_reg;
>   	struct regulator *sram_reg;
>   	struct clk *cpu_clk;
> @@ -52,6 +54,7 @@ struct mtk_cpu_dvfs_info {
>   	int opp_cpu;
>   	unsigned long opp_freq;
>   	const struct mtk_cpufreq_platform_data *soc_data;
> +	bool is_ccifreq_bounded;

bool ccifreq_bound; looks better.

>   };
>   
>   static struct platform_device *cpufreq_pdev;
> @@ -171,6 +174,29 @@ static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc)
>   	return ret;
>   }
>   
> +static bool is_ccifreq_ready(struct mtk_cpu_dvfs_info *info)
> +{
> +	struct device_link *sup_link;
> +
> +	if (info->is_ccifreq_bounded)
> +		return true;
> +
> +	sup_link = device_link_add(info->cpu_dev, info->cci_dev,
> +				   DL_FLAG_AUTOREMOVE_CONSUMER);
> +	if (!sup_link) {
> +		dev_err(info->cpu_dev, "cpu%d: sup_link is NULL\n",
> +			info->opp_cpu);

Please, don't break this line: 84 columns are ok.

> +		return false;
> +	}
> +
> +	if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND)
> +		return false;
> +
> +	info->is_ccifreq_bounded = true;
> +
> +	return true;
> +}
> +
>   static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   				  unsigned int index)
>   {
> @@ -183,6 +209,9 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   	long freq_hz, old_freq_hz;
>   	int vproc, old_vproc, inter_vproc, target_vproc, ret;
>   
> +	if (info->soc_data->is_ccifreq_support && !is_ccifreq_ready(info))
> +		return 0;

Honestly, I think that pretending that everything is alright and faking
set_target success is *not* a good idea...

You should return -EAGAIN here, not zero.

Regards,
Angelo

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Fri, 2022-04-08 at 15:37 +0200, AngeloGioacchino Del Regno wrote:
> Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
> > power
> > supplies. Cpufreq needs to check if CCI devfreq exists and wait
> > until
> > CCI devfreq ready before scaling frequency.
> > 
> > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will
> > start
> >    DVFS when CCI is ready.
> > - Add platform data for MT8183.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > ---
> >   drivers/cpufreq/mediatek-cpufreq.c | 69
> > +++++++++++++++++++++++++++++-
> >   1 file changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index b08ab7c14818..cebe5af2ef5d 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -22,6 +22,7 @@ struct mtk_cpufreq_platform_data {
> >   	int proc_max_volt;
> >   	int sram_min_volt;
> >   	int sram_max_volt;
> > +	bool is_ccifreq_support;
> 
> bool ccifreq_supported; looks better.

Hello Angelo,

Thanks for your review.

OK, I will modify this in next version.

> 
> >   };
> >   
> >   /*
> > @@ -38,6 +39,7 @@ struct mtk_cpufreq_platform_data {
> >   struct mtk_cpu_dvfs_info {
> >   	struct cpumask cpus;
> >   	struct device *cpu_dev;
> > +	struct device *cci_dev;
> >   	struct regulator *proc_reg;
> >   	struct regulator *sram_reg;
> >   	struct clk *cpu_clk;
> > @@ -52,6 +54,7 @@ struct mtk_cpu_dvfs_info {
> >   	int opp_cpu;
> >   	unsigned long opp_freq;
> >   	const struct mtk_cpufreq_platform_data *soc_data;
> > +	bool is_ccifreq_bounded;
> 
> bool ccifreq_bound; looks better.
> 

OK, I will modify this in next version.

> >   };
> >   
> >   static struct platform_device *cpufreq_pdev;
> > @@ -171,6 +174,29 @@ static int mtk_cpufreq_set_voltage(struct
> > mtk_cpu_dvfs_info *info, int vproc)
> >   	return ret;
> >   }
> >   
> > +static bool is_ccifreq_ready(struct mtk_cpu_dvfs_info *info)
> > +{
> > +	struct device_link *sup_link;
> > +
> > +	if (info->is_ccifreq_bounded)
> > +		return true;
> > +
> > +	sup_link = device_link_add(info->cpu_dev, info->cci_dev,
> > +				   DL_FLAG_AUTOREMOVE_CONSUMER);
> > +	if (!sup_link) {
> > +		dev_err(info->cpu_dev, "cpu%d: sup_link is NULL\n",
> > +			info->opp_cpu);
> 
> Please, don't break this line: 84 columns are ok.
> 

OK, I will modify this in next version.

> > +		return false;
> > +	}
> > +
> > +	if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND)
> > +		return false;
> > +
> > +	info->is_ccifreq_bounded = true;
> > +
> > +	return true;
> > +}
> > +
> >   static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> >   				  unsigned int index)
> >   {
> > @@ -183,6 +209,9 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >   	long freq_hz, old_freq_hz;
> >   	int vproc, old_vproc, inter_vproc, target_vproc, ret;
> >   
> > +	if (info->soc_data->is_ccifreq_support &&
> > !is_ccifreq_ready(info))
> > +		return 0;
> 
> Honestly, I think that pretending that everything is alright and
> faking
> set_target success is *not* a good idea...
> 
> You should return -EAGAIN here, not zero.
> 
> Regards,
> Angelo
> 

As metioneded by Kevin, I will review these three situations.
Thanks for your suggestion.

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>
> In some MediaTek SoCs, like MT8183, CPU and CCI share the same power
> supplies. Cpufreq needs to check if CCI devfreq exists and wait until
> CCI devfreq ready before scaling frequency.
>
> - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start
>   DVFS when CCI is ready.
> - Add platform data for MT8183.
>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>

The checks here are not enough, and will lead to unexpected behavior.
IIUC, before doing DVFS, you're checking:

1) if the "cci" DT node is present and
2) if the driver for that device is bound

If both those conditions are not met, you don't actually fail, you just
silently do nothing in ->set_target().  As Angelo pointed out also, this
is not a good idea, and will be rather confusing to users.

The same thing would happen if the cci DT node was present, but the CCI
devfreq driver was disabled.  Silent failure would also be quite
unexpected behavior.  Similarily, if the cci DT node is not present at
all (like it is in current upstream DT), this CPUfreq driver will
silently do nothing.  Not good.

So, this patch needs to handle several scenarios:

1) CCI DT node not present

In this case, the driver should still operate normally.  With no CCI
node, or driver there's no conflict.

2) CCI DT present/enabled but not yet bound

In this case, you could return -EAGAIN as suggested by Angelo, or maybe
better, it should do a deferred probe.

3) CCI DT present, but driver disabled

This case is similar to (1), this driver should continue to work.

Kevin

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
> > power
> > supplies. Cpufreq needs to check if CCI devfreq exists and wait
> > until
> > CCI devfreq ready before scaling frequency.
> > 
> > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will
> > start
> >   DVFS when CCI is ready.
> > - Add platform data for MT8183.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> The checks here are not enough, and will lead to unexpected behavior.
> IIUC, before doing DVFS, you're checking:
> 
> 1) if the "cci" DT node is present and
> 2) if the driver for that device is bound
> 
> If both those conditions are not met, you don't actually fail, you
> just
> silently do nothing in ->set_target().  As Angelo pointed out also,
> this
> is not a good idea, and will be rather confusing to users.
> 
> The same thing would happen if the cci DT node was present, but the
> CCI
> devfreq driver was disabled.  Silent failure would also be quite
> unexpected behavior.  Similarily, if the cci DT node is not present
> at
> all (like it is in current upstream DT), this CPUfreq driver will
> silently do nothing.  Not good.
> 
> So, this patch needs to handle several scenarios:
> 
> 1) CCI DT node not present
> 
> In this case, the driver should still operate normally.  With no CCI
> node, or driver there's no conflict.
> 
> 2) CCI DT present/enabled but not yet bound
> 
> In this case, you could return -EAGAIN as suggested by Angelo, or
> maybe
> better, it should do a deferred probe.
> 
> 3) CCI DT present, but driver disabled
> 
> This case is similar to (1), this driver should continue to work.
> 
> Kevin

Hello Kevin,

Thanks for your review.
I will review these three situations and do some modification.

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
> > power
> > supplies. Cpufreq needs to check if CCI devfreq exists and wait
> > until
> > CCI devfreq ready before scaling frequency.
> > 
> > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will
> > start
> >   DVFS when CCI is ready.
> > - Add platform data for MT8183.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> The checks here are not enough, and will lead to unexpected behavior.
> IIUC, before doing DVFS, you're checking:
> 
> 1) if the "cci" DT node is present and
> 2) if the driver for that device is bound
> 
> If both those conditions are not met, you don't actually fail, you
> just
> silently do nothing in ->set_target().  As Angelo pointed out also,
> this
> is not a good idea, and will be rather confusing to users.
> 
> The same thing would happen if the cci DT node was present, but the
> CCI
> devfreq driver was disabled.  Silent failure would also be quite
> unexpected behavior.  Similarily, if the cci DT node is not present
> at
> all (like it is in current upstream DT), this CPUfreq driver will
> silently do nothing.  Not good.
> 
> So, this patch needs to handle several scenarios:
> 
> 1) CCI DT node not present
> 
> In this case, the driver should still operate normally.  With no CCI
> node, or driver there's no conflict.
> 
> 2) CCI DT present/enabled but not yet bound
> 
> In this case, you could return -EAGAIN as suggested by Angelo, or
> maybe
> better, it should do a deferred probe.
> 
> 3) CCI DT present, but driver disabled
> 
> This case is similar to (1), this driver should continue to work.
> 
> Kevin

Hello Kevin and Angelo,

In my review, if we do not get the link or the link status is not
correct between cci and cpufreq in target_index, I think it will never
established again for this link.
Because it's not checked in probe stage.

So I think we just need to deal with the issue without cci device, and
don't expect the link between cci and cpufreq will be connected again.

If I am wrong, please correct me.
Thanks.

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> > 
>> > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
>> > power
>> > supplies. Cpufreq needs to check if CCI devfreq exists and wait
>> > until
>> > CCI devfreq ready before scaling frequency.
>> > 
>> > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will
>> > start
>> >   DVFS when CCI is ready.
>> > - Add platform data for MT8183.
>> > 
>> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>> 
>> The checks here are not enough, and will lead to unexpected behavior.
>> IIUC, before doing DVFS, you're checking:
>> 
>> 1) if the "cci" DT node is present and
>> 2) if the driver for that device is bound
>> 
>> If both those conditions are not met, you don't actually fail, you
>> just
>> silently do nothing in ->set_target().  As Angelo pointed out also,
>> this
>> is not a good idea, and will be rather confusing to users.
>> 
>> The same thing would happen if the cci DT node was present, but the
>> CCI
>> devfreq driver was disabled.  Silent failure would also be quite
>> unexpected behavior.  Similarily, if the cci DT node is not present
>> at
>> all (like it is in current upstream DT), this CPUfreq driver will
>> silently do nothing.  Not good.
>> 
>> So, this patch needs to handle several scenarios:
>> 
>> 1) CCI DT node not present
>> 
>> In this case, the driver should still operate normally.  With no CCI
>> node, or driver there's no conflict.
>> 
>> 2) CCI DT present/enabled but not yet bound
>> 
>> In this case, you could return -EAGAIN as suggested by Angelo, or
>> maybe
>> better, it should do a deferred probe.
>> 
>> 3) CCI DT present, but driver disabled
>> 
>> This case is similar to (1), this driver should continue to work.
>> 
>> Kevin
>
> Hello Kevin and Angelo,
>
> In my review, if we do not get the link or the link status is not
> correct between cci and cpufreq in target_index, I think it will never
> established again for this link.
> Because it's not checked in probe stage.
>
> So I think we just need to deal with the issue without cci device, and
> don't expect the link between cci and cpufreq will be connected again.
>
> If I am wrong, please correct me.

I don't fully understand your questions, but I think what your getting
at suggest that you might need to use deferred probe to handle the case
where the ordering of CCI and cpufreq probing is not predictable.

Kevin

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Mon, 2022-04-11 at 11:13 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote:
> > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> > > 
> > > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > > 
> > > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same
> > > > power
> > > > supplies. Cpufreq needs to check if CCI devfreq exists and wait
> > > > until
> > > > CCI devfreq ready before scaling frequency.
> > > > 
> > > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU
> > > > will
> > > > start
> > > >   DVFS when CCI is ready.
> > > > - Add platform data for MT8183.
> > > > 
> > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > > 
> > > The checks here are not enough, and will lead to unexpected
> > > behavior.
> > > IIUC, before doing DVFS, you're checking:
> > > 
> > > 1) if the "cci" DT node is present and
> > > 2) if the driver for that device is bound
> > > 
> > > If both those conditions are not met, you don't actually fail,
> > > you
> > > just
> > > silently do nothing in ->set_target().  As Angelo pointed out
> > > also,
> > > this
> > > is not a good idea, and will be rather confusing to users.
> > > 
> > > The same thing would happen if the cci DT node was present, but
> > > the
> > > CCI
> > > devfreq driver was disabled.  Silent failure would also be quite
> > > unexpected behavior.  Similarily, if the cci DT node is not
> > > present
> > > at
> > > all (like it is in current upstream DT), this CPUfreq driver will
> > > silently do nothing.  Not good.
> > > 
> > > So, this patch needs to handle several scenarios:
> > > 
> > > 1) CCI DT node not present
> > > 
> > > In this case, the driver should still operate normally.  With no
> > > CCI
> > > node, or driver there's no conflict.
> > > 
> > > 2) CCI DT present/enabled but not yet bound
> > > 
> > > In this case, you could return -EAGAIN as suggested by Angelo, or
> > > maybe
> > > better, it should do a deferred probe.
> > > 
> > > 3) CCI DT present, but driver disabled
> > > 
> > > This case is similar to (1), this driver should continue to work.
> > > 
> > > Kevin
> > 
> > Hello Kevin and Angelo,
> > 
> > In my review, if we do not get the link or the link status is not
> > correct between cci and cpufreq in target_index, I think it will
> > never
> > established again for this link.
> > Because it's not checked in probe stage.
> > 
> > So I think we just need to deal with the issue without cci device,
> > and
> > don't expect the link between cci and cpufreq will be connected
> > again.
> > 
> > If I am wrong, please correct me.
> 
> I don't fully understand your questions, but I think what your
> getting
> at suggest that you might need to use deferred probe to handle the
> case
> where the ordering of CCI and cpufreq probing is not predictable.
> 
> Kevin

Hello Kevin and Angelo,

I can summary what I got now:

1. Why we need cci for cpufreq in MT8183 and MT8186:
   a. CCI is a mediatek hw module.
   b. For mediatek SoCs before MT8183, like MT8173, the CCI hw
      is not introduced.
   c. The frequency for cci and cpufreq are determined could
      be configed at bootloader stage, so the frequency when
      entering kernel is unknown.
   d. Cpu little core and cci are using the same regulator.
   e. If we do not control CCI and just adjust the voltage in
      cpufreq driver.
      When we adjust the voltage smaller because we need to reduce
      the frequency, the CCI could run in high frequency which is
      set in bootloader.
      This will cause some problem, the cci could crash.

      Use MT8186 for a example, the bootloader set cci freq as
      1.385GHz and cpufreq as 2GHz.
      If entering kernel and we only adjust the cpufreq voltage, if
      the cpufreq is under 1.618GHz, the cci will be out of spec.
      You can refer to the chrome project bootloader "Coreboot" patch:
      
https://review.coreboot.org/c/coreboot/+/59569/2/src/mainboard/google/corsola/romstage.c

   f. If cpufreq driver wait cci ready, regulator framework will take
      the highest voltage requests from cci and cpufreq as output
      so that it prevents from high freqeuncy low voltage crash.
   d. Therefore, I think it's not a good idea to bypass cci device if
      the ccifreq_supported is true in MT8183 and MT8186.

2. Check the device link status is DL_DEV_DRIVER_BOUND is used for
   promising the cci is probed done.

3. About the cpufreq_driver->target_index
   a. When I trace the common drivers, I found if the return value is
      not zero, it will be BUG_ON.
      ref:
https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1471
   b. I also try to move is_ccifreq_ready() to other place, like
      cpufreq_driver->init and cpufreq probe function.
      There will be new issue. Do you have any suggetion that we can
      retern value of DEFER_PROBE?

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

[...]

> I can summary what I got now:
>
> 1. Why we need cci for cpufreq in MT8183 and MT8186:
>    a. CCI is a mediatek hw module.
>    b. For mediatek SoCs before MT8183, like MT8173, the CCI hw
>       is not introduced.
>    c. The frequency for cci and cpufreq are determined could
>       be configed at bootloader stage, so the frequency when
>       entering kernel is unknown.
>    d. Cpu little core and cci are using the same regulator.
>    e. If we do not control CCI and just adjust the voltage in
>       cpufreq driver.
>       When we adjust the voltage smaller because we need to reduce
>       the frequency, the CCI could run in high frequency which is
>       set in bootloader.
>       This will cause some problem, the cci could crash.
>
>       Use MT8186 for a example, the bootloader set cci freq as
>       1.385GHz and cpufreq as 2GHz.
>       If entering kernel and we only adjust the cpufreq voltage, if
>       the cpufreq is under 1.618GHz, the cci will be out of spec.
>
>    f. If cpufreq driver wait cci ready, regulator framework will take
>       the highest voltage requests from cci and cpufreq as output
>       so that it prevents from high freqeuncy low voltage crash.
>
>    d. Therefore, I think it's not a good idea to bypass cci device if
>       the ccifreq_supported is true in MT8183 and MT8186.

I do not propose to bypass CCI device.  What both Angelo and I are
saying is just that you need a better way to handle the cases when CCI
is not (yet) enabled.  The current way in the propsed patch is not good
enough.

I fully understand the potential problems with high frequency & low
voltage when using a shared regulator.  But, I think the problem we're
trying to solve here is specific to the initial boot of the platform,
while we are waiting for the CCI driver to be loaded.

The root of the problem is that the CCI bus has constraints on the
voltage regulator that are not defined anywhere until the CCI driver is
loaded.  So to fix that, you need to either:

1) not allow any voltage changes
2) register the CCI device constraints

In the current patch, you attempt to do (1).  There's nothing wrong with
the idea, we just pointed out problems in your implementation,
especially the fact that it does nothing, but it "succeeds" so the
CPUfreq framework will think the OPPs are different from what they
actually are.

Just an idea, but another option could be (2).  While waiting for the
CCI device to be ready, the CPUfreq driver could check the current CCI
freq/voltage and set min/max constraints on the regulator that prevent
CCI from breaking.  These constraints would stay in place until the CCI
driver is ready.  Once the real CCI driver is ready/registerd these
contraints would be removed.

Another version of this same idea would be to check the CCI freq/voltage
and then limit the OPPs available to CPUfreq to only the ones that would
not break CCI.  Then, when CCI is ready/registered, you remove the
limits.

> 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for
>    promising the cci is probed done.
>
> 3. About the cpufreq_driver->target_index
>    a. When I trace the common drivers, I found if the return value is
>       not zero, it will be BUG_ON.
>       ref:
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1471

Right, you should not try to do deferred probe in the ->set_target()
callback.  Deferred probe is meant for init/probe time.

>    b. I also try to move is_ccifreq_ready() to other place, like
>       cpufreq_driver->init and cpufreq probe function.
>       There will be new issue. Do you have any suggetion that we can
>       retern value of DEFER_PROBE?

The only appropriate place is in the probe function.

Kevin

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Tue, 2022-04-12 at 11:50 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> [...]
> 
> > I can summary what I got now:
> > 
> > 1. Why we need cci for cpufreq in MT8183 and MT8186:
> >    a. CCI is a mediatek hw module.
> >    b. For mediatek SoCs before MT8183, like MT8173, the CCI hw
> >       is not introduced.
> >    c. The frequency for cci and cpufreq are determined could
> >       be configed at bootloader stage, so the frequency when
> >       entering kernel is unknown.
> >    d. Cpu little core and cci are using the same regulator.
> >    e. If we do not control CCI and just adjust the voltage in
> >       cpufreq driver.
> >       When we adjust the voltage smaller because we need to reduce
> >       the frequency, the CCI could run in high frequency which is
> >       set in bootloader.
> >       This will cause some problem, the cci could crash.
> > 
> >       Use MT8186 for a example, the bootloader set cci freq as
> >       1.385GHz and cpufreq as 2GHz.
> >       If entering kernel and we only adjust the cpufreq voltage, if
> >       the cpufreq is under 1.618GHz, the cci will be out of spec.
> > 
> >    f. If cpufreq driver wait cci ready, regulator framework will
> > take
> >       the highest voltage requests from cci and cpufreq as output
> >       so that it prevents from high freqeuncy low voltage crash.
> > 
> >    d. Therefore, I think it's not a good idea to bypass cci device
> > if
> >       the ccifreq_supported is true in MT8183 and MT8186.
> 
> I do not propose to bypass CCI device.  What both Angelo and I are
> saying is just that you need a better way to handle the cases when
> CCI
> is not (yet) enabled.  The current way in the propsed patch is not
> good
> enough.
> 
> I fully understand the potential problems with high frequency & low
> voltage when using a shared regulator.  But, I think the problem
> we're
> trying to solve here is specific to the initial boot of the platform,
> while we are waiting for the CCI driver to be loaded.
> 
> The root of the problem is that the CCI bus has constraints on the
> voltage regulator that are not defined anywhere until the CCI driver
> is
> loaded.  So to fix that, you need to either:
> 
> 1) not allow any voltage changes
> 2) register the CCI device constraints
> 
> In the current patch, you attempt to do (1).  There's nothing wrong
> with
> the idea, we just pointed out problems in your implementation,
> especially the fact that it does nothing, but it "succeeds" so the
> CPUfreq framework will think the OPPs are different from what they
> actually are.
> 
> Just an idea, but another option could be (2).  While waiting for the
> CCI device to be ready, the CPUfreq driver could check the current
> CCI
> freq/voltage and set min/max constraints on the regulator that
> prevent
> CCI from breaking.  These constraints would stay in place until the
> CCI
> driver is ready.  Once the real CCI driver is ready/registerd these
> contraints would be removed.
> 
> Another version of this same idea would be to check the CCI
> freq/voltage
> and then limit the OPPs available to CPUfreq to only the ones that
> would
> not break CCI.  Then, when CCI is ready/registered, you remove the
> limits.
> 
> > 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for
> >    promising the cci is probed done.
> > 
> > 3. About the cpufreq_driver->target_index
> >    a. When I trace the common drivers, I found if the return value
> > is
> >       not zero, it will be BUG_ON.
> >       ref:
> > 
https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c*L1471__;Iw!!CTRNKA9wMg0ARbw!wgawOs1JSuJihgxA1nxhbd2Ekoys_bPCAlIH9YJhe2N9ckG6O1mDB-7zqSf6x2MhCfXo$
> >  
> 
> Right, you should not try to do deferred probe in the ->set_target()
> callback.  Deferred probe is meant for init/probe time.
> 
> >    b. I also try to move is_ccifreq_ready() to other place, like
> >       cpufreq_driver->init and cpufreq probe function.
> >       There will be new issue. Do you have any suggetion that we
> > can
> >       retern value of DEFER_PROBE?
> 
> The only appropriate place is in the probe function.
> 
> Kevin

Hello Kevin,

From the Chanwoo's devfreq passive govonor series, it's impossible to
let cci devreq probed done before cpufreq because the passive govonor
will search for cpufreq node and use it.

Ref: function: cpufreq_passive_register_notifier()

https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673

After I discuss with Angelo and Jia-wei, we think we are keeping the
function in target_index and if the cci is not ready we will use the
voltage which is set by bootloader to prevent high freqeuncy low
voltage crash. And then we can keep seting the target frequency.

We assume the setting of bootloader is correct and we can do this.

For the SoCs that including ci hardware (8183 and 8186), we think it's
not ok if we don't probe cci correctly.
If we failed to get cci node, I think we sould return -ENODEV and the
probe of cpufreq failed.

What do you think the solution?

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

[...]

> From the Chanwoo's devfreq passive govonor series, it's impossible to
> let cci devreq probed done before cpufreq because the passive govonor
> will search for cpufreq node and use it.
>
> Ref: function: cpufreq_passive_register_notifier()
>
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673

Well this is a problem, because CCI depends on CPUfreq, but CPUfreq
depends on CCI, so one of them has to load and then wait for the other.

> After I discuss with Angelo and Jia-wei, we think we are keeping the
> function in target_index and if the cci is not ready we will use the
> voltage which is set by bootloader to prevent high freqeuncy low
> voltage crash. And then we can keep seting the target frequency.
>
 > We assume the setting of bootloader is correct and we can do this.

I'm still not crazy about this because you're lying to the CPUfreq
framework.  It's requesting one OPP, but you're not setting that, you're
just keeping the bootloader frequency.

In my earlier reply, I gave two other options for handling this.

1) set a (temporary) constraint on the voltage regulator so that it
cannot change.

or more clean, IMO:

2) set a CPUfreq policy that restricts available OPPs to ones that will
not break CCI.

Either of these solutions allow you to load the CPUfreq driver early,
and then wait for the CCI driver to be ready before removing the
restrictions.

> For the SoCs that including ci hardware (8183 and 8186), we think it's
> not ok if we don't probe cci correctly.
> If we failed to get cci node, I think we sould return -ENODEV and the
> probe of cpufreq failed.
>
> What do you think the solution?

I think it would be better if CPUfreq probes sucessfully, but restricts
the OPPs available until CCI is ready.  If CCI fails to probe/load, you
still have a working CPUfreq driver, it just has a restricted set of
OPPs.

Kevin

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Wed, 2022-04-13 at 14:41 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> [...]
> 
> > From the Chanwoo's devfreq passive govonor series, it's impossible
> > to
> > let cci devreq probed done before cpufreq because the passive
> > govonor
> > will search for cpufreq node and use it.
> > 
> > Ref: function: cpufreq_passive_register_notifier()
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673__;!!CTRNKA9wMg0ARbw!z58Lc1p9REo88oHn-NkxroN_fBd0TsHYmhscNZwnWwT71ecRkTeqZ6vFl5l7HpkTdM6t$
> >  
> 
> Well this is a problem, because CCI depends on CPUfreq, but CPUfreq
> depends on CCI, so one of them has to load and then wait for the
> other.
> 
> > After I discuss with Angelo and Jia-wei, we think we are keeping
> > the
> > function in target_index and if the cci is not ready we will use
> > the
> > voltage which is set by bootloader to prevent high freqeuncy low
> > voltage crash. And then we can keep seting the target frequency.
> > 
> 
>  > We assume the setting of bootloader is correct and we can do this.
> 
> I'm still not crazy about this because you're lying to the CPUfreq
> framework.  It's requesting one OPP, but you're not setting that,
> you're
> just keeping the bootloader frequency.
> 
> In my earlier reply, I gave two other options for handling this.
> 
> 1) set a (temporary) constraint on the voltage regulator so that it
> cannot change.
> 
> or more clean, IMO:
> 
> 2) set a CPUfreq policy that restricts available OPPs to ones that
> will
> not break CCI.
> 
> Either of these solutions allow you to load the CPUfreq driver early,
> and then wait for the CCI driver to be ready before removing the
> restrictions.

Hello Kevin,

I think I do not describe this clearly.
The proposal is:

In cpufreq probe:
we record the voltage value which is set by bootloader.

In mtk_cpufreq_set_target():
We do NOT directly return 0.
Instead, we will find the voltage of target cpufreq and use the value
max(booting voltage, target cpufreq voltage)

mtk_cpufreq_set_target() {
	/* NOT return 0 if !is_ccifreq_ready */
	....
	vproc = get voltage of target cpufreq from opp.

	if (ccifreq_supported && !is_ccifreq_ready)
		vproc = max(vproc, vproc_on_boot)

	//setting voltage and target frequency
	....
}

> 
> > For the SoCs that including ci hardware (8183 and 8186), we think
> > it's
> > not ok if we don't probe cci correctly.
> > If we failed to get cci node, I think we sould return -ENODEV and
> > the
> > probe of cpufreq failed.
> > 
> > What do you think the solution?
> 
> I think it would be better if CPUfreq probes sucessfully, but
> restricts
> the OPPs available until CCI is ready.  If CCI fails to probe/load,
> you
> still have a working CPUfreq driver, it just has a restricted set of
> OPPs.
> 
> Kevin

If we can use the solution.
I think it will be ok for this situation.

Thanks!

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Wed, 2022-04-13 at 14:41 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> [...]
>> 
>> > From the Chanwoo's devfreq passive govonor series, it's impossible
>> > to
>> > let cci devreq probed done before cpufreq because the passive
>> > govonor
>> > will search for cpufreq node and use it.
>> > 
>> > Ref: function: cpufreq_passive_register_notifier()
>> > 
>> > 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673__;!!CTRNKA9wMg0ARbw!z58Lc1p9REo88oHn-NkxroN_fBd0TsHYmhscNZwnWwT71ecRkTeqZ6vFl5l7HpkTdM6t$
>> >  
>> 
>> Well this is a problem, because CCI depends on CPUfreq, but CPUfreq
>> depends on CCI, so one of them has to load and then wait for the
>> other.
>> 
>> > After I discuss with Angelo and Jia-wei, we think we are keeping
>> > the
>> > function in target_index and if the cci is not ready we will use
>> > the
>> > voltage which is set by bootloader to prevent high freqeuncy low
>> > voltage crash. And then we can keep seting the target frequency.
>> > 
>> 
>>  > We assume the setting of bootloader is correct and we can do this.
>> 
>> I'm still not crazy about this because you're lying to the CPUfreq
>> framework.  It's requesting one OPP, but you're not setting that,
>> you're
>> just keeping the bootloader frequency.
>> 
>> In my earlier reply, I gave two other options for handling this.
>> 
>> 1) set a (temporary) constraint on the voltage regulator so that it
>> cannot change.
>> 
>> or more clean, IMO:
>> 
>> 2) set a CPUfreq policy that restricts available OPPs to ones that
>> will
>> not break CCI.
>> 
>> Either of these solutions allow you to load the CPUfreq driver early,
>> and then wait for the CCI driver to be ready before removing the
>> restrictions.
>
> Hello Kevin,
>
> I think I do not describe this clearly.
> The proposal is:
>
> In cpufreq probe:
> we record the voltage value which is set by bootloader.
>
> In mtk_cpufreq_set_target():
> We do NOT directly return 0.
> Instead, we will find the voltage of target cpufreq and use the value
> max(booting voltage, target cpufreq voltage)
>
> mtk_cpufreq_set_target() {
> 	/* NOT return 0 if !is_ccifreq_ready */
> 	....
> 	vproc = get voltage of target cpufreq from opp.
>
> 	if (ccifreq_supported && !is_ccifreq_ready)
> 		vproc = max(vproc, vproc_on_boot)
>
> 	//setting voltage and target frequency
> 	....
> }

You explained this well, but it's still not an appropriate solution IMO,
because you're still not setting the target that is requested by the
CPUfreq core.

The job of ->set_target() is to set the frequency *requested by CPUfreq
core*.  If you cannot do that, you should return failure.  What you posted
in the original patch and what you're proposing here is to ignore the
frequency passed to ->set_target() and do something else.  In the
orignal patch, you propose do to nothing.  Now, you're ignoring the 
target passed in and setting something else.  In both cases, the CPUfreq
core things you have successfuly set the frequency requested, but you
have not.  This means there's a mismatch between what the CPUfreq core &
governer things the frequency is and what is actually set.  *This* is
the part that I think is wrong.

Instead, the proper way of restricting available frequencies is to use
governors or policies.  This ensures that the core & governors are
aligned with what the platform driver actually does.

As I proposed earlier, I think a clean solution to this problem is to
create a temporary policy at probe time that restricts the available
OPPs based on what the current CCI freq/voltage are.  Once CCI driver is
loaded and working, this policy can be removed.

Kevin

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Rex-BC Chen]

On Thu, 2022-04-14 at 14:48 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > On Wed, 2022-04-13 at 14:41 -0700, Kevin Hilman wrote:
> > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> > > 
> > > [...]
> > > 
> > > > From the Chanwoo's devfreq passive govonor series, it's
> > > > impossible
> > > > to
> > > > let cci devreq probed done before cpufreq because the passive
> > > > govonor
> > > > will search for cpufreq node and use it.
> > > > 
> > > > Ref: function: cpufreq_passive_register_notifier()
> > > > 
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673__;!!CTRNKA9wMg0ARbw!z58Lc1p9REo88oHn-NkxroN_fBd0TsHYmhscNZwnWwT71ecRkTeqZ6vFl5l7HpkTdM6t$
> > > >  
> > > 
> > > Well this is a problem, because CCI depends on CPUfreq, but
> > > CPUfreq
> > > depends on CCI, so one of them has to load and then wait for the
> > > other.
> > > 
> > > > After I discuss with Angelo and Jia-wei, we think we are
> > > > keeping
> > > > the
> > > > function in target_index and if the cci is not ready we will
> > > > use
> > > > the
> > > > voltage which is set by bootloader to prevent high freqeuncy
> > > > low
> > > > voltage crash. And then we can keep seting the target
> > > > frequency.
> > > > 
> > > 
> > >  > We assume the setting of bootloader is correct and we can do
> > > this.
> > > 
> > > I'm still not crazy about this because you're lying to the
> > > CPUfreq
> > > framework.  It's requesting one OPP, but you're not setting that,
> > > you're
> > > just keeping the bootloader frequency.
> > > 
> > > In my earlier reply, I gave two other options for handling this.
> > > 
> > > 1) set a (temporary) constraint on the voltage regulator so that
> > > it
> > > cannot change.
> > > 
> > > or more clean, IMO:
> > > 
> > > 2) set a CPUfreq policy that restricts available OPPs to ones
> > > that
> > > will
> > > not break CCI.
> > > 
> > > Either of these solutions allow you to load the CPUfreq driver
> > > early,
> > > and then wait for the CCI driver to be ready before removing the
> > > restrictions.
> > 
> > Hello Kevin,
> > 
> > I think I do not describe this clearly.
> > The proposal is:
> > 
> > In cpufreq probe:
> > we record the voltage value which is set by bootloader.
> > 
> > In mtk_cpufreq_set_target():
> > We do NOT directly return 0.
> > Instead, we will find the voltage of target cpufreq and use the
> > value
> > max(booting voltage, target cpufreq voltage)
> > 
> > mtk_cpufreq_set_target() {
> > 	/* NOT return 0 if !is_ccifreq_ready */
> > 	....
> > 	vproc = get voltage of target cpufreq from opp.
> > 
> > 	if (ccifreq_supported && !is_ccifreq_ready)
> > 		vproc = max(vproc, vproc_on_boot)
> > 
> > 	//setting voltage and target frequency
> > 	....
> > }
> 
> You explained this well, but it's still not an appropriate solution
> IMO,
> because you're still not setting the target that is requested by the
> CPUfreq core.
> 
> The job of ->set_target() is to set the frequency *requested by
> CPUfreq
> core*.  If you cannot do that, you should return failure.  What you
> posted
> in the original patch and what you're proposing here is to ignore the
> frequency passed to ->set_target() and do something else.  In the
> orignal patch, you propose do to nothing.  Now, you're ignoring the 
> target passed in and setting something else.  In both cases, the
> CPUfreq
> core things you have successfuly set the frequency requested, but you
> have not.  This means there's a mismatch between what the CPUfreq
> core &
> governer things the frequency is and what is actually set.  *This* is
> the part that I think is wrong.
> 
> Instead, the proper way of restricting available frequencies is to
> use
> governors or policies.  This ensures that the core & governors are
> aligned with what the platform driver actually does.
> 
> As I proposed earlier, I think a clean solution to this problem is to
> create a temporary policy at probe time that restricts the available
> OPPs based on what the current CCI freq/voltage are.  Once CCI driver
> is
> loaded and working, this policy can be removed.
> 
> Kevin
> 
> 

Hello Kevin,

In new proposal, we DO set the cpufreq passed by cpufreq core.
We just not set the corresponding voltage of target frequency which
is lookedup from opp table.
Actually, because of the shared regulator, corresponding voltage is
not always set.

I take this for example:
----------
Assumption:
- The opp table and voltage in this example are just a example,
  and it is not for any actual SoCs.
- We just use one regulator instead of two. (like proc and sram)
- cpufreq and mediatek cci devfreq share the same regulator_0.

OPP table for cpufreq:
target freq => min reguired voltage
2.0GHz => 1.0V
1.8GHZ => 0.9V
1.5GHz => 0.6V

OPP table for mediatek cci devfreq:
target freq => min reguired voltage
1.4GHz => 1.2V
1.0GHz => 0.8V

1. For normal case:
   (regulator_0 is already registered by cpufreq and cci)
   - When the cpufreq want to adjust to 1.5GHz, from the opp table the
     voltage sholud be 0.6V.
   - When the cci want to adjust to 1.4GHz, from the opp table the
     voltage sholud be 1.2V.
   - cpufreq driver use the regulator_set_voltage() to set the
     voltage, but the function will use 1.2V(ref:[1])

2. For our new proposed solution:
   (regulator_0 is registered by cpufreq but is not registered by cci)
   - Assume the freqs when booting to kernel are:
     1.8GHz for cpufreq (min 0.9V) and 1.0GHz for cci (min 0.8V).
     The voltage when booting to kernel(voltage_on_boot) should be 0.9V
	 to let cpufreq and cci work correctly.
   - When cpufreq want to set target freq to 1.5GHz the voltage from
     opp table is 0.6V and we compare this voltage with
voltage_on_boot(0.9V).
	 We will choose the max voltage 0.9V to prevent crash issue.
	 (This is original covered by regulator_set_voltage()
	  if regulator_0 is registered by both cci and cpufre.)
	- When the voltage is choosed, we will set the cpufreq to
1.5GHz while
	  the voltage is safe for both cpufreq and cci.
----------

In summary, we think it's a proper solution to cover the situation
when cci is not probed.

I think there is something to improve:
We can choose to lookup cci opp table using cci freq to determine
the voltage instead of voltage_on_boot.
But IMO, it's not neccessary to register cci opp table inside cpufreq
driver just for the short period.

Because I finish to prepare other patches and I think we also can
take a look at other patches which are including some cleanup, I will
send next version today.
If there is any concern and question, we can discuss in next version.

Thanks for your big support!

[1]:
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L4022

BRs,
Rex

Re: [PATCH V2 13/15] cpufreq: mediatek: Link CCI device to CPU

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Thu, 2022-04-14 at 14:48 -0700, Kevin Hilman wrote:

[...]

>> 
>> You explained this well, but it's still not an appropriate solution
>> IMO,
>> because you're still not setting the target that is requested by the
>> CPUfreq core.
>> 
>> The job of ->set_target() is to set the frequency *requested by
>> CPUfreq
>> core*.  If you cannot do that, you should return failure.  What you
>> posted
>> in the original patch and what you're proposing here is to ignore the
>> frequency passed to ->set_target() and do something else.  In the
>> orignal patch, you propose do to nothing.  Now, you're ignoring the 
>> target passed in and setting something else.  In both cases, the
>> CPUfreq
>> core things you have successfuly set the frequency requested, but you
>> have not.  This means there's a mismatch between what the CPUfreq
>> core &
>> governer things the frequency is and what is actually set.  *This* is
>> the part that I think is wrong.
>> 
>> Instead, the proper way of restricting available frequencies is to
>> use
>> governors or policies.  This ensures that the core & governors are
>> aligned with what the platform driver actually does.
>> 
>> As I proposed earlier, I think a clean solution to this problem is to
>> create a temporary policy at probe time that restricts the available
>> OPPs based on what the current CCI freq/voltage are.  Once CCI driver
>> is
>> loaded and working, this policy can be removed.
>> 
>> Kevin
>> 
>> 
>
> Hello Kevin,
>
> In new proposal, we DO set the cpufreq passed by cpufreq core.
> We just not set the corresponding voltage of target frequency which
> is lookedup from opp table.

OK, this makes more sense.  I thought you were ignoring frequency change
also.

[...]

> In summary, we think it's a proper solution to cover the situation
> when cci is not probed.

Yes, this is OK with me.

> I think there is something to improve:
> We can choose to lookup cci opp table using cci freq to determine
> the voltage instead of voltage_on_boot.
> But IMO, it's not neccessary to register cci opp table inside cpufreq
> driver just for the short period.

I agree.

> Because I finish to prepare other patches and I think we also can
> take a look at other patches which are including some cleanup, I will
> send next version today.
> If there is any concern and question, we can discuss in next version.

OK, I'll have a closer look at the new version.

Kevin

[PATCH V2 14/15] cpufreq: mediatek: Add support for MT8186

[Rex-BC Chen]

From: Jia-Wei Chang <jia-wei.chang@mediatek.com>

The platform data of MT8186 is different from previous MediaTek SoCs,
so we add a new compatible and platform data for it.

Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index cebe5af2ef5d..07d3b31e3ee0 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -689,6 +689,15 @@ static const struct mtk_cpufreq_platform_data mt8183_platform_data = {
 	.is_ccifreq_support = true,
 };
 
+static const struct mtk_cpufreq_platform_data mt8186_platform_data = {
+	.min_volt_shift = 100000,
+	.max_volt_shift = 250000,
+	.proc_max_volt = 1118750,
+	.sram_min_volt = 850000,
+	.sram_max_volt = 1118750,
+	.is_ccifreq_support = true,
+};
+
 /* List of machines supported by this driver */
 static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt2701", .data = &mtk_platform_data },
@@ -700,6 +709,7 @@ static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt8173", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8176", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8183", .data = &mt8183_platform_data },
+	{ .compatible = "mediatek,mt8186", .data = &mt8186_platform_data },
 	{ .compatible = "mediatek,mt8365", .data = &mtk_platform_data },
 	{ .compatible = "mediatek,mt8516", .data = &mtk_platform_data },
 	{ }
-- 
2.18.0

Re: [PATCH V2 14/15] cpufreq: mediatek: Add support for MT8186

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> 
> The platform data of MT8186 is different from previous MediaTek SoCs,
> so we add a new compatible and platform data for it.
> 
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 14/15] cpufreq: mediatek: Add support for MT8186

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
>
> The platform data of MT8186 is different from previous MediaTek SoCs,
> so we add a new compatible and platform data for it.
>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

There's no upstream DT for MT8186, so I"m curious how this was
tested/valiated with upstream?

Kevin

Re: [PATCH V2 14/15] cpufreq: mediatek: Add support for MT8186

[Rex-BC Chen]

On Fri, 2022-04-08 at 14:10 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> 
> > From: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > 
> > The platform data of MT8186 is different from previous MediaTek
> > SoCs,
> > so we add a new compatible and platform data for it.
> > 
> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> There's no upstream DT for MT8186, so I"m curious how this was
> tested/valiated with upstream?
> 
> Kevin

Hello Kevin,

Thanks for your review.
The series of MT8186 DTS is still upstreaming.
From Matthias's comment, he will review MT8186 DTS series after clock
series is accepted.

We just now test MT8186 cpufreq on chromeos kernel v5.15.

MT8186 DTS series:
message id:20220311130732.22706-1-allen-kh.cheng@mediatek.com

BRs,
Rex

[PATCH V2 15/15] cpufreq: mediatek: Use device print to show logs

[Rex-BC Chen]

Replace pr_* with dev_* to show logs.

Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 41 +++++++++++++++++-------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 07d3b31e3ee0..34e3baa4927f 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -92,13 +92,15 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 
 	old_vproc = regulator_get_voltage(proc_reg);
 	if (old_vproc < 0) {
-		pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc);
+		dev_err(info->cpu_dev,
+			"%s: invalid Vproc value: %d\n", __func__, old_vproc);
 		return old_vproc;
 	}
 
 	old_vsram = regulator_get_voltage(sram_reg);
 	if (old_vsram < 0) {
-		pr_err("%s: invalid Vsram value: %d\n", __func__, old_vsram);
+		dev_err(info->cpu_dev,
+			"%s: invalid Vsram value: %d\n", __func__, old_vsram);
 		return old_vsram;
 	}
 
@@ -217,8 +219,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	freq_hz = freq_table[index].frequency * 1000;
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz);
 	if (IS_ERR(opp)) {
-		pr_err("cpu%d: failed to find OPP for %ld\n",
-		       policy->cpu, freq_hz);
+		dev_err(cpu_dev, "cpu%d: failed to find OPP for %ld\n",
+			policy->cpu, freq_hz);
 		return PTR_ERR(opp);
 	}
 	vproc = dev_pm_opp_get_voltage(opp);
@@ -243,8 +245,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	if (old_vproc <= target_vproc) {
 		ret = mtk_cpufreq_set_voltage(info, target_vproc);
 		if (ret) {
-			pr_err("cpu%d: failed to scale up voltage!\n",
-			       policy->cpu);
+			dev_err(cpu_dev, "cpu%d: failed to scale up voltage!\n",
+				policy->cpu);
 			mtk_cpufreq_set_voltage(info, old_vproc);
 			goto out;
 		}
@@ -253,8 +255,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	/* Reparent the CPU clock to intermediate clock. */
 	ret = clk_set_parent(cpu_clk, info->inter_clk);
 	if (ret) {
-		pr_err("cpu%d: failed to re-parent cpu clock!\n",
-		       policy->cpu);
+		dev_err(cpu_dev, "cpu%d: failed to re-parent cpu clock!\n",
+			policy->cpu);
 		mtk_cpufreq_set_voltage(info, old_vproc);
 		goto out;
 	}
@@ -262,8 +264,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	/* Set the original PLL to target rate. */
 	ret = clk_set_rate(armpll, freq_hz);
 	if (ret) {
-		pr_err("cpu%d: failed to scale cpu clock rate!\n",
-		       policy->cpu);
+		dev_err(cpu_dev, "cpu%d: failed to scale cpu clock rate!\n",
+			policy->cpu);
 		clk_set_parent(cpu_clk, armpll);
 		mtk_cpufreq_set_voltage(info, old_vproc);
 		goto out;
@@ -272,8 +274,8 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	/* Set parent of CPU clock back to the original PLL. */
 	ret = clk_set_parent(cpu_clk, armpll);
 	if (ret) {
-		pr_err("cpu%d: failed to re-parent cpu clock!\n",
-		       policy->cpu);
+		dev_err(cpu_dev, "cpu%d: failed to re-parent cpu clock!\n",
+			policy->cpu);
 		mtk_cpufreq_set_voltage(info, inter_vproc);
 		goto out;
 	}
@@ -285,8 +287,9 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	if (vproc < inter_vproc || vproc < old_vproc) {
 		ret = mtk_cpufreq_set_voltage(info, vproc);
 		if (ret) {
-			pr_err("cpu%d: failed to scale down voltage!\n",
-			       policy->cpu);
+			dev_err(cpu_dev,
+				"cpu%d: failed to scale down voltage!\n",
+				policy->cpu);
 			clk_set_parent(cpu_clk, info->inter_clk);
 			clk_set_rate(armpll, old_freq_hz);
 			clk_set_parent(cpu_clk, armpll);
@@ -568,15 +571,17 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 
 	info = mtk_cpu_dvfs_info_lookup(policy->cpu);
 	if (!info) {
-		pr_err("dvfs info for cpu%d is not initialized.\n",
-		       policy->cpu);
+		dev_err(info->cpu_dev,
+			"dvfs info for cpu%d is not initialized.\n",
+			policy->cpu);
 		return -EINVAL;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table);
 	if (ret) {
-		pr_err("failed to init cpufreq table for cpu%d: %d\n",
-		       policy->cpu, ret);
+		dev_err(info->cpu_dev,
+			"failed to init cpufreq table for cpu%d: %d\n",
+			policy->cpu, ret);
 		return ret;
 	}
 
-- 
2.18.0

Re: [PATCH V2 15/15] cpufreq: mediatek: Use device print to show logs

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> Replace pr_* with dev_* to show logs.
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

That's very appreciated.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH V2 15/15] cpufreq: mediatek: Use device print to show logs

[Viresh Kumar]

On 08-04-22, 12:59, Rex-BC Chen wrote:
> Replace pr_* with dev_* to show logs.
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>  drivers/cpufreq/mediatek-cpufreq.c | 41 +++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 18 deletions(-)

One should always arrange the patches in this form:

- Fix bugs first (since they need to be applied first and need to go
  to stable kernels too).

- Trivial fixes next, like this one which you chose to be present at
  15/15. I would have applied this one right away, if it wasn't he
  last one, because of which we have conflicts now.

- Non-trivial patches later, so reviews on them don't affect other
  patches.

-- 
viresh

Re: [PATCH V2 15/15] cpufreq: mediatek: Use device print to show logs

[Rex-BC Chen]

On Mon, 2022-04-11 at 08:59 +0530, Viresh Kumar wrote:
> On 08-04-22, 12:59, Rex-BC Chen wrote:
> > Replace pr_* with dev_* to show logs.
> > 
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > ---
> >  drivers/cpufreq/mediatek-cpufreq.c | 41 +++++++++++++++++---------
> > ----
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> One should always arrange the patches in this form:
> 
> - Fix bugs first (since they need to be applied first and need to go
>   to stable kernels too).
> 
> - Trivial fixes next, like this one which you chose to be present at
>   15/15. I would have applied this one right away, if it wasn't he
>   last one, because of which we have conflicts now.
> 
> - Non-trivial patches later, so reviews on them don't affect other
>   patches.
> 
Hello Viresh,

Thanks for your review and suggestions.
I will put this patch to first in next version.

BRs,
Rex

Re: [PATCH V2 07/15] cpufreq: mediatek: Add opp notification for SVS support

[AngeloGioacchino Del Regno]

Il 08/04/22 06:59, Rex-BC Chen ha scritto:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> The Smart Voltage Scaling (SVS) is a hardware which calculates suitable
> SVS bank voltages to OPP voltage table.
> 
> When the SVS is enabled, cpufreq should listen to opp notification and do
> proper actions when receiving events of disable and voltage adjustment.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 90 +++++++++++++++++++++++++++---
>   1 file changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 472f4de29e5f..1369da62780a 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c

..snip..

> +
>   static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   {
>   	struct device *cpu_dev;
> @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
>   	dev_pm_opp_put(opp);
>   
> +	info->opp_cpu = cpu;
> +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
> +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);

You are registering this notifier, but never unregistering it.
Please unregister it in error conditions and also upon runtime removal of the
mediatek-cpufreq module.

-> dev_pm_opp_unregister_notifier()

Regards,
Angelo

> +	if (ret) {
> +		dev_warn(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu);
> +		goto out_disable_inter_clock;
> +	}
> +
> +	mutex_init(&info->reg_lock);
> +	info->opp_freq = clk_get_rate(info->cpu_clk);
> +
>   	/*
>   	 * If SRAM regulator is present, software "voltage tracking" is needed
>   	 * for this CPU power domain.

Re: [PATCH V2 07/15] cpufreq: mediatek: Add opp notification for SVS support

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>
> The Smart Voltage Scaling (SVS) is a hardware which calculates suitable
> SVS bank voltages to OPP voltage table.
>
> When the SVS is enabled, cpufreq should listen to opp notification and do
> proper actions when receiving events of disable and voltage adjustment.

So listenting for OPP notifications should be done only when SVS is enabled...

[...]

>  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  {
>  	struct device *cpu_dev;
> @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
>  	dev_pm_opp_put(opp);
>  
> +	info->opp_cpu = cpu;
> +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
> +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);

...but here youlisten to OPP notifications unconditionally.  Seems there
should be a check whether SVS is enabled before deciding to register.

Kevin

Re: [PATCH V2 07/15] cpufreq: mediatek: Add opp notification for SVS support

[Kevin Hilman]

Hi Rex,

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote:
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>> > 
>> > The Smart Voltage Scaling (SVS) is a hardware which calculates
>> > suitable
>> > SVS bank voltages to OPP voltage table.
>> > 
>> > When the SVS is enabled, cpufreq should listen to opp notification
>> > and do
>> > proper actions when receiving events of disable and voltage
>> > adjustment.
>> 
>> So listenting for OPP notifications should be done only when SVS is
>> enabled...
>> 
>
> Thanks for your review.
> Yes, the OPP notification is only called from MediaTek SVS.
>
>> [...]
>> 
>> >  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info,
>> > int cpu)
>> >  {
>> >  	struct device *cpu_dev;
>> > @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct
>> > mtk_cpu_dvfs_info *info, int cpu)
>> >  	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
>> >  	dev_pm_opp_put(opp);
>> >  
>> > +	info->opp_cpu = cpu;
>> > +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
>> > +	ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb);
>> 
>> ...but here youlisten to OPP notifications unconditionally.  Seems
>> there
>> should be a check whether SVS is enabled before deciding to register.
>> 
>> Kevin
>>
> Do you think it's ok that we wrap it with the SVS Kconfig define?
> like
> #ifdef CONFIG_MTK_SVS
> mtk_cpufreq_opp_notifier()
> ...
> dev_pm_opp_register_notifier()
> #endif

Generally, we don't like to see #ifdefs in C files[1].

But more importantly, compile-time check is not enough, because SVS
feature could be compiled into kernel, but not actually enabled for an
SoC (e.g. DT node not enabled, etc.) so checking this at compile time is
not enough.

Ideally, the SVSdriver should provide a function that allows others to
check if it's enabled.  That function needs to know not only if it's
compile in, but if it's enabled/running.  If SVS is not compiled in,
then that function just returns false.

Kevin

[1] https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef#ifdef-and-preprocessor-use-in-general

Re: [PATCH V2 07/15] cpufreq: mediatek: Add opp notification for SVS support

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote:
>> Hi Rex,
>> 
>> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> 
>> > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote:
>> > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>> > > 
>> > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>> > > > 
>> > > > The Smart Voltage Scaling (SVS) is a hardware which calculates
>> > > > suitable
>> > > > SVS bank voltages to OPP voltage table.
>> > > > 
>> > > > When the SVS is enabled, cpufreq should listen to opp
>> > > > notification
>> > > > and do
>> > > > proper actions when receiving events of disable and voltage
>> > > > adjustment.
>> > > 
>> > > So listenting for OPP notifications should be done only when SVS
>> > > is
>> > > enabled...
>> > > 
>> > 
>> > Thanks for your review.
>> > Yes, the OPP notification is only called from MediaTek SVS.
>> > 
>> > > [...]
>> > > 
>> > > >  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info
>> > > > *info,
>> > > > int cpu)
>> > > >  {
>> > > >  	struct device *cpu_dev;
>> > > > @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct
>> > > > mtk_cpu_dvfs_info *info, int cpu)
>> > > >  	info->intermediate_voltage =
>> > > > dev_pm_opp_get_voltage(opp);
>> > > >  	dev_pm_opp_put(opp);
>> > > >  
>> > > > +	info->opp_cpu = cpu;
>> > > > +	info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier;
>> > > > +	ret = dev_pm_opp_register_notifier(cpu_dev, &info-
>> > > > >opp_nb);
>> > > 
>> > > ...but here youlisten to OPP notifications
>> > > unconditionally.  Seems
>> > > there
>> > > should be a check whether SVS is enabled before deciding to
>> > > register.
>> > > 
>> > > Kevin
>> > > 
>> > 
>> > Do you think it's ok that we wrap it with the SVS Kconfig define?
>> > like
>> > #ifdef CONFIG_MTK_SVS
>> > mtk_cpufreq_opp_notifier()
>> > ...
>> > dev_pm_opp_register_notifier()
>> > #endif
>> 
>> Generally, we don't like to see #ifdefs in C files[1].
>> 
>> But more importantly, compile-time check is not enough, because SVS
>> feature could be compiled into kernel, but not actually enabled for
>> an
>> SoC (e.g. DT node not enabled, etc.) so checking this at compile time
>> is
>> not enough.
>> 
>> Ideally, the SVSdriver should provide a function that allows others
>> to
>> check if it's enabled.  That function needs to know not only if it's
>> compile in, but if it's enabled/running.  If SVS is not compiled in,
>> then that function just returns false.
>> 
>> Kevin
>> 
>> [1] 
>> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef*ifdef-and-preprocessor-use-in-general__;Iw!!CTRNKA9wMg0ARbw!z6SrEcQOLu2u-R1OLedrRUXHYXCzuQoK3F_h9Bhzv8jNFmjV5mdNVy41eND67CuV9uIS$
>> 
>
> Hello Kevin,
>
> After our internal discussion, we think the register of notifier should
> not be bound for certain module.
> If we provide the moethod to adjust voltage/disable opp, we think if
> anyone call dev_pm_opp_adjust_voltage and dev_pm_opp_disable, it could
> be used.

I don't think I understand what you mean.

Do you mean that this OPP notifier could be registered all the time,
even if SVS is not enabled?

That's fine with me.  If SVS is not compiled or enabled, then the
notifiers will never be called, so that's fine.

> May I ask what is your concern?

My concern was primarily that the changelog description did not match
the code.  The changelog says "when SVS is enabled, CPUfreq should
listen to OPP notifications."

But if I understand you correctly above, I think what you mean is that
CPUfreq should always listen to OPP notifications because there are
other users (e.g. SVS) that could change the OPP outside of this driver.

If that's what you mean, then I think the only thing to change is the
wording of the changelog.

Thanks,

Kevin

Re: [PATCH V2 00/15] cpufreq: mediatek: Cleanup and support MT8183 and MT8186

[Kevin Hilman]

Rex-BC Chen <rex-bc.chen@mediatek.com> writes:

> Cpufreq is a DVFS driver used for power saving to scale the clock frequency
> and supply the voltage for CPUs. This series do some cleanup for MediaTek
> cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI
> devfreq[3] which are supported in MT8183 and MT8186.

There's no upstream DT for MT8186 and there are no OPPs defined in the
upstream DT for MT8183.

In order to test this on mainline, could you provide a patch for MT8183
that adds OPPs to the DT so this can be tested with mainline?

Thanks,

Kevin

Re: [PATCH V2 00/15] cpufreq: mediatek: Cleanup and support MT8183 and MT8186

[Hsin-Yi Wang]

On Sat, Apr 9, 2022 at 5:11 AM Kevin Hilman <khilman@baylibre.com> wrote:
>
> Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
>
> > Cpufreq is a DVFS driver used for power saving to scale the clock frequency
> > and supply the voltage for CPUs. This series do some cleanup for MediaTek
> > cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI
> > devfreq[3] which are supported in MT8183 and MT8186.
>
> There's no upstream DT for MT8186 and there are no OPPs defined in the
> upstream DT for MT8183.
>
> In order to test this on mainline, could you provide a patch for MT8183
> that adds OPPs to the DT so this can be tested with mainline?
>
The DT change used in the downstream kernel is from here:
https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-9-git-send-email-andrew-sh.cheng@mediatek.com/
Might need some update (eg. add the cci property in cpu) though.
Rex, you can also include the 8183 DT change in the next version since
most of the mt8183 dts are in the mainline.

Thanks

> Thanks,
>
> Kevin

Re: [PATCH V2 00/15] cpufreq: mediatek: Cleanup and support MT8183 and MT8186

[Rex-BC Chen]

On Sat, 2022-04-09 at 09:05 +0800, Hsin-Yi Wang wrote:
> On Sat, Apr 9, 2022 at 5:11 AM Kevin Hilman <khilman@baylibre.com>
> wrote:
> > 
> > Rex-BC Chen <rex-bc.chen@mediatek.com> writes:
> > 
> > > Cpufreq is a DVFS driver used for power saving to scale the clock
> > > frequency
> > > and supply the voltage for CPUs. This series do some cleanup for
> > > MediaTek
> > > cpufreq drivers and add support for MediaTek SVS[2] and MediaTek
> > > CCI
> > > devfreq[3] which are supported in MT8183 and MT8186.
> > 
> > There's no upstream DT for MT8186 and there are no OPPs defined in
> > the
> > upstream DT for MT8183.
> > 
> > In order to test this on mainline, could you provide a patch for
> > MT8183
> > that adds OPPs to the DT so this can be tested with mainline?
> > 
> 
> The DT change used in the downstream kernel is from here:
> 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-9-git-send-email-andrew-sh.cheng@mediatek.com/__;!!CTRNKA9wMg0ARbw!ykRlVJPl8TZWMCfnAzLnqhtn3iqXeHh8f6tMBWpneZuJPPmJTGEDIiEgv-R_Q4gVunnp$
>  
> Might need some update (eg. add the cci property in cpu) though.
> Rex, you can also include the 8183 DT change in the next version
> since
> most of the mt8183 dts are in the mainline.
> 
> Thanks
> 
> > Thanks,
> > 
> > Kevin

Hello Kevin and Hsinyi,

OK I will add dts part of cpufreq for MT8183 in next version.
And I think the cci part will be upstreamed in cci seriues.

BRs,
Rex