Re: [PATCH 09/22] gpio: virtio: drop owner assignment
On 27-03-24, 13:41, Krzysztof Kozlowski wrote:
> virtio core already sets the .owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski
>
> ---
>
> Depends on the first patch.
> ---
> drivers/gpio/gpio-virtio.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c
> index fcc5e8c08973..9fae8e396c58 100644
> --- a/drivers/gpio/gpio-virtio.c
> +++ b/drivers/gpio/gpio-virtio.c
> @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = {
> .remove = virtio_gpio_remove,
> .driver = {
> .name = KBUILD_MODNAME,
> - .owner = THIS_MODULE,
> },
> };
> module_virtio_driver(virtio_gpio_driver);
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH v2] drm/lima: Fix opp clkname setting in case of missing regulator
On 27-10-22, 09:32, Erico Nunes wrote:
> Commit d8c32d3971e4 ("drm/lima: Migrate to dev_pm_opp_set_config()")
> introduced a regression as it may undo the clk_names setting in case
> the optional regulator is missing. This resulted in test and performance
> regressions with lima.
>
> Restore the old behavior where clk_names is set separately so it is not
> undone in case of a missing optional regulator.
>
> Fixes: d8c32d3971e4 ("drm/lima: Migrate to dev_pm_opp_set_config()")
> Signed-off-by: Erico Nunes
> ---
> v2: revert back to using devm_pm_opp_set_clkname and
> devm_pm_opp_set_regulators
> ---
> drivers/gpu/drm/lima/lima_devfreq.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
> b/drivers/gpu/drm/lima/lima_devfreq.c
> index 011be7ff51e1..bc8fb4e38d0a 100644
> --- a/drivers/gpu/drm/lima/lima_devfreq.c
> +++ b/drivers/gpu/drm/lima/lima_devfreq.c
> @@ -112,11 +112,6 @@ int lima_devfreq_init(struct lima_device *ldev)
> unsigned long cur_freq;
> int ret;
> const char *regulator_names[] = { "mali", NULL };
> - const char *clk_names[] = { "core", NULL };
> - struct dev_pm_opp_config config = {
> - .regulator_names = regulator_names,
> - .clk_names = clk_names,
> - };
>
> if (!device_property_present(dev, "operating-points-v2"))
> /* Optional, continue without devfreq */
> @@ -124,7 +119,15 @@ int lima_devfreq_init(struct lima_device *ldev)
>
> spin_lock_init(>lock);
>
> - ret = devm_pm_opp_set_config(dev, );
> + /*
> + * clkname is set separately so it is not affected by the optional
> + * regulator setting which may return error.
> + */
> + ret = devm_pm_opp_set_clkname(dev, "core");
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_opp_set_regulators(dev, regulator_names);
> if (ret) {
> /* Continue if the optional regulator is missing */
> if (ret != -ENODEV)
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH] drm/lima: Fix dev_pm_opp_set_config in case of missing regulator
On 26-10-22, 10:39, Erico Nunes wrote:
> Commit d8c32d3971e4 ("drm/lima: Migrate to dev_pm_opp_set_config()")
> introduced a regression as it may undo the clk_names setting in case
> the optional regulator is missing. This resulted in test and performance
> regressions with lima.
>
> Restore the old behavior where clk_names is set separately so it is not
> undone in case of a missing optional regulator.
>
> Fixes: d8c32d3971e4 ("drm/lima: Migrate to dev_pm_opp_set_config()")
> Signed-off-by: Erico Nunes
> ---
> drivers/gpu/drm/lima/lima_devfreq.c | 16 +---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
> b/drivers/gpu/drm/lima/lima_devfreq.c
> index 011be7ff51e1..9c8654934fea 100644
> --- a/drivers/gpu/drm/lima/lima_devfreq.c
> +++ b/drivers/gpu/drm/lima/lima_devfreq.c
> @@ -113,10 +113,12 @@ int lima_devfreq_init(struct lima_device *ldev)
> int ret;
> const char *regulator_names[] = { "mali", NULL };
> const char *clk_names[] = { "core", NULL };
> - struct dev_pm_opp_config config = {
> - .regulator_names = regulator_names,
> + struct dev_pm_opp_config config_clk_names = {
> .clk_names = clk_names,
> };
> + struct dev_pm_opp_config config_regulator_names = {
> + .regulator_names = regulator_names,
> + };
>
> if (!device_property_present(dev, "operating-points-v2"))
> /* Optional, continue without devfreq */
> @@ -124,7 +126,15 @@ int lima_devfreq_init(struct lima_device *ldev)
>
> spin_lock_init(>lock);
>
> - ret = devm_pm_opp_set_config(dev, );
> + /*
> + * Set clk_names separately so it is not undone in case of
> + * a missing optional regulator.
> + */
> + ret = devm_pm_opp_set_config(dev, _clk_names);
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_opp_set_config(dev, _regulator_names);
You can directly call devm_pm_opp_set_clkname() and
devm_pm_opp_set_regulators(), like it was done before my patch, for
single configurations. So basically a simple revert of my commit, with
additional comments you added above.
> if (ret) {
> /* Continue if the optional regulator is missing */
> if (ret != -ENODEV)
> --
> 2.37.3
--
viresh
Re: [PATCH 02/11] ARM: sa1100: remove unused board files
On 21-10-22, 17:49, Arnd Bergmann wrote:
> diff --git a/drivers/cpufreq/sa1110-cpufreq.c
> b/drivers/cpufreq/sa1110-cpufreq.c
> index 1a83c8678a63..bb7f591a8b05 100644
> --- a/drivers/cpufreq/sa1110-cpufreq.c
> +++ b/drivers/cpufreq/sa1110-cpufreq.c
> @@ -344,14 +344,8 @@ static int __init sa1110_clk_init(void)
> if (!name[0]) {
> if (machine_is_assabet())
> name = "TC59SM716-CL3";
> - if (machine_is_pt_system3())
> - name = "K4S641632D";
> - if (machine_is_h3100())
> - name = "KM416S4030CT";
> if (machine_is_jornada720() || machine_is_h3600())
> name = "K4S281632B-1H";
> - if (machine_is_nanoengine())
> - name = "MT48LC8M16A2TG-75";
> }
>
> sdram = sa1110_find_sdram(name);
Acked-by: Viresh Kumar
--
viresh
Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator
Your subject is 87 columns long, better to squeeze it a bit.
On 05-09-22, 19:16, Clément Péron wrote:
> devm_pm_opp_set_regulators() doesn't enable regulator, which make
> regulator framework switching it off during regulator_late_cleanup().
This isn't the normal behavior as it works for everyone at the moment.
You need to explain what special you are doing here, because of which
you are reaching such a situation.
i.e. you are disabling some code that uses GPU ? Please specify exact
code so others can reproduce it as well.
> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator and avoid any switch off
> by regulator_late_cleanup().
The regulator is already enabled I think at this point by the
bootloader. What you are doing here is syncing the state of the
hardware with the software, which would disallow disabling of the
resource unnecessarily.
> Suggested-by: Viresh Kumar
> Signed-off-by: Clément Péron
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..67b242407156 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> return PTR_ERR(opp);
>
> panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> + /* Setup and enable regulator */
Similarly here, explain why this is required to be done.
> + ret = dev_pm_opp_set_opp(dev, opp);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> + return ret;
> + }
> +
> dev_pm_opp_put(opp);
Do this before checking if (ret), so the resource can be freed all the
time.
>
> /*
> --
> 2.34.1
--
viresh
[PATCH V3.1 02/20] OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list
Make dev_pm_opp_set_regulators() accept a NULL terminated list of names
instead of making the callers keep the two parameters in sync, which
creates an opportunity for bugs to get in.
Suggested-by: Greg Kroah-Hartman
Signed-off-by: Viresh Kumar
---
V3->V3.1:
- Update panfrost_drv.c to include the NULL element.
drivers/cpufreq/cpufreq-dt.c| 9 -
drivers/cpufreq/ti-cpufreq.c| 7 +++
drivers/devfreq/exynos-bus.c| 4 ++--
drivers/gpu/drm/lima/lima_devfreq.c | 3 ++-
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 3 +--
drivers/gpu/drm/panfrost/panfrost_drv.c | 15 ++-
drivers/opp/core.c | 18 --
drivers/soc/tegra/pmc.c | 4 ++--
include/linux/pm_opp.h | 9 -
9 files changed, 40 insertions(+), 32 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8fcaba541539..be0c19b3ffa5 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -193,7 +193,7 @@ static int dt_cpufreq_early_init(struct device *dev, int
cpu)
struct private_data *priv;
struct device *cpu_dev;
bool fallback = false;
- const char *reg_name;
+ const char *reg_name[] = { NULL, NULL };
int ret;
/* Check if this CPU is already covered by some other policy */
@@ -218,10 +218,9 @@ static int dt_cpufreq_early_init(struct device *dev, int
cpu)
* OPP layer will be taking care of regulators now, but it needs to know
* the name of the regulator first.
*/
- reg_name = find_supply_name(cpu_dev);
- if (reg_name) {
- priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, _name,
- 1);
+ reg_name[0] = find_supply_name(cpu_dev);
+ if (reg_name[0]) {
+ priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, reg_name);
if (IS_ERR(priv->opp_table)) {
ret = PTR_ERR(priv->opp_table);
if (ret != -EPROBE_DEFER)
diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
index 8f9fdd864391..560d67a6bef1 100644
--- a/drivers/cpufreq/ti-cpufreq.c
+++ b/drivers/cpufreq/ti-cpufreq.c
@@ -173,7 +173,7 @@ static struct ti_cpufreq_soc_data omap34xx_soc_data = {
*seems to always read as 0).
*/
-static const char * const omap3_reg_names[] = {"cpu0", "vbb"};
+static const char * const omap3_reg_names[] = {"cpu0", "vbb", NULL};
static struct ti_cpufreq_soc_data omap36xx_soc_data = {
.reg_names = omap3_reg_names,
@@ -326,7 +326,7 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
const struct of_device_id *match;
struct opp_table *ti_opp_table;
struct ti_cpufreq_data *opp_data;
- const char * const default_reg_names[] = {"vdd", "vbb"};
+ const char * const default_reg_names[] = {"vdd", "vbb", NULL};
int ret;
match = dev_get_platdata(>dev);
@@ -387,8 +387,7 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
if (opp_data->soc_data->reg_names)
reg_names = opp_data->soc_data->reg_names;
ti_opp_table = dev_pm_opp_set_regulators(opp_data->cpu_dev,
-reg_names,
-
ARRAY_SIZE(default_reg_names));
+reg_names);
if (IS_ERR(ti_opp_table)) {
dev_pm_opp_put_supported_hw(opp_data->opp_table);
ret = PTR_ERR(ti_opp_table);
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index e689101abc93..541baff93ee8 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -180,10 +180,10 @@ static int exynos_bus_parent_parse_of(struct device_node
*np,
{
struct device *dev = bus->dev;
struct opp_table *opp_table;
- const char *vdd = "vdd";
+ const char *supplies[] = { "vdd", NULL };
int i, ret, count, size;
- opp_table = dev_pm_opp_set_regulators(dev, , 1);
+ opp_table = dev_pm_opp_set_regulators(dev, supplies);
if (IS_ERR(opp_table)) {
ret = PTR_ERR(opp_table);
dev_err(dev, "failed to set regulators %d\n", ret);
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
b/drivers/gpu/drm/lima/lima_devfreq.c
index 8989e215dfc9..dc83c5421125 100644
--- a/drivers/gpu/drm/lima/lima_devfreq.c
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -111,6 +111,7 @@ int lima_devfreq_init(struct lima_device *ldev)
struct dev_pm_opp *opp;
unsigned long
Re: [PATCH V3 02/20] OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list
On 04-07-22, 15:35, Steven Price wrote:
> I have to say the 'new improved' list ending with NULL approach doesn't
> work out so well for Panfrost. We already have to have a separate
> 'num_supplies' variable for devm_regulator_bulk_get() /
> regulator_bulk_{en,dis}able(), so the keeping everything in sync
> argument is lost here.
>
> I would suggest added the NULL on the end of the lists in panfrost_drv.c
> but then it would break the use of ARRAY_SIZE() to automagically keep
> the length correct...
Actually we can still make it work.
> For now the approach isn't too bad because Panfrost doesn't yet support
> enabling devfreq with more than one supply. But that array isn't going
> to work so nicely when that restriction is removed.
>
> The only sane way I can see of handling this in Panfrost would be
> replicating the loop to count the supplies in the Panfrost code which
> would allow dropping num_supplies from struct panfrost_compatible and
> then supply_names in the same struct could be NULL terminated ready for
> devm_pm_opp_set_regulators().
Or doing this, which will simplify both the cases.
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 7fcbc2a5b6cd..b3b55565b8ef 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -625,24 +625,29 @@ static int panfrost_remove(struct platform_device *pdev)
return 0;
}
-static const char * const default_supplies[] = { "mali" };
+/*
+ * The OPP core wants the supply names to be NULL terminated, but we need the
+ * correct num_supplies value for regulator core. Hence, we NULL terminate here
+ * and then initialize num_supplies with ARRAY_SIZE - 1.
+ */
+static const char * const default_supplies[] = { "mali", NULL };
static const struct panfrost_compatible default_data = {
- .num_supplies = ARRAY_SIZE(default_supplies),
+ .num_supplies = ARRAY_SIZE(default_supplies) - 1,
.supply_names = default_supplies,
.num_pm_domains = 1, /* optional */
.pm_domain_names = NULL,
};
static const struct panfrost_compatible amlogic_data = {
- .num_supplies = ARRAY_SIZE(default_supplies),
+ .num_supplies = ARRAY_SIZE(default_supplies) - 1,
.supply_names = default_supplies,
.vendor_quirk = panfrost_gpu_amlogic_quirk,
};
-static const char * const mediatek_mt8183_supplies[] = { "mali", "sram" };
+static const char * const mediatek_mt8183_supplies[] = { "mali", "sram", NULL
};
static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1",
"core2" };
static const struct panfrost_compatible mediatek_mt8183_data = {
- .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies),
+ .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies) - 1,
.supply_names = mediatek_mt8183_supplies,
.num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains),
.pm_domain_names = mediatek_mt8183_pm_domains,
--
viresh
[PATCH V3 02/20] OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list
Make dev_pm_opp_set_regulators() accept a NULL terminated list of names
instead of making the callers keep the two parameters in sync, which
creates an opportunity for bugs to get in.
Suggested-by: Greg Kroah-Hartman
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/cpufreq-dt.c| 9 -
drivers/cpufreq/ti-cpufreq.c| 7 +++
drivers/devfreq/exynos-bus.c| 4 ++--
drivers/gpu/drm/lima/lima_devfreq.c | 3 ++-
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 ++--
drivers/opp/core.c | 18 --
drivers/soc/tegra/pmc.c | 4 ++--
include/linux/pm_opp.h | 9 -
8 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8fcaba541539..be0c19b3ffa5 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -193,7 +193,7 @@ static int dt_cpufreq_early_init(struct device *dev, int
cpu)
struct private_data *priv;
struct device *cpu_dev;
bool fallback = false;
- const char *reg_name;
+ const char *reg_name[] = { NULL, NULL };
int ret;
/* Check if this CPU is already covered by some other policy */
@@ -218,10 +218,9 @@ static int dt_cpufreq_early_init(struct device *dev, int
cpu)
* OPP layer will be taking care of regulators now, but it needs to know
* the name of the regulator first.
*/
- reg_name = find_supply_name(cpu_dev);
- if (reg_name) {
- priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, _name,
- 1);
+ reg_name[0] = find_supply_name(cpu_dev);
+ if (reg_name[0]) {
+ priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, reg_name);
if (IS_ERR(priv->opp_table)) {
ret = PTR_ERR(priv->opp_table);
if (ret != -EPROBE_DEFER)
diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
index 8f9fdd864391..560d67a6bef1 100644
--- a/drivers/cpufreq/ti-cpufreq.c
+++ b/drivers/cpufreq/ti-cpufreq.c
@@ -173,7 +173,7 @@ static struct ti_cpufreq_soc_data omap34xx_soc_data = {
*seems to always read as 0).
*/
-static const char * const omap3_reg_names[] = {"cpu0", "vbb"};
+static const char * const omap3_reg_names[] = {"cpu0", "vbb", NULL};
static struct ti_cpufreq_soc_data omap36xx_soc_data = {
.reg_names = omap3_reg_names,
@@ -326,7 +326,7 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
const struct of_device_id *match;
struct opp_table *ti_opp_table;
struct ti_cpufreq_data *opp_data;
- const char * const default_reg_names[] = {"vdd", "vbb"};
+ const char * const default_reg_names[] = {"vdd", "vbb", NULL};
int ret;
match = dev_get_platdata(>dev);
@@ -387,8 +387,7 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
if (opp_data->soc_data->reg_names)
reg_names = opp_data->soc_data->reg_names;
ti_opp_table = dev_pm_opp_set_regulators(opp_data->cpu_dev,
-reg_names,
-
ARRAY_SIZE(default_reg_names));
+reg_names);
if (IS_ERR(ti_opp_table)) {
dev_pm_opp_put_supported_hw(opp_data->opp_table);
ret = PTR_ERR(ti_opp_table);
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index e689101abc93..541baff93ee8 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -180,10 +180,10 @@ static int exynos_bus_parent_parse_of(struct device_node
*np,
{
struct device *dev = bus->dev;
struct opp_table *opp_table;
- const char *vdd = "vdd";
+ const char *supplies[] = { "vdd", NULL };
int i, ret, count, size;
- opp_table = dev_pm_opp_set_regulators(dev, , 1);
+ opp_table = dev_pm_opp_set_regulators(dev, supplies);
if (IS_ERR(opp_table)) {
ret = PTR_ERR(opp_table);
dev_err(dev, "failed to set regulators %d\n", ret);
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
b/drivers/gpu/drm/lima/lima_devfreq.c
index 8989e215dfc9..dc83c5421125 100644
--- a/drivers/gpu/drm/lima/lima_devfreq.c
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -111,6 +111,7 @@ int lima_devfreq_init(struct lima_device *ldev)
struct dev_pm_opp *opp;
unsigned long cur_freq;
int ret;
+ const char *regulator_names[] = { "mali", NULL };
if (!device_property_pres
[PATCH V3 07/20] drm/lima: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/lima/lima_devfreq.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
b/drivers/gpu/drm/lima/lima_devfreq.c
index dc83c5421125..011be7ff51e1 100644
--- a/drivers/gpu/drm/lima/lima_devfreq.c
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -112,6 +112,11 @@ int lima_devfreq_init(struct lima_device *ldev)
unsigned long cur_freq;
int ret;
const char *regulator_names[] = { "mali", NULL };
+ const char *clk_names[] = { "core", NULL };
+ struct dev_pm_opp_config config = {
+ .regulator_names = regulator_names,
+ .clk_names = clk_names,
+ };
if (!device_property_present(dev, "operating-points-v2"))
/* Optional, continue without devfreq */
@@ -119,11 +124,7 @@ int lima_devfreq_init(struct lima_device *ldev)
spin_lock_init(>lock);
- ret = devm_pm_opp_set_clkname(dev, "core");
- if (ret)
- return ret;
-
- ret = devm_pm_opp_set_regulators(dev, regulator_names);
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV)
--
2.31.1.272.g89b43f80a514
[PATCH V3 00/20] OPP: Add new configuration interface: dev_pm_opp_set_config()
Hello, We have too many configuration specific APIs currently, six of them already, like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP core and its users to manage. There is also code redundancy in these APIs, in the way they add/manage the OPP table specific stuff. This patch series is an attempt to simplify these interfaces by adding a new interface, dev_pm_opp_set_config(), which is now used by all the existing ones. This series also migrates few users to the new API, where multiple configurations were required and rest are updated for API interface changes. This is pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next This was earlier tested by various folks, I have tested it again on hikey board, it will get further tested on linux-next in the coming days. Build test is already done by Linaro's bot for enough platform though. The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not merge individual patches. V2->V3: - Merged two patchsets: [PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config() [PATCH V2 0/5] OPP: Replace custom set_opp() with config_regulators() - The existing APIs aren't removed anymore, but are made to use the new core API to set various configurations (Greg KH). - clk-names and regulator-names are NULL terminated arrays now (Greg KH). - New interface added: dev_pm_opp_set_config_regulators(). V1->V2: - dev_pm_opp_set_config() doesn't return the OPP table anymore, but a token allocated with xa_alloc(). The same needs to be passed to clear-config API. - Updated all users according to that as well. - The clk_names interface is updated to allow multiple clocks. - Converted few // comments to /* */. - Added tags by few people. - Dropped the last patch to rearrange stuff, not required anymore. Thanks. -- Viresh Viresh Kumar (20): OPP: Track if clock name is configured by platform OPP: Make dev_pm_opp_set_regulators() accept NULL terminated list OPP: Add dev_pm_opp_set_config() and friends cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config() cpufreq: sti: Migrate to dev_pm_opp_set_config() cpufreq: ti: Migrate to dev_pm_opp_set_config() drm/lima: Migrate to dev_pm_opp_set_config() soc/tegra: Add comment over devm_pm_opp_set_clkname() soc/tegra: Migrate to dev_pm_opp_set_config() OPP: Migrate set-regulators API to use set-config helpers OPP: Migrate set-supported-hw API to use set-config helpers OPP: Migrate set-clk-name API to use set-config helpers OPP: Migrate set-opp-helper API to use set-config helpers OPP: Migrate attach-genpd API to use set-config helpers OPP: Migrate set-prop-name helper API to use set-config helpers OPP: Add support for config_regulators() helper OPP: Make _generic_set_opp_regulator() a config_regulators() interface OPP: Add dev_pm_opp_get_supplies() OPP: ti: Migrate to dev_pm_opp_set_config_regulators() OPP: Remove custom OPP helper support drivers/cpufreq/cpufreq-dt.c| 19 +- drivers/cpufreq/imx-cpufreq-dt.c| 12 +- drivers/cpufreq/qcom-cpufreq-nvmem.c| 109 +-- drivers/cpufreq/sti-cpufreq.c | 27 +- drivers/cpufreq/sun50i-cpufreq-nvmem.c | 31 +- drivers/cpufreq/tegra20-cpufreq.c | 12 +- drivers/cpufreq/ti-cpufreq.c| 42 +- drivers/devfreq/exynos-bus.c| 21 +- drivers/gpu/drm/lima/lima_devfreq.c | 12 +- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 4 +- drivers/memory/tegra/tegra124-emc.c | 11 +- drivers/opp/core.c | 821 +--- drivers/opp/opp.h | 32 +- drivers/opp/ti-opp-supply.c | 77 +- drivers/soc/tegra/common.c | 49 +- drivers/soc/tegra/pmc.c | 4 +- include/linux/pm_opp.h | 295 --- 17 files changed, 750 insertions(+), 828 deletions(-) -- 2.31.1.272.g89b43f80a514
[PATCH V2 16/30] drm/tegra: Migrate to dev_pm_opp_set_config()
The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().
Lets start using it.
Tested-by: Dmitry Osipenko
Signed-off-by: Viresh Kumar
---
drivers/gpu/drm/tegra/gr3d.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index a1fd3113ea96..05c45c104e13 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -389,6 +389,10 @@ static int gr3d_init_power(struct device *dev, struct gr3d
*gr3d)
struct device_link *link;
unsigned int i;
int err;
+ struct dev_pm_opp_config config = {
+ .genpd_names = opp_genpd_names,
+ .virt_devs = _virt_devs,
+ };
err = of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells");
@@ -421,7 +425,7 @@ static int gr3d_init_power(struct device *dev, struct gr3d
*gr3d)
if (dev->pm_domain)
return 0;
- err = devm_pm_opp_attach_genpd(dev, opp_genpd_names, _virt_devs);
+ err = devm_pm_opp_set_config(dev, );
if (err)
return err;
--
2.31.1.272.g89b43f80a514
[PATCH V2 15/30] drm/panfrost: Migrate to dev_pm_opp_set_config()
The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().
Lets start using it.
Acked-by: Steven Price
Signed-off-by: Viresh Kumar
---
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 194af7f607a6..7826d9366d35 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -91,6 +91,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct devfreq *devfreq;
struct thermal_cooling_device *cooling;
struct panfrost_devfreq *pfdevfreq = >pfdevfreq;
+ struct dev_pm_opp_config config = {
+ .regulator_names = pfdev->comp->supply_names,
+ .regulator_count = pfdev->comp->num_supplies,
+ };
if (pfdev->comp->num_supplies > 1) {
/*
@@ -101,13 +105,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
return 0;
}
- ret = devm_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
-pfdev->comp->num_supplies);
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV) {
if (ret != -EPROBE_DEFER)
- DRM_DEV_ERROR(dev, "Couldn't set OPP
regulators\n");
+ DRM_DEV_ERROR(dev, "Couldn't set OPP config\n");
return ret;
}
}
--
2.31.1.272.g89b43f80a514
[PATCH V2 14/30] drm/msm: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 8 ++--
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +-
drivers/gpu/drm/msm/dp/dp_ctrl.c| 6 +-
drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +-
5 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c424e9a37669..6ebb5a28c501 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1723,10 +1723,14 @@ static void check_speed_bin(struct device *dev)
{
struct nvmem_cell *cell;
u32 val;
+ struct dev_pm_opp_config config = {
+ .supported_hw = ,
+ .supported_hw_count = 1,
+ };
/*
* If the OPP table specifies a opp-supported-hw property then we have
-* to set something with dev_pm_opp_set_supported_hw() or the table
+* to set something with dev_pm_opp_set_config() or the table
* doesn't get populated so pick an arbitrary value that should
* ensure the default frequencies are selected but not conflict with any
* actual bins
@@ -1748,7 +1752,7 @@ static void check_speed_bin(struct device *dev)
nvmem_cell_put(cell);
}
- devm_pm_opp_set_supported_hw(dev, , 1);
+ devm_pm_opp_set_config(dev, );
}
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 42ed9a3c4905..82801311f7d4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1800,6 +1800,10 @@ static int a6xx_set_supported_hw(struct device *dev,
struct adreno_rev rev)
u32 supp_hw = UINT_MAX;
u32 speedbin;
int ret;
+ struct dev_pm_opp_config config = {
+ .supported_hw = _hw,
+ .supported_hw_count = 1,
+ };
ret = adreno_read_speedbin(dev, );
/*
@@ -1818,11 +1822,7 @@ static int a6xx_set_supported_hw(struct device *dev,
struct adreno_rev rev)
supp_hw = fuse_to_supp_hw(dev, rev, speedbin);
done:
- ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
- if (ret)
- return ret;
-
- return 0;
+ return devm_pm_opp_set_config(dev, );
}
static const struct adreno_gpu_funcs funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e23e2552e802..2213ce52d2fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1215,12 +1215,16 @@ static int dpu_kms_init(struct drm_device *ddev)
struct dev_pm_opp *opp;
int ret = 0;
unsigned long max_freq = ULONG_MAX;
+ struct dev_pm_opp_config config = {
+ .clk_names = (const char *[]){ "core" },
+ .clk_count = 1,
+ };
dpu_kms = devm_kzalloc(>dev, sizeof(*dpu_kms), GFP_KERNEL);
if (!dpu_kms)
return -ENOMEM;
- ret = devm_pm_opp_set_clkname(dev, "core");
+ ret = devm_pm_opp_set_config(dev, );
if (ret)
return ret;
/* OPP table is optional */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index b7f5b8d3bbd6..0c8fc151b4be 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -2022,6 +2022,10 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
dp_link *link,
{
struct dp_ctrl_private *ctrl;
int ret;
+ struct dev_pm_opp_config config = {
+ .clk_names = (const char *[]){ "ctrl_link" },
+ .clk_count = 1,
+ };
if (!dev || !panel || !aux ||
!link || !catalog) {
@@ -2035,7 +2039,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
dp_link *link,
return ERR_PTR(-ENOMEM);
}
- ret = devm_pm_opp_set_clkname(dev, "ctrl_link");
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
dev_err(dev, "invalid DP OPP table in device tree\n");
/* caller do PTR_ERR(opp_table) */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a95d5df52653..35b6722d1cf9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2034,6 +2034,10 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
struct msm_dsi_host *msm_host = NULL;
struct platform_device *pdev = msm_dsi->pdev;
int ret;
+ struct dev_pm_opp_config config = {
+ .clk_names = (const char *[]){ "byte" },
+
[PATCH V2 13/30] drm/lima: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/lima/lima_devfreq.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
b/drivers/gpu/drm/lima/lima_devfreq.c
index 8989e215dfc9..d8c67843fa1b 100644
--- a/drivers/gpu/drm/lima/lima_devfreq.c
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -111,6 +111,12 @@ int lima_devfreq_init(struct lima_device *ldev)
struct dev_pm_opp *opp;
unsigned long cur_freq;
int ret;
+ struct dev_pm_opp_config config = {
+ .regulator_names = (const char *[]){ "mali" },
+ .regulator_count = 1,
+ .clk_names = (const char *[]){ "core" },
+ .clk_count = 1,
+ };
if (!device_property_present(dev, "operating-points-v2"))
/* Optional, continue without devfreq */
@@ -118,11 +124,7 @@ int lima_devfreq_init(struct lima_device *ldev)
spin_lock_init(>lock);
- ret = devm_pm_opp_set_clkname(dev, "core");
- if (ret)
- return ret;
-
- ret = devm_pm_opp_set_regulators(dev, (const char *[]){ "mali" }, 1);
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV)
--
2.31.1.272.g89b43f80a514
[PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config()
Hello, We have too many configuration specific APIs currently, six of them already, like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP core and its users to manage. There is also code redundancy in these APIs, in the way they add/manage the OPP table specific stuff. This patch series is an attempt to simplify these interfaces by adding a single interface, dev_pm_opp_set_config(), which replaces all the existing ones. This series also migrates the users to the new API. The first two patches help get the API in place, followed by patches to migrate the end users. Once all the users are migrated, the last few patches remove the now unused interfaces. This is pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next This is already tested by various folks now. The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not merge individual patches. V1->V2: - dev_pm_opp_set_config() doesn't return the OPP table anymore, but a token allocated with xa_alloc(). The same needs to be passed to clear-config API. - Updated all users according to that as well. - The clk_names interface is updated to allow multiple clocks. - Converted few // comments to /* */. - Added tags by few people. - Dropped the last patch to rearrange stuff, not required anymore. Thanks. -- Viresh Viresh Kumar (30): OPP: Track if clock name is configured by platform OPP: Add dev_pm_opp_set_config() and friends cpufreq: dt: Migrate to dev_pm_opp_set_config() cpufreq: imx: Migrate to dev_pm_opp_set_config() cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config() cpufreq: sti: Migrate to dev_pm_opp_set_config() cpufreq: sun50i: Migrate to dev_pm_opp_set_config() cpufreq: tegra20: Migrate to dev_pm_opp_set_config() cpufreq: ti: Migrate to dev_pm_opp_set_config() devfreq: exynos: Migrate to dev_pm_opp_set_config() devfreq: sun8i: Migrate to dev_pm_opp_set_config() devfreq: tegra30: Migrate to dev_pm_opp_set_config() drm/lima: Migrate to dev_pm_opp_set_config() drm/msm: Migrate to dev_pm_opp_set_config() drm/panfrost: Migrate to dev_pm_opp_set_config() drm/tegra: Migrate to dev_pm_opp_set_config() media: venus: Migrate to dev_pm_opp_set_config() memory: tegra: Migrate to dev_pm_opp_set_config() mmc: sdhci-msm: Migrate to dev_pm_opp_set_config() OPP: ti: Migrate to dev_pm_opp_set_config() soc/tegra: Add comment over devm_pm_opp_set_clkname() soc/tegra: Migrate to dev_pm_opp_set_config() spi: qcom: Migrate to dev_pm_opp_set_config() serial: qcom: Migrate to dev_pm_opp_set_config() OPP: Remove dev_pm_opp_set_regulators() and friends OPP: Remove dev_pm_opp_set_supported_hw() and friends OPP: Remove dev_pm_opp_set_clkname() and friends OPP: Remove dev_pm_opp_register_set_opp_helper() and friends OPP: Remove dev_pm_opp_attach_genpd() and friends OPP: Remove dev_pm_opp_set_prop_name() and friends drivers/cpufreq/cpufreq-dt.c | 20 +- drivers/cpufreq/imx-cpufreq-dt.c | 18 +- drivers/cpufreq/qcom-cpufreq-nvmem.c | 109 +-- drivers/cpufreq/sti-cpufreq.c | 27 +- drivers/cpufreq/sun50i-cpufreq-nvmem.c| 36 +- drivers/cpufreq/tegra20-cpufreq.c | 18 +- drivers/cpufreq/ti-cpufreq.c | 38 +- drivers/devfreq/exynos-bus.c | 25 +- drivers/devfreq/sun8i-a33-mbus.c | 8 +- drivers/devfreq/tegra30-devfreq.c | 8 +- drivers/gpu/drm/lima/lima_devfreq.c | 12 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 8 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +- drivers/gpu/drm/msm/dp/dp_ctrl.c | 6 +- drivers/gpu/drm/msm/dsi/dsi_host.c| 6 +- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 +- drivers/gpu/drm/tegra/gr3d.c | 6 +- .../media/platform/qcom/venus/pm_helpers.c| 18 +- drivers/memory/tegra/tegra124-emc.c | 17 +- drivers/mmc/host/sdhci-msm.c | 6 +- drivers/opp/core.c| 632 -- drivers/opp/opp.h | 23 + drivers/opp/ti-opp-supply.c | 8 +- drivers/soc/tegra/common.c| 45 +- drivers/soc/tegra/pmc.c | 8 +- drivers/spi/spi-geni-qcom.c | 6 +- drivers/spi/spi-qcom-qspi.c | 6 +- drivers/tty/serial/qcom_geni_serial.c | 6 +- include/linux/pm_opp.h| 121 +--- 30 files changed, 605 insertions(+), 661 deletions(-) -- 2.31.1.272.g89b43f80a514
[PATCH 16/31] drm/tegra: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/tegra/gr3d.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index a1fd3113ea96..05c45c104e13 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -389,6 +389,10 @@ static int gr3d_init_power(struct device *dev, struct gr3d
*gr3d)
struct device_link *link;
unsigned int i;
int err;
+ struct dev_pm_opp_config config = {
+ .genpd_names = opp_genpd_names,
+ .virt_devs = _virt_devs,
+ };
err = of_count_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells");
@@ -421,7 +425,7 @@ static int gr3d_init_power(struct device *dev, struct gr3d
*gr3d)
if (dev->pm_domain)
return 0;
- err = devm_pm_opp_attach_genpd(dev, opp_genpd_names, _virt_devs);
+ err = devm_pm_opp_set_config(dev, );
if (err)
return err;
--
2.31.1.272.g89b43f80a514
[PATCH 15/31] drm/panfrost: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 194af7f607a6..7826d9366d35 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -91,6 +91,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
struct devfreq *devfreq;
struct thermal_cooling_device *cooling;
struct panfrost_devfreq *pfdevfreq = >pfdevfreq;
+ struct dev_pm_opp_config config = {
+ .regulator_names = pfdev->comp->supply_names,
+ .regulator_count = pfdev->comp->num_supplies,
+ };
if (pfdev->comp->num_supplies > 1) {
/*
@@ -101,13 +105,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
return 0;
}
- ret = devm_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
-pfdev->comp->num_supplies);
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV) {
if (ret != -EPROBE_DEFER)
- DRM_DEV_ERROR(dev, "Couldn't set OPP
regulators\n");
+ DRM_DEV_ERROR(dev, "Couldn't set OPP config\n");
return ret;
}
}
--
2.31.1.272.g89b43f80a514
[PATCH 14/31] drm/msm: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 8 ++--
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 -
drivers/gpu/drm/msm/dp/dp_ctrl.c| 5 -
drivers/gpu/drm/msm/dsi/dsi_host.c | 5 -
5 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 407f50a15faa..c39fb085a762 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1728,10 +1728,14 @@ static void check_speed_bin(struct device *dev)
{
struct nvmem_cell *cell;
u32 val;
+ struct dev_pm_opp_config config = {
+ .supported_hw = ,
+ .supported_hw_count = 1,
+ };
/*
* If the OPP table specifies a opp-supported-hw property then we have
-* to set something with dev_pm_opp_set_supported_hw() or the table
+* to set something with dev_pm_opp_set_config() or the table
* doesn't get populated so pick an arbitrary value that should
* ensure the default frequencies are selected but not conflict with any
* actual bins
@@ -1753,7 +1757,7 @@ static void check_speed_bin(struct device *dev)
nvmem_cell_put(cell);
}
- devm_pm_opp_set_supported_hw(dev, , 1);
+ devm_pm_opp_set_config(dev, );
}
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 83c31b2ad865..ddb2812b1ff7 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1805,6 +1805,10 @@ static int a6xx_set_supported_hw(struct device *dev,
struct adreno_rev rev)
u32 supp_hw = UINT_MAX;
u32 speedbin;
int ret;
+ struct dev_pm_opp_config config = {
+ .supported_hw = _hw,
+ .supported_hw_count = 1,
+ };
ret = adreno_read_speedbin(dev, );
/*
@@ -1823,11 +1827,7 @@ static int a6xx_set_supported_hw(struct device *dev,
struct adreno_rev rev)
supp_hw = fuse_to_supp_hw(dev, rev, speedbin);
done:
- ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
- if (ret)
- return ret;
-
- return 0;
+ return devm_pm_opp_set_config(dev, );
}
static const struct adreno_gpu_funcs funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e29796c4f27b..43f943fdfde5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1203,12 +1203,15 @@ static int dpu_bind(struct device *dev, struct device
*master, void *data)
struct drm_device *ddev = priv->dev;
struct dpu_kms *dpu_kms;
int ret = 0;
+ struct dev_pm_opp_config config = {
+ .clk_name = "core",
+ };
dpu_kms = devm_kzalloc(>dev, sizeof(*dpu_kms), GFP_KERNEL);
if (!dpu_kms)
return -ENOMEM;
- ret = devm_pm_opp_set_clkname(dev, "core");
+ ret = devm_pm_opp_set_config(dev, );
if (ret)
return ret;
/* OPP table is optional */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 53568567e05b..54bdb33eef45 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1974,6 +1974,9 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
dp_link *link,
{
struct dp_ctrl_private *ctrl;
int ret;
+ struct dev_pm_opp_config config = {
+ .clk_name = "ctrl_link",
+ };
if (!dev || !panel || !aux ||
!link || !catalog) {
@@ -1987,7 +1990,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
dp_link *link,
return ERR_PTR(-ENOMEM);
}
- ret = devm_pm_opp_set_clkname(dev, "ctrl_link");
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
dev_err(dev, "invalid DP OPP table in device tree\n");
/* caller do PTR_ERR(opp_table) */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index d51e70fab93d..7d5b027629d2 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1801,6 +1801,9 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
struct msm_dsi_host *msm_host = NULL;
struct platform_device *pdev = msm_dsi->pdev;
int ret;
+ struct dev_pm_opp_config config = {
+ .clk_name = "byte",
+ };
msm_host = devm_kzalloc(>dev, sizeof(*msm_host), GFP_KERNEL);
if (!msm_hos
[PATCH 13/31] drm/lima: Migrate to dev_pm_opp_set_config()
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
---
drivers/gpu/drm/lima/lima_devfreq.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
b/drivers/gpu/drm/lima/lima_devfreq.c
index 8989e215dfc9..e792ab5cd76a 100644
--- a/drivers/gpu/drm/lima/lima_devfreq.c
+++ b/drivers/gpu/drm/lima/lima_devfreq.c
@@ -111,6 +111,11 @@ int lima_devfreq_init(struct lima_device *ldev)
struct dev_pm_opp *opp;
unsigned long cur_freq;
int ret;
+ struct dev_pm_opp_config config = {
+ .regulator_names = (const char *[]){ "mali" },
+ .regulator_count = 1,
+ .clk_name = "core",
+ };
if (!device_property_present(dev, "operating-points-v2"))
/* Optional, continue without devfreq */
@@ -118,11 +123,7 @@ int lima_devfreq_init(struct lima_device *ldev)
spin_lock_init(>lock);
- ret = devm_pm_opp_set_clkname(dev, "core");
- if (ret)
- return ret;
-
- ret = devm_pm_opp_set_regulators(dev, (const char *[]){ "mali" }, 1);
+ ret = devm_pm_opp_set_config(dev, );
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV)
--
2.31.1.272.g89b43f80a514
[PATCH 00/31] OPP: Add new configuration interface: dev_pm_opp_set_config()
Hello, We have too many configuration specific APIs currently, six of them already, like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP core and its users to manage. There is also code redundancy in these API, in the way they add/manage the OPP table specific stuff. This patch series is an attempt to simplify these interfaces by adding a single interface, dev_pm_opp_set_config(), which replaces all the existing ones. This also migrates the users to the new API. The first two patches help get the API in place, followed by patches to migrate the end users. Once all the users are migrated, the last few patches remove the now unused interfaces. I have lightly tested this on Hikey960 for now and also getting help from various build/boot bots, gitlab and lkp, to get these tested. It would be helpful if someone with access to the affected platforms can give it a try. This is pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/config The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not merge individual patches. Thanks. -- Viresh Viresh Kumar (31): OPP: Track if clock name is configured by platform OPP: Add dev_pm_opp_set_config() and friends cpufreq: dt: Migrate to dev_pm_opp_set_config() cpufreq: imx: Migrate to dev_pm_opp_set_config() cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config() cpufreq: sti: Migrate to dev_pm_opp_set_config() cpufreq: sun50i: Migrate to dev_pm_opp_set_config() cpufreq: tegra20: Migrate to dev_pm_opp_set_config() cpufreq: ti: Migrate to dev_pm_opp_set_config() devfreq: exynos: Migrate to dev_pm_opp_set_config() devfreq: sun8i: Migrate to dev_pm_opp_set_config() devfreq: tegra30: Migrate to dev_pm_opp_set_config() drm/lima: Migrate to dev_pm_opp_set_config() drm/msm: Migrate to dev_pm_opp_set_config() drm/panfrost: Migrate to dev_pm_opp_set_config() drm/tegra: Migrate to dev_pm_opp_set_config() media: venus: Migrate to dev_pm_opp_set_config() media: tegra: Migrate to dev_pm_opp_set_config() mmc: sdhci-msm: Migrate to dev_pm_opp_set_config() OPP: ti: Migrate to dev_pm_opp_set_config() soc/tegra: Remove the call to devm_pm_opp_set_clkname() soc/tegra: Migrate to dev_pm_opp_set_config() spi: qcom: Migrate to dev_pm_opp_set_config() serial: qcom: Migrate to dev_pm_opp_set_config() OPP: Remove dev_pm_opp_set_regulators() and friends OPP: Remove dev_pm_opp_set_supported_hw() and friends OPP: Remove dev_pm_opp_set_clkname() and friends OPP: Remove dev_pm_opp_register_set_opp_helper() and friends OPP: Remove dev_pm_opp_attach_genpd() and friends OPP: Remove dev_pm_opp_set_prop_name() and friends OPP: Rearrange dev_pm_opp_set_config() and friends drivers/cpufreq/cpufreq-dt.c | 14 +- drivers/cpufreq/imx-cpufreq-dt.c | 12 +- drivers/cpufreq/qcom-cpufreq-nvmem.c | 107 +--- drivers/cpufreq/sti-cpufreq.c | 22 +- drivers/cpufreq/sun50i-cpufreq-nvmem.c| 11 +- drivers/cpufreq/tegra20-cpufreq.c | 12 +- drivers/cpufreq/ti-cpufreq.c | 38 +- drivers/devfreq/exynos-bus.c | 14 +- drivers/devfreq/sun8i-a33-mbus.c | 7 +- drivers/devfreq/tegra30-devfreq.c | 8 +- drivers/gpu/drm/lima/lima_devfreq.c | 11 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 8 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 +- drivers/gpu/drm/msm/dp/dp_ctrl.c | 5 +- drivers/gpu/drm/msm/dsi/dsi_host.c| 5 +- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 9 +- drivers/gpu/drm/tegra/gr3d.c | 6 +- .../media/platform/qcom/venus/pm_helpers.c| 16 +- drivers/memory/tegra/tegra124-emc.c | 14 +- drivers/mmc/host/sdhci-msm.c | 5 +- drivers/opp/core.c| 540 +++--- drivers/opp/opp.h | 2 + drivers/opp/ti-opp-supply.c | 6 +- drivers/soc/tegra/common.c| 14 +- drivers/soc/tegra/pmc.c | 8 +- drivers/spi/spi-geni-qcom.c | 5 +- drivers/spi/spi-qcom-qspi.c | 5 +- drivers/tty/serial/qcom_geni_serial.c | 5 +- include/linux/pm_opp.h| 118 ++-- 30 files changed, 444 insertions(+), 598 deletions(-) -- 2.31.1.272.g89b43f80a514
Re: [PATCH] dt-bindings: Improve phandle-array schemas
On 18-01-22, 19:50, Rob Herring wrote: > The 'phandle-array' type is a bit ambiguous. It can be either just an > array of phandles or an array of phandles plus args. Many schemas for > phandle-array properties aren't clear in the schema which case applies > though the description usually describes it. > > The array of phandles case boils down to needing: > > items: > maxItems: 1 > > The phandle plus args cases should typically take this form: > > items: > - items: > - description: A phandle > - description: 1st arg cell > - description: 2nd arg cell > > With this change, some examples need updating so that the bracketing of > property values matches the schema. > > .../devicetree/bindings/opp/opp-v2-base.yaml | 2 + > .../bindings/power/power-domain.yaml | 4 + Acked-by: Viresh Kumar -- viresh
Re: [PATCH RFC] virtio: wrap config->reset calls
On 13-10-21, 06:55, Michael S. Tsirkin wrote: > This will enable cleanups down the road. > The idea is to disable cbs, then add "flush_queued_cbs" callback > as a parameter, this way drivers can flush any work > queued after callbacks have been disabled. > > Signed-off-by: Michael S. Tsirkin > --- > drivers/gpio/gpio-virtio.c | 2 +- > drivers/i2c/busses/i2c-virtio.c | 2 +- Reviewed-by: Viresh Kumar -- viresh
Re: [PATCH v13 01/35] opp: Change type of dev_pm_opp_attach_genpd(names) argument
On 27-09-21, 01:40, Dmitry Osipenko wrote:
> Elements of the 'names' array are not changed by the code, constify them
> for consistency.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 6 +++---
> include/linux/pm_opp.h | 8
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 04b4691a8aac..3057beabd370 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2348,12 +2348,12 @@ static void _opp_detach_genpd(struct opp_table
> *opp_table)
> * "required-opps" are added in DT.
> */
> struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> - const char **names, struct device ***virt_devs)
> + const char * const *names, struct device ***virt_devs)
> {
> struct opp_table *opp_table;
> struct device *virt_dev;
> int index = 0, ret = -EINVAL;
> - const char **name = names;
> + const char * const *name = names;
>
> opp_table = _add_opp_table(dev, false);
> if (IS_ERR(opp_table))
> @@ -2457,7 +2457,7 @@ static void devm_pm_opp_detach_genpd(void *data)
> *
> * Return: 0 on success and errorno otherwise.
> */
> -int devm_pm_opp_attach_genpd(struct device *dev, const char **names,
> +int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
>struct device ***virt_devs)
> {
> struct opp_table *opp_table;
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index a95d6fdd20b6..879c138c7b8e 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -156,9 +156,9 @@ int devm_pm_opp_set_clkname(struct device *dev, const
> char *name);
> struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int
> (*set_opp)(struct dev_pm_set_opp_data *data));
> void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
> int devm_pm_opp_register_set_opp_helper(struct device *dev, int
> (*set_opp)(struct dev_pm_set_opp_data *data));
> -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char
> **names, struct device ***virt_devs);
> +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char *
> const *names, struct device ***virt_devs);
> void dev_pm_opp_detach_genpd(struct opp_table *opp_table);
> -int devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct
> device ***virt_devs);
> +int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
> struct device ***virt_devs);
> struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table
> *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp);
> int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct
> opp_table *dst_table, unsigned int pstate);
> int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
> @@ -376,7 +376,7 @@ static inline int devm_pm_opp_set_clkname(struct device
> *dev, const char *name)
> return -EOPNOTSUPP;
> }
>
> -static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> const char **names, struct device ***virt_devs)
> +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
> const char * const *names, struct device ***virt_devs)
> {
> return ERR_PTR(-EOPNOTSUPP);
> }
> @@ -384,7 +384,7 @@ static inline struct opp_table
> *dev_pm_opp_attach_genpd(struct device *dev, cons
> static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {}
>
> static inline int devm_pm_opp_attach_genpd(struct device *dev,
> -const char **names,
> +const char * const *names,
> struct device ***virt_devs)
> {
> return -EOPNOTSUPP;
Applied. Thanks.
--
viresh
Re: [PATCH v13 00/35] NVIDIA Tegra power management patches for 5.16
On 27-09-21, 01:40, Dmitry Osipenko wrote: > This series adds runtime PM support to Tegra drivers and enables core > voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. > > All patches in this series are interdependent and should go via Tegra tree. So you don't need any OPP changes anymore ? I just came back from vacation, don't know what you guys discussed in between :) -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 26-08-21, 08:24, Viresh Kumar wrote: > On 25-08-21, 18:41, Dmitry Osipenko wrote: > > Thinking a bit more about this, I got a nicer variant which actually works > > in all cases for Tegra. > > > > Viresh / Ulf, what do you think about this: > > This is what I have been suggesting from day 1 :) > > https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/ > > " > And if it is all about just syncing the genpd core, then can the > genpd core do something like what clk framework does? i.e. allow a > new optional genpd callback, get_performance_state() (just like > set_performance_state()), which can be called initially by the core > to get the performance to something other than zero. > " > > Looks good to me :) When you refresh this stuff, please send only 3-4 patches to update the core stuff and show an example. Once we finalize with the interface, you can update all the users. Else this is just noise for everyone else. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 25-08-21, 18:41, Dmitry Osipenko wrote: > Thinking a bit more about this, I got a nicer variant which actually works in > all cases for Tegra. > > Viresh / Ulf, what do you think about this: This is what I have been suggesting from day 1 :) https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/ " And if it is all about just syncing the genpd core, then can the genpd core do something like what clk framework does? i.e. allow a new optional genpd callback, get_performance_state() (just like set_performance_state()), which can be called initially by the core to get the performance to something other than zero. " Looks good to me :) -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 23-08-21, 23:24, Dmitry Osipenko wrote:
> It's not clear to me whether it will be okay to add a generic OPP syncing by
> clock rate or should it be a Tegra-specific helper. Viresh, what do you think
> about this generic OPP helper:
>
> /**
> * dev_pm_opp_sync_with_clk_rate() - Sync OPP state with clock rate
> * @dev: device for which we do this operation
> *
> * Sync OPP table state with the current clock rate of device.
> *
> * Return: 0 on success or a negative error value.
> */
> int dev_pm_opp_sync_with_clk_rate(struct device *dev)
> {
> struct opp_table *opp_table;
> int ret = 0;
>
> /* Device may not have OPP table */
> opp_table = _find_opp_table(dev);
> if (IS_ERR(opp_table))
> return 0;
>
> /* Device may not use clock */
> if (IS_ERR(opp_table->clk))
> goto put_table;
>
> /* Device may have empty OPP table */
> if (!_get_opp_count(opp_table))
> goto put_table;
>
> ret = dev_pm_opp_set_rate(dev, clk_get_rate(opp_table->clk));
> put_table:
> /* Drop reference taken by _find_opp_table() */
> dev_pm_opp_put_opp_table(opp_table);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_sync_with_clk_rate);
I am not sure why you still need this, hope we were going another way
? Anyway I will have a look at what you have posted now.
--
viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 19-08-21, 16:55, Ulf Hansson wrote: > Right, that sounds reasonable. > > We already have pm_genpd_opp_to_performance_state() which translates > an OPP to a performance state. This function invokes the > ->opp_to_performance_state() for a genpd. Maybe we need to allow a > genpd to not have ->opp_to_performance_state() callback assigned > though, but continue up in the hierarchy to see if the parent has the > callback assigned, to make this work for Tegra? > > Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(), > allowing us to pass the device instead of the genpd. But that's a > minor thing. I am not concerned a lot about how it gets implemented, and am not sure as well, as I haven't looked into these details since sometime. Any reasonable thing will be accepted, as simple as that. > Finally, the precondition to use the above, is to first get a handle > to an OPP table. This is where I am struggling to find a generic > solution, because I guess that would be platform or even consumer > driver specific for how to do this. And at what point should we do > this? Hmm, I am not very clear with the whole picture at this point of time. Dmitry, can you try to frame a sequence of events/calls/etc that will define what kind of devices we are looking at here, and how this can be made to work ? > > > Viresh, please take a look at what I did in [1]. Maybe it could be done > > > in another way. > > > > I looked into this and looked like too much trouble. The > > implementation needs to be simple. I am not sure I understand all the > > problems you faced while doing that, would be better to start with a > > simpler implementation of get_performance_state() kind of API for > > genpd, after the domain is attached and its OPP table is initialized. > > > > Note, that the OPP table isn't required to be fully initialized for > > the device at this point, we can parse the DT as well if needed be. > > Sure, but as I indicated above, you need some kind of input data to > figure out what OPP table to pick, before you can translate that into > a performance state. Is that always the clock rate, for example? Eventually it can be clock, bandwidth, or pstate of anther genpd, not sure what all we are looking for now. It should be just clock right now as far as I can imagine :) > Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or > what do you think? Do you have other suggestions? We already have similar APIs, so that won't be a problem. We also have a mechanism inside the OPP core, frequency based, which is used to guess the current OPP. Maybe we can enhance and use that directly here. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 19-08-21, 22:35, Dmitry Osipenko wrote: > 19.08.2021 16:07, Ulf Hansson пишет: > > In the other scenario where a consumer driver prefers to *not* call > > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need > > to power on the device to complete probing, then we don't want to vote > > for an OPP at all - and we also want the performance state for the > > device in genpd to be set to zero. Correct? > > Yes > > > Is this the main problem you are trying to solve, because I think this > > doesn't work out of the box as of today? > > The main problem is that the restored performance state is zero for the > first genpd_runtime_resume(), while it's not zero from the h/w perspective. This is exactly why I have been advocating that the genpd needs to sync up with the hardware before any calls are made to it from the consumer driver. Just what clock framework does to get the clock rate. > > There is another concern though, but perhaps it's not a problem after > > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources > > like clock/regulators. That could certainly be problematic, in > > particular if the device and its genpd have OPP tables associated with > > it and the consumer driver wants to follow the above sequence in > > probe. > > dev_pm_opp_set_rate() won't enable clocks and regulators, but it may It does enable regulators right now, it may choose to enable clocks later on, no guarantees. > change the clock rate and voltage. This is also platform/driver specific > because it's up to OPP user how to configure OPP table. On Tegra we only > assign clock to OPP table, regulators are unused. Right, over that platforms can set their own version of set-opp callback, where all this is done from a platform specific callback. > > Viresh, can you please chime in here and elaborate on some of the > > magic happening behind dev_pm_opp_set_rate() API - is there a problem > > here or not? It configures clock, regulators, genpds, any required OPPs, + it enables regulators right now. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 18:55, Dmitry Osipenko wrote: > 18.08.2021 12:41, Ulf Hansson пишет: > > Either way gives the equal result. The new callback allows to remove the > boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume > of consumer devices, that's it. It may not be equal, as dev_pm_opp_set_rate() may do additional stuff, now or in a later implementation. Currently it only does regulator_enable() as a special case, but it can be clk_enable() as well. Also, this tries to solve the problem in a tricky/hacky way, while all you wanted was to make the genpd aware of what the performance state should be. Your driver can break tomorrow if we started to do more stuff from this API at another time. > > dev_pm_opp_set_rate() is best called from consumer drivers, as they > > need to be in control. > >> What we need here is just configure. So something like this then: > The intent wasn't to use dev_pm_opp_set_rate() from > __genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to > the h/w configuration. Right. > On Tegra we have a chain of PDs and it's not trivial to convert the > device's OPP into pstate because only the parent domain can translate > the required OPP. The driver should just be required to make a call, and OPP/genpd core should return it a value. This is already done today while setting the pstate for a device. The same frameworks must be able to supply a value to be used for the device. > Viresh, please take a look at what I did in [1]. Maybe it could be done > in another way. I looked into this and looked like too much trouble. The implementation needs to be simple. I am not sure I understand all the problems you faced while doing that, would be better to start with a simpler implementation of get_performance_state() kind of API for genpd, after the domain is attached and its OPP table is initialized. Note, that the OPP table isn't required to be fully initialized for the device at this point, we can parse the DT as well if needed be. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 11:41, Ulf Hansson wrote: > On Wed, 18 Aug 2021 at 11:14, Viresh Kumar wrote: > > What we need here is just configure. So something like this then: > > > > - genpd->get_performance_state() > > -> dev_pm_opp_get_current_opp() //New API > > -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); > > > > This can be done just once from probe() then. > > How would dev_pm_opp_get_current_opp() work? Do you have a suggestion? The opp core already has a way of finding current OPP, that's what Dmitry is trying to use here. It finds it using clk_get_rate(), if that is zero, it picks the lowest freq possible. > I am sure I understand the problem. When a device is getting probed, > it needs to consume power, how else can the corresponding driver > successfully probe it? Dmitry can answer that better, but a device doesn't necessarily need to consume energy in probe. It can consume bus clock, like APB we have, but the more energy consuming stuff can be left disabled until the time a user comes up. Probe will just end up registering the driver and initializing it. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 10:29, Ulf Hansson wrote: > Me and Dmitry discussed adding a new genpd callback for this. I agreed > that it seems like a reasonable thing to add, if he insists. > > The intent was to invoke the new callback from __genpd_dev_pm_attach() > when the device has been attached to its genpd. This allows the > callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to > update the vote according to the current state of the HW. I wouldn't call dev_pm_opp_set_rate() from there, since it means configure and enable (both) for different resources, clk, regulator, genpd, etc.. What we need here is just configure. So something like this then: - genpd->get_performance_state() -> dev_pm_opp_get_current_opp() //New API -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate); This can be done just once from probe() then. > I am not sure if/why that approach seemed insufficient? > > Another option to solve the problem, I think, is simply to patch > drivers to let them call dev_pm_opp_set_rate() during ->probe(), this > should synchronize the HW state too. Dmitry already mentioned that this will make the device start consuming power, and he doesn't want that, else we need an explicit disble call as well. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 09:22, Dmitry Osipenko wrote: > 18.08.2021 08:58, Viresh Kumar пишет: > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here > > instead ? That will work, right ? The advantage is it works without > > any special routine to do so. > > It will work, but a dedicated helper is nicer. > > > I also wonder looking at your gr3d.c changes, you set a set-opp > > helper, but the driver doesn't call set_opp_rate at all. Who calls it > > ? > > dev_pm_opp_sync() calls it from _set_opp(). Okay, please use dev_pm_opp_set_rate() instead then. New helper just adds to the confusion and isn't doing anything special apart from doing clk_get_rate() for you. > > And if it is all about just syncing the genpd core, then can the genpd > > core do something like what clk framework does? i.e. allow a new > > optional genpd callback, get_performance_state() (just like > > set_performance_state()), which can be called initially by the core to > > get the performance to something other than zero. opp-set-rate is > > there to set the performance state and enable the stuff as well. > > That's why it looks incorrect in your case, where the function was > > only required to be called once, and you are ending up calling it on > > each resume. Limiting that with another local variable is bad as well. > > We discussed variant with get_performance_state() previously and Ulf > didn't like it either since it still requires to touch 'internals' of GENPD. Hmm, I wonder if that would be a problem since only genpd core is going to call that routine anyway. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 11:28, Viresh Kumar wrote: > On 18-08-21, 08:21, Dmitry Osipenko wrote: > > Yes, GENPD will cache the perf state across suspend/resume and initially > > cached value is out of sync with h/w. > > > > Nothing else. But let me clarify it all again. > > Thanks for your explanation. > > > Initially the performance state of all GENPDs is 0 for all devices. > > > > The clock rate is preinitialized for all devices to a some default rate > > by clk driver, or by bootloader or by assigned-clocks in DT. > > > > When device is rpm-resumed, the resume callback of a device driver > > enables the clock. > > > > Before clock is enabled, the voltage needs to be configured in > > accordance to the clk rate. > > > > So now we have a GENPD with pstate=0 on a first rpm-resume, which > > doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the > > pstate in accordance to the h/w config. > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here > instead ? That will work, right ? The advantage is it works without > any special routine to do so. > > I also wonder looking at your gr3d.c changes, you set a set-opp > helper, but the driver doesn't call set_opp_rate at all. Who calls it > ? > > And if it is all about just syncing the genpd core, then can the genpd > core do something like what clk framework does? i.e. allow a new > optional genpd callback, get_performance_state() (just like > set_performance_state()), which can be called initially by the core to > get the performance to something other than zero. opp-set-rate is > there to set the performance state and enable the stuff as well. > That's why it looks incorrect in your case, where the function was > only required to be called once, and you are ending up calling it on > each resume. Limiting that with another local variable is bad as well. Ulf, this last part is for you :) -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 08:21, Dmitry Osipenko wrote: > Yes, GENPD will cache the perf state across suspend/resume and initially > cached value is out of sync with h/w. > > Nothing else. But let me clarify it all again. Thanks for your explanation. > Initially the performance state of all GENPDs is 0 for all devices. > > The clock rate is preinitialized for all devices to a some default rate > by clk driver, or by bootloader or by assigned-clocks in DT. > > When device is rpm-resumed, the resume callback of a device driver > enables the clock. > > Before clock is enabled, the voltage needs to be configured in > accordance to the clk rate. > > So now we have a GENPD with pstate=0 on a first rpm-resume, which > doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the > pstate in accordance to the h/w config. What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here instead ? That will work, right ? The advantage is it works without any special routine to do so. I also wonder looking at your gr3d.c changes, you set a set-opp helper, but the driver doesn't call set_opp_rate at all. Who calls it ? And if it is all about just syncing the genpd core, then can the genpd core do something like what clk framework does? i.e. allow a new optional genpd callback, get_performance_state() (just like set_performance_state()), which can be called initially by the core to get the performance to something other than zero. opp-set-rate is there to set the performance state and enable the stuff as well. That's why it looks incorrect in your case, where the function was only required to be called once, and you are ending up calling it on each resume. Limiting that with another local variable is bad as well. > In a previous v7 I proposed to preset the rpm_pstate of GENPD (perf > level that is restored before device is rpm-resumed) from PD's > attach_dev callback, but Ulf didn't like that because it requires to use > and modify GENPD 'private' variables from a PD driver. We decided that > will be better to make device drivers to explicitly sync the perf state, > which I implemented in this v8. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 07:37, Dmitry Osipenko wrote: > This will set voltage level without having an actively used hardware. > Take a 3d driver for example, if you set the rate on probe and > rpm-resume will never be called, then the voltage will be set high, > while hardware is kept suspended if userspace will never wake it up by > executing a 3d job. What exactly are we looking to achieve with this stuff ? Cache the current performance state with genpd (based on the state bootloader's has set) ? Or anything else as well ? -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 07:30, Dmitry Osipenko wrote: > 18.08.2021 07:29, Dmitry Osipenko пишет: > >> The first resume initializes the OPP state on sync, all further syncs on > >> resume are no-ops. > >> > > > > Notice that we use GENPD here. GENPD core takes care of storing PD's > > performance state (voltage in case of Tegra) and dropping it to 0 after > > rpm-suspend, GENPD core also restores the state before rpm-resume. > > By 'here' I mean in this series. It is still not clear to me why you need to this on resume, and not probe. -- viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 18-08-21, 07:12, Dmitry Osipenko wrote:
> 18.08.2021 06:55, Viresh Kumar пишет:
> > On 17-08-21, 18:49, Dmitry Osipenko wrote:
> >> 17.08.2021 10:55, Viresh Kumar пишет:
> >> ...
> >>>> +int dev_pm_opp_sync(struct device *dev)
> >>>> +{
> >>>> +struct opp_table *opp_table;
> >>>> +struct dev_pm_opp *opp;
> >>>> +int ret = 0;
> >>>> +
> >>>> +/* Device may not have OPP table */
> >>>> +opp_table = _find_opp_table(dev);
> >>>> +if (IS_ERR(opp_table))
> >>>> +return 0;
> >>>> +
> >>>> +if (!_get_opp_count(opp_table))
> >>>> +goto put_table;
> >>>> +
> >>>> +opp = _find_current_opp(dev, opp_table);
> >>>> +ret = _set_opp(dev, opp_table, opp, opp->rate);
> >>>
> >>> And I am not sure how this will end up working, since new OPP will be
> >>> equal to old one. Since I see you call this from resume() at many
> >>> places.
> >>
> >> Initially OPP table is "uninitialized" and opp_table->enabled=false,
> >> hence the first sync always works even if OPP is equal to old one. Once
> >> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
> >> matter how many times syncing is called.
> >>
> >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
> >
> > Right, but how will this work from Resume ? Won't that be a no-op ?
>
> The first resume initializes the OPP state on sync, all further syncs on
> resume are no-ops.
But the OPPs should already be initialized as someone must have called
opp-set-rate earlier ? Why do this from resume and not probe ?
--
viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 17-08-21, 18:49, Dmitry Osipenko wrote:
> 17.08.2021 10:55, Viresh Kumar пишет:
> ...
> >> +int dev_pm_opp_sync(struct device *dev)
> >> +{
> >> + struct opp_table *opp_table;
> >> + struct dev_pm_opp *opp;
> >> + int ret = 0;
> >> +
> >> + /* Device may not have OPP table */
> >> + opp_table = _find_opp_table(dev);
> >> + if (IS_ERR(opp_table))
> >> + return 0;
> >> +
> >> + if (!_get_opp_count(opp_table))
> >> + goto put_table;
> >> +
> >> + opp = _find_current_opp(dev, opp_table);
> >> + ret = _set_opp(dev, opp_table, opp, opp->rate);
> >
> > And I am not sure how this will end up working, since new OPP will be
> > equal to old one. Since I see you call this from resume() at many
> > places.
>
> Initially OPP table is "uninitialized" and opp_table->enabled=false,
> hence the first sync always works even if OPP is equal to old one. Once
> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
> matter how many times syncing is called.
>
> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
Right, but how will this work from Resume ? Won't that be a no-op ?
--
viresh
Re: [PATCH v8 01/34] opp: Add dev_pm_opp_sync() helper
On 17-08-21, 04:27, Dmitry Osipenko wrote:
> Add dev_pm_opp_sync() helper which syncs OPP table with hardware state
> and vice versa.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 42 +++---
> include/linux/pm_opp.h | 6 ++
> 2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 5543c54dacc5..18016e49605f 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,8 @@ static int _set_required_opps(struct device *dev,
> return ret;
> }
>
> -static void _find_current_opp(struct device *dev, struct opp_table
> *opp_table)
> +static struct dev_pm_opp *
> +_find_current_opp(struct device *dev, struct opp_table *opp_table)
> {
> struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
> unsigned long freq;
> @@ -961,7 +962,7 @@ static void _find_current_opp(struct device *dev, struct
> opp_table *opp_table)
> mutex_unlock(_table->lock);
> }
>
> - opp_table->current_opp = opp;
> + return opp;
> }
>
> static int _disable_opp_table(struct device *dev, struct opp_table
> *opp_table)
> @@ -1003,7 +1004,7 @@ static int _set_opp(struct device *dev, struct
> opp_table *opp_table,
>
> /* Find the currently set OPP if we don't know already */
> if (unlikely(!opp_table->current_opp))
> - _find_current_opp(dev, opp_table);
> + opp_table->current_opp = _find_current_opp(dev, opp_table);
>
> old_opp = opp_table->current_opp;
>
> @@ -2931,3 +2932,38 @@ int dev_pm_opp_sync_regulators(struct device *dev)
> return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_sync() - Sync OPP state
> + * @dev: device for which we do this operation
> + *
> + * Initialize OPP table accordingly to current clock rate or
> + * first available OPP if clock not available for this device.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_sync(struct device *dev)
> +{
> + struct opp_table *opp_table;
> + struct dev_pm_opp *opp;
> + int ret = 0;
> +
> + /* Device may not have OPP table */
> + opp_table = _find_opp_table(dev);
> + if (IS_ERR(opp_table))
> + return 0;
> +
> + if (!_get_opp_count(opp_table))
> + goto put_table;
> +
> + opp = _find_current_opp(dev, opp_table);
> + ret = _set_opp(dev, opp_table, opp, opp->rate);
And I am not sure how this will end up working, since new OPP will be
equal to old one. Since I see you call this from resume() at many
places.
what exactly are you trying to do here ? Those details would be good
to have in commit log as well, I haven't really followed V7 of your
series.
--
viresh
Re: [PATCH 2/2] dt-bindings: opp: Convert to DT schema
Thanks for taking it up :) On 23-06-21, 17:07, Rob Herring wrote: > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > +$id: http://devicetree.org/schemas/opp/opp-v2-base.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic OPP (Operating Performance Points) Common Binding > + > +maintainers: > + - Viresh Kumar > + > +description: | > + Devices work at voltage-current-frequency combinations and some > implementations > + have the liberty of choosing these. These combinations are called Operating > + Performance Points aka OPPs. This document defines bindings for these OPPs > + applicable across wide range of devices. For illustration purpose, this > document > + uses CPU as a device. > + > + This describes the OPPs belonging to a device. > + > +select: false > + > +properties: > + $nodename: > +pattern: '^opp-table(-[a-z0-9]+)?$' > + > + opp-shared: > +description: > + Indicates that device nodes using this OPP Table Node's phandle switch > + their DVFS state together, i.e. they share clock/voltage/current lines. > + Missing property means devices have independent clock/voltage/current > + lines, but they share OPP tables. > +type: boolean > + > +patternProperties: > + '^opp-?[0-9]+$': > +type: object > +description: > + One or more OPP nodes describing voltage-current-frequency > combinations. > + Their name isn't significant but their phandle can be used to > reference an > + OPP. These are mandatory except for the case where the OPP table is > + present only to indicate dependency between devices using the > opp-shared > + property. > + > +properties: > + opp-hz: > +description: > + Frequency in Hz, expressed as a 64-bit big-endian integer. This is > a > + required property for all device nodes, unless another "required" > + property to uniquely identify the OPP nodes exists. Devices like > power > + domains must have another (implementation dependent) property. > + > + opp-peak-kBps: > +description: > + Peak bandwidth in kilobytes per second, expressed as an array of > + 32-bit big-endian integers. Each element of the array represents > the > + peak bandwidth value of each interconnect path. The number of > elements > + should match the number of interconnect paths. > +minItems: 1 > +maxItems: 32 # Should be enough Can we move this down, closer to opp-avg-kBps ? > + > + opp-microvolt: > +description: | > + Voltage for the OPP > + > + A single regulator's voltage is specified with an array of size > one or three. > + Single entry is for target voltage and three entries are for > > + voltages. > + > + Entries for multiple regulators shall be provided in the same > field separated > + by angular brackets <>. The OPP binding doesn't provide any > provisions to > + relate the values to their power supplies or the order in which > the supplies > + need to be configured and that is left for the implementation > specific > + binding. > + > + Entries for all regulators shall be of the same size, i.e. either > all use a > + single value or triplets. > +minItems: 1 > +maxItems: 8 For consistency with rest of the doc, maybe add # Should be enough regulators > +items: > + minItems: 1 > + maxItems: 3 > + > + opp-microamp: > +description: | > + The maximum current drawn by the device in microamperes considering > + system specific parameters (such as transients, process, aging, > + maximum operating temperature range etc.) as necessary. This may be > + used to set the most efficient regulator operating mode. > + > + Should only be set if opp-microvolt(-name)? is set for the OPP. What is the significance of '?' here ? > + > + Entries for multiple regulators shall be provided in the same field > + separated by angular brackets <>. If current values aren't required > + for a regulator, then it shall be filled with 0. If current values > + aren't required for any of the regulators, then this field is not > + required. The OPP binding doesn't provide any provisions to relate > the > + values to their power supplies or the order in which the supplies > need > + to be configured and that is left for the implementation specific > + binding. > +minItems: 1 > +maxItems: 8 # Should be enough regulators > +items: > + minItems: 1 > + maxItems: 3 Acked-by: Viresh Kumar -- viresh
Re: [PATCH 1/2] dt-bindings: Clean-up OPP binding node names in examples
On 23-06-21, 17:07, Rob Herring wrote: > In preparation to convert OPP bindings to DT schema, clean-up a few OPP > binding node names in the binding examples. > > Cc: Georgi Djakov > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Leonard Crestez > Cc: dri-devel@lists.freedesktop.org > Cc: linux...@vger.kernel.org > Signed-off-by: Rob Herring > --- > Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 2 +- > Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml | 2 +- > .../devicetree/bindings/interconnect/fsl,imx8m-noc.yaml | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) Acked-by: Viresh Kumar -- viresh
Re: [PATCH v3 14/15] media: venus: Convert to use resource-managed OPP API
On 25-03-21, 10:13, Stanimir Varbanov wrote: > Hi, > > On 3/14/21 6:34 PM, Dmitry Osipenko wrote: > > From: Yangtao Li > > > > Use resource-managed OPP API to simplify code. > > > > Signed-off-by: Yangtao Li > > Signed-off-by: Dmitry Osipenko > > --- > > drivers/media/platform/qcom/venus/core.h | 1 - > > .../media/platform/qcom/venus/pm_helpers.c| 35 +-- > > 2 files changed, 8 insertions(+), 28 deletions(-) > > > I'll take this through media-tree once OPP API changes are merged. Okay, dropped from my tree. Thanks. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 00/15] Introduce devm_pm_opp_* API
On 14-03-21, 19:33, Dmitry Osipenko wrote: > This series adds resource-managed OPP API helpers and makes drivers > to use them. > > Changelog: > > v3: - Dropped dev_pm_opp_register_notifier(). > > - Changed return type of the devm helpers from opp_table pointer > to errno. > > - Corrected drm/msm patch which missed to remove opp_put_supported_hw() > from a6xx_gpu. Note that the a5xx_gpu driver was missing the > opp_put_supported_hw() at all. > > - Corrected spelling of the ack from Mark Brown. Applied all patches except 11/15. Thanks. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/15] drm/msm: Convert to use resource-managed OPP API
On 14-03-21, 19:34, Dmitry Osipenko wrote: > From: Yangtao Li > > Use resource-managed OPP API to simplify code. > > Signed-off-by: Yangtao Li > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 11 +++-- > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 -- > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 23 +++ > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 -- > drivers/gpu/drm/msm/dp/dp_ctrl.c| 30 + > drivers/gpu/drm/msm/dp/dp_ctrl.h| 1 - > drivers/gpu/drm/msm/dp/dp_display.c | 5 + > drivers/gpu/drm/msm/dsi/dsi_host.c | 13 --- > 11 files changed, 25 insertions(+), 68 deletions(-) This patch has some updates in linux-next, which I don't have. Please get this merged with the drm tree over 5.13-rc1 later. Acked-by: Viresh Kumar -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 01/14] opp: Add devres wrapper for dev_pm_opp_set_clkname
On 11-03-21, 22:20, Dmitry Osipenko wrote:
> +struct opp_table *devm_pm_opp_set_clkname(struct device *dev, const char
> *name)
> +{
> + struct opp_table *opp_table;
> + int err;
> +
> + opp_table = dev_pm_opp_set_clkname(dev, name);
> + if (IS_ERR(opp_table))
> + return opp_table;
> +
> + err = devm_add_action_or_reset(dev, devm_pm_opp_clkname_release,
> opp_table);
> + if (err)
> + opp_table = ERR_PTR(err);
> +
> + return opp_table;
> +}
I wonder if we still need to return opp_table from here, or a simple
integer is fine.. The callers shouldn't be required to use the OPP
table directly anymore I believe and so better simplify the return
part of this and all other routines you are adding here..
If there is a user which needs the opp_table, let it use the regular
non-devm variant.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 05/14] opp: Add devres wrapper for dev_pm_opp_register_notifier
On 11-03-21, 22:20, Dmitry Osipenko wrote: > From: Yangtao Li > > Add devres wrapper for dev_pm_opp_register_notifier() to simplify driver > code. > > Signed-off-by: Yangtao Li > Signed-off-by: Dmitry Osipenko > --- > drivers/opp/core.c | 38 ++ > include/linux/pm_opp.h | 6 ++ > 2 files changed, 44 insertions(+) As I said in the previous version, I am not sure if we need this patch at all. This has only one user. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/31] Introduce devm_pm_opp_* API
On 02-03-21, 16:40, Dmitry Osipenko wrote: > 20.01.2021 19:01, Dmitry Osipenko пишет: > > 01.01.2021 19:54, Yangtao Li пишет: > >> Hi, > >> > >> This patchset add devm_pm_opp_set_clkname, devm_pm_opp_put_clkname, > >> devm_pm_opp_set_regulators, devm_pm_opp_put_regulators, > >> devm_pm_opp_set_supported_hw, devm_pm_opp_of_add_table and > >> devm_pm_opp_register_notifier. > > > > Hello Yangtao, > > > > Thank you for your effort, looking forward to v2! > > Yangtao, could you please let me know what is the status of this series? > Will you be able to make a v2 anytime soon? Dmitry, if Yangtao doesn't reply back this week with a proposal, please go ahead and respin the patches yourself. Thanks. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] mailbox: arm_mhuv2: make remove callback return void
On 02-02-21, 20:43, Uwe Kleine-König wrote:
> My build tests failed to catch that amba driver that would have needed
> adaption in commit 3fd269e74f2f ("amba: Make the remove callback return
> void"). Change the remove function to make the driver build again.
>
> Reported-by: kernel test robot
> Fixes: 3fd269e74f2f ("amba: Make the remove callback return void")
> Signed-off-by: Uwe Kleine-König
> ---
> Hello,
>
> I guess I missed that driver during rebase as it was only introduced in
> the last merge window. Sorry for that.
>
> I'm unsure what is the right thing to do now. Should I redo the pull
> request (with this patch squashed into 3fd269e74f2f)? Or do we just
> apply this patch on top?
>
> FTR, the test robot report is at
> https://lore.kernel.org/r/202102030343.d9j1wukx-...@intel.com
>
> Best regards
> Uwe
>
> drivers/mailbox/arm_mhuv2.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/mailbox/arm_mhuv2.c b/drivers/mailbox/arm_mhuv2.c
> index 67fb10885bb4..6cf1991a5c9c 100644
> --- a/drivers/mailbox/arm_mhuv2.c
> +++ b/drivers/mailbox/arm_mhuv2.c
> @@ -1095,14 +1095,12 @@ static int mhuv2_probe(struct amba_device *adev,
> const struct amba_id *id)
> return ret;
> }
>
> -static int mhuv2_remove(struct amba_device *adev)
> +static void mhuv2_remove(struct amba_device *adev)
> {
> struct mhuv2 *mhu = amba_get_drvdata(adev);
>
> if (mhu->frame == SENDER_FRAME)
> writel_relaxed(0x0, >send->access_request);
> -
> - return 0;
> }
>
> static struct amba_id mhuv2_ids[] = {
Acked-by: Viresh Kumar
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 12/13] drm: msm: Migrate to dev_pm_opp_set_opp()
dev_pm_opp_set_bw() is getting removed and dev_pm_opp_set_opp() should
be used instead. Migrate to the new API.
Signed-off-by: Viresh Kumar
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e6703ae98760..05e0ef58fe32 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -134,7 +134,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct
dev_pm_opp *opp)
if (!gmu->legacy) {
a6xx_hfi_set_freq(gmu, perf_index);
- dev_pm_opp_set_bw(>pdev->dev, opp);
+ dev_pm_opp_set_opp(>pdev->dev, opp);
pm_runtime_put(gmu->dev);
return;
}
@@ -158,7 +158,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct
dev_pm_opp *opp)
if (ret)
dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
- dev_pm_opp_set_bw(>pdev->dev, opp);
+ dev_pm_opp_set_opp(>pdev->dev, opp);
pm_runtime_put(gmu->dev);
}
@@ -866,7 +866,7 @@ static void a6xx_gmu_set_initial_bw(struct msm_gpu *gpu,
struct a6xx_gmu *gmu)
if (IS_ERR_OR_NULL(gpu_opp))
return;
- dev_pm_opp_set_bw(>pdev->dev, gpu_opp);
+ dev_pm_opp_set_opp(>pdev->dev, gpu_opp);
dev_pm_opp_put(gpu_opp);
}
@@ -1072,7 +1072,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
a6xx_gmu_shutdown(gmu);
/* Remove the bus vote */
- dev_pm_opp_set_bw(>pdev->dev, NULL);
+ dev_pm_opp_set_opp(>pdev->dev, NULL);
/*
* Make sure the GX domain is off before turning off the GMU (CX)
--
2.25.0.rc1.19.g042ed3e048af
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/13] opp: Implement dev_pm_opp_set_opp()
Hello, This patchset implements a new API dev_pm_opp_set_opp(), which configures the devices represented by an opp table to a particular opp. The opp core supports a wide variety of devices now, some of them can change frequency and other properties (like CPUs), while others can just change their pstates or regulators (like power domains) and then there are others which can change their bandwidth as well (interconnects). Instead of having separate implementations for all of them, where all will eventually lack something or the other, lets try to implement a common solution for everyone. This takes care of setting regulators, bw, required opps, etc for all device types. Dmitry, please go ahead and try this series. This is based of opp tree's linux-next branch. Sibi, since you added dev_pm_opp_set_bw() earlier, it would be good if you can give this a try. In case this breaks anything for you. I have already tested this on hikey board for CPU devices. To get this tested better and as early as possible, I have pushed it here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next This will be part of linux-next tomorrow. Note, all the patches need to go through OPP tree here. Please provide your Acks for platform specific bits. -- Viresh Viresh Kumar (13): opp: Rename _opp_set_rate_zero() opp: No need to check clk for errors opp: Keep track of currently programmed OPP opp: Split _set_opp() out of dev_pm_opp_set_rate() opp: Allow _set_opp() to work for non-freq devices opp: Allow _generic_set_opp_regulator() to work for non-freq devices opp: Allow _generic_set_opp_clk_only() to work for non-freq devices opp: Update parameters of _set_opp_custom() opp: Implement dev_pm_opp_set_opp() cpufreq: qcom: Migrate to dev_pm_opp_set_opp() devfreq: tegra30: Migrate to dev_pm_opp_set_opp() drm: msm: Migrate to dev_pm_opp_set_opp() opp: Remove dev_pm_opp_set_bw() drivers/cpufreq/qcom-cpufreq-hw.c | 2 +- drivers/devfreq/tegra30-devfreq.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 +- drivers/opp/core.c| 314 ++ drivers/opp/opp.h | 2 + include/linux/pm_opp.h| 6 +- 6 files changed, 184 insertions(+), 150 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/31] serial: qcom_geni_serial: convert to use devm_pm_opp_* API
On 01-01-21, 16:54, Yangtao Li wrote:
> Use devm_pm_opp_* API to simplify code, and we don't need
> to make opp_table glabal.
>
> Let's remove opp_table from geni_se later.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/tty/serial/qcom_geni_serial.c | 23 +--
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 5aada7ebae35..36a92df8ec11 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1352,6 +1352,7 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> int irq;
> bool console = false;
> struct uart_driver *drv;
> + struct opp_table *opp_table;
>
> if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
> console = true;
> @@ -1433,13 +1434,13 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
> port->cts_rts_swap = true;
>
> - port->se.opp_table = dev_pm_opp_set_clkname(>dev, "se");
> - if (IS_ERR(port->se.opp_table))
> - return PTR_ERR(port->se.opp_table);
> + opp_table = devm_pm_opp_set_clkname(>dev, "se");
> + if (IS_ERR(opp_table))
> + return PTR_ERR(opp_table);
> /* OPP table is optional */
> - ret = dev_pm_opp_of_add_table(>dev);
> + ret = devm_pm_opp_of_add_table(>dev);
> if (ret) {
> - dev_pm_opp_put_clkname(port->se.opp_table);
> + devm_pm_opp_put_clkname(>dev, opp_table);
We shouldn't be doing this here, i.e. put_clkname. Even when the OPP
table isn't present, this driver calls dev_pm_opp_set_rate() which
behaves like clk_set_rate() in this case and so the clk name is still
required by the OPP core.
> if (ret != -ENODEV) {
> dev_err(>dev, "invalid OPP table in device
> tree\n");
> return ret;
> @@ -1453,7 +1454,7 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
>
> ret = uart_add_one_port(drv, uport);
> if (ret)
> - goto err;
> + return ret;
>
> irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
> ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
> @@ -1461,7 +1462,7 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> if (ret) {
> dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
> uart_remove_one_port(drv, uport);
> - goto err;
> + return ret;
> }
>
> /*
> @@ -1478,15 +1479,11 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> if (ret) {
> device_init_wakeup(>dev, false);
> uart_remove_one_port(drv, uport);
> - goto err;
> + return ret;
> }
> }
>
> return 0;
> -err:
> - dev_pm_opp_of_remove_table(>dev);
> - dev_pm_opp_put_clkname(port->se.opp_table);
> - return ret;
> }
>
> static int qcom_geni_serial_remove(struct platform_device *pdev)
> @@ -1494,8 +1491,6 @@ static int qcom_geni_serial_remove(struct
> platform_device *pdev)
> struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> struct uart_driver *drv = port->private_data.drv;
>
> - dev_pm_opp_of_remove_table(>dev);
> - dev_pm_opp_put_clkname(port->se.opp_table);
> dev_pm_clear_wake_irq(>dev);
> device_init_wakeup(>dev, false);
> uart_remove_one_port(drv, >uport);
> --
> 2.25.1
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 18/31] drm/lima: remove unneeded devm_devfreq_remove_device()
On 01-01-21, 16:54, Yangtao Li wrote:
> There is no need to manually release devm related resources.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/gpu/drm/lima/lima_devfreq.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_devfreq.c
> b/drivers/gpu/drm/lima/lima_devfreq.c
> index d5937cf86504..7690c5c69f9f 100644
> --- a/drivers/gpu/drm/lima/lima_devfreq.c
> +++ b/drivers/gpu/drm/lima/lima_devfreq.c
> @@ -94,11 +94,6 @@ void lima_devfreq_fini(struct lima_device *ldev)
> devfreq_cooling_unregister(devfreq->cooling);
> devfreq->cooling = NULL;
> }
> -
> - if (devfreq->devfreq) {
> - devm_devfreq_remove_device(ldev->dev, devfreq->devfreq);
> - devfreq->devfreq = NULL;
> - }
> }
Why is this part of this patchset ?
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/31] Introduce devm_pm_opp_* API
On 01-01-21, 16:54, Yangtao Li wrote: > Hi, > > This patchset add devm_pm_opp_set_clkname, devm_pm_opp_put_clkname, > devm_pm_opp_set_regulators, devm_pm_opp_put_regulators, > devm_pm_opp_set_supported_hw, devm_pm_opp_of_add_table and > devm_pm_opp_register_notifier. Please also mention next time to all the maintainers that you need their Acks for their patches and that all these patches should get merged via the OPP tree. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 06/31] serial: qcom_geni_serial: fix potential mem leak in qcom_geni_serial_probe()
On 01-01-21, 16:54, Yangtao Li wrote:
> We should use dev_pm_opp_put_clkname() to free opp table each time
> dev_pm_opp_of_add_table() got error.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/tty/serial/qcom_geni_serial.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 291649f02821..5aada7ebae35 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1438,9 +1438,12 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> return PTR_ERR(port->se.opp_table);
> /* OPP table is optional */
> ret = dev_pm_opp_of_add_table(>dev);
> - if (ret && ret != -ENODEV) {
> - dev_err(>dev, "invalid OPP table in device tree\n");
> - goto put_clkname;
> + if (ret) {
> + dev_pm_opp_put_clkname(port->se.opp_table);
> + if (ret != -ENODEV) {
> + dev_err(>dev, "invalid OPP table in device
> tree\n");
> + return ret;
> + }
> }
>
> port->private_data.drv = drv;
> @@ -1482,7 +1485,6 @@ static int qcom_geni_serial_probe(struct
> platform_device *pdev)
> return 0;
> err:
> dev_pm_opp_of_remove_table(>dev);
> -put_clkname:
> dev_pm_opp_put_clkname(port->se.opp_table);
> return ret;
> }
Since put_clkname is always done in remove(), I don't think there is
any memleak here. Over that with your patch we will do put_clkname
twice now, once in probe and once in remove. And that is a bug AFAICT.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 05/31] opp: Add devres wrapper for dev_pm_opp_register_notifier
On 01-01-21, 16:54, Yangtao Li wrote:
> Add devres wrapper for dev_pm_opp_register_notifier() to simplify driver
> code.
>
> Signed-off-by: Yangtao Li
> ---
> drivers/opp/core.c | 38 ++
> include/linux/pm_opp.h | 6 ++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 6b83e373f0d8..ef3544f8cecd 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2599,6 +2599,44 @@ int dev_pm_opp_unregister_notifier(struct device *dev,
> }
> EXPORT_SYMBOL(dev_pm_opp_unregister_notifier);
>
> +static void devm_pm_opp_notifier_release(struct device *dev, void *res)
> +{
> + struct notifier_block *nb = *(struct notifier_block **)res;
> +
> + WARN_ON(dev_pm_opp_unregister_notifier(dev, nb));
> +}
> +
> +/**
> + * devm_pm_opp_register_notifier() - Register OPP notifier for the device
> + * @dev: Device for which notifier needs to be registered
> + * @nb: Notifier block to be registered
> + *
> + * Return: 0 on success or a negative error value.
> + *
> + * The notifier will be unregistered after the device is destroyed.
> + */
> +int devm_pm_opp_register_notifier(struct device *dev, struct notifier_block
> *nb)
> +{
> + struct notifier_block **ptr;
> + int ret;
> +
> + ptr = devres_alloc(devm_pm_opp_notifier_release, sizeof(*ptr),
> GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + ret = dev_pm_opp_register_notifier(dev, nb);
> + if (ret) {
> + devres_free(ptr);
> + return ret;
> + }
> +
> + *ptr = nb;
> + devres_add(dev, ptr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(devm_pm_opp_register_notifier);
I am not in favor of this patch, and it only has one user, which makes
it more unwanted.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/31] opp: Add devres wrapper for dev_pm_opp_set_clkname and dev_pm_opp_put_clkname
On 01-01-21, 16:54, Yangtao Li wrote:
> +/**
> + * devm_pm_opp_put_clkname() - Releases resources blocked for clk.
> + * @dev: Device for which we do this operation.
> + * @opp_table: OPP table returned from devm_pm_opp_set_clkname().
> + */
> +void devm_pm_opp_put_clkname(struct device *dev, struct opp_table *opp_table)
> +{
> + devm_release_action(dev, devm_pm_opp_clkname_release, opp_table);
> +}
> +EXPORT_SYMBOL_GPL(devm_pm_opp_put_clkname);
We shouldn't be needing changes like this, please drop them for all
patches.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 26/31] PM / devfreq: tegra30: convert to use devm_pm_opp_* API
On 03-01-21, 03:54, Yangtao Li wrote: > Use devm_pm_opp_* API to simplify code, and remove opp_table > from tegra_devfreq. Patches starting this one didn't appear in the same thread and it is a nightmare to apply these now. Please send everything properly next time. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/31] Introduce devm_pm_opp_* API
On 01-01-21, 16:54, Yangtao Li wrote: > Hi, > > This patchset add devm_pm_opp_set_clkname, devm_pm_opp_put_clkname, > devm_pm_opp_set_regulators, devm_pm_opp_put_regulators, > devm_pm_opp_set_supported_hw, devm_pm_opp_of_add_table and > devm_pm_opp_register_notifier. You can't put so many names in the cclist, we are getting failure messages while replying to your patches now. Put all people you want to inform in the bcc section and only the important ones in to/cc list. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 31/31] PM / devfreq: convert to devm_pm_opp_register_notifier and remove unused API
On 03-01-21, 03:57, Yangtao Li wrote: > Use devm_pm_opp_* API to simplify code. > > Signed-off-by: Yangtao Li > --- > drivers/devfreq/devfreq.c | 66 +-- > include/linux/devfreq.h | 23 -- > 2 files changed, 1 insertion(+), 88 deletions(-) Remove the unused stuff in a separate patch and let this layer keep doing the devm thing, I don't think others would need it. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/31] serial: qcom_geni_serial: convert to use devm_pm_opp_* API
Dropped lots of people from cc list
On 04-01-21, 12:49, Viresh Kumar wrote:
> On 01-01-21, 16:54, Yangtao Li wrote:
> > Use devm_pm_opp_* API to simplify code, and we don't need
> > to make opp_table glabal.
> >
> > Let's remove opp_table from geni_se later.
> >
> > Signed-off-by: Yangtao Li
> > ---
> > drivers/tty/serial/qcom_geni_serial.c | 23 +--
> > 1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > b/drivers/tty/serial/qcom_geni_serial.c
> > index 5aada7ebae35..36a92df8ec11 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1352,6 +1352,7 @@ static int qcom_geni_serial_probe(struct
> > platform_device *pdev)
> > int irq;
> > bool console = false;
> > struct uart_driver *drv;
> > + struct opp_table *opp_table;
> >
> > if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
> > console = true;
> > @@ -1433,13 +1434,13 @@ static int qcom_geni_serial_probe(struct
> > platform_device *pdev)
> > if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
> > port->cts_rts_swap = true;
> >
> > - port->se.opp_table = dev_pm_opp_set_clkname(>dev, "se");
> > - if (IS_ERR(port->se.opp_table))
> > - return PTR_ERR(port->se.opp_table);
> > + opp_table = devm_pm_opp_set_clkname(>dev, "se");
> > + if (IS_ERR(opp_table))
> > + return PTR_ERR(opp_table);
> > /* OPP table is optional */
> > - ret = dev_pm_opp_of_add_table(>dev);
> > + ret = devm_pm_opp_of_add_table(>dev);
> > if (ret) {
> > - dev_pm_opp_put_clkname(port->se.opp_table);
> > + devm_pm_opp_put_clkname(>dev, opp_table);
>
> We shouldn't be doing this here, i.e. put_clkname. Even when the OPP
> table isn't present, this driver calls dev_pm_opp_set_rate() which
> behaves like clk_set_rate() in this case and so the clk name is still
> required by the OPP core.
The same problem is there with multiple patches, fix them all please.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 28-12-20, 17:03, Dmitry Osipenko wrote: > 28.12.2020 09:22, Viresh Kumar пишет: > > On 24-12-20, 16:00, Dmitry Osipenko wrote: > >> In a device driver I want to set PD to the lowest performance state by > >> removing the performance vote when dev_pm_opp_set_rate(dev, 0) is > >> invoked by the driver. > >> > >> The OPP core already does this, but if OPP levels don't start from 0 in > >> a device-tree for PD, then it currently doesn't work since there is a > >> need to get a rounded-up performance state because > >> dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and > >> 28). > >> > >> The PD powering off and performance-changes are separate from each other > >> in the GENPD core. The GENPD core automatically turns off domain when > >> all devices within the domain are suspended by system-suspend or RPM. > >> > >> The performance state of a power domain is controlled solely by a device > >> driver. GENPD core only aggregates the performance requests, it doesn't > >> change the performance state of a domain by itself when device is > >> suspended or resumed, IIUC this is intentional. And I want to put domain > >> into lowest performance state when device is suspended. > > > > Right, so if you really want to just drop the performance vote, then with a > > value of 0 for the performance state the call will reach to your genpd's > > callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts > > the > > frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp > > argument > > as NULL and in that case set voltage to 0 and do regulator_disable() as > > well. > > Won't that work better than going for the lowest voltage ? > > > > We can make dev_pm_opp_set_voltage() to accept OPP=NULL in order to > disable the regulator, like it's done for dev_pm_opp_set_rate(dev, 0). > Although, I don't need this kind of behaviour for the Tegra PD driver, > and thus, would prefer to leave this for somebody else to implement in > the future, once it will be really needed. > > Still we need the dev_pm_opp_find_level_ceil() because level=0 means > that we want to set PD to the lowest (minimal) performance state, i.e. > it doesn't necessarily mean that we want to set the voltage to 0 and > disable the PD entirely. GENPD has a separate controls for on/off. Ok. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 24-12-20, 16:00, Dmitry Osipenko wrote: > In a device driver I want to set PD to the lowest performance state by > removing the performance vote when dev_pm_opp_set_rate(dev, 0) is > invoked by the driver. > > The OPP core already does this, but if OPP levels don't start from 0 in > a device-tree for PD, then it currently doesn't work since there is a > need to get a rounded-up performance state because > dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and > 28). > > The PD powering off and performance-changes are separate from each other > in the GENPD core. The GENPD core automatically turns off domain when > all devices within the domain are suspended by system-suspend or RPM. > > The performance state of a power domain is controlled solely by a device > driver. GENPD core only aggregates the performance requests, it doesn't > change the performance state of a domain by itself when device is > suspended or resumed, IIUC this is intentional. And I want to put domain > into lowest performance state when device is suspended. Right, so if you really want to just drop the performance vote, then with a value of 0 for the performance state the call will reach to your genpd's callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts the frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp argument as NULL and in that case set voltage to 0 and do regulator_disable() as well. Won't that work better than going for the lowest voltage ? -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 23-12-20, 23:37, Dmitry Osipenko wrote: > 23.12.2020 08:57, Viresh Kumar пишет: > > What's wrong with getting the regulator in the driver as well ? Apart from > > the > > OPP core ? > > The voltage syncing should be done for each consumer regulator > individually [1]. > > Secondly, regulator core doesn't work well today if the same regulator > is requested more than one time for the same device. Hmm... > >> will return the OPP table regulator in order to allow driver to use the > >> regulator directly. But I'm not sure whether this is a much better > >> option than the opp_sync_regulators() and opp_set_voltage() APIs. > > > > set_voltage() is still fine as there is some data that the OPP core has, but > > sync_regulator() has nothing to do with OPP core. > > > > And this may lead to more wrapper helpers in the OPP core, which I am > > afraid of. > > And so even if it is not the best, I would like the OPP core to provide the > > data > > and not get into this. Ofcourse there is an exception to this, opp_set_rate. > > > > The regulator_sync_voltage() should be invoked only if voltage was > changed previously [1]. > > The OPP core already has the info about whether voltage was changed and > it provides the necessary locking for both set_voltage() and > sync_regulator(). Perhaps I'll need to duplicate that functionality in > the PD driver, instead of making it all generic and re-usable by other > drivers. > > [1] > https://elixir.bootlin.com/linux/v5.10.2/source/drivers/regulator/core.c#L4107 Lets do it in the OPP core and see where we go. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 23-12-20, 23:37, Dmitry Osipenko wrote: > 23.12.2020 07:19, Viresh Kumar пишет: > > On 22-12-20, 22:15, Dmitry Osipenko wrote: > >> 22.12.2020 09:42, Viresh Kumar пишет: > >>> On 17-12-20, 21:06, Dmitry Osipenko wrote: > >>>> Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > >>>> levels don't start from 0 in OPP table and zero usually means a minimal > >>>> level. > >>>> > >>>> Signed-off-by: Dmitry Osipenko > >>> > >>> Why doesn't the exact version work for you here ? > >>> > >> > >> The exact version won't find OPP for level=0 if levels don't start with > >> 0, where 0 means that minimal level is desired. > > > > Right, but why do you need to send 0 for your platform ? > > > > To put power domain into the lowest performance state when device is idling. I see. So you really want to set it to the lowest state or just take the vote out ? Which may end up powering off the domain in the worst case ? -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 23-12-20, 23:38, Dmitry Osipenko wrote: > Well, there is no "same structure", the opp_table->set_opp_data is NULL > there. Right, I saw that yesterday. What I meant was that we need to start allocating the structure for this case now. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 23-12-20, 23:36, Dmitry Osipenko wrote:
> 23.12.2020 07:34, Viresh Kumar пишет:
> > On 22-12-20, 22:19, Dmitry Osipenko wrote:
> >> 22.12.2020 12:12, Viresh Kumar пишет:
> >>> rate will be 0 for both the OPPs here if rate_not_available is true and
> >>> so this
> >>> change shouldn't be required.
> >>
> >> The rate_not_available is negated in the condition. This change is
> >> required because both rates are 0 and then we should proceed to the
> >> levels comparison.
> >
> > Won't that happen without this patch ?
>
> No
This is how the code looks like currently:
int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
{
if (opp1->rate != opp2->rate)
return opp1->rate < opp2->rate ? -1 : 1;
if (opp1->bandwidth && opp2->bandwidth &&
opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 :
1;
if (opp1->level != opp2->level)
return opp1->level < opp2->level ? -1 : 1;
return 0;
}
Lets consider the case you are focussing on, where rate is 0 for both the OPPs,
bandwidth isn't there and we want to run the level comparison here.
Since both the rates are 0, (opp1->rate != opp2->rate) will fail and so we will
move to bandwidth check which will fail too. And so we will get to the level
comparison.
What am I missing here ? I am sure there is something for sure as you won't have
missed this..
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 22-12-20, 22:15, Dmitry Osipenko wrote: > 22.12.2020 09:42, Viresh Kumar пишет: > > On 17-12-20, 21:06, Dmitry Osipenko wrote: > >> Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > >> levels don't start from 0 in OPP table and zero usually means a minimal > >> level. > >> > >> Signed-off-by: Dmitry Osipenko > > > > Why doesn't the exact version work for you here ? > > > > The exact version won't find OPP for level=0 if levels don't start with > 0, where 0 means that minimal level is desired. Right, but why do you need to send 0 for your platform ? -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Support set_opp() customization without requiring to use regulators. This
> is needed by drivers which want to use dev_pm_opp_set_rate() for changing
> rates of a multiple clocks and don't need to touch regulator.
>
> One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware
> units which should be use to the same clock rate, meanwhile voltage
> scaling is done using a power domain. In this case OPP table doesn't have
> a regulator, causing a NULL dereference in _set_opp_custom().
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3d02fe33630b..625dae7a5ecb 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table
> *opp_table,
> struct dev_pm_opp_supply *old_supply,
> struct dev_pm_opp_supply *new_supply)
> {
> - struct dev_pm_set_opp_data *data;
> + struct dev_pm_set_opp_data *data, tmp_data;
> + unsigned int regulator_count;
> int size;
>
> - data = opp_table->set_opp_data;
> + if (opp_table->set_opp_data) {
> + data = opp_table->set_opp_data;
> + regulator_count = opp_table->regulator_count;
> + } else {
> + data = _data;
> + regulator_count = 0;
> + }
> +
We should use the same structure, you can add some checks but not replace the
structure altogether.
> data->regulators = opp_table->regulators;
> - data->regulator_count = opp_table->regulator_count;
> + data->regulator_count = regulator_count;
> data->clk = opp_table->clk;
> data->dev = dev;
>
> data->old_opp.rate = old_freq;
> - size = sizeof(*old_supply) * opp_table->regulator_count;
> + size = sizeof(*old_supply) * regulator_count;
> if (!old_supply)
> memset(data->old_opp.supplies, 0, size);
> else
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 22-12-20, 22:19, Dmitry Osipenko wrote:
> 22.12.2020 12:12, Viresh Kumar пишет:
> > On 17-12-20, 21:06, Dmitry Osipenko wrote:
> >> Fix adding OPP entries in a wrong (opposite) order if OPP rate is
> >> unavailable. The OPP comparison is erroneously skipped if OPP rate is
> >> missing, thus OPPs are left unsorted.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/opp/core.c | 23 ---
> >> drivers/opp/opp.h | 2 +-
> >> 2 files changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index 34f7e530d941..5c7f130a8de2 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct
> >> dev_pm_opp *opp,
> >>return true;
> >> }
> >>
> >> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> >> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> >> + bool rate_not_available)
> >> {
> >> - if (opp1->rate != opp2->rate)
> >> + if (!rate_not_available && opp1->rate != opp2->rate)
> >
> > rate will be 0 for both the OPPs here if rate_not_available is true and so
> > this
> > change shouldn't be required.
>
> The rate_not_available is negated in the condition. This change is
> required because both rates are 0 and then we should proceed to the
> levels comparison.
Won't that happen without this patch ?
> I guess it's not clear by looking at this patch, please see a full
> version of the function:
>
> int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> bool rate_not_available)
> {
> if (!rate_not_available && opp1->rate != opp2->rate)
> return opp1->rate < opp2->rate ? -1 : 1;
> if (opp1->bandwidth && opp2->bandwidth &&
> opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
> return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
> if (opp1->level != opp2->level)
> return opp1->level < opp2->level ? -1 : 1;
> return 0;
> }
>
> Perhaps we could check whether opp1->rate=0, like it's done for the
> opp1->bandwidth. I'll consider this variant for v3, thanks.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 22-12-20, 22:39, Dmitry Osipenko wrote:
> 22.12.2020 22:21, Dmitry Osipenko пишет:
> >>> + if (IS_ERR(opp)) {
> >>> + dev_err(>dev, "failed to find OPP for level %u: %pe\n",
> >>> + level, opp);
> >>> + return PTR_ERR(opp);
> >>> + }
> >>> +
> >>> + err = dev_pm_opp_set_voltage(>dev, opp);
> >> IIUC, you implemented this callback because you want to use the voltage
> >> triplet
> >> present in the OPP table ?
> >>
> >> And so you are setting the regulator ("power") later in this patch ?
> > yes
> >
> >> I am not in favor of implementing this routine, as it just adds a wrapper
> >> above
> >> the regulator API. What you should be doing rather is get the regulator by
> >> yourself here (instead of depending on the OPP core). And then you can do
> >> dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to
> >> implement a version supporting triplet here though for the same.
> >>
> >> And you won't require the sync version of the API as well then.
> >>
> > That's what I initially did for this driver. I don't mind to revert back
> > to the initial variant in v3, it appeared to me that it will be nicer
> > and cleaner to have OPP API managing everything here.
>
> I forgot one important detail (why the initial variant wasn't good)..
> OPP entries that have unsupportable voltages should be filtered out and
> OPP core performs the filtering only if regulator is assigned to the OPP
> table.
>
> If regulator is assigned to the OPP table, then we need to use OPP API
> for driving the regulator, hence that's why I added
> dev_pm_opp_sync_regulators() and dev_pm_opp_set_voltage().
>
> Perhaps it should be possible to add dev_pm_opp_get_regulator() that
What's wrong with getting the regulator in the driver as well ? Apart from the
OPP core ?
> will return the OPP table regulator in order to allow driver to use the
> regulator directly. But I'm not sure whether this is a much better
> option than the opp_sync_regulators() and opp_set_voltage() APIs.
set_voltage() is still fine as there is some data that the OPP core has, but
sync_regulator() has nothing to do with OPP core.
And this may lead to more wrapper helpers in the OPP core, which I am afraid of.
And so even if it is not the best, I would like the OPP core to provide the data
and not get into this. Ofcourse there is an exception to this, opp_set_rate.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 00/48] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 18-12-20, 16:51, Dmitry Osipenko wrote: > Alright, although I haven't pretended that v2 patches should be merged > right away since they are fundamentally different from v1, and thus, all > patches need to be reviewed first. I agree. I have done some basic review for the stuff. > If the current OPP changes look good to you, then please give yours r-b > to the patches. Thanks in advance! r-b-y isn't required as they will go through my tree itself. So if everyone is happy with the idea, please submit the patches separately (fixes, improvements, devm_*, etc). -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 14/48] opp: Filter out OPPs based on availability of a required-OPP
On 22-12-20, 22:17, Dmitry Osipenko wrote:
> 22.12.2020 11:59, Viresh Kumar пишет:
> > On 17-12-20, 21:06, Dmitry Osipenko wrote:
> >> A required OPP may not be available, and thus, all OPPs which are using
> >> this required OPP should be unavailable too.
> >>
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/opp/core.c | 11 ++-
> >> 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > Please send a separate patchset for fixes, as these can also go to 5.11
> > itself.
>
> Alright, although I don't think that this patch fixes any problems for
> existing OPP users.
Because nobody is using this feature, but otherwise this is a fix for me.
> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> >> index d9feb7639598..3d02fe33630b 100644
> >> --- a/drivers/opp/core.c
> >> +++ b/drivers/opp/core.c
> >> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> >> *new_opp,
> >> struct opp_table *opp_table, bool rate_not_available)
> >> {
> >>struct list_head *head;
> >> - int ret;
> >> + int i, ret;
> >>
> >>mutex_lock(_table->lock);
> >>head = _table->opp_list;
> >> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> >> *new_opp,
> >> __func__, new_opp->rate);
> >>}
> >>
> >> + for (i = 0; i < opp_table->required_opp_count && new_opp->available;
> >> i++) {
> >> + if (new_opp->required_opps[i]->available)
> >> + continue;
> >> +
> >> + new_opp->available = false;
> >> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF
> >> (%lu)\n",
> >> + __func__, new_opp->required_opps[i]->np,
> >> new_opp->rate);
> >
> > Why not just break from here ?
>
> The new_opp could be already marked as unavailable by a previous voltage
> check, hence this loop should be skipped entirely in that case.
Then add a separate check for that before the loop as we don't need that check
on every iteration here.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 44/48] ARM: tegra: Add OPP tables and power domains to Tegra30 device-tree
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> diff --git a/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> b/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> index cbe84d25e726..983db1a06682 100644
> --- a/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> +++ b/arch/arm/boot/dts/tegra30-peripherals-opp.dtsi
> @@ -1,6 +1,56 @@
> // SPDX-License-Identifier: GPL-2.0
>
> / {
> + core_opp_table: core-power-domain-opp-table {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + core_opp_950: opp@95 {
> + opp-microvolt = <95 95 135>;
> + opp-level = <95>;
Perhaps you don't need to exactly copy the voltage value into the level field.
The level field can just be kept to 0, 1,2, 3, etc..
> + };
> +
> + core_opp_1000: opp@100 {
> + opp-microvolt = <100 100 135>;
> + opp-level = <100>;
> + };
> +
> + core_opp_1050: opp@105 {
> + opp-microvolt = <105 105 135>;
> + opp-level = <105>;
> + };
> +
> + core_opp_1100: opp@110 {
> + opp-microvolt = <110 110 135>;
> + opp-level = <110>;
> + };
> +
> + core_opp_1150: opp@115 {
> + opp-microvolt = <115 115 135>;
> + opp-level = <115>;
> + };
> +
> + core_opp_1200: opp@120 {
> + opp-microvolt = <120 120 135>;
> + opp-level = <120>;
> + };
> +
> + core_opp_1250: opp@125 {
> + opp-microvolt = <125 125 135>;
> + opp-level = <125>;
> + };
> +
> + core_opp_1300: opp@130 {
> + opp-microvolt = <130 130 135>;
> + opp-level = <130>;
> + };
> +
> + core_opp_1350: opp@135 {
> + opp-microvolt = <135 135 135>;
> + opp-level = <135>;
> + };
> + };
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/48] opp: Add dev_pm_opp_set_voltage()
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add dev_pm_opp_set_voltage() which allows OPP table users to set voltage > in accordance to a given OPP. In particular this is needed for driving > voltage of a generic power domain which uses OPPs and doesn't have a > clock. > > Signed-off-by: Dmitry Osipenko We shouldn't be doing this, details in patch 28. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 14/48] opp: Filter out OPPs based on availability of a required-OPP
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> A required OPP may not be available, and thus, all OPPs which are using
> this required OPP should be unavailable too.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
Please send a separate patchset for fixes, as these can also go to 5.11 itself.
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index d9feb7639598..3d02fe33630b 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
>struct opp_table *opp_table, bool rate_not_available)
> {
> struct list_head *head;
> - int ret;
> + int i, ret;
>
> mutex_lock(_table->lock);
> head = _table->opp_list;
> @@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
>__func__, new_opp->rate);
> }
>
> + for (i = 0; i < opp_table->required_opp_count && new_opp->available;
> i++) {
> + if (new_opp->required_opps[i]->available)
> + continue;
> +
> + new_opp->available = false;
> + dev_warn(dev, "%s: OPP not supported by required OPP %pOF
> (%lu)\n",
> + __func__, new_opp->required_opps[i]->np,
> new_opp->rate);
Why not just break from here ?
> + }
> +
> return 0;
> }
>
> --
> 2.29.2
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 13/48] opp: Add resource-managed versions of OPP API functions
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add resource-managed versions of OPP API functions. This removes a need > from drivers to store and manage OPP table pointers. > > Signed-off-by: Dmitry Osipenko > --- > drivers/opp/core.c | 173 + > drivers/opp/of.c | 25 ++ > include/linux/pm_opp.h | 51 > 3 files changed, 249 insertions(+) Please send a patchset of its own for this patch, along with updates to all the existing code that can make use of these. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> +++ b/drivers/soc/tegra/core-power-domain.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NVIDIA Tegra SoC Core Power Domain Driver
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +static struct lock_class_key tegra_core_domain_lock_class;
> +static bool tegra_core_domain_state_synced;
> +
> +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd,
> + unsigned int level)
> +{
> + struct dev_pm_opp *opp;
> + int err;
> +
> + opp = dev_pm_opp_find_level_ceil(>dev, );
We don't need ceil or floor versions for level, but rather _exact() version. Or
maybe just call it dev_pm_opp_find_level().
> + if (IS_ERR(opp)) {
> + dev_err(>dev, "failed to find OPP for level %u: %pe\n",
> + level, opp);
> + return PTR_ERR(opp);
> + }
> +
> + err = dev_pm_opp_set_voltage(>dev, opp);
IIUC, you implemented this callback because you want to use the voltage triplet
present in the OPP table ?
And so you are setting the regulator ("power") later in this patch ?
I am not in favor of implementing this routine, as it just adds a wrapper above
the regulator API. What you should be doing rather is get the regulator by
yourself here (instead of depending on the OPP core). And then you can do
dev_pm_opp_get_voltage() here and set the voltage yourself. You may want to
implement a version supporting triplet here though for the same.
And you won't require the sync version of the API as well then.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 09/48] opp: Add dev_pm_opp_sync_regulators()
On 17-12-20, 21:05, Dmitry Osipenko wrote: > Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs > voltage state of regulators. > > Signed-off-by: Dmitry Osipenko We shouldn't be doing this, details in patch 28. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 43/48] ARM: tegra: Add OPP tables and power domains to Tegra20 device-tree
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> diff --git a/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> index b84afecea154..7e015cdfbc55 100644
> --- a/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> +++ b/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi
> @@ -1,6 +1,46 @@
> // SPDX-License-Identifier: GPL-2.0
>
> / {
> + core_opp_table: core-power-domain-opp-table {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + core_opp_950: opp@95 {
> + opp-microvolt = <95 95 130>;
> + opp-level = <95>;
> + };
I am not sure I fully understand this, why does it have both microvolt and level
properties ?
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Fix adding OPP entries in a wrong (opposite) order if OPP rate is
> unavailable. The OPP comparison is erroneously skipped if OPP rate is
> missing, thus OPPs are left unsorted.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 23 ---
> drivers/opp/opp.h | 2 +-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 34f7e530d941..5c7f130a8de2 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct
> dev_pm_opp *opp,
> return true;
> }
>
> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
> + bool rate_not_available)
> {
> - if (opp1->rate != opp2->rate)
> + if (!rate_not_available && opp1->rate != opp2->rate)
rate will be 0 for both the OPPs here if rate_not_available is true and so this
change shouldn't be required.
> return opp1->rate < opp2->rate ? -1 : 1;
> if (opp1->bandwidth && opp2->bandwidth &&
> opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
> @@ -1545,7 +1546,8 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct
> dev_pm_opp *opp2)
>
> static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
>struct opp_table *opp_table,
> - struct list_head **head)
> + struct list_head **head,
> + bool rate_not_available)
> {
> struct dev_pm_opp *opp;
> int opp_cmp;
> @@ -1559,13 +1561,13 @@ static int _opp_is_duplicate(struct device *dev,
> struct dev_pm_opp *new_opp,
>* loop.
>*/
> list_for_each_entry(opp, _table->opp_list, node) {
> - opp_cmp = _opp_compare_key(new_opp, opp);
> + opp_cmp = _opp_compare_key(new_opp, opp, rate_not_available);
> if (opp_cmp > 0) {
> *head = >node;
> continue;
> }
>
> - if (opp_cmp < 0)
> + if (opp_cmp < 0 || rate_not_available)
> return 0;
This shouldn't be required as well, isn't it ?
>
> /* Duplicate OPPs */
> @@ -1601,12 +1603,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp
> *new_opp,
> mutex_lock(_table->lock);
> head = _table->opp_list;
>
> - if (likely(!rate_not_available)) {
> - ret = _opp_is_duplicate(dev, new_opp, opp_table, );
> - if (ret) {
> - mutex_unlock(_table->lock);
> - return ret;
> - }
> + ret = _opp_is_duplicate(dev, new_opp, opp_table, ,
> + rate_not_available);
This is the only thing we need to do here I believe.
> + if (ret) {
> + mutex_unlock(_table->lock);
> + return ret;
> }
>
> list_add(_opp->node, head);
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 4ced7ffa8158..6f5be6c72f13 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -219,7 +219,7 @@ struct opp_table *_find_opp_table(struct device *dev);
> struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table
> *opp_table);
> struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
> void _opp_free(struct dev_pm_opp *opp);
> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool
> rate_not_available);
> int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct
> opp_table *opp_table, bool rate_not_available);
> int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned
> long freq, long u_volt, bool dynamic);
> void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int
> last_cpu);
> --
> 2.29.2
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/48] opp: Add dev_pm_opp_find_level_ceil()
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if > levels don't start from 0 in OPP table and zero usually means a minimal > level. > > Signed-off-by: Dmitry Osipenko Why doesn't the exact version work for you here ? -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On Mon, 9 Nov 2020 at 16:51, Frank Lee wrote: > On Mon, Nov 9, 2020 at 1:53 PM Viresh Kumar wrote: > > > devm_pm_opp_set_supported_hw() > > > devm_pm_opp_set_regulators() [if we won't use GENPD] > > > devm_pm_opp_set_clkname() > > > devm_pm_opp_of_add_table() > > > > I tried to look earlier for the stuff already merged in and didn't > > find a lot of stuff where the devm_* could be used, maybe I missed > > some of it. > > > > Frank, would you like to refresh your series based on suggestions from > > Dmitry and make other drivers adapt to the new APIs ? > > I am glad to do this.：） Frank, Dmitry has submitted a series with a patch that does stuff like this since you never resent your patches. http://lore.kernel.org/lkml/20201217180638.22748-14-dig...@gmail.com Since you were the first one to get to this, I would still like to give you a chance to get these patches merged under your authorship, otherwise I would be going to pick patches from Dmitry. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 15/48] opp: Support set_opp() customization without requiring to use regulators
On 17-12-20, 21:06, Dmitry Osipenko wrote:
> Support set_opp() customization without requiring to use regulators. This
> is needed by drivers which want to use dev_pm_opp_set_rate() for changing
> rates of a multiple clocks and don't need to touch regulator.
>
> One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware
> units which should be use to the same clock rate, meanwhile voltage
> scaling is done using a power domain. In this case OPP table doesn't have
> a regulator, causing a NULL dereference in _set_opp_custom().
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/opp/core.c | 16
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 3d02fe33630b..625dae7a5ecb 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table
> *opp_table,
> struct dev_pm_opp_supply *old_supply,
> struct dev_pm_opp_supply *new_supply)
> {
> - struct dev_pm_set_opp_data *data;
> + struct dev_pm_set_opp_data *data, tmp_data;
> + unsigned int regulator_count;
> int size;
>
> - data = opp_table->set_opp_data;
> + if (opp_table->set_opp_data) {
> + data = opp_table->set_opp_data;
> + regulator_count = opp_table->regulator_count;
> + } else {
> + data = _data;
> + regulator_count = 0;
> + }
> +
> data->regulators = opp_table->regulators;
> - data->regulator_count = opp_table->regulator_count;
> + data->regulator_count = regulator_count;
> data->clk = opp_table->clk;
> data->dev = dev;
>
> data->old_opp.rate = old_freq;
> - size = sizeof(*old_supply) * opp_table->regulator_count;
> + size = sizeof(*old_supply) * regulator_count;
> if (!old_supply)
> memset(data->old_opp.supplies, 0, size);
> else
I don't see you making use of this in this patchset. How did you get this to
crash ?
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 00/48] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 17-12-20, 21:05, Dmitry Osipenko wrote: > Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces > power consumption and heating of the Tegra chips. Tegra SoC has multiple > hardware units which belong to a core power domain of the SoC and share > the core voltage. The voltage must be selected in accordance to a minimum > requirement of every core hardware unit. Please submit the OPP changes separately (alone), it will never get merged this way. Send fixes at the top, any features you want later in the series. It is fine for you to base your series of patches which are under review, you just need to mention that in your cover letter for your platform's patchset. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
On 07-12-20, 11:46, Viresh Kumar wrote:
> On 19-11-20, 11:35, Viresh Kumar wrote:
> > On 18-11-20, 08:53, Rob Clark wrote:
> > > On Tue, Nov 17, 2020 at 9:28 PM Viresh Kumar
> > > wrote:
> > > >
> > > > On 17-11-20, 09:02, Rob Clark wrote:
> > > > > With that on top of the previous patch,
> > > >
> > > > Don't you still have this ? Which fixed the lockdep in the remove path.
> > > >
> > > > https://lore.kernel.org/lkml/20201022080644.2ck4okrxygmkuatn@vireshk-i7/
> > > >
> > > > To make it clear you need these patches to fix the OPP stuff:
> > > >
> > > > //From 5.10-rc3 (the one from the above link).
> > > > commit e0df59de670b ("opp: Reduce the size of critical section in
> > > > _opp_table_kref_release()")
> >
> > This fixes debugfs stuff while the OPP table is removed.
> >
> > > > //Below two from linux-next
> > > > commit ef43f01ac069 ("opp: Always add entries in dev_list with
> > > > opp_table->lock held")
> > > > commit 27c09484dd3d ("opp: Allocate the OPP table outside of
> > > > opp_table_lock")
> >
> > This fixes debugfs stuff while the OPP table is added.
> >
> > > > This matches the diff I gave you earlier.
> > > >
> > >
> > > no, I did not have all three, only "opp: Allocate the OPP table
> > > outside of opp_table_lock" plus the fixup. But with all three:
> >
> > And looking at the lockdep you gave now, it looks like we have a
> > problem with OPP table's internal lock (opp_table->lock) as well apart
> > from the global opp_table_lock.
> >
> > I wish there was a way for me to reproduce the lockdep :(
> >
> > I know this is exhausting for both of us and I really want to be over
> > with it as soon as possible, this really should be the last patch
> > here, please try this along with other two. This fixes the debugfs
> > thing while the OPPs in the OPP table are removed (they are already
> > added without a lock around debugfs stuff).
> >
> > AFAIU, there is no further debugfs stuff that happens from within the
> > locks and so this really should be the last patch unless I missed
> > something.
>
> Rob, were you able to test this patch ?
FWIW, this patch and everything else I had is merged into Linus's
master. You can test 5.11-rc1 to see if you still see a lockdep or
not.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
On 19-11-20, 11:35, Viresh Kumar wrote:
> On 18-11-20, 08:53, Rob Clark wrote:
> > On Tue, Nov 17, 2020 at 9:28 PM Viresh Kumar
> > wrote:
> > >
> > > On 17-11-20, 09:02, Rob Clark wrote:
> > > > With that on top of the previous patch,
> > >
> > > Don't you still have this ? Which fixed the lockdep in the remove path.
> > >
> > > https://lore.kernel.org/lkml/20201022080644.2ck4okrxygmkuatn@vireshk-i7/
> > >
> > > To make it clear you need these patches to fix the OPP stuff:
> > >
> > > //From 5.10-rc3 (the one from the above link).
> > > commit e0df59de670b ("opp: Reduce the size of critical section in
> > > _opp_table_kref_release()")
>
> This fixes debugfs stuff while the OPP table is removed.
>
> > > //Below two from linux-next
> > > commit ef43f01ac069 ("opp: Always add entries in dev_list with
> > > opp_table->lock held")
> > > commit 27c09484dd3d ("opp: Allocate the OPP table outside of
> > > opp_table_lock")
>
> This fixes debugfs stuff while the OPP table is added.
>
> > > This matches the diff I gave you earlier.
> > >
> >
> > no, I did not have all three, only "opp: Allocate the OPP table
> > outside of opp_table_lock" plus the fixup. But with all three:
>
> And looking at the lockdep you gave now, it looks like we have a
> problem with OPP table's internal lock (opp_table->lock) as well apart
> from the global opp_table_lock.
>
> I wish there was a way for me to reproduce the lockdep :(
>
> I know this is exhausting for both of us and I really want to be over
> with it as soon as possible, this really should be the last patch
> here, please try this along with other two. This fixes the debugfs
> thing while the OPPs in the OPP table are removed (they are already
> added without a lock around debugfs stuff).
>
> AFAIU, there is no further debugfs stuff that happens from within the
> locks and so this really should be the last patch unless I missed
> something.
Rob, were you able to test this patch ?
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v10 00/19] Introduce memory interconnect for NVIDIA Tegra SoCs
On 23-11-20, 03:27, Dmitry Osipenko wrote: > This series brings initial support for memory interconnect to Tegra20, > Tegra30 and Tegra124 SoCs. > > For the starter only display controllers and devfreq devices are getting > interconnect API support, others could be supported later on. The display > controllers have the biggest demand for interconnect API right now because > dynamic memory frequency scaling can't be done safely without taking into > account bandwidth requirement from the displays. In particular this series > fixes distorted display output on T30 Ouya and T124 TK1 devices. > > Changelog: > > v10 - In a longer run it will be much nicer if we could support EMC > hardware versioning on Tegra20 and it's not late to support it now. > Hence I added these new patches: > > dt-bindings: memory: tegra20: emc: Document opp-supported-hw property > memory: tegra20: Support hardware versioning and clean up OPP table > initialization > > - Removed error message from tegra30-devfreq driver about missing OPP > properties in a device-tree because EMC driver already prints that > message and it uses OPP API error code instead of checking DT directly, > which is a more correct way of doing that. Looks good to me (from OPP APIs usage perspective). Thanks for continuing with this and fixing all the issues Dmitry. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
On 18-11-20, 08:53, Rob Clark wrote:
> On Tue, Nov 17, 2020 at 9:28 PM Viresh Kumar wrote:
> >
> > On 17-11-20, 09:02, Rob Clark wrote:
> > > With that on top of the previous patch,
> >
> > Don't you still have this ? Which fixed the lockdep in the remove path.
> >
> > https://lore.kernel.org/lkml/20201022080644.2ck4okrxygmkuatn@vireshk-i7/
> >
> > To make it clear you need these patches to fix the OPP stuff:
> >
> > //From 5.10-rc3 (the one from the above link).
> > commit e0df59de670b ("opp: Reduce the size of critical section in
> > _opp_table_kref_release()")
This fixes debugfs stuff while the OPP table is removed.
> > //Below two from linux-next
> > commit ef43f01ac069 ("opp: Always add entries in dev_list with
> > opp_table->lock held")
> > commit 27c09484dd3d ("opp: Allocate the OPP table outside of
> > opp_table_lock")
This fixes debugfs stuff while the OPP table is added.
> > This matches the diff I gave you earlier.
> >
>
> no, I did not have all three, only "opp: Allocate the OPP table
> outside of opp_table_lock" plus the fixup. But with all three:
And looking at the lockdep you gave now, it looks like we have a
problem with OPP table's internal lock (opp_table->lock) as well apart
from the global opp_table_lock.
I wish there was a way for me to reproduce the lockdep :(
I know this is exhausting for both of us and I really want to be over
with it as soon as possible, this really should be the last patch
here, please try this along with other two. This fixes the debugfs
thing while the OPPs in the OPP table are removed (they are already
added without a lock around debugfs stuff).
AFAIU, there is no further debugfs stuff that happens from within the
locks and so this really should be the last patch unless I missed
something.
--
viresh
-8<-
From: Viresh Kumar
Date: Thu, 19 Nov 2020 11:24:32 +0530
Subject: [PATCH] opp: Reduce the size of critical section in
_opp_kref_release()
There is a lot of stuff here which can be done outside of the
opp_table->lock, do that. This helps avoiding a circular dependency
lockdeps around debugfs.
Reported-by: Rob Clark
Signed-off-by: Viresh Kumar
---
drivers/opp/core.c | 94 +++---
1 file changed, 47 insertions(+), 47 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9d145bb99a59..4268eb359915 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1251,9 +1251,14 @@ void _opp_free(struct dev_pm_opp *opp)
kfree(opp);
}
-static void _opp_kref_release(struct dev_pm_opp *opp,
- struct opp_table *opp_table)
+static void _opp_kref_release(struct kref *kref)
{
+ struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
+ struct opp_table *opp_table = opp->opp_table;
+
+ list_del(>node);
+ mutex_unlock(_table->lock);
+
/*
* Notify the changes in the availability of the operable
* frequency/voltage list.
@@ -1261,27 +1266,9 @@ static void _opp_kref_release(struct dev_pm_opp *opp,
blocking_notifier_call_chain(_table->head, OPP_EVENT_REMOVE, opp);
_of_opp_free_required_opps(opp_table, opp);
opp_debug_remove_one(opp);
- list_del(>node);
kfree(opp);
}
-static void _opp_kref_release_unlocked(struct kref *kref)
-{
- struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
- struct opp_table *opp_table = opp->opp_table;
-
- _opp_kref_release(opp, opp_table);
-}
-
-static void _opp_kref_release_locked(struct kref *kref)
-{
- struct dev_pm_opp *opp = container_of(kref, struct dev_pm_opp, kref);
- struct opp_table *opp_table = opp->opp_table;
-
- _opp_kref_release(opp, opp_table);
- mutex_unlock(_table->lock);
-}
-
void dev_pm_opp_get(struct dev_pm_opp *opp)
{
kref_get(>kref);
@@ -1289,16 +1276,10 @@ void dev_pm_opp_get(struct dev_pm_opp *opp)
void dev_pm_opp_put(struct dev_pm_opp *opp)
{
- kref_put_mutex(>kref, _opp_kref_release_locked,
- >opp_table->lock);
+ kref_put_mutex(>kref, _opp_kref_release, >opp_table->lock);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_put);
-static void dev_pm_opp_put_unlocked(struct dev_pm_opp *opp)
-{
- kref_put(>kref, _opp_kref_release_unlocked);
-}
-
/**
* dev_pm_opp_remove() - Remove an OPP from OPP table
* @dev: device for which we do this operation
@@ -1342,30 +1323,49 @@ void dev_pm_opp_remove(struct device *dev, unsigned
long freq)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table,
+ bool dynamic)
+{
+ struct dev_p
Re: [PATCH v9 07/17] PM / devfreq: tegra30: Support interconnect and OPPs from device-tree
On 17-11-20, 17:17, Dmitry Osipenko wrote:
> 17.11.2020 13:07, Viresh Kumar пишет:
> > On 16-11-20, 00:29, Dmitry Osipenko wrote:
> >> This patch moves ACTMON driver away from generating OPP table by itself,
> >> transitioning it to use the table which comes from device-tree. This
> >> change breaks compatibility with older device-trees in order to bring
> >> support for the interconnect framework to the driver. This is a mandatory
> >> change which needs to be done in order to implement interconnect-based
> >> memory DVFS. Users of legacy device-trees will get a message telling that
> >> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> >> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
> >>
> >> Tested-by: Peter Geis
> >> Tested-by: Nicolas Chauvet
> >> Acked-by: Chanwoo Choi
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/devfreq/tegra30-devfreq.c | 86 ---
> >> 1 file changed, 44 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/tegra30-devfreq.c
> >> b/drivers/devfreq/tegra30-devfreq.c
> >> index 38cc0d014738..ed6d4469c8c7 100644
> >> --- a/drivers/devfreq/tegra30-devfreq.c
> >> +++ b/drivers/devfreq/tegra30-devfreq.c
> >> @@ -19,6 +19,8 @@
> >> #include
> >> #include
> >>
> >> +#include
> >> +
> >> #include "governor.h"
> >>
> >> #define ACTMON_GLB_STATUS 0x0
> >> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
> >>
> >> struct tegra_devfreq {
> >>struct devfreq *devfreq;
> >> + struct opp_table*opp_table;
> >>
> >>struct reset_control*reset;
> >>struct clk *clock;
> >> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq
> >> *tegra)
> >> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> >>u32 flags)
> >> {
> >> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> >> - struct devfreq *devfreq = tegra->devfreq;
> >>struct dev_pm_opp *opp;
> >> - unsigned long rate;
> >> - int err;
> >> + int ret;
> >>
> >>opp = devfreq_recommended_opp(dev, freq, flags);
> >>if (IS_ERR(opp)) {
> >>dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
> >>return PTR_ERR(opp);
> >>}
> >> - rate = dev_pm_opp_get_freq(opp);
> >> - dev_pm_opp_put(opp);
> >> -
> >> - err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
> >> - if (err)
> >> - return err;
> >> -
> >> - err = clk_set_rate(tegra->emc_clock, 0);
> >> - if (err)
> >> - goto restore_min_rate;
> >>
> >> - return 0;
> >> -
> >> -restore_min_rate:
> >> - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
> >> + ret = dev_pm_opp_set_bw(dev, opp);
> >> + dev_pm_opp_put(opp);
> >>
> >> - return err;
> >> + return ret;
> >> }
> >>
> >> static int tegra_devfreq_get_dev_status(struct device *dev,
> >> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device
> >> *dev,
> >>stat->private_data = tegra;
> >>
> >>/* The below are to be used by the other governors */
> >> - stat->current_frequency = cur_freq;
> >> + stat->current_frequency = cur_freq * KHZ;
> >>
> >>actmon_dev = >devices[MCALL];
> >>
> >> @@ -705,7 +693,12 @@ static int tegra_governor_get_target(struct devfreq
> >> *devfreq,
> >>target_freq = max(target_freq, dev->target_freq);
> >>}
> >>
> >> - *freq = target_freq;
> >> + /*
> >> + * tegra-devfreq driver operates with KHz units, while OPP table
> >> + * entries use Hz units. Hence we need to convert the units for the
> >> + * devfreq core.
> >> + */
> >> + *freq = target_freq * KHZ;
> >>
> >>return 0;
> >> }
> >> @@ -774,6 +767,7 @@ static struct devfreq_governor tegra_devfreq_governor
> >> = {
> >>
> >> static int tegra_devfreq_probe(struct platform_device *pdev)
> >> {
> &
Re: [PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
On 17-11-20, 09:02, Rob Clark wrote:
> With that on top of the previous patch,
Don't you still have this ? Which fixed the lockdep in the remove path.
https://lore.kernel.org/lkml/20201022080644.2ck4okrxygmkuatn@vireshk-i7/
To make it clear you need these patches to fix the OPP stuff:
//From 5.10-rc3 (the one from the above link).
commit e0df59de670b ("opp: Reduce the size of critical section in
_opp_table_kref_release()")
//Below two from linux-next
commit ef43f01ac069 ("opp: Always add entries in dev_list with opp_table->lock
held")
commit 27c09484dd3d ("opp: Allocate the OPP table outside of opp_table_lock")
This matches the diff I gave you earlier.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
On Fri, 6 Nov 2020 at 12:46, Viresh Kumar wrote:
>
> On 05-11-20, 11:24, Rob Clark wrote:
> > On Tue, Nov 3, 2020 at 7:04 PM Viresh Kumar wrote:
> > >
> > > On 03-11-20, 08:50, Rob Clark wrote:
> > > > sorry, it didn't apply cleanly (which I guess is due to some other
> > > > dependencies that need to be picked back to v5.4 product kernel), and
> > > > due to some other things I'm in middle of debugging I didn't have time
> > > > yet to switch to v5.10-rc or look at what else needs to
> > > > cherry-picked..
> > > >
> > > > If you could, pushing a branch with this patch somewhere would be a
> > > > bit easier to work with (ie. fetch && cherry-pick is easier to deal
> > > > with than picking things from list)
> > >
> > > It has been in linux-next for a few days. Here is the HEAD to pick
> > > from. There are few patches there since rc1.
> > >
> > > commit 203e29749cc0 ("opp: Allocate the OPP table outside of
> > > opp_table_lock")
> > >
> >
> > sorry for the delay, with that cherry-picked, I'm getting a whole lot of:
>
> Ahh, sorry about that and thanks for reporting it. Here is the fix:
>
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index c718092757d9..6b7f0066942d 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -112,8 +112,6 @@ static struct opp_table *_find_table_of_opp_np(struct
> device_node *opp_np)
> struct opp_table *opp_table;
> struct device_node *opp_table_np;
>
> - lockdep_assert_held(_table_lock);
> -
> opp_table_np = of_get_parent(opp_np);
> if (!opp_table_np)
> goto err;
> @@ -121,12 +119,15 @@ static struct opp_table *_find_table_of_opp_np(struct
> device_node *opp_np)
> /* It is safe to put the node now as all we need now is its address */
> of_node_put(opp_table_np);
>
> + mutex_lock(_table_lock);
> list_for_each_entry(opp_table, _tables, node) {
> if (opp_table_np == opp_table->np) {
> _get_opp_table_kref(opp_table);
> + mutex_unlock(_table_lock);
> return opp_table;
> }
> }
> + mutex_unlock(_table_lock);
>
> err:
> return ERR_PTR(-ENODEV);
Ping.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 07/17] PM / devfreq: tegra30: Support interconnect and OPPs from device-tree
On 16-11-20, 00:29, Dmitry Osipenko wrote:
> This patch moves ACTMON driver away from generating OPP table by itself,
> transitioning it to use the table which comes from device-tree. This
> change breaks compatibility with older device-trees in order to bring
> support for the interconnect framework to the driver. This is a mandatory
> change which needs to be done in order to implement interconnect-based
> memory DVFS. Users of legacy device-trees will get a message telling that
> theirs DT needs to be upgraded. Now ACTMON issues memory bandwidth request
> using dev_pm_opp_set_bw(), instead of driving EMC clock rate directly.
>
> Tested-by: Peter Geis
> Tested-by: Nicolas Chauvet
> Acked-by: Chanwoo Choi
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/devfreq/tegra30-devfreq.c | 86 ---
> 1 file changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/devfreq/tegra30-devfreq.c
> b/drivers/devfreq/tegra30-devfreq.c
> index 38cc0d014738..ed6d4469c8c7 100644
> --- a/drivers/devfreq/tegra30-devfreq.c
> +++ b/drivers/devfreq/tegra30-devfreq.c
> @@ -19,6 +19,8 @@
> #include
> #include
>
> +#include
> +
> #include "governor.h"
>
> #define ACTMON_GLB_STATUS0x0
> @@ -155,6 +157,7 @@ struct tegra_devfreq_device {
>
> struct tegra_devfreq {
> struct devfreq *devfreq;
> + struct opp_table*opp_table;
>
> struct reset_control*reset;
> struct clk *clock;
> @@ -612,34 +615,19 @@ static void tegra_actmon_stop(struct tegra_devfreq
> *tegra)
> static int tegra_devfreq_target(struct device *dev, unsigned long *freq,
> u32 flags)
> {
> - struct tegra_devfreq *tegra = dev_get_drvdata(dev);
> - struct devfreq *devfreq = tegra->devfreq;
> struct dev_pm_opp *opp;
> - unsigned long rate;
> - int err;
> + int ret;
>
> opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(opp)) {
> dev_err(dev, "Failed to find opp for %lu Hz\n", *freq);
> return PTR_ERR(opp);
> }
> - rate = dev_pm_opp_get_freq(opp);
> - dev_pm_opp_put(opp);
> -
> - err = clk_set_min_rate(tegra->emc_clock, rate * KHZ);
> - if (err)
> - return err;
> -
> - err = clk_set_rate(tegra->emc_clock, 0);
> - if (err)
> - goto restore_min_rate;
>
> - return 0;
> -
> -restore_min_rate:
> - clk_set_min_rate(tegra->emc_clock, devfreq->previous_freq);
> + ret = dev_pm_opp_set_bw(dev, opp);
> + dev_pm_opp_put(opp);
>
> - return err;
> + return ret;
> }
>
> static int tegra_devfreq_get_dev_status(struct device *dev,
> @@ -655,7 +643,7 @@ static int tegra_devfreq_get_dev_status(struct device
> *dev,
> stat->private_data = tegra;
>
> /* The below are to be used by the other governors */
> - stat->current_frequency = cur_freq;
> + stat->current_frequency = cur_freq * KHZ;
>
> actmon_dev = >devices[MCALL];
>
> @@ -705,7 +693,12 @@ static int tegra_governor_get_target(struct devfreq
> *devfreq,
> target_freq = max(target_freq, dev->target_freq);
> }
>
> - *freq = target_freq;
> + /*
> + * tegra-devfreq driver operates with KHz units, while OPP table
> + * entries use Hz units. Hence we need to convert the units for the
> + * devfreq core.
> + */
> + *freq = target_freq * KHZ;
>
> return 0;
> }
> @@ -774,6 +767,7 @@ static struct devfreq_governor tegra_devfreq_governor = {
>
> static int tegra_devfreq_probe(struct platform_device *pdev)
> {
> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> struct tegra_devfreq_device *dev;
> struct tegra_devfreq *tegra;
> struct devfreq *devfreq;
> @@ -781,6 +775,13 @@ static int tegra_devfreq_probe(struct platform_device
> *pdev)
> long rate;
> int err;
>
> + /* legacy device-trees don't have OPP table and must be updated */
> + if (!device_property_present(>dev, "operating-points-v2")) {
> + dev_err(>dev,
> + "OPP table not found, please update your device
> tree\n");
> + return -ENODEV;
> + }
> +
You forgot to remove this ?
> tegra = devm_kzalloc(>dev, sizeof(*tegra), GFP_KERNEL);
> if (!tegra)
> return -ENOMEM;
> @@ -822,11 +823,25 @@ static int tegra_devfreq_probe(struct platform_device
> *pdev)
> return err;
> }
>
> + tegra->opp_table = dev_pm_opp_set_supported_hw(>dev,
> +_version, 1);
> + err = PTR_ERR_OR_ZERO(tegra->opp_table);
> + if (err) {
> + dev_err(>dev, "Failed to set supported HW: %d\n", err);
> + return err;
> + }
> +
> + err = dev_pm_opp_of_add_table(>dev);
> + if (err) {
> + dev_err(>dev, "Failed to add OPP table: %d\n", err);
> +
Re: [PATCH v8 09/26] memory: tegra30: Support interconnect framework
On 11-11-20, 10:32, Dmitry Osipenko wrote: > 11.11.2020 09:18, Viresh Kumar пишет: > > On 11-11-20, 09:14, Dmitry Osipenko wrote: > >> The dev_pm_opp_of_add_table() will produce a error message which doesn't > >> give a clue about what's wrong, i.e. that device-tree needs to be updated. > > > > If you think that you need to print something more, then you can do > > that in the error message you print when dev_pm_opp_of_add_table() > > fails. I would suggest to drop this redundant check here. > > > > Please give the rationale. The rationale is that the check is already performed by dev_pm_opp_of_add_table() and it isn't going to add *any* benefit to check it again here. Such a check for matching compatible platforms is normally fine, but not for this. This is like open coding part of dev_pm_opp_of_add_table(), and so is redundant. The dev_pm_opp_of_add_table() helper also checks for OPPv1 bindings in the DT (yes you won't be using them on your platform) and so relying on that API is a better thing to do. As you already said, you just wanted a better print message and so you have added this check. If you really care only about the print message, then you can add a print of your choice in the driver but otherwise this check is not going to benefit you much I am afraid. Having said that, this isn't the code I maintain. I need to guarantee that the OPP core APIs are used properly and are not misused and so I have a higher say there. But in this case all I can do is _suggest_ and not enforce. And as I said earlier, I suggest to drop this redundant check in order to make your code better and faster. Thanks. -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 09/26] memory: tegra30: Support interconnect framework
On 11-11-20, 09:14, Dmitry Osipenko wrote:
> 11.11.2020 08:53, Viresh Kumar пишет:
> >> +static int tegra_emc_opp_table_init(struct tegra_emc *emc)
> >> +{
> >> + struct opp_table *reg_opp_table = NULL, *clk_opp_table, *hw_opp_table;
> >> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> >> + const char *rname = "core";
> >> + int err;
> >> +
> >> + /*
> >> + * Legacy device-trees don't have OPP table and EMC driver isn't
> >> + * useful in this case.
> >> + */
> >> + if (!device_property_present(emc->dev, "operating-points-v2")) {
> > I don't understand why you want to check this ? The below call to
> > dev_pm_opp_of_add_table() will fail anyway and that should be good for
> > you.
> >
>
> The dev_pm_opp_of_add_table() will produce a error message which doesn't
> give a clue about what's wrong, i.e. that device-tree needs to be updated.
If you think that you need to print something more, then you can do
that in the error message you print when dev_pm_opp_of_add_table()
fails. I would suggest to drop this redundant check here.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 09/26] memory: tegra30: Support interconnect framework
On 11-11-20, 04:14, Dmitry Osipenko wrote:
> +static int tegra_emc_opp_table_init(struct tegra_emc *emc)
> +{
> + struct opp_table *reg_opp_table = NULL, *clk_opp_table, *hw_opp_table;
> + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> + const char *rname = "core";
> + int err;
> +
> + /*
> + * Legacy device-trees don't have OPP table and EMC driver isn't
> + * useful in this case.
> + */
> + if (!device_property_present(emc->dev, "operating-points-v2")) {
I don't understand why you want to check this ? The below call to
dev_pm_opp_of_add_table() will fail anyway and that should be good for
you.
> + dev_err(emc->dev,
> + "OPP table not found, please update your device
> tree\n");
> + return -ENODEV;
> + }
> +
> + /* voltage scaling is optional */
> + if (device_property_present(emc->dev, "core-supply")) {
> + reg_opp_table = dev_pm_opp_set_regulators(emc->dev, , 1);
> + if (IS_ERR(reg_opp_table))
> + return dev_err_probe(emc->dev, PTR_ERR(reg_opp_table),
> + "failed to set OPP regulator\n");
> + }
> +
> + clk_opp_table = dev_pm_opp_set_clkname(emc->dev, NULL);
I think there is still some misunderstanding here. This call with a
NULL connection id is useful only if the DT OPP table is optional for
your platform and you want the same driver to work with and without
the DT OPP table. Clearly in your case you want the OPP table in the
DT to be there and so this call is not required at all.
> + err = PTR_ERR_OR_ZERO(clk_opp_table);
> + if (err) {
> + dev_err(emc->dev, "failed to set OPP clk: %d\n", err);
> + goto put_reg_table;
> + }
> +
> + hw_opp_table = dev_pm_opp_set_supported_hw(emc->dev, _version, 1);
> + err = PTR_ERR_OR_ZERO(hw_opp_table);
> + if (err) {
> + dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
> + goto put_clk_table;
> + }
> +
> + err = dev_pm_opp_of_add_table(emc->dev);
> + if (err) {
> + dev_err(emc->dev, "failed to add OPP table: %d\n", err);
> + goto put_hw_table;
> + }
> +
> + dev_info(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
> + hw_version, clk_get_rate(emc->clk) / 100);
> +
> + /* first dummy rate-set initializes voltage state */
> + err = dev_pm_opp_set_rate(emc->dev, clk_get_rate(emc->clk));
> + if (err) {
> + dev_err(emc->dev, "failed to initialize OPP clock: %d\n", err);
> + goto remove_table;
> + }
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(emc->dev);
> +put_hw_table:
> + dev_pm_opp_put_supported_hw(hw_opp_table);
> +put_clk_table:
> + dev_pm_opp_put_clkname(clk_opp_table);
> +put_reg_table:
> + if (reg_opp_table)
> + dev_pm_opp_put_regulators(reg_opp_table);
> +
> + return err;
> +}
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 02/26] memory: tegra20-emc: Use dev_pm_opp_set_clkname()
On 11-11-20, 04:14, Dmitry Osipenko wrote:
> The dev_pm_opp_get_opp_table() shouldn't be used by drivers, use
> dev_pm_opp_set_clkname() instead.
>
> Suggested-by: Viresh Kumar
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/memory/tegra/tegra20-emc.c | 30 +++---
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/memory/tegra/tegra20-emc.c
> b/drivers/memory/tegra/tegra20-emc.c
> index 5e10aa97809f..bb3f315c9587 100644
> --- a/drivers/memory/tegra/tegra20-emc.c
> +++ b/drivers/memory/tegra/tegra20-emc.c
> @@ -902,7 +902,7 @@ static int tegra_emc_interconnect_init(struct tegra_emc
> *emc)
>
> static int tegra_emc_opp_table_init(struct tegra_emc *emc)
> {
> - struct opp_table *opp_table;
> + struct opp_table *reg_opp_table = NULL, *clk_opp_table;
> const char *rname = "core";
> int err;
>
> @@ -917,19 +917,24 @@ static int tegra_emc_opp_table_init(struct tegra_emc
> *emc)
> }
>
> /* voltage scaling is optional */
> - if (device_property_present(emc->dev, "core-supply"))
> - opp_table = dev_pm_opp_set_regulators(emc->dev, , 1);
> - else
> - opp_table = dev_pm_opp_get_opp_table(emc->dev);
> + if (device_property_present(emc->dev, "core-supply")) {
> + reg_opp_table = dev_pm_opp_set_regulators(emc->dev, , 1);
> + if (IS_ERR(reg_opp_table))
> + return dev_err_probe(emc->dev, PTR_ERR(reg_opp_table),
> + "failed to set OPP regulator\n");
> + }
>
> - if (IS_ERR(opp_table))
> - return dev_err_probe(emc->dev, PTR_ERR(opp_table),
> - "failed to prepare OPP table\n");
> + clk_opp_table = dev_pm_opp_set_clkname(emc->dev, NULL);
> + err = PTR_ERR_OR_ZERO(clk_opp_table);
Don't check for NULL here.
> + if (err) {
> + dev_err(emc->dev, "failed to set OPP clk: %d\n", err);
> + goto put_reg_table;
> + }
>
> err = dev_pm_opp_of_add_table(emc->dev);
> if (err) {
> dev_err(emc->dev, "failed to add OPP table: %d\n", err);
> - goto put_table;
> + goto put_clk_table;
> }
>
> dev_info(emc->dev, "current clock rate %lu MHz\n",
> @@ -946,8 +951,11 @@ static int tegra_emc_opp_table_init(struct tegra_emc
> *emc)
>
> remove_table:
> dev_pm_opp_of_remove_table(emc->dev);
> -put_table:
> - dev_pm_opp_put_regulators(opp_table);
> +put_clk_table:
> + dev_pm_opp_put_clkname(clk_opp_table);
> +put_reg_table:
> + if (reg_opp_table)
This won't be required after my other patchset and yeah it is a
classic chicken and egg problem we have here :)
Maybe you can fix them separately in 5.11 after everything is applied.
> + dev_pm_opp_put_regulators(reg_opp_table);
>
> return err;
> }
> --
> 2.29.2
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v8 02/26] memory: tegra20-emc: Use dev_pm_opp_set_clkname()
On 11-11-20, 11:15, Viresh Kumar wrote:
> On 11-11-20, 04:14, Dmitry Osipenko wrote:
> > The dev_pm_opp_get_opp_table() shouldn't be used by drivers, use
> > dev_pm_opp_set_clkname() instead.
> >
> > Suggested-by: Viresh Kumar
> > Signed-off-by: Dmitry Osipenko
> > ---
> > drivers/memory/tegra/tegra20-emc.c | 30 +++---
> > 1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/memory/tegra/tegra20-emc.c
> > b/drivers/memory/tegra/tegra20-emc.c
> > index 5e10aa97809f..bb3f315c9587 100644
> > --- a/drivers/memory/tegra/tegra20-emc.c
> > +++ b/drivers/memory/tegra/tegra20-emc.c
> > @@ -902,7 +902,7 @@ static int tegra_emc_interconnect_init(struct tegra_emc
> > *emc)
> >
> > static int tegra_emc_opp_table_init(struct tegra_emc *emc)
> > {
> > - struct opp_table *opp_table;
> > + struct opp_table *reg_opp_table = NULL, *clk_opp_table;
> > const char *rname = "core";
> > int err;
> >
> > @@ -917,19 +917,24 @@ static int tegra_emc_opp_table_init(struct tegra_emc
> > *emc)
> > }
> >
> > /* voltage scaling is optional */
> > - if (device_property_present(emc->dev, "core-supply"))
> > - opp_table = dev_pm_opp_set_regulators(emc->dev, , 1);
> > - else
> > - opp_table = dev_pm_opp_get_opp_table(emc->dev);
> > + if (device_property_present(emc->dev, "core-supply")) {
> > + reg_opp_table = dev_pm_opp_set_regulators(emc->dev, , 1);
> > + if (IS_ERR(reg_opp_table))
> > + return dev_err_probe(emc->dev, PTR_ERR(reg_opp_table),
> > +"failed to set OPP regulator\n");
> > + }
> >
> > - if (IS_ERR(opp_table))
> > - return dev_err_probe(emc->dev, PTR_ERR(opp_table),
> > -"failed to prepare OPP table\n");
> > + clk_opp_table = dev_pm_opp_set_clkname(emc->dev, NULL);
> > + err = PTR_ERR_OR_ZERO(clk_opp_table);
>
> Don't check for NULL here.
My bad. You aren't checking but just converting to err. Its fine.
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On 08-11-20, 15:19, Dmitry Osipenko wrote: > I took a detailed look at the GENPD and tried to implement it. Here is > what was found: > > 1. GENPD framework doesn't aggregate performance requests from the > attached devices. This means that if deviceA requests performance state > 10 and then deviceB requests state 3, then framework will set domain's > state to 3 instead of 10. It does. Look at _genpd_reeval_performance_state(). -- viresh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling
On 09-11-20, 08:19, Dmitry Osipenko wrote:
> Thanks, I made it in a different way by simply adding helpers to the
> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> add new kernel symbols.
I will prefer to add it in core.c itself, and yes
devm_add_action_or_reset() looks better. But I am still not sure for
which helpers do we need the devm_*() variants, as this is only useful
for non-CPU devices. But if we have users that we can add right now,
why not.
> static inline int devm_pm_opp_of_add_table(struct device *dev)
> {
> int err;
>
> err = dev_pm_opp_of_add_table(dev);
> if (err)
> return err;
>
> err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
> dev);
> if (err)
> return err;
>
> return 0;
> }
--
viresh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
