[PATCH v4 0/14] Enahance SVS's robustness

[PATCH v4 0/14] Enahance 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 v3:
- Remove cleanup codes in patch [01/14] for letting it backport easily
- Enclose svs debug cmd codes with "CONFIG_DEBUG_FS" where needed
- Reorder patch sequenece to put fix up patch first

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

Matthias Brugger (4):
  soc: mediatek: mtk-svs: clean up platform probing
  soc: mediatek: mtk-svs: improve readability of platform_probe
  soc: mediatek: mtk-svs: move svs_platform_probe into probe
  soc: mediatek: mtk-svs: delete superfluous platform data entries

Ricardo Ribalda (1):
  soc: mediatek: mtk-svs: enable the IRQ later

Roger Lu (8):
  soc: mediatek: mtk-svs: restore default voltages when svs_init02()
    fail
  soc: mediatek: mtk-svs: reset svs when svs_resume() fail
  soc: mediatek: mtk-svs: use svs clk control APIs
  soc: mediatek: mtk-svs: add thermal voltage compensation if needed
  soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not
    supported
  soc: mediatek: mtk-svs: use svs get efuse common function
  soc: mediatek: mtk-svs: use common function to disable restore
    voltages
  soc: mtk-svs: mt8183: refactor o_slope calculation

Shang XiaoJing (1):
  soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in
    svs_init01()

 drivers/soc/mediatek/mtk-svs.c | 339 +++++++++++++++++----------------
 1 file changed, 173 insertions(+), 166 deletions(-)

[PATCH v4 0/14] Enahance 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 v3:
- Remove cleanup codes in patch [01/14] for letting it backport easily
- Enclose svs debug cmd codes with "CONFIG_DEBUG_FS" where needed
- Reorder patch sequenece to put fix up patch first

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

Matthias Brugger (4):
  soc: mediatek: mtk-svs: clean up platform probing
  soc: mediatek: mtk-svs: improve readability of platform_probe
  soc: mediatek: mtk-svs: move svs_platform_probe into probe
  soc: mediatek: mtk-svs: delete superfluous platform data entries

Ricardo Ribalda (1):
  soc: mediatek: mtk-svs: enable the IRQ later

Roger Lu (8):
  soc: mediatek: mtk-svs: restore default voltages when svs_init02()
    fail
  soc: mediatek: mtk-svs: reset svs when svs_resume() fail
  soc: mediatek: mtk-svs: use svs clk control APIs
  soc: mediatek: mtk-svs: add thermal voltage compensation if needed
  soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not
    supported
  soc: mediatek: mtk-svs: use svs get efuse common function
  soc: mediatek: mtk-svs: use common function to disable restore
    voltages
  soc: mtk-svs: mt8183: refactor o_slope calculation

Shang XiaoJing (1):
  soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in
    svs_init01()

 drivers/soc/mediatek/mtk-svs.c | 339 +++++++++++++++++----------------
 1 file changed, 173 insertions(+), 166 deletions(-)



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 01/14] soc: mediatek: mtk-svs: restore default voltages when svs_init02() fail

[Roger Lu]

If svs init02 fail, it means we cannot rely on svs bank voltages anymore.
We need to disable svs function and restore DVFS opp voltages back to the
default voltages for making sure we have enough DVFS voltages.

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Fixes: 0bbb09b2af9d ("soc: mediatek: SVS: add mt8192 SVS GPU driver")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 0469c9dfeb04..2df30a6bca28 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1461,6 +1461,7 @@ static int svs_init02(struct svs_platform *svsp)
 {
 	struct svs_bank *svsb;
 	unsigned long flags, time_left;
+	int ret;
 	u32 idx;
 
 	for (idx = 0; idx < svsp->bank_max; idx++) {
@@ -1479,7 +1480,8 @@ static int svs_init02(struct svs_platform *svsp)
 							msecs_to_jiffies(5000));
 		if (!time_left) {
 			dev_err(svsb->dev, "init02 completion timeout\n");
-			return -EBUSY;
+			ret = -EBUSY;
+			goto out_of_init02;
 		}
 	}
 
@@ -1497,12 +1499,30 @@ static int svs_init02(struct svs_platform *svsp)
 		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
 			if (svs_sync_bank_volts_from_opp(svsb)) {
 				dev_err(svsb->dev, "sync volt fail\n");
-				return -EPERM;
+				ret = -EPERM;
+				goto out_of_init02;
 			}
 		}
 	}
 
 	return 0;
+
+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);
+	}
+
+	return ret;
 }
 
 static void svs_mon_mode(struct svs_platform *svsp)
-- 
2.18.0

[PATCH v4 01/14] soc: mediatek: mtk-svs: restore default voltages when svs_init02() fail

[Roger Lu]

If svs init02 fail, it means we cannot rely on svs bank voltages anymore.
We need to disable svs function and restore DVFS opp voltages back to the
default voltages for making sure we have enough DVFS voltages.

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Fixes: 0bbb09b2af9d ("soc: mediatek: SVS: add mt8192 SVS GPU driver")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 0469c9dfeb04..2df30a6bca28 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1461,6 +1461,7 @@ static int svs_init02(struct svs_platform *svsp)
 {
 	struct svs_bank *svsb;
 	unsigned long flags, time_left;
+	int ret;
 	u32 idx;
 
 	for (idx = 0; idx < svsp->bank_max; idx++) {
@@ -1479,7 +1480,8 @@ static int svs_init02(struct svs_platform *svsp)
 							msecs_to_jiffies(5000));
 		if (!time_left) {
 			dev_err(svsb->dev, "init02 completion timeout\n");
-			return -EBUSY;
+			ret = -EBUSY;
+			goto out_of_init02;
 		}
 	}
 
@@ -1497,12 +1499,30 @@ static int svs_init02(struct svs_platform *svsp)
 		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
 			if (svs_sync_bank_volts_from_opp(svsb)) {
 				dev_err(svsb->dev, "sync volt fail\n");
-				return -EPERM;
+				ret = -EPERM;
+				goto out_of_init02;
 			}
 		}
 	}
 
 	return 0;
+
+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);
+	}
+
+	return ret;
 }
 
 static void svs_mon_mode(struct svs_platform *svsp)
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 02/14] soc: mediatek: mtk-svs: reset svs when svs_resume() fail

[Roger Lu]

Add svs reset when svs_resume() fail.

Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 2df30a6bca28..a803b92afc3d 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1614,12 +1614,16 @@ static int svs_resume(struct device *dev)
 
 	ret = svs_init02(svsp);
 	if (ret)
-		goto out_of_resume;
+		goto svs_resume_reset_assert;
 
 	svs_mon_mode(svsp);
 
 	return 0;
 
+svs_resume_reset_assert:
+	dev_err(svsp->dev, "assert reset: %d\n",
+		reset_control_assert(svsp->rst));
+
 out_of_resume:
 	clk_disable_unprepare(svsp->main_clk);
 	return ret;
-- 
2.18.0

[PATCH v4 02/14] soc: mediatek: mtk-svs: reset svs when svs_resume() fail

[Roger Lu]

Add svs reset when svs_resume() fail.

Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 2df30a6bca28..a803b92afc3d 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1614,12 +1614,16 @@ static int svs_resume(struct device *dev)
 
 	ret = svs_init02(svsp);
 	if (ret)
-		goto out_of_resume;
+		goto svs_resume_reset_assert;
 
 	svs_mon_mode(svsp);
 
 	return 0;
 
+svs_resume_reset_assert:
+	dev_err(svsp->dev, "assert reset: %d\n",
+		reset_control_assert(svsp->rst));
+
 out_of_resume:
 	clk_disable_unprepare(svsp->main_clk);
 	return ret;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Roger Lu]

From: Ricardo Ribalda <ribalda@chromium.org>

If the system does not come from reset (like when is booted via
kexec(), the peripheral might trigger an IRQ before the data structures
are initialized.

Fixes:

[    0.227710] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000f08
[    0.227913] Call trace:
[    0.227918]  svs_isr+0x8c/0x538

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index a803b92afc3d..64d4d03ab71c 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_free_resource;
 	}
 
-	svsp_irq = platform_get_irq(pdev, 0);
-	if (svsp_irq < 0) {
-		ret = svsp_irq;
-		goto svs_probe_free_resource;
-	}
-
-	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
-					IRQF_ONESHOT, svsp->name, svsp);
-	if (ret) {
-		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
-			svsp_irq, ret);
-		goto svs_probe_free_resource;
-	}
-
 	svsp->main_clk = devm_clk_get(svsp->dev, "main");
 	if (IS_ERR(svsp->main_clk)) {
 		dev_err(svsp->dev, "failed to get clock: %ld\n",
@@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_clk_disable;
 	}
 
+	svsp_irq = platform_get_irq(pdev, 0);
+	if (svsp_irq < 0) {
+		ret = svsp_irq;
+		goto svs_probe_iounmap;
+	}
+
+	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
+					IRQF_ONESHOT, svsp->name, svsp);
+	if (ret) {
+		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
+			svsp_irq, ret);
+		goto svs_probe_iounmap;
+	}
+
 	ret = svs_start(svsp);
 	if (ret) {
 		dev_err(svsp->dev, "svs start fail: %d\n", ret);
-- 
2.18.0

[PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Roger Lu]

From: Ricardo Ribalda <ribalda@chromium.org>

If the system does not come from reset (like when is booted via
kexec(), the peripheral might trigger an IRQ before the data structures
are initialized.

Fixes:

[    0.227710] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000f08
[    0.227913] Call trace:
[    0.227918]  svs_isr+0x8c/0x538

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index a803b92afc3d..64d4d03ab71c 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_free_resource;
 	}
 
-	svsp_irq = platform_get_irq(pdev, 0);
-	if (svsp_irq < 0) {
-		ret = svsp_irq;
-		goto svs_probe_free_resource;
-	}
-
-	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
-					IRQF_ONESHOT, svsp->name, svsp);
-	if (ret) {
-		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
-			svsp_irq, ret);
-		goto svs_probe_free_resource;
-	}
-
 	svsp->main_clk = devm_clk_get(svsp->dev, "main");
 	if (IS_ERR(svsp->main_clk)) {
 		dev_err(svsp->dev, "failed to get clock: %ld\n",
@@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_clk_disable;
 	}
 
+	svsp_irq = platform_get_irq(pdev, 0);
+	if (svsp_irq < 0) {
+		ret = svsp_irq;
+		goto svs_probe_iounmap;
+	}
+
+	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
+					IRQF_ONESHOT, svsp->name, svsp);
+	if (ret) {
+		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
+			svsp_irq, ret);
+		goto svs_probe_iounmap;
+	}
+
 	ret = svs_start(svsp);
 	if (ret) {
 		dev_err(svsp->dev, "svs start fail: %d\n", ret);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 04/14] soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in svs_init01()

[Roger Lu]

From: Shang XiaoJing <shangxiaojing@huawei.com>

svs_init01() calls pm_runtime_get_sync() and added fail path as
svs_init01_finish to put usage_counter. However, pm_runtime_get_sync()
will increment usage_counter even it failed. Fix it by replacing it with
pm_runtime_resume_and_get() to keep usage counter balanced.

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-svs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 64d4d03ab71c..9575aa645643 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1324,7 +1324,7 @@ static int svs_init01(struct svs_platform *svsp)
 				svsb->pm_runtime_enabled_count++;
 			}
 
-			ret = pm_runtime_get_sync(svsb->opp_dev);
+			ret = pm_runtime_resume_and_get(svsb->opp_dev);
 			if (ret < 0) {
 				dev_err(svsb->dev, "mtcmos on fail: %d\n", ret);
 				goto svs_init01_resume_cpuidle;
-- 
2.18.0

[PATCH v4 04/14] soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in svs_init01()

[Roger Lu]

From: Shang XiaoJing <shangxiaojing@huawei.com>

svs_init01() calls pm_runtime_get_sync() and added fail path as
svs_init01_finish to put usage_counter. However, pm_runtime_get_sync()
will increment usage_counter even it failed. Fix it by replacing it with
pm_runtime_resume_and_get() to keep usage counter balanced.

Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-svs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 64d4d03ab71c..9575aa645643 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1324,7 +1324,7 @@ static int svs_init01(struct svs_platform *svsp)
 				svsb->pm_runtime_enabled_count++;
 			}
 
-			ret = pm_runtime_get_sync(svsb->opp_dev);
+			ret = pm_runtime_resume_and_get(svsb->opp_dev);
 			if (ret < 0) {
 				dev_err(svsb->dev, "mtcmos on fail: %d\n", ret);
 				goto svs_init01_resume_cpuidle;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu]

In MediaTek HW design, svs and thermal both use the same clk source.
It means that svs clk reference count from CCF includes thermal control
counts. That makes svs driver confuse whether it disabled svs's main clk
or not from CCF's perspective and lead to turn off their shared clk
unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
main clk is controlled well by svs driver itself.

Here is a NG example. Rely on CCF's reference count and cause problem.

thermal probe (clk ref = 1)
-> svs probe (clk ref = 2)
   -> svs suspend (clk ref = 1)
      -> thermal suspend (clk ref = 0)
      -> thermal resume (clk ref = 1)
   -> svs resume (encounter error, clk ref = 1)
   -> svs suspend (clk ref = 0)
      -> thermal suspend (Fail here, thermal HW control w/o clk)

Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 9575aa645643..830263bad81e 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = {
  * @bank_max: total number of svs banks
  * @efuse: svs efuse data received from NVMEM framework
  * @tefuse: thermal efuse data received from NVMEM framework
+ * @clk_cnt: clock count shows the clk enable/disable times by svs driver
  */
 struct svs_platform {
 	char *name;
@@ -343,6 +344,7 @@ struct svs_platform {
 	u32 bank_max;
 	u32 *efuse;
 	u32 *tefuse;
+	s32 clk_cnt;
 };
 
 struct svs_platform_data {
@@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp)
 	svs_writel_relaxed(svsp, svsb->core_sel, CORESEL);
 }
 
+static bool svs_is_clk_enabled(struct svs_platform *svsp)
+{
+	return svsp->clk_cnt > 0 ? true : false;
+}
+
+static int svs_clk_enable(struct svs_platform *svsp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(svsp->main_clk);
+	if (ret) {
+		dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret);
+		return ret;
+	}
+
+	svsp->clk_cnt++;
+
+	return 0;
+}
+
+static void svs_clk_disable(struct svs_platform *svsp)
+{
+	clk_disable_unprepare(svsp->main_clk);
+	svsp->clk_cnt--;
+}
+
 static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
 				     u32 svsb_volt_base)
 {
@@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev)
 	int ret;
 	u32 idx;
 
+	if (!svs_is_clk_enabled(svsp)) {
+		dev_err(svsp->dev, "svs clk is disabled already (%d)\n",
+			svsp->clk_cnt);
+		return 0;
+	}
+
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
 
@@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev)
 		return ret;
 	}
 
-	clk_disable_unprepare(svsp->main_clk);
+	svs_clk_disable(svsp);
 
 	return 0;
 }
@@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev)
 	struct svs_platform *svsp = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(svsp->main_clk);
-	if (ret) {
-		dev_err(svsp->dev, "cannot enable main_clk, disable svs\n");
+	ret = svs_clk_enable(svsp);
+	if (ret)
 		return ret;
-	}
 
 	ret = reset_control_deassert(svsp->rst);
 	if (ret) {
 		dev_err(svsp->dev, "cannot deassert reset %d\n", ret);
-		goto out_of_resume;
+		goto svs_resume_clk_disable;
 	}
 
 	ret = svs_init02(svsp);
@@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev)
 	dev_err(svsp->dev, "assert reset: %d\n",
 		reset_control_assert(svsp->rst));
 
-out_of_resume:
-	clk_disable_unprepare(svsp->main_clk);
+svs_resume_clk_disable:
+	svs_clk_disable(svsp);
+
 	return ret;
 }
 
@@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_free_resource;
 	}
 
-	ret = clk_prepare_enable(svsp->main_clk);
-	if (ret) {
-		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
+	ret = svs_clk_enable(svsp);
+	if (ret)
 		goto svs_probe_free_resource;
-	}
 
 	svsp->base = of_iomap(svsp->dev->of_node, 0);
 	if (IS_ERR_OR_NULL(svsp->base)) {
@@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev)
 	iounmap(svsp->base);
 
 svs_probe_clk_disable:
-	clk_disable_unprepare(svsp->main_clk);
+	svs_clk_disable(svsp);
 
 svs_probe_free_resource:
 	if (!IS_ERR_OR_NULL(svsp->efuse))
-- 
2.18.0

[PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu]

In MediaTek HW design, svs and thermal both use the same clk source.
It means that svs clk reference count from CCF includes thermal control
counts. That makes svs driver confuse whether it disabled svs's main clk
or not from CCF's perspective and lead to turn off their shared clk
unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
main clk is controlled well by svs driver itself.

Here is a NG example. Rely on CCF's reference count and cause problem.

thermal probe (clk ref = 1)
-> svs probe (clk ref = 2)
   -> svs suspend (clk ref = 1)
      -> thermal suspend (clk ref = 0)
      -> thermal resume (clk ref = 1)
   -> svs resume (encounter error, clk ref = 1)
   -> svs suspend (clk ref = 0)
      -> thermal suspend (Fail here, thermal HW control w/o clk)

Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 9575aa645643..830263bad81e 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = {
  * @bank_max: total number of svs banks
  * @efuse: svs efuse data received from NVMEM framework
  * @tefuse: thermal efuse data received from NVMEM framework
+ * @clk_cnt: clock count shows the clk enable/disable times by svs driver
  */
 struct svs_platform {
 	char *name;
@@ -343,6 +344,7 @@ struct svs_platform {
 	u32 bank_max;
 	u32 *efuse;
 	u32 *tefuse;
+	s32 clk_cnt;
 };
 
 struct svs_platform_data {
@@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp)
 	svs_writel_relaxed(svsp, svsb->core_sel, CORESEL);
 }
 
+static bool svs_is_clk_enabled(struct svs_platform *svsp)
+{
+	return svsp->clk_cnt > 0 ? true : false;
+}
+
+static int svs_clk_enable(struct svs_platform *svsp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(svsp->main_clk);
+	if (ret) {
+		dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret);
+		return ret;
+	}
+
+	svsp->clk_cnt++;
+
+	return 0;
+}
+
+static void svs_clk_disable(struct svs_platform *svsp)
+{
+	clk_disable_unprepare(svsp->main_clk);
+	svsp->clk_cnt--;
+}
+
 static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
 				     u32 svsb_volt_base)
 {
@@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev)
 	int ret;
 	u32 idx;
 
+	if (!svs_is_clk_enabled(svsp)) {
+		dev_err(svsp->dev, "svs clk is disabled already (%d)\n",
+			svsp->clk_cnt);
+		return 0;
+	}
+
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
 
@@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev)
 		return ret;
 	}
 
-	clk_disable_unprepare(svsp->main_clk);
+	svs_clk_disable(svsp);
 
 	return 0;
 }
@@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev)
 	struct svs_platform *svsp = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(svsp->main_clk);
-	if (ret) {
-		dev_err(svsp->dev, "cannot enable main_clk, disable svs\n");
+	ret = svs_clk_enable(svsp);
+	if (ret)
 		return ret;
-	}
 
 	ret = reset_control_deassert(svsp->rst);
 	if (ret) {
 		dev_err(svsp->dev, "cannot deassert reset %d\n", ret);
-		goto out_of_resume;
+		goto svs_resume_clk_disable;
 	}
 
 	ret = svs_init02(svsp);
@@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev)
 	dev_err(svsp->dev, "assert reset: %d\n",
 		reset_control_assert(svsp->rst));
 
-out_of_resume:
-	clk_disable_unprepare(svsp->main_clk);
+svs_resume_clk_disable:
+	svs_clk_disable(svsp);
+
 	return ret;
 }
 
@@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_free_resource;
 	}
 
-	ret = clk_prepare_enable(svsp->main_clk);
-	if (ret) {
-		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
+	ret = svs_clk_enable(svsp);
+	if (ret)
 		goto svs_probe_free_resource;
-	}
 
 	svsp->base = of_iomap(svsp->dev->of_node, 0);
 	if (IS_ERR_OR_NULL(svsp->base)) {
@@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev)
 	iounmap(svsp->base);
 
 svs_probe_clk_disable:
-	clk_disable_unprepare(svsp->main_clk);
+	svs_clk_disable(svsp);
 
 svs_probe_free_resource:
 	if (!IS_ERR_OR_NULL(svsp->efuse))
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 06/14] 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 830263bad81e..a3c84e819bc5 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -590,7 +590,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)) {
@@ -605,7 +605,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;
 		}
@@ -621,11 +622,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,
@@ -1728,7 +1724,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",
@@ -2201,6 +2197,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,
@@ -2218,6 +2215,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

[PATCH v4 06/14] 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 830263bad81e..a3c84e819bc5 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -590,7 +590,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)) {
@@ -605,7 +605,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;
 		}
@@ -621,11 +622,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,
@@ -1728,7 +1724,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",
@@ -2201,6 +2197,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,
@@ -2218,6 +2215,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/14] soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not supported

[Roger Lu]

Some projects might not support CONFIG_DEBUG_FS but still needs svs to be
alive. Therefore, enclose debug cmd codes with CONFIG_DEBUG_FS to make sure
svs can be alive when CONFIG_DEBUG_FS not supported.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index a3c84e819bc5..70ca9c9acae0 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -138,6 +138,7 @@
 
 static DEFINE_SPINLOCK(svs_lock);
 
+#ifdef CONFIG_DEBUG_FS
 #define debug_fops_ro(name)						\
 	static int svs_##name##_debug_open(struct inode *inode,		\
 					   struct file *filp)		\
@@ -170,6 +171,7 @@ static DEFINE_SPINLOCK(svs_lock);
 	}
 
 #define svs_dentry_data(name)	{__stringify(name), &svs_##name##_debug_fops}
+#endif
 
 /**
  * enum svsb_phase - svs bank phase enumeration
@@ -652,6 +654,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 	return ret;
 }
 
+#ifdef CONFIG_DEBUG_FS
 static int svs_dump_debug_show(struct seq_file *m, void *p)
 {
 	struct svs_platform *svsp = (struct svs_platform *)m->private;
@@ -867,6 +870,7 @@ static int svs_create_debug_cmds(struct svs_platform *svsp)
 
 	return 0;
 }
+#endif /* CONFIG_DEBUG_FS */
 
 static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
 {
@@ -2476,11 +2480,13 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_iounmap;
 	}
 
+#ifdef CONFIG_DEBUG_FS
 	ret = svs_create_debug_cmds(svsp);
 	if (ret) {
 		dev_err(svsp->dev, "svs create debug cmds fail: %d\n", ret);
 		goto svs_probe_iounmap;
 	}
+#endif
 
 	return 0;
 
-- 
2.18.0

[PATCH v4 07/14] soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not supported

[Roger Lu]

Some projects might not support CONFIG_DEBUG_FS but still needs svs to be
alive. Therefore, enclose debug cmd codes with CONFIG_DEBUG_FS to make sure
svs can be alive when CONFIG_DEBUG_FS not supported.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index a3c84e819bc5..70ca9c9acae0 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -138,6 +138,7 @@
 
 static DEFINE_SPINLOCK(svs_lock);
 
+#ifdef CONFIG_DEBUG_FS
 #define debug_fops_ro(name)						\
 	static int svs_##name##_debug_open(struct inode *inode,		\
 					   struct file *filp)		\
@@ -170,6 +171,7 @@ static DEFINE_SPINLOCK(svs_lock);
 	}
 
 #define svs_dentry_data(name)	{__stringify(name), &svs_##name##_debug_fops}
+#endif
 
 /**
  * enum svsb_phase - svs bank phase enumeration
@@ -652,6 +654,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
 	return ret;
 }
 
+#ifdef CONFIG_DEBUG_FS
 static int svs_dump_debug_show(struct seq_file *m, void *p)
 {
 	struct svs_platform *svsp = (struct svs_platform *)m->private;
@@ -867,6 +870,7 @@ static int svs_create_debug_cmds(struct svs_platform *svsp)
 
 	return 0;
 }
+#endif /* CONFIG_DEBUG_FS */
 
 static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
 {
@@ -2476,11 +2480,13 @@ static int svs_probe(struct platform_device *pdev)
 		goto svs_probe_iounmap;
 	}
 
+#ifdef CONFIG_DEBUG_FS
 	ret = svs_create_debug_cmds(svsp);
 	if (ret) {
 		dev_err(svsp->dev, "svs create debug cmds fail: %d\n", ret);
 		goto svs_probe_iounmap;
 	}
+#endif
 
 	return 0;
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 08/14] soc: mediatek: mtk-svs: clean up platform probing

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

We only ever call the SoC specific probe function from
svs_platform_probe. No need to carry that function in a global
datastructure around.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 70ca9c9acae0..41c0ac5ed8d3 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -321,7 +321,6 @@ static const u32 svs_regs_v2[] = {
  * @banks: svs banks that svs platform supports
  * @rst: svs platform reset control
  * @efuse_parsing: svs platform efuse parsing function pointer
- * @probe: svs platform probe function pointer
  * @efuse_max: total number of svs efuse
  * @tefuse_max: total number of thermal efuse
  * @regs: svs platform registers map
@@ -339,7 +338,6 @@ struct svs_platform {
 	struct svs_bank *banks;
 	struct reset_control *rst;
 	bool (*efuse_parsing)(struct svs_platform *svsp);
-	int (*probe)(struct svs_platform *svsp);
 	size_t efuse_max;
 	size_t tefuse_max;
 	const u32 *regs;
@@ -2409,11 +2407,10 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 	svsp->name = svsp_data->name;
 	svsp->banks = svsp_data->banks;
 	svsp->efuse_parsing = svsp_data->efuse_parsing;
-	svsp->probe = svsp_data->probe;
 	svsp->regs = svsp_data->regs;
 	svsp->bank_max = svsp_data->bank_max;
 
-	ret = svsp->probe(svsp);
+	ret = svsp_data->probe(svsp);
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
2.18.0

[PATCH v4 08/14] soc: mediatek: mtk-svs: clean up platform probing

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

We only ever call the SoC specific probe function from
svs_platform_probe. No need to carry that function in a global
datastructure around.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 70ca9c9acae0..41c0ac5ed8d3 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -321,7 +321,6 @@ static const u32 svs_regs_v2[] = {
  * @banks: svs banks that svs platform supports
  * @rst: svs platform reset control
  * @efuse_parsing: svs platform efuse parsing function pointer
- * @probe: svs platform probe function pointer
  * @efuse_max: total number of svs efuse
  * @tefuse_max: total number of thermal efuse
  * @regs: svs platform registers map
@@ -339,7 +338,6 @@ struct svs_platform {
 	struct svs_bank *banks;
 	struct reset_control *rst;
 	bool (*efuse_parsing)(struct svs_platform *svsp);
-	int (*probe)(struct svs_platform *svsp);
 	size_t efuse_max;
 	size_t tefuse_max;
 	const u32 *regs;
@@ -2409,11 +2407,10 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 	svsp->name = svsp_data->name;
 	svsp->banks = svsp_data->banks;
 	svsp->efuse_parsing = svsp_data->efuse_parsing;
-	svsp->probe = svsp_data->probe;
 	svsp->regs = svsp_data->regs;
 	svsp->bank_max = svsp_data->bank_max;
 
-	ret = svsp->probe(svsp);
+	ret = svsp_data->probe(svsp);
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 09/14] soc: mediatek: mtk-svs: improve readability of platform_probe

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

If a compatible misses a match data entry, then something is wrong in
the development phase, we don't need to check for that at runtime.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 41c0ac5ed8d3..354582825082 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2394,10 +2394,6 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 	int ret;
 
 	svsp_data = of_device_get_match_data(&pdev->dev);
-	if (!svsp_data) {
-		dev_err(&pdev->dev, "no svs platform data?\n");
-		return ERR_PTR(-EPERM);
-	}
 
 	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
 	if (!svsp)
-- 
2.18.0

[PATCH v4 09/14] soc: mediatek: mtk-svs: improve readability of platform_probe

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

If a compatible misses a match data entry, then something is wrong in
the development phase, we don't need to check for that at runtime.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 41c0ac5ed8d3..354582825082 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2394,10 +2394,6 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 	int ret;
 
 	svsp_data = of_device_get_match_data(&pdev->dev);
-	if (!svsp_data) {
-		dev_err(&pdev->dev, "no svs platform data?\n");
-		return ERR_PTR(-EPERM);
-	}
 
 	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
 	if (!svsp)
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/14] soc: mediatek: mtk-svs: move svs_platform_probe into probe

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

Moving svs_platform_probe into driver probe function will allow us to
reduce svs_platform members. This will be done in a follow-up patch.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 354582825082..9d9210c22289 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2387,17 +2387,17 @@ static const struct of_device_id svs_of_match[] = {
 	},
 };
 
-static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
+static int svs_probe(struct platform_device *pdev)
 {
 	struct svs_platform *svsp;
 	const struct svs_platform_data *svsp_data;
-	int ret;
+	int ret, svsp_irq;
 
 	svsp_data = of_device_get_match_data(&pdev->dev);
 
 	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
 	if (!svsp)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	svsp->dev = &pdev->dev;
 	svsp->name = svsp_data->name;
@@ -2408,19 +2408,7 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 
 	ret = svsp_data->probe(svsp);
 	if (ret)
-		return ERR_PTR(ret);
-
-	return svsp;
-}
-
-static int svs_probe(struct platform_device *pdev)
-{
-	struct svs_platform *svsp;
-	int svsp_irq, ret;
-
-	svsp = svs_platform_probe(pdev);
-	if (IS_ERR(svsp))
-		return PTR_ERR(svsp);
+		return ret;
 
 	if (!svs_is_efuse_data_correct(svsp)) {
 		dev_notice(svsp->dev, "efuse data isn't correct\n");
-- 
2.18.0

[PATCH v4 10/14] soc: mediatek: mtk-svs: move svs_platform_probe into probe

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

Moving svs_platform_probe into driver probe function will allow us to
reduce svs_platform members. This will be done in a follow-up patch.

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Roger Lu <roger.lu@mediatek.com>
---
 drivers/soc/mediatek/mtk-svs.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 354582825082..9d9210c22289 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -2387,17 +2387,17 @@ static const struct of_device_id svs_of_match[] = {
 	},
 };
 
-static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
+static int svs_probe(struct platform_device *pdev)
 {
 	struct svs_platform *svsp;
 	const struct svs_platform_data *svsp_data;
-	int ret;
+	int ret, svsp_irq;
 
 	svsp_data = of_device_get_match_data(&pdev->dev);
 
 	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
 	if (!svsp)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	svsp->dev = &pdev->dev;
 	svsp->name = svsp_data->name;
@@ -2408,19 +2408,7 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
 
 	ret = svsp_data->probe(svsp);
 	if (ret)
-		return ERR_PTR(ret);
-
-	return svsp;
-}
-
-static int svs_probe(struct platform_device *pdev)
-{
-	struct svs_platform *svsp;
-	int svsp_irq, ret;
-
-	svsp = svs_platform_probe(pdev);
-	if (IS_ERR(svsp))
-		return PTR_ERR(svsp);
+		return ret;
 
 	if (!svs_is_efuse_data_correct(svsp)) {
 		dev_notice(svsp->dev, "efuse data isn't correct\n");
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/14] soc: mediatek: mtk-svs: delete superfluous platform data entries

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

The platform name and efuse parsing function pointer are only used while
probing the device. Use them from the svs_platform_data struct instead.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 9d9210c22289..bd23d1111d7b 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -313,14 +313,12 @@ static const u32 svs_regs_v2[] = {
 
 /**
  * struct svs_platform - svs platform control
- * @name: svs platform name
  * @base: svs platform register base
  * @dev: svs platform device
  * @main_clk: main clock for svs bank
  * @pbank: svs bank pointer needing to be protected by spin_lock section
  * @banks: svs banks that svs platform supports
  * @rst: svs platform reset control
- * @efuse_parsing: svs platform efuse parsing function pointer
  * @efuse_max: total number of svs efuse
  * @tefuse_max: total number of thermal efuse
  * @regs: svs platform registers map
@@ -330,14 +328,12 @@ static const u32 svs_regs_v2[] = {
  * @clk_cnt: clock count shows the clk enable/disable times by svs driver
  */
 struct svs_platform {
-	char *name;
 	void __iomem *base;
 	struct device *dev;
 	struct clk *main_clk;
 	struct svs_bank *pbank;
 	struct svs_bank *banks;
 	struct reset_control *rst;
-	bool (*efuse_parsing)(struct svs_platform *svsp);
 	size_t efuse_max;
 	size_t tefuse_max;
 	const u32 *regs;
@@ -2066,7 +2062,7 @@ static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
 	svsp->efuse_max /= sizeof(u32);
 	nvmem_cell_put(cell);
 
-	return svsp->efuse_parsing(svsp);
+	return true;
 }
 
 static struct device *svs_get_subsys_device(struct svs_platform *svsp,
@@ -2400,9 +2396,7 @@ static int svs_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	svsp->dev = &pdev->dev;
-	svsp->name = svsp_data->name;
 	svsp->banks = svsp_data->banks;
-	svsp->efuse_parsing = svsp_data->efuse_parsing;
 	svsp->regs = svsp_data->regs;
 	svsp->bank_max = svsp_data->bank_max;
 
@@ -2413,6 +2407,12 @@ static int svs_probe(struct platform_device *pdev)
 	if (!svs_is_efuse_data_correct(svsp)) {
 		dev_notice(svsp->dev, "efuse data isn't correct\n");
 		ret = -EPERM;
+		goto svs_probe_free_efuse;
+	}
+
+	if (!svsp_data->efuse_parsing(svsp)) {
+		dev_err(svsp->dev, "efuse data parsing failed\n");
+		ret = -EPERM;
 		goto svs_probe_free_resource;
 	}
 
@@ -2448,7 +2448,7 @@ static int svs_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
-					IRQF_ONESHOT, svsp->name, svsp);
+					IRQF_ONESHOT, svsp_data->name, svsp);
 	if (ret) {
 		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
 			svsp_irq, ret);
@@ -2478,11 +2478,13 @@ static int svs_probe(struct platform_device *pdev)
 	svs_clk_disable(svsp);
 
 svs_probe_free_resource:
-	if (!IS_ERR_OR_NULL(svsp->efuse))
-		kfree(svsp->efuse);
 	if (!IS_ERR_OR_NULL(svsp->tefuse))
 		kfree(svsp->tefuse);
 
+svs_probe_free_efuse:
+	if (!IS_ERR_OR_NULL(svsp->efuse))
+		kfree(svsp->efuse);
+
 	return ret;
 }
 
-- 
2.18.0

[PATCH v4 11/14] soc: mediatek: mtk-svs: delete superfluous platform data entries

[Roger Lu]

From: Matthias Brugger <matthias.bgg@gmail.com>

The platform name and efuse parsing function pointer are only used while
probing the device. Use them from the svs_platform_data struct instead.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 9d9210c22289..bd23d1111d7b 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -313,14 +313,12 @@ static const u32 svs_regs_v2[] = {
 
 /**
  * struct svs_platform - svs platform control
- * @name: svs platform name
  * @base: svs platform register base
  * @dev: svs platform device
  * @main_clk: main clock for svs bank
  * @pbank: svs bank pointer needing to be protected by spin_lock section
  * @banks: svs banks that svs platform supports
  * @rst: svs platform reset control
- * @efuse_parsing: svs platform efuse parsing function pointer
  * @efuse_max: total number of svs efuse
  * @tefuse_max: total number of thermal efuse
  * @regs: svs platform registers map
@@ -330,14 +328,12 @@ static const u32 svs_regs_v2[] = {
  * @clk_cnt: clock count shows the clk enable/disable times by svs driver
  */
 struct svs_platform {
-	char *name;
 	void __iomem *base;
 	struct device *dev;
 	struct clk *main_clk;
 	struct svs_bank *pbank;
 	struct svs_bank *banks;
 	struct reset_control *rst;
-	bool (*efuse_parsing)(struct svs_platform *svsp);
 	size_t efuse_max;
 	size_t tefuse_max;
 	const u32 *regs;
@@ -2066,7 +2062,7 @@ static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
 	svsp->efuse_max /= sizeof(u32);
 	nvmem_cell_put(cell);
 
-	return svsp->efuse_parsing(svsp);
+	return true;
 }
 
 static struct device *svs_get_subsys_device(struct svs_platform *svsp,
@@ -2400,9 +2396,7 @@ static int svs_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	svsp->dev = &pdev->dev;
-	svsp->name = svsp_data->name;
 	svsp->banks = svsp_data->banks;
-	svsp->efuse_parsing = svsp_data->efuse_parsing;
 	svsp->regs = svsp_data->regs;
 	svsp->bank_max = svsp_data->bank_max;
 
@@ -2413,6 +2407,12 @@ static int svs_probe(struct platform_device *pdev)
 	if (!svs_is_efuse_data_correct(svsp)) {
 		dev_notice(svsp->dev, "efuse data isn't correct\n");
 		ret = -EPERM;
+		goto svs_probe_free_efuse;
+	}
+
+	if (!svsp_data->efuse_parsing(svsp)) {
+		dev_err(svsp->dev, "efuse data parsing failed\n");
+		ret = -EPERM;
 		goto svs_probe_free_resource;
 	}
 
@@ -2448,7 +2448,7 @@ static int svs_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
-					IRQF_ONESHOT, svsp->name, svsp);
+					IRQF_ONESHOT, svsp_data->name, svsp);
 	if (ret) {
 		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
 			svsp_irq, ret);
@@ -2478,11 +2478,13 @@ static int svs_probe(struct platform_device *pdev)
 	svs_clk_disable(svsp);
 
 svs_probe_free_resource:
-	if (!IS_ERR_OR_NULL(svsp->efuse))
-		kfree(svsp->efuse);
 	if (!IS_ERR_OR_NULL(svsp->tefuse))
 		kfree(svsp->tefuse);
 
+svs_probe_free_efuse:
+	if (!IS_ERR_OR_NULL(svsp->efuse))
+		kfree(svsp->efuse);
+
 	return ret;
 }
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 12/14] 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 | 69 ++++++++++++----------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index bd23d1111d7b..a7f0a6f02d52 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1758,26 +1758,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;
@@ -1825,7 +1827,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;
 
@@ -1930,7 +1933,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;
 
@@ -2039,32 +2043,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)
 {
@@ -2404,8 +2382,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;
 	}
@@ -2413,13 +2392,13 @@ 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->main_clk = devm_clk_get(svsp->dev, "main");
@@ -2427,12 +2406,12 @@ 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 = svs_clk_enable(svsp);
 	if (ret)
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 
 	svsp->base = of_iomap(svsp->dev->of_node, 0);
 	if (IS_ERR_OR_NULL(svsp->base)) {
@@ -2477,7 +2456,7 @@ static int svs_probe(struct platform_device *pdev)
 svs_probe_clk_disable:
 	svs_clk_disable(svsp);
 
-svs_probe_free_resource:
+svs_probe_free_tefuse:
 	if (!IS_ERR_OR_NULL(svsp->tefuse))
 		kfree(svsp->tefuse);
 
-- 
2.18.0

[PATCH v4 12/14] 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 | 69 ++++++++++++----------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index bd23d1111d7b..a7f0a6f02d52 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1758,26 +1758,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;
@@ -1825,7 +1827,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;
 
@@ -1930,7 +1933,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;
 
@@ -2039,32 +2043,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)
 {
@@ -2404,8 +2382,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;
 	}
@@ -2413,13 +2392,13 @@ 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->main_clk = devm_clk_get(svsp->dev, "main");
@@ -2427,12 +2406,12 @@ 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 = svs_clk_enable(svsp);
 	if (ret)
-		goto svs_probe_free_resource;
+		goto svs_probe_free_tefuse;
 
 	svsp->base = of_iomap(svsp->dev->of_node, 0);
 	if (IS_ERR_OR_NULL(svsp->base)) {
@@ -2477,7 +2456,7 @@ static int svs_probe(struct platform_device *pdev)
 svs_probe_clk_disable:
 	svs_clk_disable(svsp);
 
-svs_probe_free_resource:
+svs_probe_free_tefuse:
 	if (!IS_ERR_OR_NULL(svsp->tefuse))
 		kfree(svsp->tefuse);
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 13/14] 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 a7f0a6f02d52..89117807e85d 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -648,6 +648,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)
 {
@@ -724,7 +743,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;
 
@@ -740,16 +758,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);
@@ -1532,16 +1542,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;
@@ -1587,7 +1588,6 @@ 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;
 
@@ -1599,17 +1599,7 @@ static int svs_suspend(struct device *dev)
 
 	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

[PATCH v4 13/14] 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 a7f0a6f02d52..89117807e85d 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -648,6 +648,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)
 {
@@ -724,7 +743,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;
 
@@ -740,16 +758,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);
@@ -1532,16 +1542,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;
@@ -1587,7 +1588,6 @@ 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;
 
@@ -1599,17 +1599,7 @@ static int svs_suspend(struct device *dev)
 
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 14/14] soc: mtk-svs: mt8183: refactor o_slope calculation

[Roger Lu]

The o_slope value is dependent of the o_slope_sign, refactor code to get
rid of unnecessary if constructs.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 89117807e85d..8cfbf8ffb138 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1944,26 +1944,27 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 	o_slope_sign = (svsp->tefuse[0] >> 7) & BIT(0);
 
 	ts_id = (svsp->tefuse[1] >> 9) & BIT(0);
-	o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
-
-	if (adc_cali_en_t == 1) {
-		if (!ts_id)
-			o_slope = 0;
-
-		if (adc_ge_t < 265 || adc_ge_t > 758 ||
-		    adc_oe_t < 265 || adc_oe_t > 758 ||
-		    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
-		    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
-		    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
-		    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
-		    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
-		    o_vtsabb < -8 || o_vtsabb > 484 ||
-		    degc_cali < 1 || degc_cali > 63) {
-			dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
-			goto remove_mt8183_svsb_mon_mode;
-		}
+	if (!ts_id) {
+		o_slope = 1534;
 	} else {
-		dev_err(svsp->dev, "no thermal efuse, no mon mode\n");
+		o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
+		if (!o_slope_sign)
+			o_slope = 1534 + o_slope * 10;
+		else
+			o_slope = 1534 - o_slope * 10;
+	}
+
+	if (adc_cali_en_t == 0 ||
+	    adc_ge_t < 265 || adc_ge_t > 758 ||
+	    adc_oe_t < 265 || adc_oe_t > 758 ||
+	    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
+	    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
+	    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
+	    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
+	    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
+	    o_vtsabb < -8 || o_vtsabb > 484 ||
+	    degc_cali < 1 || degc_cali > 63) {
+		dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
 		goto remove_mt8183_svsb_mon_mode;
 	}
 
@@ -1982,11 +1983,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 		x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / gain;
 
 	temp0 = (10000 * 100000 / gain) * 15 / 18;
-
-	if (!o_slope_sign)
-		mts = (temp0 * 10) / (1534 + o_slope * 10);
-	else
-		mts = (temp0 * 10) / (1534 - o_slope * 10);
+	mts = (temp0 * 10) / o_slope;
 
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
@@ -2013,11 +2010,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 		temp0 = (degc_cali * 10 / 2);
 		temp1 = ((10000 * 100000 / 4096 / gain) *
 			 oe + tb_roomt * 10) * 15 / 18;
-
-		if (!o_slope_sign)
-			temp2 = temp1 * 100 / (1534 + o_slope * 10);
-		else
-			temp2 = temp1 * 100 / (1534 - o_slope * 10);
+		temp2 = temp1 * 100 / o_slope;
 
 		svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
 	}
-- 
2.18.0

[PATCH v4 14/14] soc: mtk-svs: mt8183: refactor o_slope calculation

[Roger Lu]

The o_slope value is dependent of the o_slope_sign, refactor code to get
rid of unnecessary if constructs.

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

diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
index 89117807e85d..8cfbf8ffb138 100644
--- a/drivers/soc/mediatek/mtk-svs.c
+++ b/drivers/soc/mediatek/mtk-svs.c
@@ -1944,26 +1944,27 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 	o_slope_sign = (svsp->tefuse[0] >> 7) & BIT(0);
 
 	ts_id = (svsp->tefuse[1] >> 9) & BIT(0);
-	o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
-
-	if (adc_cali_en_t == 1) {
-		if (!ts_id)
-			o_slope = 0;
-
-		if (adc_ge_t < 265 || adc_ge_t > 758 ||
-		    adc_oe_t < 265 || adc_oe_t > 758 ||
-		    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
-		    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
-		    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
-		    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
-		    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
-		    o_vtsabb < -8 || o_vtsabb > 484 ||
-		    degc_cali < 1 || degc_cali > 63) {
-			dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
-			goto remove_mt8183_svsb_mon_mode;
-		}
+	if (!ts_id) {
+		o_slope = 1534;
 	} else {
-		dev_err(svsp->dev, "no thermal efuse, no mon mode\n");
+		o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
+		if (!o_slope_sign)
+			o_slope = 1534 + o_slope * 10;
+		else
+			o_slope = 1534 - o_slope * 10;
+	}
+
+	if (adc_cali_en_t == 0 ||
+	    adc_ge_t < 265 || adc_ge_t > 758 ||
+	    adc_oe_t < 265 || adc_oe_t > 758 ||
+	    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
+	    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
+	    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
+	    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
+	    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
+	    o_vtsabb < -8 || o_vtsabb > 484 ||
+	    degc_cali < 1 || degc_cali > 63) {
+		dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
 		goto remove_mt8183_svsb_mon_mode;
 	}
 
@@ -1982,11 +1983,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 		x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / gain;
 
 	temp0 = (10000 * 100000 / gain) * 15 / 18;
-
-	if (!o_slope_sign)
-		mts = (temp0 * 10) / (1534 + o_slope * 10);
-	else
-		mts = (temp0 * 10) / (1534 - o_slope * 10);
+	mts = (temp0 * 10) / o_slope;
 
 	for (idx = 0; idx < svsp->bank_max; idx++) {
 		svsb = &svsp->banks[idx];
@@ -2013,11 +2010,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
 		temp0 = (degc_cali * 10 / 2);
 		temp1 = ((10000 * 100000 / 4096 / gain) *
 			 oe + tb_roomt * 10) * 15 / 18;
-
-		if (!o_slope_sign)
-			temp2 = temp1 * 100 / (1534 + o_slope * 10);
-		else
-			temp2 = temp1 * 100 / (1534 - o_slope * 10);
+		temp2 = temp1 * 100 / o_slope;
 
 		svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
 	}
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Ricardo Ribalda <ribalda@chromium.org>
> 
> If the system does not come from reset (like when is booted via
> kexec(), the peripheral might trigger an IRQ before the data structures
> are initialized.
> 
> Fixes:
> 
> [    0.227710] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000f08
> [    0.227913] Call trace:
> [    0.227918]  svs_isr+0x8c/0x538
> 

This patch is different from
https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org

If you think the patch from Ricardo is wrong, then you should have mentioned 
that in the review.

Regards,
Matthias

> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
>   drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index a803b92afc3d..64d4d03ab71c 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_free_resource;
>   	}
>   
> -	svsp_irq = platform_get_irq(pdev, 0);
> -	if (svsp_irq < 0) {
> -		ret = svsp_irq;
> -		goto svs_probe_free_resource;
> -	}
> -
> -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> -					IRQF_ONESHOT, svsp->name, svsp);
> -	if (ret) {
> -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> -			svsp_irq, ret);
> -		goto svs_probe_free_resource;
> -	}
> -
>   	svsp->main_clk = devm_clk_get(svsp->dev, "main");
>   	if (IS_ERR(svsp->main_clk)) {
>   		dev_err(svsp->dev, "failed to get clock: %ld\n",
> @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_clk_disable;
>   	}
>   
> +	svsp_irq = platform_get_irq(pdev, 0);
> +	if (svsp_irq < 0) {
> +		ret = svsp_irq;
> +		goto svs_probe_iounmap;
> +	}
> +
> +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> +					IRQF_ONESHOT, svsp->name, svsp);
> +	if (ret) {
> +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> +			svsp_irq, ret);
> +		goto svs_probe_iounmap;
> +	}
> +
>   	ret = svs_start(svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "svs start fail: %d\n", ret);

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Ricardo Ribalda <ribalda@chromium.org>
> 
> If the system does not come from reset (like when is booted via
> kexec(), the peripheral might trigger an IRQ before the data structures
> are initialized.
> 
> Fixes:
> 
> [    0.227710] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000f08
> [    0.227913] Call trace:
> [    0.227918]  svs_isr+0x8c/0x538
> 

This patch is different from
https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org

If you think the patch from Ricardo is wrong, then you should have mentioned 
that in the review.

Regards,
Matthias

> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
>   drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index a803b92afc3d..64d4d03ab71c 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_free_resource;
>   	}
>   
> -	svsp_irq = platform_get_irq(pdev, 0);
> -	if (svsp_irq < 0) {
> -		ret = svsp_irq;
> -		goto svs_probe_free_resource;
> -	}
> -
> -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> -					IRQF_ONESHOT, svsp->name, svsp);
> -	if (ret) {
> -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> -			svsp_irq, ret);
> -		goto svs_probe_free_resource;
> -	}
> -
>   	svsp->main_clk = devm_clk_get(svsp->dev, "main");
>   	if (IS_ERR(svsp->main_clk)) {
>   		dev_err(svsp->dev, "failed to get clock: %ld\n",
> @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_clk_disable;
>   	}
>   
> +	svsp_irq = platform_get_irq(pdev, 0);
> +	if (svsp_irq < 0) {
> +		ret = svsp_irq;
> +		goto svs_probe_iounmap;
> +	}
> +
> +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> +					IRQF_ONESHOT, svsp->name, svsp);
> +	if (ret) {
> +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> +			svsp_irq, ret);
> +		goto svs_probe_iounmap;
> +	}
> +
>   	ret = svs_start(svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "svs start fail: %d\n", ret);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> In MediaTek HW design, svs and thermal both use the same clk source.
> It means that svs clk reference count from CCF includes thermal control
> counts. That makes svs driver confuse whether it disabled svs's main clk
> or not from CCF's perspective and lead to turn off their shared clk
> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> main clk is controlled well by svs driver itself.
> 
> Here is a NG example. Rely on CCF's reference count and cause problem.
> 
> thermal probe (clk ref = 1)
> -> svs probe (clk ref = 2)
>     -> svs suspend (clk ref = 1)
>        -> thermal suspend (clk ref = 0)
>        -> thermal resume (clk ref = 1)
>     -> svs resume (encounter error, clk ref = 1)
>     -> svs suspend (clk ref = 0)
>        -> thermal suspend (Fail here, thermal HW control w/o clk)
> 
> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

That looks wrong. Although I don't out of my mind, there should be a way to tell 
the clock framework that this clock is shared between several devices.

I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't be 
enough.

Regards,
Matthias

> ---
>   drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++--------
>   1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 9575aa645643..830263bad81e 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = {
>    * @bank_max: total number of svs banks
>    * @efuse: svs efuse data received from NVMEM framework
>    * @tefuse: thermal efuse data received from NVMEM framework
> + * @clk_cnt: clock count shows the clk enable/disable times by svs driver
>    */
>   struct svs_platform {
>   	char *name;
> @@ -343,6 +344,7 @@ struct svs_platform {
>   	u32 bank_max;
>   	u32 *efuse;
>   	u32 *tefuse;
> +	s32 clk_cnt;
>   };
>   
>   struct svs_platform_data {
> @@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp)
>   	svs_writel_relaxed(svsp, svsb->core_sel, CORESEL);
>   }
>   
> +static bool svs_is_clk_enabled(struct svs_platform *svsp)
> +{
> +	return svsp->clk_cnt > 0 ? true : false;
> +}
> +
> +static int svs_clk_enable(struct svs_platform *svsp)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(svsp->main_clk);
> +	if (ret) {
> +		dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	svsp->clk_cnt++;
> +
> +	return 0;
> +}
> +
> +static void svs_clk_disable(struct svs_platform *svsp)
> +{
> +	clk_disable_unprepare(svsp->main_clk);
> +	svsp->clk_cnt--;
> +}
> +
>   static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
>   				     u32 svsb_volt_base)
>   {
> @@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev)
>   	int ret;
>   	u32 idx;
>   
> +	if (!svs_is_clk_enabled(svsp)) {
> +		dev_err(svsp->dev, "svs clk is disabled already (%d)\n",
> +			svsp->clk_cnt);
> +		return 0;
> +	}
> +
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
>   
> @@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev)
>   		return ret;
>   	}
>   
> -	clk_disable_unprepare(svsp->main_clk);
> +	svs_clk_disable(svsp);
>   
>   	return 0;
>   }
> @@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev)
>   	struct svs_platform *svsp = dev_get_drvdata(dev);
>   	int ret;
>   
> -	ret = clk_prepare_enable(svsp->main_clk);
> -	if (ret) {
> -		dev_err(svsp->dev, "cannot enable main_clk, disable svs\n");
> +	ret = svs_clk_enable(svsp);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	ret = reset_control_deassert(svsp->rst);
>   	if (ret) {
>   		dev_err(svsp->dev, "cannot deassert reset %d\n", ret);
> -		goto out_of_resume;
> +		goto svs_resume_clk_disable;
>   	}
>   
>   	ret = svs_init02(svsp);
> @@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev)
>   	dev_err(svsp->dev, "assert reset: %d\n",
>   		reset_control_assert(svsp->rst));
>   
> -out_of_resume:
> -	clk_disable_unprepare(svsp->main_clk);
> +svs_resume_clk_disable:
> +	svs_clk_disable(svsp);
> +
>   	return ret;
>   }
>   
> @@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_free_resource;
>   	}
>   
> -	ret = clk_prepare_enable(svsp->main_clk);
> -	if (ret) {
> -		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
> +	ret = svs_clk_enable(svsp);
> +	if (ret)
>   		goto svs_probe_free_resource;
> -	}
>   
>   	svsp->base = of_iomap(svsp->dev->of_node, 0);
>   	if (IS_ERR_OR_NULL(svsp->base)) {
> @@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev)
>   	iounmap(svsp->base);
>   
>   svs_probe_clk_disable:
> -	clk_disable_unprepare(svsp->main_clk);
> +	svs_clk_disable(svsp);
>   
>   svs_probe_free_resource:
>   	if (!IS_ERR_OR_NULL(svsp->efuse))

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> In MediaTek HW design, svs and thermal both use the same clk source.
> It means that svs clk reference count from CCF includes thermal control
> counts. That makes svs driver confuse whether it disabled svs's main clk
> or not from CCF's perspective and lead to turn off their shared clk
> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> main clk is controlled well by svs driver itself.
> 
> Here is a NG example. Rely on CCF's reference count and cause problem.
> 
> thermal probe (clk ref = 1)
> -> svs probe (clk ref = 2)
>     -> svs suspend (clk ref = 1)
>        -> thermal suspend (clk ref = 0)
>        -> thermal resume (clk ref = 1)
>     -> svs resume (encounter error, clk ref = 1)
>     -> svs suspend (clk ref = 0)
>        -> thermal suspend (Fail here, thermal HW control w/o clk)
> 
> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

That looks wrong. Although I don't out of my mind, there should be a way to tell 
the clock framework that this clock is shared between several devices.

I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't be 
enough.

Regards,
Matthias

> ---
>   drivers/soc/mediatek/mtk-svs.c | 57 ++++++++++++++++++++++++++--------
>   1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 9575aa645643..830263bad81e 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -326,6 +326,7 @@ static const u32 svs_regs_v2[] = {
>    * @bank_max: total number of svs banks
>    * @efuse: svs efuse data received from NVMEM framework
>    * @tefuse: thermal efuse data received from NVMEM framework
> + * @clk_cnt: clock count shows the clk enable/disable times by svs driver
>    */
>   struct svs_platform {
>   	char *name;
> @@ -343,6 +344,7 @@ struct svs_platform {
>   	u32 bank_max;
>   	u32 *efuse;
>   	u32 *tefuse;
> +	s32 clk_cnt;
>   };
>   
>   struct svs_platform_data {
> @@ -502,6 +504,32 @@ static void svs_switch_bank(struct svs_platform *svsp)
>   	svs_writel_relaxed(svsp, svsb->core_sel, CORESEL);
>   }
>   
> +static bool svs_is_clk_enabled(struct svs_platform *svsp)
> +{
> +	return svsp->clk_cnt > 0 ? true : false;
> +}
> +
> +static int svs_clk_enable(struct svs_platform *svsp)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(svsp->main_clk);
> +	if (ret) {
> +		dev_err(svsp->dev, "cannot enable main_clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	svsp->clk_cnt++;
> +
> +	return 0;
> +}
> +
> +static void svs_clk_disable(struct svs_platform *svsp)
> +{
> +	clk_disable_unprepare(svsp->main_clk);
> +	svsp->clk_cnt--;
> +}
> +
>   static u32 svs_bank_volt_to_opp_volt(u32 svsb_volt, u32 svsb_volt_step,
>   				     u32 svsb_volt_base)
>   {
> @@ -1569,6 +1597,12 @@ static int svs_suspend(struct device *dev)
>   	int ret;
>   	u32 idx;
>   
> +	if (!svs_is_clk_enabled(svsp)) {
> +		dev_err(svsp->dev, "svs clk is disabled already (%d)\n",
> +			svsp->clk_cnt);
> +		return 0;
> +	}
> +
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
>   
> @@ -1590,7 +1624,7 @@ static int svs_suspend(struct device *dev)
>   		return ret;
>   	}
>   
> -	clk_disable_unprepare(svsp->main_clk);
> +	svs_clk_disable(svsp);
>   
>   	return 0;
>   }
> @@ -1600,16 +1634,14 @@ static int svs_resume(struct device *dev)
>   	struct svs_platform *svsp = dev_get_drvdata(dev);
>   	int ret;
>   
> -	ret = clk_prepare_enable(svsp->main_clk);
> -	if (ret) {
> -		dev_err(svsp->dev, "cannot enable main_clk, disable svs\n");
> +	ret = svs_clk_enable(svsp);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	ret = reset_control_deassert(svsp->rst);
>   	if (ret) {
>   		dev_err(svsp->dev, "cannot deassert reset %d\n", ret);
> -		goto out_of_resume;
> +		goto svs_resume_clk_disable;
>   	}
>   
>   	ret = svs_init02(svsp);
> @@ -1624,8 +1656,9 @@ static int svs_resume(struct device *dev)
>   	dev_err(svsp->dev, "assert reset: %d\n",
>   		reset_control_assert(svsp->rst));
>   
> -out_of_resume:
> -	clk_disable_unprepare(svsp->main_clk);
> +svs_resume_clk_disable:
> +	svs_clk_disable(svsp);
> +
>   	return ret;
>   }
>   
> @@ -2411,11 +2444,9 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_free_resource;
>   	}
>   
> -	ret = clk_prepare_enable(svsp->main_clk);
> -	if (ret) {
> -		dev_err(svsp->dev, "cannot enable main clk: %d\n", ret);
> +	ret = svs_clk_enable(svsp);
> +	if (ret)
>   		goto svs_probe_free_resource;
> -	}
>   
>   	svsp->base = of_iomap(svsp->dev->of_node, 0);
>   	if (IS_ERR_OR_NULL(svsp->base)) {
> @@ -2456,7 +2487,7 @@ static int svs_probe(struct platform_device *pdev)
>   	iounmap(svsp->base);
>   
>   svs_probe_clk_disable:
> -	clk_disable_unprepare(svsp->main_clk);
> +	svs_clk_disable(svsp);
>   
>   svs_probe_free_resource:
>   	if (!IS_ERR_OR_NULL(svsp->efuse))

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 01/14] soc: mediatek: mtk-svs: restore default voltages when svs_init02() fail

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> If svs init02 fail, it means we cannot rely on svs bank voltages anymore.
> We need to disable svs function and restore DVFS opp voltages back to the
> default voltages for making sure we have enough DVFS voltages.
> 
> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Fixes: 0bbb09b2af9d ("soc: mediatek: SVS: add mt8192 SVS GPU driver")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 0469c9dfeb04..2df30a6bca28 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1461,6 +1461,7 @@ static int svs_init02(struct svs_platform *svsp)
>   {
>   	struct svs_bank *svsb;
>   	unsigned long flags, time_left;
> +	int ret;
>   	u32 idx;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
> @@ -1479,7 +1480,8 @@ static int svs_init02(struct svs_platform *svsp)
>   							msecs_to_jiffies(5000));
>   		if (!time_left) {
>   			dev_err(svsb->dev, "init02 completion timeout\n");
> -			return -EBUSY;
> +			ret = -EBUSY;
> +			goto out_of_init02;
>   		}
>   	}
>   
> @@ -1497,12 +1499,30 @@ static int svs_init02(struct svs_platform *svsp)
>   		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
>   			if (svs_sync_bank_volts_from_opp(svsb)) {
>   				dev_err(svsb->dev, "sync volt fail\n");
> -				return -EPERM;
> +				ret = -EPERM;
> +				goto out_of_init02;
>   			}
>   		}
>   	}
>   
>   	return 0;
> +
> +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);
> +	}
> +
> +	return ret;
>   }
>   
>   static void svs_mon_mode(struct svs_platform *svsp)

Re: [PATCH v4 01/14] soc: mediatek: mtk-svs: restore default voltages when svs_init02() fail

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> If svs init02 fail, it means we cannot rely on svs bank voltages anymore.
> We need to disable svs function and restore DVFS opp voltages back to the
> default voltages for making sure we have enough DVFS voltages.
> 
> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Fixes: 0bbb09b2af9d ("soc: mediatek: SVS: add mt8192 SVS GPU driver")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 0469c9dfeb04..2df30a6bca28 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1461,6 +1461,7 @@ static int svs_init02(struct svs_platform *svsp)
>   {
>   	struct svs_bank *svsb;
>   	unsigned long flags, time_left;
> +	int ret;
>   	u32 idx;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
> @@ -1479,7 +1480,8 @@ static int svs_init02(struct svs_platform *svsp)
>   							msecs_to_jiffies(5000));
>   		if (!time_left) {
>   			dev_err(svsb->dev, "init02 completion timeout\n");
> -			return -EBUSY;
> +			ret = -EBUSY;
> +			goto out_of_init02;
>   		}
>   	}
>   
> @@ -1497,12 +1499,30 @@ static int svs_init02(struct svs_platform *svsp)
>   		if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
>   			if (svs_sync_bank_volts_from_opp(svsb)) {
>   				dev_err(svsb->dev, "sync volt fail\n");
> -				return -EPERM;
> +				ret = -EPERM;
> +				goto out_of_init02;
>   			}
>   		}
>   	}
>   
>   	return 0;
> +
> +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);
> +	}
> +
> +	return ret;
>   }
>   
>   static void svs_mon_mode(struct svs_platform *svsp)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/14] soc: mediatek: mtk-svs: reset svs when svs_resume() fail

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> Add svs reset when svs_resume() fail.
> 
> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 2df30a6bca28..a803b92afc3d 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1614,12 +1614,16 @@ static int svs_resume(struct device *dev)
>   
>   	ret = svs_init02(svsp);
>   	if (ret)
> -		goto out_of_resume;
> +		goto svs_resume_reset_assert;
>   
>   	svs_mon_mode(svsp);
>   
>   	return 0;
>   
> +svs_resume_reset_assert:
> +	dev_err(svsp->dev, "assert reset: %d\n",
> +		reset_control_assert(svsp->rst));
> +
>   out_of_resume:
>   	clk_disable_unprepare(svsp->main_clk);
>   	return ret;

Re: [PATCH v4 02/14] soc: mediatek: mtk-svs: reset svs when svs_resume() fail

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> Add svs reset when svs_resume() fail.
> 
> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on err in svs_resume()")
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 2df30a6bca28..a803b92afc3d 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1614,12 +1614,16 @@ static int svs_resume(struct device *dev)
>   
>   	ret = svs_init02(svsp);
>   	if (ret)
> -		goto out_of_resume;
> +		goto svs_resume_reset_assert;
>   
>   	svs_mon_mode(svsp);
>   
>   	return 0;
>   
> +svs_resume_reset_assert:
> +	dev_err(svsp->dev, "assert reset: %d\n",
> +		reset_control_assert(svsp->rst));
> +
>   out_of_resume:
>   	clk_disable_unprepare(svsp->main_clk);
>   	return ret;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 04/14] soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in svs_init01()

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Shang XiaoJing <shangxiaojing@huawei.com>
> 
> svs_init01() calls pm_runtime_get_sync() and added fail path as
> svs_init01_finish to put usage_counter. However, pm_runtime_get_sync()
> will increment usage_counter even it failed. Fix it by replacing it with
> pm_runtime_resume_and_get() to keep usage counter balanced.
> 
> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 64d4d03ab71c..9575aa645643 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1324,7 +1324,7 @@ static int svs_init01(struct svs_platform *svsp)
>   				svsb->pm_runtime_enabled_count++;
>   			}
>   
> -			ret = pm_runtime_get_sync(svsb->opp_dev);
> +			ret = pm_runtime_resume_and_get(svsb->opp_dev);
>   			if (ret < 0) {
>   				dev_err(svsb->dev, "mtcmos on fail: %d\n", ret);
>   				goto svs_init01_resume_cpuidle;

Re: [PATCH v4 04/14] soc: mediatek: mtk-svs: Use pm_runtime_resume_and_get() in svs_init01()

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Shang XiaoJing <shangxiaojing@huawei.com>
> 
> svs_init01() calls pm_runtime_get_sync() and added fail path as
> svs_init01_finish to put usage_counter. However, pm_runtime_get_sync()
> will increment usage_counter even it failed. Fix it by replacing it with
> pm_runtime_resume_and_get() to keep usage counter balanced.
> 
> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 64d4d03ab71c..9575aa645643 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1324,7 +1324,7 @@ static int svs_init01(struct svs_platform *svsp)
>   				svsb->pm_runtime_enabled_count++;
>   			}
>   
> -			ret = pm_runtime_get_sync(svsb->opp_dev);
> +			ret = pm_runtime_resume_and_get(svsb->opp_dev);
>   			if (ret < 0) {
>   				dev_err(svsb->dev, "mtcmos on fail: %d\n", ret);
>   				goto svs_init01_resume_cpuidle;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/14] soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not supported

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> Some projects might not support CONFIG_DEBUG_FS but still needs svs to be
> alive. Therefore, enclose debug cmd codes with CONFIG_DEBUG_FS to make sure
> svs can be alive when CONFIG_DEBUG_FS not supported.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index a3c84e819bc5..70ca9c9acae0 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -138,6 +138,7 @@
>   
>   static DEFINE_SPINLOCK(svs_lock);
>   
> +#ifdef CONFIG_DEBUG_FS
>   #define debug_fops_ro(name)						\
>   	static int svs_##name##_debug_open(struct inode *inode,		\
>   					   struct file *filp)		\
> @@ -170,6 +171,7 @@ static DEFINE_SPINLOCK(svs_lock);
>   	}
>   
>   #define svs_dentry_data(name)	{__stringify(name), &svs_##name##_debug_fops}
> +#endif
>   
>   /**
>    * enum svsb_phase - svs bank phase enumeration
> @@ -652,6 +654,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   	return ret;
>   }
>   
> +#ifdef CONFIG_DEBUG_FS
>   static int svs_dump_debug_show(struct seq_file *m, void *p)
>   {
>   	struct svs_platform *svsp = (struct svs_platform *)m->private;
> @@ -867,6 +870,7 @@ static int svs_create_debug_cmds(struct svs_platform *svsp)
>   
>   	return 0;
>   }
> +#endif /* CONFIG_DEBUG_FS */
>   
>   static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
>   {
> @@ -2476,11 +2480,13 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_iounmap;
>   	}
>   
> +#ifdef CONFIG_DEBUG_FS
>   	ret = svs_create_debug_cmds(svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "svs create debug cmds fail: %d\n", ret);
>   		goto svs_probe_iounmap;
>   	}
> +#endif
>   
>   	return 0;
>   

Re: [PATCH v4 07/14] soc: mediatek: mtk-svs: keep svs alive if CONFIG_DEBUG_FS not supported

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> Some projects might not support CONFIG_DEBUG_FS but still needs svs to be
> alive. Therefore, enclose debug cmd codes with CONFIG_DEBUG_FS to make sure
> svs can be alive when CONFIG_DEBUG_FS not supported.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index a3c84e819bc5..70ca9c9acae0 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -138,6 +138,7 @@
>   
>   static DEFINE_SPINLOCK(svs_lock);
>   
> +#ifdef CONFIG_DEBUG_FS
>   #define debug_fops_ro(name)						\
>   	static int svs_##name##_debug_open(struct inode *inode,		\
>   					   struct file *filp)		\
> @@ -170,6 +171,7 @@ static DEFINE_SPINLOCK(svs_lock);
>   	}
>   
>   #define svs_dentry_data(name)	{__stringify(name), &svs_##name##_debug_fops}
> +#endif
>   
>   /**
>    * enum svsb_phase - svs bank phase enumeration
> @@ -652,6 +654,7 @@ static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>   	return ret;
>   }
>   
> +#ifdef CONFIG_DEBUG_FS
>   static int svs_dump_debug_show(struct seq_file *m, void *p)
>   {
>   	struct svs_platform *svsp = (struct svs_platform *)m->private;
> @@ -867,6 +870,7 @@ static int svs_create_debug_cmds(struct svs_platform *svsp)
>   
>   	return 0;
>   }
> +#endif /* CONFIG_DEBUG_FS */
>   
>   static u32 interpolate(u32 f0, u32 f1, u32 v0, u32 v1, u32 fx)
>   {
> @@ -2476,11 +2480,13 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_iounmap;
>   	}
>   
> +#ifdef CONFIG_DEBUG_FS
>   	ret = svs_create_debug_cmds(svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "svs create debug cmds fail: %d\n", ret);
>   		goto svs_probe_iounmap;
>   	}
> +#endif
>   
>   	return 0;
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 08/14] soc: mediatek: mtk-svs: clean up platform probing

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> We only ever call the SoC specific probe function from
> svs_platform_probe. No need to carry that function in a global
> datastructure around.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 70ca9c9acae0..41c0ac5ed8d3 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -321,7 +321,6 @@ static const u32 svs_regs_v2[] = {
>    * @banks: svs banks that svs platform supports
>    * @rst: svs platform reset control
>    * @efuse_parsing: svs platform efuse parsing function pointer
> - * @probe: svs platform probe function pointer
>    * @efuse_max: total number of svs efuse
>    * @tefuse_max: total number of thermal efuse
>    * @regs: svs platform registers map
> @@ -339,7 +338,6 @@ struct svs_platform {
>   	struct svs_bank *banks;
>   	struct reset_control *rst;
>   	bool (*efuse_parsing)(struct svs_platform *svsp);
> -	int (*probe)(struct svs_platform *svsp);
>   	size_t efuse_max;
>   	size_t tefuse_max;
>   	const u32 *regs;
> @@ -2409,11 +2407,10 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   	svsp->name = svsp_data->name;
>   	svsp->banks = svsp_data->banks;
>   	svsp->efuse_parsing = svsp_data->efuse_parsing;
> -	svsp->probe = svsp_data->probe;
>   	svsp->regs = svsp_data->regs;
>   	svsp->bank_max = svsp_data->bank_max;
>   
> -	ret = svsp->probe(svsp);
> +	ret = svsp_data->probe(svsp);
>   	if (ret)
>   		return ERR_PTR(ret);
>   

Re: [PATCH v4 08/14] soc: mediatek: mtk-svs: clean up platform probing

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> We only ever call the SoC specific probe function from
> svs_platform_probe. No need to carry that function in a global
> datastructure around.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 70ca9c9acae0..41c0ac5ed8d3 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -321,7 +321,6 @@ static const u32 svs_regs_v2[] = {
>    * @banks: svs banks that svs platform supports
>    * @rst: svs platform reset control
>    * @efuse_parsing: svs platform efuse parsing function pointer
> - * @probe: svs platform probe function pointer
>    * @efuse_max: total number of svs efuse
>    * @tefuse_max: total number of thermal efuse
>    * @regs: svs platform registers map
> @@ -339,7 +338,6 @@ struct svs_platform {
>   	struct svs_bank *banks;
>   	struct reset_control *rst;
>   	bool (*efuse_parsing)(struct svs_platform *svsp);
> -	int (*probe)(struct svs_platform *svsp);
>   	size_t efuse_max;
>   	size_t tefuse_max;
>   	const u32 *regs;
> @@ -2409,11 +2407,10 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   	svsp->name = svsp_data->name;
>   	svsp->banks = svsp_data->banks;
>   	svsp->efuse_parsing = svsp_data->efuse_parsing;
> -	svsp->probe = svsp_data->probe;
>   	svsp->regs = svsp_data->regs;
>   	svsp->bank_max = svsp_data->bank_max;
>   
> -	ret = svsp->probe(svsp);
> +	ret = svsp_data->probe(svsp);
>   	if (ret)
>   		return ERR_PTR(ret);
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/14] soc: mediatek: mtk-svs: improve readability of platform_probe

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> If a compatible misses a match data entry, then something is wrong in
> the development phase, we don't need to check for that at runtime.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 41c0ac5ed8d3..354582825082 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2394,10 +2394,6 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   	int ret;
>   
>   	svsp_data = of_device_get_match_data(&pdev->dev);
> -	if (!svsp_data) {
> -		dev_err(&pdev->dev, "no svs platform data?\n");
> -		return ERR_PTR(-EPERM);
> -	}
>   
>   	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
>   	if (!svsp)

Re: [PATCH v4 09/14] soc: mediatek: mtk-svs: improve readability of platform_probe

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> If a compatible misses a match data entry, then something is wrong in
> the development phase, we don't need to check for that at runtime.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 41c0ac5ed8d3..354582825082 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2394,10 +2394,6 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   	int ret;
>   
>   	svsp_data = of_device_get_match_data(&pdev->dev);
> -	if (!svsp_data) {
> -		dev_err(&pdev->dev, "no svs platform data?\n");
> -		return ERR_PTR(-EPERM);
> -	}
>   
>   	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
>   	if (!svsp)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 10/14] soc: mediatek: mtk-svs: move svs_platform_probe into probe

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> Moving svs_platform_probe into driver probe function will allow us to
> reduce svs_platform members. This will be done in a follow-up patch.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 20 ++++----------------
>   1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 354582825082..9d9210c22289 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2387,17 +2387,17 @@ static const struct of_device_id svs_of_match[] = {
>   	},
>   };
>   
> -static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
> +static int svs_probe(struct platform_device *pdev)
>   {
>   	struct svs_platform *svsp;
>   	const struct svs_platform_data *svsp_data;
> -	int ret;
> +	int ret, svsp_irq;
>   
>   	svsp_data = of_device_get_match_data(&pdev->dev);
>   
>   	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
>   	if (!svsp)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	svsp->dev = &pdev->dev;
>   	svsp->name = svsp_data->name;
> @@ -2408,19 +2408,7 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   
>   	ret = svsp_data->probe(svsp);
>   	if (ret)
> -		return ERR_PTR(ret);
> -
> -	return svsp;
> -}
> -
> -static int svs_probe(struct platform_device *pdev)
> -{
> -	struct svs_platform *svsp;
> -	int svsp_irq, ret;
> -
> -	svsp = svs_platform_probe(pdev);
> -	if (IS_ERR(svsp))
> -		return PTR_ERR(svsp);
> +		return ret;
>   
>   	if (!svs_is_efuse_data_correct(svsp)) {
>   		dev_notice(svsp->dev, "efuse data isn't correct\n");

Re: [PATCH v4 10/14] soc: mediatek: mtk-svs: move svs_platform_probe into probe

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> Moving svs_platform_probe into driver probe function will allow us to
> reduce svs_platform members. This will be done in a follow-up patch.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 20 ++++----------------
>   1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 354582825082..9d9210c22289 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -2387,17 +2387,17 @@ static const struct of_device_id svs_of_match[] = {
>   	},
>   };
>   
> -static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
> +static int svs_probe(struct platform_device *pdev)
>   {
>   	struct svs_platform *svsp;
>   	const struct svs_platform_data *svsp_data;
> -	int ret;
> +	int ret, svsp_irq;
>   
>   	svsp_data = of_device_get_match_data(&pdev->dev);
>   
>   	svsp = devm_kzalloc(&pdev->dev, sizeof(*svsp), GFP_KERNEL);
>   	if (!svsp)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>   
>   	svsp->dev = &pdev->dev;
>   	svsp->name = svsp_data->name;
> @@ -2408,19 +2408,7 @@ static struct svs_platform *svs_platform_probe(struct platform_device *pdev)
>   
>   	ret = svsp_data->probe(svsp);
>   	if (ret)
> -		return ERR_PTR(ret);
> -
> -	return svsp;
> -}
> -
> -static int svs_probe(struct platform_device *pdev)
> -{
> -	struct svs_platform *svsp;
> -	int svsp_irq, ret;
> -
> -	svsp = svs_platform_probe(pdev);
> -	if (IS_ERR(svsp))
> -		return PTR_ERR(svsp);
> +		return ret;
>   
>   	if (!svs_is_efuse_data_correct(svsp)) {
>   		dev_notice(svsp->dev, "efuse data isn't correct\n");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/14] soc: mediatek: mtk-svs: delete superfluous platform data entries

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> The platform name and efuse parsing function pointer are only used while
> probing the device. Use them from the svs_platform_data struct instead.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 9d9210c22289..bd23d1111d7b 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -313,14 +313,12 @@ static const u32 svs_regs_v2[] = {
>   
>   /**
>    * struct svs_platform - svs platform control
> - * @name: svs platform name
>    * @base: svs platform register base
>    * @dev: svs platform device
>    * @main_clk: main clock for svs bank
>    * @pbank: svs bank pointer needing to be protected by spin_lock section
>    * @banks: svs banks that svs platform supports
>    * @rst: svs platform reset control
> - * @efuse_parsing: svs platform efuse parsing function pointer
>    * @efuse_max: total number of svs efuse
>    * @tefuse_max: total number of thermal efuse
>    * @regs: svs platform registers map
> @@ -330,14 +328,12 @@ static const u32 svs_regs_v2[] = {
>    * @clk_cnt: clock count shows the clk enable/disable times by svs driver
>    */
>   struct svs_platform {
> -	char *name;
>   	void __iomem *base;
>   	struct device *dev;
>   	struct clk *main_clk;
>   	struct svs_bank *pbank;
>   	struct svs_bank *banks;
>   	struct reset_control *rst;
> -	bool (*efuse_parsing)(struct svs_platform *svsp);
>   	size_t efuse_max;
>   	size_t tefuse_max;
>   	const u32 *regs;
> @@ -2066,7 +2062,7 @@ static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
>   	svsp->efuse_max /= sizeof(u32);
>   	nvmem_cell_put(cell);
>   
> -	return svsp->efuse_parsing(svsp);
> +	return true;
>   }
>   
>   static struct device *svs_get_subsys_device(struct svs_platform *svsp,
> @@ -2400,9 +2396,7 @@ static int svs_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	svsp->dev = &pdev->dev;
> -	svsp->name = svsp_data->name;
>   	svsp->banks = svsp_data->banks;
> -	svsp->efuse_parsing = svsp_data->efuse_parsing;
>   	svsp->regs = svsp_data->regs;
>   	svsp->bank_max = svsp_data->bank_max;
>   
> @@ -2413,6 +2407,12 @@ static int svs_probe(struct platform_device *pdev)
>   	if (!svs_is_efuse_data_correct(svsp)) {
>   		dev_notice(svsp->dev, "efuse data isn't correct\n");
>   		ret = -EPERM;
> +		goto svs_probe_free_efuse;
> +	}
> +
> +	if (!svsp_data->efuse_parsing(svsp)) {
> +		dev_err(svsp->dev, "efuse data parsing failed\n");
> +		ret = -EPERM;
>   		goto svs_probe_free_resource;
>   	}
>   
> @@ -2448,7 +2448,7 @@ static int svs_probe(struct platform_device *pdev)
>   	}
>   
>   	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> -					IRQF_ONESHOT, svsp->name, svsp);
> +					IRQF_ONESHOT, svsp_data->name, svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>   			svsp_irq, ret);
> @@ -2478,11 +2478,13 @@ static int svs_probe(struct platform_device *pdev)
>   	svs_clk_disable(svsp);
>   
>   svs_probe_free_resource:
> -	if (!IS_ERR_OR_NULL(svsp->efuse))
> -		kfree(svsp->efuse);
>   	if (!IS_ERR_OR_NULL(svsp->tefuse))
>   		kfree(svsp->tefuse);
>   
> +svs_probe_free_efuse:
> +	if (!IS_ERR_OR_NULL(svsp->efuse))
> +		kfree(svsp->efuse);
> +
>   	return ret;
>   }
>   

Re: [PATCH v4 11/14] soc: mediatek: mtk-svs: delete superfluous platform data entries

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> The platform name and efuse parsing function pointer are only used while
> probing the device. Use them from the svs_platform_data struct instead.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 9d9210c22289..bd23d1111d7b 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -313,14 +313,12 @@ static const u32 svs_regs_v2[] = {
>   
>   /**
>    * struct svs_platform - svs platform control
> - * @name: svs platform name
>    * @base: svs platform register base
>    * @dev: svs platform device
>    * @main_clk: main clock for svs bank
>    * @pbank: svs bank pointer needing to be protected by spin_lock section
>    * @banks: svs banks that svs platform supports
>    * @rst: svs platform reset control
> - * @efuse_parsing: svs platform efuse parsing function pointer
>    * @efuse_max: total number of svs efuse
>    * @tefuse_max: total number of thermal efuse
>    * @regs: svs platform registers map
> @@ -330,14 +328,12 @@ static const u32 svs_regs_v2[] = {
>    * @clk_cnt: clock count shows the clk enable/disable times by svs driver
>    */
>   struct svs_platform {
> -	char *name;
>   	void __iomem *base;
>   	struct device *dev;
>   	struct clk *main_clk;
>   	struct svs_bank *pbank;
>   	struct svs_bank *banks;
>   	struct reset_control *rst;
> -	bool (*efuse_parsing)(struct svs_platform *svsp);
>   	size_t efuse_max;
>   	size_t tefuse_max;
>   	const u32 *regs;
> @@ -2066,7 +2062,7 @@ static bool svs_is_efuse_data_correct(struct svs_platform *svsp)
>   	svsp->efuse_max /= sizeof(u32);
>   	nvmem_cell_put(cell);
>   
> -	return svsp->efuse_parsing(svsp);
> +	return true;
>   }
>   
>   static struct device *svs_get_subsys_device(struct svs_platform *svsp,
> @@ -2400,9 +2396,7 @@ static int svs_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	svsp->dev = &pdev->dev;
> -	svsp->name = svsp_data->name;
>   	svsp->banks = svsp_data->banks;
> -	svsp->efuse_parsing = svsp_data->efuse_parsing;
>   	svsp->regs = svsp_data->regs;
>   	svsp->bank_max = svsp_data->bank_max;
>   
> @@ -2413,6 +2407,12 @@ static int svs_probe(struct platform_device *pdev)
>   	if (!svs_is_efuse_data_correct(svsp)) {
>   		dev_notice(svsp->dev, "efuse data isn't correct\n");
>   		ret = -EPERM;
> +		goto svs_probe_free_efuse;
> +	}
> +
> +	if (!svsp_data->efuse_parsing(svsp)) {
> +		dev_err(svsp->dev, "efuse data parsing failed\n");
> +		ret = -EPERM;
>   		goto svs_probe_free_resource;
>   	}
>   
> @@ -2448,7 +2448,7 @@ static int svs_probe(struct platform_device *pdev)
>   	}
>   
>   	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> -					IRQF_ONESHOT, svsp->name, svsp);
> +					IRQF_ONESHOT, svsp_data->name, svsp);
>   	if (ret) {
>   		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>   			svsp_irq, ret);
> @@ -2478,11 +2478,13 @@ static int svs_probe(struct platform_device *pdev)
>   	svs_clk_disable(svsp);
>   
>   svs_probe_free_resource:
> -	if (!IS_ERR_OR_NULL(svsp->efuse))
> -		kfree(svsp->efuse);
>   	if (!IS_ERR_OR_NULL(svsp->tefuse))
>   		kfree(svsp->tefuse);
>   
> +svs_probe_free_efuse:
> +	if (!IS_ERR_OR_NULL(svsp->efuse))
> +		kfree(svsp->efuse);
> +
>   	return ret;
>   }
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 12/14] soc: mediatek: mtk-svs: use svs get efuse common function

[Matthias Brugger]

On 11/01/2023 08:45, 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>

Looks good to me, could you please rebase.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/soc/mediatek/mtk-svs.c | 69 ++++++++++++----------------------
>   1 file changed, 24 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index bd23d1111d7b..a7f0a6f02d52 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1758,26 +1758,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;
> @@ -1825,7 +1827,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;
>   
> @@ -1930,7 +1933,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;
>   
> @@ -2039,32 +2043,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)
>   {
> @@ -2404,8 +2382,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;
>   	}
> @@ -2413,13 +2392,13 @@ 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->main_clk = devm_clk_get(svsp->dev, "main");
> @@ -2427,12 +2406,12 @@ 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 = svs_clk_enable(svsp);
>   	if (ret)
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   
>   	svsp->base = of_iomap(svsp->dev->of_node, 0);
>   	if (IS_ERR_OR_NULL(svsp->base)) {
> @@ -2477,7 +2456,7 @@ static int svs_probe(struct platform_device *pdev)
>   svs_probe_clk_disable:
>   	svs_clk_disable(svsp);
>   
> -svs_probe_free_resource:
> +svs_probe_free_tefuse:
>   	if (!IS_ERR_OR_NULL(svsp->tefuse))
>   		kfree(svsp->tefuse);
>   

Re: [PATCH v4 12/14] soc: mediatek: mtk-svs: use svs get efuse common function

[Matthias Brugger]

On 11/01/2023 08:45, 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>

Looks good to me, could you please rebase.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/soc/mediatek/mtk-svs.c | 69 ++++++++++++----------------------
>   1 file changed, 24 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index bd23d1111d7b..a7f0a6f02d52 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1758,26 +1758,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;
> @@ -1825,7 +1827,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;
>   
> @@ -1930,7 +1933,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;
>   
> @@ -2039,32 +2043,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)
>   {
> @@ -2404,8 +2382,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;
>   	}
> @@ -2413,13 +2392,13 @@ 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->main_clk = devm_clk_get(svsp->dev, "main");
> @@ -2427,12 +2406,12 @@ 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 = svs_clk_enable(svsp);
>   	if (ret)
> -		goto svs_probe_free_resource;
> +		goto svs_probe_free_tefuse;
>   
>   	svsp->base = of_iomap(svsp->dev->of_node, 0);
>   	if (IS_ERR_OR_NULL(svsp->base)) {
> @@ -2477,7 +2456,7 @@ static int svs_probe(struct platform_device *pdev)
>   svs_probe_clk_disable:
>   	svs_clk_disable(svsp);
>   
> -svs_probe_free_resource:
> +svs_probe_free_tefuse:
>   	if (!IS_ERR_OR_NULL(svsp->tefuse))
>   		kfree(svsp->tefuse);
>   

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 13/14] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Matthias Brugger]

On 11/01/2023 08:45, 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>
Same here, change looks good. Could you please rebase and resend:

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.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 a7f0a6f02d52..89117807e85d 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -648,6 +648,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)
>   {
> @@ -724,7 +743,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;
>   
> @@ -740,16 +758,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);
> @@ -1532,16 +1542,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;
> @@ -1587,7 +1588,6 @@ 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;
>   
> @@ -1599,17 +1599,7 @@ static int svs_suspend(struct device *dev)
>   
>   	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);

Re: [PATCH v4 13/14] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Matthias Brugger]

On 11/01/2023 08:45, 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>
Same here, change looks good. Could you please rebase and resend:

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.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 a7f0a6f02d52..89117807e85d 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -648,6 +648,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)
>   {
> @@ -724,7 +743,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;
>   
> @@ -740,16 +758,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);
> @@ -1532,16 +1542,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;
> @@ -1587,7 +1588,6 @@ 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;
>   
> @@ -1599,17 +1599,7 @@ static int svs_suspend(struct device *dev)
>   
>   	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);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 14/14] soc: mtk-svs: mt8183: refactor o_slope calculation

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> The o_slope value is dependent of the o_slope_sign, refactor code to get
> rid of unnecessary if constructs.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 51 +++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 89117807e85d..8cfbf8ffb138 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1944,26 +1944,27 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   	o_slope_sign = (svsp->tefuse[0] >> 7) & BIT(0);
>   
>   	ts_id = (svsp->tefuse[1] >> 9) & BIT(0);
> -	o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
> -
> -	if (adc_cali_en_t == 1) {
> -		if (!ts_id)
> -			o_slope = 0;
> -
> -		if (adc_ge_t < 265 || adc_ge_t > 758 ||
> -		    adc_oe_t < 265 || adc_oe_t > 758 ||
> -		    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
> -		    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
> -		    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
> -		    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
> -		    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
> -		    o_vtsabb < -8 || o_vtsabb > 484 ||
> -		    degc_cali < 1 || degc_cali > 63) {
> -			dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
> -			goto remove_mt8183_svsb_mon_mode;
> -		}
> +	if (!ts_id) {
> +		o_slope = 1534;
>   	} else {
> -		dev_err(svsp->dev, "no thermal efuse, no mon mode\n");
> +		o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
> +		if (!o_slope_sign)
> +			o_slope = 1534 + o_slope * 10;
> +		else
> +			o_slope = 1534 - o_slope * 10;
> +	}
> +
> +	if (adc_cali_en_t == 0 ||
> +	    adc_ge_t < 265 || adc_ge_t > 758 ||
> +	    adc_oe_t < 265 || adc_oe_t > 758 ||
> +	    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
> +	    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
> +	    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
> +	    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
> +	    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
> +	    o_vtsabb < -8 || o_vtsabb > 484 ||
> +	    degc_cali < 1 || degc_cali > 63) {
> +		dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
>   		goto remove_mt8183_svsb_mon_mode;
>   	}
>   
> @@ -1982,11 +1983,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / gain;
>   
>   	temp0 = (10000 * 100000 / gain) * 15 / 18;
> -
> -	if (!o_slope_sign)
> -		mts = (temp0 * 10) / (1534 + o_slope * 10);
> -	else
> -		mts = (temp0 * 10) / (1534 - o_slope * 10);
> +	mts = (temp0 * 10) / o_slope;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> @@ -2013,11 +2010,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		temp0 = (degc_cali * 10 / 2);
>   		temp1 = ((10000 * 100000 / 4096 / gain) *
>   			 oe + tb_roomt * 10) * 15 / 18;
> -
> -		if (!o_slope_sign)
> -			temp2 = temp1 * 100 / (1534 + o_slope * 10);
> -		else
> -			temp2 = temp1 * 100 / (1534 - o_slope * 10);
> +		temp2 = temp1 * 100 / o_slope;
>   
>   		svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
>   	}

Re: [PATCH v4 14/14] soc: mtk-svs: mt8183: refactor o_slope calculation

[Matthias Brugger]

On 11/01/2023 08:45, Roger Lu wrote:
> The o_slope value is dependent of the o_slope_sign, refactor code to get
> rid of unnecessary if constructs.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Applied, thanks!

> ---
>   drivers/soc/mediatek/mtk-svs.c | 51 +++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> index 89117807e85d..8cfbf8ffb138 100644
> --- a/drivers/soc/mediatek/mtk-svs.c
> +++ b/drivers/soc/mediatek/mtk-svs.c
> @@ -1944,26 +1944,27 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   	o_slope_sign = (svsp->tefuse[0] >> 7) & BIT(0);
>   
>   	ts_id = (svsp->tefuse[1] >> 9) & BIT(0);
> -	o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
> -
> -	if (adc_cali_en_t == 1) {
> -		if (!ts_id)
> -			o_slope = 0;
> -
> -		if (adc_ge_t < 265 || adc_ge_t > 758 ||
> -		    adc_oe_t < 265 || adc_oe_t > 758 ||
> -		    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
> -		    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
> -		    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
> -		    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
> -		    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
> -		    o_vtsabb < -8 || o_vtsabb > 484 ||
> -		    degc_cali < 1 || degc_cali > 63) {
> -			dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
> -			goto remove_mt8183_svsb_mon_mode;
> -		}
> +	if (!ts_id) {
> +		o_slope = 1534;
>   	} else {
> -		dev_err(svsp->dev, "no thermal efuse, no mon mode\n");
> +		o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0);
> +		if (!o_slope_sign)
> +			o_slope = 1534 + o_slope * 10;
> +		else
> +			o_slope = 1534 - o_slope * 10;
> +	}
> +
> +	if (adc_cali_en_t == 0 ||
> +	    adc_ge_t < 265 || adc_ge_t > 758 ||
> +	    adc_oe_t < 265 || adc_oe_t > 758 ||
> +	    o_vtsmcu[0] < -8 || o_vtsmcu[0] > 484 ||
> +	    o_vtsmcu[1] < -8 || o_vtsmcu[1] > 484 ||
> +	    o_vtsmcu[2] < -8 || o_vtsmcu[2] > 484 ||
> +	    o_vtsmcu[3] < -8 || o_vtsmcu[3] > 484 ||
> +	    o_vtsmcu[4] < -8 || o_vtsmcu[4] > 484 ||
> +	    o_vtsabb < -8 || o_vtsabb > 484 ||
> +	    degc_cali < 1 || degc_cali > 63) {
> +		dev_err(svsp->dev, "bad thermal efuse, no mon mode\n");
>   		goto remove_mt8183_svsb_mon_mode;
>   	}
>   
> @@ -1982,11 +1983,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / gain;
>   
>   	temp0 = (10000 * 100000 / gain) * 15 / 18;
> -
> -	if (!o_slope_sign)
> -		mts = (temp0 * 10) / (1534 + o_slope * 10);
> -	else
> -		mts = (temp0 * 10) / (1534 - o_slope * 10);
> +	mts = (temp0 * 10) / o_slope;
>   
>   	for (idx = 0; idx < svsp->bank_max; idx++) {
>   		svsb = &svsp->banks[idx];
> @@ -2013,11 +2010,7 @@ static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp)
>   		temp0 = (degc_cali * 10 / 2);
>   		temp1 = ((10000 * 100000 / 4096 / gain) *
>   			 oe + tb_roomt * 10) * 15 / 18;
> -
> -		if (!o_slope_sign)
> -			temp2 = temp1 * 100 / (1534 + o_slope * 10);
> -		else
> -			temp2 = temp1 * 100 / (1534 - o_slope * 10);
> +		temp2 = temp1 * 100 / o_slope;
>   
>   		svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
>   	}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 13/14] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:40 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, 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>
> 
> Same here, change looks good. Could you please rebase and resend:

No Problem, I'll rebase and resend it. Thanks.

> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

... [snip] ...

Re: [PATCH v4 13/14] soc: mediatek: mtk-svs: use common function to disable restore voltages

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:40 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, 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>
> 
> Same here, change looks good. Could you please rebase and resend:

No Problem, I'll rebase and resend it. Thanks.

> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

... [snip] ...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 12/14] soc: mediatek: mtk-svs: use svs get efuse common function

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:37 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, 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>
> 
> Looks good to me, could you please rebase.

No Problem, I'll rebase and resend it. Thanks.

> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

... [snip] ...

Re: [PATCH v4 12/14] soc: mediatek: mtk-svs: use svs get efuse common function

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:37 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, 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>
> 
> Looks good to me, could you please rebase.

No Problem, I'll rebase and resend it. Thanks.

> 
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

... [snip] ...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, Roger Lu wrote:
> > In MediaTek HW design, svs and thermal both use the same clk source.
> > It means that svs clk reference count from CCF includes thermal control
> > counts. That makes svs driver confuse whether it disabled svs's main clk
> > or not from CCF's perspective and lead to turn off their shared clk
> > unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> > main clk is controlled well by svs driver itself.
> > 
> > Here is a NG example. Rely on CCF's reference count and cause problem.
> > 
> > thermal probe (clk ref = 1)
> > -> svs probe (clk ref = 2)
> >     -> svs suspend (clk ref = 1)
> >        -> thermal suspend (clk ref = 0)
> >        -> thermal resume (clk ref = 1)
> >     -> svs resume (encounter error, clk ref = 1)
> >     -> svs suspend (clk ref = 0)
> >        -> thermal suspend (Fail here, thermal HW control w/o clk)
> > 
> > Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on
> > err in svs_resume()")
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> 
> That looks wrong. Although I don't out of my mind, there should be a way to
> tell 
> the clock framework that this clock is shared between several devices.
> 
> I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't
> be 
> enough.

Oh yes, Common Clock Framework (CCF) knows the clock shared between several
devices and maintains clock "on/off" by reference count.

We concern how to stop running svs_suspend() when svs clk is already disabled by
svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
result for knowing svs clk status, it will return "true" all the time because
thermal clk is still on. This causes the problem mentioned in commit message.

static int svs_suspend(struct device *dev)
{
...
	if (!__clk_is_enabled(svsp->main_clk)) //always get `true`
		return 0;
...
}

> 
> Regards,
> Matthias

... [snip] ...

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, Roger Lu wrote:
> > In MediaTek HW design, svs and thermal both use the same clk source.
> > It means that svs clk reference count from CCF includes thermal control
> > counts. That makes svs driver confuse whether it disabled svs's main clk
> > or not from CCF's perspective and lead to turn off their shared clk
> > unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> > main clk is controlled well by svs driver itself.
> > 
> > Here is a NG example. Rely on CCF's reference count and cause problem.
> > 
> > thermal probe (clk ref = 1)
> > -> svs probe (clk ref = 2)
> >     -> svs suspend (clk ref = 1)
> >        -> thermal suspend (clk ref = 0)
> >        -> thermal resume (clk ref = 1)
> >     -> svs resume (encounter error, clk ref = 1)
> >     -> svs suspend (clk ref = 0)
> >        -> thermal suspend (Fail here, thermal HW control w/o clk)
> > 
> > Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on
> > err in svs_resume()")
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> 
> That looks wrong. Although I don't out of my mind, there should be a way to
> tell 
> the clock framework that this clock is shared between several devices.
> 
> I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't
> be 
> enough.

Oh yes, Common Clock Framework (CCF) knows the clock shared between several
devices and maintains clock "on/off" by reference count.

We concern how to stop running svs_suspend() when svs clk is already disabled by
svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
result for knowing svs clk status, it will return "true" all the time because
thermal clk is still on. This causes the problem mentioned in commit message.

static int svs_suspend(struct device *dev)
{
...
	if (!__clk_is_enabled(svsp->main_clk)) //always get `true`
		return 0;
...
}

> 
> Regards,
> Matthias

... [snip] ...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 13:59 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, Roger Lu wrote:
> > From: Ricardo Ribalda <ribalda@chromium.org>
> > 
> > If the system does not come from reset (like when is booted via
> > kexec(), the peripheral might trigger an IRQ before the data structures
> > are initialized.
> > 
> > Fixes:
> > 
> > [    0.227710] Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000f08
> > [    0.227913] Call trace:
> > [    0.227918]  svs_isr+0x8c/0x538
> > 
> 
> This patch is different from
> https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org
> 
> If you think the patch from Ricardo is wrong, then you should have mentioned 
> that in the review.

My bad. Ricardo's patch is correct. Moving platform_get_irq() brings no
functional change and is for helping the svs probe readability as my preference.

> 
> Regards,
> Matthias
> 
> > Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> >   drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> > index a803b92afc3d..64d4d03ab71c 100644
> > --- a/drivers/soc/mediatek/mtk-svs.c
> > +++ b/drivers/soc/mediatek/mtk-svs.c
> > @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
> >   		goto svs_probe_free_resource;
> >   	}
> >   
> > -	svsp_irq = platform_get_irq(pdev, 0);
> > -	if (svsp_irq < 0) {
> > -		ret = svsp_irq;
> > -		goto svs_probe_free_resource;
> > -	}
> > -
> > -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> > -					IRQF_ONESHOT, svsp->name, svsp);
> > -	if (ret) {
> > -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> > -			svsp_irq, ret);
> > -		goto svs_probe_free_resource;
> > -	}
> > -
> >   	svsp->main_clk = devm_clk_get(svsp->dev, "main");
> >   	if (IS_ERR(svsp->main_clk)) {
> >   		dev_err(svsp->dev, "failed to get clock: %ld\n",
> > @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
> >   		goto svs_probe_clk_disable;
> >   	}
> >   
> > +	svsp_irq = platform_get_irq(pdev, 0);
> > +	if (svsp_irq < 0) {
> > +		ret = svsp_irq;
> > +		goto svs_probe_iounmap;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> > +					IRQF_ONESHOT, svsp->name, svsp);
> > +	if (ret) {
> > +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> > +			svsp_irq, ret);
> > +		goto svs_probe_iounmap;
> > +	}
> > +
> >   	ret = svs_start(svsp);
> >   	if (ret) {
> >   		dev_err(svsp->dev, "svs start fail: %d\n", ret);

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Tue, 2023-01-31 at 13:59 +0100, Matthias Brugger wrote:
> 
> On 11/01/2023 08:45, Roger Lu wrote:
> > From: Ricardo Ribalda <ribalda@chromium.org>
> > 
> > If the system does not come from reset (like when is booted via
> > kexec(), the peripheral might trigger an IRQ before the data structures
> > are initialized.
> > 
> > Fixes:
> > 
> > [    0.227710] Unable to handle kernel NULL pointer dereference at virtual
> > address 0000000000000f08
> > [    0.227913] Call trace:
> > [    0.227918]  svs_isr+0x8c/0x538
> > 
> 
> This patch is different from
> https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org
> 
> If you think the patch from Ricardo is wrong, then you should have mentioned 
> that in the review.

My bad. Ricardo's patch is correct. Moving platform_get_irq() brings no
functional change and is for helping the svs probe readability as my preference.

> 
> Regards,
> Matthias
> 
> > Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> >   drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> > index a803b92afc3d..64d4d03ab71c 100644
> > --- a/drivers/soc/mediatek/mtk-svs.c
> > +++ b/drivers/soc/mediatek/mtk-svs.c
> > @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
> >   		goto svs_probe_free_resource;
> >   	}
> >   
> > -	svsp_irq = platform_get_irq(pdev, 0);
> > -	if (svsp_irq < 0) {
> > -		ret = svsp_irq;
> > -		goto svs_probe_free_resource;
> > -	}
> > -
> > -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> > -					IRQF_ONESHOT, svsp->name, svsp);
> > -	if (ret) {
> > -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> > -			svsp_irq, ret);
> > -		goto svs_probe_free_resource;
> > -	}
> > -
> >   	svsp->main_clk = devm_clk_get(svsp->dev, "main");
> >   	if (IS_ERR(svsp->main_clk)) {
> >   		dev_err(svsp->dev, "failed to get clock: %ld\n",
> > @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
> >   		goto svs_probe_clk_disable;
> >   	}
> >   
> > +	svsp_irq = platform_get_irq(pdev, 0);
> > +	if (svsp_irq < 0) {
> > +		ret = svsp_irq;
> > +		goto svs_probe_iounmap;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
> > +					IRQF_ONESHOT, svsp->name, svsp);
> > +	if (ret) {
> > +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
> > +			svsp_irq, ret);
> > +		goto svs_probe_iounmap;
> > +	}
> > +
> >   	ret = svs_start(svsp);
> >   	if (ret) {
> >   		dev_err(svsp->dev, "svs start fail: %d\n", ret);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Matthias Brugger]

On 01/02/2023 14:43, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> On Tue, 2023-01-31 at 13:59 +0100, Matthias Brugger wrote:
>>
>> On 11/01/2023 08:45, Roger Lu wrote:
>>> From: Ricardo Ribalda <ribalda@chromium.org>
>>>
>>> If the system does not come from reset (like when is booted via
>>> kexec(), the peripheral might trigger an IRQ before the data structures
>>> are initialized.
>>>
>>> Fixes:
>>>
>>> [    0.227710] Unable to handle kernel NULL pointer dereference at virtual
>>> address 0000000000000f08
>>> [    0.227913] Call trace:
>>> [    0.227918]  svs_isr+0x8c/0x538
>>>
>>
>> This patch is different from
>> https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org
>>
>> If you think the patch from Ricardo is wrong, then you should have mentioned
>> that in the review.
> 
> My bad. Ricardo's patch is correct. Moving platform_get_irq() brings no
> functional change and is for helping the svs probe readability as my preference.
> 

Thanks for the confirmation!

Regards,
Matthias

>>
>> Regards,
>> Matthias
>>
>>> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
>>>    1 file changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
>>> index a803b92afc3d..64d4d03ab71c 100644
>>> --- a/drivers/soc/mediatek/mtk-svs.c
>>> +++ b/drivers/soc/mediatek/mtk-svs.c
>>> @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
>>>    		goto svs_probe_free_resource;
>>>    	}
>>>    
>>> -	svsp_irq = platform_get_irq(pdev, 0);
>>> -	if (svsp_irq < 0) {
>>> -		ret = svsp_irq;
>>> -		goto svs_probe_free_resource;
>>> -	}
>>> -
>>> -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
>>> -					IRQF_ONESHOT, svsp->name, svsp);
>>> -	if (ret) {
>>> -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>>> -			svsp_irq, ret);
>>> -		goto svs_probe_free_resource;
>>> -	}
>>> -
>>>    	svsp->main_clk = devm_clk_get(svsp->dev, "main");
>>>    	if (IS_ERR(svsp->main_clk)) {
>>>    		dev_err(svsp->dev, "failed to get clock: %ld\n",
>>> @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
>>>    		goto svs_probe_clk_disable;
>>>    	}
>>>    
>>> +	svsp_irq = platform_get_irq(pdev, 0);
>>> +	if (svsp_irq < 0) {
>>> +		ret = svsp_irq;
>>> +		goto svs_probe_iounmap;
>>> +	}
>>> +
>>> +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
>>> +					IRQF_ONESHOT, svsp->name, svsp);
>>> +	if (ret) {
>>> +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>>> +			svsp_irq, ret);
>>> +		goto svs_probe_iounmap;
>>> +	}
>>> +
>>>    	ret = svs_start(svsp);
>>>    	if (ret) {
>>>    		dev_err(svsp->dev, "svs start fail: %d\n", ret);

Re: [PATCH v4 03/14] soc: mediatek: mtk-svs: enable the IRQ later

[Matthias Brugger]

On 01/02/2023 14:43, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> On Tue, 2023-01-31 at 13:59 +0100, Matthias Brugger wrote:
>>
>> On 11/01/2023 08:45, Roger Lu wrote:
>>> From: Ricardo Ribalda <ribalda@chromium.org>
>>>
>>> If the system does not come from reset (like when is booted via
>>> kexec(), the peripheral might trigger an IRQ before the data structures
>>> are initialized.
>>>
>>> Fixes:
>>>
>>> [    0.227710] Unable to handle kernel NULL pointer dereference at virtual
>>> address 0000000000000f08
>>> [    0.227913] Call trace:
>>> [    0.227918]  svs_isr+0x8c/0x538
>>>
>>
>> This patch is different from
>> https://lore.kernel.org/r/20221127-mtk-svs-v2-0-145b07663ea8@chromium.org
>>
>> If you think the patch from Ricardo is wrong, then you should have mentioned
>> that in the review.
> 
> My bad. Ricardo's patch is correct. Moving platform_get_irq() brings no
> functional change and is for helping the svs probe readability as my preference.
> 

Thanks for the confirmation!

Regards,
Matthias

>>
>> Regards,
>> Matthias
>>
>>> Fixes: 681a02e95000 ("soc: mediatek: SVS: introduce MTK SVS engine")
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-svs.c | 28 ++++++++++++++--------------
>>>    1 file changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
>>> index a803b92afc3d..64d4d03ab71c 100644
>>> --- a/drivers/soc/mediatek/mtk-svs.c
>>> +++ b/drivers/soc/mediatek/mtk-svs.c
>>> @@ -2403,20 +2403,6 @@ static int svs_probe(struct platform_device *pdev)
>>>    		goto svs_probe_free_resource;
>>>    	}
>>>    
>>> -	svsp_irq = platform_get_irq(pdev, 0);
>>> -	if (svsp_irq < 0) {
>>> -		ret = svsp_irq;
>>> -		goto svs_probe_free_resource;
>>> -	}
>>> -
>>> -	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
>>> -					IRQF_ONESHOT, svsp->name, svsp);
>>> -	if (ret) {
>>> -		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>>> -			svsp_irq, ret);
>>> -		goto svs_probe_free_resource;
>>> -	}
>>> -
>>>    	svsp->main_clk = devm_clk_get(svsp->dev, "main");
>>>    	if (IS_ERR(svsp->main_clk)) {
>>>    		dev_err(svsp->dev, "failed to get clock: %ld\n",
>>> @@ -2438,6 +2424,20 @@ static int svs_probe(struct platform_device *pdev)
>>>    		goto svs_probe_clk_disable;
>>>    	}
>>>    
>>> +	svsp_irq = platform_get_irq(pdev, 0);
>>> +	if (svsp_irq < 0) {
>>> +		ret = svsp_irq;
>>> +		goto svs_probe_iounmap;
>>> +	}
>>> +
>>> +	ret = devm_request_threaded_irq(svsp->dev, svsp_irq, NULL, svs_isr,
>>> +					IRQF_ONESHOT, svsp->name, svsp);
>>> +	if (ret) {
>>> +		dev_err(svsp->dev, "register irq(%d) failed: %d\n",
>>> +			svsp_irq, ret);
>>> +		goto svs_probe_iounmap;
>>> +	}
>>> +
>>>    	ret = svs_start(svsp);
>>>    	if (ret) {
>>>    		dev_err(svsp->dev, "svs start fail: %d\n", ret);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

你好，

On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
>>
>> On 11/01/2023 08:45, Roger Lu wrote:
>>> In MediaTek HW design, svs and thermal both use the same clk source.
>>> It means that svs clk reference count from CCF includes thermal control
>>> counts. That makes svs driver confuse whether it disabled svs's main clk
>>> or not from CCF's perspective and lead to turn off their shared clk
>>> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
>>> main clk is controlled well by svs driver itself.
>>>
>>> Here is a NG example. Rely on CCF's reference count and cause problem.
>>>
>>> thermal probe (clk ref = 1)
>>> -> svs probe (clk ref = 2)
>>>      -> svs suspend (clk ref = 1)
>>>         -> thermal suspend (clk ref = 0)
>>>         -> thermal resume (clk ref = 1)
>>>      -> svs resume (encounter error, clk ref = 1)
>>>      -> svs suspend (clk ref = 0)
>>>         -> thermal suspend (Fail here, thermal HW control w/o clk)
>>>
>>> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on
>>> err in svs_resume()")
>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>
>> That looks wrong. Although I don't out of my mind, there should be a way to
>> tell
>> the clock framework that this clock is shared between several devices.
>>
>> I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't
>> be
>> enough.
> 
> Oh yes, Common Clock Framework (CCF) knows the clock shared between several
> devices and maintains clock "on/off" by reference count.
> 

The thing is if you use clk_prepare_enable then the clock framework check's if 
the clock is already prepared, which could happen like you described in the 
svs_resume (encount error) case in the commit message. The question is, can't we 
just use clk_enable and clk_disable in resume/suspend and only prepare the clock 
in the probe function?

> We concern how to stop running svs_suspend() when svs clk is already disabled by
> svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
> result for knowing svs clk status, it will return "true" all the time because
> thermal clk is still on. This causes the problem mentioned in commit message.
> 

I would expect that the kernel takes care that we can't enter a resume path for 
a device before the suspend path has finished. Honestly I don't really 
understand the problem here. It seems something different then what you 
described in the commit message.

Please help me understand better.

谢谢，再见

Matthias

> static int svs_suspend(struct device *dev)
> {
> ...
> 	if (!__clk_is_enabled(svsp->main_clk)) //always get `true`
> 		return 0;
> ...
> }
> 
>>
>> Regards,
>> Matthias
> 
> ... [snip] ...

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

你好，

On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
>>
>> On 11/01/2023 08:45, Roger Lu wrote:
>>> In MediaTek HW design, svs and thermal both use the same clk source.
>>> It means that svs clk reference count from CCF includes thermal control
>>> counts. That makes svs driver confuse whether it disabled svs's main clk
>>> or not from CCF's perspective and lead to turn off their shared clk
>>> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
>>> main clk is controlled well by svs driver itself.
>>>
>>> Here is a NG example. Rely on CCF's reference count and cause problem.
>>>
>>> thermal probe (clk ref = 1)
>>> -> svs probe (clk ref = 2)
>>>      -> svs suspend (clk ref = 1)
>>>         -> thermal suspend (clk ref = 0)
>>>         -> thermal resume (clk ref = 1)
>>>      -> svs resume (encounter error, clk ref = 1)
>>>      -> svs suspend (clk ref = 0)
>>>         -> thermal suspend (Fail here, thermal HW control w/o clk)
>>>
>>> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare() on
>>> err in svs_resume()")
>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>
>> That looks wrong. Although I don't out of my mind, there should be a way to
>> tell
>> the clock framework that this clock is shared between several devices.
>>
>> I wonder if using clk_enable and clk_disable in svs_resume/suspend wouldn't
>> be
>> enough.
> 
> Oh yes, Common Clock Framework (CCF) knows the clock shared between several
> devices and maintains clock "on/off" by reference count.
> 

The thing is if you use clk_prepare_enable then the clock framework check's if 
the clock is already prepared, which could happen like you described in the 
svs_resume (encount error) case in the commit message. The question is, can't we 
just use clk_enable and clk_disable in resume/suspend and only prepare the clock 
in the probe function?

> We concern how to stop running svs_suspend() when svs clk is already disabled by
> svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
> result for knowing svs clk status, it will return "true" all the time because
> thermal clk is still on. This causes the problem mentioned in commit message.
> 

I would expect that the kernel takes care that we can't enter a resume path for 
a device before the suspend path has finished. Honestly I don't really 
understand the problem here. It seems something different then what you 
described in the commit message.

Please help me understand better.

谢谢，再见

Matthias

> static int svs_suspend(struct device *dev)
> {
> ...
> 	if (!__clk_is_enabled(svsp->main_clk)) //always get `true`
> 		return 0;
> ...
> }
> 
>>
>> Regards,
>> Matthias
> 
> ... [snip] ...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,


On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
> 你好，

I got shock and thought someone used your name to reply. However,
your email account helps me clear my mind. Haha.. Nice and warm to see mandarin
on patchwork. It's so fresh and exciting :-).

> 
> On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> > Hi Matthias Sir,
> > 
> > On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> > > 
> > > On 11/01/2023 08:45, Roger Lu wrote:
> > > > In MediaTek HW design, svs and thermal both use the same clk source.
> > > > It means that svs clk reference count from CCF includes thermal control
> > > > counts. That makes svs driver confuse whether it disabled svs's main clk
> > > > or not from CCF's perspective and lead to turn off their shared clk
> > > > unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> > > > main clk is controlled well by svs driver itself.
> > > > 
> > > > Here is a NG example. Rely on CCF's reference count and cause problem.
> > > > 
> > > > thermal probe (clk ref = 1)
> > > > -> svs probe (clk ref = 2)
> > > >      -> svs suspend (clk ref = 1)
> > > >         -> thermal suspend (clk ref = 0)
> > > >         -> thermal resume (clk ref = 1)
> > > >      -> svs resume (encounter error, clk ref = 1)
> > > >      -> svs suspend (clk ref = 0)
> > > >         -> thermal suspend (Fail here, thermal HW control w/o clk)
> > > > 
> > > > Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare()
> > > > on
> > > > err in svs_resume()")
> > > > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > > 
> > > That looks wrong. Although I don't out of my mind, there should be a way
> > > to
> > > tell
> > > the clock framework that this clock is shared between several devices.
> > > 
> > > I wonder if using clk_enable and clk_disable in svs_resume/suspend
> > > wouldn't
> > > be
> > > enough.
> > 
> > Oh yes, Common Clock Framework (CCF) knows the clock shared between several
> > devices and maintains clock "on/off" by reference count.
> > 
> 
> The thing is if you use clk_prepare_enable then the clock framework check's
> if 
> the clock is already prepared, which could happen like you described in the 
> svs_resume (encount error) case in the commit message. The question is, can't
> we 
> just use clk_enable and clk_disable in resume/suspend and only prepare the
> clock 
> in the probe function?

We'll think if this can fix the problem. Thanks for the advice very much.

> 
> > We concern how to stop running svs_suspend() when svs clk is already
> > disabled by
> > svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
> > result for knowing svs clk status, it will return "true" all the time
> > because
> > thermal clk is still on. This causes the problem mentioned in commit
> > message.
> > 
> 
> I would expect that the kernel takes care that we can't enter a resume path
> for 
> a device before the suspend path has finished. Honestly I don't really 
> understand the problem here. It seems something different then what you 
> described in the commit message.
> 
> Please help me understand better.

I see. This patch title needs to be changed to "avoid turning off svs clk twice
unexpectedly" for pointing out the problem precisely. We saw a loophole that svs
clk might be turned off in svs_resume() first and in svs_suspend() again without
enabling svs clk during these the process. Therefore, we try to fix it by this
patch. Below is our thinking process to explain how we got here.

1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent svs
clk from being turned off twice when svs_resume() turned off svs clk in the
error-handling process. Nonetheless, we met the NG case in the commit message.
2. (current patch) We add svs clk control hint to understand if we need to run
svs_suspend() or not if svs_resume() turned off svs clk before.

> 
> 谢谢，再见

:-)


Sincerely,
Roger Lu

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,


On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
> 你好，

I got shock and thought someone used your name to reply. However,
your email account helps me clear my mind. Haha.. Nice and warm to see mandarin
on patchwork. It's so fresh and exciting :-).

> 
> On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> > Hi Matthias Sir,
> > 
> > On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> > > 
> > > On 11/01/2023 08:45, Roger Lu wrote:
> > > > In MediaTek HW design, svs and thermal both use the same clk source.
> > > > It means that svs clk reference count from CCF includes thermal control
> > > > counts. That makes svs driver confuse whether it disabled svs's main clk
> > > > or not from CCF's perspective and lead to turn off their shared clk
> > > > unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
> > > > main clk is controlled well by svs driver itself.
> > > > 
> > > > Here is a NG example. Rely on CCF's reference count and cause problem.
> > > > 
> > > > thermal probe (clk ref = 1)
> > > > -> svs probe (clk ref = 2)
> > > >      -> svs suspend (clk ref = 1)
> > > >         -> thermal suspend (clk ref = 0)
> > > >         -> thermal resume (clk ref = 1)
> > > >      -> svs resume (encounter error, clk ref = 1)
> > > >      -> svs suspend (clk ref = 0)
> > > >         -> thermal suspend (Fail here, thermal HW control w/o clk)
> > > > 
> > > > Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare()
> > > > on
> > > > err in svs_resume()")
> > > > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > > 
> > > That looks wrong. Although I don't out of my mind, there should be a way
> > > to
> > > tell
> > > the clock framework that this clock is shared between several devices.
> > > 
> > > I wonder if using clk_enable and clk_disable in svs_resume/suspend
> > > wouldn't
> > > be
> > > enough.
> > 
> > Oh yes, Common Clock Framework (CCF) knows the clock shared between several
> > devices and maintains clock "on/off" by reference count.
> > 
> 
> The thing is if you use clk_prepare_enable then the clock framework check's
> if 
> the clock is already prepared, which could happen like you described in the 
> svs_resume (encount error) case in the commit message. The question is, can't
> we 
> just use clk_enable and clk_disable in resume/suspend and only prepare the
> clock 
> in the probe function?

We'll think if this can fix the problem. Thanks for the advice very much.

> 
> > We concern how to stop running svs_suspend() when svs clk is already
> > disabled by
> > svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
> > result for knowing svs clk status, it will return "true" all the time
> > because
> > thermal clk is still on. This causes the problem mentioned in commit
> > message.
> > 
> 
> I would expect that the kernel takes care that we can't enter a resume path
> for 
> a device before the suspend path has finished. Honestly I don't really 
> understand the problem here. It seems something different then what you 
> described in the commit message.
> 
> Please help me understand better.

I see. This patch title needs to be changed to "avoid turning off svs clk twice
unexpectedly" for pointing out the problem precisely. We saw a loophole that svs
clk might be turned off in svs_resume() first and in svs_suspend() again without
enabling svs clk during these the process. Therefore, we try to fix it by this
patch. Below is our thinking process to explain how we got here.

1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent svs
clk from being turned off twice when svs_resume() turned off svs clk in the
error-handling process. Nonetheless, we met the NG case in the commit message.
2. (current patch) We add svs clk control hint to understand if we need to run
svs_suspend() or not if svs_resume() turned off svs clk before.

> 
> 谢谢，再见

:-)


Sincerely,
Roger Lu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

On 06/02/2023 03:01, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> 
> On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
>> 你好，
> 
> I got shock and thought someone used your name to reply. However,
> your email account helps me clear my mind. Haha.. Nice and warm to see mandarin
> on patchwork. It's so fresh and exciting :-).
> 

谢谢。 I'm learning mainland Chinese for a few month now, I also learned that you 
use different symbols in Taiwan, which I don't know. 对不起。

>>
>> On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
>>> Hi Matthias Sir,
>>>
>>> On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
>>>>
>>>> On 11/01/2023 08:45, Roger Lu wrote:
>>>>> In MediaTek HW design, svs and thermal both use the same clk source.
>>>>> It means that svs clk reference count from CCF includes thermal control
>>>>> counts. That makes svs driver confuse whether it disabled svs's main clk
>>>>> or not from CCF's perspective and lead to turn off their shared clk
>>>>> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
>>>>> main clk is controlled well by svs driver itself.
>>>>>
>>>>> Here is a NG example. Rely on CCF's reference count and cause problem.
>>>>>
>>>>> thermal probe (clk ref = 1)
>>>>> -> svs probe (clk ref = 2)
>>>>>       -> svs suspend (clk ref = 1)
>>>>>          -> thermal suspend (clk ref = 0)
>>>>>          -> thermal resume (clk ref = 1)
>>>>>       -> svs resume (encounter error, clk ref = 1)
>>>>>       -> svs suspend (clk ref = 0)
>>>>>          -> thermal suspend (Fail here, thermal HW control w/o clk)
>>>>>
>>>>> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare()
>>>>> on
>>>>> err in svs_resume()")
>>>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>>>
>>>> That looks wrong. Although I don't out of my mind, there should be a way
>>>> to
>>>> tell
>>>> the clock framework that this clock is shared between several devices.
>>>>
>>>> I wonder if using clk_enable and clk_disable in svs_resume/suspend
>>>> wouldn't
>>>> be
>>>> enough.
>>>
>>> Oh yes, Common Clock Framework (CCF) knows the clock shared between several
>>> devices and maintains clock "on/off" by reference count.
>>>
>>
>> The thing is if you use clk_prepare_enable then the clock framework check's
>> if
>> the clock is already prepared, which could happen like you described in the
>> svs_resume (encount error) case in the commit message. The question is, can't
>> we
>> just use clk_enable and clk_disable in resume/suspend and only prepare the
>> clock
>> in the probe function?
> 
> We'll think if this can fix the problem. Thanks for the advice very much.
> 
>>
>>> We concern how to stop running svs_suspend() when svs clk is already
>>> disabled by
>>> svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
>>> result for knowing svs clk status, it will return "true" all the time
>>> because
>>> thermal clk is still on. This causes the problem mentioned in commit
>>> message.
>>>
>>
>> I would expect that the kernel takes care that we can't enter a resume path
>> for
>> a device before the suspend path has finished. Honestly I don't really
>> understand the problem here. It seems something different then what you
>> described in the commit message.
>>
>> Please help me understand better.
> 
> I see. This patch title needs to be changed to "avoid turning off svs clk twice
> unexpectedly" for pointing out the problem precisely. We saw a loophole that svs
> clk might be turned off in svs_resume() first and in svs_suspend() again without
> enabling svs clk during these the process. Therefore, we try to fix it by this
> patch. Below is our thinking process to explain how we got here.
> 
> 1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent svs
> clk from being turned off twice when svs_resume() turned off svs clk in the
> error-handling process. Nonetheless, we met the NG case in the commit message.
> 2. (current patch) We add svs clk control hint to understand if we need to run
> svs_suspend() or not if svs_resume() turned off svs clk before.
> 

Did you had a look on the dev_pm_ops？ Maybe we can use suspend_late, 
resume_early to make sure there is no race condition. I wonder also if we can't 
make sure that this does not happen using device links. Sorry, I can't give 
better guidance on how to use this technologies, but I have the feeling we can 
fix this with existing infrastructure.

再见。

Matthias

>>
>> 谢谢，再见
> 
> :-)
> 
> 
> Sincerely,
> Roger Lu

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Matthias Brugger]

On 06/02/2023 03:01, Roger Lu (陸瑞傑) wrote:
> Hi Matthias Sir,
> 
> 
> On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
>> 你好，
> 
> I got shock and thought someone used your name to reply. However,
> your email account helps me clear my mind. Haha.. Nice and warm to see mandarin
> on patchwork. It's so fresh and exciting :-).
> 

谢谢。 I'm learning mainland Chinese for a few month now, I also learned that you 
use different symbols in Taiwan, which I don't know. 对不起。

>>
>> On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
>>> Hi Matthias Sir,
>>>
>>> On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
>>>>
>>>> On 11/01/2023 08:45, Roger Lu wrote:
>>>>> In MediaTek HW design, svs and thermal both use the same clk source.
>>>>> It means that svs clk reference count from CCF includes thermal control
>>>>> counts. That makes svs driver confuse whether it disabled svs's main clk
>>>>> or not from CCF's perspective and lead to turn off their shared clk
>>>>> unexpectedly. Therefore, we add svs clk control APIs to make sure svs's
>>>>> main clk is controlled well by svs driver itself.
>>>>>
>>>>> Here is a NG example. Rely on CCF's reference count and cause problem.
>>>>>
>>>>> thermal probe (clk ref = 1)
>>>>> -> svs probe (clk ref = 2)
>>>>>       -> svs suspend (clk ref = 1)
>>>>>          -> thermal suspend (clk ref = 0)
>>>>>          -> thermal resume (clk ref = 1)
>>>>>       -> svs resume (encounter error, clk ref = 1)
>>>>>       -> svs suspend (clk ref = 0)
>>>>>          -> thermal suspend (Fail here, thermal HW control w/o clk)
>>>>>
>>>>> Fixes: a825d72f74a3 ("soc: mediatek: fix missing clk_disable_unprepare()
>>>>> on
>>>>> err in svs_resume()")
>>>>> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
>>>>
>>>> That looks wrong. Although I don't out of my mind, there should be a way
>>>> to
>>>> tell
>>>> the clock framework that this clock is shared between several devices.
>>>>
>>>> I wonder if using clk_enable and clk_disable in svs_resume/suspend
>>>> wouldn't
>>>> be
>>>> enough.
>>>
>>> Oh yes, Common Clock Framework (CCF) knows the clock shared between several
>>> devices and maintains clock "on/off" by reference count.
>>>
>>
>> The thing is if you use clk_prepare_enable then the clock framework check's
>> if
>> the clock is already prepared, which could happen like you described in the
>> svs_resume (encount error) case in the commit message. The question is, can't
>> we
>> just use clk_enable and clk_disable in resume/suspend and only prepare the
>> clock
>> in the probe function?
> 
> We'll think if this can fix the problem. Thanks for the advice very much.
> 
>>
>>> We concern how to stop running svs_suspend() when svs clk is already
>>> disabled by
>>> svs_resume(). Take an example as below, if we refers to __clk_is_enabled()
>>> result for knowing svs clk status, it will return "true" all the time
>>> because
>>> thermal clk is still on. This causes the problem mentioned in commit
>>> message.
>>>
>>
>> I would expect that the kernel takes care that we can't enter a resume path
>> for
>> a device before the suspend path has finished. Honestly I don't really
>> understand the problem here. It seems something different then what you
>> described in the commit message.
>>
>> Please help me understand better.
> 
> I see. This patch title needs to be changed to "avoid turning off svs clk twice
> unexpectedly" for pointing out the problem precisely. We saw a loophole that svs
> clk might be turned off in svs_resume() first and in svs_suspend() again without
> enabling svs clk during these the process. Therefore, we try to fix it by this
> patch. Below is our thinking process to explain how we got here.
> 
> 1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent svs
> clk from being turned off twice when svs_resume() turned off svs clk in the
> error-handling process. Nonetheless, we met the NG case in the commit message.
> 2. (current patch) We add svs clk control hint to understand if we need to run
> svs_suspend() or not if svs_resume() turned off svs clk before.
> 

Did you had a look on the dev_pm_ops？ Maybe we can use suspend_late, 
resume_early to make sure there is no race condition. I wonder also if we can't 
make sure that this does not happen using device links. Sorry, I can't give 
better guidance on how to use this technologies, but I have the feeling we can 
fix this with existing infrastructure.

再见。

Matthias

>>
>> 谢谢，再见
> 
> :-)
> 
> 
> Sincerely,
> Roger Lu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Mon, 2023-02-06 at 13:09 +0100, Matthias Brugger wrote:
> 
> On 06/02/2023 03:01, Roger Lu (陸瑞傑) wrote:
> > Hi Matthias Sir,
> > 
> > 
> > On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
> > > 你好，
> > 
> > I got shock and thought someone used your name to reply. However,
> > your email account helps me clear my mind. Haha.. Nice and warm to see
> > mandarin
> > on patchwork. It's so fresh and exciting :-).
> > 
> 
> 谢谢。 I'm learning mainland Chinese for a few month now, I also learned that
> you 
> use different symbols in Taiwan, which I don't know. 对不起。

Ha. Both symbols are welcome to me.  :-)

> 
> > > 
> > > On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> > > > Hi Matthias Sir,
> > > > 
> > > > On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> > > > > 
> > > > > On 11/01/2023 08:45, Roger Lu wrote:
> > > > > > In MediaTek HW design, svs and thermal both use the same clk source.
> > > > > > It means that svs clk reference count from CCF includes thermal
> > > > > > control
> > > > > > counts. That makes svs driver confuse whether it disabled svs's main
> > > > > > clk
> > > > > > or not from CCF's perspective and lead to turn off their shared clk
> > > > > > unexpectedly. Therefore, we add svs clk control APIs to make sure
> > > > > > svs's
> > > > > > main clk is controlled well by svs driver itself.
> > > > > > 
> > > > > > Here is a NG example. Rely on CCF's reference count and cause
> > > > > > problem.
> > > > > > 
> > > > > > thermal probe (clk ref = 1)
> > > > > > -> svs probe (clk ref = 2)
> > > > > >       -> svs suspend (clk ref = 1)
> > > > > >          -> thermal suspend (clk ref = 0)
> > > > > >          -> thermal resume (clk ref = 1)
> > > > > >       -> svs resume (encounter error, clk ref = 1)
> > > > > >       -> svs suspend (clk ref = 0)
> > > > > >          -> thermal suspend (Fail here, thermal HW control w/o clk)
> > > > > > 
> > > > > > Fixes: a825d72f74a3 ("soc: mediatek: fix missing
> > > > > > clk_disable_unprepare()
> > > > > > on
> > > > > > err in svs_resume()")
> > > > > > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > > > > 
> > > > > That looks wrong. Although I don't out of my mind, there should be a
> > > > > way
> > > > > to
> > > > > tell
> > > > > the clock framework that this clock is shared between several devices.
> > > > > 
> > > > > I wonder if using clk_enable and clk_disable in svs_resume/suspend
> > > > > wouldn't
> > > > > be
> > > > > enough.
> > > > 
> > > > Oh yes, Common Clock Framework (CCF) knows the clock shared between
> > > > several
> > > > devices and maintains clock "on/off" by reference count.
> > > > 
> > > 
> > > The thing is if you use clk_prepare_enable then the clock framework
> > > check's
> > > if
> > > the clock is already prepared, which could happen like you described in
> > > the
> > > svs_resume (encount error) case in the commit message. The question is,
> > > can't
> > > we
> > > just use clk_enable and clk_disable in resume/suspend and only prepare the
> > > clock
> > > in the probe function?
> > 
> > We'll think if this can fix the problem. Thanks for the advice very much.
> > 
> > > 
> > > > We concern how to stop running svs_suspend() when svs clk is already
> > > > disabled by
> > > > svs_resume(). Take an example as below, if we refers to
> > > > __clk_is_enabled()
> > > > result for knowing svs clk status, it will return "true" all the time
> > > > because
> > > > thermal clk is still on. This causes the problem mentioned in commit
> > > > message.
> > > > 
> > > 
> > > I would expect that the kernel takes care that we can't enter a resume
> > > path
> > > for
> > > a device before the suspend path has finished. Honestly I don't really
> > > understand the problem here. It seems something different then what you
> > > described in the commit message.
> > > 
> > > Please help me understand better.
> > 
> > I see. This patch title needs to be changed to "avoid turning off svs clk
> > twice
> > unexpectedly" for pointing out the problem precisely. We saw a loophole that
> > svs
> > clk might be turned off in svs_resume() first and in svs_suspend() again
> > without
> > enabling svs clk during these the process. Therefore, we try to fix it by
> > this
> > patch. Below is our thinking process to explain how we got here.
> > 
> > 1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent
> > svs
> > clk from being turned off twice when svs_resume() turned off svs clk in the
> > error-handling process. Nonetheless, we met the NG case in the commit
> > message.
> > 2. (current patch) We add svs clk control hint to understand if we need to
> > run
> > svs_suspend() or not if svs_resume() turned off svs clk before.
> > 
> 
> Did you had a look on the dev_pm_ops？ Maybe we can use suspend_late, 
> resume_early to make sure there is no race condition. I wonder also if we
> can't 
> make sure that this does not happen using device links. Sorry, I can't give 
> better guidance on how to use this technologies, but I have the feeling we
> can 
> fix this with existing infrastructure.

No, we didn't look on dev_pm_ops and it seems like another way to fix this issue
with __clk_is_enabled() check in svs_suspend(). Thanks for the advice again.
we'll keep looking for any possible answers to this issue.

Sincerely,
Roger Lu.

Re: [PATCH v4 05/14] soc: mediatek: mtk-svs: use svs clk control APIs

[Roger Lu (陸瑞傑)]

Hi Matthias Sir,

On Mon, 2023-02-06 at 13:09 +0100, Matthias Brugger wrote:
> 
> On 06/02/2023 03:01, Roger Lu (陸瑞傑) wrote:
> > Hi Matthias Sir,
> > 
> > 
> > On Thu, 2023-02-02 at 11:29 +0100, Matthias Brugger wrote:
> > > 你好，
> > 
> > I got shock and thought someone used your name to reply. However,
> > your email account helps me clear my mind. Haha.. Nice and warm to see
> > mandarin
> > on patchwork. It's so fresh and exciting :-).
> > 
> 
> 谢谢。 I'm learning mainland Chinese for a few month now, I also learned that
> you 
> use different symbols in Taiwan, which I don't know. 对不起。

Ha. Both symbols are welcome to me.  :-)

> 
> > > 
> > > On 01/02/2023 13:28, Roger Lu (陸瑞傑) wrote:
> > > > Hi Matthias Sir,
> > > > 
> > > > On Tue, 2023-01-31 at 14:19 +0100, Matthias Brugger wrote:
> > > > > 
> > > > > On 11/01/2023 08:45, Roger Lu wrote:
> > > > > > In MediaTek HW design, svs and thermal both use the same clk source.
> > > > > > It means that svs clk reference count from CCF includes thermal
> > > > > > control
> > > > > > counts. That makes svs driver confuse whether it disabled svs's main
> > > > > > clk
> > > > > > or not from CCF's perspective and lead to turn off their shared clk
> > > > > > unexpectedly. Therefore, we add svs clk control APIs to make sure
> > > > > > svs's
> > > > > > main clk is controlled well by svs driver itself.
> > > > > > 
> > > > > > Here is a NG example. Rely on CCF's reference count and cause
> > > > > > problem.
> > > > > > 
> > > > > > thermal probe (clk ref = 1)
> > > > > > -> svs probe (clk ref = 2)
> > > > > >       -> svs suspend (clk ref = 1)
> > > > > >          -> thermal suspend (clk ref = 0)
> > > > > >          -> thermal resume (clk ref = 1)
> > > > > >       -> svs resume (encounter error, clk ref = 1)
> > > > > >       -> svs suspend (clk ref = 0)
> > > > > >          -> thermal suspend (Fail here, thermal HW control w/o clk)
> > > > > > 
> > > > > > Fixes: a825d72f74a3 ("soc: mediatek: fix missing
> > > > > > clk_disable_unprepare()
> > > > > > on
> > > > > > err in svs_resume()")
> > > > > > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > > > > 
> > > > > That looks wrong. Although I don't out of my mind, there should be a
> > > > > way
> > > > > to
> > > > > tell
> > > > > the clock framework that this clock is shared between several devices.
> > > > > 
> > > > > I wonder if using clk_enable and clk_disable in svs_resume/suspend
> > > > > wouldn't
> > > > > be
> > > > > enough.
> > > > 
> > > > Oh yes, Common Clock Framework (CCF) knows the clock shared between
> > > > several
> > > > devices and maintains clock "on/off" by reference count.
> > > > 
> > > 
> > > The thing is if you use clk_prepare_enable then the clock framework
> > > check's
> > > if
> > > the clock is already prepared, which could happen like you described in
> > > the
> > > svs_resume (encount error) case in the commit message. The question is,
> > > can't
> > > we
> > > just use clk_enable and clk_disable in resume/suspend and only prepare the
> > > clock
> > > in the probe function?
> > 
> > We'll think if this can fix the problem. Thanks for the advice very much.
> > 
> > > 
> > > > We concern how to stop running svs_suspend() when svs clk is already
> > > > disabled by
> > > > svs_resume(). Take an example as below, if we refers to
> > > > __clk_is_enabled()
> > > > result for knowing svs clk status, it will return "true" all the time
> > > > because
> > > > thermal clk is still on. This causes the problem mentioned in commit
> > > > message.
> > > > 
> > > 
> > > I would expect that the kernel takes care that we can't enter a resume
> > > path
> > > for
> > > a device before the suspend path has finished. Honestly I don't really
> > > understand the problem here. It seems something different then what you
> > > described in the commit message.
> > > 
> > > Please help me understand better.
> > 
> > I see. This patch title needs to be changed to "avoid turning off svs clk
> > twice
> > unexpectedly" for pointing out the problem precisely. We saw a loophole that
> > svs
> > clk might be turned off in svs_resume() first and in svs_suspend() again
> > without
> > enabling svs clk during these the process. Therefore, we try to fix it by
> > this
> > patch. Below is our thinking process to explain how we got here.
> > 
> > 1. (abandoned) We add __clk_is_enabled() check in svs_suspend() to prevent
> > svs
> > clk from being turned off twice when svs_resume() turned off svs clk in the
> > error-handling process. Nonetheless, we met the NG case in the commit
> > message.
> > 2. (current patch) We add svs clk control hint to understand if we need to
> > run
> > svs_suspend() or not if svs_resume() turned off svs clk before.
> > 
> 
> Did you had a look on the dev_pm_ops？ Maybe we can use suspend_late, 
> resume_early to make sure there is no race condition. I wonder also if we
> can't 
> make sure that this does not happen using device links. Sorry, I can't give 
> better guidance on how to use this technologies, but I have the feeling we
> can 
> fix this with existing infrastructure.

No, we didn't look on dev_pm_ops and it seems like another way to fix this issue
with __clk_is_enabled() check in svs_suspend(). Thanks for the advice again.
we'll keep looking for any possible answers to this issue.

Sincerely,
Roger Lu.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel