[PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Rajendra Nayak]

Changes in v6:
1. rebased on 5.8-rc1, no functional change. 

Changes in v5:
1. Opp cleanup path fixed up across drivers

Changes in v4:
1. Fixed all review feedback on v3
2. Dropped the dts patches, will post as a seperate series once
driver changes are reviewed and merged.
The driver changes without DT updates to include OPP tables will
have zero functional change.
3. Dropped the mmc/sdhc patch, which is a standalone patch. will
repost if needed seperately.

Changes in v3:
1. Added better error handling for dev_pm_opp_of_add_table()
2. Some minor changes and fixes in 'PATCH 12/17' as compared to v2
3. Dropped the mmc patch picked up by Ulf [2]

Changes in v2:
1. Added error handling for dev_pm_opp_set_clkname()
and dev_pm_opp_of_add_table()
2. Used dev_pm_opp_put_clkname() in the cleanup path
3. Dropped the OPP patch pulled in by Viresh [1]
4. Dropped the UFS patches since they had some major rework
needed because of changes that were merged in the merge window
and I don't have a UFS device currently to validate the changes.

We have had support added in the OPP core for a while now to support
DVFS for IO devices, and this series uses that infrastructure to
add DVFS support for various IO devices in sdm845 and sc7180 SoCs.

[1] https://lkml.org/lkml/2020/4/14/98
[2] https://lore.kernel.org/patchwork/patch/1226381/

Rajendra Nayak (6):
  tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
  spi: spi-geni-qcom: Use OPP API to set clk/perf state
  drm/msm/dpu: Use OPP API to set clk/perf state
  drm/msm: dsi: Use OPP API to set clk/perf state
  media: venus: core: Add support for opp tables/perf voting
  spi: spi-qcom-qspi: Use OPP API to set clk/perf state

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c  |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 26 +++++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h        |  4 ++
 drivers/gpu/drm/msm/dsi/dsi.h                  |  2 +
 drivers/gpu/drm/msm/dsi/dsi_cfg.c              |  4 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c             | 58 ++++++++++++++++++++++++++
 drivers/media/platform/qcom/venus/core.c       | 43 ++++++++++++++++---
 drivers/media/platform/qcom/venus/core.h       |  5 +++
 drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++--
 drivers/spi/spi-geni-qcom.c                    | 26 ++++++++++--
 drivers/spi/spi-qcom-qspi.c                    | 28 ++++++++++++-
 drivers/tty/serial/qcom_geni_serial.c          | 34 ++++++++++++---
 include/linux/qcom-geni-se.h                   |  4 ++
 13 files changed, 268 insertions(+), 23 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

geni serial needs to express a perforamnce state requirement on CX
powerdomain depending on the frequency of the clock rates.
Use OPP table from DT to register with OPP framework and use
dev_pm_opp_set_rate() to set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: linux-serial@vger.kernel.org
---
This patch needs to land via the msm tree. Greg had this already pulled in,
but later dropped it on my request.
No change in v6, just resposting it here so Bjorn/Andy can pull it in.

 drivers/tty/serial/qcom_geni_serial.c | 34 +++++++++++++++++++++++++++++-----
 include/linux/qcom-geni-se.h          |  4 ++++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 457c0bf..a90f8ec 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pm_opp.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_wakeirq.h>
@@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		goto out_restart_rx;
 
 	uport->uartclk = clk_rate;
-	clk_set_rate(port->se.clk, clk_rate);
+	dev_pm_opp_set_rate(uport->dev, clk_rate);
 	ser_clk_cfg = SER_CLK_EN;
 	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
 
@@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
 	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
 		geni_se_resources_on(&port->se);
 	else if (new_state == UART_PM_STATE_OFF &&
-			old_state == UART_PM_STATE_ON)
+			old_state == UART_PM_STATE_ON) {
+		/* Drop the performance state vote */
+		dev_pm_opp_set_rate(uport->dev, 0);
 		geni_se_resources_off(&port->se);
+	}
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
@@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
 		port->cts_rts_swap = true;
 
+	port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se");
+	if (IS_ERR(port->se.opp_table))
+		return PTR_ERR(port->se.opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (!ret) {
+		port->se.has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
 	uport->private_data = drv;
 	platform_set_drvdata(pdev, port);
 	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
 
 	ret = uart_add_one_port(drv, uport);
 	if (ret)
-		return ret;
+		goto err;
 
 	irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
 	ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
@@ -1365,7 +1381,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
 		uart_remove_one_port(drv, uport);
-		return ret;
+		goto err;
 	}
 
 	/*
@@ -1382,11 +1398,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 		if (ret) {
 			device_init_wakeup(&pdev->dev, false);
 			uart_remove_one_port(drv, uport);
-			return ret;
+			goto err;
 		}
 	}
 
 	return 0;
+err:
+	if (port->se.has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(port->se.opp_table);
+	return ret;
 }
 
 static int qcom_geni_serial_remove(struct platform_device *pdev)
@@ -1394,6 +1415,9 @@ static int qcom_geni_serial_remove(struct platform_device *pdev)
 	struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
 	struct uart_driver *drv = port->uport.private_data;
 
+	if (port->se.has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(port->se.opp_table);
 	dev_pm_clear_wake_irq(&pdev->dev);
 	device_init_wakeup(&pdev->dev, false);
 	uart_remove_one_port(drv, &port->uport);
diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index dd46494..6b78094 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -33,6 +33,8 @@ struct clk;
  * @clk:		Handle to the core serial engine clock
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
+ * @opp_table:		Pointer to the OPP table
+ * @has_opp_table:	Specifies if the SE has an OPP table
  */
 struct geni_se {
 	void __iomem *base;
@@ -41,6 +43,8 @@ struct geni_se {
 	struct clk *clk;
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
+	struct opp_table *opp_table;
+	bool has_opp_table;
 };
 
 /* Common SE registers */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 2/6] spi: spi-geni-qcom: Use OPP API to set clk/perf state

[Rajendra Nayak]

geni spi needs to express a perforamnce state requirement on CX
depending on the frequency of the clock rates. Use OPP table from
DT to register with OPP framework and use dev_pm_opp_set_rate() to
set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Mark Brown <broonie@kernel.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: linux-spi@vger.kernel.org
---
This patch needs to land via the msm tree. Mark has acked v5, so
this is good to land I think. v6 is just rebased on 5.8-rc1.

 drivers/spi/spi-geni-qcom.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index c397242..0d7ead1 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -7,6 +7,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/qcom-geni-se.h>
 #include <linux/spi/spi.h>
@@ -95,7 +96,6 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
 {
 	unsigned long sclk_freq;
 	unsigned int actual_hz;
-	struct geni_se *se = &mas->se;
 	int ret;
 
 	ret = geni_se_clk_freq_match(&mas->se,
@@ -112,9 +112,9 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
 
 	dev_dbg(mas->dev, "req %u=>%u sclk %lu, idx %d, div %d\n", speed_hz,
 				actual_hz, sclk_freq, *clk_idx, *clk_div);
-	ret = clk_set_rate(se->clk, sclk_freq);
+	ret = dev_pm_opp_set_rate(mas->dev, sclk_freq);
 	if (ret)
-		dev_err(mas->dev, "clk_set_rate failed %d\n", ret);
+		dev_err(mas->dev, "dev_pm_opp_set_rate failed %d\n", ret);
 	return ret;
 }
 
@@ -561,6 +561,17 @@ static int spi_geni_probe(struct platform_device *pdev)
 	mas->se.wrapper = dev_get_drvdata(dev->parent);
 	mas->se.base = base;
 	mas->se.clk = clk;
+	mas->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se");
+	if (IS_ERR(mas->se.opp_table))
+		return PTR_ERR(mas->se.opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (!ret) {
+		mas->se.has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
 
 	spi->bus_num = -1;
 	spi->dev.of_node = dev->of_node;
@@ -596,6 +607,9 @@ static int spi_geni_probe(struct platform_device *pdev)
 spi_geni_probe_runtime_disable:
 	pm_runtime_disable(dev);
 	spi_master_put(spi);
+	if (mas->se.has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(mas->se.opp_table);
 	return ret;
 }
 
@@ -609,6 +623,9 @@ static int spi_geni_remove(struct platform_device *pdev)
 
 	free_irq(mas->irq, spi);
 	pm_runtime_disable(&pdev->dev);
+	if (mas->se.has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(mas->se.opp_table);
 	return 0;
 }
 
@@ -617,6 +634,9 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
 	struct spi_master *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
 
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(dev, 0);
+
 	return geni_se_resources_off(&mas->se);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 3/6] drm/msm/dpu: Use OPP API to set clk/perf state

[Rajendra Nayak]

On some qualcomm platforms DPU needs to express a performance state
requirement on a power domain depending on the clock rates.
Use OPP table from DT to register with OPP framework and use
dev_pm_opp_set_rate() to set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: dri-devel@lists.freedesktop.org
---
No functional change in v6, rebased over 5.8-rc1

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c |  3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 26 +++++++++++++++++++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  4 ++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7c230f7..b36919d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -7,6 +7,7 @@
 #include <linux/debugfs.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/pm_opp.h>
 #include <linux/sort.h>
 #include <linux/clk.h>
 #include <linux/bitmap.h>
@@ -218,7 +219,7 @@ static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
 		rate = core_clk->max_rate;
 
 	core_clk->rate = rate;
-	return msm_dss_clk_set_rate(core_clk, 1);
+	return dev_pm_opp_set_rate(&kms->pdev->dev, core_clk->rate);
 }
 
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index b8615d4..0bc8ec4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -10,6 +10,7 @@
 #include <linux/debugfs.h>
 #include <linux/dma-buf.h>
 #include <linux/of_irq.h>
+#include <linux/pm_opp.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_file.h>
@@ -1025,11 +1026,23 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 	if (!dpu_kms)
 		return -ENOMEM;
 
+	dpu_kms->opp_table = dev_pm_opp_set_clkname(dev, "core");
+	if (IS_ERR(dpu_kms->opp_table))
+		return PTR_ERR(dpu_kms->opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(dev);
+	if (!ret) {
+		dpu_kms->has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
 	mp = &dpu_kms->mp;
 	ret = msm_dss_parse_clock(pdev, mp);
 	if (ret) {
 		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
-		return ret;
+		goto err;
 	}
 
 	platform_set_drvdata(pdev, dpu_kms);
@@ -1043,6 +1056,11 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 
 	priv->kms = &dpu_kms->base;
 	return ret;
+err:
+	if (dpu_kms->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(dpu_kms->opp_table);
+	return ret;
 }
 
 static void dpu_unbind(struct device *dev, struct device *master, void *data)
@@ -1057,6 +1075,10 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
 
 	if (dpu_kms->rpm_enabled)
 		pm_runtime_disable(&pdev->dev);
+
+	if (dpu_kms->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(dpu_kms->opp_table);
 }
 
 static const struct component_ops dpu_ops = {
@@ -1082,6 +1104,8 @@ static int __maybe_unused dpu_runtime_suspend(struct device *dev)
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
 	struct dss_module_power *mp = &dpu_kms->mp;
 
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(dev, 0);
 	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
 	if (rc)
 		DPU_ERROR("clock disable failed rc:%d\n", rc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index a3b122b..7400cd7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -128,6 +128,10 @@ struct dpu_kms {
 
 	struct platform_device *pdev;
 	bool rpm_enabled;
+
+	struct opp_table *opp_table;
+	bool has_opp_table;
+
 	struct dss_module_power mp;
 
 	/* reference count bandwidth requests, so we know when we can
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 3/6] drm/msm/dpu: Use OPP API to set clk/perf state

[Rajendra Nayak]

On some qualcomm platforms DPU needs to express a performance state
requirement on a power domain depending on the clock rates.
Use OPP table from DT to register with OPP framework and use
dev_pm_opp_set_rate() to set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: dri-devel@lists.freedesktop.org
---
No functional change in v6, rebased over 5.8-rc1

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c |  3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 26 +++++++++++++++++++++++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  4 ++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 7c230f7..b36919d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -7,6 +7,7 @@
 #include <linux/debugfs.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/pm_opp.h>
 #include <linux/sort.h>
 #include <linux/clk.h>
 #include <linux/bitmap.h>
@@ -218,7 +219,7 @@ static int _dpu_core_perf_set_core_clk_rate(struct dpu_kms *kms, u64 rate)
 		rate = core_clk->max_rate;
 
 	core_clk->rate = rate;
-	return msm_dss_clk_set_rate(core_clk, 1);
+	return dev_pm_opp_set_rate(&kms->pdev->dev, core_clk->rate);
 }
 
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index b8615d4..0bc8ec4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -10,6 +10,7 @@
 #include <linux/debugfs.h>
 #include <linux/dma-buf.h>
 #include <linux/of_irq.h>
+#include <linux/pm_opp.h>
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_file.h>
@@ -1025,11 +1026,23 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 	if (!dpu_kms)
 		return -ENOMEM;
 
+	dpu_kms->opp_table = dev_pm_opp_set_clkname(dev, "core");
+	if (IS_ERR(dpu_kms->opp_table))
+		return PTR_ERR(dpu_kms->opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(dev);
+	if (!ret) {
+		dpu_kms->has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
 	mp = &dpu_kms->mp;
 	ret = msm_dss_parse_clock(pdev, mp);
 	if (ret) {
 		DPU_ERROR("failed to parse clocks, ret=%d\n", ret);
-		return ret;
+		goto err;
 	}
 
 	platform_set_drvdata(pdev, dpu_kms);
@@ -1043,6 +1056,11 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 
 	priv->kms = &dpu_kms->base;
 	return ret;
+err:
+	if (dpu_kms->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(dpu_kms->opp_table);
+	return ret;
 }
 
 static void dpu_unbind(struct device *dev, struct device *master, void *data)
@@ -1057,6 +1075,10 @@ static void dpu_unbind(struct device *dev, struct device *master, void *data)
 
 	if (dpu_kms->rpm_enabled)
 		pm_runtime_disable(&pdev->dev);
+
+	if (dpu_kms->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(dpu_kms->opp_table);
 }
 
 static const struct component_ops dpu_ops = {
@@ -1082,6 +1104,8 @@ static int __maybe_unused dpu_runtime_suspend(struct device *dev)
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
 	struct dss_module_power *mp = &dpu_kms->mp;
 
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(dev, 0);
 	rc = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false);
 	if (rc)
 		DPU_ERROR("clock disable failed rc:%d\n", rc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index a3b122b..7400cd7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -128,6 +128,10 @@ struct dpu_kms {
 
 	struct platform_device *pdev;
 	bool rpm_enabled;
+
+	struct opp_table *opp_table;
+	bool has_opp_table;
+
 	struct dss_module_power mp;
 
 	/* reference count bandwidth requests, so we know when we can
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v6 4/6] drm/msm: dsi: Use OPP API to set clk/perf state

[Rajendra Nayak]

On SDM845 DSI needs to express a perforamnce state
requirement on a power domain depending on the clock rates.
Use OPP table from DT to register with OPP framework and use
dev_pm_opp_set_rate() to set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: dri-devel@lists.freedesktop.org
---
No functional change in v6, rebased over 5.8-rc1

 drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_cfg.c  |  4 +--
 drivers/gpu/drm/msm/dsi/dsi_host.c | 58 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 4de771d..ba7583c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -180,10 +180,12 @@ int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
 int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host);
+int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
+void dsi_link_clk_disable_6g_v2(struct msm_dsi_host *msm_host);
 int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
 int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
 void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 813d69d..773c4fe 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -210,9 +210,9 @@ static const struct msm_dsi_host_cfg_ops msm_dsi_6g_host_ops = {
 };
 
 static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {
-	.link_clk_set_rate = dsi_link_clk_set_rate_6g,
+	.link_clk_set_rate = dsi_link_clk_set_rate_6g_v2,
 	.link_clk_enable = dsi_link_clk_enable_6g,
-	.link_clk_disable = dsi_link_clk_disable_6g,
+	.link_clk_disable = dsi_link_clk_disable_6g_v2,
 	.clk_init_ver = dsi_clk_init_6g_v2,
 	.tx_buf_alloc = dsi_tx_buf_alloc_6g,
 	.tx_buf_get = dsi_tx_buf_get_6g,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 11ae5b8..890531c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -14,6 +14,7 @@
 #include <linux/of_graph.h>
 #include <linux/of_irq.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_opp.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spinlock.h>
@@ -111,6 +112,9 @@ struct msm_dsi_host {
 	struct clk *pixel_clk_src;
 	struct clk *byte_intf_clk;
 
+	struct opp_table *opp_table;
+	bool has_opp_table;
+
 	u32 byte_clk_rate;
 	u32 pixel_clk_rate;
 	u32 esc_clk_rate;
@@ -537,6 +541,38 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
 	return 0;
 }
 
+int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host)
+{
+	int ret;
+	struct device *dev = &msm_host->pdev->dev;
+
+	DBG("Set clk rates: pclk=%d, byteclk=%d",
+		msm_host->mode->clock, msm_host->byte_clk_rate);
+
+	ret = dev_pm_opp_set_rate(dev, msm_host->byte_clk_rate);
+	if (ret) {
+		pr_err("%s: dev_pm_opp_set_rate failed %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = clk_set_rate(msm_host->pixel_clk, msm_host->pixel_clk_rate);
+	if (ret) {
+		pr_err("%s: Failed to set rate pixel clk, %d\n", __func__, ret);
+		return ret;
+	}
+
+	if (msm_host->byte_intf_clk) {
+		ret = clk_set_rate(msm_host->byte_intf_clk,
+				   msm_host->byte_clk_rate / 2);
+		if (ret) {
+			pr_err("%s: Failed to set rate byte intf clk, %d\n",
+			       __func__, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
 
 int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
 {
@@ -665,6 +701,13 @@ void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host)
 	clk_disable_unprepare(msm_host->byte_clk);
 }
 
+void dsi_link_clk_disable_6g_v2(struct msm_dsi_host *msm_host)
+{
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(&msm_host->pdev->dev, 0);
+	dsi_link_clk_disable_6g(msm_host);
+}
+
 void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 {
 	clk_disable_unprepare(msm_host->pixel_clk);
@@ -1879,6 +1922,18 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		goto fail;
 	}
 
+	msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "byte");
+	if (IS_ERR(msm_host->opp_table))
+		return PTR_ERR(msm_host->opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (!ret) {
+		msm_host->has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
 	init_completion(&msm_host->dma_comp);
 	init_completion(&msm_host->video_comp);
 	mutex_init(&msm_host->dev_mutex);
@@ -1914,6 +1969,9 @@ void msm_dsi_host_destroy(struct mipi_dsi_host *host)
 	mutex_destroy(&msm_host->cmd_mutex);
 	mutex_destroy(&msm_host->dev_mutex);
 
+	if (msm_host->has_opp_table)
+		dev_pm_opp_of_remove_table(&msm_host->pdev->dev);
+	dev_pm_opp_put_clkname(msm_host->opp_table);
 	pm_runtime_disable(&msm_host->pdev->dev);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 4/6] drm/msm: dsi: Use OPP API to set clk/perf state

[Rajendra Nayak]

On SDM845 DSI needs to express a perforamnce state
requirement on a power domain depending on the clock rates.
Use OPP table from DT to register with OPP framework and use
dev_pm_opp_set_rate() to set the clk/perf state.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: dri-devel@lists.freedesktop.org
---
No functional change in v6, rebased over 5.8-rc1

 drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_cfg.c  |  4 +--
 drivers/gpu/drm/msm/dsi/dsi_host.c | 58 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 4de771d..ba7583c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -180,10 +180,12 @@ int msm_dsi_runtime_suspend(struct device *dev);
 int msm_dsi_runtime_resume(struct device *dev);
 int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host);
+int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
+void dsi_link_clk_disable_6g_v2(struct msm_dsi_host *msm_host);
 int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
 int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
 void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
index 813d69d..773c4fe 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
@@ -210,9 +210,9 @@ static const struct msm_dsi_host_cfg_ops msm_dsi_6g_host_ops = {
 };
 
 static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {
-	.link_clk_set_rate = dsi_link_clk_set_rate_6g,
+	.link_clk_set_rate = dsi_link_clk_set_rate_6g_v2,
 	.link_clk_enable = dsi_link_clk_enable_6g,
-	.link_clk_disable = dsi_link_clk_disable_6g,
+	.link_clk_disable = dsi_link_clk_disable_6g_v2,
 	.clk_init_ver = dsi_clk_init_6g_v2,
 	.tx_buf_alloc = dsi_tx_buf_alloc_6g,
 	.tx_buf_get = dsi_tx_buf_get_6g,
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 11ae5b8..890531c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -14,6 +14,7 @@
 #include <linux/of_graph.h>
 #include <linux/of_irq.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_opp.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spinlock.h>
@@ -111,6 +112,9 @@ struct msm_dsi_host {
 	struct clk *pixel_clk_src;
 	struct clk *byte_intf_clk;
 
+	struct opp_table *opp_table;
+	bool has_opp_table;
+
 	u32 byte_clk_rate;
 	u32 pixel_clk_rate;
 	u32 esc_clk_rate;
@@ -537,6 +541,38 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
 	return 0;
 }
 
+int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host)
+{
+	int ret;
+	struct device *dev = &msm_host->pdev->dev;
+
+	DBG("Set clk rates: pclk=%d, byteclk=%d",
+		msm_host->mode->clock, msm_host->byte_clk_rate);
+
+	ret = dev_pm_opp_set_rate(dev, msm_host->byte_clk_rate);
+	if (ret) {
+		pr_err("%s: dev_pm_opp_set_rate failed %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = clk_set_rate(msm_host->pixel_clk, msm_host->pixel_clk_rate);
+	if (ret) {
+		pr_err("%s: Failed to set rate pixel clk, %d\n", __func__, ret);
+		return ret;
+	}
+
+	if (msm_host->byte_intf_clk) {
+		ret = clk_set_rate(msm_host->byte_intf_clk,
+				   msm_host->byte_clk_rate / 2);
+		if (ret) {
+			pr_err("%s: Failed to set rate byte intf clk, %d\n",
+			       __func__, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
 
 int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host)
 {
@@ -665,6 +701,13 @@ void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host)
 	clk_disable_unprepare(msm_host->byte_clk);
 }
 
+void dsi_link_clk_disable_6g_v2(struct msm_dsi_host *msm_host)
+{
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(&msm_host->pdev->dev, 0);
+	dsi_link_clk_disable_6g(msm_host);
+}
+
 void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 {
 	clk_disable_unprepare(msm_host->pixel_clk);
@@ -1879,6 +1922,18 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
 		goto fail;
 	}
 
+	msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "byte");
+	if (IS_ERR(msm_host->opp_table))
+		return PTR_ERR(msm_host->opp_table);
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (!ret) {
+		msm_host->has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
+		return ret;
+	}
+
 	init_completion(&msm_host->dma_comp);
 	init_completion(&msm_host->video_comp);
 	mutex_init(&msm_host->dev_mutex);
@@ -1914,6 +1969,9 @@ void msm_dsi_host_destroy(struct mipi_dsi_host *host)
 	mutex_destroy(&msm_host->cmd_mutex);
 	mutex_destroy(&msm_host->dev_mutex);
 
+	if (msm_host->has_opp_table)
+		dev_pm_opp_of_remove_table(&msm_host->pdev->dev);
+	dev_pm_opp_put_clkname(msm_host->opp_table);
 	pm_runtime_disable(&msm_host->pdev->dev);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v6 5/6] media: venus: core: Add support for opp tables/perf voting

[Rajendra Nayak]

Add support to add OPP tables and perf voting on the OPP powerdomain.
This is needed so venus votes on the corresponding performance state
for the OPP powerdomain along with setting the core clock rate.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc: linux-media@vger.kernel.org
---
No functional change in v6, rebased over 5.8-rc1
Bindings update to add optional PD https://lore.kernel.org/patchwork/patch/1241077/

 drivers/media/platform/qcom/venus/core.c       | 43 +++++++++++++++++---
 drivers/media/platform/qcom/venus/core.h       |  5 +++
 drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++++--
 3 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 203c653..630f61b 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -12,6 +12,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-mem2mem.h>
@@ -216,21 +217,37 @@ static int venus_probe(struct platform_device *pdev)
 	if (!core->pm_ops)
 		return -ENODEV;
 
+	core->opp_table = dev_pm_opp_set_clkname(dev, "core");
+	if (IS_ERR(core->opp_table))
+		return PTR_ERR(core->opp_table);
+
+	if (core->res->opp_pmdomain) {
+		ret = dev_pm_opp_of_add_table(dev);
+		if (!ret) {
+			core->has_opp_table = true;
+		} else if (ret != -ENODEV) {
+			dev_err(dev, "invalid OPP table in device tree\n");
+			return ret;
+		}
+	}
+
 	if (core->pm_ops->core_get) {
 		ret = core->pm_ops->core_get(dev);
 		if (ret)
-			return ret;
+			goto err_opp_cleanup;
 	}
 
 	ret = dma_set_mask_and_coherent(dev, core->res->dma_mask);
 	if (ret)
-		return ret;
+		goto err_opp_cleanup;
 
 	if (!dev->dma_parms) {
 		dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
 					      GFP_KERNEL);
-		if (!dev->dma_parms)
-			return -ENOMEM;
+		if (!dev->dma_parms) {
+			ret = -ENOMEM;
+			goto err_opp_cleanup;
+		}
 	}
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
@@ -242,11 +259,11 @@ static int venus_probe(struct platform_device *pdev)
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 					"venus", core);
 	if (ret)
-		return ret;
+		goto err_opp_cleanup;
 
 	ret = hfi_create(core, &venus_core_ops);
 	if (ret)
-		return ret;
+		goto err_opp_cleanup;
 
 	pm_runtime_enable(dev);
 
@@ -302,6 +319,10 @@ static int venus_probe(struct platform_device *pdev)
 	pm_runtime_set_suspended(dev);
 	pm_runtime_disable(dev);
 	hfi_destroy(core);
+err_opp_cleanup:
+	if (core->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(core->opp_table);
 	return ret;
 }
 
@@ -326,6 +347,10 @@ static int venus_remove(struct platform_device *pdev)
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 
+	if (core->has_opp_table)
+		dev_pm_opp_of_remove_table(dev);
+	dev_pm_opp_put_clkname(core->opp_table);
+
 	if (pm_ops->core_put)
 		pm_ops->core_put(dev);
 
@@ -355,6 +380,10 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* Drop the performance state vote */
+	if (core->opp_pmdomain)
+		dev_pm_opp_set_rate(dev, 0);
+
 	if (pm_ops->core_power)
 		ret = pm_ops->core_power(dev, POWER_OFF);
 
@@ -520,6 +549,7 @@ static const struct venus_resources sdm845_res_v2 = {
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = { "venus", "vcodec0", "vcodec1" },
 	.vcodec_pmdomains_num = 3,
+	.opp_pmdomain = (const char *[]) { "opp-pd", NULL },
 	.vcodec_num = 2,
 	.max_load = 3110400,	/* 4096x2160@90 */
 	.hfi_version = HFI_VERSION_4XX,
@@ -565,6 +595,7 @@ static const struct venus_resources sc7180_res = {
 	.vcodec_clks_num = 2,
 	.vcodec_pmdomains = { "venus", "vcodec0" },
 	.vcodec_pmdomains_num = 2,
+	.opp_pmdomain = (const char *[]) { "opp-pd", NULL },
 	.vcodec_num = 1,
 	.hfi_version = HFI_VERSION_4XX,
 	.vmem_id = VIDC_RESOURCE_NONE,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7118612..b0cc544 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -62,6 +62,7 @@ struct venus_resources {
 	unsigned int vcodec_clks_num;
 	const char * const vcodec_pmdomains[VIDC_PMDOMAINS_NUM_MAX];
 	unsigned int vcodec_pmdomains_num;
+	const char **opp_pmdomain;
 	unsigned int vcodec_num;
 	enum hfi_version hfi_version;
 	u32 max_load;
@@ -145,8 +146,12 @@ struct venus_core {
 	struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
 	struct icc_path *video_path;
 	struct icc_path *cpucfg_path;
+	struct opp_table *opp_table;
+	bool has_opp_table;
 	struct device_link *pd_dl_venus;
 	struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
+	struct device_link *opp_dl_venus;
+	struct device *opp_pmdomain;
 	struct video_device *vdev_dec;
 	struct video_device *vdev_enc;
 	struct v4l2_device v4l2_dev;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index abf9315..bfe7421 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -9,6 +9,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <media/v4l2-mem2mem.h>
@@ -66,10 +67,9 @@ static void core_clks_disable(struct venus_core *core)
 
 static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
 {
-	struct clk *clk = core->clks[0];
 	int ret;
 
-	ret = clk_set_rate(clk, freq);
+	ret = dev_pm_opp_set_rate(core->dev, freq);
 	if (ret)
 		return ret;
 
@@ -740,13 +740,16 @@ static int venc_power_v4(struct device *dev, int on)
 
 static int vcodec_domains_get(struct device *dev)
 {
+	int ret;
+	struct opp_table *opp_table;
+	struct device **opp_virt_dev;
 	struct venus_core *core = dev_get_drvdata(dev);
 	const struct venus_resources *res = core->res;
 	struct device *pd;
 	unsigned int i;
 
 	if (!res->vcodec_pmdomains_num)
-		return -ENODEV;
+		goto skip_pmdomains;
 
 	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
 		pd = dev_pm_domain_attach_by_name(dev,
@@ -763,7 +766,41 @@ static int vcodec_domains_get(struct device *dev)
 	if (!core->pd_dl_venus)
 		return -ENODEV;
 
+skip_pmdomains:
+	if (!core->has_opp_table)
+		return 0;
+
+	/* Attach the power domain for setting performance state */
+	opp_table = dev_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
+	if (IS_ERR(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto opp_attach_err;
+	}
+
+	core->opp_pmdomain = *opp_virt_dev;
+	core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
+					     DL_FLAG_RPM_ACTIVE |
+					     DL_FLAG_PM_RUNTIME |
+					     DL_FLAG_STATELESS);
+	if (!core->opp_dl_venus) {
+		ret = -ENODEV;
+		goto opp_dl_add_err;
+	}
+
 	return 0;
+
+opp_dl_add_err:
+	dev_pm_domain_detach(core->opp_pmdomain, true);
+opp_attach_err:
+	if (core->pd_dl_venus) {
+		device_link_del(core->pd_dl_venus);
+		for (i = 0; i < res->vcodec_pmdomains_num; i++) {
+			if (IS_ERR_OR_NULL(core->pmdomains[i]))
+				continue;
+			dev_pm_domain_detach(core->pmdomains[i], true);
+		}
+	}
+	return ret;
 }
 
 static void vcodec_domains_put(struct device *dev)
@@ -773,7 +810,7 @@ static void vcodec_domains_put(struct device *dev)
 	unsigned int i;
 
 	if (!res->vcodec_pmdomains_num)
-		return;
+		goto skip_pmdomains;
 
 	if (core->pd_dl_venus)
 		device_link_del(core->pd_dl_venus);
@@ -783,6 +820,15 @@ static void vcodec_domains_put(struct device *dev)
 			continue;
 		dev_pm_domain_detach(core->pmdomains[i], true);
 	}
+
+skip_pmdomains:
+	if (!res->opp_pmdomain)
+		return;
+
+	if (core->opp_dl_venus)
+		device_link_del(core->opp_dl_venus);
+
+	dev_pm_domain_detach(core->opp_pmdomain, true);
 }
 
 static int core_get_v4(struct device *dev)
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Rajendra Nayak]

QSPI needs to vote on a performance state of a power domain depending on
the clock rate. Add support for it by specifying the perf state/clock rate
as an OPP table in device tree.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Mark Brown <broonie@kernel.org>
Cc: Alok Chauhan <alokc@codeaurora.org>
Cc: Akash Asthana <akashast@codeaurora.org>
Cc: linux-spi@vger.kernel.org
---
No functional change in v6, rebased over 5.8-rc1

 drivers/spi/spi-qcom-qspi.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index 3c4f83b..ef51982 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -8,6 +8,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_opp.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi-mem.h>
 
@@ -139,6 +140,8 @@ struct qcom_qspi {
 	struct device *dev;
 	struct clk_bulk_data *clks;
 	struct qspi_xfer xfer;
+	struct opp_table *opp_table;
+	bool has_opp_table;
 	/* Lock to protect xfer and IRQ accessed registers */
 	spinlock_t lock;
 };
@@ -235,7 +238,7 @@ static int qcom_qspi_transfer_one(struct spi_master *master,
 		speed_hz = xfer->speed_hz;
 
 	/* In regular operation (SBL_EN=1) core must be 4x transfer clock */
-	ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
+	ret = dev_pm_opp_set_rate(ctrl->dev, speed_hz * 4);
 	if (ret) {
 		dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
 		return ret;
@@ -481,6 +484,20 @@ static int qcom_qspi_probe(struct platform_device *pdev)
 	master->handle_err = qcom_qspi_handle_err;
 	master->auto_runtime_pm = true;
 
+	ctrl->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core");
+	if (IS_ERR(ctrl->opp_table)) {
+		ret = PTR_ERR(ctrl->opp_table);
+		goto exit_probe_master_put;
+	}
+	/* OPP table is optional */
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (!ret) {
+		ctrl->has_opp_table = true;
+	} else if (ret != -ENODEV) {
+		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
+		goto exit_probe_master_put;
+	}
+
 	pm_runtime_enable(dev);
 
 	ret = spi_register_master(master);
@@ -488,6 +505,9 @@ static int qcom_qspi_probe(struct platform_device *pdev)
 		return 0;
 
 	pm_runtime_disable(dev);
+	if (ctrl->has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(ctrl->opp_table);
 
 exit_probe_master_put:
 	spi_master_put(master);
@@ -498,11 +518,15 @@ static int qcom_qspi_probe(struct platform_device *pdev)
 static int qcom_qspi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = platform_get_drvdata(pdev);
+	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
 
 	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
 	spi_unregister_master(master);
 
 	pm_runtime_disable(&pdev->dev);
+	if (ctrl->has_opp_table)
+		dev_pm_opp_of_remove_table(&pdev->dev);
+	dev_pm_opp_put_clkname(ctrl->opp_table);
 
 	return 0;
 }
@@ -512,6 +536,8 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
 
+	/* Drop the performance state vote */
+	dev_pm_opp_set_rate(dev, 0);
 	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
 
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Matthias Kaehlcke]

What is the plan for landing these, it seems not all must/should
go through the QCOM tree.

My guesses:

tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
spi: spi-geni-qcom: Use OPP API to set clk/perf state
  QCOM tree due to shared dependency on change in include/linux/qcom-geni-se.h

drm/msm/dpu: Use OPP API to set clk/perf state
drm/msm: dsi: Use OPP API to set clk/perf state
  drm/msm tree

media: venus: core: Add support for opp tables/perf voting
  venus tree

spi: spi-qcom-qspi: Use OPP API to set clk/perf state
  SPI tree


Does this make sense or are there any dependencies I'm missing?

Thanks

Matthias

On Mon, Jun 15, 2020 at 05:32:38PM +0530, Rajendra Nayak wrote:
> Changes in v6:
> 1. rebased on 5.8-rc1, no functional change. 
> 
> Changes in v5:
> 1. Opp cleanup path fixed up across drivers
> 
> Changes in v4:
> 1. Fixed all review feedback on v3
> 2. Dropped the dts patches, will post as a seperate series once
> driver changes are reviewed and merged.
> The driver changes without DT updates to include OPP tables will
> have zero functional change.
> 3. Dropped the mmc/sdhc patch, which is a standalone patch. will
> repost if needed seperately.
> 
> Changes in v3:
> 1. Added better error handling for dev_pm_opp_of_add_table()
> 2. Some minor changes and fixes in 'PATCH 12/17' as compared to v2
> 3. Dropped the mmc patch picked up by Ulf [2]
> 
> Changes in v2:
> 1. Added error handling for dev_pm_opp_set_clkname()
> and dev_pm_opp_of_add_table()
> 2. Used dev_pm_opp_put_clkname() in the cleanup path
> 3. Dropped the OPP patch pulled in by Viresh [1]
> 4. Dropped the UFS patches since they had some major rework
> needed because of changes that were merged in the merge window
> and I don't have a UFS device currently to validate the changes.
> 
> We have had support added in the OPP core for a while now to support
> DVFS for IO devices, and this series uses that infrastructure to
> add DVFS support for various IO devices in sdm845 and sc7180 SoCs.
> 
> [1] https://lkml.org/lkml/2020/4/14/98
> [2] https://lore.kernel.org/patchwork/patch/1226381/
> 
> Rajendra Nayak (6):
>   tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>   spi: spi-geni-qcom: Use OPP API to set clk/perf state
>   drm/msm/dpu: Use OPP API to set clk/perf state
>   drm/msm: dsi: Use OPP API to set clk/perf state
>   media: venus: core: Add support for opp tables/perf voting
>   spi: spi-qcom-qspi: Use OPP API to set clk/perf state
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c  |  3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 26 +++++++++++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h        |  4 ++
>  drivers/gpu/drm/msm/dsi/dsi.h                  |  2 +
>  drivers/gpu/drm/msm/dsi/dsi_cfg.c              |  4 +-
>  drivers/gpu/drm/msm/dsi/dsi_host.c             | 58 ++++++++++++++++++++++++++
>  drivers/media/platform/qcom/venus/core.c       | 43 ++++++++++++++++---
>  drivers/media/platform/qcom/venus/core.h       |  5 +++
>  drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++--
>  drivers/spi/spi-geni-qcom.c                    | 26 ++++++++++--
>  drivers/spi/spi-qcom-qspi.c                    | 28 ++++++++++++-
>  drivers/tty/serial/qcom_geni_serial.c          | 34 ++++++++++++---
>  include/linux/qcom-geni-se.h                   |  4 ++
>  13 files changed, 268 insertions(+), 23 deletions(-)
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Rajendra Nayak]

Hey Matthias, thanks for summarizing this.

On 6/18/2020 3:45 AM, Matthias Kaehlcke wrote:
> What is the plan for landing these, it seems not all must/should
> go through the QCOM tree.
> 
> My guesses:
> 
> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
> spi: spi-geni-qcom: Use OPP API to set clk/perf state
>    QCOM tree due to shared dependency on change in include/linux/qcom-geni-se.h

That's correct, Bjorn/Andy, can these be pulled in now for 5.9?
They have acks from Greg for serial and Mark for the spi patch.
  
> drm/msm/dpu: Use OPP API to set clk/perf state
> drm/msm: dsi: Use OPP API to set clk/perf state
>    drm/msm tree

Correct, the dsi patch is still not reviewed by Rob, so once that's done,
I am guessing Rob would pull both of these.

> 
> media: venus: core: Add support for opp tables/perf voting
>    venus tree

correct, this is pending review/ack from Stan.

> 
> spi: spi-qcom-qspi: Use OPP API to set clk/perf state
>    SPI tree

Right, Mark has this acked, I am guessing he will pull this in at
some point.

thanks,
Rajendra

> 
> 
> Does this make sense or are there any dependencies I'm missing?
> 
> Thanks
> 
> Matthias
> 
> On Mon, Jun 15, 2020 at 05:32:38PM +0530, Rajendra Nayak wrote:
>> Changes in v6:
>> 1. rebased on 5.8-rc1, no functional change.
>>
>> Changes in v5:
>> 1. Opp cleanup path fixed up across drivers
>>
>> Changes in v4:
>> 1. Fixed all review feedback on v3
>> 2. Dropped the dts patches, will post as a seperate series once
>> driver changes are reviewed and merged.
>> The driver changes without DT updates to include OPP tables will
>> have zero functional change.
>> 3. Dropped the mmc/sdhc patch, which is a standalone patch. will
>> repost if needed seperately.
>>
>> Changes in v3:
>> 1. Added better error handling for dev_pm_opp_of_add_table()
>> 2. Some minor changes and fixes in 'PATCH 12/17' as compared to v2
>> 3. Dropped the mmc patch picked up by Ulf [2]
>>
>> Changes in v2:
>> 1. Added error handling for dev_pm_opp_set_clkname()
>> and dev_pm_opp_of_add_table()
>> 2. Used dev_pm_opp_put_clkname() in the cleanup path
>> 3. Dropped the OPP patch pulled in by Viresh [1]
>> 4. Dropped the UFS patches since they had some major rework
>> needed because of changes that were merged in the merge window
>> and I don't have a UFS device currently to validate the changes.
>>
>> We have had support added in the OPP core for a while now to support
>> DVFS for IO devices, and this series uses that infrastructure to
>> add DVFS support for various IO devices in sdm845 and sc7180 SoCs.
>>
>> [1] https://lkml.org/lkml/2020/4/14/98
>> [2] https://lore.kernel.org/patchwork/patch/1226381/
>>
>> Rajendra Nayak (6):
>>    tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>>    spi: spi-geni-qcom: Use OPP API to set clk/perf state
>>    drm/msm/dpu: Use OPP API to set clk/perf state
>>    drm/msm: dsi: Use OPP API to set clk/perf state
>>    media: venus: core: Add support for opp tables/perf voting
>>    spi: spi-qcom-qspi: Use OPP API to set clk/perf state
>>
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c  |  3 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 26 +++++++++++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h        |  4 ++
>>   drivers/gpu/drm/msm/dsi/dsi.h                  |  2 +
>>   drivers/gpu/drm/msm/dsi/dsi_cfg.c              |  4 +-
>>   drivers/gpu/drm/msm/dsi/dsi_host.c             | 58 ++++++++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/core.c       | 43 ++++++++++++++++---
>>   drivers/media/platform/qcom/venus/core.h       |  5 +++
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++--
>>   drivers/spi/spi-geni-qcom.c                    | 26 ++++++++++--
>>   drivers/spi/spi-qcom-qspi.c                    | 28 ++++++++++++-
>>   drivers/tty/serial/qcom_geni_serial.c          | 34 ++++++++++++---
>>   include/linux/qcom-geni-se.h                   |  4 ++
>>   13 files changed, 268 insertions(+), 23 deletions(-)
>>
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Stanimir Varbanov]

Hi Rajendra,

On 6/18/20 7:47 AM, Rajendra Nayak wrote:
> Hey Matthias, thanks for summarizing this.
> 
> On 6/18/2020 3:45 AM, Matthias Kaehlcke wrote:
>> What is the plan for landing these, it seems not all must/should
>> go through the QCOM tree.
>>
>> My guesses:
>>
>> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>> spi: spi-geni-qcom: Use OPP API to set clk/perf state
>>    QCOM tree due to shared dependency on change in
>> include/linux/qcom-geni-se.h
> 
> That's correct, Bjorn/Andy, can these be pulled in now for 5.9?
> They have acks from Greg for serial and Mark for the spi patch.
>  
>> drm/msm/dpu: Use OPP API to set clk/perf state
>> drm/msm: dsi: Use OPP API to set clk/perf state
>>    drm/msm tree
> 
> Correct, the dsi patch is still not reviewed by Rob, so once that's done,
> I am guessing Rob would pull both of these.
> 
>>
>> media: venus: core: Add support for opp tables/perf voting
>>    venus tree
> 
> correct, this is pending review/ack from Stan.

I tested the changes in the driver, and they looks fine. But when
applied the corresponding change in the DT node I see this message when
the streaming is stopping:

qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x30000

I tested on v5.7 (linaro-integration). Should I be worried ?


-- 
regards,
Stan

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Rajendra Nayak]

Hey Stan,
  
> On 6/18/20 7:47 AM, Rajendra Nayak wrote:
>> Hey Matthias, thanks for summarizing this.
>>
>> On 6/18/2020 3:45 AM, Matthias Kaehlcke wrote:
>>> What is the plan for landing these, it seems not all must/should
>>> go through the QCOM tree.
>>>
>>> My guesses:
>>>
>>> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>>> spi: spi-geni-qcom: Use OPP API to set clk/perf state
>>>     QCOM tree due to shared dependency on change in
>>> include/linux/qcom-geni-se.h
>>
>> That's correct, Bjorn/Andy, can these be pulled in now for 5.9?
>> They have acks from Greg for serial and Mark for the spi patch.
>>   
>>> drm/msm/dpu: Use OPP API to set clk/perf state
>>> drm/msm: dsi: Use OPP API to set clk/perf state
>>>     drm/msm tree
>>
>> Correct, the dsi patch is still not reviewed by Rob, so once that's done,
>> I am guessing Rob would pull both of these.
>>
>>>
>>> media: venus: core: Add support for opp tables/perf voting
>>>     venus tree
>>
>> correct, this is pending review/ack from Stan.
> 
> I tested the changes in the driver, and they looks fine. But when
> applied the corresponding change in the DT node I see this message when
> the streaming is stopping:
> 
> qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x30000
> 
> I tested on v5.7 (linaro-integration). Should I be worried ?

Is this seen on sdm845 or sc7180, or both?

thanks,
Rajendra  -- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Stanimir Varbanov]

On 6/18/20 3:11 PM, Rajendra Nayak wrote:
> 
> Hey Stan,
>  
>> On 6/18/20 7:47 AM, Rajendra Nayak wrote:
>>> Hey Matthias, thanks for summarizing this.
>>>
>>> On 6/18/2020 3:45 AM, Matthias Kaehlcke wrote:
>>>> What is the plan for landing these, it seems not all must/should
>>>> go through the QCOM tree.
>>>>
>>>> My guesses:
>>>>
>>>> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>>>> spi: spi-geni-qcom: Use OPP API to set clk/perf state
>>>>     QCOM tree due to shared dependency on change in
>>>> include/linux/qcom-geni-se.h
>>>
>>> That's correct, Bjorn/Andy, can these be pulled in now for 5.9?
>>> They have acks from Greg for serial and Mark for the spi patch.
>>>  
>>>> drm/msm/dpu: Use OPP API to set clk/perf state
>>>> drm/msm: dsi: Use OPP API to set clk/perf state
>>>>     drm/msm tree
>>>
>>> Correct, the dsi patch is still not reviewed by Rob, so once that's
>>> done,
>>> I am guessing Rob would pull both of these.
>>>
>>>>
>>>> media: venus: core: Add support for opp tables/perf voting
>>>>     venus tree
>>>
>>> correct, this is pending review/ack from Stan.
>>
>> I tested the changes in the driver, and they looks fine. But when
>> applied the corresponding change in the DT node I see this message when
>> the streaming is stopping:
>>
>> qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x30000
>>
>> I tested on v5.7 (linaro-integration). Should I be worried ?
> 
> Is this seen on sdm845 or sc7180, or both?

Seen on sdm845, I don't have sc7180 to test.

I can try it on 5.8-rc1 integration-linux-qcomlt if you think there
might be a fix in rpmh.


-- 
regards,
Stan

Re: [PATCH v6 5/6] media: venus: core: Add support for opp tables/perf voting

[Stanimir Varbanov]

Hi Rajendra,

On 6/15/20 3:02 PM, Rajendra Nayak wrote:
> Add support to add OPP tables and perf voting on the OPP powerdomain.
> This is needed so venus votes on the corresponding performance state
> for the OPP powerdomain along with setting the core clock rate.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Cc: linux-media@vger.kernel.org
> ---
> No functional change in v6, rebased over 5.8-rc1
> Bindings update to add optional PD https://lore.kernel.org/patchwork/patch/1241077/
> 
>  drivers/media/platform/qcom/venus/core.c       | 43 +++++++++++++++++---
>  drivers/media/platform/qcom/venus/core.h       |  5 +++
>  drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++++--
>  3 files changed, 92 insertions(+), 10 deletions(-)
> 

<cut>

>  
> @@ -740,13 +740,16 @@ static int venc_power_v4(struct device *dev, int on)
>  
>  static int vcodec_domains_get(struct device *dev)
>  {
> +	int ret;
> +	struct opp_table *opp_table;
> +	struct device **opp_virt_dev;
>  	struct venus_core *core = dev_get_drvdata(dev);
>  	const struct venus_resources *res = core->res;
>  	struct device *pd;
>  	unsigned int i;
>  
>  	if (!res->vcodec_pmdomains_num)
> -		return -ENODEV;
> +		goto skip_pmdomains;
>  
>  	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
>  		pd = dev_pm_domain_attach_by_name(dev,
> @@ -763,7 +766,41 @@ static int vcodec_domains_get(struct device *dev)
>  	if (!core->pd_dl_venus)
>  		return -ENODEV;
>  
> +skip_pmdomains:
> +	if (!core->has_opp_table)
> +		return 0;
> +
> +	/* Attach the power domain for setting performance state */
> +	opp_table = dev_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
> +	if (IS_ERR(opp_table)) {
> +		ret = PTR_ERR(opp_table);
> +		goto opp_attach_err;
> +	}
> +
> +	core->opp_pmdomain = *opp_virt_dev;
> +	core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
> +					     DL_FLAG_RPM_ACTIVE |
> +					     DL_FLAG_PM_RUNTIME |
> +					     DL_FLAG_STATELESS);
> +	if (!core->opp_dl_venus) {
> +		ret = -ENODEV;
> +		goto opp_dl_add_err;
> +	}
> +
>  	return 0;
> +
> +opp_dl_add_err:
> +	dev_pm_domain_detach(core->opp_pmdomain, true);
> +opp_attach_err:
> +	if (core->pd_dl_venus) {
> +		device_link_del(core->pd_dl_venus);
> +		for (i = 0; i < res->vcodec_pmdomains_num; i++) {
> +			if (IS_ERR_OR_NULL(core->pmdomains[i]))
> +				continue;
> +			dev_pm_domain_detach(core->pmdomains[i], true);
> +		}
> +	}
> +	return ret;
>  }
>  
>  static void vcodec_domains_put(struct device *dev)
> @@ -773,7 +810,7 @@ static void vcodec_domains_put(struct device *dev)
>  	unsigned int i;
>  
>  	if (!res->vcodec_pmdomains_num)
> -		return;
> +		goto skip_pmdomains;
>  
>  	if (core->pd_dl_venus)
>  		device_link_del(core->pd_dl_venus);
> @@ -783,6 +820,15 @@ static void vcodec_domains_put(struct device *dev)
>  			continue;
>  		dev_pm_domain_detach(core->pmdomains[i], true);
>  	}
> +
> +skip_pmdomains:
> +	if (!res->opp_pmdomain)
> +		return;
> +
> +	if (core->opp_dl_venus)
> +		device_link_del(core->opp_dl_venus);
> +
> +	dev_pm_domain_detach(core->opp_pmdomain, true);

Without corresponding changes in venus DT node we call pm_domain_detach
with core->opp_pmdomain = NULL which triggers NULL pointer dereference.

I guess you should check:

	if (core->has_opp_table)
		dev_pm_domain_detach(core->opp_pmdomain, true);

or

	if (core->opp_pmdomain)
		dev_pm_domain_detach(core->opp_pmdomain, true);


... not sure which one is more appropriate or both are.


-- 
regards,
Stan

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

Hi Mark,

do you plan to land this in your tree?

I know you hate contentless pings, but since you acked this patch and
usually don't seem to do that when patches go through your tree I want
to make sure we aren't in a situation where everybody thinks that the
patch will go through someone else's tree.

Thanks

Matthias

On Mon, Jun 15, 2020 at 05:32:44PM +0530, Rajendra Nayak wrote:
> QSPI needs to vote on a performance state of a power domain depending on
> the clock rate. Add support for it by specifying the perf state/clock rate
> as an OPP table in device tree.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Acked-by: Mark Brown <broonie@kernel.org>
> Cc: Alok Chauhan <alokc@codeaurora.org>
> Cc: Akash Asthana <akashast@codeaurora.org>
> Cc: linux-spi@vger.kernel.org
> ---
> No functional change in v6, rebased over 5.8-rc1
> 
>  drivers/spi/spi-qcom-qspi.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> index 3c4f83b..ef51982 100644
> --- a/drivers/spi/spi-qcom-qspi.c
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -8,6 +8,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_opp.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi-mem.h>
>  
> @@ -139,6 +140,8 @@ struct qcom_qspi {
>  	struct device *dev;
>  	struct clk_bulk_data *clks;
>  	struct qspi_xfer xfer;
> +	struct opp_table *opp_table;
> +	bool has_opp_table;
>  	/* Lock to protect xfer and IRQ accessed registers */
>  	spinlock_t lock;
>  };
> @@ -235,7 +238,7 @@ static int qcom_qspi_transfer_one(struct spi_master *master,
>  		speed_hz = xfer->speed_hz;
>  
>  	/* In regular operation (SBL_EN=1) core must be 4x transfer clock */
> -	ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
> +	ret = dev_pm_opp_set_rate(ctrl->dev, speed_hz * 4);
>  	if (ret) {
>  		dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
>  		return ret;
> @@ -481,6 +484,20 @@ static int qcom_qspi_probe(struct platform_device *pdev)
>  	master->handle_err = qcom_qspi_handle_err;
>  	master->auto_runtime_pm = true;
>  
> +	ctrl->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core");
> +	if (IS_ERR(ctrl->opp_table)) {
> +		ret = PTR_ERR(ctrl->opp_table);
> +		goto exit_probe_master_put;
> +	}
> +	/* OPP table is optional */
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (!ret) {
> +		ctrl->has_opp_table = true;
> +	} else if (ret != -ENODEV) {
> +		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
> +		goto exit_probe_master_put;
> +	}
> +
>  	pm_runtime_enable(dev);
>  
>  	ret = spi_register_master(master);
> @@ -488,6 +505,9 @@ static int qcom_qspi_probe(struct platform_device *pdev)
>  		return 0;
>  
>  	pm_runtime_disable(dev);
> +	if (ctrl->has_opp_table)
> +		dev_pm_opp_of_remove_table(&pdev->dev);
> +	dev_pm_opp_put_clkname(ctrl->opp_table);
>  
>  exit_probe_master_put:
>  	spi_master_put(master);
> @@ -498,11 +518,15 @@ static int qcom_qspi_probe(struct platform_device *pdev)
>  static int qcom_qspi_remove(struct platform_device *pdev)
>  {
>  	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
>  
>  	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
>  	spi_unregister_master(master);
>  
>  	pm_runtime_disable(&pdev->dev);
> +	if (ctrl->has_opp_table)
> +		dev_pm_opp_of_remove_table(&pdev->dev);
> +	dev_pm_opp_put_clkname(ctrl->opp_table);
>  
>  	return 0;
>  }
> @@ -512,6 +536,8 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
>  	struct spi_master *master = dev_get_drvdata(dev);
>  	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
>  
> +	/* Drop the performance state vote */
> +	dev_pm_opp_set_rate(dev, 0);
>  	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
>  
>  	return 0;
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Wed, Jun 24, 2020 at 10:09:33AM -0700, Matthias Kaehlcke wrote:
> Hi Mark,
> 
> do you plan to land this in your tree?
> 
> I know you hate contentless pings, but since you acked this patch and
> usually don't seem to do that when patches go through your tree I want
> to make sure we aren't in a situation where everybody thinks that the
> patch will go through someone else's tree.

Aren't there dependencies on earlier patches in the series?  In general
if someone acks something for their tree that means they don't expect to
apply it themselves.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 10:09:33AM -0700, Matthias Kaehlcke wrote:
> > Hi Mark,
> > 
> > do you plan to land this in your tree?
> > 
> > I know you hate contentless pings, but since you acked this patch and
> > usually don't seem to do that when patches go through your tree I want
> > to make sure we aren't in a situation where everybody thinks that the
> > patch will go through someone else's tree.
> 
> Aren't there dependencies on earlier patches in the series?

Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set
clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made
by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state",
however that's not true for this patch.

I wonder if it would have been better to split this series into individual
patches/mini-series, to avoid this kind of confusion.

> In general if someone acks something for their tree that means they don't
> expect to apply it themselves.

Yes, that was my understanding and prompted me to clarify this with you.

The patch could go through the QCOM tree, but to my knowledge there is no
reason for it.

Btw, the patch "[V8,7/8] spi: spi-qcom-qspi: Add interconnect support"
(https://patchwork.kernel.org/patch/11620285/) is in a similar situation.
Another patch of the series for the 'spi-geni-qcom' driver has to go
through the QCOM change due to changes in geni, but the QSPI driver
doesn't use geni and could therefore go through your tree.

Ultimately I don't really care too much through which tree the patches
land as long as you and Bjorn agree on it :)

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

On Wed, Jun 24, 2020 at 10:39:48AM -0700, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote:

> > Aren't there dependencies on earlier patches in the series?

> Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set
> clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made
> by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state",
> however that's not true for this patch.

Wait, so *some* of the series should go together but not other bits?
But you want them split up for some reason?

> I wonder if it would have been better to split this series into individual
> patches/mini-series, to avoid this kind of confusion.

Yes, if there's no dependencies then bundling things up into a series
just causes confusion.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 10:39:48AM -0700, Matthias Kaehlcke wrote:
> > On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote:
> 
> > > Aren't there dependencies on earlier patches in the series?
> 
> > Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set
> > clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made
> > by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state",
> > however that's not true for this patch.
> 
> Wait, so *some* of the series should go together but not other bits?
> But you want them split up for some reason?

Yes, this will almost certainly be the case, even if not for this patch.
I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).

It seems very unlikely to me that the DRM patches will go through the QCOM
tree. The venus patch also doesn't have any dependencies and is more likely
to cause conflicts if it lands through QCOM instead of it's maintainer tree.
For the QSPI patch you could argue to just take it through QCOM since the SPI
patch of this series goes through this tree, up to you, I just want to make
sure everybody is on the same page.

> > I wonder if it would have been better to split this series into individual
> > patches/mini-series, to avoid this kind of confusion.
> 
> Yes, if there's no dependencies then bundling things up into a series
> just causes confusion.

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:

> > Wait, so *some* of the series should go together but not other bits?
> > But you want them split up for some reason?

> Yes, this will almost certainly be the case, even if not for this patch.
> I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).

I'm not really reading any of this stuff for the series as a whole, as
far as I could tell I'd reviewed all my bits and was hoping whatever
random platform stuff needs sorting out was going to be sorted out so I
stopped getting copied on revisions :(

> For the QSPI patch you could argue to just take it through QCOM since the SPI
> patch of this series goes through this tree, up to you, I just want to make
> sure everybody is on the same page.

If there are some part of this that don't have a connection with the
rest of the series and should be applied separately please split them
out and send them separately so it's clear what's going on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote:
> > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:
> 
> > > Wait, so *some* of the series should go together but not other bits?
> > > But you want them split up for some reason?
> 
> > Yes, this will almost certainly be the case, even if not for this patch.
> > I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).
> 
> I'm not really reading any of this stuff for the series as a whole, as
> far as I could tell I'd reviewed all my bits and was hoping whatever
> random platform stuff needs sorting out was going to be sorted out so I
> stopped getting copied on revisions :(

Sorry this caused you extra work, I only fully realized this when the series
was basically ready to land :(

Avoiding unnecessary revision spam is another good reason to not combine
technically unrelated patches in a single series.

If I notice similar series in the future I'll try to bring it up early.

> > For the QSPI patch you could argue to just take it through QCOM since the SPI
> > patch of this series goes through this tree, up to you, I just want to make
> > sure everybody is on the same page.
> 
> If there are some part of this that don't have a connection with the
> rest of the series and should be applied separately please split them
> out and send them separately so it's clear what's going on.

Rajendra, IIUC you have to re-spin this series anyway, please split it
up in self-contained chunks.

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote:

> > I'm not really reading any of this stuff for the series as a whole, as
> > far as I could tell I'd reviewed all my bits and was hoping whatever
> > random platform stuff needs sorting out was going to be sorted out so I
> > stopped getting copied on revisions :(

> Sorry this caused you extra work, I only fully realized this when the series
> was basically ready to land :(

It's fine, mostly it's just checking to see that I'd reviewed all the
relevant patches which takes moments.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Bjorn Andersson]

On Mon 15 Jun 05:02 PDT 2020, Rajendra Nayak wrote:

> geni serial needs to express a perforamnce state requirement on CX
> powerdomain depending on the frequency of the clock rates.
> Use OPP table from DT to register with OPP framework and use
> dev_pm_opp_set_rate() to set the clk/perf state.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Akash Asthana <akashast@codeaurora.org>
> Cc: linux-serial@vger.kernel.org

Picked up patch 1 and 2 through the qcom tree.

As Mark requested, please don't lump together patches that doesn't
actually depend on each other in the same series - it's quite confusing
to know what to pick and not.

Regards,
Bjorn

> ---
> This patch needs to land via the msm tree. Greg had this already pulled in,
> but later dropped it on my request.
> No change in v6, just resposting it here so Bjorn/Andy can pull it in.
> 
>  drivers/tty/serial/qcom_geni_serial.c | 34 +++++++++++++++++++++++++++++-----
>  include/linux/qcom-geni-se.h          |  4 ++++
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 457c0bf..a90f8ec 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_wakeirq.h>
> @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>  		goto out_restart_rx;
>  
>  	uport->uartclk = clk_rate;
> -	clk_set_rate(port->se.clk, clk_rate);
> +	dev_pm_opp_set_rate(uport->dev, clk_rate);
>  	ser_clk_cfg = SER_CLK_EN;
>  	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>  
> @@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>  	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>  		geni_se_resources_on(&port->se);
>  	else if (new_state == UART_PM_STATE_OFF &&
> -			old_state == UART_PM_STATE_ON)
> +			old_state == UART_PM_STATE_ON) {
> +		/* Drop the performance state vote */
> +		dev_pm_opp_set_rate(uport->dev, 0);
>  		geni_se_resources_off(&port->se);
> +	}
>  }
>  
>  static const struct uart_ops qcom_geni_console_pops = {
> @@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
>  		port->cts_rts_swap = true;
>  
> +	port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se");
> +	if (IS_ERR(port->se.opp_table))
> +		return PTR_ERR(port->se.opp_table);
> +	/* OPP table is optional */
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (!ret) {
> +		port->se.has_opp_table = true;
> +	} else if (ret != -ENODEV) {
> +		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
> +		return ret;
> +	}
> +
>  	uport->private_data = drv;
>  	platform_set_drvdata(pdev, port);
>  	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
>  
>  	ret = uart_add_one_port(drv, uport);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
>  	ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
> @@ -1365,7 +1381,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
>  		uart_remove_one_port(drv, uport);
> -		return ret;
> +		goto err;
>  	}
>  
>  	/*
> @@ -1382,11 +1398,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  		if (ret) {
>  			device_init_wakeup(&pdev->dev, false);
>  			uart_remove_one_port(drv, uport);
> -			return ret;
> +			goto err;
>  		}
>  	}
>  
>  	return 0;
> +err:
> +	if (port->se.has_opp_table)
> +		dev_pm_opp_of_remove_table(&pdev->dev);
> +	dev_pm_opp_put_clkname(port->se.opp_table);
> +	return ret;
>  }
>  
>  static int qcom_geni_serial_remove(struct platform_device *pdev)
> @@ -1394,6 +1415,9 @@ static int qcom_geni_serial_remove(struct platform_device *pdev)
>  	struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
>  	struct uart_driver *drv = port->uport.private_data;
>  
> +	if (port->se.has_opp_table)
> +		dev_pm_opp_of_remove_table(&pdev->dev);
> +	dev_pm_opp_put_clkname(port->se.opp_table);
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	device_init_wakeup(&pdev->dev, false);
>  	uart_remove_one_port(drv, &port->uport);
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index dd46494..6b78094 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -33,6 +33,8 @@ struct clk;
>   * @clk:		Handle to the core serial engine clock
>   * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>   * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
> + * @opp_table:		Pointer to the OPP table
> + * @has_opp_table:	Specifies if the SE has an OPP table
>   */
>  struct geni_se {
>  	void __iomem *base;
> @@ -41,6 +43,8 @@ struct geni_se {
>  	struct clk *clk;
>  	unsigned int num_clk_levels;
>  	unsigned long *clk_perf_tbl;
> +	struct opp_table *opp_table;
> +	bool has_opp_table;
>  };
>  
>  /* Common SE registers */
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Matthias Kaehlcke]

On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote:
> > On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote:
> > > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:
> > 
> > > > Wait, so *some* of the series should go together but not other bits?
> > > > But you want them split up for some reason?
> > 
> > > Yes, this will almost certainly be the case, even if not for this patch.
> > > I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).
> > 
> > I'm not really reading any of this stuff for the series as a whole, as
> > far as I could tell I'd reviewed all my bits and was hoping whatever
> > random platform stuff needs sorting out was going to be sorted out so I
> > stopped getting copied on revisions :(
> 
> Sorry this caused you extra work, I only fully realized this when the series
> was basically ready to land :(
> 
> Avoiding unnecessary revision spam is another good reason to not combine
> technically unrelated patches in a single series.
> 
> If I notice similar series in the future I'll try to bring it up early.
> 
> > > For the QSPI patch you could argue to just take it through QCOM since the SPI
> > > patch of this series goes through this tree, up to you, I just want to make
> > > sure everybody is on the same page.
> > 
> > If there are some part of this that don't have a connection with the
> > rest of the series and should be applied separately please split them
> > out and send them separately so it's clear what's going on.
> 
> Rajendra, IIUC you have to re-spin this series anyway, please split it
> up in self-contained chunks.

One more thing: when you do the split it seems it would make sense to
include the DT changes that were initially part of this series
(https://patchwork.kernel.org/project/linux-arm-msm/list/?series=278691&state=*)

Re: [PATCH v6 5/6] media: venus: core: Add support for opp tables/perf voting

[Rajendra Nayak]

On 6/18/2020 8:24 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
> 
> On 6/15/20 3:02 PM, Rajendra Nayak wrote:
>> Add support to add OPP tables and perf voting on the OPP powerdomain.
>> This is needed so venus votes on the corresponding performance state
>> for the OPP powerdomain along with setting the core clock rate.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> Cc: linux-media@vger.kernel.org
>> ---
>> No functional change in v6, rebased over 5.8-rc1
>> Bindings update to add optional PD https://lore.kernel.org/patchwork/patch/1241077/
>>
>>   drivers/media/platform/qcom/venus/core.c       | 43 +++++++++++++++++---
>>   drivers/media/platform/qcom/venus/core.h       |  5 +++
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++++++++++++++++++++++++--
>>   3 files changed, 92 insertions(+), 10 deletions(-)
>>
> 
> <cut>
> 
>>   
>> @@ -740,13 +740,16 @@ static int venc_power_v4(struct device *dev, int on)
>>   
>>   static int vcodec_domains_get(struct device *dev)
>>   {
>> +	int ret;
>> +	struct opp_table *opp_table;
>> +	struct device **opp_virt_dev;
>>   	struct venus_core *core = dev_get_drvdata(dev);
>>   	const struct venus_resources *res = core->res;
>>   	struct device *pd;
>>   	unsigned int i;
>>   
>>   	if (!res->vcodec_pmdomains_num)
>> -		return -ENODEV;
>> +		goto skip_pmdomains;
>>   
>>   	for (i = 0; i < res->vcodec_pmdomains_num; i++) {
>>   		pd = dev_pm_domain_attach_by_name(dev,
>> @@ -763,7 +766,41 @@ static int vcodec_domains_get(struct device *dev)
>>   	if (!core->pd_dl_venus)
>>   		return -ENODEV;
>>   
>> +skip_pmdomains:
>> +	if (!core->has_opp_table)
>> +		return 0;
>> +
>> +	/* Attach the power domain for setting performance state */
>> +	opp_table = dev_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
>> +	if (IS_ERR(opp_table)) {
>> +		ret = PTR_ERR(opp_table);
>> +		goto opp_attach_err;
>> +	}
>> +
>> +	core->opp_pmdomain = *opp_virt_dev;
>> +	core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
>> +					     DL_FLAG_RPM_ACTIVE |
>> +					     DL_FLAG_PM_RUNTIME |
>> +					     DL_FLAG_STATELESS);
>> +	if (!core->opp_dl_venus) {
>> +		ret = -ENODEV;
>> +		goto opp_dl_add_err;
>> +	}
>> +
>>   	return 0;
>> +
>> +opp_dl_add_err:
>> +	dev_pm_domain_detach(core->opp_pmdomain, true);
>> +opp_attach_err:
>> +	if (core->pd_dl_venus) {
>> +		device_link_del(core->pd_dl_venus);
>> +		for (i = 0; i < res->vcodec_pmdomains_num; i++) {
>> +			if (IS_ERR_OR_NULL(core->pmdomains[i]))
>> +				continue;
>> +			dev_pm_domain_detach(core->pmdomains[i], true);
>> +		}
>> +	}
>> +	return ret;
>>   }
>>   
>>   static void vcodec_domains_put(struct device *dev)
>> @@ -773,7 +810,7 @@ static void vcodec_domains_put(struct device *dev)
>>   	unsigned int i;
>>   
>>   	if (!res->vcodec_pmdomains_num)
>> -		return;
>> +		goto skip_pmdomains;
>>   
>>   	if (core->pd_dl_venus)
>>   		device_link_del(core->pd_dl_venus);
>> @@ -783,6 +820,15 @@ static void vcodec_domains_put(struct device *dev)
>>   			continue;
>>   		dev_pm_domain_detach(core->pmdomains[i], true);
>>   	}
>> +
>> +skip_pmdomains:
>> +	if (!res->opp_pmdomain)
>> +		return;
>> +
>> +	if (core->opp_dl_venus)
>> +		device_link_del(core->opp_dl_venus);
>> +
>> +	dev_pm_domain_detach(core->opp_pmdomain, true);
> 
> Without corresponding changes in venus DT node we call pm_domain_detach
> with core->opp_pmdomain = NULL which triggers NULL pointer dereference.
> 
> I guess you should check:
> 
> 	if (core->has_opp_table)
> 		dev_pm_domain_detach(core->opp_pmdomain, true);
> 
> or
> 
> 	if (core->opp_pmdomain)
> 		dev_pm_domain_detach(core->opp_pmdomain, true);
> 
> 
> ... not sure which one is more appropriate or both are.

Thanks, I'll fix that up when I repost. Sorry I was out for a while so
haven't been able to reproduce the rpmh timeout issue you reported.
Will spend some time on it this week and see if I can reproduce it.


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Rajendra Nayak]

On 6/24/2020 11:42 PM, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote:
>> On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote:
>>> On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:
>>
>>>> Wait, so *some* of the series should go together but not other bits?
>>>> But you want them split up for some reason?
>>
>>> Yes, this will almost certainly be the case, even if not for this patch.
>>> I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).
>>
>> I'm not really reading any of this stuff for the series as a whole, as
>> far as I could tell I'd reviewed all my bits and was hoping whatever
>> random platform stuff needs sorting out was going to be sorted out so I
>> stopped getting copied on revisions :(
> 
> Sorry this caused you extra work, I only fully realized this when the series
> was basically ready to land :(
> 
> Avoiding unnecessary revision spam is another good reason to not combine
> technically unrelated patches in a single series.
> 
> If I notice similar series in the future I'll try to bring it up early.
> 
>>> For the QSPI patch you could argue to just take it through QCOM since the SPI
>>> patch of this series goes through this tree, up to you, I just want to make
>>> sure everybody is on the same page.
>>
>> If there are some part of this that don't have a connection with the
>> rest of the series and should be applied separately please split them
>> out and send them separately so it's clear what's going on.
> 
> Rajendra, IIUC you have to re-spin this series anyway, please split it
> up in self-contained chunks.

Thanks, I'll respin these as separate patches, the only reason to club them
was because they all added 'similar' support in all these different drivers.
Sorry for the delay, I had been out a bit and I just got back.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state

[Rajendra Nayak]

On 6/25/2020 8:55 PM, Matthias Kaehlcke wrote:
> On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote:
>> On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote:
>>> On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote:
>>>> On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote:
>>>
>>>>> Wait, so *some* of the series should go together but not other bits?
>>>>> But you want them split up for some reason?
>>>
>>>> Yes, this will almost certainly be the case, even if not for this patch.
>>>> I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709).
>>>
>>> I'm not really reading any of this stuff for the series as a whole, as
>>> far as I could tell I'd reviewed all my bits and was hoping whatever
>>> random platform stuff needs sorting out was going to be sorted out so I
>>> stopped getting copied on revisions :(
>>
>> Sorry this caused you extra work, I only fully realized this when the series
>> was basically ready to land :(
>>
>> Avoiding unnecessary revision spam is another good reason to not combine
>> technically unrelated patches in a single series.
>>
>> If I notice similar series in the future I'll try to bring it up early.
>>
>>>> For the QSPI patch you could argue to just take it through QCOM since the SPI
>>>> patch of this series goes through this tree, up to you, I just want to make
>>>> sure everybody is on the same page.
>>>
>>> If there are some part of this that don't have a connection with the
>>> rest of the series and should be applied separately please split them
>>> out and send them separately so it's clear what's going on.
>>
>> Rajendra, IIUC you have to re-spin this series anyway, please split it
>> up in self-contained chunks.
> 
> One more thing: when you do the split it seems it would make sense to
> include the DT changes that were initially part of this series
> (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=278691&state=*)

Sure, I'll send the ones out for which driver changes are already merged/pulled in,
like sdhc, geni-uart and geni-spi.
For the rest, I will include them with the driver changes.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Stephen Boyd]

Quoting Rajendra Nayak (2020-06-15 05:02:39)
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 457c0bf..a90f8ec 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_wakeirq.h>
> @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>                 goto out_restart_rx;
>  
>         uport->uartclk = clk_rate;
> -       clk_set_rate(port->se.clk, clk_rate);
> +       dev_pm_opp_set_rate(uport->dev, clk_rate);

If there isn't an OPP table for the device because it is optional then
how can we unconditionally call dev_pm_opp_set_rate()?

>         ser_clk_cfg = SER_CLK_EN;
>         ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>  
> @@ -1231,8 +1232,11 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>         if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>                 geni_se_resources_on(&port->se);
>         else if (new_state == UART_PM_STATE_OFF &&
> -                       old_state == UART_PM_STATE_ON)
> +                       old_state == UART_PM_STATE_ON) {
> +               /* Drop the performance state vote */
> +               dev_pm_opp_set_rate(uport->dev, 0);
>                 geni_se_resources_off(&port->se);
> +       }
>  }
>  
>  static const struct uart_ops qcom_geni_console_pops = {
> @@ -1351,13 +1355,25 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>         if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
>                 port->cts_rts_swap = true;
>  
> +       port->se.opp_table = dev_pm_opp_set_clkname(&pdev->dev, "se");
> +       if (IS_ERR(port->se.opp_table))
> +               return PTR_ERR(port->se.opp_table);
> +       /* OPP table is optional */
> +       ret = dev_pm_opp_of_add_table(&pdev->dev);
> +       if (!ret) {
> +               port->se.has_opp_table = true;
> +       } else if (ret != -ENODEV) {
> +               dev_err(&pdev->dev, "invalid OPP table in device tree\n");
> +               return ret;
> +       }

At least it looks optional here.

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

On 6/30/2020 4:47 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2020-06-15 05:02:39)
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 457c0bf..a90f8ec 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>> +#include <linux/pm_opp.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/pm_wakeirq.h>
>> @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>                  goto out_restart_rx;
>>   
>>          uport->uartclk = clk_rate;
>> -       clk_set_rate(port->se.clk, clk_rate);
>> +       dev_pm_opp_set_rate(uport->dev, clk_rate);
> 
> If there isn't an OPP table for the device because it is optional then
> how can we unconditionally call dev_pm_opp_set_rate()?

because we have 'aca48b6 opp: Manage empty OPP tables with clk handle' to handle this.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Viresh Kumar]

On 30-06-20, 08:31, Rajendra Nayak wrote:
> 
> 
> On 6/30/2020 4:47 AM, Stephen Boyd wrote:
> > Quoting Rajendra Nayak (2020-06-15 05:02:39)
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > index 457c0bf..a90f8ec 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > @@ -9,6 +9,7 @@
> > >   #include <linux/module.h>
> > >   #include <linux/of.h>
> > >   #include <linux/of_device.h>
> > > +#include <linux/pm_opp.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/pm_wakeirq.h>
> > > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > >                  goto out_restart_rx;
> > >          uport->uartclk = clk_rate;
> > > -       clk_set_rate(port->se.clk, clk_rate);
> > > +       dev_pm_opp_set_rate(uport->dev, clk_rate);
> > 
> > If there isn't an OPP table for the device because it is optional then
> > how can we unconditionally call dev_pm_opp_set_rate()?

Looks like some *Maintainers* aren't paying enough attention lately ;)

Just kidding.

> because we have 'aca48b6 opp: Manage empty OPP tables with clk handle' to handle this.

-- 
viresh

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Rajendra Nayak]

On 6/25/2020 10:38 AM, Bjorn Andersson wrote:
> On Mon 15 Jun 05:02 PDT 2020, Rajendra Nayak wrote:
> 
>> geni serial needs to express a perforamnce state requirement on CX
>> powerdomain depending on the frequency of the clock rates.
>> Use OPP table from DT to register with OPP framework and use
>> dev_pm_opp_set_rate() to set the clk/perf state.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Akash Asthana <akashast@codeaurora.org>
>> Cc: linux-serial@vger.kernel.org
> 
> Picked up patch 1 and 2 through the qcom tree.

thanks Bjorn

> As Mark requested, please don't lump together patches that doesn't
> actually depend on each other in the same series - it's quite confusing
> to know what to pick and not.

Sorry for the confusion, I will try and split them into driver specific patches henceforth.
For this series though, I see that you have merged the entire ICC series for
QUP and QSPI into for-next of the qcom tree.
In which case it perhaps makes sense for you to pull in the QSPI change from this series
as well? i.e PATCH 6/6.
If so, I will rebase the patch on your for-next and resend (Its already Ack'ed by Mark)

thanks,
Rajendra

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180

[Rajendra Nayak]

Hey Stan,

>>
>> Hey Stan,
>>   
>>> On 6/18/20 7:47 AM, Rajendra Nayak wrote:
>>>> Hey Matthias, thanks for summarizing this.
>>>>
>>>> On 6/18/2020 3:45 AM, Matthias Kaehlcke wrote:
>>>>> What is the plan for landing these, it seems not all must/should
>>>>> go through the QCOM tree.
>>>>>
>>>>> My guesses:
>>>>>
>>>>> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
>>>>> spi: spi-geni-qcom: Use OPP API to set clk/perf state
>>>>>      QCOM tree due to shared dependency on change in
>>>>> include/linux/qcom-geni-se.h
>>>>
>>>> That's correct, Bjorn/Andy, can these be pulled in now for 5.9?
>>>> They have acks from Greg for serial and Mark for the spi patch.
>>>>   
>>>>> drm/msm/dpu: Use OPP API to set clk/perf state
>>>>> drm/msm: dsi: Use OPP API to set clk/perf state
>>>>>      drm/msm tree
>>>>
>>>> Correct, the dsi patch is still not reviewed by Rob, so once that's
>>>> done,
>>>> I am guessing Rob would pull both of these.
>>>>
>>>>>
>>>>> media: venus: core: Add support for opp tables/perf voting
>>>>>      venus tree
>>>>
>>>> correct, this is pending review/ack from Stan.
>>>
>>> I tested the changes in the driver, and they looks fine. But when
>>> applied the corresponding change in the DT node I see this message when
>>> the streaming is stopping:
>>>
>>> qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x30000
>>>
>>> I tested on v5.7 (linaro-integration). Should I be worried ?

I had a look at this message and also spoke to Maulik, and it seems
like its fairly harmless. It just suggests we had to wait a while to
get a TCS, and then retry, So nothing to worry about.

thanks,
Rajendra

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Stephen Boyd]

Quoting Viresh Kumar (2020-06-29 20:05:52)
> On 30-06-20, 08:31, Rajendra Nayak wrote:
> > 
> > 
> > On 6/30/2020 4:47 AM, Stephen Boyd wrote:
> > > Quoting Rajendra Nayak (2020-06-15 05:02:39)
> > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > > index 457c0bf..a90f8ec 100644
> > > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > > @@ -9,6 +9,7 @@
> > > >   #include <linux/module.h>
> > > >   #include <linux/of.h>
> > > >   #include <linux/of_device.h>
> > > > +#include <linux/pm_opp.h>
> > > >   #include <linux/platform_device.h>
> > > >   #include <linux/pm_runtime.h>
> > > >   #include <linux/pm_wakeirq.h>
> > > > @@ -962,7 +963,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > > >                  goto out_restart_rx;
> > > >          uport->uartclk = clk_rate;
> > > > -       clk_set_rate(port->se.clk, clk_rate);
> > > > +       dev_pm_opp_set_rate(uport->dev, clk_rate);
> > > 
> > > If there isn't an OPP table for the device because it is optional then
> > > how can we unconditionally call dev_pm_opp_set_rate()?
> 
> Looks like some *Maintainers* aren't paying enough attention lately ;)
> 
> Just kidding.
> 

It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds
something that isn't an error pointer but then dev_pm_opp_of_add_table()
returns an error value because there isn't an operating-points property
in DT. We're getting saved because this driver also happens to call
dev_pm_opp_set_clkname() which allocates the OPP table a second time
(because the first time it got freed when dev_pm_opp_of_add_table()
return -ENODEV because the property was missing).

Why do we need 'has_opp_table' logic? It seems that we have to keep
track of the fact that dev_pm_opp_of_add_table() failed so that we don't
put the table again, but then dev_pm_opp_set_clkname() can be called
to allocate the table regardless.

This maintainer is paying very close attention to super confusing code like
this:

	if (drv->has_opp_table)
		dev_pm_opp_of_remove_table(dev);
	dev_pm_opp_put_clkname(drv->opp_table);

which reads as "if I have an opp table remove it and oh by the way
remove the clk name for this opp table pointer I also happen to always
have".

Maybe I would be happier if dev_pm_opp_of_table() went away and we just
had dev_pm_opp_add_table(const struct opp_config *config) that did all
the things for us like set a clk name, set the supported hw, set the
prop name, etc. based on the single config struct pointer and also
parsed out the OPP table from DT or just ignored that if there isn't any
operating-points property. Then the caller wouldn't need to keep track
of 'if has_opp_table' because it doesn't seem to actually care and the
core is happy to allocate a table for the device anyway so long as it
sets a clk name.

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Viresh Kumar]

On 21-07-20, 01:43, Stephen Boyd wrote:
> It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds
> something that isn't an error pointer but then dev_pm_opp_of_add_table()
> returns an error value because there isn't an operating-points property
> in DT. We're getting saved because this driver also happens to call
> dev_pm_opp_set_clkname() which allocates the OPP table a second time
> (because the first time it got freed when dev_pm_opp_of_add_table()
> return -ENODEV because the property was missing).
> 
> Why do we need 'has_opp_table' logic? It seems that we have to keep
> track of the fact that dev_pm_opp_of_add_table() failed so that we don't
> put the table again, but then dev_pm_opp_set_clkname() can be called
> to allocate the table regardless.
> 
> This maintainer is paying very close attention

:)

> to super confusing code like
> this:
> 
> 	if (drv->has_opp_table)
> 		dev_pm_opp_of_remove_table(dev);
> 	dev_pm_opp_put_clkname(drv->opp_table);
> 
> which reads as "if I have an opp table remove it and oh by the way
> remove the clk name for this opp table pointer I also happen to always
> have".
> 
> Maybe I would be happier if dev_pm_opp_of_table() went away and we just
> had dev_pm_opp_add_table(const struct opp_config *config) that did all
> the things for us like set a clk name, set the supported hw, set the
> prop name, etc. based on the single config struct pointer and also
> parsed out the OPP table from DT or just ignored that if there isn't any
> operating-points property. Then the caller wouldn't need to keep track
> of 'if has_opp_table' because it doesn't seem to actually care and the
> core is happy to allocate a table for the device anyway so long as it
> sets a clk name.

The config style wouldn't work as well as we don't really want to
allocate an OPP table if the property isn't found in DT.

All the mess is coming from the fact that I wanted to make it easy for
the generic drivers to have code which can do opp-set-rate or
clk-set-rate depending on how the platform is configured. While the
intention was fine, the end result is still not great as you figured
out.

Because we need to keep a flag to make the right decision anyway, I
wonder if doing this is the best solution we have at hand.

if (opp-table-present)
        opp_set_rate();
else
        clk_set_rate();

Or maybe stop printing errors from dev_pm_opp_of_remove_table() if the
OPP table isn't found. And so we can get rid of the flag.

-- 
viresh

Re: [PATCH v6 1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Viresh Kumar]

On 22-07-20, 10:54, Viresh Kumar wrote:
> On 21-07-20, 01:43, Stephen Boyd wrote:
> > It seems that dev_pm_opp_set_rate() calls _find_opp_table() and finds
> > something that isn't an error pointer but then dev_pm_opp_of_add_table()
> > returns an error value because there isn't an operating-points property
> > in DT. We're getting saved because this driver also happens to call
> > dev_pm_opp_set_clkname() which allocates the OPP table a second time
> > (because the first time it got freed when dev_pm_opp_of_add_table()
> > return -ENODEV because the property was missing).
> > 
> > Why do we need 'has_opp_table' logic? It seems that we have to keep
> > track of the fact that dev_pm_opp_of_add_table() failed so that we don't
> > put the table again, but then dev_pm_opp_set_clkname() can be called
> > to allocate the table regardless.

I have sent a patchset to clean this stuff up a bit now.

-- 
viresh