| Message ID | 449b344f037c7ef1970bc84d31e0d4c4cb4d2951.1653564321.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
On 26-05-22, 17:12, Viresh Kumar wrote: > The OPP core now provides a unified API for setting all configuration > types, i.e. dev_pm_opp_set_config(). > > Lets start using it. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/soc/tegra/common.c | 8 ++++++-- > drivers/soc/tegra/pmc.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index 49a5360f4507..7ba15cb836e8 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -107,6 +107,10 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > { > u32 hw_version; > int err; > + struct dev_pm_opp_config config = { > + .supported_hw = &hw_version, > + .supported_hw_count = 1, > + }; > > /* Tegra114+ doesn't support OPP yet */ > if (!of_machine_is_compatible("nvidia,tegra20") && > @@ -118,9 +122,9 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > else > hw_version = BIT(tegra_sku_info.soc_speedo_id); > > - err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1); > + err = devm_pm_opp_set_config(dev, &config); > if (err) { > - dev_err(dev, "failed to set OPP supported HW: %d\n", err); > + dev_err(dev, "failed to set OPP config: %d\n", err); > return err; > } Jon/Dmitry, Because of the update [1] to previous patch 21/31, I am updating this file as (fresh diff): diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index 9f3fdeb1a11c..cd53e46c4058 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -107,36 +107,42 @@ 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); - return err; - } - - /* Tegra114+ doesn't support OPP yet */ - if (!of_machine_is_compatible("nvidia,tegra20") && - !of_machine_is_compatible("nvidia,tegra30")) - return -ENODEV; - - if (of_machine_is_compatible("nvidia,tegra20")) + struct dev_pm_opp_config config = { + /* + * 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. 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. + */ + .clk_names = NULL, + .clk_count = 1, + }; + + if (of_machine_is_compatible("nvidia,tegra20")) { hw_version = BIT(tegra_sku_info.soc_process_id); - else + config.supported_hw = &hw_version; + config.supported_hw_count = 1; + } else if (of_machine_is_compatible("nvidia,tegra30")) { hw_version = BIT(tegra_sku_info.soc_speedo_id); + config.supported_hw = &hw_version; + config.supported_hw_count = 1; + } - err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1); + err = devm_pm_opp_set_config(dev, &config); if (err) { - dev_err(dev, "failed to set OPP supported HW: %d\n", err); + dev_err(dev, "failed to set OPP config: %d\n", err); return err; } + /* + * Tegra114+ doesn't support OPP yet, return early for non tegra20/30 + * case. + */ + if (!config.supported_hw) + return -ENODEV; + /* * Older device-trees have an empty OPP table, we will get * -ENODEV from devm_pm_opp_of_add_table() in this case. -------------------------8<------------------------- The idea here is to always set the clk name (to NULL) and skip other stuff for SoCs other than tegra 20/30. Just see if you can find something odd with the review of it, I will resend it properly later once the issues are settled.
On 24-06-22, 06:18, Viresh Kumar wrote: > + struct dev_pm_opp_config config = { > + /* > + * 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. 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. > + */ > + .clk_names = NULL, > + .clk_count = 1, > + }; Slight modification here, sorry about that. We just need to set the name as NULL and not the array itself. diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index cd53e46c4058..6a099d764cce 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -116,7 +116,7 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, * of dev_pm_opp_set_rate() and doesn't provide any other * functionality. */ - .clk_names = NULL, + .clk_names = (const char *[]){ NULL }, .clk_count = 1, };
24.06.2022 03:57, Viresh Kumar пишет: > On 24-06-22, 06:18, Viresh Kumar wrote: >> + struct dev_pm_opp_config config = { >> + /* >> + * 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. 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. >> + */ >> + .clk_names = NULL, >> + .clk_count = 1, >> + }; > > Slight modification here, sorry about that. We just need to set the > name as NULL and not the array itself. > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index cd53e46c4058..6a099d764cce 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -116,7 +116,7 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, > * of dev_pm_opp_set_rate() and doesn't provide any other > * functionality. > */ > - .clk_names = NULL, > + .clk_names = (const char *[]){ NULL }, > .clk_count = 1, > }; > Looks okay. If you'll solve the cpufreq problem where OPP config is set by two drivers for the same cpu device and will keep the set_opp() helper that is needed by the Tegra 3d driver, then it all should work for Tegra. Looking forward to the next update of the OPP patches, thank you.
On 27-06-22, 01:14, Dmitry Osipenko wrote: > Looks okay. If you'll solve the cpufreq problem where OPP config is set > by two drivers for the same cpu device This is supported, there is some early freeing of resources on the removal path though, the reasoning for which I already gave in another email. Though, I am open to sorting that out as well, but nothing breaks the code for now AFAICT. > and will keep the set_opp() > helper that is needed by the Tegra 3d driver, then it all should work > for Tegra. I have responded to that as well on another thread. > Looking forward to the next update of the OPP patches, thank you. All that I have is already pushed to linux-next, I don't have any more changes. Yes I still need to send the updated changes to list.
27.06.2022 09:45, Viresh Kumar пишет: >> Looks okay. If you'll solve the cpufreq problem where OPP config is set >> by two drivers for the same cpu device > This is supported, there is some early freeing of resources on the > removal path though, the reasoning for which I already gave in another > email. Though, I am open to sorting that out as well, but nothing > breaks the code for now AFAICT. > In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the supported_hw will be lost on the re-probe. I haven't checked yet, but I suppose that cpufreq-dt driver defers on Tegra30 because of the CPU regulator and that's why we get the "OPP table is missing" error.
On 27-06-22, 10:14, Dmitry Osipenko wrote: > 27.06.2022 09:45, Viresh Kumar пишет: > >> Looks okay. If you'll solve the cpufreq problem where OPP config is set > >> by two drivers for the same cpu device > > This is supported, there is some early freeing of resources on the > > removal path though, the reasoning for which I already gave in another > > email. Though, I am open to sorting that out as well, but nothing > > breaks the code for now AFAICT. > > > > In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and > registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the > supported_hw will be lost on the re-probe. I haven't checked yet, but I > suppose that cpufreq-dt driver defers on Tegra30 because of the CPU > regulator and that's why we get the "OPP table is missing" error. Aha, I get it now. I see, this is a real problem. Will fix it. Give me some time to think. Thanks.
On 28-06-22, 13:08, Dmitry Osipenko wrote: > The opp/linux-next works fine, thank you. > > Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Thanks. I should add this to all the core + tegra -patches in that branch, right ? > BTW, the idr_alloc() is obsoleted by xa_alloc(). The earlier interface isn't deprecated, right ? I really don't want to go change it again :)
On 6/28/22 13:11, Viresh Kumar wrote: > On 28-06-22, 13:08, Dmitry Osipenko wrote: >> The opp/linux-next works fine, thank you. >> >> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > Thanks. I should add this to all the core + tegra -patches in that > branch, right ? Yes, please. >> BTW, the idr_alloc() is obsoleted by xa_alloc(). > > The earlier interface isn't deprecated, right ? I really don't want to > go change it again :) > It has been in a process of deprecation for a couple years now. All IDR instances are slowly converting to XA. You won't need to take mutex with xa_alloc().
On 28-06-22, 13:16, Dmitry Osipenko wrote: > It has been in a process of deprecation for a couple years now. All IDR > instances are slowly converting to XA. You won't need to take mutex with > xa_alloc(). Okay, migrated to it now and pushed :)
On 28/06/2022 11:08, Dmitry Osipenko wrote: > On 6/28/22 10:09, Viresh Kumar wrote: >> On 27-06-22, 12:51, Viresh Kumar wrote: >>> On 27-06-22, 10:14, Dmitry Osipenko wrote: >>>> 27.06.2022 09:45, Viresh Kumar пишет: >>>>>> Looks okay. If you'll solve the cpufreq problem where OPP config is set >>>>>> by two drivers for the same cpu device >>>>> This is supported, there is some early freeing of resources on the >>>>> removal path though, the reasoning for which I already gave in another >>>>> email. Though, I am open to sorting that out as well, but nothing >>>>> breaks the code for now AFAICT. >>>>> >>>> >>>> In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and >>>> registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the >>>> supported_hw will be lost on the re-probe. I haven't checked yet, but I >>>> suppose that cpufreq-dt driver defers on Tegra30 because of the CPU >>>> regulator and that's why we get the "OPP table is missing" error. >>> >>> Aha, I get it now. I see, this is a real problem. Will fix it. Give me >>> some time to think. Thanks. >> >> Okay, I fixed this in opp/linux-next, can you or Jon please give it a >> go on tegra30 to see if the issue is fixed ? >> >> FWIW, I have fixed this with the IDR API and the OPP core will only >> free the resources in clear-config, that the corresponding set-config >> has configured. I have tested it with the clk API only though. >> >> Once you confirm, I will resend all the patches and hope no issues are >> left here. >> >> Thanks for helping out guys. Really appreciate it. >> > > The opp/linux-next works fine, thank you. > > Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Today's -next is also working fine for me too! Thanks Jon
diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index 49a5360f4507..7ba15cb836e8 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -107,6 +107,10 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, { u32 hw_version; int err; + struct dev_pm_opp_config config = { + .supported_hw = &hw_version, + .supported_hw_count = 1, + }; /* Tegra114+ doesn't support OPP yet */ if (!of_machine_is_compatible("nvidia,tegra20") && @@ -118,9 +122,9 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev, else hw_version = BIT(tegra_sku_info.soc_speedo_id); - err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1); + err = devm_pm_opp_set_config(dev, &config); if (err) { - dev_err(dev, "failed to set OPP supported HW: %d\n", err); + dev_err(dev, "failed to set OPP config: %d\n", err); return err; } diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index fdf508e03400..01ec76dd433d 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -1366,6 +1366,10 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) struct generic_pm_domain *genpd; const char *rname = "core"; int err; + struct dev_pm_opp_config config = { + .regulator_names = &rname, + .regulator_count = 1, + }; genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL); if (!genpd) @@ -1375,10 +1379,10 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np) genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state; genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state; - err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1); + err = devm_pm_opp_set_config(pmc->dev, &config); if (err) return dev_err_probe(pmc->dev, err, - "failed to set core OPP regulator\n"); + "failed to set OPP config\n"); err = pm_genpd_init(genpd, NULL, false); if (err) {
The OPP core now provides a unified API for setting all configuration types, i.e. dev_pm_opp_set_config(). Lets start using it. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/soc/tegra/common.c | 8 ++++++-- drivers/soc/tegra/pmc.c | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-)