diff mbox series

[V3,11/15] cpufreq: mediatek: Link CCI device to CPU

Message ID 20220415055916.28350-12-rex-bc.chen@mediatek.com (mailing list archive)
State Superseded, archived
Headers show
Series cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand

Commit Message

Rex-BC Chen (陳柏辰) April 15, 2022, 5:59 a.m. UTC
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.

Before CCI devfreq is ready, we record the voltage when booting to
kernel and use the max(cpu target voltage, booting voltage) to
prevent cpufreq adjust to the lower voltage which will cause the CCI
crash because of high frequency and low voltage.

- 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>
Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
---
 drivers/cpufreq/mediatek-cpufreq.c | 80 +++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno April 15, 2022, 12:24 p.m. UTC | #1
Il 15/04/22 07: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.
> 
> Before CCI devfreq is ready, we record the voltage when booting to
> kernel and use the max(cpu target voltage, booting voltage) to
> prevent cpufreq adjust to the lower voltage which will cause the CCI
> crash because of high frequency and low voltage.
> 
> - 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>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

I am enthusiast to see that the solution that I've proposed was welcome!

I only have one nit on this patch, check below:

> ---
>   drivers/cpufreq/mediatek-cpufreq.c | 80 +++++++++++++++++++++++++++++-
>   1 file changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index d4c00237e862..dd3f739fede1 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c

..snip..

> @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>   	vproc = dev_pm_opp_get_voltage(opp);
>   	dev_pm_opp_put(opp);
>   
> +	/*
> +	 * If MediaTek cci is supported but is not ready, we will use the value
> +	 * of max(target cpu voltage, booting voltage) to prevent high freqeuncy
> +	 * low voltage crash.
> +	 */
> +	if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info))
> +		vproc = max(vproc, info->vproc_on_boot);
> +
>   	/*
>   	 * If the new voltage or the intermediate voltage is higher than the
>   	 * current voltage, scale up voltage first.

..snip..

> @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>   	if (ret)
>   		goto out_disable_mux_clock;
>   
> +	info->vproc_on_boot = regulator_get_voltage(info->proc_reg);

This result is used only if we use ccifreq, so this should be enclosed in an if
condition: this will spare us some (yes, small) time on devices that don't use it.

	if (info->soc_data->ccifreq_supported) {
		info->vproc_on_boot = regulator_get_voltage(info->proc_reg);
		if (info->vproc_on_boot < 0) {
			dev_err(....
			goto ..
		}
	}

P.S.: While at it, since the maximum width is 100 columns, the dev_err() call fits,
so don't break that line!

After the requested change:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Rex-BC Chen (陳柏辰) April 18, 2022, 1:45 a.m. UTC | #2
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote:
> Il 15/04/22 07: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.
> > 
> > Before CCI devfreq is ready, we record the voltage when booting to
> > kernel and use the max(cpu target voltage, booting voltage) to
> > prevent cpufreq adjust to the lower voltage which will cause the
> > CCI
> > crash because of high frequency and low voltage.
> > 
> > - 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>
> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> 
> I am enthusiast to see that the solution that I've proposed was
> welcome!
> 
> I only have one nit on this patch, check below:
> 

Hello Angelo,

Thanks for your advice for this modification.
I also reply to Kevin and describe this solution.
Let's wait for Kevin's feedback and other suggestion.

> > ---
> >   drivers/cpufreq/mediatek-cpufreq.c | 80
> > +++++++++++++++++++++++++++++-
> >   1 file changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c
> > b/drivers/cpufreq/mediatek-cpufreq.c
> > index d4c00237e862..dd3f739fede1 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> 
> ..snip..
> 
> > @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct
> > cpufreq_policy *policy,
> >   	vproc = dev_pm_opp_get_voltage(opp);
> >   	dev_pm_opp_put(opp);
> >   
> > +	/*
> > +	 * If MediaTek cci is supported but is not ready, we will use
> > the value
> > +	 * of max(target cpu voltage, booting voltage) to prevent high
> > freqeuncy
> > +	 * low voltage crash.
> > +	 */
> > +	if (info->soc_data->ccifreq_supported &&
> > !is_ccifreq_ready(info))
> > +		vproc = max(vproc, info->vproc_on_boot);
> > +
> >   	/*
> >   	 * If the new voltage or the intermediate voltage is higher
> > than the
> >   	 * current voltage, scale up voltage first.
> 
> ..snip..
> 
> > @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct
> > mtk_cpu_dvfs_info *info, int cpu)
> >   	if (ret)
> >   		goto out_disable_mux_clock;
> >   
> > +	info->vproc_on_boot = regulator_get_voltage(info->proc_reg);
> 
> This result is used only if we use ccifreq, so this should be
> enclosed in an if
> condition: this will spare us some (yes, small) time on devices that
> don't use it.
> 
> 	if (info->soc_data->ccifreq_supported) {
> 		info->vproc_on_boot = regulator_get_voltage(info-
> >proc_reg);
> 		if (info->vproc_on_boot < 0) {
> 			dev_err(....
> 			goto ..
> 		}
> 	}
> 
> P.S.: While at it, since the maximum width is 100 columns, the
> dev_err() call fits,
> so don't break that line!
> 
> After the requested change:
> 
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>

I will add this in next version if there is no any suggestion for this
patch.

Thanks!

BRs,
Rex
Kevin Hilman April 20, 2022, 6:09 p.m. UTC | #3
Hi Rex,

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.
>
> Before CCI devfreq is ready, we record the voltage when booting to
> kernel and use the max(cpu target voltage, booting voltage) to
> prevent cpufreq adjust to the lower voltage which will cause the CCI
> crash because of high frequency and low voltage.
>
> - 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>
> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>

The solution of keeping the max of the CPU voltage from OPP and boot-up
voltage makes sense until CCI is ready.  Thank you for the rework and
the detailed technical explanations.

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin
diff mbox series

Patch

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index d4c00237e862..dd3f739fede1 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 ccifreq_supported;
 };
 
 /*
@@ -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;
@@ -45,6 +47,7 @@  struct mtk_cpu_dvfs_info {
 	struct list_head list_head;
 	int intermediate_voltage;
 	bool need_voltage_tracking;
+	int vproc_on_boot;
 	int pre_vproc;
 	/* Avoid race condition for regulators between notify and policy */
 	struct mutex reg_lock;
@@ -52,6 +55,7 @@  struct mtk_cpu_dvfs_info {
 	int opp_cpu;
 	unsigned long opp_freq;
 	const struct mtk_cpufreq_platform_data *soc_data;
+	bool ccifreq_bound;
 };
 
 static struct platform_device *cpufreq_pdev;
@@ -188,6 +192,28 @@  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->ccifreq_bound)
+		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->ccifreq_bound = true;
+
+	return true;
+}
+
 static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 				  unsigned int index)
 {
@@ -225,6 +251,14 @@  static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	vproc = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
 
+	/*
+	 * If MediaTek cci is supported but is not ready, we will use the value
+	 * of max(target cpu voltage, booting voltage) to prevent high freqeuncy
+	 * low voltage crash.
+	 */
+	if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info))
+		vproc = max(vproc, info->vproc_on_boot);
+
 	/*
 	 * If the new voltage or the intermediate voltage is higher than the
 	 * current voltage, scale up voltage first.
@@ -346,6 +380,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, "mediatek,cci", 0);
+	if (IS_ERR_OR_NULL(np))
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+	if (IS_ERR_OR_NULL(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;
@@ -360,6 +411,16 @@  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	}
 	info->cpu_dev = cpu_dev;
 
+	info->ccifreq_bound = false;
+	if (info->soc_data->ccifreq_supported) {
+		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);
@@ -423,6 +484,13 @@  static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 	if (ret)
 		goto out_disable_mux_clock;
 
+	info->vproc_on_boot = regulator_get_voltage(info->proc_reg);
+	if (info->vproc_on_boot < 0) {
+		dev_err(info->cpu_dev,
+			"invalid Vproc value: %d\n", info->vproc_on_boot);
+		goto out_disable_inter_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);
@@ -623,6 +691,16 @@  static const struct mtk_cpufreq_platform_data mt2701_platform_data = {
 	.proc_max_volt = 1150000,
 	.sram_min_volt = 0,
 	.sram_max_volt = 1150000,
+	.ccifreq_supported = 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,
+	.ccifreq_supported = true,
 };
 
 /* List of machines supported by this driver */
@@ -635,7 +713,7 @@  static const struct of_device_id mtk_cpufreq_machines[] __initconst = {
 	{ .compatible = "mediatek,mt817x", .data = &mt2701_platform_data },
 	{ .compatible = "mediatek,mt8173", .data = &mt2701_platform_data },
 	{ .compatible = "mediatek,mt8176", .data = &mt2701_platform_data },
-	{ .compatible = "mediatek,mt8183", .data = &mt2701_platform_data },
+	{ .compatible = "mediatek,mt8183", .data = &mt8183_platform_data },
 	{ .compatible = "mediatek,mt8365", .data = &mt2701_platform_data },
 	{ .compatible = "mediatek,mt8516", .data = &mt2701_platform_data },
 	{ }