[PATCH v5 0/3] Enhance SVS's robustness

[PATCH v5 0/3] Enhance SVS's robustness

[Roger Lu]

SVS driver got accepted upstream but still has room to be improved.
Therefore, we add these patches to fix issues and coding style.

Change since v4:
- Rebase unmerged patches and resend to patchwork

Tested on:
MT8183 Platform (kernel v5.10)
MT8192 Platform (kernel v5.4)

Roger Lu (3):
  soc: mediatek: mtk-svs: use svs get efuse common function
  soc: mediatek: mtk-svs: use common function to disable restore
    voltages
  soc: mediatek: mtk-svs: add thermal voltage compensation if needed

 drivers/soc/mediatek/mtk-svs.c | 142 +++++++++++++--------------------
 1 file changed, 56 insertions(+), 86 deletions(-)

[PATCH v5 1/3] soc: mediatek: mtk-svs: use svs get efuse common function

[Roger Lu]

SVS might need to read both svs efuse and thermal efuse on the probe flow.
Therefore, add a common efuse read function to remove the superfluous
codes.

Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 71 ++++++++++++----------------------
 1 file changed, 25 insertions(+), 46 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index f26eb2f637d5..c9899f5df60a 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1729,26 +1729,28 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
 	return 0;
 }
 
-static int svs_thermal_efuse_get_data(struct svs_platform *svsp)
+static int svs_get_efuse_data(struct svs_platform *svsp,
+			      const char *nvmem_cell_name,
+			      u32 **svsp_efuse, size_t *svsp_efuse_max)
 {
 	struct nvmem_cell *cell;
 
-	/* Thermal efuse parsing */
-	cell = nvmem_cell_get(svsp->dev, "t-calibration-data");
+	cell = nvmem_cell_get(svsp->dev, nvmem_cell_name);
 	if (IS_ERR_OR_NULL(cell)) {
-		dev_err(svsp->dev, "no \"t-calibration-data\"? %ld\n", PTR_ERR(cell));
+		dev_err(svsp->dev, "no \"%s\"? %ld\n",
+			nvmem_cell_name, PTR_ERR(cell));
 		return PTR_ERR(cell);
 	}
 
-	svsp->tefuse = nvmem_cell_read(cell, &svsp->tefuse_max);
-	if (IS_ERR(svsp->tefuse)) {
-		dev_err(svsp->dev, "cannot read thermal efuse: %ld\n",
-			PTR_ERR(svsp->tefuse));
+	*svsp_efuse = nvmem_cell_read(cell, svsp_efuse_max);
+	if (IS_ERR(*svsp_efuse)) {
+		dev_err(svsp->dev, "cannot read \"%s\" efuse: %ld\n",
+			nvmem_cell_name, PTR_ERR(*svsp_efuse));
 		nvmem_cell_put(cell);
-		return PTR_ERR(svsp->tefuse);
+		return PTR_ERR(*svsp_efuse);
 	}
 
-	svsp->tefuse_max /= sizeof(u32);
+	*svsp_efuse_max /= sizeof(u32);
 	nvmem_cell_put(cell);
 
 	return 0;
@@ -1796,7 +1798,8 @@ static bool svs_mt8192_efuse_parsing(struct svs_platform *svsp)
 		svsb->vmax += svsb->dvt_fixed;
 	}
 
-	ret = svs_thermal_efuse_get_data(svsp);
+	ret = svs_get_efuse_data(svsp, "t-calibration-data",
+				 &svsp->tefuse, &svsp->tefuse_max);
 	if (ret)
 		return false;
 
@@ -1901,7 +1904,8 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 		}
 	}
 
-	ret = svs_thermal_efuse_get_data(svsp);
+	ret = svs_get_efuse_data(svsp, "t-calibration-data",
+				 &svsp->tefuse, &svsp->tefuse_max);
 	if (ret)
 		return false;
 
@@ -2003,32 +2007,6 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 	return true;
 }
 
-static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
-{
-	struct nvmem_cell *cell;
-
-	/* Get svs efuse by nvmem */
-	cell = nvmem_cell_get(svsp->dev, "svs-calibration-data");
-	if (IS_ERR(cell)) {
-		dev_err(svsp->dev, "no \"svs-calibration-data\"? %ld\n",
-			PTR_ERR(cell));
-		return false;
-	}
-
-	svsp->efuse = nvmem_cell_read(cell, &svsp->efuse_max);
-	if (IS_ERR(svsp->efuse)) {
-		dev_err(svsp->dev, "cannot read svs efuse: %ld\n",
-			PTR_ERR(svsp->efuse));
-		nvmem_cell_put(cell);
-		return false;
-	}
-
-	svsp->efuse_max /= sizeof(u32);
-	nvmem_cell_put(cell);
-
-	return true;
-}
-
 static struct device *svs_get_subsys_device(struct svs_platform *svsp,
 					    const char *node_name)
 {
@@ -2364,8 +2342,9 @@ static int svs_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (!svs_is_efuse_data_correct(svsp)) {
-		dev_notice(svsp->dev, "efuse data isn't correct\n");
+	ret = svs_get_efuse_data(svsp, "svs-calibration-data",
+				 &svsp->efuse, &svsp->efuse_max);
+	if (ret) {
 		ret = -EPERM;
 		goto svs_probe_free_efuse;
 	}
@@ -2373,19 +2352,19 @@ static int svs_probe(struct platform_device *pdev)
 	if (!svsp_data->efuse_parsing(svsp)) {
 		dev_err(svsp->dev, "efuse data parsing failed\n");
 		ret = -EPERM;
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 	}
 
 	ret = svs_bank_resource_setup(svsp);
 	if (ret) {
 		dev_err(svsp->dev, "svs bank resource setup fail: %d\n", ret);
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 	}
 
 	svsp_irq = platform_get_irq(pdev, 0);
 	if (svsp_irq < 0) {
 		ret = svsp_irq;
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 	}
 
 	svsp->main_clk = devm_clk_get(svsp->dev, "main");
@@ -2393,13 +2372,13 @@ static int svs_probe(struct platform_device *pdev)
 		dev_err(svsp->dev, "failed to get clock: %ld\n",
 			PTR_ERR(svsp->main_clk));
 		ret = PTR_ERR(svsp->main_clk);
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 	}
 
 	ret = clk_prepare_enable(svsp->main_clk);
 	if (ret) {
 		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 	}
 
 	svsp->base = of_iomap(svsp->dev->of_node, 0);
@@ -2439,7 +2418,7 @@ static int svs_probe(struct platform_device *pdev)
 svs_probe_clk_disable:
 	clk_disable_unprepare(svsp->main_clk);
 
-svs_probe_free_resource:
+svs_probe_free_tefuse:
 	if (!IS_ERR_OR_NULL(svsp->tefuse))
 		kfree(svsp->tefuse);
 
-- 
2.18.0

Re: [PATCH v5 1/3] soc: mediatek: mtk-svs: use svs get efuse common function

[Matthias Brugger]

On 02/02/2023 13:41, Roger Lu wrote:
> SVS might need to read both svs efuse and thermal efuse on the probe flow.
> Therefore, add a common efuse read function to remove the superfluous
> codes.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Queued for the next merge window.

Thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 71 ++++++++++++----------------------
>   1 file changed, 25 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index f26eb2f637d5..c9899f5df60a 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1729,26 +1729,28 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   	return 0;
>   }
>   
> -static int svs_thermal_efuse_get_data(struct svs_platform *svsp)
> +static int svs_get_efuse_data(struct svs_platform *svsp,
> +			      const char *nvmem_cell_name,
> +			      u32 **svsp_efuse, size_t *svsp_efuse_max)
>   {
>   	struct nvmem_cell *cell;
>   
> -	/* Thermal efuse parsing */
> -	cell = nvmem_cell_get(svsp->dev, "t-calibration-data");
> +	cell = nvmem_cell_get(svsp->dev, nvmem_cell_name);
>   	if (IS_ERR_OR_NULL(cell)) {
> -		dev_err(svsp->dev, "no \"t-calibration-data\"? %ld\n", PTR_ERR(cell));
> +		dev_err(svsp->dev, "no \"%s\"? %ld\n",
> +			nvmem_cell_name, PTR_ERR(cell));
>   		return PTR_ERR(cell);
>   	}
>   
> -	svsp->tefuse = nvmem_cell_read(cell, &svsp->tefuse_max);
> -	if (IS_ERR(svsp->tefuse)) {
> -		dev_err(svsp->dev, "cannot read thermal efuse: %ld\n",
> -			PTR_ERR(svsp->tefuse));
> +	*svsp_efuse = nvmem_cell_read(cell, svsp_efuse_max);
> +	if (IS_ERR(*svsp_efuse)) {
> +		dev_err(svsp->dev, "cannot read \"%s\" efuse: %ld\n",
> +			nvmem_cell_name, PTR_ERR(*svsp_efuse));
>   		nvmem_cell_put(cell);
> -		return PTR_ERR(svsp->tefuse);
> +		return PTR_ERR(*svsp_efuse);
>   	}
>   
> -	svsp->tefuse_max /= sizeof(u32);
> +	*svsp_efuse_max /= sizeof(u32);
>   	nvmem_cell_put(cell);
>   
>   	return 0;
> @@ -1796,7 +1798,8 @@ static bool svs_mt8192_efuse_parsing(struct svs_platform *svsp)
>   		svsb->vmax += svsb->dvt_fixed;
>   	}
>   
> -	ret = svs_thermal_efuse_get_data(svsp);
> +	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> +				 &svsp->tefuse, &svsp->tefuse_max);
>   	if (ret)
>   		return false;
>   
> @@ -1901,7 +1904,8 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		}
>   	}
>   
> -	ret = svs_thermal_efuse_get_data(svsp);
> +	ret = svs_get_efuse_data(svsp, "t-calibration-data",
> +				 &svsp->tefuse, &svsp->tefuse_max);
>   	if (ret)
>   		return false;
>   
> @@ -2003,32 +2007,6 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   	return true;
>   }
>   
> -static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
> -{
> -	struct nvmem_cell *cell;
> -
> -	/* Get svs efuse by nvmem */
> -	cell = nvmem_cell_get(svsp->dev, "svs-calibration-data");
> -	if (IS_ERR(cell)) {
> -		dev_err(svsp->dev, "no \"svs-calibration-data\"? %ld\n",
> -			PTR_ERR(cell));
> -		return false;
> -	}
> -
> -	svsp->efuse = nvmem_cell_read(cell, &svsp->efuse_max);
> -	if (IS_ERR(svsp->efuse)) {
> -		dev_err(svsp->dev, "cannot read svs efuse: %ld\n",
> -			PTR_ERR(svsp->efuse));
> -		nvmem_cell_put(cell);
> -		return false;
> -	}
> -
> -	svsp->efuse_max /= sizeof(u32);
> -	nvmem_cell_put(cell);
> -
> -	return true;
> -}
> -
>   static struct device *svs_get_subsys_device(struct svs_platform *svsp,
>   					    const char *node_name)
>   {
> @@ -2364,8 +2342,9 @@ static int svs_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> -	if (!svs_is_efuse_data_correct(svsp)) {
> -		dev_notice(svsp->dev, "efuse data isn't correct\n");
> +	ret = svs_get_efuse_data(svsp, "svs-calibration-data",
> +				 &svsp->efuse, &svsp->efuse_max);
> +	if (ret) {
>   		ret = -EPERM;
>   		goto svs_probe_free_efuse;
>   	}
> @@ -2373,19 +2352,19 @@ static int svs_probe(struct platform_device *pdev)
>   	if (!svsp_data->efuse_parsing(svsp)) {
>   		dev_err(svsp->dev, "efuse data parsing failed\n");
>   		ret = -EPERM;
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   	}
>   
>   	ret = svs_bank_resource_setup(svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "svs bank resource setup fail: %d\n", ret);
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   	}
>   
>   	svsp_irq = platform_get_irq(pdev, 0);
>   	if (svsp_irq < 0) {
>   		ret = svsp_irq;
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   	}
>   
>   	svsp->main_clk = devm_clk_get(svsp->dev, "main");
> @@ -2393,13 +2372,13 @@ static int svs_probe(struct platform_device *pdev)
>   		dev_err(svsp->dev, "failed to get clock: %ld\n",
>   			PTR_ERR(svsp->main_clk));
>   		ret = PTR_ERR(svsp->main_clk);
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   	}
>   
>   	ret = clk_prepare_enable(svsp->main_clk);
>   	if (ret) {
>   		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   	}
>   
>   	svsp->base = of_iomap(svsp->dev->of_node, 0);
> @@ -2439,7 +2418,7 @@ static int svs_probe(struct platform_device *pdev)
>   svs_probe_clk_disable:
>   	clk_disable_unprepare(svsp->main_clk);
>   
> -svs_probe_free_resource:
> +svs_probe_free_tefuse:
>   	if (!IS_ERR_OR_NULL(svsp->tefuse))
>   		kfree(svsp->tefuse);
>   

[PATCH v5 2/3] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Roger Lu]

The timing of disabling SVS bank and restore default voltage is more
than one place. Therefore, add a common function to use for removing
the superfluous codes.

Signed-off-by: Roger Lu <roger.lu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-svs.c | 54 ++++++++++++++--------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index c9899f5df60a..299f580847bd 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -624,6 +624,25 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 	return ret;
 }
 
+static void svs_bank_disable_and_restore_default_volts(struct svs_platform *svsp,
+						       struct svs_bank *svsb)
+{
+	unsigned long flags;
+
+	if (svsb->mode_support == SVSB_MODE_ALL_DISABLE)
+		return;
+
+	spin_lock_irqsave(&svs_lock, flags);
+	svsp->pbank = svsb;
+	svs_switch_bank(svsp);
+	svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
+	svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
+	spin_unlock_irqrestore(&svs_lock, flags);
+
+	svsb->phase = SVSB_PHASE_ERROR;
+	svs_adjust_pm_opp_volts(svsb);
+}
+
 #ifdef CONFIG_DEBUG_FS
 static int svs_dump_debug_show(struct seq_file *m, void *p)
 {
@@ -700,7 +719,6 @@ static ssize_t svs_enable_debug_write(struct file *filp,
 {
 	struct svs_bank *svsb = file_inode(filp)->i_private;
 	struct svs_platform *svsp = dev_get_drvdata(svsb->dev);
-	unsigned long flags;
 	int enabled, ret;
 	char *buf = NULL;
 
@@ -716,16 +734,8 @@ static ssize_t svs_enable_debug_write(struct file *filp,
 		return ret;
 
 	if (!enabled) {
-		spin_lock_irqsave(&svs_lock, flags);
-		svsp->pbank = svsb;
+		svs_bank_disable_and_restore_default_volts(svsp, svsb);
 		svsb->mode_support = SVSB_MODE_ALL_DISABLE;
-		svs_switch_bank(svsp);
-		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
-		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
-		spin_unlock_irqrestore(&svs_lock, flags);
-
-		svsb->phase = SVSB_PHASE_ERROR;
-		svs_adjust_pm_opp_volts(svsb);
 	}
 
 	kfree(buf);
@@ -1508,16 +1518,7 @@ static int svs_init02(struct svs_platform *svsp)
 out_of_init02:
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
-
-		spin_lock_irqsave(&svs_lock, flags);
-		svsp->pbank = svsb;
-		svs_switch_bank(svsp);
-		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
-		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
-		spin_unlock_irqrestore(&svs_lock, flags);
-
-		svsb->phase = SVSB_PHASE_ERROR;
-		svs_adjust_pm_opp_volts(svsb);
+		svs_bank_disable_and_restore_default_volts(svsp, svsb);
 	}
 
 	return ret;
@@ -1563,23 +1564,12 @@ static int svs_suspend(struct device *dev)
 {
 	struct svs_platform *svsp = dev_get_drvdata(dev);
 	struct svs_bank *svsb;
-	unsigned long flags;
 	int ret;
 	u32 idx;
 
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
-
-		/* This might wait for svs_isr() process */
-		spin_lock_irqsave(&svs_lock, flags);
-		svsp->pbank = svsb;
-		svs_switch_bank(svsp);
-		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
-		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
-		spin_unlock_irqrestore(&svs_lock, flags);
-
-		svsb->phase = SVSB_PHASE_ERROR;
-		svs_adjust_pm_opp_volts(svsb);
+		svs_bank_disable_and_restore_default_volts(svsp, svsb);
 	}
 
 	ret = reset_control_assert(svsp->rst);
-- 
2.18.0

Re: [PATCH v5 2/3] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Matthias Brugger]

On 02/02/2023 13:41, Roger Lu wrote:
> The timing of disabling SVS bank and restore default voltage is more
> than one place. Therefore, add a common function to use for removing
> the superfluous codes.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Queued for the next merge window.

Thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 54 ++++++++++++++--------------------
>   1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index c9899f5df60a..299f580847bd 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -624,6 +624,25 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   	return ret;
>   }
>   
> +static void svs_bank_disable_and_restore_default_volts(struct svs_platform *svsp,
> +						       struct svs_bank *svsb)
> +{
> +	unsigned long flags;
> +
> +	if (svsb->mode_support == SVSB_MODE_ALL_DISABLE)
> +		return;
> +
> +	spin_lock_irqsave(&svs_lock, flags);
> +	svsp->pbank = svsb;
> +	svs_switch_bank(svsp);
> +	svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
> +	svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
> +	spin_unlock_irqrestore(&svs_lock, flags);
> +
> +	svsb->phase = SVSB_PHASE_ERROR;
> +	svs_adjust_pm_opp_volts(svsb);
> +}
> +
>   #ifdef CONFIG_DEBUG_FS
>   static int svs_dump_debug_show(struct seq_file *m, void *p)
>   {
> @@ -700,7 +719,6 @@ static ssize_t svs_enable_debug_write(struct file *filp,
>   {
>   	struct svs_bank *svsb = file_inode(filp)->i_private;
>   	struct svs_platform *svsp = dev_get_drvdata(svsb->dev);
> -	unsigned long flags;
>   	int enabled, ret;
>   	char *buf = NULL;
>   
> @@ -716,16 +734,8 @@ static ssize_t svs_enable_debug_write(struct file *filp,
>   		return ret;
>   
>   	if (!enabled) {
> -		spin_lock_irqsave(&svs_lock, flags);
> -		svsp->pbank = svsb;
> +		svs_bank_disable_and_restore_default_volts(svsp, svsb);
>   		svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> -		svs_switch_bank(svsp);
> -		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
> -		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
> -		spin_unlock_irqrestore(&svs_lock, flags);
> -
> -		svsb->phase = SVSB_PHASE_ERROR;
> -		svs_adjust_pm_opp_volts(svsb);
>   	}
>   
>   	kfree(buf);
> @@ -1508,16 +1518,7 @@ static int svs_init02(struct svs_platform *svsp)
>   out_of_init02:
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> -
> -		spin_lock_irqsave(&svs_lock, flags);
> -		svsp->pbank = svsb;
> -		svs_switch_bank(svsp);
> -		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
> -		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
> -		spin_unlock_irqrestore(&svs_lock, flags);
> -
> -		svsb->phase = SVSB_PHASE_ERROR;
> -		svs_adjust_pm_opp_volts(svsb);
> +		svs_bank_disable_and_restore_default_volts(svsp, svsb);
>   	}
>   
>   	return ret;
> @@ -1563,23 +1564,12 @@ static int svs_suspend(struct device *dev)
>   {
>   	struct svs_platform *svsp = dev_get_drvdata(dev);
>   	struct svs_bank *svsb;
> -	unsigned long flags;
>   	int ret;
>   	u32 idx;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> -
> -		/* This might wait for svs_isr() process */
> -		spin_lock_irqsave(&svs_lock, flags);
> -		svsp->pbank = svsb;
> -		svs_switch_bank(svsp);
> -		svs_writel_relaxed(svsp, SVSB_PTPEN_OFF, SVSEN);
> -		svs_writel_relaxed(svsp, SVSB_INTSTS_VAL_CLEAN, INTSTS);
> -		spin_unlock_irqrestore(&svs_lock, flags);
> -
> -		svsb->phase = SVSB_PHASE_ERROR;
> -		svs_adjust_pm_opp_volts(svsb);
> +		svs_bank_disable_and_restore_default_volts(svsp, svsb);
>   	}
>   
>   	ret = reset_control_assert(svsp->rst);

[PATCH v5 3/3] soc: mediatek: mtk-svs: add thermal voltage compensation if needed

[Roger Lu]

Some extreme test environment may keep IC temperature very low or very high
during system boot stage. For stability concern, we add thermal voltage
compenstation if needed no matter svs bank phase is in init02 or mon mode.

Signed-off-by: Roger Lu <roger.lu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-svs.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 299f580847bd..e104866d1ab5 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -558,7 +558,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 	}
 
 	/* Get thermal effect */
-	if (svsb->phase == SVSB_PHASE_MON) {
+	if (!IS_ERR_OR_NULL(svsb->tzd)) {
 		ret = thermal_zone_get_temp(svsb->tzd, &tzone_temp);
 		if (ret || (svsb->temp > SVSB_TEMP_UPPER_BOUND &&
 			    svsb->temp < SVSB_TEMP_LOWER_BOUND)) {
@@ -573,7 +573,8 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 			temp_voffset += svsb->tzone_ltemp_voffset;
 
 		/* 2-line bank update all opp volts when running mon mode */
-		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
+		if (svsb->phase == SVSB_PHASE_MON && (svsb->type == SVSB_HIGH ||
+						      svsb->type == SVSB_LOW)) {
 			opp_start = 0;
 			opp_stop = svsb->opp_count;
 		}
@@ -589,11 +590,6 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 			/* do nothing */
 			goto unlock_mutex;
 		case SVSB_PHASE_INIT02:
-			svsb_volt = max(svsb->volt[i], svsb->vmin);
-			opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
-							     svsb->volt_step,
-							     svsb->volt_base);
-			break;
 		case SVSB_PHASE_MON:
 			svsb_volt = max(svsb->volt[i] + temp_voffset, svsb->vmin);
 			opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
@@ -1683,7 +1679,7 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
 			}
 		}
 
-		if (svsb->mode_support & SVSB_MODE_MON) {
+		if (!IS_ERR_OR_NULL(svsb->tzone_name)) {
 			svsb->tzd = thermal_zone_get_zone_by_name(svsb->tzone_name);
 			if (IS_ERR(svsb->tzd)) {
 				dev_err(svsb->dev, "cannot get \"%s\" thermal zone\n",
@@ -2127,6 +2123,7 @@ static struct svs_bank svs_mt8192_banks[] = {
 		.type			= SVSB_LOW,
 		.set_freq_pct		= svs_set_bank_freq_pct_v3,
 		.get_volts		= svs_get_bank_volts_v3,
+		.tzone_name		= "gpu1",
 		.volt_flags		= SVSB_REMOVE_DVTFIXED_VOLT,
 		.mode_support		= SVSB_MODE_INIT02,
 		.opp_count		= MAX_OPP_ENTRIES,
@@ -2144,6 +2141,10 @@ static struct svs_bank svs_mt8192_banks[] = {
 		.core_sel		= 0x0fff0100,
 		.int_st			= BIT(0),
 		.ctl0			= 0x00540003,
+		.tzone_htemp		= 85000,
+		.tzone_htemp_voffset	= 0,
+		.tzone_ltemp		= 25000,
+		.tzone_ltemp_voffset	= 7,
 	},
 	{
 		.sw_id			= SVSB_GPU,
-- 
2.18.0

Re: [PATCH v5 3/3] soc: mediatek: mtk-svs: add thermal voltage compensation if needed

[Matthias Brugger]

Hi Roger,

I have some doubts, please see below.

On 02/02/2023 13:41, Roger Lu wrote:
> Some extreme test environment may keep IC temperature very low or very high
> during system boot stage. For stability concern, we add thermal voltage
> compenstation if needed no matter svs bank phase is in init02 or mon mode.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-svs.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 299f580847bd..e104866d1ab5 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -558,7 +558,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   	}
>   
>   	/* Get thermal effect */
> -	if (svsb->phase == SVSB_PHASE_MON) {
> +	if (!IS_ERR_OR_NULL(svsb->tzd)) {
>   		ret = thermal_zone_get_temp(svsb->tzd, &tzone_temp);
>   		if (ret || (svsb->temp > SVSB_TEMP_UPPER_BOUND &&
>   			    svsb->temp < SVSB_TEMP_LOWER_BOUND)) {
> @@ -573,7 +573,8 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   			temp_voffset += svsb->tzone_ltemp_voffset;
>   
>   		/* 2-line bank update all opp volts when running mon mode */
> -		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
> +		if (svsb->phase == SVSB_PHASE_MON && (svsb->type == SVSB_HIGH ||
> +						      svsb->type == SVSB_LOW)) {
>   			opp_start = 0;
>   			opp_stop = svsb->opp_count;
>   		}
> @@ -589,11 +590,6 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   			/* do nothing */
>   			goto unlock_mutex;
>   		case SVSB_PHASE_INIT02:
> -			svsb_volt = max(svsb->volt[i], svsb->vmin);
> -			opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
> -							     svsb->volt_step,
> -							     svsb->volt_base);
> -			break;
>   		case SVSB_PHASE_MON:
>   			svsb_volt = max(svsb->volt[i] + temp_voffset, svsb->vmin);
>   			opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
> @@ -1683,7 +1679,7 @@ static int svs_bank_resource_setup(struct svs_platform *svsp)
>   			}
>   		}
>   
> -		if (svsb->mode_support & SVSB_MODE_MON) {
> +		if (!IS_ERR_OR_NULL(svsb->tzone_name)) {
>   			svsb->tzd = thermal_zone_get_zone_by_name(svsb->tzone_name);
>   			if (IS_ERR(svsb->tzd)) {
>   				dev_err(svsb->dev, "cannot get \"%s\" thermal zone\n",
> @@ -2127,6 +2123,7 @@ static struct svs_bank svs_mt8192_banks[] = {
>   		.type			= SVSB_LOW,
>   		.set_freq_pct		= svs_set_bank_freq_pct_v3,
>   		.get_volts		= svs_get_bank_volts_v3,
> +		.tzone_name		= "gpu1",
>   		.volt_flags		= SVSB_REMOVE_DVTFIXED_VOLT,
>   		.mode_support		= SVSB_MODE_INIT02,
>   		.opp_count		= MAX_OPP_ENTRIES,
> @@ -2144,6 +2141,10 @@ static struct svs_bank svs_mt8192_banks[] = {
>   		.core_sel		= 0x0fff0100,
>   		.int_st			= BIT(0),
>   		.ctl0			= 0x00540003,
> +		.tzone_htemp		= 85000,
> +		.tzone_htemp_voffset	= 0,
> +		.tzone_ltemp		= 25000,
> +		.tzone_ltemp_voffset	= 7,

Which is the exact same tzone then in the other bank. Which brings me to a good 
point:
Is the tzone bank specific or the same for all banks?
At least for mt8192 they are not. I suppose with this change to the code mt8183 
could take advantage of this on all it's banks as well. In that case, can we 
start to restructure the struct svs_bank to only have the tzone values declared 
once?

Background is that I'm very unhappy with the svs_bank data strucutre. It seems 
like a "throw it all in here". It should be structured for functional parts of 
the banks. Maybe using structs, maybe unions where possible. In any case having 
a flat struct of over 50 members isn't really what we want.

Regards,
Matthias

>   	},
>   	{
>   		.sw_id			= SVSB_GPU,

Re: [PATCH v5 3/3] soc: mediatek: mtk-svs: add thermal voltage compensation if needed

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

Sorry for the late reply.

... [snip] ...

> > @@ -2127,6 +2123,7 @@ static struct svs_bank svs_mt8192_banks[] = {
> >   		.type			= SVSB_LOW,
> >   		.set_freq_pct		= svs_set_bank_freq_pct_v3,
> >   		.get_volts		= svs_get_bank_volts_v3,
> > +		.tzone_name		= "gpu1",
> >   		.volt_flags		= SVSB_REMOVE_DVTFIXED_VOLT,
> >   		.mode_support		= SVSB_MODE_INIT02,
> >   		.opp_count		= MAX_OPP_ENTRIES,
> > @@ -2144,6 +2141,10 @@ static struct svs_bank svs_mt8192_banks[] = {
> >   		.core_sel		= 0x0fff0100,
> >   		.int_st			= BIT(0),
> >   		.ctl0			= 0x00540003,
> > +		.tzone_htemp		= 85000,
> > +		.tzone_htemp_voffset	= 0,
> > +		.tzone_ltemp		= 25000,
> > +		.tzone_ltemp_voffset	= 7,
> 
> Which is the exact same tzone then in the other bank. Which brings me to a
> good 
> point:
> Is the tzone bank specific or the same for all banks?

Thermal zone (tzone) isn't for all SVS banks. In other words, tzone is specific
for corresponding DVFS domain like SVS GPU tzone is for GPU DVFS domain. Let's
take MT8183 SVS and MT8192 SVS as examples.

MT8192 SVS applies 2-line HW design (High/low 2 banks optimize the same DVFS
domain). So, SVS GPU High/low bank uses the same GPU tzone.

MT8183 SVS applies 1-line HW design (1 bank optimizes 1 DVFS domain)
Therefore, SVS CPU/GPU/CCI bank use different tzone because they are different
DVFS domain.

> At least for mt8192 they are not. I suppose with this change to the code
> mt8183 
> could take advantage of this on all it's banks as well.
> In that case, can we 
> start to restructure the struct svs_bank to only have the tzone values
> declared 
> once?

Since tzone isn't for all banks, we cannot declare it once for all IC supports
from this point of view. 

> 
> Background is that I'm very unhappy with the svs_bank data strucutre. It
> seems 
> like a "throw it all in here". It should be structured for functional parts
> of 
> the banks. Maybe using structs, maybe unions where possible. In any case
> having 
> a flat struct of over 50 members isn't really what we want.

My apology. We'll structure svs_bank for functional parts of them.

> 
> Regards,
> Matthias

Re: [PATCH v5 3/3] soc: mediatek: mtk-svs: add thermal voltage compensation if needed

[Matthias Brugger]

On 11/02/2023 12:12, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> Sorry for the late reply.
> 
> ... [snip] ...
> 
>>> @@ -2127,6 +2123,7 @@ static struct svs_bank svs_mt8192_banks[] = {
>>>    		.type			= SVSB_LOW,
>>>    		.set_freq_pct		= svs_set_bank_freq_pct_v3,
>>>    		.get_volts		= svs_get_bank_volts_v3,
>>> +		.tzone_name		= "gpu1",
>>>    		.volt_flags		= SVSB_REMOVE_DVTFIXED_VOLT,
>>>    		.mode_support		= SVSB_MODE_INIT02,
>>>    		.opp_count		= MAX_OPP_ENTRIES,
>>> @@ -2144,6 +2141,10 @@ static struct svs_bank svs_mt8192_banks[] = {
>>>    		.core_sel		= 0x0fff0100,
>>>    		.int_st			= BIT(0),
>>>    		.ctl0			= 0x00540003,
>>> +		.tzone_htemp		= 85000,
>>> +		.tzone_htemp_voffset	= 0,
>>> +		.tzone_ltemp		= 25000,
>>> +		.tzone_ltemp_voffset	= 7,
>>
>> Which is the exact same tzone then in the other bank. Which brings me to a
>> good
>> point:
>> Is the tzone bank specific or the same for all banks?
> 
> Thermal zone (tzone) isn't for all SVS banks. In other words, tzone is specific
> for corresponding DVFS domain like SVS GPU tzone is for GPU DVFS domain. Let's
> take MT8183 SVS and MT8192 SVS as examples.
> 
> MT8192 SVS applies 2-line HW design (High/low 2 banks optimize the same DVFS
> domain). So, SVS GPU High/low bank uses the same GPU tzone.
> 
> MT8183 SVS applies 1-line HW design (1 bank optimizes 1 DVFS domain)
> Therefore, SVS CPU/GPU/CCI bank use different tzone because they are different
> DVFS domain.
> 
>> At least for mt8192 they are not. I suppose with this change to the code
>> mt8183
>> could take advantage of this on all it's banks as well.
>> In that case, can we
>> start to restructure the struct svs_bank to only have the tzone values
>> declared
>> once?
> 
> Since tzone isn't for all banks, we cannot declare it once for all IC supports
> from this point of view.
> 

Thanks for clarification, applied now.

>>
>> Background is that I'm very unhappy with the svs_bank data strucutre. It
>> seems
>> like a "throw it all in here". It should be structured for functional parts
>> of
>> the banks. Maybe using structs, maybe unions where possible. In any case
>> having
>> a flat struct of over 50 members isn't really what we want.
> 
> My apology. We'll structure svs_bank for functional parts of them.
> 
>>
>> Regards,
>> Matthias