| Message ID | 20200908072557.GC294938@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Chanwoo Choi |
| Headers | show |
| Series | PM / devfreq: tegra30: disable clock on error in probe | expand |
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>
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; >
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.
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
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; }
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.
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.
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. >
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.
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
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.
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; }
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++) { /*
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.
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.
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.
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.
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;
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(-)