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

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

[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 <dan.carpenter@oracle.com>
---
---
 drivers/devfreq/tegra30-devfreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index e94a27804c20..dedd39de7367 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -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;
-- 
2.28.0

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

[Dmitry Osipenko]

08.09.2020 10:25, 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 <dan.carpenter@oracle.com>
> ---
> ---
>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index e94a27804c20..dedd39de7367 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -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;
> 

Hello, Dan! Thank you for the patch!

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

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

[Chanwoo Choi]

Hi,

On 9/8/20 4:25 PM, Dan Carpenter wrote:
> This error path needs to call clk_disable_unprepare().
> 
> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> ---
>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> index e94a27804c20..dedd39de7367 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -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);

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

>  
>  	return err;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Dmitry Osipenko]

14.09.2020 10:09, Chanwoo Choi пишет:
> Hi,
> 
> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>> This error path needs to call clk_disable_unprepare().
>>
>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>> ---
>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>> index e94a27804c20..dedd39de7367 100644
>> --- a/drivers/devfreq/tegra30-devfreq.c
>> +++ b/drivers/devfreq/tegra30-devfreq.c
>> @@ -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);
> 
> 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.

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

[Dan Carpenter]

On Mon, Sep 14, 2020 at 04:56:26PM +0300, Dmitry Osipenko wrote:
> 14.09.2020 10:09, Chanwoo Choi пишет:
> > Hi,
> > 
> > On 9/8/20 4:25 PM, Dan Carpenter wrote:
> >> This error path needs to call clk_disable_unprepare().
> >>
> >> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> ---
> >> ---
> >>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> >> index e94a27804c20..dedd39de7367 100644
> >> --- a/drivers/devfreq/tegra30-devfreq.c
> >> +++ b/drivers/devfreq/tegra30-devfreq.c
> >> @@ -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);
> > 
> > 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.

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 <dan.carpenter@oracle.com>
---
 drivers/devfreq/tegra30-devfreq.c | 1 +
 1 file changed, 1 insertions(+), 0 deletion(-)

diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index e94a27804c20..ce217aba7b9d 100644
--- 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;
 	}
 

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

[Dmitry Osipenko]

14.09.2020 17:17, Dan Carpenter пишет:
...
>>> 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.
> 
> 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.

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

[Chanwoo Choi]

Hi Dmitry,

On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
> 14.09.2020 10:09, Chanwoo Choi пишет:
>> Hi,
>>
>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>> This error path needs to call clk_disable_unprepare().
>>>
>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>> ---
>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>> index e94a27804c20..dedd39de7367 100644
>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>> @@ -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);
>>
>> 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.

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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

On 9/15/20 11:00 AM, Chanwoo Choi wrote:
> Hi Dmitry,
> 
> On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
>> 14.09.2020 10:09, Chanwoo Choi пишет:
>>> Hi,
>>>
>>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>>> This error path needs to call clk_disable_unprepare().
>>>>
>>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> ---
>>>> ---
>>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>>> index e94a27804c20..dedd39de7367 100644
>>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>>> @@ -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);
>>>
>>> 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.
> 
> 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.

> 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.
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Dmitry Osipenko]

15.09.2020 05:13, Chanwoo Choi пишет:
> On 9/15/20 11:00 AM, Chanwoo Choi wrote:
>> Hi Dmitry,
>>
>> On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
>>> 14.09.2020 10:09, Chanwoo Choi пишет:
>>>> Hi,
>>>>
>>>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>>>> This error path needs to call clk_disable_unprepare().
>>>>>
>>>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>> ---
>>>>> ---
>>>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>>>> index e94a27804c20..dedd39de7367 100644
>>>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>>>> @@ -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);
>>>>
>>>> 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.
>>
>> 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

>> If tree, I think that 'reset_control_reset(tegra->reset)' invocation
> 
> I'm sorry for my typo. s/tree/true.
> 
>> 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.

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

[Chanwoo Choi]

On 9/16/20 2:01 AM, Dmitry Osipenko wrote:
> 15.09.2020 05:13, Chanwoo Choi пишет:
>> On 9/15/20 11:00 AM, Chanwoo Choi wrote:
>>> Hi Dmitry,
>>>
>>> On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
>>>> 14.09.2020 10:09, Chanwoo Choi пишет:
>>>>> Hi,
>>>>>
>>>>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>>>>> This error path needs to call clk_disable_unprepare().
>>>>>>
>>>>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>> ---
>>>>>> ---
>>>>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>>>>> index e94a27804c20..dedd39de7367 100644
>>>>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>>>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>>>>> @@ -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);
>>>>>
>>>>> 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.
>>>
>>> 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
> 
>>> If tree, I think that 'reset_control_reset(tegra->reset)' invocation
>>
>> I'm sorry for my typo. s/tree/true.
>>
>>> 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.
> 
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Dmitry Osipenko]

16.09.2020 05:38, Chanwoo Choi пишет:
> On 9/16/20 2:01 AM, Dmitry Osipenko wrote:
>> 15.09.2020 05:13, Chanwoo Choi пишет:
>>> On 9/15/20 11:00 AM, Chanwoo Choi wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
>>>>> 14.09.2020 10:09, Chanwoo Choi пишет:
>>>>>> Hi,
>>>>>>
>>>>>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>>>>>> This error path needs to call clk_disable_unprepare().
>>>>>>>
>>>>>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>> ---
>>>>>>> ---
>>>>>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>>>>>> index e94a27804c20..dedd39de7367 100644
>>>>>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>>>>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>>>>>> @@ -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);
>>>>>>
>>>>>> 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.
>>>>
>>>> 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
>>
>>>> If tree, I think that 'reset_control_reset(tegra->reset)' invocation
>>>
>>> I'm sorry for my typo. s/tree/true.
>>>
>>>> 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.
>>
>>
> 
> 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.

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

[Chanwoo Choi]

On 9/17/20 4:07 AM, Dmitry Osipenko wrote:
> 16.09.2020 05:38, Chanwoo Choi пишет:
>> On 9/16/20 2:01 AM, Dmitry Osipenko wrote:
>>> 15.09.2020 05:13, Chanwoo Choi пишет:
>>>> On 9/15/20 11:00 AM, Chanwoo Choi wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> On 9/14/20 10:56 PM, Dmitry Osipenko wrote:
>>>>>> 14.09.2020 10:09, Chanwoo Choi пишет:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 9/8/20 4:25 PM, Dan Carpenter wrote:
>>>>>>>> This error path needs to call clk_disable_unprepare().
>>>>>>>>
>>>>>>>> Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
>>>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>>> ---
>>>>>>>> ---
>>>>>>>>  drivers/devfreq/tegra30-devfreq.c | 4 +++-
>>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
>>>>>>>> index e94a27804c20..dedd39de7367 100644
>>>>>>>> --- a/drivers/devfreq/tegra30-devfreq.c
>>>>>>>> +++ b/drivers/devfreq/tegra30-devfreq.c
>>>>>>>> @@ -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);
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> 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
>>>
>>>>> If tree, I think that 'reset_control_reset(tegra->reset)' invocation
>>>>
>>>> I'm sorry for my typo. s/tree/true.
>>>>
>>>>> 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.
>>>
>>>
>>
>> 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.
>

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;
        }


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Dmitry Osipenko]

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++) {
 		/*

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

[Chanwoo Choi]

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

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

[Dmitry Osipenko]

18.09.2020 12:23, Chanwoo Choi пишет:
...
>> 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.

Okay, I'll make a patch sometime soon.

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

[Dmitry Osipenko]

21.09.2020 00:37, Dmitry Osipenko пишет:
> 18.09.2020 12:23, Chanwoo Choi пишет:
> ...
>>> 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.
> 
> 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.

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

[Chanwoo Choi]

On 9/23/20 9:23 AM, Dmitry Osipenko wrote:
> 21.09.2020 00:37, Dmitry Osipenko пишет:
>> 18.09.2020 12:23, Chanwoo Choi пишет:
>> ...
>>>> 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.
>>
>> 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.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Dan Carpenter]

On Mon, Sep 14, 2020 at 04:09:02PM +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 9/8/20 4:25 PM, Dan Carpenter wrote:
> > This error path needs to call clk_disable_unprepare().
> > 
> > Fixes: 7296443b900e ("PM / devfreq: tegra30: Handle possible round-rate error")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > ---
> >  drivers/devfreq/tegra30-devfreq.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
> > index e94a27804c20..dedd39de7367 100644
> > --- a/drivers/devfreq/tegra30-devfreq.c
> > +++ b/drivers/devfreq/tegra30-devfreq.c
> > @@ -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);
> 
> 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