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
