* [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-08 7:25 ` Dan Carpenter 0 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-08 7:25 UTC (permalink / raw) To: Dmitry Osipenko Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-08 7:25 ` Dan Carpenter 0 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-08 7:25 UTC (permalink / raw) To: Dmitry Osipenko Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-08 7:25 ` Dan Carpenter @ 2020-09-08 13:02 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-08 13:02 UTC (permalink / raw) To: Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-08 13:02 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-08 13:02 UTC (permalink / raw) To: Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-08 7:25 ` Dan Carpenter @ 2020-09-14 7:09 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-14 6:57 UTC (permalink / raw) To: Dan Carpenter, Dmitry Osipenko Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-14 7:09 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-14 7:09 UTC (permalink / raw) To: Dan Carpenter, Dmitry Osipenko Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-14 7:09 ` Chanwoo Choi @ 2020-09-14 13:56 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-14 13:56 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-14 13:56 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-14 13:56 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-14 13:56 ` Dmitry Osipenko @ 2020-09-14 14:17 ` Dan Carpenter -1 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-14 14:17 UTC (permalink / raw) To: Dmitry Osipenko Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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; } ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-14 14:17 ` Dan Carpenter 0 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-14 14:17 UTC (permalink / raw) To: Dmitry Osipenko Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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; } ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-14 14:17 ` Dan Carpenter @ 2020-09-14 14:28 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-14 14:28 UTC (permalink / raw) To: Dan Carpenter Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-14 14:28 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-14 14:28 UTC (permalink / raw) To: Dan Carpenter Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-14 13:56 ` Dmitry Osipenko @ 2020-09-15 2:00 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-15 1:48 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-15 2:00 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-15 2:00 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-15 2:00 ` Chanwoo Choi @ 2020-09-15 2:13 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-15 2:13 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-15 2:13 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-15 2:13 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-15 2:13 ` Chanwoo Choi @ 2020-09-15 17:01 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-15 17:01 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-15 17:01 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-15 17:01 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-15 17:01 ` Dmitry Osipenko @ 2020-09-16 2:38 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-16 2:38 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-16 2:38 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-16 2:38 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-16 2:38 ` Chanwoo Choi @ 2020-09-16 19:07 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-16 19:07 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-16 19:07 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-16 19:07 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-16 19:07 ` Dmitry Osipenko @ 2020-09-17 2:32 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-17 2:32 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-17 2:32 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-17 2:32 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-17 2:32 ` Chanwoo Choi @ 2020-09-17 21:14 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-17 21:14 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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++) { /* ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-17 21:14 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-17 21:14 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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++) { /* ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-17 21:14 ` Dmitry Osipenko @ 2020-09-18 9:23 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-18 9:23 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-18 9:23 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-18 9:23 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-18 9:23 ` Chanwoo Choi @ 2020-09-20 21:37 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-20 21:37 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-20 21:37 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-20 21:37 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-20 21:37 ` Dmitry Osipenko @ 2020-09-23 0:23 ` Dmitry Osipenko -1 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-23 0:23 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-23 0:23 ` Dmitry Osipenko 0 siblings, 0 replies; 36+ messages in thread From: Dmitry Osipenko @ 2020-09-23 0:23 UTC (permalink / raw) To: Chanwoo Choi, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-23 0:23 ` Dmitry Osipenko @ 2020-09-23 0:42 ` Chanwoo Choi -1 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-23 0:42 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-23 0:42 ` Chanwoo Choi 0 siblings, 0 replies; 36+ messages in thread From: Chanwoo Choi @ 2020-09-23 0:42 UTC (permalink / raw) To: Dmitry Osipenko, Dan Carpenter Cc: MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe 2020-09-14 7:09 ` Chanwoo Choi @ 2020-09-14 13:57 ` Dan Carpenter -1 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-14 13:57 UTC (permalink / raw) To: Chanwoo Choi Cc: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] PM / devfreq: tegra30: disable clock on error in probe @ 2020-09-14 13:57 ` Dan Carpenter 0 siblings, 0 replies; 36+ messages in thread From: Dan Carpenter @ 2020-09-14 13:57 UTC (permalink / raw) To: Chanwoo Choi Cc: Dmitry Osipenko, MyungJoo Ham, Kyungmin Park, Thierry Reding, Jonathan Hunter, linux-pm, linux-tegra, kernel-janitors 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2020-09-23 0:42 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200908072627epcas1p41f2c8c2730d42bd8935a40b0ab8122f7@epcas1p4.samsung.com> 2020-09-08 7:25 ` [PATCH] PM / devfreq: tegra30: disable clock on error in probe Dan Carpenter 2020-09-08 7:25 ` Dan Carpenter 2020-09-08 13:02 ` Dmitry Osipenko 2020-09-08 13:02 ` Dmitry Osipenko 2020-09-14 6:57 ` Chanwoo Choi 2020-09-14 7:09 ` Chanwoo Choi 2020-09-14 13:56 ` Dmitry Osipenko 2020-09-14 13:56 ` Dmitry Osipenko 2020-09-14 14:17 ` Dan Carpenter 2020-09-14 14:17 ` Dan Carpenter 2020-09-14 14:28 ` Dmitry Osipenko 2020-09-14 14:28 ` Dmitry Osipenko 2020-09-15 1:48 ` Chanwoo Choi 2020-09-15 2:00 ` Chanwoo Choi 2020-09-15 2:13 ` Chanwoo Choi 2020-09-15 2:13 ` Chanwoo Choi 2020-09-15 17:01 ` Dmitry Osipenko 2020-09-15 17:01 ` Dmitry Osipenko 2020-09-16 2:38 ` Chanwoo Choi 2020-09-16 2:38 ` Chanwoo Choi 2020-09-16 19:07 ` Dmitry Osipenko 2020-09-16 19:07 ` Dmitry Osipenko 2020-09-17 2:32 ` Chanwoo Choi 2020-09-17 2:32 ` Chanwoo Choi 2020-09-17 21:14 ` Dmitry Osipenko 2020-09-17 21:14 ` Dmitry Osipenko 2020-09-18 9:23 ` Chanwoo Choi 2020-09-18 9:23 ` Chanwoo Choi 2020-09-20 21:37 ` Dmitry Osipenko 2020-09-20 21:37 ` Dmitry Osipenko 2020-09-23 0:23 ` Dmitry Osipenko 2020-09-23 0:23 ` Dmitry Osipenko 2020-09-23 0:42 ` Chanwoo Choi 2020-09-23 0:42 ` Chanwoo Choi 2020-09-14 13:57 ` Dan Carpenter 2020-09-14 13:57 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.