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

[Chanwoo Choi <cw00.choi@samsung.com>]

On 9/18/20 6:14 AM, Dmitry Osipenko wrote:
> 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++) {
>  		/*

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics