| Message ID | 1e88b248352afe03cd3bf0e887b1f2be86b5afb5.1653564321.git.viresh.kumar@linaro.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
On 5/26/22 14:42, Viresh Kumar wrote: > The OPP core already performs devm_pm_opp_set_clkname() with name as > NULL, the callers shouldn't be doing the same unless they have a > different clock name to add here. > > Drop the call. > > Cc: Dmitry Osipenko <digetx@gmail.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/soc/tegra/common.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index 32c346b72635..49a5360f4507 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -108,12 +108,6 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > u32 hw_version; > int err; > > - err = devm_pm_opp_set_clkname(dev, NULL); > - if (err) { > - dev_err(dev, "failed to set OPP clk: %d\n", err); > - return err; > - } > - > /* Tegra114+ doesn't support OPP yet */ > if (!of_machine_is_compatible("nvidia,tegra20") && > !of_machine_is_compatible("nvidia,tegra30")) I can't see where OPP core performs devm_pm_opp_set_clkname().
On 26-05-22, 20:57, Dmitry Osipenko wrote: > On 5/26/22 14:42, Viresh Kumar wrote: > > The OPP core already performs devm_pm_opp_set_clkname() with name as > > NULL, the callers shouldn't be doing the same unless they have a > > different clock name to add here. This is confusing. Updated this as: The OPP core already performs clk_get(dev, NULL) by default for everyone and the callers shouldn't try to set clkname unless they have an actual clock name to add here. > > > > Drop the call. > > > > Cc: Dmitry Osipenko <digetx@gmail.com> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/soc/tegra/common.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > > index 32c346b72635..49a5360f4507 100644 > > --- a/drivers/soc/tegra/common.c > > +++ b/drivers/soc/tegra/common.c > > @@ -108,12 +108,6 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > > u32 hw_version; > > int err; > > > > - err = devm_pm_opp_set_clkname(dev, NULL); > > - if (err) { > > - dev_err(dev, "failed to set OPP clk: %d\n", err); > > - return err; > > - } > > - > > /* Tegra114+ doesn't support OPP yet */ > > if (!of_machine_is_compatible("nvidia,tegra20") && > > !of_machine_is_compatible("nvidia,tegra30")) > > I can't see where OPP core performs devm_pm_opp_set_clkname(). Sorry about that, it is clk_get() it performs from _update_opp_table_clk(). I don't think you need to call devm_pm_opp_set_clkname() here, but lets see if this breaks anything for you.
Hi Viresh, On 26/05/2022 12:42, Viresh Kumar wrote: > The OPP core already performs devm_pm_opp_set_clkname() with name as > NULL, the callers shouldn't be doing the same unless they have a > different clock name to add here. > > Drop the call. > > Cc: Dmitry Osipenko <digetx@gmail.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/soc/tegra/common.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index 32c346b72635..49a5360f4507 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -108,12 +108,6 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > u32 hw_version; > int err; > > - err = devm_pm_opp_set_clkname(dev, NULL); > - if (err) { > - dev_err(dev, "failed to set OPP clk: %d\n", err); > - return err; > - } > - > /* Tegra114+ doesn't support OPP yet */ > if (!of_machine_is_compatible("nvidia,tegra20") && > !of_machine_is_compatible("nvidia,tegra30")) This appears to be breaking a few Tegra drivers. For example, on Tegra210 Jetson TX1 I am seeing the following and the eMMC is no longer working ... [ 0.526729] sdhci-tegra 700b0600.mmc: dev_pm_opp_set_rate: device's opp table doesn't exist [ 0.526733] sdhci-tegra 700b0600.mmc: failed to set clk rate to 400000Hz: -19 [ 0.528830] sdhci-tegra 700b0600.mmc: dev_pm_opp_set_rate: device's opp table doesn't exist [ 0.528833] sdhci-tegra 700b0600.mmc: failed to set clk rate to 400000Hz: -19 I have seen another instance of this on Jetson Xavier NX ... [ 12.301336] tegra-pwm 32d0000.pwm: dev_pm_opp_set_rate: device's opp table doesn't exist [ 12.301350] tegra-pwm 32d0000.pwm: Failed to set max frequency: -19 Bisect is point to this commit and so something is not working as expected. Cheers Jon
On 23-06-22, 22:15, Jon Hunter wrote: > On 26/05/2022 12:42, Viresh Kumar wrote: > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > > index 32c346b72635..49a5360f4507 100644 > > --- a/drivers/soc/tegra/common.c > > +++ b/drivers/soc/tegra/common.c > > @@ -108,12 +108,6 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > > u32 hw_version; > > int err; > > - err = devm_pm_opp_set_clkname(dev, NULL); > > - if (err) { > > - dev_err(dev, "failed to set OPP clk: %d\n", err); > > - return err; > > - } > > - > > /* Tegra114+ doesn't support OPP yet */ > > if (!of_machine_is_compatible("nvidia,tegra20") && > > !of_machine_is_compatible("nvidia,tegra30")) > > > This appears to be breaking a few Tegra drivers. For example, on Tegra210 > Jetson TX1 I am seeing the following and the eMMC is no longer working ... > > [ 0.526729] sdhci-tegra 700b0600.mmc: dev_pm_opp_set_rate: device's opp table doesn't exist > [ 0.526733] sdhci-tegra 700b0600.mmc: failed to set clk rate to 400000Hz: -19 > [ 0.528830] sdhci-tegra 700b0600.mmc: dev_pm_opp_set_rate: device's opp table doesn't exist > [ 0.528833] sdhci-tegra 700b0600.mmc: failed to set clk rate to 400000Hz: -19 > > I have seen another instance of this on Jetson Xavier NX ... > > [ 12.301336] tegra-pwm 32d0000.pwm: dev_pm_opp_set_rate: device's opp table doesn't exist > [ 12.301350] tegra-pwm 32d0000.pwm: Failed to set max frequency: -19 > > Bisect is point to this commit and so something is not working as > expected. Thanks again Jon. This is what happens when the special code doesn't have a comment attached with it. Neither the reviewer, nor the author remember why the special piece was required :) I had to go through the whole sequence, along with DT to understand what might have broken this stuff :) I will drop this patch and add this comment in its place: diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index 32c346b72635..9f3fdeb1a11c 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -108,6 +108,13 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, u32 hw_version; int err; + /* + * For some devices we don't have any OPP table in the DT, and in order + * to use the same code path for all the devices, we create a dummy OPP + * table for them via this call. The dummy OPP table is only capable of + * doing clk_set_rate() on invocation of dev_pm_opp_set_rate() and + * doesn't provide any other functionality. + */ err = devm_pm_opp_set_clkname(dev, NULL); if (err) { dev_err(dev, "failed to set OPP clk: %d\n", err); Though there will still be a problem here with my changes, we don't accept NULL clkname anymore for the set-clkname API. And tegra does this to pick the first clock available in DT (at index 0) I think. Other drivers (mostly qcom) who need such dummy OPP table, provide a real clock name instead. Will it be possible to pass that here somehow ?
On 24-06-22, 05:58, Viresh Kumar wrote: > Though there will still be a problem here with my changes, we don't > accept NULL clkname anymore for the set-clkname API. And tegra does > this to pick the first clock available in DT (at index 0) I think. > Other drivers (mostly qcom) who need such dummy OPP table, provide a > real clock name instead. Will it be possible to pass that here somehow > ? Jon, Okay, I was able to handle it without making any further updates to the OPP core. Nothing else required from your side on this. I have pushed the updates (to this patch and 22/31) to opp/linux-next branch. You can try it now and it should just work. I have only build tested it though. Thanks.
On 24/06/2022 01:59, Viresh Kumar wrote: > On 24-06-22, 05:58, Viresh Kumar wrote: >> Though there will still be a problem here with my changes, we don't >> accept NULL clkname anymore for the set-clkname API. And tegra does >> this to pick the first clock available in DT (at index 0) I think. >> Other drivers (mostly qcom) who need such dummy OPP table, provide a >> real clock name instead. Will it be possible to pass that here somehow >> ? > > Jon, > > Okay, I was able to handle it without making any further updates to > the OPP core. Nothing else required from your side on this. > > I have pushed the updates (to this patch and 22/31) to opp/linux-next > branch. You can try it now and it should just work. I have only build > tested it though. Thanks. I have reverted the previous versions of 21 and 22, and pulled in the latest and these are working. Cheers Jon
diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index 32c346b72635..49a5360f4507 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -108,12 +108,6 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, u32 hw_version; int err; - err = devm_pm_opp_set_clkname(dev, NULL); - if (err) { - dev_err(dev, "failed to set OPP clk: %d\n", err); - return err; - } - /* Tegra114+ doesn't support OPP yet */ if (!of_machine_is_compatible("nvidia,tegra20") && !of_machine_is_compatible("nvidia,tegra30"))
The OPP core already performs devm_pm_opp_set_clkname() with name as NULL, the callers shouldn't be doing the same unless they have a different clock name to add here. Drop the call. Cc: Dmitry Osipenko <digetx@gmail.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/soc/tegra/common.c | 6 ------ 1 file changed, 6 deletions(-)