PM / devfreq: tegra30: disable clock on error in probe

PATCH In review Dan Carpenter 3 participants 18 messages Sep 8, 2020 – Sep 23, 2020

Subsystems (4)

Kernel janitors Tegra architecture support Device frequency (devfreq) Memory frequency scaling drivers for NVIDIA Tegra

lore

Share & tools

lore

Dan Carpenter

author

2020-09-08 07:25 UTC #0 lore raw

PATCH 1/1 PM / devfreq: tegra30: disable clock on error in probe

This error path needs to call clk_disable_unprepare().

Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")

Signed-off-by Dan Carpenter <[email protected]>

---

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +3 -1

index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;

@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
 
 	return err;

Signature · 1 line

2.28.0

Dmitry Osipenko

maintainer Reviewed-by on v1 1/1

2020-09-08 13:02 UTC #1 lore raw

08.09.2020 10:25, Dan Carpenter пишет:

Quoted 25 lines

This error path needs to call clk_disable_unprepare().

Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")

Signed-off-by Dan Carpenter <[email protected]>

---

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +2 -1

index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 904

Quoted 7 lines

drivers/devfreq/tegra30-devfreq.c +1 -0

@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
 
 	return err;

Hello, Dan! Thank you for the patch!

Reviewed-by Dmitry Osipenko <[email protected]>

Chanwoo Choi

maintainer

2020-09-14 06:57 UTC #2 lore raw

In reply to Dan Carpenter

Hi,

Quoted 24 lines · Dan Carpenter

This error path needs to call clk_disable_unprepare().

Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")

Signed-off-by Dan Carpenter <[email protected]>

---

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +2 -1

index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902

Quoted 5 lines

drivers/devfreq/tegra30-devfreq.c +1 -0

@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);

Is it doesn't need to reset with reset_contrl_reset()?

Quoted 3 lines

drivers/devfreq/tegra30-devfreq.c +0 -0
 	return err;
 

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

maintainer

2020-09-14 13:56 UTC #3 lore raw

14.09.2020 10:09, Chanwoo Choi пишет:

Quoted 29 lines

Hi,

Quoted 26 lines · Dan Carpenter

This error path needs to call clk_disable_unprepare().

Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")

Signed-off-by Dan Carpenter <[email protected]>

---

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +2 -1

index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;

@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);

Quoted 5 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?

Hello, Chanwoo!

It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.

Dan Carpenter

author

2020-09-14 14:17 UTC #4 lore raw

Quoted 36 lines · Dmitry Osipenko

14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 29 lines
Hi,
Quoted 26 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?

Quoted 4 lines

Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.

Ah... I was looking the wrong code. Plus I don't really know this code very well.

If clk_prepare_enable() fails, then I would have assumed we need to call reset_control_deassert(). I would have assumed the reset_control_assert() and _deassert() functions were paired. So what I'm suggesting is something like the following: (I'll resend this if it's correct).

[PATCH] PM / devfreq: tegra30: Add missing reset_control_deassert()

If clk_prepare_enable() fails then probe needs to call the reset_control_deassert() function.

Fixes 6234f38016ad ("PM / devfreq: tegra: add devfreq driver for Tegra Activity Monitor")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/devfreq/tegra30-devfreq.c | 1 +
 1 file changed, 1 insertions(+), 0 deletion(-)

drivers/devfreq/tegra30-devfreq.c +1 -0

index e94a27804c20..ce217aba7b9d 100644
@@ -828,6 +828,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	if (err) {
 		dev_err(&pdev->dev,
 			"Failed to prepare and enable ACTMON clock\n");
+		reset_control_deassert(tegra->reset);
 		return err;
 	}

Dmitry Osipenko

maintainer

2020-09-14 14:28 UTC #5 lore raw

14.09.2020 17:17, Dan Carpenter пишет:
...

Quoted 7 lines

Quoted 6 lines
Quoted 1 line
Is it doesn't need to reset with reset_contrl_reset()?
Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.

Quoted 8 lines

Ah... I was looking the wrong code. Plus I don't really know this code very well.
If clk_prepare_enable() fails, then I would have assumed we need to call reset_control_deassert(). I would have assumed the reset_control_assert() and _deassert() functions were paired. So what I'm suggesting is something like the following: (I'll resend this if it's correct).

The reset shouldn't be deasserted if clk-enable fails.

Reset deassertion should be done only with enabled clock because reset happens synchronously with a clock tick, otherwise it makes no much sense to deassert the reset.

Yours current v1 variant is already good to me.

Chanwoo Choi

maintainer

2020-09-15 01:48 UTC #6 lore raw

In reply to Dmitry Osipenko

Hi Dmitry,

Quoted 36 lines · Dmitry Osipenko

14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 29 lines
Hi,
Quoted 26 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?

Quoted 4 lines

Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.

Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?

If tree, I think that 'reset_control_reset(tegra->reset)' invocation is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.

Actually, it is not clear in my case.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Chanwoo Choi

maintainer

2020-09-15 02:01 UTC #7 lore raw

Quoted 41 lines · Chanwoo Choi

Hi Dmitry,
Quoted 38 lines · Dmitry Osipenko
14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 29 lines
Hi,
Quoted 26 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?
Hello, Chanwoo!

Quoted 7 lines

Quoted 2 lines
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.
Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?
If tree, I think that 'reset_control_reset(tegra->reset)' invocation

I'm sorry for my typo. s/tree/true.

Quoted 6 lines

is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.
Actually, it is not clear in my case.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

maintainer

2020-09-15 17:01 UTC #8 lore raw

15.09.2020 05:13, Chanwoo Choi пишет:

Quoted 40 lines · Chanwoo Choi

Quoted 39 lines
Hi Dmitry,
Quoted 36 lines · Dmitry Osipenko
14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 34 lines
Hi,
Quoted 24 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.
Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?

I only mean that the tegra30-devfreq driver deasserts the reset before the clk_round_rate():

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n834

Quoted 9 lines

Quoted 1 line
If tree, I think that 'reset_control_reset(tegra->reset)' invocation
I'm sorry for my typo. s/tree/true.
Quoted 5 lines
is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.
Actually, it is not clear in my case.

The reset_control_reset() in the error path of the driver probe function is placed that way to make the tear-down order match the driver removal order. Perhaps the reset could be moved before the remove_opp, but this change won't make any real difference, hence it already should be good as-is.

Chanwoo Choi

maintainer

2020-09-16 02:25 UTC #9 lore raw

Quoted 62 lines · Dmitry Osipenko

15.09.2020 05:13, Chanwoo Choi пишет:
Quoted 40 lines · Chanwoo Choi
Quoted 39 lines
Hi Dmitry,
Quoted 36 lines · Dmitry Osipenko
14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 34 lines
Hi,
Quoted 24 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.
Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?
I only mean that the tegra30-devfreq driver deasserts the reset before the clk_round_rate():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n834
Quoted 8 lines
Quoted 1 line
If tree, I think that 'reset_control_reset(tegra->reset)' invocation
I'm sorry for my typo. s/tree/true.
Quoted 4 lines
is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.

Quoted 9 lines

Quoted 1 line
Quoted 1 line
Actually, it is not clear in my case.
The reset_control_reset() in the error path of the driver probe function is placed that way to make the tear-down order match the driver removal order. Perhaps the reset could be moved before the remove_opp, but this change won't make any real difference, hence it already should be good as-is.

I have one more question.
When failed to enable clock on line829[1],
does it need any reset_control invocation?
In this case on line829, just return without any restoration about reset control.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n829

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

maintainer

2020-09-16 19:07 UTC #10 lore raw

16.09.2020 05:38, Chanwoo Choi пишет:

Quoted 73 lines · Dmitry Osipenko

Quoted 62 lines
15.09.2020 05:13, Chanwoo Choi пишет:
Quoted 40 lines · Chanwoo Choi
Quoted 39 lines
Hi Dmitry,
Quoted 36 lines · Dmitry Osipenko
14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 34 lines
Hi,
Quoted 24 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.
Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?
I only mean that the tegra30-devfreq driver deasserts the reset before the clk_round_rate():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n834
Quoted 8 lines
Quoted 1 line
If tree, I think that 'reset_control_reset(tegra->reset)' invocation
I'm sorry for my typo. s/tree/true.
Quoted 4 lines
is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Actually, it is not clear in my case.
The reset_control_reset() in the error path of the driver probe function is placed that way to make the tear-down order match the driver removal order. Perhaps the reset could be moved before the remove_opp, but this change won't make any real difference, hence it already should be good as-is.

Quoted 8 lines

I have one more question.
When failed to enable clock on line829[1],
does it need any reset_control invocation?
In this case on line829, just return without any restoration about reset control.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n829

There is no need to deassert the reset if clk-enable fails because reset control of tegra30-devfreq is exclusive, i.e it isn't shared with any other peripherals, and thus, reset control could asserted/deasserted at any time by the devfreq driver. If clk-enable fails, then reset will stay asserted and it will be fine to re-assert it again.

Chanwoo Choi

maintainer

2020-09-17 02:27 UTC #11 lore raw

Quoted 80 lines · Dmitry Osipenko

16.09.2020 05:38, Chanwoo Choi пишет:
Quoted 79 lines · Dmitry Osipenko
Quoted 62 lines
15.09.2020 05:13, Chanwoo Choi пишет:
Quoted 40 lines · Chanwoo Choi
Quoted 39 lines
Hi Dmitry,
Quoted 36 lines · Dmitry Osipenko
14.09.2020 10:09, Chanwoo Choi пишет:
Quoted 34 lines
Hi,
Quoted 24 lines · Dan Carpenter
This error path needs to call clk_disable_unprepare().
Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
Signed-off-by Dan Carpenter <[email protected]>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/devfreq/tegra30-devfreq.c +2 -1
index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);
 
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Hello, Chanwoo!
It's reset just before the clk_round_rate() invocation, hence there shouldn't be a need to reset it second time.
Do you mean that reset is deasserted automatically when invoke clk_round_rate() on tegra?
I only mean that the tegra30-devfreq driver deasserts the reset before the clk_round_rate():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n834
Quoted 8 lines
Quoted 1 line
If tree, I think that 'reset_control_reset(tegra->reset)' invocation
I'm sorry for my typo. s/tree/true.
Quoted 4 lines
is not needed on 'remove_opp:' goto. Because already reset deassertion is invoked by clk_round_rate(), it seems that doesn't need to invoke anymore during exception case.
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Actually, it is not clear in my case.
The reset_control_reset() in the error path of the driver probe function is placed that way to make the tear-down order match the driver removal order. Perhaps the reset could be moved before the remove_opp, but this change won't make any real difference, hence it already should be good as-is.
I have one more question.
When failed to enable clock on line829[1],
does it need any reset_control invocation?
In this case on line829, just return without any restoration about reset control.

Quoted 9 lines

Quoted 2 lines
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/devfreq/tegra30-devfreq.c?h=v5.9-rc5#n829
There is no need to deassert the reset if clk-enable fails because reset control of tegra30-devfreq is exclusive, i.e it isn't shared with any other peripherals, and thus, reset control could asserted/deasserted at any time by the devfreq driver. If clk-enable fails, then reset will stay asserted and it will be fine to re-assert it again.

Thanks for the detailed explanation.
But, I think that almost people don't know the detailed h/w information. If possible, how about matching the pair when clk-enable fails as following?

drivers/devfreq/tegra30-devfreq.c +1 -0

@@ -828,6 +828,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
        if (err) {
                dev_err(&pdev->dev,
                        "Failed to prepare and enable ACTMON clock\n");
+               reset_control_deassert(tegra->reset);
                return err;
        }

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

maintainer

2020-09-17 21:14 UTC #12 lore raw

17.09.2020 05:32, Chanwoo Choi пишет:
...

Quoted 14 lines

Quoted 6 lines
There is no need to deassert the reset if clk-enable fails because reset control of tegra30-devfreq is exclusive, i.e it isn't shared with any other peripherals, and thus, reset control could asserted/deasserted at any time by the devfreq driver. If clk-enable fails, then reset will stay asserted and it will be fine to re-assert it again.
Thanks for the detailed explanation.
But, I think that almost people don't know the detailed h/w information. If possible, how about matching the pair when clk-enable fails as following?

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 833

Quoted 6 lines

drivers/devfreq/tegra30-devfreq.c +1 -0

@@ -828,6 +828,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
        if (err) {
                dev_err(&pdev->dev,
                        "Failed to prepare and enable ACTMON clock\n");
+               reset_control_deassert(tegra->reset);
                return err;
        }

That change won't bring any real benefits and will confuse people who know the HW, so we shouldn't do it.

Since the interrupt is disabled by default at the probe time, we actually don't need to care in a what state ACTMON hardware is at the driver-probe time since even if HW is active, it won't cause any damage to the system since ACTMON only collects and processes stats.

I made some experiments and looks like at least on Tegra30 the ACTMON hardware block uses multiple clocks and the ACTMON-clk isn't strictly necessary for the resetting of the HW state, it's actually quite common for Tegra peripherals that a part of HW logic runs off some root clk.

Hence if we want to improve the code, I think we can make this change:

(quoted diff) +0 -0

b/drivers/devfreq/tegra30-devfreq.c
index ee274daa57ac..4e3ac23e6850 100644

drivers/devfreq/tegra30-devfreq.c +0 -0

@@ -891,8 +891,6 @@ static int tegra_devfreq_probe(struct

platform_device *pdev)
return err;
}

- reset_control_assert(tegra->reset);
-
err = clk_prepare_enable(tegra->clock); if (err) {
dev_err(&pdev->dev,
@@ -900,7 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return err;
}

- reset_control_deassert(tegra->reset);
+ reset_control_reset(tegra->reset);

for (i = 0; i < mc->num_timings; i++) {
/*

Chanwoo Choi

maintainer

2020-09-18 09:10 UTC #13 lore raw

Quoted 62 lines · Dmitry Osipenko

17.09.2020 05:32, Chanwoo Choi пишет:
...
Quoted 14 lines
Quoted 6 lines
There is no need to deassert the reset if clk-enable fails because reset control of tegra30-devfreq is exclusive, i.e it isn't shared with any other peripherals, and thus, reset control could asserted/deasserted at any time by the devfreq driver. If clk-enable fails, then reset will stay asserted and it will be fine to re-assert it again.
Thanks for the detailed explanation.
But, I think that almost people don't know the detailed h/w information. If possible, how about matching the pair when clk-enable fails as following?
drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 833
Quoted 6 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
@@ -828,6 +828,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
        if (err) {
                dev_err(&pdev->dev,
                        "Failed to prepare and enable ACTMON clock\n");
+               reset_control_deassert(tegra->reset);
                return err;
        }
That change won't bring any real benefits and will confuse people who know the HW, so we shouldn't do it.
Since the interrupt is disabled by default at the probe time, we actually don't need to care in a what state ACTMON hardware is at the driver-probe time since even if HW is active, it won't cause any damage to the system since ACTMON only collects and processes stats.
I made some experiments and looks like at least on Tegra30 the ACTMON hardware block uses multiple clocks and the ACTMON-clk isn't strictly necessary for the resetting of the HW state, it's actually quite common for Tegra peripherals that a part of HW logic runs off some root clk.
Hence if we want to improve the code, I think we can make this change:
drivers/devfreq/tegra30-devfreq.c +0 -2
index ee274daa57ac..4e3ac23e6850 100644
@@ -891,8 +891,6 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_assert(tegra->reset);
-
 	err = clk_prepare_enable(tegra->clock);
 	if (err) {
drivers/devfreq/tegra30-devfreq.c +1 -1
 		dev_err(&pdev->dev,
@@ -900,7 +898,7 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_deassert(tegra->reset);
+	reset_control_reset(tegra->reset);
 
 	for (i = 0; i < mc->num_timings; i++) {

Quoted 1 line

/*

It looks good to me for improving the readability for everyone who don't know the detailed h/w information.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dmitry Osipenko

maintainer

2020-09-20 21:37 UTC #14 lore raw

18.09.2020 12:23, Chanwoo Choi пишет:
...

Quoted 22 lines

Hence if we want to improve the code, I think we can make this change:

drivers/devfreq/tegra30-devfreq.c +0 -2

index ee274daa57ac..4e3ac23e6850 100644
@@ -891,8 +891,6 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_assert(tegra->reset);
-
 	err = clk_prepare_enable(tegra->clock);
 	if (err) {

drivers/devfreq/tegra30-devfreq.c +0 -0

 		dev_err(&pdev->dev,

@@ -900,7 +898,7 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}

Quoted 8 lines

Quoted 5 lines
drivers/devfreq/tegra30-devfreq.c +1 -1
-	reset_control_deassert(tegra->reset);
+	reset_control_reset(tegra->reset);
 
 	for (i = 0; i < mc->num_timings; i++) {
/*
It looks good to me for improving the readability for everyone who don't know the detailed h/w information.

Okay, I'll make a patch sometime soon.

Dmitry Osipenko

maintainer

2020-09-23 00:23 UTC #15 lore raw

21.09.2020 00:37, Dmitry Osipenko пишет:

Quoted 27 lines

18.09.2020 12:23, Chanwoo Choi пишет:
...

Quoted 25 lines

Hence if we want to improve the code, I think we can make this change:

drivers/devfreq/tegra30-devfreq.c +0 -2

index ee274daa57ac..4e3ac23e6850 100644
@@ -891,8 +891,6 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_assert(tegra->reset);
-
 	err = clk_prepare_enable(tegra->clock);
 	if (err) {

drivers/devfreq/tegra30-devfreq.c +1 -1

 		dev_err(&pdev->dev,

@@ -900,7 +898,7 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_deassert(tegra->reset);
+	reset_control_reset(tegra->reset);

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
drivers/devfreq/tegra30-devfreq.c +0 -0
 	for (i = 0; i < mc->num_timings; i++) {
/*
It looks good to me for improving the readability for everyone who don't know the detailed h/w information.
Okay, I'll make a patch sometime soon.

Chanwoo, I want to clarify that the Dan's v1 patch is still good to me and should be applied. I'll improve the readability on top of the Dan's change.

Chanwoo Choi

maintainer

2020-09-23 00:29 UTC #16 lore raw

Quoted 34 lines · Dmitry Osipenko

21.09.2020 00:37, Dmitry Osipenko пишет:
Quoted 33 lines
18.09.2020 12:23, Chanwoo Choi пишет:
...
Quoted 30 lines
Quoted 26 lines
Hence if we want to improve the code, I think we can make this change:
drivers/devfreq/tegra30-devfreq.c +0 -2
index ee274daa57ac..4e3ac23e6850 100644
@@ -891,8 +891,6 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_assert(tegra->reset);
-
 	err = clk_prepare_enable(tegra->clock);
 	if (err) {
drivers/devfreq/tegra30-devfreq.c +1 -1
 		dev_err(&pdev->dev,
@@ -900,7 +898,7 @@ static int tegra_devfreq_probe(struct
 platform_device *pdev)
 		return err;
 	}
 
-	reset_control_deassert(tegra->reset);
+	reset_control_reset(tegra->reset);
 
 	for (i = 0; i < mc->num_timings; i++) {
Quoted 1 line
/*
It looks good to me for improving the readability for everyone who don't know the detailed h/w information.

Quoted 7 lines

Quoted 1 line
Okay, I'll make a patch sometime soon.
Chanwoo, I want to clarify that the Dan's v1 patch is still good to me and should be applied. I'll improve the readability on top of the Dan's change.

OK. Applied it. Thanks.

Signature · 3 lines

Best Regards,
Chanwoo Choi
Samsung Electronics

Dan Carpenter

author

2020-09-14 13:57 UTC #17 lore raw

In reply to Chanwoo Choi

Quoted 29 lines · Chanwoo Choi

Hi,

Quoted 26 lines · Dan Carpenter

This error path needs to call clk_disable_unprepare().

Fixes 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")

Signed-off-by Dan Carpenter <[email protected]>

---

---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/devfreq/tegra30-devfreq.c +2 -1

index e94a27804c20..dedd39de7367 100644
@@ -836,7 +836,8 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
 	if (rate < 0) {
 		dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate);
-		return rate;
+		err = rate;
+		goto disable_clk;
 	}
 
 	tegra->max_freq = rate / KHZ;

@@ -897,6 +898,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
 	dev_pm_opp_remove_all_dynamic(&pdev->dev);

Quoted 5 lines

drivers/devfreq/tegra30-devfreq.c static int tegra_devfreq_probe(struct platform_device *pdev) · line 902
Quoted 3 lines
drivers/devfreq/tegra30-devfreq.c +1 -0
 	reset_control_reset(tegra->reset);
+disable_clk:
 	clk_disable_unprepare(tegra->clock);
Is it doesn't need to reset with reset_contrl_reset()?

Is that a rhetorical question? I don't know this code very well but it looks like you are right.

I'll send a v2.

regards,
dan carpenter