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
Track if the clock name is configured by the platform or not. This is a
preparatory change and will be used by later commits. This also makes
the behavior of the clkname API similar to other ones, which allow
repeated calls to the same API for each CPU.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 7 +++++++
drivers/opp/opp.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index ff0364733dcb..254782b3a6a0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2277,6 +2277,10 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
goto err;
}
+ /* Another CPU that shares the OPP table has set the clkname ? */
+ if (opp_table->clk_configured)
+ return opp_table;
+
/* clk shouldn't be initialized at this point */
if (WARN_ON(opp_table->clk)) {
ret = -EBUSY;
@@ -2291,6 +2295,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
goto err;
}
+ opp_table->clk_configured = true;
+
return opp_table;
err:
@@ -2311,6 +2317,7 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table)
clk_put(opp_table->clk);
opp_table->clk = ERR_PTR(-EINVAL);
+ opp_table->clk_configured = false;
dev_pm_opp_put_opp_table(opp_table);
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 45e3a55239a1..9e1cfcb0ea98 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -149,6 +149,7 @@ enum opp_table_access {
* @supported_hw: Array of version number to support.
* @supported_hw_count: Number of elements in supported_hw array.
* @prop_name: A name to postfix to many DT properties, while parsing them.
+ * @clk_configured: Clock name is configured by the platform.
* @clk: Device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators. Its value can be -1
@@ -200,6 +201,7 @@ struct opp_table {
unsigned int *supported_hw;
unsigned int supported_hw_count;
const char *prop_name;
+ bool clk_configured;
struct clk *clk;
struct regulator **regulators;
int regulator_count;
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/soc/tegra/common.c | 8 ++++++--
drivers/soc/tegra/pmc.c | 8 ++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index 49a5360f4507..7ba15cb836e8 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -107,6 +107,10 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
{
u32 hw_version;
int err;
+ struct dev_pm_opp_config config = {
+ .supported_hw = &hw_version,
+ .supported_hw_count = 1,
+ };
/* Tegra114+ doesn't support OPP yet */
if (!of_machine_is_compatible("nvidia,tegra20") &&
@@ -118,9 +122,9 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
else
hw_version = BIT(tegra_sku_info.soc_speedo_id);
- err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
+ err = devm_pm_opp_set_config(dev, &config);
if (err) {
- dev_err(dev, "failed to set OPP supported HW: %d\n", err);
+ dev_err(dev, "failed to set OPP config: %d\n", err);
return err;
}
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index fdf508e03400..01ec76dd433d 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -1366,6 +1366,10 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
struct generic_pm_domain *genpd;
const char *rname = "core";
int err;
+ struct dev_pm_opp_config config = {
+ .regulator_names = &rname,
+ .regulator_count = 1,
+ };
genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
if (!genpd)
@@ -1375,10 +1379,10 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
- err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
+ err = devm_pm_opp_set_config(pmc->dev, &config);
if (err)
return dev_err_probe(pmc->dev, err,
- "failed to set core OPP regulator\n");
+ "failed to set OPP config\n");
err = pm_genpd_init(genpd, NULL, false);
if (err) {
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/spi/spi-geni-qcom.c | 5 ++++-
drivers/spi/spi-qcom-qspi.c | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 4e83cc5b445d..d869f270dcca 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -892,6 +892,9 @@ static int spi_geni_probe(struct platform_device *pdev)
void __iomem *base;
struct clk *clk;
struct device *dev = &pdev->dev;
+ struct dev_pm_opp_config config = {
+ .clk_name = "se",
+ };
irq = platform_get_irq(pdev, 0);
if (irq < 0)
@@ -922,7 +925,7 @@ static int spi_geni_probe(struct platform_device *pdev)
mas->se.base = base;
mas->se.clk = clk;
- ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
+ ret = devm_pm_opp_set_config(&pdev->dev, &config);
if (ret)
return ret;
/* OPP table is optional */
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index c334dfec4117..5ab3ae406ef7 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -458,6 +458,9 @@ static int qcom_qspi_probe(struct platform_device *pdev)
struct device *dev;
struct spi_master *master;
struct qcom_qspi *ctrl;
+ struct dev_pm_opp_config config = {
+ .clk_name = "core",
+ };
dev = &pdev->dev;
@@ -529,7 +532,7 @@ static int qcom_qspi_probe(struct platform_device *pdev)
master->handle_err = qcom_qspi_handle_err;
master->auto_runtime_pm = true;
- ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
+ ret = devm_pm_opp_set_config(&pdev->dev, &config);
if (ret)
return ret;
/* OPP table is optional */
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1543a6028856..391fcc3a0f61 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1331,6 +1331,9 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
int irq;
bool console = false;
struct uart_driver *drv;
+ struct dev_pm_opp_config config = {
+ .clk_name = "se",
+ };
if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
console = true;
@@ -1414,7 +1417,7 @@ 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;
- ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
+ ret = devm_pm_opp_set_config(&pdev->dev, &config);
if (ret)
return ret;
/* OPP table is optional */
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/memory/tegra/tegra124-emc.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
index 908f8d5392b2..d1e8f9ffef63 100644
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -1395,13 +1395,17 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
static int tegra_emc_opp_table_init(struct tegra_emc *emc)
{
u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
- struct opp_table *hw_opp_table;
+ struct opp_table *opp_table;
int err;
+ struct dev_pm_opp_config config = {
+ .supported_hw = &hw_version,
+ .supported_hw_count = 1,
+ };
- hw_opp_table = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
- err = PTR_ERR_OR_ZERO(hw_opp_table);
+ opp_table = dev_pm_opp_set_config(emc->dev, &config);
+ err = PTR_ERR_OR_ZERO(opp_table);
if (err) {
- dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
+ dev_err(emc->dev, "failed to set OPP config: %d\n", err);
return err;
}
@@ -1430,7 +1434,7 @@ static int tegra_emc_opp_table_init(struct tegra_emc *emc)
remove_table:
dev_pm_opp_of_remove_table(emc->dev);
put_hw_table:
- dev_pm_opp_put_supported_hw(hw_opp_table);
+ dev_pm_opp_clear_config(opp_table);
return err;
}
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/devfreq/sun8i-a33-mbus.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/sun8i-a33-mbus.c b/drivers/devfreq/sun8i-a33-mbus.c
index 13d32213139f..125b479c9d6d 100644
--- a/drivers/devfreq/sun8i-a33-mbus.c
+++ b/drivers/devfreq/sun8i-a33-mbus.c
@@ -337,6 +337,9 @@ static int sun8i_a33_mbus_probe(struct platform_device *pdev)
unsigned int max_state;
const char *err;
int i, ret;
+ struct dev_pm_opp_config config = {
+ .clk_name = "dram",
+ };
variant = device_get_match_data(dev);
if (!variant)
@@ -404,9 +407,9 @@ static int sun8i_a33_mbus_probe(struct platform_device *pdev)
priv->profile.freq_table = priv->freq_table;
priv->profile.max_state = max_state;
- ret = devm_pm_opp_set_clkname(dev, "dram");
+ ret = devm_pm_opp_set_config(dev, &config);
if (ret) {
- err = "failed to add OPP table\n";
+ err = "failed to set OPP config\n";
goto err_unlock_mbus;
}
--
2.31.1.272.g89b43f80a514
On Thu, May 26, 2022 at 05:12:22PM +0530, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
>
> Lets start using it.
Acked-by: Mark Brown <[email protected]>
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 <[email protected]>
---
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 = &opp_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, &opp_virt_devs);
+ err = devm_pm_opp_set_config(dev, &config);
if (err)
return err;
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/cpufreq/tegra20-cpufreq.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index e8db3d75be25..2c73623e3abb 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -34,7 +34,7 @@ static bool cpu0_node_has_opp_v2_prop(void)
static void tegra20_cpufreq_put_supported_hw(void *opp_table)
{
- dev_pm_opp_put_supported_hw(opp_table);
+ dev_pm_opp_clear_config(opp_table);
}
static void tegra20_cpufreq_dt_unregister(void *cpufreq_dt)
@@ -49,6 +49,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
struct device *cpu_dev;
u32 versions[2];
int err;
+ struct dev_pm_opp_config config = {
+ .supported_hw = versions,
+ .supported_hw_count = ARRAY_SIZE(versions),
+ };
if (!cpu0_node_has_opp_v2_prop()) {
dev_err(&pdev->dev, "operating points not found\n");
@@ -71,10 +75,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
if (WARN_ON(!cpu_dev))
return -ENODEV;
- opp_table = dev_pm_opp_set_supported_hw(cpu_dev, versions, 2);
- err = PTR_ERR_OR_ZERO(opp_table);
+ opp_table = dev_pm_opp_set_config(cpu_dev, &config);
+ err = PTR_ERR(opp_table);
if (err) {
- dev_err(&pdev->dev, "failed to set supported hw: %d\n", err);
+ dev_err(&pdev->dev, "failed to set OPP config: %d\n", err);
return err;
}
--
2.31.1.272.g89b43f80a514
The OPP core already have few configuration specific APIs and it is
getting complex or messy for both the OPP core and its users.
Lets introduce a new set of API which will be used for all kind of
different configurations, and shall eventually replace all the existing
ones.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 145 +++++++++++++++++++++++++++++++++++++++++
include/linux/pm_opp.h | 42 ++++++++++++
2 files changed, 187 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 254782b3a6a0..30dbef0f4d17 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2618,6 +2618,151 @@ int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
}
EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
+/**
+ * dev_pm_opp_set_config() - Set OPP configuration for the device.
+ * @dev: Device for which configuration is being set.
+ * @config: OPP configuration.
+ *
+ * This allows all device OPP configurations to be performed at once.
+ *
+ * This must be called before any OPPs are initialized for the device. This may
+ * be called multiple times for the same OPP table, for example once for each
+ * CPU that share the same table. This must be balanced by the same number of
+ * calls to dev_pm_opp_clear_config() in order to free the OPP table properly.
+ */
+struct opp_table *dev_pm_opp_set_config(struct device *dev,
+ struct dev_pm_opp_config *config)
+{
+ struct opp_table *opp_table, *ret;
+
+ opp_table = _add_opp_table(dev, false);
+ if (IS_ERR(opp_table))
+ return opp_table;
+
+ /* This should be called before OPPs are initialized */
+ if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+ ret = ERR_PTR(-EBUSY);
+ goto err;
+ }
+
+ // Configure clock
+ if (config->clk_name) {
+ ret = dev_pm_opp_set_clkname(dev, config->clk_name);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ // Configure property names
+ if (config->prop_name) {
+ ret = dev_pm_opp_set_prop_name(dev, config->prop_name);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ // Configure opp helper
+ if (config->set_opp) {
+ ret = dev_pm_opp_register_set_opp_helper(dev, config->set_opp);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ // Configure supported hardware
+ if (config->supported_hw) {
+ ret = dev_pm_opp_set_supported_hw(dev, config->supported_hw,
+ config->supported_hw_count);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ // Configure supplies
+ if (config->regulator_names) {
+ ret = dev_pm_opp_set_regulators(dev, config->regulator_names,
+ config->regulator_count);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ // Attach genpds
+ if (config->genpd_names) {
+ ret = dev_pm_opp_attach_genpd(dev, config->genpd_names,
+ config->virt_devs);
+ if (IS_ERR(ret))
+ goto err;
+ }
+
+ return opp_table;
+
+err:
+ dev_pm_opp_clear_config(opp_table);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
+
+/**
+ * dev_pm_opp_clear_config() - Releases resources blocked for OPP configuration.
+ * @opp_table: OPP table returned from dev_pm_opp_set_config().
+ *
+ * This allows all device OPP configurations to be cleared at once. This must be
+ * called once for each call made to dev_pm_opp_set_config(), in order to free
+ * the OPPs properly.
+ *
+ * Currently the first call itself ends up freeing all the OPP configurations,
+ * while the later ones only drop the OPP table reference. This works well for
+ * now as we would never want to use an half initialized OPP table and want to
+ * remove the configurations together.
+ */
+void dev_pm_opp_clear_config(struct opp_table *opp_table)
+{
+ if (opp_table->genpd_virt_devs)
+ dev_pm_opp_detach_genpd(opp_table);
+
+ if (opp_table->regulators)
+ dev_pm_opp_put_regulators(opp_table);
+
+ if (opp_table->supported_hw)
+ dev_pm_opp_put_supported_hw(opp_table);
+
+ if (opp_table->set_opp)
+ dev_pm_opp_unregister_set_opp_helper(opp_table);
+
+ if (opp_table->prop_name)
+ dev_pm_opp_put_prop_name(opp_table);
+
+ if (opp_table->clk_configured)
+ dev_pm_opp_put_clkname(opp_table);
+
+ dev_pm_opp_put_opp_table(opp_table);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_clear_config);
+
+static void devm_pm_opp_config_release(void *data)
+{
+ dev_pm_opp_clear_config(data);
+}
+
+/**
+ * devm_pm_opp_set_config() - Set OPP configuration for the device.
+ * @dev: Device for which configuration is being set.
+ * @config: OPP configuration.
+ *
+ * This allows all device OPP configurations to be performed at once.
+ * This is a resource-managed variant of dev_pm_opp_set_config().
+ *
+ * Return: 0 on success and errorno otherwise.
+ */
+int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+ struct opp_table *opp_table;
+
+ opp_table = dev_pm_opp_set_config(dev, config);
+ if (IS_ERR(opp_table))
+ return PTR_ERR(opp_table);
+
+ return devm_add_action_or_reset(dev, devm_pm_opp_config_release,
+ opp_table);
+}
+EXPORT_SYMBOL_GPL(devm_pm_opp_set_config);
+
/**
* dev_pm_opp_xlate_required_opp() - Find required OPP for @src_table OPP.
* @src_table: OPP table which has @dst_table as one of its required OPP table.
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6708b4ec244d..0d5d07dd164a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -90,6 +90,32 @@ struct dev_pm_set_opp_data {
struct device *dev;
};
+/**
+ * struct dev_pm_opp_config - Device OPP configuration values
+ * @clk_name: Clk name.
+ * @prop_name: Name to postfix to properties.
+ * @set_opp: Custom set OPP helper.
+ * @supported_hw: Array of hierarchy of versions to match.
+ * @supported_hw_count: Number of elements in the array.
+ * @regulator_names: Array of pointers to the names of the regulator.
+ * @regulator_count: Number of regulators.
+ * @genpd_names: Null terminated array of pointers containing names of genpd to attach.
+ * @virt_devs: Pointer to return the array of virtual devices.
+ *
+ * This structure contains platform specific OPP configurations for the device.
+ */
+struct dev_pm_opp_config {
+ const char *clk_name;
+ const char *prop_name;
+ int (*set_opp)(struct dev_pm_set_opp_data *data);
+ unsigned int *supported_hw;
+ unsigned int supported_hw_count;
+ const char * const *regulator_names;
+ unsigned int regulator_count;
+ const char * const *genpd_names;
+ struct device ***virt_devs;
+};
+
#if defined(CONFIG_PM_OPP)
struct opp_table *dev_pm_opp_get_opp_table(struct device *dev);
@@ -154,6 +180,10 @@ int dev_pm_opp_disable(struct device *dev, unsigned long freq);
int dev_pm_opp_register_notifier(struct device *dev, struct notifier_block *nb);
int dev_pm_opp_unregister_notifier(struct device *dev, struct notifier_block *nb);
+struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
+int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
+void dev_pm_opp_clear_config(struct opp_table *opp_table);
+
struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
void dev_pm_opp_put_supported_hw(struct opp_table *opp_table);
int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
@@ -419,6 +449,18 @@ static inline int devm_pm_opp_attach_genpd(struct device *dev,
return -EOPNOTSUPP;
}
+static inline struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void dev_pm_opp_clear_config(struct opp_table *opp_table) {}
+
static inline 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)
{
--
2.31.1.272.g89b43f80a514
Now that everyone has migrated to dev_pm_opp_set_config(), remove the
public interface for dev_pm_opp_set_supported_hw() and friends.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 88 +++++++++++-------------------------------
include/linux/pm_opp.h | 19 ---------
2 files changed, 22 insertions(+), 85 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 9297b5e944f7..07cb8ff33a6d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1948,7 +1948,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
}
/**
- * dev_pm_opp_set_supported_hw() - Set supported platforms
+ * _opp_set_supported_hw() - Set supported platforms
* @dev: Device for which supported-hw has to be set.
* @versions: Array of hierarchy of versions to match.
* @count: Number of elements in the array.
@@ -1958,84 +1958,39 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
* OPPs, which are available for those versions, based on its 'opp-supported-hw'
* property.
*/
-struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
- const u32 *versions, unsigned int count)
+static int _opp_set_supported_hw(struct opp_table *opp_table,
+ const u32 *versions, unsigned int count)
{
- struct opp_table *opp_table;
-
- opp_table = _add_opp_table(dev, false);
- if (IS_ERR(opp_table))
- return opp_table;
-
- /* Make sure there are no concurrent readers while updating opp_table */
- WARN_ON(!list_empty(&opp_table->opp_list));
-
/* Another CPU that shares the OPP table has set the property ? */
if (opp_table->supported_hw)
- return opp_table;
+ return 0;
opp_table->supported_hw = kmemdup(versions, count * sizeof(*versions),
GFP_KERNEL);
- if (!opp_table->supported_hw) {
- dev_pm_opp_put_opp_table(opp_table);
- return ERR_PTR(-ENOMEM);
- }
+ if (!opp_table->supported_hw)
+ return -ENOMEM;
opp_table->supported_hw_count = count;
- return opp_table;
+ return 0;
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_supported_hw);
/**
- * dev_pm_opp_put_supported_hw() - Releases resources blocked for supported hw
- * @opp_table: OPP table returned by dev_pm_opp_set_supported_hw().
+ * _opp_put_supported_hw() - Releases resources blocked for supported hw
+ * @opp_table: OPP table returned by _opp_set_supported_hw().
*
* This is required only for the V2 bindings, and is called for a matching
- * dev_pm_opp_set_supported_hw(). Until this is called, the opp_table structure
+ * _opp_set_supported_hw(). Until this is called, the opp_table structure
* will not be freed.
*/
-void dev_pm_opp_put_supported_hw(struct opp_table *opp_table)
-{
- if (unlikely(!opp_table))
- return;
-
- kfree(opp_table->supported_hw);
- opp_table->supported_hw = NULL;
- opp_table->supported_hw_count = 0;
-
- dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_supported_hw);
-
-static void devm_pm_opp_supported_hw_release(void *data)
-{
- dev_pm_opp_put_supported_hw(data);
-}
-
-/**
- * devm_pm_opp_set_supported_hw() - Set supported platforms
- * @dev: Device for which supported-hw has to be set.
- * @versions: Array of hierarchy of versions to match.
- * @count: Number of elements in the array.
- *
- * This is a resource-managed variant of dev_pm_opp_set_supported_hw().
- *
- * Return: 0 on success and errorno otherwise.
- */
-int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
- unsigned int count)
+static void _opp_put_supported_hw(struct opp_table *opp_table)
{
- struct opp_table *opp_table;
-
- opp_table = dev_pm_opp_set_supported_hw(dev, versions, count);
- if (IS_ERR(opp_table))
- return PTR_ERR(opp_table);
-
- return devm_add_action_or_reset(dev, devm_pm_opp_supported_hw_release,
- opp_table);
+ if (opp_table->supported_hw) {
+ kfree(opp_table->supported_hw);
+ opp_table->supported_hw = NULL;
+ opp_table->supported_hw_count = 0;
+ }
}
-EXPORT_SYMBOL_GPL(devm_pm_opp_set_supported_hw);
/**
* dev_pm_opp_set_prop_name() - Set prop-extn name
@@ -2615,10 +2570,12 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
// Configure supported hardware
if (config->supported_hw) {
- ret = dev_pm_opp_set_supported_hw(dev, config->supported_hw,
- config->supported_hw_count);
- if (IS_ERR(ret))
+ err = _opp_set_supported_hw(opp_table, config->supported_hw,
+ config->supported_hw_count);
+ if (err) {
+ ret = ERR_PTR(err);
goto err;
+ }
}
// Configure supplies
@@ -2668,8 +2625,7 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
_opp_put_regulators(opp_table);
- if (opp_table->supported_hw)
- dev_pm_opp_put_supported_hw(opp_table);
+ _opp_put_supported_hw(opp_table);
if (opp_table->set_opp)
dev_pm_opp_unregister_set_opp_helper(opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 11896ebe1fb1..b80982e5a067 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -184,9 +184,6 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_co
int devm_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config);
void dev_pm_opp_clear_config(struct opp_table *opp_table);
-struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
-void dev_pm_opp_put_supported_hw(struct opp_table *opp_table);
-int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name);
@@ -369,22 +366,6 @@ static inline int dev_pm_opp_unregister_notifier(struct device *dev, struct noti
return -EOPNOTSUPP;
}
-static inline struct opp_table *dev_pm_opp_set_supported_hw(struct device *dev,
- const u32 *versions,
- unsigned int count)
-{
- return ERR_PTR(-EOPNOTSUPP);
-}
-
-static inline void dev_pm_opp_put_supported_hw(struct opp_table *opp_table) {}
-
-static inline int devm_pm_opp_set_supported_hw(struct device *dev,
- const u32 *versions,
- unsigned int count)
-{
- return -EOPNOTSUPP;
-}
-
static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
int (*set_opp)(struct dev_pm_set_opp_data *data))
{
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/cpufreq/imx-cpufreq-dt.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/imx-cpufreq-dt.c b/drivers/cpufreq/imx-cpufreq-dt.c
index 3fe9125156b4..57917b0670f2 100644
--- a/drivers/cpufreq/imx-cpufreq-dt.c
+++ b/drivers/cpufreq/imx-cpufreq-dt.c
@@ -86,6 +86,10 @@ static int imx_cpufreq_dt_probe(struct platform_device *pdev)
u32 cell_value, supported_hw[2];
int speed_grade, mkt_segment;
int ret;
+ struct dev_pm_opp_config config = {
+ .supported_hw = supported_hw,
+ .supported_hw_count = ARRAY_SIZE(supported_hw),
+ };
cpu_dev = get_cpu_device(0);
@@ -153,17 +157,17 @@ static int imx_cpufreq_dt_probe(struct platform_device *pdev)
dev_info(&pdev->dev, "cpu speed grade %d mkt segment %d supported-hw %#x %#x\n",
speed_grade, mkt_segment, supported_hw[0], supported_hw[1]);
- cpufreq_opp_table = dev_pm_opp_set_supported_hw(cpu_dev, supported_hw, 2);
+ cpufreq_opp_table = dev_pm_opp_set_config(cpu_dev, &config);
if (IS_ERR(cpufreq_opp_table)) {
ret = PTR_ERR(cpufreq_opp_table);
- dev_err(&pdev->dev, "Failed to set supported opp: %d\n", ret);
+ dev_err(&pdev->dev, "Failed to set Opp config: %d\n", ret);
return ret;
}
cpufreq_dt_pdev = platform_device_register_data(
&pdev->dev, "cpufreq-dt", -1, NULL, 0);
if (IS_ERR(cpufreq_dt_pdev)) {
- dev_pm_opp_put_supported_hw(cpufreq_opp_table);
+ dev_pm_opp_clear_config(cpufreq_opp_table);
ret = PTR_ERR(cpufreq_dt_pdev);
dev_err(&pdev->dev, "Failed to register cpufreq-dt: %d\n", ret);
return ret;
@@ -176,7 +180,7 @@ static int imx_cpufreq_dt_remove(struct platform_device *pdev)
{
platform_device_unregister(cpufreq_dt_pdev);
if (!of_machine_is_compatible("fsl,imx7ulp"))
- dev_pm_opp_put_supported_hw(cpufreq_opp_table);
+ dev_pm_opp_clear_config(cpufreq_opp_table);
else
clk_bulk_put(ARRAY_SIZE(imx7ulp_clks), imx7ulp_clks);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
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(&ldevfreq->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, &config);
if (ret) {
/* Continue if the optional regulator is missing */
if (ret != -ENODEV)
--
2.31.1.272.g89b43f80a514
Rearrange the helpers now to make them look clean.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 49 +++++++++++++++-------------------------------
1 file changed, 16 insertions(+), 33 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e6c76b90dbf7..a9e39ebfe9da 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2370,8 +2370,8 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
struct opp_table *dev_pm_opp_set_config(struct device *dev,
struct dev_pm_opp_config *config)
{
- struct opp_table *opp_table, *ret;
- int err;
+ struct opp_table *opp_table;
+ int ret;
opp_table = _add_opp_table(dev, false);
if (IS_ERR(opp_table))
@@ -2379,73 +2379,61 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
/* This should be called before OPPs are initialized */
if (WARN_ON(!list_empty(&opp_table->opp_list))) {
- ret = ERR_PTR(-EBUSY);
+ ret = -EBUSY;
goto err;
}
// Configure clock
if (config->clk_name) {
- err = _opp_set_clkname(opp_table, dev, config->clk_name);
- if (err) {
- ret = ERR_PTR(err);
+ ret = _opp_set_clkname(opp_table, dev, config->clk_name);
+ if (ret)
goto err;
- }
}
// Configure property names
if (config->prop_name) {
- err = _opp_set_prop_name(opp_table, config->prop_name);
- if (err) {
- ret = ERR_PTR(err);
+ ret = _opp_set_prop_name(opp_table, config->prop_name);
+ if (ret)
goto err;
- }
}
// Configure opp helper
if (config->set_opp) {
- err = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
- if (err) {
- ret = ERR_PTR(err);
+ ret = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
+ if (ret)
goto err;
- }
}
// Configure supported hardware
if (config->supported_hw) {
- err = _opp_set_supported_hw(opp_table, config->supported_hw,
+ ret = _opp_set_supported_hw(opp_table, config->supported_hw,
config->supported_hw_count);
- if (err) {
- ret = ERR_PTR(err);
+ if (ret)
goto err;
- }
}
// Configure supplies
if (config->regulator_names) {
- err = _opp_set_regulators(opp_table, dev,
+ ret = _opp_set_regulators(opp_table, dev,
config->regulator_names,
config->regulator_count);
- if (err) {
- ret = ERR_PTR(err);
+ if (ret)
goto err;
- }
}
// Attach genpds
if (config->genpd_names) {
- err = _opp_attach_genpd(opp_table, dev, config->genpd_names,
+ ret = _opp_attach_genpd(opp_table, dev, config->genpd_names,
config->virt_devs);
- if (err) {
- ret = ERR_PTR(err);
+ if (ret)
goto err;
- }
}
return opp_table;
err:
dev_pm_opp_clear_config(opp_table);
- return ret;
+ return ERR_PTR(ret);
}
EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
@@ -2465,15 +2453,10 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
void dev_pm_opp_clear_config(struct opp_table *opp_table)
{
_opp_detach_genpd(opp_table);
-
_opp_put_regulators(opp_table);
-
_opp_put_supported_hw(opp_table);
-
_opp_unregister_set_opp_helper(opp_table);
-
_opp_put_prop_name(opp_table);
-
_opp_put_clkname(opp_table);
dev_pm_opp_put_opp_table(opp_table);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/mmc/host/sdhci-msm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 50c71e0ba5e4..994f3f0231f7 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2496,6 +2496,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
const struct sdhci_msm_offset *msm_offset;
const struct sdhci_msm_variant_info *var_info;
struct device_node *node = pdev->dev.of_node;
+ struct dev_pm_opp_config opp_config = {
+ .clk_name = "core",
+ };
host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
if (IS_ERR(host))
@@ -2564,7 +2567,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
if (ret)
goto bus_clk_disable;
- ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
+ ret = devm_pm_opp_set_config(&pdev->dev, &opp_config);
if (ret)
goto bus_clk_disable;
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/cpufreq/sun50i-cpufreq-nvmem.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 2deed8d8773f..c1bee39758e2 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -104,6 +104,9 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
snprintf(name, MAX_NAME_LEN, "speed%d", speed);
for_each_possible_cpu(cpu) {
+ struct dev_pm_opp_config config = {
+ .prop_name = name,
+ };
struct device *cpu_dev = get_cpu_device(cpu);
if (!cpu_dev) {
@@ -111,10 +114,10 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
goto free_opp;
}
- opp_tables[cpu] = dev_pm_opp_set_prop_name(cpu_dev, name);
+ opp_tables[cpu] = dev_pm_opp_set_config(cpu_dev, &config);
if (IS_ERR(opp_tables[cpu])) {
ret = PTR_ERR(opp_tables[cpu]);
- pr_err("Failed to set prop name\n");
+ pr_err("Failed to set OPP config\n");
goto free_opp;
}
}
@@ -133,7 +136,7 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
for_each_possible_cpu(cpu) {
if (IS_ERR_OR_NULL(opp_tables[cpu]))
break;
- dev_pm_opp_put_prop_name(opp_tables[cpu]);
+ dev_pm_opp_clear_config(opp_tables[cpu]);
}
kfree(opp_tables);
@@ -148,7 +151,7 @@ static int sun50i_cpufreq_nvmem_remove(struct platform_device *pdev)
platform_device_unregister(cpufreq_dt_pdev);
for_each_possible_cpu(cpu)
- dev_pm_opp_put_prop_name(opp_tables[cpu]);
+ dev_pm_opp_clear_config(opp_tables[cpu]);
kfree(opp_tables);
--
2.31.1.272.g89b43f80a514
Now that everyone has migrated to dev_pm_opp_set_config(), remove the
public interface for dev_pm_opp_register_set_opp_helper() and friends.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 89 ++++++++++--------------------------------
include/linux/pm_opp.h | 17 --------
2 files changed, 21 insertions(+), 85 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2590c0c05a0..412af91d2039 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2200,7 +2200,7 @@ static void _opp_put_clkname(struct opp_table *opp_table)
}
/**
- * dev_pm_opp_register_set_opp_helper() - Register custom set OPP helper
+ * _opp_register_set_opp_helper() - Register custom set OPP helper
* @dev: Device for which the helper is getting registered.
* @set_opp: Custom set OPP helper.
*
@@ -2209,32 +2209,18 @@ static void _opp_put_clkname(struct opp_table *opp_table)
*
* This must be called before any OPPs are initialized for the device.
*/
-struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
- int (*set_opp)(struct dev_pm_set_opp_data *data))
+static int _opp_register_set_opp_helper(struct opp_table *opp_table,
+ struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data))
{
struct dev_pm_set_opp_data *data;
- struct opp_table *opp_table;
-
- if (!set_opp)
- return ERR_PTR(-EINVAL);
-
- opp_table = _add_opp_table(dev, false);
- if (IS_ERR(opp_table))
- return opp_table;
-
- /* This should be called before OPPs are initialized */
- if (WARN_ON(!list_empty(&opp_table->opp_list))) {
- dev_pm_opp_put_opp_table(opp_table);
- return ERR_PTR(-EBUSY);
- }
/* Another CPU that shares the OPP table has set the helper ? */
if (opp_table->set_opp)
- return opp_table;
+ return 0;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
mutex_lock(&opp_table->lock);
opp_table->set_opp_data = data;
@@ -2247,60 +2233,26 @@ struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
opp_table->set_opp = set_opp;
- return opp_table;
+ return 0;
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_register_set_opp_helper);
/**
- * dev_pm_opp_unregister_set_opp_helper() - Releases resources blocked for
- * set_opp helper
- * @opp_table: OPP table returned from dev_pm_opp_register_set_opp_helper().
+ * _opp_unregister_set_opp_helper() - Releases resources blocked for set_opp helper
+ * @opp_table: OPP table returned from _opp_register_set_opp_helper().
*
* Release resources blocked for platform specific set_opp helper.
*/
-void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
-{
- if (unlikely(!opp_table))
- return;
-
- opp_table->set_opp = NULL;
-
- mutex_lock(&opp_table->lock);
- kfree(opp_table->set_opp_data);
- opp_table->set_opp_data = NULL;
- mutex_unlock(&opp_table->lock);
-
- dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
-
-static void devm_pm_opp_unregister_set_opp_helper(void *data)
-{
- dev_pm_opp_unregister_set_opp_helper(data);
-}
-
-/**
- * devm_pm_opp_register_set_opp_helper() - Register custom set OPP helper
- * @dev: Device for which the helper is getting registered.
- * @set_opp: Custom set OPP helper.
- *
- * This is a resource-managed version of dev_pm_opp_register_set_opp_helper().
- *
- * Return: 0 on success and errorno otherwise.
- */
-int devm_pm_opp_register_set_opp_helper(struct device *dev,
- int (*set_opp)(struct dev_pm_set_opp_data *data))
+static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
{
- struct opp_table *opp_table;
-
- opp_table = dev_pm_opp_register_set_opp_helper(dev, set_opp);
- if (IS_ERR(opp_table))
- return PTR_ERR(opp_table);
+ if (opp_table->set_opp) {
+ opp_table->set_opp = NULL;
- return devm_add_action_or_reset(dev, devm_pm_opp_unregister_set_opp_helper,
- opp_table);
+ mutex_lock(&opp_table->lock);
+ kfree(opp_table->set_opp_data);
+ opp_table->set_opp_data = NULL;
+ mutex_unlock(&opp_table->lock);
+ }
}
-EXPORT_SYMBOL_GPL(devm_pm_opp_register_set_opp_helper);
static void _opp_detach_genpd(struct opp_table *opp_table)
{
@@ -2513,9 +2465,11 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
// Configure opp helper
if (config->set_opp) {
- ret = dev_pm_opp_register_set_opp_helper(dev, config->set_opp);
- if (IS_ERR(ret))
+ err = _opp_register_set_opp_helper(opp_table, dev, config->set_opp);
+ if (err) {
+ ret = ERR_PTR(err);
goto err;
+ }
}
// Configure supported hardware
@@ -2577,8 +2531,7 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
_opp_put_supported_hw(opp_table);
- if (opp_table->set_opp)
- dev_pm_opp_unregister_set_opp_helper(opp_table);
+ _opp_unregister_set_opp_helper(opp_table);
if (opp_table->prop_name)
dev_pm_opp_put_prop_name(opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 7afa8160590d..0ad60c7dca02 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -186,9 +186,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct opp_table *opp_table);
-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 * 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 * const *names, struct device ***virt_devs);
@@ -363,20 +360,6 @@ static inline int dev_pm_opp_unregister_notifier(struct device *dev, struct noti
return -EOPNOTSUPP;
}
-static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev,
- int (*set_opp)(struct dev_pm_set_opp_data *data))
-{
- return ERR_PTR(-EOPNOTSUPP);
-}
-
-static inline void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) {}
-
-static inline int devm_pm_opp_register_set_opp_helper(struct device *dev,
- int (*set_opp)(struct dev_pm_set_opp_data *data))
-{
- return -EOPNOTSUPP;
-}
-
static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
{
return ERR_PTR(-EOPNOTSUPP);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/cpufreq/ti-cpufreq.c | 38 +++++++++++++++---------------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
index 8f9fdd864391..cc58675df5c4 100644
--- a/drivers/cpufreq/ti-cpufreq.c
+++ b/drivers/cpufreq/ti-cpufreq.c
@@ -324,10 +324,13 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
{
u32 version[VERSION_COUNT];
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"};
int ret;
+ struct dev_pm_opp_config config = {
+ .supported_hw = version,
+ .supported_hw_count = ARRAY_SIZE(version),
+ };
match = dev_get_platdata(&pdev->dev);
if (!match)
@@ -370,33 +373,24 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
if (ret)
goto fail_put_node;
- ti_opp_table = dev_pm_opp_set_supported_hw(opp_data->cpu_dev,
- version, VERSION_COUNT);
- if (IS_ERR(ti_opp_table)) {
- dev_err(opp_data->cpu_dev,
- "Failed to set supported hardware\n");
- ret = PTR_ERR(ti_opp_table);
- goto fail_put_node;
- }
-
- opp_data->opp_table = ti_opp_table;
-
if (opp_data->soc_data->multi_regulator) {
- const char * const *reg_names = default_reg_names;
+ config.regulator_count = ARRAY_SIZE(default_reg_names);
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));
- if (IS_ERR(ti_opp_table)) {
- dev_pm_opp_put_supported_hw(opp_data->opp_table);
- ret = PTR_ERR(ti_opp_table);
- goto fail_put_node;
- }
+ config.regulator_names = opp_data->soc_data->reg_names;
+ else
+ config.regulator_names = default_reg_names;
+ }
+
+ opp_data->opp_table = dev_pm_opp_set_config(opp_data->cpu_dev, &config);
+ if (IS_ERR(opp_data->opp_table)) {
+ dev_err(opp_data->cpu_dev, "Failed to set OPP config\n");
+ ret = PTR_ERR(opp_data->opp_table);
+ goto fail_put_node;
}
of_node_put(opp_data->opp_node);
+
register_cpufreq_dt:
platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
--
2.31.1.272.g89b43f80a514
Now that everyone has migrated to dev_pm_opp_set_config(), remove the
public interface for dev_pm_opp_attach_genpd() and friends.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 86 ++++++++++--------------------------------
include/linux/pm_opp.h | 17 ---------
2 files changed, 20 insertions(+), 83 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 412af91d2039..69c6cf6a0bcc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2254,7 +2254,7 @@ static void _opp_unregister_set_opp_helper(struct opp_table *opp_table)
}
}
-static void _opp_detach_genpd(struct opp_table *opp_table)
+static void _detach_genpd(struct opp_table *opp_table)
{
int index;
@@ -2274,7 +2274,7 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
}
/**
- * dev_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer
+ * _opp_attach_genpd - Attach genpd(s) for the device and save virtual device pointer
* @dev: Consumer device for which the genpd is getting attached.
* @names: Null terminated array of pointers containing names of genpd to attach.
* @virt_devs: Pointer to return the array of virtual devices.
@@ -2295,30 +2295,23 @@ static void _opp_detach_genpd(struct opp_table *opp_table)
* The order of entries in the names array must match the order in which
* "required-opps" are added in DT.
*/
-struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
- const char * const *names, struct device ***virt_devs)
+static int _opp_attach_genpd(struct opp_table *opp_table, struct device *dev,
+ const char * const *names, struct device ***virt_devs)
{
- struct opp_table *opp_table;
struct device *virt_dev;
int index = 0, ret = -EINVAL;
const char * const *name = names;
- opp_table = _add_opp_table(dev, false);
- if (IS_ERR(opp_table))
- return opp_table;
-
if (opp_table->genpd_virt_devs)
- return opp_table;
+ return 0;
/*
* If the genpd's OPP table isn't already initialized, parsing of the
* required-opps fail for dev. We should retry this after genpd's OPP
* table is added.
*/
- if (!opp_table->required_opp_count) {
- ret = -EPROBE_DEFER;
- goto put_table;
- }
+ if (!opp_table->required_opp_count)
+ return -EPROBE_DEFER;
mutex_lock(&opp_table->genpd_virt_dev_lock);
@@ -2351,74 +2344,34 @@ struct opp_table *dev_pm_opp_attach_genpd(struct device *dev,
*virt_devs = opp_table->genpd_virt_devs;
mutex_unlock(&opp_table->genpd_virt_dev_lock);
- return opp_table;
+ return 0;
err:
- _opp_detach_genpd(opp_table);
+ _detach_genpd(opp_table);
unlock:
mutex_unlock(&opp_table->genpd_virt_dev_lock);
+ return ret;
-put_table:
- dev_pm_opp_put_opp_table(opp_table);
-
- return ERR_PTR(ret);
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_attach_genpd);
/**
- * dev_pm_opp_detach_genpd() - Detach genpd(s) from the device.
- * @opp_table: OPP table returned by dev_pm_opp_attach_genpd().
+ * _opp_detach_genpd() - Detach genpd(s) from the device.
+ * @opp_table: OPP table returned by _opp_attach_genpd().
*
* This detaches the genpd(s), resets the virtual device pointers, and puts the
* OPP table.
*/
-void dev_pm_opp_detach_genpd(struct opp_table *opp_table)
+static void _opp_detach_genpd(struct opp_table *opp_table)
{
- if (unlikely(!opp_table))
- return;
-
/*
* Acquire genpd_virt_dev_lock to make sure virt_dev isn't getting
* used in parallel.
*/
mutex_lock(&opp_table->genpd_virt_dev_lock);
- _opp_detach_genpd(opp_table);
+ _detach_genpd(opp_table);
mutex_unlock(&opp_table->genpd_virt_dev_lock);
-
- dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_detach_genpd);
-
-static void devm_pm_opp_detach_genpd(void *data)
-{
- dev_pm_opp_detach_genpd(data);
}
-/**
- * devm_pm_opp_attach_genpd - Attach genpd(s) for the device and save virtual
- * device pointer
- * @dev: Consumer device for which the genpd is getting attached.
- * @names: Null terminated array of pointers containing names of genpd to attach.
- * @virt_devs: Pointer to return the array of virtual devices.
- *
- * This is a resource-managed version of dev_pm_opp_attach_genpd().
- *
- * Return: 0 on success and errorno otherwise.
- */
-int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names,
- struct device ***virt_devs)
-{
- struct opp_table *opp_table;
-
- opp_table = dev_pm_opp_attach_genpd(dev, names, virt_devs);
- if (IS_ERR(opp_table))
- return PTR_ERR(opp_table);
-
- return devm_add_action_or_reset(dev, devm_pm_opp_detach_genpd,
- opp_table);
-}
-EXPORT_SYMBOL_GPL(devm_pm_opp_attach_genpd);
-
/**
* dev_pm_opp_set_config() - Set OPP configuration for the device.
* @dev: Device for which configuration is being set.
@@ -2495,10 +2448,12 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
// Attach genpds
if (config->genpd_names) {
- ret = dev_pm_opp_attach_genpd(dev, config->genpd_names,
- config->virt_devs);
- if (IS_ERR(ret))
+ err = _opp_attach_genpd(opp_table, dev, config->genpd_names,
+ config->virt_devs);
+ if (err) {
+ ret = ERR_PTR(err);
goto err;
+ }
}
return opp_table;
@@ -2524,8 +2479,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
*/
void dev_pm_opp_clear_config(struct opp_table *opp_table)
{
- if (opp_table->genpd_virt_devs)
- dev_pm_opp_detach_genpd(opp_table);
+ _opp_detach_genpd(opp_table);
_opp_put_regulators(opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0ad60c7dca02..a310564ab698 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -186,9 +186,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct opp_table *opp_table);
-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 * 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);
@@ -367,20 +364,6 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con
static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {}
-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);
-}
-
-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 * const *names,
- struct device ***virt_devs)
-{
- return -EOPNOTSUPP;
-}
-
static inline struct opp_table *dev_pm_opp_set_config(struct device *dev, struct dev_pm_opp_config *config)
{
return ERR_PTR(-EOPNOTSUPP);
--
2.31.1.272.g89b43f80a514
Now that everyone has migrated to dev_pm_opp_set_config(), remove the
public interface for dev_pm_opp_set_regulators() and friends.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 95 ++++++++++--------------------------------
include/linux/pm_opp.h | 17 --------
2 files changed, 22 insertions(+), 90 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 30dbef0f4d17..9297b5e944f7 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -987,8 +987,8 @@ static int _set_opp_custom(const struct opp_table *opp_table,
int size;
/*
- * We support this only if dev_pm_opp_set_regulators() was called
- * earlier.
+ * We support this only if dev_pm_opp_set_config() was called
+ * earlier to set regulators.
*/
if (opp_table->sod_supplies) {
size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
@@ -2093,7 +2093,7 @@ void dev_pm_opp_put_prop_name(struct opp_table *opp_table)
EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
/**
- * dev_pm_opp_set_regulators() - Set regulator names for the device
+ * _opp_set_regulators() - Set regulator names for the device
* @dev: Device for which regulator name is being set.
* @names: Array of pointers to the names of the regulator.
* @count: Number of regulators.
@@ -2104,36 +2104,22 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_put_prop_name);
*
* This must be called before any OPPs are initialized for the device.
*/
-struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
- const char * const names[],
- unsigned int count)
+static int _opp_set_regulators(struct opp_table *opp_table, struct device *dev,
+ const char * const names[], unsigned int count)
{
struct dev_pm_opp_supply *supplies;
- struct opp_table *opp_table;
struct regulator *reg;
int ret, i;
- opp_table = _add_opp_table(dev, false);
- if (IS_ERR(opp_table))
- return opp_table;
-
- /* This should be called before OPPs are initialized */
- if (WARN_ON(!list_empty(&opp_table->opp_list))) {
- ret = -EBUSY;
- goto err;
- }
-
/* Another CPU that shares the OPP table has set the regulators ? */
if (opp_table->regulators)
- return opp_table;
+ return 0;
opp_table->regulators = kmalloc_array(count,
sizeof(*opp_table->regulators),
GFP_KERNEL);
- if (!opp_table->regulators) {
- ret = -ENOMEM;
- goto err;
- }
+ if (!opp_table->regulators)
+ return -ENOMEM;
for (i = 0; i < count; i++) {
reg = regulator_get_optional(dev, names[i]);
@@ -2163,7 +2149,7 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
}
mutex_unlock(&opp_table->lock);
- return opp_table;
+ return 0;
free_regulators:
while (i != 0)
@@ -2172,26 +2158,20 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
kfree(opp_table->regulators);
opp_table->regulators = NULL;
opp_table->regulator_count = -1;
-err:
- dev_pm_opp_put_opp_table(opp_table);
- return ERR_PTR(ret);
+ return ret;
}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_regulators);
/**
- * dev_pm_opp_put_regulators() - Releases resources blocked for regulator
- * @opp_table: OPP table returned from dev_pm_opp_set_regulators().
+ * _opp_put_regulators() - Releases resources blocked for regulator
+ * @opp_table: OPP table returned from _opp_set_regulators().
*/
-void dev_pm_opp_put_regulators(struct opp_table *opp_table)
+static void _opp_put_regulators(struct opp_table *opp_table)
{
int i;
- if (unlikely(!opp_table))
- return;
-
if (!opp_table->regulators)
- goto put_opp_table;
+ return;
if (opp_table->enabled) {
for (i = opp_table->regulator_count - 1; i >= 0; i--)
@@ -2214,41 +2194,7 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
kfree(opp_table->regulators);
opp_table->regulators = NULL;
opp_table->regulator_count = -1;
-
-put_opp_table:
- dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_regulators);
-
-static void devm_pm_opp_regulators_release(void *data)
-{
- dev_pm_opp_put_regulators(data);
-}
-
-/**
- * devm_pm_opp_set_regulators() - Set regulator names for the device
- * @dev: Device for which regulator name is being set.
- * @names: Array of pointers to the names of the regulator.
- * @count: Number of regulators.
- *
- * This is a resource-managed variant of dev_pm_opp_set_regulators().
- *
- * Return: 0 on success and errorno otherwise.
- */
-int devm_pm_opp_set_regulators(struct device *dev,
- const char * const names[],
- unsigned int count)
-{
- struct opp_table *opp_table;
-
- opp_table = dev_pm_opp_set_regulators(dev, names, count);
- if (IS_ERR(opp_table))
- return PTR_ERR(opp_table);
-
- return devm_add_action_or_reset(dev, devm_pm_opp_regulators_release,
- opp_table);
}
-EXPORT_SYMBOL_GPL(devm_pm_opp_set_regulators);
/**
* dev_pm_opp_set_clkname() - Set clk name for the device
@@ -2634,6 +2580,7 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
struct dev_pm_opp_config *config)
{
struct opp_table *opp_table, *ret;
+ int err;
opp_table = _add_opp_table(dev, false);
if (IS_ERR(opp_table))
@@ -2676,10 +2623,13 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
// Configure supplies
if (config->regulator_names) {
- ret = dev_pm_opp_set_regulators(dev, config->regulator_names,
- config->regulator_count);
- if (IS_ERR(ret))
+ err = _opp_set_regulators(opp_table, dev,
+ config->regulator_names,
+ config->regulator_count);
+ if (err) {
+ ret = ERR_PTR(err);
goto err;
+ }
}
// Attach genpds
@@ -2716,8 +2666,7 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
if (opp_table->genpd_virt_devs)
dev_pm_opp_detach_genpd(opp_table);
- if (opp_table->regulators)
- dev_pm_opp_put_regulators(opp_table);
+ _opp_put_regulators(opp_table);
if (opp_table->supported_hw)
dev_pm_opp_put_supported_hw(opp_table);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0d5d07dd164a..11896ebe1fb1 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -189,9 +189,6 @@ void dev_pm_opp_put_supported_hw(struct opp_table *opp_table);
int devm_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, unsigned int count);
struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct opp_table *opp_table);
-struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
-void dev_pm_opp_put_regulators(struct opp_table *opp_table);
-int devm_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count);
struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name);
void dev_pm_opp_put_clkname(struct opp_table *opp_table);
int devm_pm_opp_set_clkname(struct device *dev, const char *name);
@@ -409,20 +406,6 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con
static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {}
-static inline struct opp_table *dev_pm_opp_set_regulators(struct device *dev, const char * const names[], unsigned int count)
-{
- return ERR_PTR(-EOPNOTSUPP);
-}
-
-static inline void dev_pm_opp_put_regulators(struct opp_table *opp_table) {}
-
-static inline int devm_pm_opp_set_regulators(struct device *dev,
- const char * const names[],
- unsigned int count)
-{
- return -EOPNOTSUPP;
-}
-
static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
{
return ERR_PTR(-EOPNOTSUPP);
--
2.31.1.272.g89b43f80a514
Now that everyone has migrated to dev_pm_opp_set_config(), remove the
public interface for dev_pm_opp_set_clkname() and friends.
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/opp/core.c | 93 ++++++++++--------------------------------
include/linux/pm_opp.h | 15 -------
2 files changed, 21 insertions(+), 87 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 07cb8ff33a6d..c2590c0c05a0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2152,7 +2152,7 @@ static void _opp_put_regulators(struct opp_table *opp_table)
}
/**
- * dev_pm_opp_set_clkname() - Set clk name for the device
+ * _opp_set_clkname() - Set clk name for the device
* @dev: Device for which clk name is being set.
* @name: Clk name.
*
@@ -2163,93 +2163,41 @@ static void _opp_put_regulators(struct opp_table *opp_table)
*
* This must be called before any OPPs are initialized for the device.
*/
-struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
+static int _opp_set_clkname(struct opp_table *opp_table, struct device *dev,
+ const char *name)
{
- struct opp_table *opp_table;
- int ret;
-
- opp_table = _add_opp_table(dev, false);
- if (IS_ERR(opp_table))
- return opp_table;
-
- /* This should be called before OPPs are initialized */
- if (WARN_ON(!list_empty(&opp_table->opp_list))) {
- ret = -EBUSY;
- goto err;
- }
-
/* Another CPU that shares the OPP table has set the clkname ? */
if (opp_table->clk_configured)
- return opp_table;
+ return 0;
/* clk shouldn't be initialized at this point */
- if (WARN_ON(opp_table->clk)) {
- ret = -EBUSY;
- goto err;
- }
+ if (WARN_ON(opp_table->clk))
+ return -EBUSY;
/* Find clk for the device */
opp_table->clk = clk_get(dev, name);
if (IS_ERR(opp_table->clk)) {
- ret = dev_err_probe(dev, PTR_ERR(opp_table->clk),
+ return dev_err_probe(dev, PTR_ERR(opp_table->clk),
"%s: Couldn't find clock\n", __func__);
- goto err;
}
opp_table->clk_configured = true;
- return opp_table;
-
-err:
- dev_pm_opp_put_opp_table(opp_table);
-
- return ERR_PTR(ret);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_clkname);
-
-/**
- * dev_pm_opp_put_clkname() - Releases resources blocked for clk.
- * @opp_table: OPP table returned from dev_pm_opp_set_clkname().
- */
-void dev_pm_opp_put_clkname(struct opp_table *opp_table)
-{
- if (unlikely(!opp_table))
- return;
-
- clk_put(opp_table->clk);
- opp_table->clk = ERR_PTR(-EINVAL);
- opp_table->clk_configured = false;
-
- dev_pm_opp_put_opp_table(opp_table);
-}
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);
-
-static void devm_pm_opp_clkname_release(void *data)
-{
- dev_pm_opp_put_clkname(data);
+ return 0;
}
/**
- * devm_pm_opp_set_clkname() - Set clk name for the device
- * @dev: Device for which clk name is being set.
- * @name: Clk name.
- *
- * This is a resource-managed variant of dev_pm_opp_set_clkname().
- *
- * Return: 0 on success and errorno otherwise.
+ * _opp_put_clkname() - Releases resources blocked for clk.
+ * @opp_table: OPP table returned from _opp_set_clkname().
*/
-int devm_pm_opp_set_clkname(struct device *dev, const char *name)
+static void _opp_put_clkname(struct opp_table *opp_table)
{
- struct opp_table *opp_table;
-
- opp_table = dev_pm_opp_set_clkname(dev, name);
- if (IS_ERR(opp_table))
- return PTR_ERR(opp_table);
-
- return devm_add_action_or_reset(dev, devm_pm_opp_clkname_release,
- opp_table);
+ if (opp_table->clk_configured) {
+ clk_put(opp_table->clk);
+ opp_table->clk = ERR_PTR(-EINVAL);
+ opp_table->clk_configured = false;
+ }
}
-EXPORT_SYMBOL_GPL(devm_pm_opp_set_clkname);
/**
* dev_pm_opp_register_set_opp_helper() - Register custom set OPP helper
@@ -2549,9 +2497,11 @@ struct opp_table *dev_pm_opp_set_config(struct device *dev,
// Configure clock
if (config->clk_name) {
- ret = dev_pm_opp_set_clkname(dev, config->clk_name);
- if (IS_ERR(ret))
+ err = _opp_set_clkname(opp_table, dev, config->clk_name);
+ if (err) {
+ ret = ERR_PTR(err);
goto err;
+ }
}
// Configure property names
@@ -2633,8 +2583,7 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table)
if (opp_table->prop_name)
dev_pm_opp_put_prop_name(opp_table);
- if (opp_table->clk_configured)
- dev_pm_opp_put_clkname(opp_table);
+ _opp_put_clkname(opp_table);
dev_pm_opp_put_opp_table(opp_table);
}
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index b80982e5a067..7afa8160590d 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -186,9 +186,6 @@ void dev_pm_opp_clear_config(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name);
void dev_pm_opp_put_prop_name(struct opp_table *opp_table);
-struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name);
-void dev_pm_opp_put_clkname(struct opp_table *opp_table);
-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));
@@ -387,18 +384,6 @@ static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, con
static inline void dev_pm_opp_put_prop_name(struct opp_table *opp_table) {}
-static inline struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
-{
- return ERR_PTR(-EOPNOTSUPP);
-}
-
-static inline void dev_pm_opp_put_clkname(struct opp_table *opp_table) {}
-
-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 * const *names, struct device ***virt_devs)
{
return ERR_PTR(-EOPNOTSUPP);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
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 = &pfdev->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, &config);
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
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 <[email protected]>
---
drivers/devfreq/tegra30-devfreq.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c
index 65ecf17a36f4..30382bdfc655 100644
--- a/drivers/devfreq/tegra30-devfreq.c
+++ b/drivers/devfreq/tegra30-devfreq.c
@@ -830,6 +830,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
unsigned int i;
long rate;
int err;
+ struct dev_pm_opp_config config = {
+ .supported_hw = &hw_version,
+ .supported_hw_count = 1,
+ };
tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
if (!tegra)
@@ -874,9 +878,9 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
return err;
}
- err = devm_pm_opp_set_supported_hw(&pdev->dev, &hw_version, 1);
+ err = devm_pm_opp_set_config(&pdev->dev, &config);
if (err) {
- dev_err(&pdev->dev, "Failed to set supported HW: %d\n", err);
+ dev_err(&pdev->dev, "Failed to set OPP config: %d\n", err);
return err;
}
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/opp/ti-opp-supply.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index bd4771f388ab..a30825dc30cf 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -382,6 +382,9 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
const struct of_device_id *match;
const struct ti_opp_supply_of_data *of_data;
int ret = 0;
+ struct dev_pm_opp_config config = {
+ .set_opp = ti_opp_supply_set_opp,
+ };
match = of_match_device(ti_opp_supply_of_match, dev);
if (!match) {
@@ -405,8 +408,7 @@ static int ti_opp_supply_probe(struct platform_device *pdev)
return ret;
}
- ret = PTR_ERR_OR_ZERO(dev_pm_opp_register_set_opp_helper(cpu_dev,
- ti_opp_supply_set_opp));
+ ret = PTR_ERR_OR_ZERO(dev_pm_opp_set_config(cpu_dev, &config));
if (ret)
_free_optimized_voltages(dev, &opp_data);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/devfreq/exynos-bus.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index e689101abc93..780e525eb92a 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -161,7 +161,7 @@ static void exynos_bus_exit(struct device *dev)
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
- dev_pm_opp_put_regulators(bus->opp_table);
+ dev_pm_opp_clear_config(bus->opp_table);
bus->opp_table = NULL;
}
@@ -182,11 +182,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct opp_table *opp_table;
const char *vdd = "vdd";
int i, ret, count, size;
+ struct dev_pm_opp_config config = {
+ .regulator_names = &vdd,
+ .regulator_count = 1,
+ };
- opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
+ opp_table = dev_pm_opp_set_config(dev, &config);
if (IS_ERR(opp_table)) {
ret = PTR_ERR(opp_table);
- dev_err(dev, "failed to set regulators %d\n", ret);
+ dev_err(dev, "failed to set OPP config %d\n", ret);
return ret;
}
@@ -236,7 +240,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
return 0;
err_regulator:
- dev_pm_opp_put_regulators(bus->opp_table);
+ dev_pm_opp_clear_config(bus->opp_table);
bus->opp_table = NULL;
return ret;
@@ -459,7 +463,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
err_reg:
- dev_pm_opp_put_regulators(bus->opp_table);
+ dev_pm_opp_clear_config(bus->opp_table);
bus->opp_table = NULL;
return ret;
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
---
drivers/cpufreq/cpufreq-dt.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index 8fcaba541539..65f299acb0c4 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -220,12 +220,16 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu)
*/
reg_name = find_supply_name(cpu_dev);
if (reg_name) {
- priv->opp_table = dev_pm_opp_set_regulators(cpu_dev, ®_name,
- 1);
+ struct dev_pm_opp_config config = {
+ .regulator_names = ®_name,
+ .regulator_count = 1,
+ };
+
+ priv->opp_table = dev_pm_opp_set_config(cpu_dev, &config);
if (IS_ERR(priv->opp_table)) {
ret = PTR_ERR(priv->opp_table);
if (ret != -EPROBE_DEFER)
- dev_err(cpu_dev, "failed to set regulators: %d\n",
+ dev_err(cpu_dev, "failed to set OPP config: %d\n",
ret);
goto free_cpumask;
}
@@ -295,7 +299,7 @@ static int dt_cpufreq_early_init(struct device *dev, int cpu)
out:
if (priv->have_static_opps)
dev_pm_opp_of_cpumask_remove_table(priv->cpus);
- dev_pm_opp_put_regulators(priv->opp_table);
+ dev_pm_opp_clear_config(priv->opp_table);
free_cpumask:
free_cpumask_var(priv->cpus);
return ret;
@@ -309,7 +313,7 @@ static void dt_cpufreq_release(void)
dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table);
if (priv->have_static_opps)
dev_pm_opp_of_cpumask_remove_table(priv->cpus);
- dev_pm_opp_put_regulators(priv->opp_table);
+ dev_pm_opp_clear_config(priv->opp_table);
free_cpumask_var(priv->cpus);
list_del(&priv->node);
}
--
2.31.1.272.g89b43f80a514
On 5/29/22 19:19, Dmitry Osipenko wrote:
> On 5/26/22 14:42, Viresh Kumar wrote:
>> The OPP core now provides a unified API for setting all configuration
>> types, i.e. dev_pm_opp_set_config().
>>
>> Lets start using it.
>>
>> Signed-off-by: Viresh Kumar <[email protected]>
>> ---
>> drivers/cpufreq/tegra20-cpufreq.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
>> index e8db3d75be25..2c73623e3abb 100644
>> --- a/drivers/cpufreq/tegra20-cpufreq.c
>> +++ b/drivers/cpufreq/tegra20-cpufreq.c
>> @@ -34,7 +34,7 @@ static bool cpu0_node_has_opp_v2_prop(void)
>>
>> static void tegra20_cpufreq_put_supported_hw(void *opp_table)
>> {
>> - dev_pm_opp_put_supported_hw(opp_table);
>> + dev_pm_opp_clear_config(opp_table);
>> }
>>
>> static void tegra20_cpufreq_dt_unregister(void *cpufreq_dt)
>> @@ -49,6 +49,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>> struct device *cpu_dev;
>> u32 versions[2];
>> int err;
>> + struct dev_pm_opp_config config = {
>> + .supported_hw = versions,
>> + .supported_hw_count = ARRAY_SIZE(versions),
>> + };
>>
>> if (!cpu0_node_has_opp_v2_prop()) {
>> dev_err(&pdev->dev, "operating points not found\n");
>> @@ -71,10 +75,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
>> if (WARN_ON(!cpu_dev))
>> return -ENODEV;
>>
>> - opp_table = dev_pm_opp_set_supported_hw(cpu_dev, versions, 2);
>> - err = PTR_ERR_OR_ZERO(opp_table);
>> + opp_table = dev_pm_opp_set_config(cpu_dev, &config);
>> + err = PTR_ERR(opp_table);
>
> Please keep the PTR_ERR_OR_ZERO.
>
> tegra20-cpufreq tegra20-cpufreq: failed to set OPP config: -1042688000
>
With that fixed, now there is another error:
[ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
[ 1.761960] cpu cpu0: OPP table can't be empty
I see this on Tegra30, but not on Tegra20. Apparently OPP table
refcounting is broken on Tegra30 by this patchset. To make it clear,
there are no error without these OPP patches applied. I may take a
closer look if will be needed, just ping me.
--
Best regards,
Dmitry
On 5/26/22 14:42, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
>
> Lets start using it.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/cpufreq/tegra20-cpufreq.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
> index e8db3d75be25..2c73623e3abb 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -34,7 +34,7 @@ static bool cpu0_node_has_opp_v2_prop(void)
>
> static void tegra20_cpufreq_put_supported_hw(void *opp_table)
> {
> - dev_pm_opp_put_supported_hw(opp_table);
> + dev_pm_opp_clear_config(opp_table);
> }
>
> static void tegra20_cpufreq_dt_unregister(void *cpufreq_dt)
> @@ -49,6 +49,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
> struct device *cpu_dev;
> u32 versions[2];
> int err;
> + struct dev_pm_opp_config config = {
> + .supported_hw = versions,
> + .supported_hw_count = ARRAY_SIZE(versions),
> + };
>
> if (!cpu0_node_has_opp_v2_prop()) {
> dev_err(&pdev->dev, "operating points not found\n");
> @@ -71,10 +75,10 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev)
> if (WARN_ON(!cpu_dev))
> return -ENODEV;
>
> - opp_table = dev_pm_opp_set_supported_hw(cpu_dev, versions, 2);
> - err = PTR_ERR_OR_ZERO(opp_table);
> + opp_table = dev_pm_opp_set_config(cpu_dev, &config);
> + err = PTR_ERR(opp_table);
Please keep the PTR_ERR_OR_ZERO.
tegra20-cpufreq tegra20-cpufreq: failed to set OPP config: -1042688000
--
Best regards,
Dmitry
On 31-05-22, 14:05, Chanwoo Choi wrote:
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 780e525eb92a..8fca24565e7d 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev)
> >
> > dev_pm_opp_of_remove_table(dev);
> > clk_disable_unprepare(bus->clk);
> > - dev_pm_opp_clear_config(bus->opp_table);
> > - bus->opp_table = NULL;
> > +
> > + if (bus->opp_table) {
> > + dev_pm_opp_clear_config(bus->opp_table);
> > + bus->opp_table = NULL;
> > + }
> > }
> >
> > static void exynos_bus_passive_exit(struct device *dev)
> > @@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > dev_pm_opp_of_remove_table(dev);
> > clk_disable_unprepare(bus->clk);
> > err_reg:
> > - dev_pm_opp_clear_config(bus->opp_table);
> > - bus->opp_table = NULL;
> > + if (bus->opp_table) {
> > + dev_pm_opp_clear_config(bus->opp_table);
> > + bus->opp_table = NULL;
> > + }
> >
> > return ret;
> > }
> >
>
> This change is enough to remove the null pointer error. Thanks.
Pushed this and WARN_ON() in OPP core. Thanks.
--
viresh
Hi,
On 5/26/22 8:42 PM, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
>
> Lets start using it.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/devfreq/exynos-bus.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index e689101abc93..780e525eb92a 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -161,7 +161,7 @@ static void exynos_bus_exit(struct device *dev)
>
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> - dev_pm_opp_put_regulators(bus->opp_table);
> + dev_pm_opp_clear_config(bus->opp_table);
> bus->opp_table = NULL;
> }
>
> @@ -182,11 +182,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct opp_table *opp_table;
> const char *vdd = "vdd";
> int i, ret, count, size;
> + struct dev_pm_opp_config config = {
> + .regulator_names = &vdd,
> + .regulator_count = 1,
> + };
>
> - opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> + opp_table = dev_pm_opp_set_config(dev, &config);
> if (IS_ERR(opp_table)) {
> ret = PTR_ERR(opp_table);
> - dev_err(dev, "failed to set regulators %d\n", ret);
> + dev_err(dev, "failed to set OPP config %d\n", ret);
> return ret;
> }
>
> @@ -236,7 +240,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> return 0;
>
> err_regulator:
> - dev_pm_opp_put_regulators(bus->opp_table);
> + dev_pm_opp_clear_config(bus->opp_table);
> bus->opp_table = NULL;
>
> return ret;
> @@ -459,7 +463,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> err_reg:
> - dev_pm_opp_put_regulators(bus->opp_table);
> + dev_pm_opp_clear_config(bus->opp_table);
> bus->opp_table = NULL;
>
> return ret;
>
When I tested them with patch1/2 and patch10/11/12,
I got the following message.
[ 1.083669] Unable to handle kernel NULL pointer dereference at virtual address 000000b4
[ 1.083683] [000000b4] *pgd=00000000
[ 1.083719] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[ 1.083735] Modules linked in:
[ 1.083764] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 5.18.0-11272-g7093c1f2a99c #2
[ 1.083780] Hardware name: Samsung Exynos (Flattened Device Tree)
[ 1.083797] PC is at dev_pm_opp_clear_config+0x10/0x90
[ 1.083823] LR is at exynos_bus_probe+0x384/0x634
[ 1.083843] pc : [<c0933740>] lr : [<c09a33e0>] psr: 60000013
[ 1.083859] sp : f0835d38 ip : c1988000 fp : 00000001
[ 1.083874] r10: c207bc10 r9 : c1b1a580 r8 : c160b708
[ 1.083890] r7 : c207bc00 r6 : c1b1a580 r5 : fffffdfb r4 : 00000000
[ 1.083907] r3 : c1988000 r2 : 00000000 r1 : 00000000 r0 : 00000000
[ 1.083924] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 1.083940] Control: 10c5387d Table: 4000406a DAC: 00000051
[ 1.083955] Register r0 information: NULL pointer
[ 1.083989] Register r1 information: NULL pointer
[ 1.084021] Register r2 information: NULL pointer
[ 1.084053] Register r3 information: slab task_struct start c1988000 pointer offset 0
[ 1.084126] Register r4 information: NULL pointer
[ 1.084157] Register r5 information: non-paged memory
[ 1.084189] Register r6 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64
[ 1.084270] Register r7 information: slab kmalloc-1k start c207bc00 pointer offset 0 size 1024
[ 1.084351] Register r8 information: non-slab/vmalloc memory
[ 1.084383] Register r9 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64
[ 1.084463] Register r10 information: slab kmalloc-1k start c207bc00 pointer offset 16 size 1024
[ 1.084542] Register r11 information: non-paged memory
[ 1.084573] Register r12 information: slab task_struct start c1988000 pointer offset 0
[ 1.084635] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[ 1.084651] Stack: (0xf0835d38 to 0xf0836000)
[ 1.084669] 5d20: c26b3940 c09a33e0
[ 1.084687] 5d40: 00000000 f0835d9c a0000013 cfd97040 df8a6280 c0e031ec c17188f8 c0994758
[ 1.084703] 5d60: 069db9c0 c0e0338c 60000013 c0e03380 c1784000 c0e0338c c17188f8 c0994758
[ 1.084717] 5d80: f0835dc8 c160b708 ffffffff df8a6280 c12de76c c0994bb8 c181fdd4 df8a601c
[ 1.084737] 5da0: 00000000 c160b708 c1718f84 c1765ea0 00000000 000001bb c1550860 c0995684
[ 1.084755] 5dc0: ffffffff 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1.084772] 5de0: 00000000 0f7634d1 00000000 c207bc10 00000000 c1718f84 c1765ea0 00000000
[ 1.084787] 5e00: 000001bb c1550860 c1784000 c061fb8c c207bc10 00000000 c1718f84 c1765ea0
[ 1.084803] 5e20: 00000000 c061d078 c207bc10 c062b764 c207bc10 c207bc10 c1718f84 c1718f84
[ 1.084818] 5e40: c1765ea0 c061d3e0 c1988000 000001bb c1550860 c17cb200 c17cb204 c1718f84
[ 1.084834] 5e60: c207bc10 00000000 000001bb c1550860 c1784000 c061d588 00000000 c207bc10
[ 1.084849] 5e80: c1718f84 c160b708 c1988000 c061dabc 00000000 c1718f84 c061d9d0 c061ac7c
[ 1.084865] 5ea0: c1784000 c2040074 c2054bc0 0f7634d1 c1718f84 c1718f84 cfd92f00 c16edb98
[ 1.084882] 5ec0: 00000000 c061c050 c1367300 c1758040 c1533c54 c1718f84 c1758040 c1533c54
[ 1.084895] 5ee0: 00000000 c061e568 c160b708 c1758040 c1533c54 c0102078 c0191390 c1500504
[ 1.084911] 5f00: c1762f78 00000000 00000006 c132f7c4 c136690c 00000000 00000000 c160b708
[ 1.084927] 5f20: c12a2e5c c1297aec 39360000 c202722b c2027233 0f7634d1 c168112c c1406728
[ 1.084943] 5f40: 00000007 0f7634d1 c1406728 c15a7da0 00000007 c1550840 c2027200 c15014d0
[ 1.084959] 5f60: 00000006 00000006 00000000 c1500504 00000000 c1500504 00000000 c160b700
[ 1.084976] 5f80: c0df8c68 00000000 00000000 00000000 00000000 00000000 00000000 c0df8c88
[ 1.084991] 5fa0: 00000000 c0df8c68 00000000 c0100148 00000000 00000000 00000000 00000000
[ 1.085006] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1.085022] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 1.085042] dev_pm_opp_clear_config from exynos_bus_probe+0x384/0x634
[ 1.085063] exynos_bus_probe from platform_probe+0x64/0xc0
[ 1.085085] platform_probe from really_probe+0x104/0x3c4
[ 1.085109] really_probe from __driver_probe_device+0xa8/0x214
[ 1.085130] __driver_probe_device from driver_probe_device+0x3c/0xdc
[ 1.085149] driver_probe_device from __driver_attach+0xec/0x190
[ 1.085170] __driver_attach from bus_for_each_dev+0x7c/0xbc
[ 1.085191] bus_for_each_dev from bus_add_driver+0x17c/0x218
[ 1.085211] bus_add_driver from driver_register+0x7c/0x110
[ 1.085234] driver_register from do_one_initcall+0x50/0x268
[ 1.085257] do_one_initcall from kernel_init_freeable+0x234/0x280
[ 1.085279] kernel_init_freeable from kernel_init+0x20/0x140
[ 1.085305] kernel_init from ret_from_fork+0x14/0x2c
[ 1.085322] Exception stack(0xf0835fb0 to 0xf0835ff8)
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 5/31/22 1:15 PM, Viresh Kumar wrote:
> On 31-05-22, 13:12, Chanwoo Choi wrote:
>> I try to find the cause of this warning.
>> I think that dev_pm_opp_clear_config needs to check
>> whether 'opp_table' is NULL or not as following:
>>
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index fba6e2b20b8f..cbf8f10b9ff0 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
>> */
>> void dev_pm_opp_clear_config(struct opp_table *opp_table)
>> {
>> + if (unlikely(!opp_table))
>> + return;
>> +
>> if (opp_table->genpd_virt_devs)
>> dev_pm_opp_detach_genpd(opp_table);
>
> Does this fixes it for you ?
>
> It isn't allowed to call this routine with opp_table as NULL, I should
> rather have a WARN() for the same instead.
>
> Can you check why exynos is passing NULL here as I don't see an
> obvious reason currently.
>
exynos-bus.c contains the two type of devfreq device as following:
1. devfreq device controls the regulator
2. devfreq device doesn't control the regulator
Above two devfreq devices share the same error path
because two devices are similar.
As you said, if you use WARN(),
I can change it on exynos-bus.c as following:
if (bus->opp_table)
dev_pm_opp_clear_config(bus->opp_table)
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 29-05-22, 19:59, Dmitry Osipenko wrote:
> > Please keep the PTR_ERR_OR_ZERO.
Ahh, sorry about that. Fixed.
> > tegra20-cpufreq tegra20-cpufreq: failed to set OPP config: -1042688000
>
> With that fixed, now there is another error:
>
> [ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
> [ 1.761960] cpu cpu0: OPP table can't be empty
So we failed to find any OPPs which work with the hardware version of
updated with dev_pm_opp_set_config(). I tried to follow the path and
see if there is something wrong here. Failed to find that :(
> I see this on Tegra30, but not on Tegra20. Apparently OPP table
> refcounting is broken on Tegra30 by this patchset. To make it clear,
> there are no error without these OPP patches applied. I may take a
> closer look if will be needed, just ping me.
Yes, it would be very helpful as I don't have the necessary hardware.
--
viresh
Hi,
On 5/30/22 6:45 PM, Chanwoo Choi wrote:
> Hi,
>
> On 5/26/22 8:42 PM, Viresh Kumar wrote:
>> The OPP core now provides a unified API for setting all configuration
>> types, i.e. dev_pm_opp_set_config().
>>
>> Lets start using it.
>>
>> Signed-off-by: Viresh Kumar <[email protected]>
>> ---
>> drivers/devfreq/exynos-bus.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index e689101abc93..780e525eb92a 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -161,7 +161,7 @@ static void exynos_bus_exit(struct device *dev)
>>
>> dev_pm_opp_of_remove_table(dev);
>> clk_disable_unprepare(bus->clk);
>> - dev_pm_opp_put_regulators(bus->opp_table);
>> + dev_pm_opp_clear_config(bus->opp_table);
>> bus->opp_table = NULL;
>> }
>>
>> @@ -182,11 +182,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> struct opp_table *opp_table;
>> const char *vdd = "vdd";
>> int i, ret, count, size;
>> + struct dev_pm_opp_config config = {
>> + .regulator_names = &vdd,
>> + .regulator_count = 1,
>> + };
>>
>> - opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> + opp_table = dev_pm_opp_set_config(dev, &config);
>> if (IS_ERR(opp_table)) {
>> ret = PTR_ERR(opp_table);
>> - dev_err(dev, "failed to set regulators %d\n", ret);
>> + dev_err(dev, "failed to set OPP config %d\n", ret);
>> return ret;
>> }
>>
>> @@ -236,7 +240,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>> return 0;
>>
>> err_regulator:
>> - dev_pm_opp_put_regulators(bus->opp_table);
>> + dev_pm_opp_clear_config(bus->opp_table);
>> bus->opp_table = NULL;
>>
>> return ret;
>> @@ -459,7 +463,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>> dev_pm_opp_of_remove_table(dev);
>> clk_disable_unprepare(bus->clk);
>> err_reg:
>> - dev_pm_opp_put_regulators(bus->opp_table);
>> + dev_pm_opp_clear_config(bus->opp_table);
>> bus->opp_table = NULL;
>>
>> return ret;
>>
>
> When I tested them with patch1/2 and patch10/11/12,
> I got the following message.
>
> [ 1.083669] Unable to handle kernel NULL pointer dereference at virtual address 000000b4
> [ 1.083683] [000000b4] *pgd=00000000
> [ 1.083719] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> [ 1.083735] Modules linked in:
> [ 1.083764] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 5.18.0-11272-g7093c1f2a99c #2
> [ 1.083780] Hardware name: Samsung Exynos (Flattened Device Tree)
> [ 1.083797] PC is at dev_pm_opp_clear_config+0x10/0x90
> [ 1.083823] LR is at exynos_bus_probe+0x384/0x634
> [ 1.083843] pc : [<c0933740>] lr : [<c09a33e0>] psr: 60000013
> [ 1.083859] sp : f0835d38 ip : c1988000 fp : 00000001
> [ 1.083874] r10: c207bc10 r9 : c1b1a580 r8 : c160b708
> [ 1.083890] r7 : c207bc00 r6 : c1b1a580 r5 : fffffdfb r4 : 00000000
> [ 1.083907] r3 : c1988000 r2 : 00000000 r1 : 00000000 r0 : 00000000
> [ 1.083924] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 1.083940] Control: 10c5387d Table: 4000406a DAC: 00000051
> [ 1.083955] Register r0 information: NULL pointer
> [ 1.083989] Register r1 information: NULL pointer
> [ 1.084021] Register r2 information: NULL pointer
> [ 1.084053] Register r3 information: slab task_struct start c1988000 pointer offset 0
> [ 1.084126] Register r4 information: NULL pointer
> [ 1.084157] Register r5 information: non-paged memory
> [ 1.084189] Register r6 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64
> [ 1.084270] Register r7 information: slab kmalloc-1k start c207bc00 pointer offset 0 size 1024
> [ 1.084351] Register r8 information: non-slab/vmalloc memory
> [ 1.084383] Register r9 information: slab kmalloc-64 start c1b1a580 pointer offset 0 size 64
> [ 1.084463] Register r10 information: slab kmalloc-1k start c207bc00 pointer offset 16 size 1024
> [ 1.084542] Register r11 information: non-paged memory
> [ 1.084573] Register r12 information: slab task_struct start c1988000 pointer offset 0
> [ 1.084635] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
> [ 1.084651] Stack: (0xf0835d38 to 0xf0836000)
> [ 1.084669] 5d20: c26b3940 c09a33e0
> [ 1.084687] 5d40: 00000000 f0835d9c a0000013 cfd97040 df8a6280 c0e031ec c17188f8 c0994758
> [ 1.084703] 5d60: 069db9c0 c0e0338c 60000013 c0e03380 c1784000 c0e0338c c17188f8 c0994758
> [ 1.084717] 5d80: f0835dc8 c160b708 ffffffff df8a6280 c12de76c c0994bb8 c181fdd4 df8a601c
> [ 1.084737] 5da0: 00000000 c160b708 c1718f84 c1765ea0 00000000 000001bb c1550860 c0995684
> [ 1.084755] 5dc0: ffffffff 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 1.084772] 5de0: 00000000 0f7634d1 00000000 c207bc10 00000000 c1718f84 c1765ea0 00000000
> [ 1.084787] 5e00: 000001bb c1550860 c1784000 c061fb8c c207bc10 00000000 c1718f84 c1765ea0
> [ 1.084803] 5e20: 00000000 c061d078 c207bc10 c062b764 c207bc10 c207bc10 c1718f84 c1718f84
> [ 1.084818] 5e40: c1765ea0 c061d3e0 c1988000 000001bb c1550860 c17cb200 c17cb204 c1718f84
> [ 1.084834] 5e60: c207bc10 00000000 000001bb c1550860 c1784000 c061d588 00000000 c207bc10
> [ 1.084849] 5e80: c1718f84 c160b708 c1988000 c061dabc 00000000 c1718f84 c061d9d0 c061ac7c
> [ 1.084865] 5ea0: c1784000 c2040074 c2054bc0 0f7634d1 c1718f84 c1718f84 cfd92f00 c16edb98
> [ 1.084882] 5ec0: 00000000 c061c050 c1367300 c1758040 c1533c54 c1718f84 c1758040 c1533c54
> [ 1.084895] 5ee0: 00000000 c061e568 c160b708 c1758040 c1533c54 c0102078 c0191390 c1500504
> [ 1.084911] 5f00: c1762f78 00000000 00000006 c132f7c4 c136690c 00000000 00000000 c160b708
> [ 1.084927] 5f20: c12a2e5c c1297aec 39360000 c202722b c2027233 0f7634d1 c168112c c1406728
> [ 1.084943] 5f40: 00000007 0f7634d1 c1406728 c15a7da0 00000007 c1550840 c2027200 c15014d0
> [ 1.084959] 5f60: 00000006 00000006 00000000 c1500504 00000000 c1500504 00000000 c160b700
> [ 1.084976] 5f80: c0df8c68 00000000 00000000 00000000 00000000 00000000 00000000 c0df8c88
> [ 1.084991] 5fa0: 00000000 c0df8c68 00000000 c0100148 00000000 00000000 00000000 00000000
> [ 1.085006] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 1.085022] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [ 1.085042] dev_pm_opp_clear_config from exynos_bus_probe+0x384/0x634
> [ 1.085063] exynos_bus_probe from platform_probe+0x64/0xc0
> [ 1.085085] platform_probe from really_probe+0x104/0x3c4
> [ 1.085109] really_probe from __driver_probe_device+0xa8/0x214
> [ 1.085130] __driver_probe_device from driver_probe_device+0x3c/0xdc
> [ 1.085149] driver_probe_device from __driver_attach+0xec/0x190
> [ 1.085170] __driver_attach from bus_for_each_dev+0x7c/0xbc
> [ 1.085191] bus_for_each_dev from bus_add_driver+0x17c/0x218
> [ 1.085211] bus_add_driver from driver_register+0x7c/0x110
> [ 1.085234] driver_register from do_one_initcall+0x50/0x268
> [ 1.085257] do_one_initcall from kernel_init_freeable+0x234/0x280
> [ 1.085279] kernel_init_freeable from kernel_init+0x20/0x140
> [ 1.085305] kernel_init from ret_from_fork+0x14/0x2c
> [ 1.085322] Exception stack(0xf0835fb0 to 0xf0835ff8)
I try to find the cause of this warning.
I think that dev_pm_opp_clear_config needs to check
whether 'opp_table' is NULL or not as following:
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fba6e2b20b8f..cbf8f10b9ff0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
*/
void dev_pm_opp_clear_config(struct opp_table *opp_table)
{
+ if (unlikely(!opp_table))
+ return;
+
if (opp_table->genpd_virt_devs)
dev_pm_opp_detach_genpd(opp_table);
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 5/31/22 1:38 PM, Viresh Kumar wrote:
> On 31-05-22, 09:45, Viresh Kumar wrote:
>> On 31-05-22, 13:12, Chanwoo Choi wrote:
>>> I try to find the cause of this warning.
>>> I think that dev_pm_opp_clear_config needs to check
>>> whether 'opp_table' is NULL or not as following:
>>>
>>>
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index fba6e2b20b8f..cbf8f10b9ff0 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
>>> */
>>> void dev_pm_opp_clear_config(struct opp_table *opp_table)
>>> {
>>> + if (unlikely(!opp_table))
>>> + return;
>>> +
>>> if (opp_table->genpd_virt_devs)
>>> dev_pm_opp_detach_genpd(opp_table);
>>
>> Does this fixes it for you ?
>>
>> It isn't allowed to call this routine with opp_table as NULL, I should
>> rather have a WARN() for the same instead.
>>
>> Can you check why exynos is passing NULL here as I don't see an
>> obvious reason currently.
>
> Looking at the exynos devfreq driver again, it feels like the design
> of the driver itself is causing all these issues.
>
> Ideally, whatever resources are acquired by probe() must be freed only
> and only by remove()/shutdown(). But you are trying to do it from
> exynos_bus_exit() as well. Calling dev_pm_opp_of_remove_table() as
> well from this function is wrong as you may very well end up
> corrupting the OPP refcount and OPP may never get freed or something
> else may come up.
>
> For now I am adding following to the patch, please see if it fixes it
> or not (I have pushed the changes to my branch as well).
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 780e525eb92a..8fca24565e7d 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev)
>
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> - dev_pm_opp_clear_config(bus->opp_table);
> - bus->opp_table = NULL;
> +
> + if (bus->opp_table) {
> + dev_pm_opp_clear_config(bus->opp_table);
> + bus->opp_table = NULL;
> + }
> }
>
> static void exynos_bus_passive_exit(struct device *dev)
> @@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
> dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
> err_reg:
> - dev_pm_opp_clear_config(bus->opp_table);
> - bus->opp_table = NULL;
> + if (bus->opp_table) {
> + dev_pm_opp_clear_config(bus->opp_table);
> + bus->opp_table = NULL;
> + }
>
> return ret;
> }
>
This change is enough to remove the null pointer error. Thanks.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 31-05-22, 09:45, Viresh Kumar wrote:
> On 31-05-22, 13:12, Chanwoo Choi wrote:
> > I try to find the cause of this warning.
> > I think that dev_pm_opp_clear_config needs to check
> > whether 'opp_table' is NULL or not as following:
> >
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index fba6e2b20b8f..cbf8f10b9ff0 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
> > */
> > void dev_pm_opp_clear_config(struct opp_table *opp_table)
> > {
> > + if (unlikely(!opp_table))
> > + return;
> > +
> > if (opp_table->genpd_virt_devs)
> > dev_pm_opp_detach_genpd(opp_table);
>
> Does this fixes it for you ?
>
> It isn't allowed to call this routine with opp_table as NULL, I should
> rather have a WARN() for the same instead.
>
> Can you check why exynos is passing NULL here as I don't see an
> obvious reason currently.
Looking at the exynos devfreq driver again, it feels like the design
of the driver itself is causing all these issues.
Ideally, whatever resources are acquired by probe() must be freed only
and only by remove()/shutdown(). But you are trying to do it from
exynos_bus_exit() as well. Calling dev_pm_opp_of_remove_table() as
well from this function is wrong as you may very well end up
corrupting the OPP refcount and OPP may never get freed or something
else may come up.
For now I am adding following to the patch, please see if it fixes it
or not (I have pushed the changes to my branch as well).
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 780e525eb92a..8fca24565e7d 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -161,8 +161,11 @@ static void exynos_bus_exit(struct device *dev)
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
- dev_pm_opp_clear_config(bus->opp_table);
- bus->opp_table = NULL;
+
+ if (bus->opp_table) {
+ dev_pm_opp_clear_config(bus->opp_table);
+ bus->opp_table = NULL;
+ }
}
static void exynos_bus_passive_exit(struct device *dev)
@@ -463,8 +466,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
dev_pm_opp_of_remove_table(dev);
clk_disable_unprepare(bus->clk);
err_reg:
- dev_pm_opp_clear_config(bus->opp_table);
- bus->opp_table = NULL;
+ if (bus->opp_table) {
+ dev_pm_opp_clear_config(bus->opp_table);
+ bus->opp_table = NULL;
+ }
return ret;
}
--
viresh
On 31-05-22, 13:12, Chanwoo Choi wrote:
> I try to find the cause of this warning.
> I think that dev_pm_opp_clear_config needs to check
> whether 'opp_table' is NULL or not as following:
>
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index fba6e2b20b8f..cbf8f10b9ff0 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2598,6 +2598,9 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_set_config);
> */
> void dev_pm_opp_clear_config(struct opp_table *opp_table)
> {
> + if (unlikely(!opp_table))
> + return;
> +
> if (opp_table->genpd_virt_devs)
> dev_pm_opp_detach_genpd(opp_table);
Does this fixes it for you ?
It isn't allowed to call this routine with opp_table as NULL, I should
rather have a WARN() for the same instead.
Can you check why exynos is passing NULL here as I don't see an
obvious reason currently.
--
viresh
On Thu, 26 May 2022 at 13:44, Viresh Kumar <[email protected]> wrote:
>
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
>
> Lets start using it.
>
> Signed-off-by: Viresh Kumar <[email protected]>
Acked-by: Ulf Hansson <[email protected]>
Kind regards
Uffe
> ---
> drivers/mmc/host/sdhci-msm.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 50c71e0ba5e4..994f3f0231f7 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2496,6 +2496,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> const struct sdhci_msm_offset *msm_offset;
> const struct sdhci_msm_variant_info *var_info;
> struct device_node *node = pdev->dev.of_node;
> + struct dev_pm_opp_config opp_config = {
> + .clk_name = "core",
> + };
>
> host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
> if (IS_ERR(host))
> @@ -2564,7 +2567,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> if (ret)
> goto bus_clk_disable;
>
> - ret = devm_pm_opp_set_clkname(&pdev->dev, "core");
> + ret = devm_pm_opp_set_config(&pdev->dev, &opp_config);
> if (ret)
> goto bus_clk_disable;
>
> --
> 2.31.1.272.g89b43f80a514
>
On 30-05-22, 13:22, Viresh Kumar wrote:
> On 29-05-22, 19:59, Dmitry Osipenko wrote:
> > > Please keep the PTR_ERR_OR_ZERO.
>
> Ahh, sorry about that. Fixed.
>
> > > tegra20-cpufreq tegra20-cpufreq: failed to set OPP config: -1042688000
> >
> > With that fixed, now there is another error:
> >
> > [ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
> > [ 1.761960] cpu cpu0: OPP table can't be empty
>
> So we failed to find any OPPs which work with the hardware version of
> updated with dev_pm_opp_set_config(). I tried to follow the path and
> see if there is something wrong here. Failed to find that :(
>
> > I see this on Tegra30, but not on Tegra20. Apparently OPP table
> > refcounting is broken on Tegra30 by this patchset. To make it clear,
> > there are no error without these OPP patches applied. I may take a
> > closer look if will be needed, just ping me.
>
> Yes, it would be very helpful as I don't have the necessary hardware.
Hey, any updates on this ? I am looking to resend the series soon, would be nice
to fix this before that.
--
viresh
On 6/7/22 11:43, Viresh Kumar wrote:
> On 30-05-22, 13:22, Viresh Kumar wrote:
>> On 29-05-22, 19:59, Dmitry Osipenko wrote:
>>>> Please keep the PTR_ERR_OR_ZERO.
>>
>> Ahh, sorry about that. Fixed.
>>
>>>> tegra20-cpufreq tegra20-cpufreq: failed to set OPP config: -1042688000
>>>
>>> With that fixed, now there is another error:
>>>
>>> [ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
>>> [ 1.761960] cpu cpu0: OPP table can't be empty
>>
>> So we failed to find any OPPs which work with the hardware version of
>> updated with dev_pm_opp_set_config(). I tried to follow the path and
>> see if there is something wrong here. Failed to find that :(
>>
>>> I see this on Tegra30, but not on Tegra20. Apparently OPP table
>>> refcounting is broken on Tegra30 by this patchset. To make it clear,
>>> there are no error without these OPP patches applied. I may take a
>>> closer look if will be needed, just ping me.
>>
>> Yes, it would be very helpful as I don't have the necessary hardware.
>
> Hey, any updates on this ? I am looking to resend the series soon, would be nice
> to fix this before that.
>
I'll take a look over this weekend. Sorry for the delay.
--
Best regards,
Dmitry
Hi,
On 5/26/22 14:42, Viresh Kumar wrote:
> +/**
> + * dev_pm_opp_clear_config() - Releases resources blocked for OPP configuration.
> + * @opp_table: OPP table returned from dev_pm_opp_set_config().
> + *
> + * This allows all device OPP configurations to be cleared at once. This must be
> + * called once for each call made to dev_pm_opp_set_config(), in order to free
> + * the OPPs properly.
> + *
> + * Currently the first call itself ends up freeing all the OPP configurations,
> + * while the later ones only drop the OPP table reference. This works well for
> + * now as we would never want to use an half initialized OPP table and want to
> + * remove the configurations together.
> + */
> +void dev_pm_opp_clear_config(struct opp_table *opp_table)
> +{
> + if (opp_table->genpd_virt_devs)
> + dev_pm_opp_detach_genpd(opp_table);
> +
> + if (opp_table->regulators)
> + dev_pm_opp_put_regulators(opp_table);
> +
> + if (opp_table->supported_hw)
> + dev_pm_opp_put_supported_hw(opp_table);
> +
> + if (opp_table->set_opp)
> + dev_pm_opp_unregister_set_opp_helper(opp_table);
> +
> + if (opp_table->prop_name)
> + dev_pm_opp_put_prop_name(opp_table);
> +
> + if (opp_table->clk_configured)
> + dev_pm_opp_put_clkname(opp_table);
> +
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_clear_config);
1. I started to look at the Tegra regressions caused by these OPP
patches and this one looks wrong to me because dev_pm_opp_set_config()
could be invoked multiple times by different drivers for the same device
and then you're putting table not in accordance to the config that was
used by a particular driver.
For example, if parent tegra-cpufreq driver sets supported_hw for
cpu_dev and then cpufreq-dt also does dev_pm_opp_set_config(cpu_dev),
then dev_pm_opp_clear_config(cpu_dev) of cpufreq-dt will put
supported_hw(cpu_dev) of tegra-cpufreq. Hence this
dev_pm_opp_set/clear_config() approach isn't viable, unless I'm missing
something.
2. Patches aren't bisectable, please make sure that all patches compile
individually and without warnings.
3. There is a new NULL dereference in the recent linux-next on Tegra in
_set_opp() of the gpu/host1x driver. I'll take a closer look at this
crash a bit later.
Unable to handle kernel NULL pointer dereference at virtual address 00000000
[00000000] *pgd=00000000
Internal error: Oops: 80000005 [#1] SMP ARM
Modules linked in:
CPU: 3 PID: 38 Comm: kworker/u8:1 Not tainted
5.19.0-rc3-next-20220621-00013-g0f8bc1c418c4-dirty #18
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
PC is at 0x0
LR is at _set_opp+0x15c/0x414
pc : [<00000000>] lr : [<c0afa928>] psr: 20000013
sp : df989b60 ip : df989b60 fp : df989ba4
r10: 00000000 r9 : c21e4b40 r8 : c2861e34
r7 : c21b3010 r6 : c28a5080 r5 : c2861e00 r4 : 00000000
r3 : 00000000 r2 : c28a5080 r1 : c2861e00 r0 : c21b3010
Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 8000404a DAC: 00000051
Register r0 information: slab kmalloc-1k start c21b3000 pointer offset
16 size 1024
Register r1 information: slab kmalloc-512 start c2861e00 pointer offset
0 size 512
Register r2 information: slab kmalloc-128 start c28a5080 pointer offset
0 size 128
Register r3 information: NULL pointer
Register r4 information: NULL pointer
Register r5 information: slab kmalloc-512 start c2861e00 pointer offset
0 size 512
Register r6 information: slab kmalloc-128 start c28a5080 pointer offset
0 size 128
Register r7 information: slab kmalloc-1k start c21b3000 pointer offset
16 size 1024
Register r8 information: slab kmalloc-512 start c2861e00 pointer offset
52 size 512
Register r9 information: slab task_struct start c21e4b40 pointer offset 0
Register r10 information: NULL pointer
Register r11 information: 2-page vmalloc region starting at 0xdf988000
allocated at kernel_clone+0x64/0x43c
Register r12 information: 2-page vmalloc region starting at 0xdf988000
allocated at kernel_clone+0x64/0x43c
Process kworker/u8:1 (pid: 38, stack limit = 0x(ptrval))
Stack: (0xdf989b60 to 0xdf98a000)
...
Backtrace:
_set_opp from dev_pm_opp_set_opp+0x70/0xd8
r10:00000001 r9:000f4240 r8:c2848440 r7:c2848440 r6:c28a5080 r5:c21b3010
r4:c2861e00
dev_pm_opp_set_opp from tegra_pmc_core_pd_set_performance_state+0x50/0xb8
r6:c2848440 r5:c1807654 r4:c28a5080
tegra_pmc_core_pd_set_performance_state from
_genpd_set_performance_state+0x1fc/0x288
r6:c2848690 r5:c2848680 r4:000f4240
_genpd_set_performance_state from _genpd_set_performance_state+0xb8/0x288
r10:00000001 r9:000f4240 r8:c284a000 r7:c2848440 r6:c284a250 r5:c28a7180
r4:000f4240
_genpd_set_performance_state from genpd_set_performance_state+0xb8/0xd4
r10:c0185b00 r9:c28a7580 r8:00000000 r7:00000000 r6:c284a000 r5:00000000
r4:c28a7580
genpd_set_performance_state from genpd_runtime_resume+0x228/0x29c
r5:c21b3810 r4:c284a1d0
genpd_runtime_resume from __rpm_callback+0x68/0x1a0
r10:c0185b00 r9:00000004 r8:00000000 r7:c08dd55c r6:c2173800 r5:c08dd55c
r4:c21b3810
__rpm_callback from rpm_callback+0x60/0x64
r9:00000004 r8:c21b3894 r7:c08dd55c r6:c2173800 r5:c21e4b40 r4:c21b3810
rpm_callback from rpm_resume+0x608/0x83c
r7:c08dd55c r6:c2173800 r5:c21e4b40 r4:c21b3810
rpm_resume from __pm_runtime_resume+0x58/0xb4
r10:c21e4b40 r9:c21b3810 r8:c21b3800 r7:00000000 r6:c21b3894 r5:60000013
r4:c21b3810
__pm_runtime_resume from host1x_probe+0x48c/0x700
r7:00000000 r6:c2862504 r5:00000000 r4:c2862440
host1x_probe from platform_probe+0x6c/0xcc
r10:c202c00d r9:c21e4b40 r8:00000001 r7:00000000 r6:c1812420 r5:c21b3810
r4:00000000
platform_probe from really_probe+0xd8/0x300
r7:00000000 r6:c1812420 r5:00000000 r4:c21b3810
really_probe from __driver_probe_device+0x94/0xf4
r7:0000000b r6:c21b3810 r5:c1812420 r4:c21b3810
__driver_probe_device from driver_probe_device+0x40/0x114
r5:df989e84 r4:c1901580
driver_probe_device from __device_attach_driver+0xc4/0x108
r9:c21e4b40 r8:00000001 r7:c08c0fb4 r6:c21b3810 r5:df989e84 r4:c1812420
__device_attach_driver from bus_for_each_drv+0x8c/0xd0
r7:c08c0fb4 r6:c21e4b40 r5:df989e84 r4:00000000
bus_for_each_drv from __device_attach+0xb8/0x1e8
r7:c21b3854 r6:c21e4b40 r5:c21b3810 r4:c21b3810
__device_attach from device_initial_probe+0x1c/0x20
r8:c1882620 r7:00000000 r6:c1814e90 r5:c21b3810 r4:c21b3810
device_initial_probe from bus_probe_device+0x94/0x9c
bus_probe_device from deferred_probe_work_func+0x88/0xb4
r7:00000000 r6:c1814c00 r5:c1814bec r4:c21b3810
deferred_probe_work_func from process_one_work+0x21c/0x548
r7:c202c000 r6:c2006a00 r5:c23e8380 r4:c1814c14
process_one_work from worker_thread+0x27c/0x5ac
r10:00000088 r9:c2006a00 r8:c1703d40 r7:c2006a1c r6:c23e8398 r5:c2006a00
r4:c23e8380
worker_thread from kthread+0x100/0x120
r10:00000000 r9:df831e7c r8:c23ed0c0 r7:c23e8380 r6:c014bcfc r5:c23ed000
r4:c21e4b40
kthread from ret_from_fork+0x14/0x2c
Exception stack(0xdf989fb0 to 0xdf989ff8)
9fa0: 00000000 00000000 00000000
00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c01539f4 r4:c23ed000
Code: bad PC value
---[ end trace 0000000000000000 ]---
--
Best regards,
Dmitry
Hi Dmitry,
On 21-06-22, 18:09, Dmitry Osipenko wrote:
> 1. I started to look at the Tegra regressions caused by these OPP
> patches and this one looks wrong to me because dev_pm_opp_set_config()
> could be invoked multiple times by different drivers for the same device
> and then you're putting table not in accordance to the config that was
> used by a particular driver.
>
> For example, if parent tegra-cpufreq driver sets supported_hw for
> cpu_dev and then cpufreq-dt also does dev_pm_opp_set_config(cpu_dev),
> then dev_pm_opp_clear_config(cpu_dev) of cpufreq-dt will put
> supported_hw(cpu_dev) of tegra-cpufreq. Hence this
> dev_pm_opp_set/clear_config() approach isn't viable, unless I'm missing
> something.
Yeah, I know that and I didn't put a lot of effort into it because of multiple
reasons:
- That is partially the existing behavior. For example, we can call the
set-supported-hw interface right now for each CPU of a policy, which many
drivers do right now btw, and then while putting them back we drop the
resource on the first call itself and not on the last CPU.
- Yes, with the new patchset we will drop the resources even for an unrelated
resource call, I will think again about it though and maybe add a flag field
to notify which all resources to clean, but even in the current case it should
be fine as we won't be able to use a half initialized OPP table anyway (which
may actually be harmful). What I mean is, if you set regulators and
supported-hw in the beginning, then on un-init, we won't want to work with
only one of them in place. We always want all of them.
> 2. Patches aren't bisectable, please make sure that all patches compile
> individually and without warnings.
That is strange. I will try build over each and every patch (again). Also I
think the kernel bots (from LKP) test individual patches and I haven't got any
failure messages yet. Which patch broke the build for you ?
> 3. There is a new NULL dereference in the recent linux-next on Tegra in
> _set_opp() of the gpu/host1x driver. I'll take a closer look at this
> crash a bit later.
I just fixed a bug for devices which don't have the clock property, but just
level or bandwidth. Not sure if that is the one that caused trouble for you. It
is pushed to opp/linux-next.
--
viresh
On 21-06-22, 21:04, Viresh Kumar wrote:
> > 2. Patches aren't bisectable, please make sure that all patches compile
> > individually and without warnings.
>
> That is strange. I will try build over each and every patch (again). Also I
> think the kernel bots (from LKP) test individual patches and I haven't got any
> failure messages yet. Which patch broke the build for you ?
Hmm, surprisingly (as I do test this for my patches) one patch was emitting
non-harmful warnings and one broke the build. Fixed them both, the final diff
remains the same though for opp/linux-next.
Thanks for reporting this.
--
viresh
On 24-06-22, 06:18, Viresh Kumar wrote:
> + struct dev_pm_opp_config config = {
> + /*
> + * For some devices we don't have any OPP table in the DT, and
> + * in order to use the same code path for all the devices, we
> + * create a dummy OPP table for them via this. The dummy OPP
> + * table is only capable of doing clk_set_rate() on invocation
> + * of dev_pm_opp_set_rate() and doesn't provide any other
> + * functionality.
> + */
> + .clk_names = NULL,
> + .clk_count = 1,
> + };
Slight modification here, sorry about that. We just need to set the
name as NULL and not the array itself.
diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index cd53e46c4058..6a099d764cce 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -116,7 +116,7 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
* of dev_pm_opp_set_rate() and doesn't provide any other
* functionality.
*/
- .clk_names = NULL,
+ .clk_names = (const char *[]){ NULL },
.clk_count = 1,
};
--
viresh
On 26-05-22, 17:12, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
>
> Lets start using it.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/soc/tegra/common.c | 8 ++++++--
> drivers/soc/tegra/pmc.c | 8 ++++++--
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index 49a5360f4507..7ba15cb836e8 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -107,6 +107,10 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
> {
> u32 hw_version;
> int err;
> + struct dev_pm_opp_config config = {
> + .supported_hw = &hw_version,
> + .supported_hw_count = 1,
> + };
>
> /* Tegra114+ doesn't support OPP yet */
> if (!of_machine_is_compatible("nvidia,tegra20") &&
> @@ -118,9 +122,9 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
> else
> hw_version = BIT(tegra_sku_info.soc_speedo_id);
>
> - err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
> + err = devm_pm_opp_set_config(dev, &config);
> if (err) {
> - dev_err(dev, "failed to set OPP supported HW: %d\n", err);
> + dev_err(dev, "failed to set OPP config: %d\n", err);
> return err;
> }
Jon/Dmitry,
Because of the update [1] to previous patch 21/31, I am updating this
file as (fresh diff):
diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index 9f3fdeb1a11c..cd53e46c4058 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -107,36 +107,42 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
{
u32 hw_version;
int err;
-
- /*
- * For some devices we don't have any OPP table in the DT, and in order
- * to use the same code path for all the devices, we create a dummy OPP
- * table for them via this call. The dummy OPP table is only capable of
- * doing clk_set_rate() on invocation of dev_pm_opp_set_rate() and
- * doesn't provide any other functionality.
- */
- err = devm_pm_opp_set_clkname(dev, NULL);
- if (err) {
- dev_err(dev, "failed to set OPP clk: %d\n", err);
- return err;
- }
-
- /* Tegra114+ doesn't support OPP yet */
- if (!of_machine_is_compatible("nvidia,tegra20") &&
- !of_machine_is_compatible("nvidia,tegra30"))
- return -ENODEV;
-
- if (of_machine_is_compatible("nvidia,tegra20"))
+ struct dev_pm_opp_config config = {
+ /*
+ * For some devices we don't have any OPP table in the DT, and
+ * in order to use the same code path for all the devices, we
+ * create a dummy OPP table for them via this. The dummy OPP
+ * table is only capable of doing clk_set_rate() on invocation
+ * of dev_pm_opp_set_rate() and doesn't provide any other
+ * functionality.
+ */
+ .clk_names = NULL,
+ .clk_count = 1,
+ };
+
+ if (of_machine_is_compatible("nvidia,tegra20")) {
hw_version = BIT(tegra_sku_info.soc_process_id);
- else
+ config.supported_hw = &hw_version;
+ config.supported_hw_count = 1;
+ } else if (of_machine_is_compatible("nvidia,tegra30")) {
hw_version = BIT(tegra_sku_info.soc_speedo_id);
+ config.supported_hw = &hw_version;
+ config.supported_hw_count = 1;
+ }
- err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
+ err = devm_pm_opp_set_config(dev, &config);
if (err) {
- dev_err(dev, "failed to set OPP supported HW: %d\n", err);
+ dev_err(dev, "failed to set OPP config: %d\n", err);
return err;
}
+ /*
+ * Tegra114+ doesn't support OPP yet, return early for non tegra20/30
+ * case.
+ */
+ if (!config.supported_hw)
+ return -ENODEV;
+
/*
* Older device-trees have an empty OPP table, we will get
* -ENODEV from devm_pm_opp_of_add_table() in this case.
-------------------------8<-------------------------
The idea here is to always set the clk name (to NULL) and skip other
stuff for SoCs other than tegra 20/30.
Just see if you can find something odd with the review of it, I will
resend it properly later once the issues are settled.
--
viresh
[1] https://lore.kernel.org/lkml/20220624002805.anv62ufihdrncwus@vireshk-i7/
On 29-05-22, 19:59, Dmitry Osipenko wrote:
> With that fixed, now there is another error:
>
> [ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
> [ 1.761960] cpu cpu0: OPP table can't be empty
>
> I see this on Tegra30, but not on Tegra20. Apparently OPP table
> refcounting is broken on Tegra30 by this patchset. To make it clear,
> there are no error without these OPP patches applied. I may take a
> closer look if will be needed, just ping me.
Hi Jon,
Dmitry reported this on Tegra30 earlier, do you also see such a
failure ? Would be helpful to get this fixed as well, if it still
exists.
--
viresh
On 24/06/2022 06:38, Viresh Kumar wrote:
> On 29-05-22, 19:59, Dmitry Osipenko wrote:
>> With that fixed, now there is another error:
>>
>> [ 1.761945] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
>> [ 1.761960] cpu cpu0: OPP table can't be empty
>>
>> I see this on Tegra30, but not on Tegra20. Apparently OPP table
>> refcounting is broken on Tegra30 by this patchset. To make it clear,
>> there are no error without these OPP patches applied. I may take a
>> closer look if will be needed, just ping me.
>
> Hi Jon,
>
> Dmitry reported this on Tegra30 earlier, do you also see such a
> failure ? Would be helpful to get this fixed as well, if it still
> exists.
Yes I am seeing the same issue on Tegra30 ...
[ 2.177437] cpu cpu0: _of_add_opp_table_v2: no supported OPPs
[ 2.177455] cpu cpu0: OPP table can't be empty
And the cpufreq test we are running is failing.
Thanks
Jon
--
nvpublic
24.06.2022 03:57, Viresh Kumar пишет:
> On 24-06-22, 06:18, Viresh Kumar wrote:
>> + struct dev_pm_opp_config config = {
>> + /*
>> + * For some devices we don't have any OPP table in the DT, and
>> + * in order to use the same code path for all the devices, we
>> + * create a dummy OPP table for them via this. The dummy OPP
>> + * table is only capable of doing clk_set_rate() on invocation
>> + * of dev_pm_opp_set_rate() and doesn't provide any other
>> + * functionality.
>> + */
>> + .clk_names = NULL,
>> + .clk_count = 1,
>> + };
>
> Slight modification here, sorry about that. We just need to set the
> name as NULL and not the array itself.
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index cd53e46c4058..6a099d764cce 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -116,7 +116,7 @@ int devm_tegra_core_dev_init_opp_table(struct device *dev,
> * of dev_pm_opp_set_rate() and doesn't provide any other
> * functionality.
> */
> - .clk_names = NULL,
> + .clk_names = (const char *[]){ NULL },
> .clk_count = 1,
> };
>
Looks okay. If you'll solve the cpufreq problem where OPP config is set
by two drivers for the same cpu device and will keep the set_opp()
helper that is needed by the Tegra 3d driver, then it all should work
for Tegra. Looking forward to the next update of the OPP patches, thank you.
On 27-06-22, 01:14, Dmitry Osipenko wrote:
> Looks okay. If you'll solve the cpufreq problem where OPP config is set
> by two drivers for the same cpu device
This is supported, there is some early freeing of resources on the
removal path though, the reasoning for which I already gave in another
email. Though, I am open to sorting that out as well, but nothing
breaks the code for now AFAICT.
> and will keep the set_opp()
> helper that is needed by the Tegra 3d driver, then it all should work
> for Tegra.
I have responded to that as well on another thread.
> Looking forward to the next update of the OPP patches, thank you.
All that I have is already pushed to linux-next, I don't have any more
changes. Yes I still need to send the updated changes to list.
--
viresh
On 27-06-22, 10:14, Dmitry Osipenko wrote:
> 27.06.2022 09:45, Viresh Kumar пишет:
> >> Looks okay. If you'll solve the cpufreq problem where OPP config is set
> >> by two drivers for the same cpu device
> > This is supported, there is some early freeing of resources on the
> > removal path though, the reasoning for which I already gave in another
> > email. Though, I am open to sorting that out as well, but nothing
> > breaks the code for now AFAICT.
> >
>
> In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and
> registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the
> supported_hw will be lost on the re-probe. I haven't checked yet, but I
> suppose that cpufreq-dt driver defers on Tegra30 because of the CPU
> regulator and that's why we get the "OPP table is missing" error.
Aha, I get it now. I see, this is a real problem. Will fix it. Give me
some time to think. Thanks.
--
viresh
27.06.2022 09:45, Viresh Kumar пишет:
>> Looks okay. If you'll solve the cpufreq problem where OPP config is set
>> by two drivers for the same cpu device
> This is supported, there is some early freeing of resources on the
> removal path though, the reasoning for which I already gave in another
> email. Though, I am open to sorting that out as well, but nothing
> breaks the code for now AFAICT.
>
In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and
registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the
supported_hw will be lost on the re-probe. I haven't checked yet, but I
suppose that cpufreq-dt driver defers on Tegra30 because of the CPU
regulator and that's why we get the "OPP table is missing" error.
On 27-06-22, 12:51, Viresh Kumar wrote:
> On 27-06-22, 10:14, Dmitry Osipenko wrote:
> > 27.06.2022 09:45, Viresh Kumar пишет:
> > >> Looks okay. If you'll solve the cpufreq problem where OPP config is set
> > >> by two drivers for the same cpu device
> > > This is supported, there is some early freeing of resources on the
> > > removal path though, the reasoning for which I already gave in another
> > > email. Though, I am open to sorting that out as well, but nothing
> > > breaks the code for now AFAICT.
> > >
> >
> > In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and
> > registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the
> > supported_hw will be lost on the re-probe. I haven't checked yet, but I
> > suppose that cpufreq-dt driver defers on Tegra30 because of the CPU
> > regulator and that's why we get the "OPP table is missing" error.
>
> Aha, I get it now. I see, this is a real problem. Will fix it. Give me
> some time to think. Thanks.
Okay, I fixed this in opp/linux-next, can you or Jon please give it a
go on tegra30 to see if the issue is fixed ?
FWIW, I have fixed this with the IDR API and the OPP core will only
free the resources in clear-config, that the corresponding set-config
has configured. I have tested it with the clk API only though.
Once you confirm, I will resend all the patches and hope no issues are
left here.
Thanks for helping out guys. Really appreciate it.
--
viresh
On 6/28/22 13:11, Viresh Kumar wrote:
> On 28-06-22, 13:08, Dmitry Osipenko wrote:
>> The opp/linux-next works fine, thank you.
>>
>> Tested-by: Dmitry Osipenko <[email protected]>
>
> Thanks. I should add this to all the core + tegra -patches in that
> branch, right ?
Yes, please.
>> BTW, the idr_alloc() is obsoleted by xa_alloc().
>
> The earlier interface isn't deprecated, right ? I really don't want to
> go change it again :)
>
It has been in a process of deprecation for a couple years now. All IDR
instances are slowly converting to XA. You won't need to take mutex with
xa_alloc().
--
Best regards,
Dmitry
On 28-06-22, 13:08, Dmitry Osipenko wrote:
> The opp/linux-next works fine, thank you.
>
> Tested-by: Dmitry Osipenko <[email protected]>
Thanks. I should add this to all the core + tegra -patches in that
branch, right ?
> BTW, the idr_alloc() is obsoleted by xa_alloc().
The earlier interface isn't deprecated, right ? I really don't want to
go change it again :)
--
viresh
On 6/28/22 10:09, Viresh Kumar wrote:
> On 27-06-22, 12:51, Viresh Kumar wrote:
>> On 27-06-22, 10:14, Dmitry Osipenko wrote:
>>> 27.06.2022 09:45, Viresh Kumar пишет:
>>>>> Looks okay. If you'll solve the cpufreq problem where OPP config is set
>>>>> by two drivers for the same cpu device
>>>> This is supported, there is some early freeing of resources on the
>>>> removal path though, the reasoning for which I already gave in another
>>>> email. Though, I am open to sorting that out as well, but nothing
>>>> breaks the code for now AFAICT.
>>>>
>>>
>>> In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and
>>> registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the
>>> supported_hw will be lost on the re-probe. I haven't checked yet, but I
>>> suppose that cpufreq-dt driver defers on Tegra30 because of the CPU
>>> regulator and that's why we get the "OPP table is missing" error.
>>
>> Aha, I get it now. I see, this is a real problem. Will fix it. Give me
>> some time to think. Thanks.
>
> Okay, I fixed this in opp/linux-next, can you or Jon please give it a
> go on tegra30 to see if the issue is fixed ?
>
> FWIW, I have fixed this with the IDR API and the OPP core will only
> free the resources in clear-config, that the corresponding set-config
> has configured. I have tested it with the clk API only though.
>
> Once you confirm, I will resend all the patches and hope no issues are
> left here.
>
> Thanks for helping out guys. Really appreciate it.
>
The opp/linux-next works fine, thank you.
Tested-by: Dmitry Osipenko <[email protected]>
BTW, the idr_alloc() is obsoleted by xa_alloc().
--
Best regards,
Dmitry
On 28-06-22, 13:16, Dmitry Osipenko wrote:
> It has been in a process of deprecation for a couple years now. All IDR
> instances are slowly converting to XA. You won't need to take mutex with
> xa_alloc().
Okay, migrated to it now and pushed :)
--
viresh
On 28/06/2022 11:08, Dmitry Osipenko wrote:
> On 6/28/22 10:09, Viresh Kumar wrote:
>> On 27-06-22, 12:51, Viresh Kumar wrote:
>>> On 27-06-22, 10:14, Dmitry Osipenko wrote:
>>>> 27.06.2022 09:45, Viresh Kumar пишет:
>>>>>> Looks okay. If you'll solve the cpufreq problem where OPP config is set
>>>>>> by two drivers for the same cpu device
>>>>> This is supported, there is some early freeing of resources on the
>>>>> removal path though, the reasoning for which I already gave in another
>>>>> email. Though, I am open to sorting that out as well, but nothing
>>>>> breaks the code for now AFAICT.
>>>>>
>>>>
>>>> In case of Tegra, we use tegra-cpufreq driver that sets supported_hw and
>>>> registers cpufreq-dt. If cpufreq-dt driver defers the probe, then the
>>>> supported_hw will be lost on the re-probe. I haven't checked yet, but I
>>>> suppose that cpufreq-dt driver defers on Tegra30 because of the CPU
>>>> regulator and that's why we get the "OPP table is missing" error.
>>>
>>> Aha, I get it now. I see, this is a real problem. Will fix it. Give me
>>> some time to think. Thanks.
>>
>> Okay, I fixed this in opp/linux-next, can you or Jon please give it a
>> go on tegra30 to see if the issue is fixed ?
>>
>> FWIW, I have fixed this with the IDR API and the OPP core will only
>> free the resources in clear-config, that the corresponding set-config
>> has configured. I have tested it with the clk API only though.
>>
>> Once you confirm, I will resend all the patches and hope no issues are
>> left here.
>>
>> Thanks for helping out guys. Really appreciate it.
>>
>
> The opp/linux-next works fine, thank you.
>
> Tested-by: Dmitry Osipenko <[email protected]>
Today's -next is also working fine for me too!
Thanks
Jon
--
nvpublic
On 29-06-22, 18:03, Jon Hunter wrote:
> Today's -next is also working fine for me too!
Thanks.
I hope all the trouble with core update was worth it :)
--
viresh