* [PATCH v1 0/1] Add devm_tegra_core_dev_init_opp_table() helper @ 2021-05-16 20:51 Dmitry Osipenko 2021-05-16 20:51 ` [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() Dmitry Osipenko 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Osipenko @ 2021-05-16 20:51 UTC (permalink / raw) To: Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd, Krzysztof Kozlowski Cc: linux-tegra, linux-kernel Hello, This patch adds a new devm_tegra_core_dev_init_opp_table() helper which is specific to NVIDIA Tegra SoCs. The goal of this helper is to remove code duplication from Tegra device drivers. Previously this helper was a part of patchsets that added core power domain support. Krzysztof Kozlowski suggested to separate patches into smaller sets with explicit dependencies. This helper is a mandatory prerequisite by a multiple patchsets that will add more advanced power management to GPU/media/clk and etc drivers, fixing overheating troubles of Tegra devices. It provides OPP support for newer device-trees and preserves compatibility with the older DTBs. Dmitry Osipenko (1): soc/tegra: Add devm_tegra_core_dev_init_opp_table() drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++ include/soc/tegra/common.h | 30 ++++++++++ 2 files changed, 142 insertions(+) -- 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-16 20:51 [PATCH v1 0/1] Add devm_tegra_core_dev_init_opp_table() helper Dmitry Osipenko @ 2021-05-16 20:51 ` Dmitry Osipenko 2021-05-17 3:37 ` Viresh Kumar 2021-05-17 11:43 ` Krzysztof Kozlowski 0 siblings, 2 replies; 9+ messages in thread From: Dmitry Osipenko @ 2021-05-16 20:51 UTC (permalink / raw) To: Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd, Krzysztof Kozlowski Cc: linux-tegra, linux-kernel Add common helper which initializes OPP table for Tegra SoC core devices. Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++ include/soc/tegra/common.h | 30 ++++++++++ 2 files changed, 142 insertions(+) diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c index 3dc54f59cafe..c3fd2facfc2d 100644 --- a/drivers/soc/tegra/common.c +++ b/drivers/soc/tegra/common.c @@ -3,9 +3,16 @@ * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved. */ +#define dev_fmt(fmt) "tegra-soc: " fmt + +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/export.h> #include <linux/of.h> +#include <linux/pm_opp.h> #include <soc/tegra/common.h> +#include <soc/tegra/fuse.h> static const struct of_device_id tegra_machine_match[] = { { .compatible = "nvidia,tegra20", }, @@ -31,3 +38,108 @@ bool soc_is_tegra(void) return match != NULL; } + +static int tegra_core_dev_init_opp_state(struct device *dev) +{ + struct dev_pm_opp *opp; + unsigned long rate; + struct clk *clk; + int err; + + clk = devm_clk_get(dev, NULL); + if (IS_ERR(clk)) { + dev_err(dev, "failed to get clk: %pe\n", clk); + return PTR_ERR(clk); + } + + rate = clk_get_rate(clk); + if (!rate) { + dev_err(dev, "failed to get clk rate\n"); + return -EINVAL; + } + + opp = dev_pm_opp_find_freq_ceil(dev, &rate); + + if (opp == ERR_PTR(-ERANGE)) + opp = dev_pm_opp_find_freq_floor(dev, &rate); + + err = PTR_ERR_OR_ZERO(opp); + if (err) { + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", + rate, err); + return err; + } + + dev_pm_opp_put(opp); + + /* first dummy rate-setting initializes voltage vote */ + err = dev_pm_opp_set_rate(dev, rate); + if (err) { + dev_err(dev, "failed to initialize OPP clock: %d\n", err); + return err; + } + + return 0; +} + +/** + * devm_tegra_core_dev_init_opp_table() - initialize OPP table + * @dev: device for which OPP table is initialized + * @params: pointer to the OPP table configuration + * + * This function will initialize OPP table and sync OPP state of a Tegra SoC + * core device. + * + * Return: 0 on success or errorno. + */ +int devm_tegra_core_dev_init_opp_table(struct device *dev, + struct tegra_core_opp_params *params) +{ + 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")) + return -ENODEV; + + if (of_machine_is_compatible("nvidia,tegra20")) + hw_version = BIT(tegra_sku_info.soc_process_id); + else + hw_version = BIT(tegra_sku_info.soc_speedo_id); + + err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1); + if (err) { + dev_err(dev, "failed to set OPP supported HW: %d\n", err); + return err; + } + + /* + * Older device-trees have an empty OPP table, we will get + * -ENODEV from devm_pm_opp_of_add_table() in this case. + */ + err = devm_pm_opp_of_add_table(dev); + if (err) { + if (err == -ENODEV) + dev_err_once(dev, "OPP table not found, please update device-tree\n"); + else + dev_err(dev, "failed to add OPP table: %d\n", err); + + return err; + } + + if (params->init_state) { + err = tegra_core_dev_init_opp_state(dev); + if (err) + return err; + } + + return 0; +} +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table); diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h index 98027a76ce3d..e8eab13aa199 100644 --- a/include/soc/tegra/common.h +++ b/include/soc/tegra/common.h @@ -6,6 +6,36 @@ #ifndef __SOC_TEGRA_COMMON_H__ #define __SOC_TEGRA_COMMON_H__ +#include <linux/errno.h> +#include <linux/types.h> + +struct device; + +/** + * Tegra SoC core device OPP table configuration + * + * @init_state: pre-initialize OPP state of a device + */ +struct tegra_core_opp_params { + bool init_state; +}; + +#ifdef CONFIG_ARCH_TEGRA bool soc_is_tegra(void); +int devm_tegra_core_dev_init_opp_table(struct device *dev, + struct tegra_core_opp_params *params); +#else +static inline bool soc_is_tegra(void) +{ + return false; +} + +static inline int +devm_tegra_core_dev_init_opp_table(struct device *dev, + struct tegra_core_opp_params *params) +{ + return -ENODEV; +} +#endif #endif /* __SOC_TEGRA_COMMON_H__ */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-16 20:51 ` [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() Dmitry Osipenko @ 2021-05-17 3:37 ` Viresh Kumar 2021-05-17 14:09 ` Dmitry Osipenko 2021-05-17 11:43 ` Krzysztof Kozlowski 1 sibling, 1 reply; 9+ messages in thread From: Viresh Kumar @ 2021-05-17 3:37 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd, Krzysztof Kozlowski, linux-tegra, linux-kernel I am not sure why you divided this into three different patchsets, while it should really have been one. Like this one just adds the API but doesn't use it. On 16-05-21, 23:51, Dmitry Osipenko wrote: > Add common helper which initializes OPP table for Tegra SoC core devices. > > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++ > include/soc/tegra/common.h | 30 ++++++++++ > 2 files changed, 142 insertions(+) > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index 3dc54f59cafe..c3fd2facfc2d 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -3,9 +3,16 @@ > * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved. > */ > > +#define dev_fmt(fmt) "tegra-soc: " fmt > + > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/export.h> > #include <linux/of.h> > +#include <linux/pm_opp.h> > > #include <soc/tegra/common.h> > +#include <soc/tegra/fuse.h> > > static const struct of_device_id tegra_machine_match[] = { > { .compatible = "nvidia,tegra20", }, > @@ -31,3 +38,108 @@ bool soc_is_tegra(void) > > return match != NULL; > } > + > +static int tegra_core_dev_init_opp_state(struct device *dev) > +{ > + struct dev_pm_opp *opp; > + unsigned long rate; > + struct clk *clk; > + int err; > + > + clk = devm_clk_get(dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(dev, "failed to get clk: %pe\n", clk); > + return PTR_ERR(clk); > + } > + > + rate = clk_get_rate(clk); > + if (!rate) { > + dev_err(dev, "failed to get clk rate\n"); > + return -EINVAL; > + } > + > + opp = dev_pm_opp_find_freq_ceil(dev, &rate); > + > + if (opp == ERR_PTR(-ERANGE)) > + opp = dev_pm_opp_find_freq_floor(dev, &rate); > + > + err = PTR_ERR_OR_ZERO(opp); > + if (err) { > + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", > + rate, err); > + return err; > + } Why do you need to do this floor/ceil thing? Why can't you simply do set-rate ? > + > + dev_pm_opp_put(opp); > + > + /* first dummy rate-setting initializes voltage vote */ > + err = dev_pm_opp_set_rate(dev, rate); > + if (err) { > + dev_err(dev, "failed to initialize OPP clock: %d\n", err); > + return err; > + } > + > + return 0; > +} -- viresh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-17 3:37 ` Viresh Kumar @ 2021-05-17 14:09 ` Dmitry Osipenko 2021-05-18 3:26 ` Viresh Kumar 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Osipenko @ 2021-05-17 14:09 UTC (permalink / raw) To: Viresh Kumar Cc: Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd, Krzysztof Kozlowski, linux-tegra, linux-kernel 17.05.2021 06:37, Viresh Kumar пишет: > I am not sure why you divided this into three different patchsets, > while it should really have been one. Like this one just adds the API > but doesn't use it. Previously Krzysztof Kozlowski asked to split the large series into smaller sets which could be reviewed and applied separately by maintainers. He suggested that the immutable branch is a better option, I decided to implement this suggestion. So far I only sent out the memory patches which make use of the new helper, there will be more patches. The memory patches are intended to show that this helper can be utilized right now. My plan was to finalize this patch first, then Thierry will apply it and I will be able to sent the rest of the patches telling that they depend on the immutable branch. I'll merge this helper patch and the memory patches into a single series in v2. > On 16-05-21, 23:51, Dmitry Osipenko wrote: >> Add common helper which initializes OPP table for Tegra SoC core devices. >> >> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 >> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 >> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 >> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++ >> include/soc/tegra/common.h | 30 ++++++++++ >> 2 files changed, 142 insertions(+) >> >> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c >> index 3dc54f59cafe..c3fd2facfc2d 100644 >> --- a/drivers/soc/tegra/common.c >> +++ b/drivers/soc/tegra/common.c >> @@ -3,9 +3,16 @@ >> * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved. >> */ >> >> +#define dev_fmt(fmt) "tegra-soc: " fmt >> + >> +#include <linux/clk.h> >> +#include <linux/device.h> >> +#include <linux/export.h> >> #include <linux/of.h> >> +#include <linux/pm_opp.h> >> >> #include <soc/tegra/common.h> >> +#include <soc/tegra/fuse.h> >> >> static const struct of_device_id tegra_machine_match[] = { >> { .compatible = "nvidia,tegra20", }, >> @@ -31,3 +38,108 @@ bool soc_is_tegra(void) >> >> return match != NULL; >> } >> + >> +static int tegra_core_dev_init_opp_state(struct device *dev) >> +{ >> + struct dev_pm_opp *opp; >> + unsigned long rate; >> + struct clk *clk; >> + int err; >> + >> + clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "failed to get clk: %pe\n", clk); >> + return PTR_ERR(clk); >> + } >> + >> + rate = clk_get_rate(clk); >> + if (!rate) { >> + dev_err(dev, "failed to get clk rate\n"); >> + return -EINVAL; >> + } >> + >> + opp = dev_pm_opp_find_freq_ceil(dev, &rate); >> + >> + if (opp == ERR_PTR(-ERANGE)) >> + opp = dev_pm_opp_find_freq_floor(dev, &rate); >> + >> + err = PTR_ERR_OR_ZERO(opp); >> + if (err) { >> + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", >> + rate, err); >> + return err; >> + } > > Why do you need to do this floor/ceil thing? Why can't you simply do > set-rate ? The previous versions of this patch had this comment: /* * dev_pm_opp_set_rate() doesn't search for a floor clock rate and it * will error out if default clock rate is too high, i.e. unsupported * by a SoC hardware version. Hence find floor rate by ourselves. */ I removed it because it appeared to me that it should be obvious why this is needed. The reason why it was added in the first place is that the tegra-clk driver initializes some clock rates to values that aren't supported by all hardware versions in accordance to the OPP tables, although technically those higher rates work okay in practice, this made dev_pm_opp_set_rate() to fail without fixing up the clock rate. You might be right that this is not necessary anymore, the code changed since the last time it was needed. I'll re-check it for the v2. Thank you for the review. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-17 14:09 ` Dmitry Osipenko @ 2021-05-18 3:26 ` Viresh Kumar 0 siblings, 0 replies; 9+ messages in thread From: Viresh Kumar @ 2021-05-18 3:26 UTC (permalink / raw) To: Dmitry Osipenko Cc: Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd, Krzysztof Kozlowski, linux-tegra, linux-kernel On 17-05-21, 17:09, Dmitry Osipenko wrote: > 17.05.2021 06:37, Viresh Kumar пишет: > > I am not sure why you divided this into three different patchsets, > > while it should really have been one. Like this one just adds the API > > but doesn't use it. > > Previously Krzysztof Kozlowski asked to split the large series into smaller sets which could be reviewed and applied separately by maintainers. He suggested that the immutable branch is a better option, I decided to implement this suggestion. So far I only sent out the memory patches which make use of the new helper, there will be more patches. The memory patches are intended to show that this helper can be utilized right now. My plan was to finalize this patch first, then Thierry will apply it and I will be able to sent the rest of the patches telling that they depend on the immutable branch. > > I'll merge this helper patch and the memory patches into a single series in v2. Diving the series is fine, but an API and its users should always be in the same series. You can still apply them differently anyway. > The previous versions of this patch had this comment: > > /* > * dev_pm_opp_set_rate() doesn't search for a floor clock rate and it > * will error out if default clock rate is too high, i.e. unsupported > * by a SoC hardware version. Hence find floor rate by ourselves. > */ > > I removed it because it appeared to me that it should be obvious why this is needed. It can never be obvious to anyone without looking at the API in detail. So if it is indeed required, please keep the comment as is. -- viresh ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-16 20:51 ` [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() Dmitry Osipenko 2021-05-17 3:37 ` Viresh Kumar @ 2021-05-17 11:43 ` Krzysztof Kozlowski 2021-05-17 14:47 ` Dmitry Osipenko 1 sibling, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2021-05-17 11:43 UTC (permalink / raw) To: Dmitry Osipenko, Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd Cc: linux-tegra, linux-kernel On 16/05/2021 16:51, Dmitry Osipenko wrote: > Add common helper which initializes OPP table for Tegra SoC core devices. > > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30 > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20 > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30 > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/soc/tegra/common.c | 112 +++++++++++++++++++++++++++++++++++++ > include/soc/tegra/common.h | 30 ++++++++++ > 2 files changed, 142 insertions(+) > > diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c > index 3dc54f59cafe..c3fd2facfc2d 100644 > --- a/drivers/soc/tegra/common.c > +++ b/drivers/soc/tegra/common.c > @@ -3,9 +3,16 @@ > * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved. > */ > > +#define dev_fmt(fmt) "tegra-soc: " fmt > + > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/export.h> > #include <linux/of.h> > +#include <linux/pm_opp.h> > > #include <soc/tegra/common.h> > +#include <soc/tegra/fuse.h> > > static const struct of_device_id tegra_machine_match[] = { > { .compatible = "nvidia,tegra20", }, > @@ -31,3 +38,108 @@ bool soc_is_tegra(void) > > return match != NULL; > } > + > +static int tegra_core_dev_init_opp_state(struct device *dev) > +{ > + struct dev_pm_opp *opp; > + unsigned long rate; > + struct clk *clk; > + int err; > + > + clk = devm_clk_get(dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(dev, "failed to get clk: %pe\n", clk); > + return PTR_ERR(clk); > + } > + > + rate = clk_get_rate(clk); > + if (!rate) { > + dev_err(dev, "failed to get clk rate\n"); > + return -EINVAL; > + } > + > + opp = dev_pm_opp_find_freq_ceil(dev, &rate); > + > + if (opp == ERR_PTR(-ERANGE)) > + opp = dev_pm_opp_find_freq_floor(dev, &rate); > + > + err = PTR_ERR_OR_ZERO(opp); > + if (err) { > + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", > + rate, err); > + return err; > + } > + > + dev_pm_opp_put(opp); > + > + /* first dummy rate-setting initializes voltage vote */ > + err = dev_pm_opp_set_rate(dev, rate); > + if (err) { > + dev_err(dev, "failed to initialize OPP clock: %d\n", err); > + return err; > + } The devm_pm_opp_set_clkname will call clk_get(), so here you should drop the clk reference at the end. Why having it twice? > + > + return 0; > +} > + > +/** > + * devm_tegra_core_dev_init_opp_table() - initialize OPP table > + * @dev: device for which OPP table is initialized > + * @params: pointer to the OPP table configuration > + * > + * This function will initialize OPP table and sync OPP state of a Tegra SoC > + * core device. > + * > + * Return: 0 on success or errorno. > + */ > +int devm_tegra_core_dev_init_opp_table(struct device *dev, > + struct tegra_core_opp_params *params) > +{ > + 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")) > + return -ENODEV; > + > + if (of_machine_is_compatible("nvidia,tegra20")) > + hw_version = BIT(tegra_sku_info.soc_process_id); > + else > + hw_version = BIT(tegra_sku_info.soc_speedo_id); > + > + err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1); > + if (err) { > + dev_err(dev, "failed to set OPP supported HW: %d\n", err); > + return err; > + } > + > + /* > + * Older device-trees have an empty OPP table, we will get > + * -ENODEV from devm_pm_opp_of_add_table() in this case. > + */ > + err = devm_pm_opp_of_add_table(dev); > + if (err) { > + if (err == -ENODEV) > + dev_err_once(dev, "OPP table not found, please update device-tree\n"); > + else > + dev_err(dev, "failed to add OPP table: %d\n", err); > + > + return err; > + } > + > + if (params->init_state) { > + err = tegra_core_dev_init_opp_state(dev); > + if (err) > + return err; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table); > diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h > index 98027a76ce3d..e8eab13aa199 100644 > --- a/include/soc/tegra/common.h > +++ b/include/soc/tegra/common.h > @@ -6,6 +6,36 @@ > #ifndef __SOC_TEGRA_COMMON_H__ > #define __SOC_TEGRA_COMMON_H__ > > +#include <linux/errno.h> > +#include <linux/types.h> > + > +struct device; > + > +/** > + * Tegra SoC core device OPP table configuration > + * > + * @init_state: pre-initialize OPP state of a device > + */ > +struct tegra_core_opp_params { > + bool init_state; > +}; > + > +#ifdef CONFIG_ARCH_TEGRA > bool soc_is_tegra(void); > +int devm_tegra_core_dev_init_opp_table(struct device *dev, > + struct tegra_core_opp_params *params); > +#else > +static inline bool soc_is_tegra(void) This looks unrelated. Please make it a separate patch. > +{ > + return false; > +} > + > +static inline int > +devm_tegra_core_dev_init_opp_table(struct device *dev, > + struct tegra_core_opp_params *params) > +{ > + return -ENODEV; > +} > +#endif > > #endif /* __SOC_TEGRA_COMMON_H__ */ > Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-17 11:43 ` Krzysztof Kozlowski @ 2021-05-17 14:47 ` Dmitry Osipenko 2021-05-17 14:52 ` Krzysztof Kozlowski 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Osipenko @ 2021-05-17 14:47 UTC (permalink / raw) To: Krzysztof Kozlowski, Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd Cc: linux-tegra, linux-kernel 17.05.2021 14:43, Krzysztof Kozlowski пишет: ... >> +static int tegra_core_dev_init_opp_state(struct device *dev) >> +{ >> + struct dev_pm_opp *opp; >> + unsigned long rate; >> + struct clk *clk; >> + int err; >> + >> + clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "failed to get clk: %pe\n", clk); >> + return PTR_ERR(clk); >> + } >> + >> + rate = clk_get_rate(clk); >> + if (!rate) { >> + dev_err(dev, "failed to get clk rate\n"); >> + return -EINVAL; >> + } >> + >> + opp = dev_pm_opp_find_freq_ceil(dev, &rate); >> + >> + if (opp == ERR_PTR(-ERANGE)) >> + opp = dev_pm_opp_find_freq_floor(dev, &rate); >> + >> + err = PTR_ERR_OR_ZERO(opp); >> + if (err) { >> + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", >> + rate, err); >> + return err; >> + } >> + >> + dev_pm_opp_put(opp); >> + >> + /* first dummy rate-setting initializes voltage vote */ >> + err = dev_pm_opp_set_rate(dev, rate); >> + if (err) { >> + dev_err(dev, "failed to initialize OPP clock: %d\n", err); >> + return err; >> + } > > > The devm_pm_opp_set_clkname will call clk_get(), so here you should drop > the clk reference at the end. Why having it twice? The devm_pm_opp_set_clkname assigns clock to the OPP table. The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't initialize voltage vote and we need this initialization for the Tegra memory drivers. The reference count of the clk will be dropped automatically once device driver is released. The resource-managed helper avoids the need to care about the error unwinding in the code, making it clean and easy to follow. ... >> +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table); >> diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h >> index 98027a76ce3d..e8eab13aa199 100644 >> --- a/include/soc/tegra/common.h >> +++ b/include/soc/tegra/common.h >> @@ -6,6 +6,36 @@ >> #ifndef __SOC_TEGRA_COMMON_H__ >> #define __SOC_TEGRA_COMMON_H__ >> >> +#include <linux/errno.h> >> +#include <linux/types.h> >> + >> +struct device; >> + >> +/** >> + * Tegra SoC core device OPP table configuration >> + * >> + * @init_state: pre-initialize OPP state of a device >> + */ >> +struct tegra_core_opp_params { >> + bool init_state; >> +}; >> + >> +#ifdef CONFIG_ARCH_TEGRA >> bool soc_is_tegra(void); >> +int devm_tegra_core_dev_init_opp_table(struct device *dev, >> + struct tegra_core_opp_params *params); >> +#else >> +static inline bool soc_is_tegra(void) > > This looks unrelated. Please make it a separate patch. The missing stub for soc_is_tegra() popped up multiple times before. Hence it didn't look like a bad idea to me to add stub for it since this patch touches code around it. I'll factor it out into a separate patch in v2. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-17 14:47 ` Dmitry Osipenko @ 2021-05-17 14:52 ` Krzysztof Kozlowski 2021-05-17 15:23 ` Dmitry Osipenko 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2021-05-17 14:52 UTC (permalink / raw) To: Dmitry Osipenko, Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd Cc: linux-tegra, linux-kernel On 17/05/2021 10:47, Dmitry Osipenko wrote: > 17.05.2021 14:43, Krzysztof Kozlowski пишет: > ... >>> +static int tegra_core_dev_init_opp_state(struct device *dev) >>> +{ >>> + struct dev_pm_opp *opp; >>> + unsigned long rate; >>> + struct clk *clk; >>> + int err; >>> + >>> + clk = devm_clk_get(dev, NULL); >>> + if (IS_ERR(clk)) { >>> + dev_err(dev, "failed to get clk: %pe\n", clk); >>> + return PTR_ERR(clk); >>> + } >>> + >>> + rate = clk_get_rate(clk); >>> + if (!rate) { >>> + dev_err(dev, "failed to get clk rate\n"); >>> + return -EINVAL; >>> + } >>> + >>> + opp = dev_pm_opp_find_freq_ceil(dev, &rate); >>> + >>> + if (opp == ERR_PTR(-ERANGE)) >>> + opp = dev_pm_opp_find_freq_floor(dev, &rate); >>> + >>> + err = PTR_ERR_OR_ZERO(opp); >>> + if (err) { >>> + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", >>> + rate, err); >>> + return err; >>> + } >>> + >>> + dev_pm_opp_put(opp); >>> + >>> + /* first dummy rate-setting initializes voltage vote */ >>> + err = dev_pm_opp_set_rate(dev, rate); >>> + if (err) { >>> + dev_err(dev, "failed to initialize OPP clock: %d\n", err); >>> + return err; >>> + } >> >> >> The devm_pm_opp_set_clkname will call clk_get(), so here you should drop >> the clk reference at the end. Why having it twice? > > The devm_pm_opp_set_clkname assigns clock to the OPP table. > > The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't > initialize voltage vote and we need this initialization for the Tegra > memory drivers. I did not get the answer to my question. Why you need to keep the clk reference past this point? Why you cannot drop it after getting rate? > The reference count of the clk will be dropped automatically once device > driver is released. The resource-managed helper avoids the need to care > about the error unwinding in the code, making it clean and easy to follow. I am not saying there is a leak. > > ... >>> +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table); >>> diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h >>> index 98027a76ce3d..e8eab13aa199 100644 >>> --- a/include/soc/tegra/common.h >>> +++ b/include/soc/tegra/common.h >>> @@ -6,6 +6,36 @@ >>> #ifndef __SOC_TEGRA_COMMON_H__ >>> #define __SOC_TEGRA_COMMON_H__ >>> >>> +#include <linux/errno.h> >>> +#include <linux/types.h> >>> + >>> +struct device; >>> + >>> +/** >>> + * Tegra SoC core device OPP table configuration >>> + * >>> + * @init_state: pre-initialize OPP state of a device >>> + */ >>> +struct tegra_core_opp_params { >>> + bool init_state; >>> +}; >>> + >>> +#ifdef CONFIG_ARCH_TEGRA >>> bool soc_is_tegra(void); >>> +int devm_tegra_core_dev_init_opp_table(struct device *dev, >>> + struct tegra_core_opp_params *params); >>> +#else >>> +static inline bool soc_is_tegra(void) >> >> This looks unrelated. Please make it a separate patch. > > The missing stub for soc_is_tegra() popped up multiple times before. > Hence it didn't look like a bad idea to me to add stub for it since this > patch touches code around it. > > I'll factor it out into a separate patch in v2. Thanks! Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() 2021-05-17 14:52 ` Krzysztof Kozlowski @ 2021-05-17 15:23 ` Dmitry Osipenko 0 siblings, 0 replies; 9+ messages in thread From: Dmitry Osipenko @ 2021-05-17 15:23 UTC (permalink / raw) To: Krzysztof Kozlowski, Thierry Reding, Jonathan Hunter, Paul Fertser, Matt Merhar, Peter Geis, Nicolas Chauvet, Viresh Kumar, Stephen Boyd Cc: linux-tegra, linux-kernel 17.05.2021 17:52, Krzysztof Kozlowski пишет: >>>> +static int tegra_core_dev_init_opp_state(struct device *dev) >>>> +{ >>>> + struct dev_pm_opp *opp; >>>> + unsigned long rate; >>>> + struct clk *clk; >>>> + int err; >>>> + >>>> + clk = devm_clk_get(dev, NULL); >>>> + if (IS_ERR(clk)) { >>>> + dev_err(dev, "failed to get clk: %pe\n", clk); >>>> + return PTR_ERR(clk); >>>> + } >>>> + >>>> + rate = clk_get_rate(clk); >>>> + if (!rate) { >>>> + dev_err(dev, "failed to get clk rate\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + opp = dev_pm_opp_find_freq_ceil(dev, &rate); >>>> + >>>> + if (opp == ERR_PTR(-ERANGE)) >>>> + opp = dev_pm_opp_find_freq_floor(dev, &rate); >>>> + >>>> + err = PTR_ERR_OR_ZERO(opp); >>>> + if (err) { >>>> + dev_err(dev, "failed to get OPP for %ld Hz: %d\n", >>>> + rate, err); >>>> + return err; >>>> + } >>>> + >>>> + dev_pm_opp_put(opp); >>>> + >>>> + /* first dummy rate-setting initializes voltage vote */ >>>> + err = dev_pm_opp_set_rate(dev, rate); >>>> + if (err) { >>>> + dev_err(dev, "failed to initialize OPP clock: %d\n", err); >>>> + return err; >>>> + } >>> >>> The devm_pm_opp_set_clkname will call clk_get(), so here you should drop >>> the clk reference at the end. Why having it twice? >> The devm_pm_opp_set_clkname assigns clock to the OPP table. >> >> The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't >> initialize voltage vote and we need this initialization for the Tegra >> memory drivers. > I did not get the answer to my question. Why you need to keep the clk > reference past this point? Why you cannot drop it after getting rate? > >> The reference count of the clk will be dropped automatically once device >> driver is released. The resource-managed helper avoids the need to care >> about the error unwinding in the code, making it clean and easy to follow. > I am not saying there is a leak. > The clk reference is not needed past this point. It doesn't hurt to have additional reference since this allows to make code cleaner. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-05-18 3:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-16 20:51 [PATCH v1 0/1] Add devm_tegra_core_dev_init_opp_table() helper Dmitry Osipenko 2021-05-16 20:51 ` [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table() Dmitry Osipenko 2021-05-17 3:37 ` Viresh Kumar 2021-05-17 14:09 ` Dmitry Osipenko 2021-05-18 3:26 ` Viresh Kumar 2021-05-17 11:43 ` Krzysztof Kozlowski 2021-05-17 14:47 ` Dmitry Osipenko 2021-05-17 14:52 ` Krzysztof Kozlowski 2021-05-17 15:23 ` Dmitry Osipenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).