Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe

[Dmitry Osipenko <digetx@gmail.com>]

17.09.2020 05:32, Chanwoo Choi пишет:
...
>> 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?
> 
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -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:

diff --git a/drivers/devfreq/tegra30-devfreq.c
b/drivers/devfreq/tegra30-devfreq.c
index ee274daa57ac..4e3ac23e6850 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -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++) {
 		/*