[PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config()

[PATCH V2 00/30] OPP: Add new configuration interface: dev_pm_opp_set_config()

[Viresh Kumar]

Hello,

We have too many configuration specific APIs currently, six of them already,
like dev_pm_opp_set_regulators(). This makes it complex/messy for both the OPP
core and its users to manage. There is also code redundancy in these APIs, in
the way they add/manage the OPP table specific stuff.

This patch series is an attempt to simplify these interfaces by adding a single
interface, dev_pm_opp_set_config(), which replaces all the existing ones. This
series also migrates the users to the new API.

The first two patches help get the API in place, followed by patches to migrate
the end users. Once all the users are migrated, the last few patches remove the
now unused interfaces.

This is pushed here:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next

This is already tested by various folks now.

The entire patchset shall get merged via the OPP tree in 5.20-rc1, please do not
merge individual patches.

V1->V2:
- dev_pm_opp_set_config() doesn't return the OPP table anymore, but a token
  allocated with xa_alloc(). The same needs to be passed to clear-config API.
- Updated all users according to that as well.
- The clk_names interface is updated to allow multiple clocks.
- Converted few // comments to /* */.
- Added tags by few people.
- Dropped the last patch to rearrange stuff, not required anymore.

Thanks.

--
Viresh

Viresh Kumar (30):
  OPP: Track if clock name is configured by platform
  OPP: Add dev_pm_opp_set_config() and friends
  cpufreq: dt: Migrate to dev_pm_opp_set_config()
  cpufreq: imx: Migrate to dev_pm_opp_set_config()
  cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config()
  cpufreq: sti: Migrate to dev_pm_opp_set_config()
  cpufreq: sun50i: Migrate to dev_pm_opp_set_config()
  cpufreq: tegra20: Migrate to dev_pm_opp_set_config()
  cpufreq: ti: Migrate to dev_pm_opp_set_config()
  devfreq: exynos: Migrate to dev_pm_opp_set_config()
  devfreq: sun8i: Migrate to dev_pm_opp_set_config()
  devfreq: tegra30: Migrate to dev_pm_opp_set_config()
  drm/lima: Migrate to dev_pm_opp_set_config()
  drm/msm: Migrate to dev_pm_opp_set_config()
  drm/panfrost: Migrate to dev_pm_opp_set_config()
  drm/tegra: Migrate to dev_pm_opp_set_config()
  media: venus: Migrate to dev_pm_opp_set_config()
  memory: tegra: Migrate to dev_pm_opp_set_config()
  mmc: sdhci-msm: Migrate to dev_pm_opp_set_config()
  OPP: ti: Migrate to dev_pm_opp_set_config()
  soc/tegra: Add comment over devm_pm_opp_set_clkname()
  soc/tegra: Migrate to dev_pm_opp_set_config()
  spi: qcom: Migrate to dev_pm_opp_set_config()
  serial: qcom: Migrate to dev_pm_opp_set_config()
  OPP: Remove dev_pm_opp_set_regulators() and friends
  OPP: Remove dev_pm_opp_set_supported_hw() and friends
  OPP: Remove dev_pm_opp_set_clkname() and friends
  OPP: Remove dev_pm_opp_register_set_opp_helper() and friends
  OPP: Remove dev_pm_opp_attach_genpd() and friends
  OPP: Remove dev_pm_opp_set_prop_name() and friends

 drivers/cpufreq/cpufreq-dt.c                  |  20 +-
 drivers/cpufreq/imx-cpufreq-dt.c              |  18 +-
 drivers/cpufreq/qcom-cpufreq-nvmem.c          | 109 +--
 drivers/cpufreq/sti-cpufreq.c                 |  27 +-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        |  36 +-
 drivers/cpufreq/tegra20-cpufreq.c             |  18 +-
 drivers/cpufreq/ti-cpufreq.c                  |  38 +-
 drivers/devfreq/exynos-bus.c                  |  25 +-
 drivers/devfreq/sun8i-a33-mbus.c              |   8 +-
 drivers/devfreq/tegra30-devfreq.c             |   8 +-
 drivers/gpu/drm/lima/lima_devfreq.c           |  12 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c         |   8 +-
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c         |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   6 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c              |   6 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c            |   6 +-
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |   9 +-
 drivers/gpu/drm/tegra/gr3d.c                  |   6 +-
 .../media/platform/qcom/venus/pm_helpers.c    |  18 +-
 drivers/memory/tegra/tegra124-emc.c           |  17 +-
 drivers/mmc/host/sdhci-msm.c                  |   6 +-
 drivers/opp/core.c                            | 632 ++++++++----------
 drivers/opp/opp.h                             |  23 +
 drivers/opp/ti-opp-supply.c                   |   8 +-
 drivers/soc/tegra/common.c                    |  45 +-
 drivers/soc/tegra/pmc.c                       |   8 +-
 drivers/spi/spi-geni-qcom.c                   |   6 +-
 drivers/spi/spi-qcom-qspi.c                   |   6 +-
 drivers/tty/serial/qcom_geni_serial.c         |   6 +-
 include/linux/pm_opp.h                        | 121 +---
 30 files changed, 605 insertions(+), 661 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 05/30] cpufreq: qcom-nvmem: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 109 +++++++--------------------
 1 file changed, 28 insertions(+), 81 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 6dfa86971a75..863548f59c3e 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -55,9 +55,7 @@ struct qcom_cpufreq_match_data {
 };
 
 struct qcom_cpufreq_drv {
-	struct opp_table **names_opp_tables;
-	struct opp_table **hw_opp_tables;
-	struct opp_table **genpd_opp_tables;
+	int *opp_tokens;
 	u32 versions;
 	const struct qcom_cpufreq_match_data *data;
 };
@@ -315,72 +313,43 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
 	}
 	of_node_put(np);
 
-	drv->names_opp_tables = kcalloc(num_possible_cpus(),
-				  sizeof(*drv->names_opp_tables),
+	drv->opp_tokens = kcalloc(num_possible_cpus(), sizeof(*drv->opp_tokens),
 				  GFP_KERNEL);
-	if (!drv->names_opp_tables) {
+	if (!drv->opp_tokens) {
 		ret = -ENOMEM;
 		goto free_drv;
 	}
-	drv->hw_opp_tables = kcalloc(num_possible_cpus(),
-				  sizeof(*drv->hw_opp_tables),
-				  GFP_KERNEL);
-	if (!drv->hw_opp_tables) {
-		ret = -ENOMEM;
-		goto free_opp_names;
-	}
-
-	drv->genpd_opp_tables = kcalloc(num_possible_cpus(),
-					sizeof(*drv->genpd_opp_tables),
-					GFP_KERNEL);
-	if (!drv->genpd_opp_tables) {
-		ret = -ENOMEM;
-		goto free_opp;
-	}
 
 	for_each_possible_cpu(cpu) {
+		struct dev_pm_opp_config config = {
+			.supported_hw = NULL,
+		};
+
 		cpu_dev = get_cpu_device(cpu);
 		if (NULL == cpu_dev) {
 			ret = -ENODEV;
-			goto free_genpd_opp;
+			goto free_opp;
 		}
 
 		if (drv->data->get_version) {
+			config.supported_hw = &drv->versions;
+			config.supported_hw_count = 1;
 
-			if (pvs_name) {
-				drv->names_opp_tables[cpu] = dev_pm_opp_set_prop_name(
-								     cpu_dev,
-								     pvs_name);
-				if (IS_ERR(drv->names_opp_tables[cpu])) {
-					ret = PTR_ERR(drv->names_opp_tables[cpu]);
-					dev_err(cpu_dev, "Failed to add OPP name %s\n",
-						pvs_name);
-					goto free_opp;
-				}
-			}
-
-			drv->hw_opp_tables[cpu] = dev_pm_opp_set_supported_hw(
-									 cpu_dev, &drv->versions, 1);
-			if (IS_ERR(drv->hw_opp_tables[cpu])) {
-				ret = PTR_ERR(drv->hw_opp_tables[cpu]);
-				dev_err(cpu_dev,
-					"Failed to set supported hardware\n");
-				goto free_genpd_opp;
-			}
+			if (pvs_name)
+				config.prop_name = pvs_name;
 		}
 
 		if (drv->data->genpd_names) {
-			drv->genpd_opp_tables[cpu] =
-				dev_pm_opp_attach_genpd(cpu_dev,
-							drv->data->genpd_names,
-							NULL);
-			if (IS_ERR(drv->genpd_opp_tables[cpu])) {
-				ret = PTR_ERR(drv->genpd_opp_tables[cpu]);
-				if (ret != -EPROBE_DEFER)
-					dev_err(cpu_dev,
-						"Could not attach to pm_domain: %d\n",
-						ret);
-				goto free_genpd_opp;
+			config.genpd_names = drv->data->genpd_names;
+			config.virt_devs = NULL;
+		}
+
+		if (config.supported_hw || config.genpd_names) {
+			drv->opp_tokens[cpu] = dev_pm_opp_set_config(cpu_dev, &config);
+			if (drv->opp_tokens[cpu] < 0) {
+				ret = drv->opp_tokens[cpu];
+				dev_err(cpu_dev, "Failed to set OPP config\n");
+				goto free_opp;
 			}
 		}
 	}
@@ -395,27 +364,10 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
 	ret = PTR_ERR(cpufreq_dt_pdev);
 	dev_err(cpu_dev, "Failed to register platform device\n");
 
-free_genpd_opp:
-	for_each_possible_cpu(cpu) {
-		if (IS_ERR(drv->genpd_opp_tables[cpu]))
-			break;
-		dev_pm_opp_detach_genpd(drv->genpd_opp_tables[cpu]);
-	}
-	kfree(drv->genpd_opp_tables);
 free_opp:
-	for_each_possible_cpu(cpu) {
-		if (IS_ERR(drv->names_opp_tables[cpu]))
-			break;
-		dev_pm_opp_put_prop_name(drv->names_opp_tables[cpu]);
-	}
-	for_each_possible_cpu(cpu) {
-		if (IS_ERR(drv->hw_opp_tables[cpu]))
-			break;
-		dev_pm_opp_put_supported_hw(drv->hw_opp_tables[cpu]);
-	}
-	kfree(drv->hw_opp_tables);
-free_opp_names:
-	kfree(drv->names_opp_tables);
+	for_each_possible_cpu(cpu)
+		dev_pm_opp_clear_config(drv->opp_tokens[cpu]);
+	kfree(drv->opp_tokens);
 free_drv:
 	kfree(drv);
 
@@ -429,15 +381,10 @@ static int qcom_cpufreq_remove(struct platform_device *pdev)
 
 	platform_device_unregister(cpufreq_dt_pdev);
 
-	for_each_possible_cpu(cpu) {
-		dev_pm_opp_put_supported_hw(drv->names_opp_tables[cpu]);
-		dev_pm_opp_put_supported_hw(drv->hw_opp_tables[cpu]);
-		dev_pm_opp_detach_genpd(drv->genpd_opp_tables[cpu]);
-	}
+	for_each_possible_cpu(cpu)
+		dev_pm_opp_clear_config(drv->opp_tokens[cpu]);
 
-	kfree(drv->names_opp_tables);
-	kfree(drv->hw_opp_tables);
-	kfree(drv->genpd_opp_tables);
+	kfree(drv->opp_tokens);
 	kfree(drv);
 
 	return 0;
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 14/30] drm/msm: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   |  8 ++++++--
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 10 +++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  6 +++++-
 drivers/gpu/drm/msm/dp/dp_ctrl.c        |  6 +++++-
 drivers/gpu/drm/msm/dsi/dsi_host.c      |  6 +++++-
 5 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c424e9a37669..6ebb5a28c501 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1723,10 +1723,14 @@ static void check_speed_bin(struct device *dev)
 {
 	struct nvmem_cell *cell;
 	u32 val;
+	struct dev_pm_opp_config config = {
+		.supported_hw = &val,
+		.supported_hw_count = 1,
+	};
 
 	/*
 	 * If the OPP table specifies a opp-supported-hw property then we have
-	 * to set something with dev_pm_opp_set_supported_hw() or the table
+	 * to set something with dev_pm_opp_set_config() or the table
 	 * doesn't get populated so pick an arbitrary value that should
 	 * ensure the default frequencies are selected but not conflict with any
 	 * actual bins
@@ -1748,7 +1752,7 @@ static void check_speed_bin(struct device *dev)
 		nvmem_cell_put(cell);
 	}
 
-	devm_pm_opp_set_supported_hw(dev, &val, 1);
+	devm_pm_opp_set_config(dev, &config);
 }
 
 struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 42ed9a3c4905..82801311f7d4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1800,6 +1800,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct adreno_rev rev)
 	u32 supp_hw = UINT_MAX;
 	u32 speedbin;
 	int ret;
+	struct dev_pm_opp_config config = {
+		.supported_hw = &supp_hw,
+		.supported_hw_count = 1,
+	};
 
 	ret = adreno_read_speedbin(dev, &speedbin);
 	/*
@@ -1818,11 +1822,7 @@ static int a6xx_set_supported_hw(struct device *dev, struct adreno_rev rev)
 	supp_hw = fuse_to_supp_hw(dev, rev, speedbin);
 
 done:
-	ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
-	if (ret)
-		return ret;
-
-	return 0;
+	return devm_pm_opp_set_config(dev, &config);
 }
 
 static const struct adreno_gpu_funcs funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e23e2552e802..2213ce52d2fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1215,12 +1215,16 @@ static int dpu_kms_init(struct drm_device *ddev)
 	struct dev_pm_opp *opp;
 	int ret = 0;
 	unsigned long max_freq = ULONG_MAX;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "core" },
+		.clk_count = 1,
+	};
 
 	dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL);
 	if (!dpu_kms)
 		return -ENOMEM;
 
-	ret = devm_pm_opp_set_clkname(dev, "core");
+	ret = devm_pm_opp_set_config(dev, &config);
 	if (ret)
 		return ret;
 	/* OPP table is optional */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index b7f5b8d3bbd6..0c8fc151b4be 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -2022,6 +2022,10 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
 {
 	struct dp_ctrl_private *ctrl;
 	int ret;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "ctrl_link" },
+		.clk_count = 1,
+	};
 
 	if (!dev || !panel || !aux ||
 	    !link || !catalog) {
@@ -2035,7 +2039,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	ret = devm_pm_opp_set_clkname(dev, "ctrl_link");
+	ret = devm_pm_opp_set_config(dev, &config);
 	if (ret) {
 		dev_err(dev, "invalid DP OPP table in device tree\n");
 		/* caller do PTR_ERR(opp_table) */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a95d5df52653..35b6722d1cf9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2034,6 +2034,10 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 	struct msm_dsi_host *msm_host = NULL;
 	struct platform_device *pdev = msm_dsi->pdev;
 	int ret;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "byte" },
+		.clk_count = 1,
+	};
 
 	msm_host = devm_kzalloc(&pdev->dev, sizeof(*msm_host), GFP_KERNEL);
 	if (!msm_host) {
@@ -2095,7 +2099,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		goto fail;
 	}
 
-	ret = devm_pm_opp_set_clkname(&pdev->dev, "byte");
+	ret = devm_pm_opp_set_config(&pdev->dev, &config);
 	if (ret)
 		return ret;
 	/* OPP table is optional */
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 17/30] media: venus: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index cb48c5ff3dee..f68cc938ebff 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -294,12 +294,16 @@ static int load_scale_v1(struct venus_inst *inst)
 static int core_get_v1(struct venus_core *core)
 {
 	int ret;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "core" },
+		.clk_count = 1,
+	};
 
 	ret = core_clks_get(core);
 	if (ret)
 		return ret;
 
-	ret = devm_pm_opp_set_clkname(core->dev, "core");
+	ret = devm_pm_opp_set_config(core->dev, &config);
 	if (ret)
 		return ret;
 
@@ -862,6 +866,10 @@ static int vcodec_domains_get(struct venus_core *core)
 	const struct venus_resources *res = core->res;
 	struct device *pd;
 	unsigned int i;
+	struct dev_pm_opp_config config = {
+		.genpd_names = res->opp_pmdomain,
+		.virt_devs = &opp_virt_dev,
+	};
 
 	if (!res->vcodec_pmdomains_num)
 		goto skip_pmdomains;
@@ -879,7 +887,7 @@ static int vcodec_domains_get(struct venus_core *core)
 		return 0;
 
 	/* Attach the power domain for setting performance state */
-	ret = devm_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
+	ret = devm_pm_opp_set_config(dev, &config);
 	if (ret)
 		goto opp_attach_err;
 
@@ -978,6 +986,10 @@ static int core_get_v4(struct venus_core *core)
 	struct device *dev = core->dev;
 	const struct venus_resources *res = core->res;
 	int ret;
+	struct dev_pm_opp_config config = {
+		.clk_names = (const char *[]){ "core" },
+		.clk_count = 1,
+	};
 
 	ret = core_clks_get(core);
 	if (ret)
@@ -1003,7 +1015,7 @@ static int core_get_v4(struct venus_core *core)
 	if (legacy_binding)
 		return 0;
 
-	ret = devm_pm_opp_set_clkname(dev, "core");
+	ret = devm_pm_opp_set_config(dev, &config);
 	if (ret)
 		return ret;
 
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 19/30] mmc: sdhci-msm: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Ulf, I have kept your Ack as the diff is really small, clk_names is an array
now.

 drivers/mmc/host/sdhci-msm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e395411fb6fd..a018b45c5a9a 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2559,6 +2559,10 @@ 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_names = (const char *[]){ "core" },
+		.clk_count = 1,
+	};
 
 	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
@@ -2631,7 +2635,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

[PATCH V2 23/30] spi: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

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 <broonie@kernel.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Mark, I have kept your Ack as the diff is really small, clk_names is an array
now.

 drivers/spi/spi-geni-qcom.c | 6 +++++-
 drivers/spi/spi-qcom-qspi.c | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 4e83cc5b445d..428585bae6a8 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -892,6 +892,10 @@ 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_names = (const char *[]){ "se" },
+		.clk_count = 1,
+	};
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -922,7 +926,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..7e7732d61b25 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -458,6 +458,10 @@ 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_names = (const char *[]){ "core" },
+		.clk_count = 1,
+	};
 
 	dev = &pdev->dev;
 
@@ -529,7 +533,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

[PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 4733a233bd0c..ab667838d082 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1347,6 +1347,10 @@ 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_names = (const char *[]){ "se" },
+		.clk_count = 1,
+	};
 
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
 		console = true;
@@ -1430,7 +1434,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

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 01:50:19PM +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.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4733a233bd0c..ab667838d082 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1347,6 +1347,10 @@ 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_names = (const char *[]){ "se" },
> +		.clk_count = 1,
> +	};
>  
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
>  		console = true;
> @@ -1430,7 +1434,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);

This feels like a step back.  This is much harder now, what's wrong with
devm_pm_opp_set_clkname() as is?

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 10:44, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > +	struct dev_pm_opp_config config = {
> > +		.clk_names = (const char *[]){ "se" },
> > +		.clk_count = 1,
> > +	};
> >  
> > -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> > +	ret = devm_pm_opp_set_config(&pdev->dev, &config);
> 
> This feels like a step back.  This is much harder now, what's wrong with
> devm_pm_opp_set_clkname() as is?

Hi Greg,

There are a number of configurations one can do for a device's OPP
table currently:

- clk, single or multiple (new)
- helper to configure multiple clocks (for multiple clocks)
- supplies or regulators
- helper to configure supplies (for multiple supplies)
- OPP supported-hw property
- OPP Property-name
- Genpd specific one
- etc

One problem was that it was a mess within the OPP core with a separate
interface for each of these interfaces, almost duplicate code, etc.

But then it was a bigger mess for the user drivers that need to manage
a few of these. They were required to call multiple APIs, with all the
interfaces returning tokens, which the callers need to save and supply
back to free the resources later. More code, hard to manage, easy to
abuse and add bugs to.

The new interface makes it easier and clean for everyone and allows
easy upgrades of interfaces in future. Adding a new interface, like
support for multiple clocks for a device that I just did, is much
easier now.

I really believe this is a step in the right direction :)

-- 
viresh

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 02:54:58PM +0530, Viresh Kumar wrote:
> On 01-07-22, 10:44, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > +	struct dev_pm_opp_config config = {
> > > +		.clk_names = (const char *[]){ "se" },
> > > +		.clk_count = 1,
> > > +	};
> > >  
> > > -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> > > +	ret = devm_pm_opp_set_config(&pdev->dev, &config);
> > 
> > This feels like a step back.  This is much harder now, what's wrong with
> > devm_pm_opp_set_clkname() as is?
> 
> Hi Greg,
> 
> There are a number of configurations one can do for a device's OPP
> table currently:
> 
> - clk, single or multiple (new)
> - helper to configure multiple clocks (for multiple clocks)
> - supplies or regulators
> - helper to configure supplies (for multiple supplies)
> - OPP supported-hw property
> - OPP Property-name
> - Genpd specific one
> - etc
> 
> One problem was that it was a mess within the OPP core with a separate
> interface for each of these interfaces, almost duplicate code, etc.
> 
> But then it was a bigger mess for the user drivers that need to manage
> a few of these. They were required to call multiple APIs, with all the
> interfaces returning tokens, which the callers need to save and supply
> back to free the resources later. More code, hard to manage, easy to
> abuse and add bugs to.
> 
> The new interface makes it easier and clean for everyone and allows
> easy upgrades of interfaces in future. Adding a new interface, like
> support for multiple clocks for a device that I just did, is much
> easier now.
> 
> I really believe this is a step in the right direction :)

It's now more complex for simple drivers like this, right?  Why not
provide translations of the devm_pm_opp_set_clkname() to use internally
devm_pm_opp_set_config() if you want to do complex things, allowing you
to continue to do simple things without the overhead of a driver having
to create a structure on the stack and remember how the "const char *[]"
syntax looks like (seriously, that's crazy).

Make it simple for simple things, and provide the ability to do complex
things only if that is required.

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 11:39, Greg Kroah-Hartman wrote:
> It's now more complex for simple drivers like this, right?

They need to add a structure, yes.

> Why not
> provide translations of the devm_pm_opp_set_clkname() to use internally
> devm_pm_opp_set_config() if you want to do complex things,

That can be done, yes.

> allowing you
> to continue to do simple things without the overhead of a driver having
> to create a structure on the stack

I didn't think of it as complexity, and I still feel it is okay-ish.

> and remember how the "const char *[]"
> syntax looks like (seriously, that's crazy).

The syntax can be fixed, if we want, by avoiding the cast with
something like this:

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index a018b45c5a9a..1a5480214a43 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2559,8 +2559,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;
+       const char *clks[] = { "core" };
        struct dev_pm_opp_config opp_config = {
-               .clk_names = (const char *[]){ "core" },
+               .clk_names = clks,
                .clk_count = 1,
        };

> Make it simple for simple things, and provide the ability to do complex
> things only if that is required.

I still feel it isn't too bad for simple cases right now too, it is
just a structure to fill out but I don't have hard feelings for
keeping the old API around. I just feel it isn't too helpful to keep
the old interfaces around, it will just confuse people at the best.

Anyway, I will keep them around.

-- 
viresh

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> On 01-07-22, 11:39, Greg Kroah-Hartman wrote:
> > It's now more complex for simple drivers like this, right?
> 
> They need to add a structure, yes.
> 
> > Why not
> > provide translations of the devm_pm_opp_set_clkname() to use internally
> > devm_pm_opp_set_config() if you want to do complex things,
> 
> That can be done, yes.
> 
> > allowing you
> > to continue to do simple things without the overhead of a driver having
> > to create a structure on the stack
> 
> I didn't think of it as complexity, and I still feel it is okay-ish.
> 
> > and remember how the "const char *[]"
> > syntax looks like (seriously, that's crazy).
> 
> The syntax can be fixed, if we want, by avoiding the cast with
> something like this:
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index a018b45c5a9a..1a5480214a43 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2559,8 +2559,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;
> +       const char *clks[] = { "core" };
>         struct dev_pm_opp_config opp_config = {
> -               .clk_names = (const char *[]){ "core" },
> +               .clk_names = clks,
>                 .clk_count = 1,
>         };

Still crazy, but a bit better.

Why do you need the clk_count?  A null terminated list is better, as the
compiler can do it for you and you do not have to keep things in sync
like you are expecting people to be forced to do now.

> > Make it simple for simple things, and provide the ability to do complex
> > things only if that is required.
> 
> I still feel it isn't too bad for simple cases right now too, it is
> just a structure to fill out but I don't have hard feelings for
> keeping the old API around. I just feel it isn't too helpful to keep
> the old interfaces around, it will just confuse people at the best.

The above is much more complex than a simple function call to make.
Remember to make it very simple for driver authors, and more
importantly, reviewers.

> Anyway, I will keep them around.

Thanks, and drop the count field please.

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 12:18, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> Still crazy, but a bit better.

:)

> Why do you need the clk_count?  A null terminated list is better,

Because I am not a big fan of the null terminated lists :)

I had to chase a bug once where someone removed that NULL at the end
and it was a nightmare to understand what's going on.

> as the
> compiler can do it for you and you do not have to keep things in sync
> like you are expecting people to be forced to do now.

I am not sure I understand what the compiler can do for us here.

The users will be required to do this here, isn't it ?

        const char *clks[] = { "core", NULL };
        struct dev_pm_opp_config opp_config = {
               .clk_names = clks,
        };


> The above is much more complex than a simple function call to make.
> Remember to make it very simple for driver authors, and more
> importantly, reviewers.

Hmm.

> Thanks, and drop the count field please.

There is one case at least [1] where we actually have to pass NULL in
the clk name. This is basically to allow the same code to run on
different devices, one where an OPP table is present and one where it
isn't. We don't want to do clk_set_rate() for the second case but just
use dev_pm_opp_set_rate() (which does a lot of stuff apart from just
clk).

-- 
viresh

[1] https://lore.kernel.org/lkml/b19a02422cae2408f953b92ae3c46a37fba688a3.1656660185.git.viresh.kumar@linaro.org/

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman]

On Fri, Jul 01, 2022 at 03:59:26PM +0530, Viresh Kumar wrote:
> On 01-07-22, 12:18, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> > Still crazy, but a bit better.
> 
> :)
> 
> > Why do you need the clk_count?  A null terminated list is better,
> 
> Because I am not a big fan of the null terminated lists :)
> 
> I had to chase a bug once where someone removed that NULL at the end
> and it was a nightmare to understand what's going on.

But that's the "normal" way the kernel does things.  Trying to keep a
count in sync with a list is a pain, and just gets harder and harder
over time.  Make it a null-terminated list so that the cpu makes this
always work and prevents errors.

> > as the
> > compiler can do it for you and you do not have to keep things in sync
> > like you are expecting people to be forced to do now.
> 
> I am not sure I understand what the compiler can do for us here.
> 
> The users will be required to do this here, isn't it ?
> 
>         const char *clks[] = { "core", NULL };
>         struct dev_pm_opp_config opp_config = {
>                .clk_names = clks,
>         };
> 

The "in sync" is the count issue.  Don't force humans to count up the
number of items in a list please.

> > The above is much more complex than a simple function call to make.
> > Remember to make it very simple for driver authors, and more
> > importantly, reviewers.
> 
> Hmm.
> 
> > Thanks, and drop the count field please.
> 
> There is one case at least [1] where we actually have to pass NULL in
> the clk name. This is basically to allow the same code to run on
> different devices, one where an OPP table is present and one where it
> isn't. We don't want to do clk_set_rate() for the second case but just
> use dev_pm_opp_set_rate() (which does a lot of stuff apart from just
> clk).

That feels completely wrong, don't have NULL for a name, make a fake name
or something.  Don't make all users in the kernel have a horrible
interface just for one piece of broken hardware out there.

Worst case, name it "".

thanks,

greg k-h

Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Viresh Kumar]

On 01-07-22, 12:41, Greg Kroah-Hartman wrote:
> That feels completely wrong, don't have NULL for a name, make a fake name
> or something.  Don't make all users in the kernel have a horrible
> interface just for one piece of broken hardware out there.
> 
> Worst case, name it "".

This name goes into the second argument of clk_get(dev, const char *con_id);

I will see how else I should hack it :)

-- 
viresh