[RFC/PATCH 0/5] DVFS in the OPP core

[RFC/PATCH 0/5] DVFS in the OPP core

[Stephen Boyd]

This patch series is an RFC around how we can implement DVFS for devices
that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
strict set of frequencies that they have been tested at to derive some
operating performance point. Instead they have a coarser set of
frequency max or 'fmax' OPPs that describe the maiximum frequency the
device can operate at with a given voltage.

The approach we take is to let the devm_pm_opp_set_rate() API accept 0
as a valid frequency indicating the frequency isn't required anymore and
to make the same API use the clk framework to round the frequency passed
in instead of relying on the OPP table to specify each frequency that
can be used. Once we have these two patches in place, we can use the OPP
API to change clk rates instead of clk_set_rate() and use all the recent
OPP enhancements that have been made around required-opps and genpd
performance states to do DVFS for all devices.

One nice feature of this approach is that we don't need to change the
OPP binding to support this. We can specify only the max frequencies and
the voltage requirements in DT with the existing binding and slightly
tweak the OPP code to achieve these results. 

This series includes a conversion of the uart and spi drivers on
qcom sdm845 where these patches are being developed. It shows how a
driver is converted from the clk APIs to the OPP APIs and how
enable/disable state of the clk is communicated to the OPP layer.

Some open topics and initial points for discussion are:

1) The dev_pm_opp_set_rate() API changes may break something that's 
relying on the rate rounding that OPP provides. If those exist,
we may need to implement another API that is more explicit about using
the clk API instead of the OPP tables.

2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
dropping the rate requirement. Is there anything better than this?

3) How do we handle devices that already have power-domains specified in
DT? The opp binding for required-opps doesn't let us specify the power
domain to target, instead it assumes that whatever power domain is
attached to a device is the one that OPP needs to use to change the
genpd performance state. Do we need a
dev_pm_opp_set_required_opps_name() or something to be explicit about
this? Can we have some way for the power domain that required-opps
correspond to be expressed in the OPP tables themselves?

4) How do we achieve the "full constraint" state? i.e. what do we do
about some devices being active and others being inactive during boot
and making sure that the voltage for the shared power domains doesn't
drop until all devices requiring it have informed OPP about their
power requirements?

Rajendra Nayak (4):
  OPP: Make dev_pm_opp_set_rate() with freq=0 as valid
  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
  arm64: dts: sdm845: Add OPP table for all qup devices

Stephen Boyd (1):
  OPP: Don't overwrite rounded clk rate

 arch/arm64/boot/dts/qcom/sdm845.dtsi  | 115 ++++++++++++++++++++++++++
 drivers/opp/core.c                    |  26 ++++--
 drivers/spi/spi-geni-qcom.c           |  12 ++-
 drivers/tty/serial/qcom_geni_serial.c |  15 +++-
 4 files changed, 155 insertions(+), 13 deletions(-)

For the interested, these patches are located here:

   https://github.com/rrnayak/linux/ v5.0-rc3/opp-corners-wip

I've generated these patches by cutting off the top of that tree and
massaging the commit text a bit for the first patch.

base-commit: 49a57857aeea06ca831043acbb0fa5e0f50602fd
prerequisite-patch-id: 9c3ee728603596b8b0ba06ffd66084bdc21b1271
prerequisite-patch-id: f160e050bcd74f5de6fad66329381853869a6a97
prerequisite-patch-id: aa23548d2b486c29489b4304d85799d08762254e
prerequisite-patch-id: 40dd117c45fecb4308298352346546612db94b64
prerequisite-patch-id: cd102fa42bab19897c2295e8b990b2156626054a
prerequisite-patch-id: 3b9e5c8ed65ee96cc0f1c50166cf6bbb597ef582
prerequisite-patch-id: 7e71b957b90ad51d0619944d5ebc859380e8e3e4
prerequisite-patch-id: 5abd2bd6b3ae3e91551e2b8f9295169019ba82c7
prerequisite-patch-id: 68bb3e44cf4e5dbd363a1a1750e4d378971ed393
prerequisite-patch-id: 882b14ef9527b15d22cfddbb5fa2b9d43df1ff04
prerequisite-patch-id: 6fc14ddeb074fb0826f1f40031e9d161f1361666
-- 
Sent by a computer through tubes

[RFC/PATCH 1/5] OPP: Don't overwrite rounded clk rate

[Stephen Boyd]

Doing this allows us to call this API with any rate requested and have
it not need to match in the OPP table. Instead, we'll round the rate up
to the nearest OPP that we see so that we can get the voltage or level
that's required for that OPP. This supports users of OPP that want to
specify the 'fmax' tables of a device instead of every single frequency
that they need. And for devices that required the exact frequency, we
can rely on the clk framework to round the rate to the nearest supported
frequency instead of the OPP framework to do so.

Note that this may affect drivers that don't want the clk framework to
do rounding, but instead want the OPP table to do the rounding for them.
Do we have that case? Should we add some flag to the OPP table to
indicate this and then not have that flag set when there isn't an OPP
table for the device and also introduce a property like 'opp-use-clk' to
tell the table that it should use the clk APIs to round rates instead of
OPP?

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/opp/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e06a0ab05ad6..b6516d623c5a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -704,7 +704,7 @@ static int _set_required_opps(struct device *dev,
 int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 {
 	struct opp_table *opp_table;
-	unsigned long freq, old_freq;
+	unsigned long freq, opp_freq, old_freq, old_opp_freq;
 	struct dev_pm_opp *old_opp, *opp;
 	struct clk *clk;
 	int ret;
@@ -743,13 +743,15 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		goto put_opp_table;
 	}
 
-	old_opp = _find_freq_ceil(opp_table, &old_freq);
+	old_opp_freq = old_freq;
+	old_opp = _find_freq_ceil(opp_table, &old_opp_freq);
 	if (IS_ERR(old_opp)) {
 		dev_err(dev, "%s: failed to find current OPP for freq %lu (%ld)\n",
 			__func__, old_freq, PTR_ERR(old_opp));
 	}
 
-	opp = _find_freq_ceil(opp_table, &freq);
+	opp_freq = freq;
+	opp = _find_freq_ceil(opp_table, &opp_freq);
 	if (IS_ERR(opp)) {
 		ret = PTR_ERR(opp);
 		dev_err(dev, "%s: failed to find OPP for freq %lu (%d)\n",
-- 
Sent by a computer through tubes

[RFC/PATCH 2/5] OPP: Make dev_pm_opp_set_rate() with freq=0 as valid

[Stephen Boyd]

From: Rajendra Nayak <rnayak@codeaurora.org>

For devices with performance state, we use dev_pm_opp_set_rate()
to set the appropriate clk rate and the performance state.
We do need a way to *remove* the performance state vote when
we idle the device and turn the clocks off. Use dev_pm_opp_set_rate()
with freq=0 to achieve this.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/opp/core.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index b6516d623c5a..ac5099c237de 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -709,18 +709,24 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 	struct clk *clk;
 	int ret;
 
-	if (unlikely(!target_freq)) {
-		dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
-			target_freq);
-		return -EINVAL;
-	}
-
 	opp_table = _find_opp_table(dev);
 	if (IS_ERR(opp_table)) {
 		dev_err(dev, "%s: device opp doesn't exist\n", __func__);
 		return PTR_ERR(opp_table);
 	}
 
+	if (unlikely(!target_freq)) {
+		if (opp_table->required_opp_tables) {
+			/* drop the performance state vote */
+			dev_pm_genpd_set_performance_state(dev, 0);
+			return 0;
+		} else {
+			dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
+				target_freq);
+			return -EINVAL;
+		}
+	}
+
 	clk = opp_table->clk;
 	if (IS_ERR(clk)) {
 		dev_err(dev, "%s: No clock available for the device\n",
-- 
Sent by a computer through tubes

[RFC/PATCH 3/5] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state

[Stephen Boyd]

From: Rajendra Nayak <rnayak@codeaurora.org>

geni serial 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>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index a72d6d9fb983..dca8f6845463 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -12,6 +12,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/qcom-geni-se.h>
 #include <linux/serial.h>
@@ -119,6 +120,7 @@ struct qcom_geni_serial_port {
 	bool brk;
 
 	unsigned int tx_remaining;
+	struct device *dev;
 };
 
 static const struct uart_ops qcom_geni_console_pops;
@@ -1028,7 +1030,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(port->dev, clk_rate);
 	ser_clk_cfg = SER_CLK_EN;
 	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
 
@@ -1265,8 +1267,10 @@ 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) {
+		dev_pm_opp_set_rate(port->dev, 0);
 		geni_se_resources_off(&port->se);
+	}
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
@@ -1332,6 +1336,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Invalid line %d\n", line);
 		return PTR_ERR(port);
 	}
+	port->dev = &pdev->dev;
 
 	uport = &port->uport;
 	/* Don't allow 2 drivers to access the same port */
@@ -1353,6 +1358,12 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 		return -EINVAL;
 	uport->mapbase = res->start;
 
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to init OPP table: %d\n", ret);
+		return ret;
+	}
+
 	port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
 	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
 	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
-- 
Sent by a computer through tubes

[RFC/PATCH 4/5] spi: spi-geni-qcom: Use OPP API to set clk/perf state

[Stephen Boyd]

From: Rajendra Nayak <rnayak@codeaurora.org>

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>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/spi/spi-geni-qcom.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index fdb7cb88fb56..3ffb6b25b58d 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -116,9 +116,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;
 }
 
@@ -564,6 +564,12 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (!spi)
 		return -ENOMEM;
 
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to init OPP table: %d\n", ret);
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, spi);
 	mas = spi_master_get_devdata(spi);
 	mas->irq = irq;
@@ -629,6 +635,8 @@ 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);
 }
 
-- 
Sent by a computer through tubes

[RFC/PATCH 5/5] arm64: dts: sdm845: Add OPP table for all qup devices

[Stephen Boyd]

From: Rajendra Nayak <rnayak@codeaurora.org>

qup has a requirement to vote on the performance state of the CX domain
in sdm845 devices. Add OPP tables for these and also add power-domains
property for all qup instances.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 115 +++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 22dc2b2df662..05c4862a195c 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -366,6 +366,25 @@
 			clock-names = "core";
 		};
 
+		qup_opp_table: opp-table {
+			compatible = "operating-points-v2";
+
+			opp-19200000 {
+				opp-hz = /bits/ 64 <19200000>;
+				required-opps = <&rpmhpd_opp_min_svs>;
+			};
+
+			opp-75000000 {
+				opp-hz = /bits/ 64 <75000000>;
+				required-opps = <&rpmhpd_opp_low_svs>;
+			};
+
+			opp-100000000 {
+				opp-hz = /bits/ 64 <100000000>;
+				required-opps = <&rpmhpd_opp_svs>;
+			};
+		};
+
 		qupv3_id_0: geniqup@8c0000 {
 			compatible = "qcom,geni-se-qup";
 			reg = <0x8c0000 0x6000>;
@@ -387,6 +406,8 @@
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -400,6 +421,8 @@
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -411,6 +434,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart0_default>;
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -424,6 +449,8 @@
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -437,6 +464,8 @@
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -448,6 +477,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart1_default>;
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -461,6 +492,8 @@
 				interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -474,6 +507,8 @@
 				interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -485,6 +520,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart2_default>;
 				interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -498,6 +535,8 @@
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -511,6 +550,8 @@
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -522,6 +563,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart3_default>;
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -535,6 +578,8 @@
 				interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -548,6 +593,8 @@
 				interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -559,6 +606,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart4_default>;
 				interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -572,6 +621,8 @@
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -585,6 +636,8 @@
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -596,6 +649,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart5_default>;
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -609,6 +664,8 @@
 				interrupts = <GIC_SPI 607 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -622,6 +679,8 @@
 				interrupts = <GIC_SPI 607 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -633,6 +692,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart6_default>;
 				interrupts = <GIC_SPI 607 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -646,6 +707,8 @@
 				interrupts = <GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -659,6 +722,8 @@
 				interrupts = <GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -670,6 +735,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart7_default>;
 				interrupts = <GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 		};
@@ -695,6 +762,8 @@
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -708,6 +777,8 @@
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -719,6 +790,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart8_default>;
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -732,6 +805,8 @@
 				interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -745,6 +820,8 @@
 				interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -756,6 +833,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart9_default>;
 				interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -769,6 +848,8 @@
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -782,6 +863,8 @@
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -793,6 +876,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart10_default>;
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -806,6 +891,8 @@
 				interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -819,6 +906,8 @@
 				interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -830,6 +919,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart11_default>;
 				interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -843,6 +934,8 @@
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -856,6 +949,8 @@
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -867,6 +962,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart12_default>;
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -880,6 +977,8 @@
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -893,6 +992,8 @@
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -904,6 +1005,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart13_default>;
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -917,6 +1020,8 @@
 				interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -930,6 +1035,8 @@
 				interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -941,6 +1048,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart14_default>;
 				interrupts = <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -954,6 +1063,8 @@
 				interrupts = <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -967,6 +1078,8 @@
 				interrupts = <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 
@@ -978,6 +1091,8 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart15_default>;
 				interrupts = <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SDM845_CX>;
+				operating-points-v2 = <&qup_opp_table>;
 				status = "disabled";
 			};
 		};
-- 
Sent by a computer through tubes

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

Adding few folks to the thread who might be interested in this stuff.

On 28-01-19, 17:55, Stephen Boyd wrote:
> This patch series is an RFC around how we can implement DVFS for devices
> that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> strict set of frequencies that they have been tested at to derive some
> operating performance point. Instead they have a coarser set of
> frequency max or 'fmax' OPPs that describe the maiximum frequency the
> device can operate at with a given voltage.
> 
> The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> as a valid frequency indicating the frequency isn't required anymore and
> to make the same API use the clk framework to round the frequency passed
> in instead of relying on the OPP table to specify each frequency that
> can be used. Once we have these two patches in place, we can use the OPP
> API to change clk rates instead of clk_set_rate() and use all the recent
> OPP enhancements that have been made around required-opps and genpd
> performance states to do DVFS for all devices.

Generally speaking I am fine with such an approach but I am not sure
about what others would say on this as they had objections to using
OPP core for setting the rate itself.

FWIW, I suggested exactly this solution sometime back [1]

- Drivers need to use two API sets to change clock rate (OPP helpers)
  and enable/disable them (CLK framework helpers) and this leads us to
  exactly that combination. Is that acceptable ? It doesn't look great
  to me as well..

- Do we expect the callers will disable clk before calling
  opp-set-rate with 0 ? We should remove the regulator requirements as
  well along with perf-state.

- What about enabling/disabling clock as well from OPP framework. We
  can enable it on the very first call to opp-set-rate and disable
  when freq is 0. That will simplify the drivers as well.

> One nice feature of this approach is that we don't need to change the
> OPP binding to support this. We can specify only the max frequencies and
> the voltage requirements in DT with the existing binding and slightly
> tweak the OPP code to achieve these results. 
> 
> This series includes a conversion of the uart and spi drivers on
> qcom sdm845 where these patches are being developed. It shows how a
> driver is converted from the clk APIs to the OPP APIs and how
> enable/disable state of the clk is communicated to the OPP layer.
> 
> Some open topics and initial points for discussion are:
> 
> 1) The dev_pm_opp_set_rate() API changes may break something that's 
> relying on the rate rounding that OPP provides. If those exist,
> we may need to implement another API that is more explicit about using
> the clk API instead of the OPP tables.
> 
> 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> dropping the rate requirement. Is there anything better than this?
> 
> 3) How do we handle devices that already have power-domains specified in
> DT? The opp binding for required-opps doesn't let us specify the power
> domain to target, instead it assumes that whatever power domain is
> attached to a device is the one that OPP needs to use to change the
> genpd performance state. Do we need a
> dev_pm_opp_set_required_opps_name() or something to be explicit about
> this? Can we have some way for the power domain that required-opps
> correspond to be expressed in the OPP tables themselves?
> 
> 4) How do we achieve the "full constraint" state? i.e. what do we do
> about some devices being active and others being inactive during boot
> and making sure that the voltage for the shared power domains doesn't
> drop until all devices requiring it have informed OPP about their
> power requirements?
> 
> Rajendra Nayak (4):
>   OPP: Make dev_pm_opp_set_rate() with freq=0 as valid
>   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
>   arm64: dts: sdm845: Add OPP table for all qup devices
> 
> Stephen Boyd (1):
>   OPP: Don't overwrite rounded clk rate
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi  | 115 ++++++++++++++++++++++++++
>  drivers/opp/core.c                    |  26 ++++--
>  drivers/spi/spi-geni-qcom.c           |  12 ++-
>  drivers/tty/serial/qcom_geni_serial.c |  15 +++-
>  4 files changed, 155 insertions(+), 13 deletions(-)
> 
> For the interested, these patches are located here:
> 
>    https://github.com/rrnayak/linux/ v5.0-rc3/opp-corners-wip
> 
> I've generated these patches by cutting off the top of that tree and
> massaging the commit text a bit for the first patch.
> 
> base-commit: 49a57857aeea06ca831043acbb0fa5e0f50602fd
> prerequisite-patch-id: 9c3ee728603596b8b0ba06ffd66084bdc21b1271
> prerequisite-patch-id: f160e050bcd74f5de6fad66329381853869a6a97
> prerequisite-patch-id: aa23548d2b486c29489b4304d85799d08762254e
> prerequisite-patch-id: 40dd117c45fecb4308298352346546612db94b64
> prerequisite-patch-id: cd102fa42bab19897c2295e8b990b2156626054a
> prerequisite-patch-id: 3b9e5c8ed65ee96cc0f1c50166cf6bbb597ef582
> prerequisite-patch-id: 7e71b957b90ad51d0619944d5ebc859380e8e3e4
> prerequisite-patch-id: 5abd2bd6b3ae3e91551e2b8f9295169019ba82c7
> prerequisite-patch-id: 68bb3e44cf4e5dbd363a1a1750e4d378971ed393
> prerequisite-patch-id: 882b14ef9527b15d22cfddbb5fa2b9d43df1ff04
> prerequisite-patch-id: 6fc14ddeb074fb0826f1f40031e9d161f1361666
> -- 
> Sent by a computer through tubes

-- 
viresh

[1] https://lore.kernel.org/linux-clk/20180704065522.p4qpfnpayeobaok3@vireshk-i7/

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Rafael J. Wysocki]

On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Adding few folks to the thread who might be interested in this stuff.
>
> On 28-01-19, 17:55, Stephen Boyd wrote:
> > This patch series is an RFC around how we can implement DVFS for devices
> > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > strict set of frequencies that they have been tested at to derive some
> > operating performance point. Instead they have a coarser set of
> > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > device can operate at with a given voltage.
> >
> > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > as a valid frequency indicating the frequency isn't required anymore and
> > to make the same API use the clk framework to round the frequency passed
> > in instead of relying on the OPP table to specify each frequency that
> > can be used. Once we have these two patches in place, we can use the OPP
> > API to change clk rates instead of clk_set_rate() and use all the recent
> > OPP enhancements that have been made around required-opps and genpd
> > performance states to do DVFS for all devices.
>
> Generally speaking I am fine with such an approach but I am not sure
> about what others would say on this as they had objections to using
> OPP core for setting the rate itself.
>
> FWIW, I suggested exactly this solution sometime back [1]
>
> - Drivers need to use two API sets to change clock rate (OPP helpers)
>   and enable/disable them (CLK framework helpers) and this leads us to
>   exactly that combination. Is that acceptable ? It doesn't look great
>   to me as well..

I agree here.

> - Do we expect the callers will disable clk before calling
>   opp-set-rate with 0 ? We should remove the regulator requirements as
>   well along with perf-state.

Well, disabling clk affects HW in general, doesn't it?

> - What about enabling/disabling clock as well from OPP framework. We
>   can enable it on the very first call to opp-set-rate and disable
>   when freq is 0. That will simplify the drivers as well.

That sounds compelling, but I guess there are cases in which you can
gate the clock regardless of the frequency setting.  How would that
work then?

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Adding few folks to the thread who might be interested in this stuff.
> >
> > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > This patch series is an RFC around how we can implement DVFS for devices
> > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > strict set of frequencies that they have been tested at to derive some
> > > operating performance point. Instead they have a coarser set of
> > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > device can operate at with a given voltage.
> > >
> > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > as a valid frequency indicating the frequency isn't required anymore and
> > > to make the same API use the clk framework to round the frequency passed
> > > in instead of relying on the OPP table to specify each frequency that
> > > can be used. Once we have these two patches in place, we can use the OPP
> > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > OPP enhancements that have been made around required-opps and genpd
> > > performance states to do DVFS for all devices.
> >
> > Generally speaking I am fine with such an approach but I am not sure
> > about what others would say on this as they had objections to using
> > OPP core for setting the rate itself.
> >
> > FWIW, I suggested exactly this solution sometime back [1]
> >
> > - Drivers need to use two API sets to change clock rate (OPP helpers)
> >   and enable/disable them (CLK framework helpers) and this leads us to
> >   exactly that combination. Is that acceptable ? It doesn't look great
> >   to me as well..
> 
> I agree here.
> 
> > - Do we expect the callers will disable clk before calling
> >   opp-set-rate with 0 ? We should remove the regulator requirements as
> >   well along with perf-state.
> 
> Well, disabling clk affects HW in general, doesn't it?

Yeah, but the regulator may be shared and is running at higher
voltages just because of the clock requirement of the device getting
disabled here. Or did I misunderstood what you wanted to say ?

> > - What about enabling/disabling clock as well from OPP framework. We
> >   can enable it on the very first call to opp-set-rate and disable
> >   when freq is 0. That will simplify the drivers as well.
> 
> That sounds compelling, but I guess there are cases in which you can
> gate the clock regardless of the frequency setting.  How would that
> work then?

Can you give any example here ? I am not sure I understood the concern
here.

-- 
viresh

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Rafael J. Wysocki]

On Thu, Jan 31, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Adding few folks to the thread who might be interested in this stuff.
> > >
> > > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > > This patch series is an RFC around how we can implement DVFS for devices
> > > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > > strict set of frequencies that they have been tested at to derive some
> > > > operating performance point. Instead they have a coarser set of
> > > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > > device can operate at with a given voltage.
> > > >
> > > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > > as a valid frequency indicating the frequency isn't required anymore and
> > > > to make the same API use the clk framework to round the frequency passed
> > > > in instead of relying on the OPP table to specify each frequency that
> > > > can be used. Once we have these two patches in place, we can use the OPP
> > > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > > OPP enhancements that have been made around required-opps and genpd
> > > > performance states to do DVFS for all devices.
> > >
> > > Generally speaking I am fine with such an approach but I am not sure
> > > about what others would say on this as they had objections to using
> > > OPP core for setting the rate itself.
> > >
> > > FWIW, I suggested exactly this solution sometime back [1]
> > >
> > > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > >   and enable/disable them (CLK framework helpers) and this leads us to
> > >   exactly that combination. Is that acceptable ? It doesn't look great
> > >   to me as well..
> >
> > I agree here.
> >
> > > - Do we expect the callers will disable clk before calling
> > >   opp-set-rate with 0 ? We should remove the regulator requirements as
> > >   well along with perf-state.
> >
> > Well, disabling clk affects HW in general, doesn't it?
>
> Yeah, but the regulator may be shared and is running at higher
> voltages just because of the clock requirement of the device getting
> disabled here. Or did I misunderstood what you wanted to say ?

What I wanted to say is that if the caller is required to disable clk
beforehand, that may be as good as setting its rate to zero already.

> > > - What about enabling/disabling clock as well from OPP framework. We
> > >   can enable it on the very first call to opp-set-rate and disable
> > >   when freq is 0. That will simplify the drivers as well.
> >
> > That sounds compelling, but I guess there are cases in which you can
> > gate the clock regardless of the frequency setting.  How would that
> > work then?
>
> Can you give any example here ? I am not sure I understood the concern
> here.

It looks like I was confused somehow, never mind. :-)

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 31-01-19, 11:36, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> > > On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > Adding few folks to the thread who might be interested in this stuff.
> > > >
> > > > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > > > This patch series is an RFC around how we can implement DVFS for devices
> > > > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > > > strict set of frequencies that they have been tested at to derive some
> > > > > operating performance point. Instead they have a coarser set of
> > > > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > > > device can operate at with a given voltage.
> > > > >
> > > > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > > > as a valid frequency indicating the frequency isn't required anymore and
> > > > > to make the same API use the clk framework to round the frequency passed
> > > > > in instead of relying on the OPP table to specify each frequency that
> > > > > can be used. Once we have these two patches in place, we can use the OPP
> > > > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > > > OPP enhancements that have been made around required-opps and genpd
> > > > > performance states to do DVFS for all devices.
> > > >
> > > > Generally speaking I am fine with such an approach but I am not sure
> > > > about what others would say on this as they had objections to using
> > > > OPP core for setting the rate itself.
> > > >
> > > > FWIW, I suggested exactly this solution sometime back [1]
> > > >
> > > > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > > >   and enable/disable them (CLK framework helpers) and this leads us to
> > > >   exactly that combination. Is that acceptable ? It doesn't look great
> > > >   to me as well..
> > >
> > > I agree here.
> > >
> > > > - Do we expect the callers will disable clk before calling
> > > >   opp-set-rate with 0 ? We should remove the regulator requirements as
> > > >   well along with perf-state.
> > >
> > > Well, disabling clk affects HW in general, doesn't it?
> >
> > Yeah, but the regulator may be shared and is running at higher
> > voltages just because of the clock requirement of the device getting
> > disabled here. Or did I misunderstood what you wanted to say ?
> 
> What I wanted to say is that if the caller is required to disable clk
> beforehand, that may be as good as setting its rate to zero already.

Right, but that doesn't mean that the requirements put on the
regulator and genpd are gone as well and that is why I favor the OPP
core to handle all the parts otherwise write the rules explicitly. If
the OPP core doesn't disable the clock and the caller also hasn't done
the same, then disabling the genpd/regulator may break things up.

-- 
viresh

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Rajendra Nayak]

> 3) How do we handle devices that already have power-domains specified in
> DT? The opp binding for required-opps doesn't let us specify the power
> domain to target, instead it assumes that whatever power domain is
> attached to a device is the one that OPP needs to use to change the
> genpd performance state. Do we need a
> dev_pm_opp_set_required_opps_name() or something to be explicit about
> this? Can we have some way for the power domain that required-opps
> correspond to be expressed in the OPP tables themselves?

I was converting a few more drivers to use the proposed approach in this
RFC, in order to identify all outstanding issues we need to deal with,
and specifically for UFS, I end up with this exact scenario where UFS already
has an existing power domain (gdsc) and I need to add another one (rpmhpd) for
setting the performance state.

If I use dev_pm_opp_of_add_table() to add the opp table from DT, the opp
layer assumes its the same device on which it can do a dev_pm_genpd_set_performance_state()
with, however the device that's actually associated with the pm_domain when we
have multiple power domains is infact the one (dummy) that we create when
the driver makes a call to dev_pm_domain_attach_by_name/id().

Any thoughts on whats a good way 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: [RFC/PATCH 0/5] DVFS in the OPP core

[Stephen Boyd]

Quoting Viresh Kumar (2019-01-31 01:23:49)
> Adding few folks to the thread who might be interested in this stuff.
> 
> On 28-01-19, 17:55, Stephen Boyd wrote:
> > This patch series is an RFC around how we can implement DVFS for devices
> > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > strict set of frequencies that they have been tested at to derive some
> > operating performance point. Instead they have a coarser set of
> > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > device can operate at with a given voltage.
> > 
> > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > as a valid frequency indicating the frequency isn't required anymore and
> > to make the same API use the clk framework to round the frequency passed
> > in instead of relying on the OPP table to specify each frequency that
> > can be used. Once we have these two patches in place, we can use the OPP
> > API to change clk rates instead of clk_set_rate() and use all the recent
> > OPP enhancements that have been made around required-opps and genpd
> > performance states to do DVFS for all devices.
> 
> Generally speaking I am fine with such an approach but I am not sure
> about what others would say on this as they had objections to using
> OPP core for setting the rate itself.
> 
> FWIW, I suggested exactly this solution sometime back [1]
> 
> - Drivers need to use two API sets to change clock rate (OPP helpers)
>   and enable/disable them (CLK framework helpers) and this leads us to
>   exactly that combination. Is that acceptable ? It doesn't look great
>   to me as well..

Agreed. I don't think anyone thinks this looks great, but I'll argue
that it's improving OPP for devices that already use it so that we can
remove voltage requirements when their clk is off. Think about CPUs that
are in their own clk domain where we want to remove the voltage
requirement when those CPUs are offline, or a GPU that wants to remove
its voltage requirement when it turns off clks. The combination is
already out there, just OPP hasn't solved this problem.

The only other plan I had was to implement another API like
dev_pm_set_state() or something like that and have that do something
like what the OPP rate API does right now. The main difference being
that the argument to the function would be some opaque u64 that's
converted by the bus/class/genpd attached to the device into whatever
frequency/voltage/performance state is desired (and sequenced in the
right order too). And then I was thinking that runtime PM or explicit
dev_pm_set_state() calls would be used to tell this new layer that the
device was going to a lower power mode with some other number (sub-kHz
integer?) and have that be translated again into some
frequency/voltage/performance state.

Either way, each driver will have to change from using the clk APIs to
changes rates to something else like one of these APIs, so I don't see a
huge difference. Drivers will have to change.

> 
> - Do we expect the callers will disable clk before calling
>   opp-set-rate with 0 ? We should remove the regulator requirements as
>   well along with perf-state.

Yes, that's the plan. Problems come along with this though, like shared
resource constraints and actually knowing the clk on/off state,
frequency, voltage, etc. at boot time and making sure to keep those
constraints satisfied during normal operation.

> 
> - What about enabling/disabling clock as well from OPP framework. We
>   can enable it on the very first call to opp-set-rate and disable
>   when freq is 0. That will simplify the drivers as well.

It works when those drivers aren't calling clk_disable() directly from
some irq handler. I don't think that's very common, but in those cases
we would probably want to allow drivers to quickly gate and ungate their
clks but then defer the sleeping stuff (voltages and off chip clks) to
the schedulable contexts. We'll still be left with a small number of
drivers that want to only call clk_prepare() and clk_unprepare() from
within OPP and keep calling clk_enable() and clk_disable() from their
driver. So introduce different APIs for those drivers to indicate this
to OPP? And only do that when it becomes a requirement?

Otherwise I don't really see a problem with the OPP call handling the
enable state of the clk as well.

> 
> > One nice feature of this approach is that we don't need to change the
> > OPP binding to support this. We can specify only the max frequencies and
> > the voltage requirements in DT with the existing binding and slightly
> > tweak the OPP code to achieve these results. 
> > 
> > This series includes a conversion of the uart and spi drivers on
> > qcom sdm845 where these patches are being developed. It shows how a
> > driver is converted from the clk APIs to the OPP APIs and how
> > enable/disable state of the clk is communicated to the OPP layer.
> > 
> > Some open topics and initial points for discussion are:
> > 
> > 1) The dev_pm_opp_set_rate() API changes may break something that's 
> > relying on the rate rounding that OPP provides. If those exist,
> > we may need to implement another API that is more explicit about using
> > the clk API instead of the OPP tables.
> > 
> > 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> > dropping the rate requirement. Is there anything better than this?
> > 
> > 3) How do we handle devices that already have power-domains specified in
> > DT? The opp binding for required-opps doesn't let us specify the power
> > domain to target, instead it assumes that whatever power domain is
> > attached to a device is the one that OPP needs to use to change the
> > genpd performance state. Do we need a
> > dev_pm_opp_set_required_opps_name() or something to be explicit about
> > this? Can we have some way for the power domain that required-opps
> > correspond to be expressed in the OPP tables themselves?
> > 
> > 4) How do we achieve the "full constraint" state? i.e. what do we do
> > about some devices being active and others being inactive during boot
> > and making sure that the voltage for the shared power domains doesn't
> > drop until all devices requiring it have informed OPP about their
> > power requirements?
> > 

Any comments on these topics?

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Ulf Hansson]

On Thu, 7 Feb 2019 at 08:58, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Viresh Kumar (2019-01-31 01:23:49)
> > Adding few folks to the thread who might be interested in this stuff.
> >
> > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > This patch series is an RFC around how we can implement DVFS for devices
> > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > strict set of frequencies that they have been tested at to derive some
> > > operating performance point. Instead they have a coarser set of
> > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > device can operate at with a given voltage.
> > >
> > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > as a valid frequency indicating the frequency isn't required anymore and
> > > to make the same API use the clk framework to round the frequency passed
> > > in instead of relying on the OPP table to specify each frequency that
> > > can be used. Once we have these two patches in place, we can use the OPP
> > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > OPP enhancements that have been made around required-opps and genpd
> > > performance states to do DVFS for all devices.
> >
> > Generally speaking I am fine with such an approach but I am not sure
> > about what others would say on this as they had objections to using
> > OPP core for setting the rate itself.
> >
> > FWIW, I suggested exactly this solution sometime back [1]
> >
> > - Drivers need to use two API sets to change clock rate (OPP helpers)
> >   and enable/disable them (CLK framework helpers) and this leads us to
> >   exactly that combination. Is that acceptable ? It doesn't look great
> >   to me as well..
>
> Agreed. I don't think anyone thinks this looks great, but I'll argue
> that it's improving OPP for devices that already use it so that we can
> remove voltage requirements when their clk is off. Think about CPUs that
> are in their own clk domain where we want to remove the voltage
> requirement when those CPUs are offline, or a GPU that wants to remove
> its voltage requirement when it turns off clks. The combination is
> already out there, just OPP hasn't solved this problem.
>
> The only other plan I had was to implement another API like
> dev_pm_set_state() or something like that and have that do something
> like what the OPP rate API does right now. The main difference being
> that the argument to the function would be some opaque u64 that's
> converted by the bus/class/genpd attached to the device into whatever
> frequency/voltage/performance state is desired (and sequenced in the
> right order too). And then I was thinking that runtime PM or explicit
> dev_pm_set_state() calls would be used to tell this new layer that the
> device was going to a lower power mode with some other number (sub-kHz
> integer?) and have that be translated again into some
> frequency/voltage/performance state.
>
> Either way, each driver will have to change from using the clk APIs to
> changes rates to something else like one of these APIs, so I don't see a
> huge difference. Drivers will have to change.
>
> >
> > - Do we expect the callers will disable clk before calling
> >   opp-set-rate with 0 ? We should remove the regulator requirements as
> >   well along with perf-state.
>
> Yes, that's the plan. Problems come along with this though, like shared
> resource constraints and actually knowing the clk on/off state,
> frequency, voltage, etc. at boot time and making sure to keep those
> constraints satisfied during normal operation.
>
> >
> > - What about enabling/disabling clock as well from OPP framework. We
> >   can enable it on the very first call to opp-set-rate and disable
> >   when freq is 0. That will simplify the drivers as well.
>
> It works when those drivers aren't calling clk_disable() directly from
> some irq handler. I don't think that's very common, but in those cases
> we would probably want to allow drivers to quickly gate and ungate their
> clks but then defer the sleeping stuff (voltages and off chip clks) to
> the schedulable contexts. We'll still be left with a small number of
> drivers that want to only call clk_prepare() and clk_unprepare() from
> within OPP and keep calling clk_enable() and clk_disable() from their
> driver. So introduce different APIs for those drivers to indicate this
> to OPP? And only do that when it becomes a requirement?
>
> Otherwise I don't really see a problem with the OPP call handling the
> enable state of the clk as well.

I think we also need to consider cross SoC drivers. One SoC may have
both clocks and OPPs to manage, while another may have only clocks.

Even it this may be fairly uncommon, we should consider it, before we
decide to fold in additional clock management, like
clk_prepare|unprepare() for example, behind the dev_pm_opp_set_rate()
API.

The point is, the driver may need to call clk_prepare|enable()
anyways, unless we make that conditional depending on a DT compatible
string, for example. Of course, because the clock prepare/enable is
reference counted, there may not be a problem in practice to have both
the OPP and driver to deal with it.

>
> >
> > > One nice feature of this approach is that we don't need to change the
> > > OPP binding to support this. We can specify only the max frequencies and
> > > the voltage requirements in DT with the existing binding and slightly
> > > tweak the OPP code to achieve these results.
> > >
> > > This series includes a conversion of the uart and spi drivers on
> > > qcom sdm845 where these patches are being developed. It shows how a
> > > driver is converted from the clk APIs to the OPP APIs and how
> > > enable/disable state of the clk is communicated to the OPP layer.
> > >
> > > Some open topics and initial points for discussion are:
> > >
> > > 1) The dev_pm_opp_set_rate() API changes may break something that's
> > > relying on the rate rounding that OPP provides. If those exist,
> > > we may need to implement another API that is more explicit about using
> > > the clk API instead of the OPP tables.
> > >
> > > 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> > > dropping the rate requirement. Is there anything better than this?

This seems like a reasonable approach to me.

At least it seams silly to invent a separate API and I can't think of
anything else that makes sense.

> > >
> > > 3) How do we handle devices that already have power-domains specified in
> > > DT? The opp binding for required-opps doesn't let us specify the power
> > > domain to target, instead it assumes that whatever power domain is
> > > attached to a device is the one that OPP needs to use to change the
> > > genpd performance state. Do we need a
> > > dev_pm_opp_set_required_opps_name() or something to be explicit about
> > > this? Can we have some way for the power domain that required-opps
> > > correspond to be expressed in the OPP tables themselves?

Is the about the multiple PM domains per device? I thought we have
already covered this case, or at least part of it [1].

Doesn't dev_pm_opp_set_genpd_virt_dev() and friends, help you with this, no?

> > >
> > > 4) How do we achieve the "full constraint" state? i.e. what do we do
> > > about some devices being active and others being inactive during boot
> > > and making sure that the voltage for the shared power domains doesn't
> > > drop until all devices requiring it have informed OPP about their
> > > power requirements?

Good question. What kind of problems do you see.

Will drivers fail to probe? Will the HW operate outside valid conditions?

> > >
>
> Any comments on these topics?
>

Kind regards
Uffe

[1]
https://lkml.org/lkml/2018/10/25/204

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Stephen Boyd]

Quoting Rajendra Nayak (2019-02-06 22:57:12)
> 
> > 3) How do we handle devices that already have power-domains specified in
> > DT? The opp binding for required-opps doesn't let us specify the power
> > domain to target, instead it assumes that whatever power domain is
> > attached to a device is the one that OPP needs to use to change the
> > genpd performance state. Do we need a
> > dev_pm_opp_set_required_opps_name() or something to be explicit about
> > this? Can we have some way for the power domain that required-opps
> > correspond to be expressed in the OPP tables themselves?
> 
> I was converting a few more drivers to use the proposed approach in this
> RFC, in order to identify all outstanding issues we need to deal with,
> and specifically for UFS, I end up with this exact scenario where UFS already
> has an existing power domain (gdsc) and I need to add another one (rpmhpd) for
> setting the performance state.
> 
> If I use dev_pm_opp_of_add_table() to add the opp table from DT, the opp
> layer assumes its the same device on which it can do a dev_pm_genpd_set_performance_state()
> with, however the device that's actually associated with the pm_domain when we
> have multiple power domains is infact the one (dummy) that we create when
> the driver makes a call to dev_pm_domain_attach_by_name/id().
> 
> Any thoughts on whats a good way to handle this?
> 

Ulf mentioned that we can use dev_pm_opp_set_genpd_virt_dev() for this.
Does that API work here?

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Rajendra Nayak]

On 2/8/2019 1:17 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2019-02-06 22:57:12)
>>
>>> 3) How do we handle devices that already have power-domains specified in
>>> DT? The opp binding for required-opps doesn't let us specify the power
>>> domain to target, instead it assumes that whatever power domain is
>>> attached to a device is the one that OPP needs to use to change the
>>> genpd performance state. Do we need a
>>> dev_pm_opp_set_required_opps_name() or something to be explicit about
>>> this? Can we have some way for the power domain that required-opps
>>> correspond to be expressed in the OPP tables themselves?
>>
>> I was converting a few more drivers to use the proposed approach in this
>> RFC, in order to identify all outstanding issues we need to deal with,
>> and specifically for UFS, I end up with this exact scenario where UFS already
>> has an existing power domain (gdsc) and I need to add another one (rpmhpd) for
>> setting the performance state.
>>
>> If I use dev_pm_opp_of_add_table() to add the opp table from DT, the opp
>> layer assumes its the same device on which it can do a dev_pm_genpd_set_performance_state()
>> with, however the device that's actually associated with the pm_domain when we
>> have multiple power domains is infact the one (dummy) that we create when
>> the driver makes a call to dev_pm_domain_attach_by_name/id().
>>
>> Any thoughts on whats a good way to handle this?
>>
> 
> Ulf mentioned that we can use dev_pm_opp_set_genpd_virt_dev() for this.
> Does that API work here?

Ah, yes, that should work, I hadn't noticed this API existed.

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

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 06-02-19, 23:58, Stephen Boyd wrote:
> Quoting Viresh Kumar (2019-01-31 01:23:49)
> > FWIW, I suggested exactly this solution sometime back [1]
> > 
> > - Drivers need to use two API sets to change clock rate (OPP helpers)
> >   and enable/disable them (CLK framework helpers) and this leads us to
> >   exactly that combination. Is that acceptable ? It doesn't look great
> >   to me as well..
> 
> Agreed. I don't think anyone thinks this looks great, but I'll argue
> that it's improving OPP for devices that already use it so that we can
> remove voltage requirements when their clk is off. Think about CPUs that
> are in their own clk domain where we want to remove the voltage
> requirement when those CPUs are offline, or a GPU that wants to remove
> its voltage requirement when it turns off clks. The combination is
> already out there, just OPP hasn't solved this problem.
> 
> The only other plan I had was to implement another API like
> dev_pm_set_state() or something like that and have that do something
> like what the OPP rate API does right now. The main difference being
> that the argument to the function would be some opaque u64 that's
> converted by the bus/class/genpd attached to the device into whatever
> frequency/voltage/performance state is desired (and sequenced in the
> right order too). And then I was thinking that runtime PM or explicit
> dev_pm_set_state() calls would be used to tell this new layer that the
> device was going to a lower power mode with some other number (sub-kHz
> integer?) and have that be translated again into some
> frequency/voltage/performance state.
> 
> Either way, each driver will have to change from using the clk APIs to
> changes rates to something else like one of these APIs, so I don't see a
> huge difference. Drivers will have to change.

I agree, that's why I wrote the dev_pm_opp_set_rate() API initially.

> > 
> > - Do we expect the callers will disable clk before calling
> >   opp-set-rate with 0 ? We should remove the regulator requirements as
> >   well along with perf-state.
> 
> Yes, that's the plan. Problems come along with this though, like shared
> resource constraints and actually knowing the clk on/off state,
> frequency, voltage, etc. at boot time and making sure to keep those
> constraints satisfied during normal operation.

But that isn't any different from drivers doing clk_disable directly,
right ? So that shouldn't worry us.

> > - What about enabling/disabling clock as well from OPP framework. We
> >   can enable it on the very first call to opp-set-rate and disable
> >   when freq is 0. That will simplify the drivers as well.
> 
> It works when those drivers aren't calling clk_disable() directly from
> some irq handler. I don't think that's very common, but in those cases
> we would probably want to allow drivers to quickly gate and ungate their
> clks but then defer the sleeping stuff (voltages and off chip clks) to
> the schedulable contexts. We'll still be left with a small number of
> drivers that want to only call clk_prepare() and clk_unprepare() from
> within OPP and keep calling clk_enable() and clk_disable() from their
> driver. So introduce different APIs for those drivers to indicate this
> to OPP? And only do that when it becomes a requirement?

I am not sure I understood this well. The reference counting within
clk/regulator should let both the layers (driver and opp core) work
just fine. Why would a driver don't want OPP core to call
clk_prepare_enable() all the time ?

> Otherwise I don't really see a problem with the OPP call handling the
> enable state of the clk as well.

Right, so I would like that to be part of this series when this gets
implemented.

> > > One nice feature of this approach is that we don't need to change the
> > > OPP binding to support this. We can specify only the max frequencies and
> > > the voltage requirements in DT with the existing binding and slightly
> > > tweak the OPP code to achieve these results. 
> > > 
> > > This series includes a conversion of the uart and spi drivers on
> > > qcom sdm845 where these patches are being developed. It shows how a
> > > driver is converted from the clk APIs to the OPP APIs and how
> > > enable/disable state of the clk is communicated to the OPP layer.
> > > 
> > > Some open topics and initial points for discussion are:
> > > 
> > > 1) The dev_pm_opp_set_rate() API changes may break something that's 
> > > relying on the rate rounding that OPP provides. If those exist,
> > > we may need to implement another API that is more explicit about using
> > > the clk API instead of the OPP tables.

I don't remember any such cases, I may have forgotten about those
though.

> > > 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> > > dropping the rate requirement. Is there anything better than this?

I am okay with it. I don't want to invent another set of APIs to
enable / disable the resources.

> > > 3) How do we handle devices that already have power-domains specified in
> > > DT? The opp binding for required-opps doesn't let us specify the power
> > > domain to target, instead it assumes that whatever power domain is
> > > attached to a device is the one that OPP needs to use to change the
> > > genpd performance state. Do we need a
> > > dev_pm_opp_set_required_opps_name() or something to be explicit about

Yeah, we may need to come up with something like this eventually. I
had written something like that earlier, but then it wasn't required.

> > > this? Can we have some way for the power domain that required-opps
> > > correspond to be expressed in the OPP tables themselves?

Not sure I understood it. Can you explain with some example please ?

> > > 4) How do we achieve the "full constraint" state? i.e. what do we do
> > > about some devices being active and others being inactive during boot
> > > and making sure that the voltage for the shared power domains doesn't
> > > drop until all devices requiring it have informed OPP about their
> > > power requirements?

We need the boot-constraint framework for that. I think this is a
problem which we have currently as well. I am waiting for the
bus-scaling framework to get in, after that we will have lot of cases
where boot-constraints would be required and it won't be limited to
just clcd then.

-- 
viresh

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 07-02-19, 14:37, Ulf Hansson wrote:
> I think we also need to consider cross SoC drivers. One SoC may have
> both clocks and OPPs to manage, while another may have only clocks.

We already have that case with CPUs as well and dev_pm_opp_set_rate()
takes care of it.

> Even it this may be fairly uncommon, we should consider it, before we
> decide to fold in additional clock management, like
> clk_prepare|unprepare() for example, behind the dev_pm_opp_set_rate()
> API.
> 
> The point is, the driver may need to call clk_prepare|enable()
> anyways, unless we make that conditional depending on a DT compatible
> string, for example. Of course, because the clock prepare/enable is
> reference counted, there may not be a problem in practice to have both
> the OPP and driver to deal with it.

-- 
viresh

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Ulf Hansson]

On Fri, 8 Feb 2019 at 08:17, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 07-02-19, 14:37, Ulf Hansson wrote:
> > I think we also need to consider cross SoC drivers. One SoC may have
> > both clocks and OPPs to manage, while another may have only clocks.
>
> We already have that case with CPUs as well and dev_pm_opp_set_rate()
> takes care of it.

I think you may have misunderstood my point. Or maybe I don't get yours. :-)

What if there is no OPP at all to use, then dev_pm_opp_set_rate() is
just a noop, right? In this scenario the driver still need to call
clk_set_rate().

How do we cope with these cases?

>
> > Even it this may be fairly uncommon, we should consider it, before we
> > decide to fold in additional clock management, like
> > clk_prepare|unprepare() for example, behind the dev_pm_opp_set_rate()
> > API.
> >
> > The point is, the driver may need to call clk_prepare|enable()
> > anyways, unless we make that conditional depending on a DT compatible
> > string, for example. Of course, because the clock prepare/enable is
> > reference counted, there may not be a problem in practice to have both
> > the OPP and driver to deal with it.
>

Kind regards
Uffe

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 08-02-19, 10:45, Ulf Hansson wrote:
> On Fri, 8 Feb 2019 at 08:17, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 07-02-19, 14:37, Ulf Hansson wrote:
> > > I think we also need to consider cross SoC drivers. One SoC may have
> > > both clocks and OPPs to manage, while another may have only clocks.
> >
> > We already have that case with CPUs as well and dev_pm_opp_set_rate()
> > takes care of it.
> 
> I think you may have misunderstood my point. Or maybe I don't get yours. :-)

It was me. I thought you are talking about regulators and that is what
is already managed, i.e. to work with or without regulators.

> What if there is no OPP at all to use, then dev_pm_opp_set_rate() is
> just a noop, right? In this scenario the driver still need to call
> clk_set_rate().
> 
> How do we cope with these cases?

Yeah, that would be a problem and hacking the OPP core may not be the
right solution :(

-- 
viresh

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Ulf Hansson]

On Fri, 8 Feb 2019 at 11:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-02-19, 10:45, Ulf Hansson wrote:
> > On Fri, 8 Feb 2019 at 08:17, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 07-02-19, 14:37, Ulf Hansson wrote:
> > > > I think we also need to consider cross SoC drivers. One SoC may have
> > > > both clocks and OPPs to manage, while another may have only clocks.
> > >
> > > We already have that case with CPUs as well and dev_pm_opp_set_rate()
> > > takes care of it.
> >
> > I think you may have misunderstood my point. Or maybe I don't get yours. :-)
>
> It was me. I thought you are talking about regulators and that is what
> is already managed, i.e. to work with or without regulators.
>
> > What if there is no OPP at all to use, then dev_pm_opp_set_rate() is
> > just a noop, right? In this scenario the driver still need to call
> > clk_set_rate().
> >
> > How do we cope with these cases?
>
> Yeah, that would be a problem and hacking the OPP core may not be the
> right solution :(

I guess one simple way forward could just be to check if there is an
OPP handle/table available, then use dev_pm_opp_set_rate(). When no
OPP handle/table, use clk_set_rate() *instead*, not both.

That could work, don't you think?

Kind regards
Uffe

Re: [RFC/PATCH 0/5] DVFS in the OPP core

[Viresh Kumar]

On 08-02-19, 11:31, Ulf Hansson wrote:
> On Fri, 8 Feb 2019 at 11:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 08-02-19, 10:45, Ulf Hansson wrote:
> > > On Fri, 8 Feb 2019 at 08:17, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > On 07-02-19, 14:37, Ulf Hansson wrote:
> > > > > I think we also need to consider cross SoC drivers. One SoC may have
> > > > > both clocks and OPPs to manage, while another may have only clocks.
> > > >
> > > > We already have that case with CPUs as well and dev_pm_opp_set_rate()
> > > > takes care of it.
> > >
> > > I think you may have misunderstood my point. Or maybe I don't get yours. :-)
> >
> > It was me. I thought you are talking about regulators and that is what
> > is already managed, i.e. to work with or without regulators.
> >
> > > What if there is no OPP at all to use, then dev_pm_opp_set_rate() is
> > > just a noop, right? In this scenario the driver still need to call
> > > clk_set_rate().
> > >
> > > How do we cope with these cases?
> >
> > Yeah, that would be a problem and hacking the OPP core may not be the
> > right solution :(
> 
> I guess one simple way forward could just be to check if there is an
> OPP handle/table available, then use dev_pm_opp_set_rate(). When no
> OPP handle/table, use clk_set_rate() *instead*, not both.
> 
> That could work, don't you think?

Yeah, just that it adds more conditional code in drivers, while we
wanted to make them light-weight :)

-- 
viresh