Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

Power domain fits much better than a voltage regulator in regards to a proper hardware description and from a software perspective as well. Hence replace the core regulator with the power domain. Note that this doesn't affect any existing DTBs because we haven't started to use the regulator yet, and thus, it's okay to change it.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra20-emc.txt        | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Power domain fits much better than a voltage regulator in regards to a proper hardware description and from a software perspective as well. Hence replace the core regulator with the power domain. Note that this doesn't affect any existing DTBs because we haven't started to use the regulator yet, and thus, it's okay to change it.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra30-emc.yaml     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml +0 -0

index 0a2e2c0d0fdd..7b4af9169b0b 100644

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml +1 -1

   operating-points-v2:
     description:

@@ -241,7 +241,7 @@ examples:
 
         nvidia,memory-controller = <&mc>;
         operating-points-v2 = <&dvfs_opp_table>;
-        core-supply = <&vdd_core>;
+        power-domains = <&domain>;
 
         #interconnect-cells = <0>;
 

Signature · 1 line

2.29.2

Quoted 19 lines

Power domain fits much better than a voltage regulator in regards to a proper hardware description and from a software perspective as well. Hence replace the core regulator with the power domain. Note that this doesn't affect any existing DTBs because we haven't started to use the regulator yet, and thus, it's okay to change it.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra30-emc.yaml     | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml +0 -0

index 0a2e2c0d0fdd..7b4af9169b0b 100644
@@ -39,9 +39,9 @@ properties:
     description:
       Phandle of the Memory Controller node.
 

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml properties: · line 44

Quoted 5 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.yaml +2 -2

-  core-supply:
+  power-domains:
     description:
-      Phandle of voltage regulator of the SoC "core" power domain.
+      Phandle to the SoC "core" power domain.

power-domains needs to define how many (maxItems).

Document new DVFS OPP table and power domain properties of the Host1x bus and devices sitting on the bus.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../display/tegra/nvidia,tegra20-host1x.txt   | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

Document new DVFS OPP table and power domain properties of the video decoder engine.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../devicetree/bindings/media/nvidia,tegra-vde.txt   | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Document "clocks" sub-node which describes Tegra SoC clocks that require a higher voltage of the core power domain in order to operate properly on a higher rates.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../bindings/clock/nvidia,tegra20-car.txt     | 26 +++++++++++++++++++
 .../bindings/clock/nvidia,tegra30-car.txt     | 26 +++++++++++++++++++
 2 files changed, 52 insertions(+)

Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt +26 -0

index 6c5901b503d0..353354477785 100644
@@ -19,6 +19,16 @@ Required properties :
   In clock consumers, this cell represents the bit number in the CAR's
   array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
 
+Optional child sub-node "clocks" should contain nodes matching the clocks
+on the Tegra SoC. Refer to Tegra TRM for mode details on the clock nodes.
+
+Required properties :
+- compatible : Should be "nvidia,tegra20-clock".
+- operating-points-v2: See ../bindings/opp/opp.txt for details.
+- clocks : Should contain clock which corresponds to the node.
+- power-domains: Phandle to a power domain node as described by generic
+                 PM domain bindings.
+
 Example SoC include file:
 
 / {

@@ -27,6 +37,22 @@ Example SoC include file:
 		reg = <0x60006000 0x1000>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
+
+		clocks {
+			hdmi {
+				compatible = "nvidia,tegra20-clock";
+				operating-points-v2 = <&hdmi_opp_table>;
+				clocks = <&tegra_car TEGRA20_CLK_HDMI>;
+				power-domains = <&domain>;
+			};
+
+			pll_m {
+				compatible = "nvidia,tegra20-clock";
+				operating-points-v2 = <&pll_m_opp_table>;
+				clocks = <&tegra_car TEGRA20_CLK_PLL_M>;
+				power-domains = <&domain>;
+			};
+		};
 	};
 
 	usb@c5004000 {

Documentation/devicetree/bindings/clock/nvidia,tegra30-car.txt +26 -0

index 63618cde12df..bc7bbdaa9d3f 100644
@@ -19,6 +19,16 @@ Required properties :
   In clock consumers, this cell represents the bit number in the CAR's
   array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
 
+Optional child sub-node "clocks" should contain nodes matching the clocks
+on the Tegra SoC. Refer to Tegra TRM for mode details on the clock nodes.
+
+Required properties :
+- compatible : Should be "nvidia,tegra30-clock".
+- operating-points-v2: See ../bindings/opp/opp.txt for details.
+- clocks : Should contain clock which corresponds to the node.
+- power-domains: Phandle to a power domain node as described by generic
+                 PM domain bindings.
+
 Example SoC include file:
 
 / {

@@ -31,6 +41,22 @@ Example SoC include file:
 
 	usb@c5004000 {
 		clocks = <&tegra_car TEGRA30_CLK_USB2>;
+
+		clocks {
+			hdmi {
+				compatible = "nvidia,tegra30-clock";
+				operating-points-v2 = <&hdmi_opp_table>;
+				clocks = <&tegra_car TEGRA30_CLK_HDMI>;
+				power-domains = <&domain>;
+			};
+
+			pll_m {
+				compatible = "nvidia,tegra30-clock";
+				operating-points-v2 = <&pll_m_opp_table>;
+				clocks = <&tegra_car TEGRA30_CLK_PLL_M>;
+				power-domains = <&domain>;
+			};
+		};
 	};
 };
 

Signature · 1 line

2.29.2

Quoted 3 lines

Document "clocks" sub-node which describes Tegra SoC clocks that require a higher voltage of the core power domain in order to operate properly on a higher rates.

Seems like an odd reason to have a bunch of child nodes. It very much seems like a problem you'd need to solve after you design the binding which should be fixed within the kernel.

This is also above my threshold for 'convert to schema' first...

All NVIDIA Tegra SoCs have a core power domain where majority of hardware blocks reside. Add binding for the core power domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../arm/tegra/nvidia,tegra20-core-domain.yaml | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +8 -0

new file mode 100644
index 000000000000..f3d8fd2d8371
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/tegra/nvidia,tegra20-core-domain.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVIDIA Tegra Core Power Domain
+

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +15 -0

+
+  operating-points-v2:
+    description:
+      Should contain level, voltages and opp-supported-hw property.
+      The supported-hw is a bitfield indicating SoC speedo or process
+      ID mask.
+
+  "#power-domain-cells":
+    const: 0
+
+  power-supply:
+    description:
+      Phandle to voltage regulator connected to the SoC Core power rail.
+
+required:

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +5 -0

+        compatible = "nvidia,tegra20-core-domain";
+        operating-points-v2 = <&opp_table>;
+        power-supply = <&regulator>;
+        #power-domain-cells = <0>;
+    };

Signature · 1 line

2.29.2

Quoted 23 lines

All NVIDIA Tegra SoCs have a core power domain where majority of hardware blocks reside. Add binding for the core power domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 .../arm/tegra/nvidia,tegra20-core-domain.yaml | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +8 -0

new file mode 100644
index 000000000000..f3d8fd2d8371
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/tegra/nvidia,tegra20-core-domain.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVIDIA Tegra Core Power Domain
+

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml line 18

Quoted 10 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +10 -0

+maintainers:
+  - Dmitry Osipenko <[email protected]>
+  - Jon Hunter <[email protected]>
+  - Thierry Reding <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - nvidia,tegra20-core-domain
+      - nvidia,tegra30-core-domain

The file should be in bindings/power.
Include also the power-domain.yaml schema.

Quoted 19 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +19 -0

+
+  operating-points-v2:
+    description:
+      Should contain level, voltages and opp-supported-hw property.
+      The supported-hw is a bitfield indicating SoC speedo or process
+      ID mask.
+
+  "#power-domain-cells":
+    const: 0
+
+  power-supply:
+    description:
+      Phandle to voltage regulator connected to the SoC Core power rail.
+
+required:
+  - compatible
+  - operating-points-v2
+  - "#power-domain-cells"
+  - power-supply

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml line 43

Quoted 6 lines

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-core-domain.yaml new file +6 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    core-domain {

power-domain (to follow schema and devicetree spec)

Extend OPP API with dev_pm_opp_sync_regulators() function, which syncs voltage state of regulators.

Signed-off-by Dmitry Osipenko <[email protected]>

Add dev_pm_opp_set_voltage() which allows OPP table users to set voltage in accordance to a given OPP. In particular this is needed for driving voltage of a generic power domain which uses OPPs and doesn't have a clock.

Signed-off-by Dmitry Osipenko <[email protected]>

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Signed-off-by Dmitry Osipenko <[email protected]>

Quoted 5 lines

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Signed-off-by Dmitry Osipenko <[email protected]>

Why doesn't the exact version work for you here ?

22.12.2020 09:42, Viresh Kumar пишет:

Quoted 5 lines · Dmitry Osipenko

Quoted 4 lines

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Quoted 4 lines

Quoted 1 line

Signed-off-by Dmitry Osipenko <[email protected]>

Why doesn't the exact version work for you here ?

The exact version won't find OPP for level=0 if levels don't start with 0, where 0 means that minimal level is desired.

Quoted 8 lines

22.12.2020 09:42, Viresh Kumar пишет:

Quoted 7 lines · Dmitry Osipenko

Quoted 5 lines

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Signed-off-by Dmitry Osipenko <[email protected]>

Quoted 5 lines

Quoted 2 lines

Why doesn't the exact version work for you here ?

The exact version won't find OPP for level=0 if levels don't start with 0, where 0 means that minimal level is desired.

Right, but why do you need to send 0 for your platform ?

23.12.2020 07:19, Viresh Kumar пишет:

Quoted 12 lines · Dmitry Osipenko

Quoted 11 lines

22.12.2020 09:42, Viresh Kumar пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 5 lines

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Signed-off-by Dmitry Osipenko <[email protected]>

Why doesn't the exact version work for you here ?

Quoted 5 lines

Quoted 2 lines

The exact version won't find OPP for level=0 if levels don't start with 0, where 0 means that minimal level is desired.

Right, but why do you need to send 0 for your platform ?

To put power domain into the lowest performance state when device is idling.

Quoted 16 lines

23.12.2020 07:19, Viresh Kumar пишет:

Quoted 15 lines · Dmitry Osipenko

Quoted 13 lines

22.12.2020 09:42, Viresh Kumar пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 5 lines

Add a ceil version of the dev_pm_opp_find_level(). It's handy to have if levels don't start from 0 in OPP table and zero usually means a minimal level.

Signed-off-by Dmitry Osipenko <[email protected]>

Why doesn't the exact version work for you here ?

The exact version won't find OPP for level=0 if levels don't start with 0, where 0 means that minimal level is desired.

Quoted 4 lines

Quoted 2 lines

Right, but why do you need to send 0 for your platform ?

To put power domain into the lowest performance state when device is idling.

I see. So you really want to set it to the lowest state or just take the vote out ? Which may end up powering off the domain in the worst case ?

In a device driver I want to set PD to the lowest performance state by removing the performance vote when dev_pm_opp_set_rate(dev, 0) is invoked by the driver.

The OPP core already does this, but if OPP levels don't start from 0 in a device-tree for PD, then it currently doesn't work since there is a need to get a rounded-up performance state because dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and 28).

The PD powering off and performance-changes are separate from each other in the GENPD core. The GENPD core automatically turns off domain when all devices within the domain are suspended by system-suspend or RPM.

The performance state of a power domain is controlled solely by a device driver. GENPD core only aggregates the performance requests, it doesn't change the performance state of a domain by itself when device is suspended or resumed, IIUC this is intentional. And I want to put domain into lowest performance state when device is suspended.

Quoted 14 lines

In a device driver I want to set PD to the lowest performance state by removing the performance vote when dev_pm_opp_set_rate(dev, 0) is invoked by the driver.

The OPP core already does this, but if OPP levels don't start from 0 in a device-tree for PD, then it currently doesn't work since there is a need to get a rounded-up performance state because dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and 28).

The PD powering off and performance-changes are separate from each other in the GENPD core. The GENPD core automatically turns off domain when all devices within the domain are suspended by system-suspend or RPM.

Quoted 5 lines

The performance state of a power domain is controlled solely by a device driver. GENPD core only aggregates the performance requests, it doesn't change the performance state of a domain by itself when device is suspended or resumed, IIUC this is intentional. And I want to put domain into lowest performance state when device is suspended.

Right, so if you really want to just drop the performance vote, then with a value of 0 for the performance state the call will reach to your genpd's callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts the frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp argument as NULL and in that case set voltage to 0 and do regulator_disable() as well. Won't that work better than going for the lowest voltage ?

28.12.2020 09:22, Viresh Kumar пишет:

Quoted 21 lines · Dmitry Osipenko

Quoted 14 lines

In a device driver I want to set PD to the lowest performance state by removing the performance vote when dev_pm_opp_set_rate(dev, 0) is invoked by the driver.

The OPP core already does this, but if OPP levels don't start from 0 in a device-tree for PD, then it currently doesn't work since there is a need to get a rounded-up performance state because dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and 28).

The PD powering off and performance-changes are separate from each other in the GENPD core. The GENPD core automatically turns off domain when all devices within the domain are suspended by system-suspend or RPM.

Quoted 5 lines

The performance state of a power domain is controlled solely by a device driver. GENPD core only aggregates the performance requests, it doesn't change the performance state of a domain by itself when device is suspended or resumed, IIUC this is intentional. And I want to put domain into lowest performance state when device is suspended.

Quoted 7 lines

Right, so if you really want to just drop the performance vote, then with a value of 0 for the performance state the call will reach to your genpd's callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts the frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp argument as NULL and in that case set voltage to 0 and do regulator_disable() as well. Won't that work better than going for the lowest voltage ?

We can make dev_pm_opp_set_voltage() to accept OPP=NULL in order to disable the regulator, like it's done for dev_pm_opp_set_rate(dev, 0). Although, I don't need this kind of behaviour for the Tegra PD driver, and thus, would prefer to leave this for somebody else to implement in the future, once it will be really needed.

Still we need the dev_pm_opp_find_level_ceil() because level=0 means that we want to set PD to the lowest (minimal) performance state, i.e. it doesn't necessarily mean that we want to set the voltage to 0 and disable the PD entirely. GENPD has a separate controls for on/off.

Quoted 36 lines

28.12.2020 09:22, Viresh Kumar пишет:

Quoted 21 lines · Dmitry Osipenko

Quoted 14 lines

In a device driver I want to set PD to the lowest performance state by removing the performance vote when dev_pm_opp_set_rate(dev, 0) is invoked by the driver.

The OPP core already does this, but if OPP levels don't start from 0 in a device-tree for PD, then it currently doesn't work since there is a need to get a rounded-up performance state because dev_pm_opp_set_voltage() takes OPP entry for the argument (patches 9 and 28).

The PD powering off and performance-changes are separate from each other in the GENPD core. The GENPD core automatically turns off domain when all devices within the domain are suspended by system-suspend or RPM.

Quoted 5 lines

The performance state of a power domain is controlled solely by a device driver. GENPD core only aggregates the performance requests, it doesn't change the performance state of a domain by itself when device is suspended or resumed, IIUC this is intentional. And I want to put domain into lowest performance state when device is suspended.

Quoted 7 lines

Right, so if you really want to just drop the performance vote, then with a value of 0 for the performance state the call will reach to your genpd's callback ->set_performance_state(). Just as dev_pm_opp_set_rate() accepts the frequency to be 0, I would expect dev_pm_opp_set_rate() to accept opp argument as NULL and in that case set voltage to 0 and do regulator_disable() as well. Won't that work better than going for the lowest voltage ?

We can make dev_pm_opp_set_voltage() to accept OPP=NULL in order to disable the regulator, like it's done for dev_pm_opp_set_rate(dev, 0). Although, I don't need this kind of behaviour for the Tegra PD driver, and thus, would prefer to leave this for somebody else to implement in the future, once it will be really needed.

Quoted 4 lines

Still we need the dev_pm_opp_find_level_ceil() because level=0 means that we want to set PD to the lowest (minimal) performance state, i.e. it doesn't necessarily mean that we want to set the voltage to 0 and disable the PD entirely. GENPD has a separate controls for on/off.

Ok.

Add resource-managed versions of OPP API functions. This removes a need from drivers to store and manage OPP table pointers.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 173 +++++++++++++++++++++++++++++++++++++++++
 drivers/opp/of.c       |  25 ++++++
 include/linux/pm_opp.h |  51 ++++++++++++
 3 files changed, 249 insertions(+)

Quoted 9 lines

Add resource-managed versions of OPP API functions. This removes a need from drivers to store and manage OPP table pointers.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c     | 173 +++++++++++++++++++++++++++++++++++++++++
 drivers/opp/of.c       |  25 ++++++
 include/linux/pm_opp.h |  51 ++++++++++++
 3 files changed, 249 insertions(+)

Please send a patchset of its own for this patch, along with updates to all the existing code that can make use of these.

A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/opp/core.c +1 -1

index d9feb7639598..3d02fe33630b 100644
@@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table, bool rate_not_available)
 {
 	struct list_head *head;
-	int ret;
+	int i, ret;
 
 	mutex_lock(&opp_table->lock);
 	head = &opp_table->opp_list;

drivers/opp/core.c +2 -0

+	}
+
 	return 0;
 }
 

Signature · 1 line

2.29.2

Quoted 7 lines

A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Please send a separate patchset for fixes, as these can also go to 5.11 itself.

Quoted 17 lines

drivers/opp/core.c +1 -1

index d9feb7639598..3d02fe33630b 100644
@@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table, bool rate_not_available)
 {
 	struct list_head *head;
-	int ret;
+	int i, ret;
 
 	mutex_lock(&opp_table->lock);
 	head = &opp_table->opp_list;

@@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 			 __func__, new_opp->rate);
 	}
 

drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1624

Quoted 7 lines

drivers/opp/core.c +7 -0

+	for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) {
+		if (new_opp->required_opps[i]->available)
+			continue;
+
+		new_opp->available = false;
+		dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n",
+			 __func__, new_opp->required_opps[i]->np, new_opp->rate);

Why not just break from here ?

22.12.2020 11:59, Viresh Kumar пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 7 lines

A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Quoted 1 line

Please send a separate patchset for fixes, as these can also go to 5.11 itself.

Alright, although I don't think that this patch fixes any problems for existing OPP users.

Quoted 17 lines

Quoted 17 lines

drivers/opp/core.c +1 -1

index d9feb7639598..3d02fe33630b 100644
@@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table, bool rate_not_available)
 {
 	struct list_head *head;
-	int ret;
+	int i, ret;
 
 	mutex_lock(&opp_table->lock);
 	head = &opp_table->opp_list;

@@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 			 __func__, new_opp->rate);
 	}
 

Quoted 9 lines

drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1624

Quoted 7 lines

drivers/opp/core.c +7 -0

+	for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) {
+		if (new_opp->required_opps[i]->available)
+			continue;
+
+		new_opp->available = false;
+		dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n",
+			 __func__, new_opp->required_opps[i]->np, new_opp->rate);

Why not just break from here ?

The new_opp could be already marked as unavailable by a previous voltage check, hence this loop should be skipped entirely in that case.

Quoted 10 lines

22.12.2020 11:59, Viresh Kumar пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 7 lines

A required OPP may not be available, and thus, all OPPs which are using this required OPP should be unavailable too.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Quoted 4 lines

Quoted 1 line

Please send a separate patchset for fixes, as these can also go to 5.11 itself.

Alright, although I don't think that this patch fixes any problems for existing OPP users.

Because nobody is using this feature, but otherwise this is a fix for me.

Quoted 27 lines

Quoted 26 lines

Quoted 14 lines

drivers/opp/core.c +1 -1

index d9feb7639598..3d02fe33630b 100644
@@ -1588,7 +1588,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	     struct opp_table *opp_table, bool rate_not_available)
 {
 	struct list_head *head;
-	int ret;
+	int i, ret;
 
 	mutex_lock(&opp_table->lock);
 	head = &opp_table->opp_list;

drivers/opp/core.c int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, · line 1624

Quoted 10 lines

drivers/opp/core.c +7 -0

@@ -1615,6 +1615,15 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 			 __func__, new_opp->rate);
 	}
 
+	for (i = 0; i < opp_table->required_opp_count && new_opp->available; i++) {
+		if (new_opp->required_opps[i]->available)
+			continue;
+
+		new_opp->available = false;
+		dev_warn(dev, "%s: OPP not supported by required OPP %pOF (%lu)\n",
+			 __func__, new_opp->required_opps[i]->np, new_opp->rate);

Why not just break from here ?

Quoted 2 lines

The new_opp could be already marked as unavailable by a previous voltage check, hence this loop should be skipped entirely in that case.

Then add a separate check for that before the loop as we don't need that check on every iteration here.

Support set_opp() customization without requiring to use regulators. This is needed by drivers which want to use dev_pm_opp_set_rate() for changing rates of a multiple clocks and don't need to touch regulator.

One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware units which should be use to the same clock rate, meanwhile voltage scaling is done using a power domain. In this case OPP table doesn't have a regulator, causing a NULL dereference in _set_opp_custom().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/opp/core.c +10 -3

index 3d02fe33630b..625dae7a5ecb 100644
@@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *old_supply,
 			   struct dev_pm_opp_supply *new_supply)
 {
-	struct dev_pm_set_opp_data *data;
+	struct dev_pm_set_opp_data *data, tmp_data;
+	unsigned int regulator_count;
 	int size;
 
-	data = opp_table->set_opp_data;
+	if (opp_table->set_opp_data) {
+		data = opp_table->set_opp_data;
+		regulator_count = opp_table->regulator_count;
+	} else {
+		data = &tmp_data;
+		regulator_count = 0;
+	}
+
 	data->regulators = opp_table->regulators;
-	data->regulator_count = opp_table->regulator_count;

Quoted 42 lines

Support set_opp() customization without requiring to use regulators. This is needed by drivers which want to use dev_pm_opp_set_rate() for changing rates of a multiple clocks and don't need to touch regulator.

One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware units which should be use to the same clock rate, meanwhile voltage scaling is done using a power domain. In this case OPP table doesn't have a regulator, causing a NULL dereference in _set_opp_custom().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/opp/core.c +11 -3

index 3d02fe33630b..625dae7a5ecb 100644
@@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *old_supply,
 			   struct dev_pm_opp_supply *new_supply)
 {
-	struct dev_pm_set_opp_data *data;
+	struct dev_pm_set_opp_data *data, tmp_data;
+	unsigned int regulator_count;
 	int size;
 
-	data = opp_table->set_opp_data;
+	if (opp_table->set_opp_data) {
+		data = opp_table->set_opp_data;
+		regulator_count = opp_table->regulator_count;
+	} else {
+		data = &tmp_data;
+		regulator_count = 0;
+	}
+
 	data->regulators = opp_table->regulators;
-	data->regulator_count = opp_table->regulator_count;
+	data->regulator_count = regulator_count;
 	data->clk = opp_table->clk;
 	data->dev = dev;
 

drivers/opp/core.c static int _set_opp_custom(const struct opp_table *opp_table, · line 852

Quoted 6 lines

drivers/opp/core.c +1 -1

 	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
+	size = sizeof(*old_supply) * regulator_count;
 	if (!old_supply)
 		memset(data->old_opp.supplies, 0, size);
 	else

I don't see you making use of this in this patchset. How did you get this to crash ?

Support set_opp() customization without requiring to use regulators. This is needed by drivers which want to use dev_pm_opp_set_rate() for changing rates of a multiple clocks and don't need to touch regulator.

One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware units which should be use to the same clock rate, meanwhile voltage scaling is done using a power domain. In this case OPP table doesn't have a regulator, causing a NULL dereference in _set_opp_custom().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/opp/core.c +2 -1

index 3d02fe33630b..625dae7a5ecb 100644
@@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *old_supply,
 			   struct dev_pm_opp_supply *new_supply)
 {
-	struct dev_pm_set_opp_data *data;
+	struct dev_pm_set_opp_data *data, tmp_data;
+	unsigned int regulator_count;
 	int size;

drivers/opp/core.c +2 -2

 	data->regulators = opp_table->regulators;
-	data->regulator_count = opp_table->regulator_count;
+	data->regulator_count = regulator_count;
 	data->clk = opp_table->clk;
 	data->dev = dev;
 
 	data->old_opp.rate = old_freq;
-	size = sizeof(*old_supply) * opp_table->regulator_count;
+	size = sizeof(*old_supply) * regulator_count;
 	if (!old_supply)
 		memset(data->old_opp.supplies, 0, size);
 	else

Quoted 27 lines

Support set_opp() customization without requiring to use regulators. This is needed by drivers which want to use dev_pm_opp_set_rate() for changing rates of a multiple clocks and don't need to touch regulator.

One example is NVIDIA Tegra30/114 SoCs which have two sibling 3D hardware units which should be use to the same clock rate, meanwhile voltage scaling is done using a power domain. In this case OPP table doesn't have a regulator, causing a NULL dereference in _set_opp_custom().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/opp/core.c +2 -1

index 3d02fe33630b..625dae7a5ecb 100644
@@ -828,17 +828,25 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 			   struct dev_pm_opp_supply *old_supply,
 			   struct dev_pm_opp_supply *new_supply)
 {
-	struct dev_pm_set_opp_data *data;
+	struct dev_pm_set_opp_data *data, tmp_data;
+	unsigned int regulator_count;
 	int size;
 

drivers/opp/core.c static int _set_opp_custom(const struct opp_table *opp_table, · line 842

Quoted 9 lines

drivers/opp/core.c +8 -1

-	data = opp_table->set_opp_data;
+	if (opp_table->set_opp_data) {
+		data = opp_table->set_opp_data;
+		regulator_count = opp_table->regulator_count;
+	} else {
+		data = &tmp_data;
+		regulator_count = 0;
+	}
+

We should use the same structure, you can add some checks but not replace the structure altogether.

Well, there is no "same structure", the opp_table->set_opp_data is NULL there.

Quoted 2 lines

Well, there is no "same structure", the opp_table->set_opp_data is NULL there.

Right, I saw that yesterday. What I meant was that we need to start allocating the structure for this case now.

Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison is erroneously skipped if OPP rate is missing, thus OPPs are left unsorted.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 23 ++++++++++++-----------
 drivers/opp/opp.h  |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

drivers/opp/core.c +0 -0

index 34f7e530d941..5c7f130a8de2 100644

drivers/opp/core.c +2 -1

 		return opp1->rate < opp2->rate ? -1 : 1;
 	if (opp1->bandwidth && opp2->bandwidth &&
 	    opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)

@@ -1545,7 +1546,8 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
 
 static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 			     struct opp_table *opp_table,
-			     struct list_head **head)
+			     struct list_head **head,
+			     bool rate_not_available)
 {
 	struct dev_pm_opp *opp;
 	int opp_cmp;

@@ -1559,13 +1561,13 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp,
 	 * loop.
 	 */
 	list_for_each_entry(opp, &opp_table->opp_list, node) {

drivers/opp/core.c +0 -0

 		/* Duplicate OPPs */

@@ -1601,12 +1603,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 	mutex_lock(&opp_table->lock);

drivers/opp/core.c +3 -0

+	if (ret) {
+		mutex_unlock(&opp_table->lock);
+		return ret;
 	}
 
 	list_add(&new_opp->node, head);

drivers/opp/opp.h +1 -1

index 4ced7ffa8158..6f5be6c72f13 100644
@@ -219,7 +219,7 @@ struct opp_table *_find_opp_table(struct device *dev);
 struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table);
 struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table);
 void _opp_free(struct dev_pm_opp *opp);
-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2);
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool rate_not_available);
 int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available);
 int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic);
 void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);

Signature · 1 line

2.29.2

Quoted 18 lines

Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison is erroneously skipped if OPP rate is missing, thus OPPs are left unsorted.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 23 ++++++++++++-----------
 drivers/opp/opp.h  |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

drivers/opp/core.c +0 -0

index 34f7e530d941..5c7f130a8de2 100644
@@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 

drivers/opp/core.c static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, · line 1537

Quoted 6 lines

drivers/opp/core.c +3 -2

-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
+		     bool rate_not_available)
 {
-	if (opp1->rate != opp2->rate)
+	if (!rate_not_available && opp1->rate != opp2->rate)

rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required.

22.12.2020 12:12, Viresh Kumar пишет:

Quoted 19 lines · Dmitry Osipenko

Quoted 18 lines

Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison is erroneously skipped if OPP rate is missing, thus OPPs are left unsorted.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 23 ++++++++++++-----------
 drivers/opp/opp.h  |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

drivers/opp/core.c +0 -0

index 34f7e530d941..5c7f130a8de2 100644
@@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 

Quoted 9 lines

drivers/opp/core.c static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, · line 1537

Quoted 6 lines

drivers/opp/core.c +3 -2

-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
+		     bool rate_not_available)
 {
-	if (opp1->rate != opp2->rate)
+	if (!rate_not_available && opp1->rate != opp2->rate)

rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required.

The rate_not_available is negated in the condition. This change is required because both rates are 0 and then we should proceed to the levels comparison.

I guess it's not clear by looking at this patch, please see a full version of the function:

int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
bool rate_not_available)
{
if (!rate_not_available && opp1->rate != opp2->rate)

    return opp1->rate < opp2->rate ? -1 : 1;
  if (opp1->bandwidth && opp2->bandwidth &&
      opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
    return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
  if (opp1->level != opp2->level)
    return opp1->level < opp2->level ? -1 : 1;

return 0;
}

Perhaps we could check whether opp1->rate=0, like it's done for the opp1->bandwidth. I'll consider this variant for v3, thanks.

Quoted 30 lines

22.12.2020 12:12, Viresh Kumar пишет:

Quoted 28 lines · Dmitry Osipenko

Quoted 15 lines

Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison is erroneously skipped if OPP rate is missing, thus OPPs are left unsorted.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/opp/core.c | 23 ++++++++++++-----------
 drivers/opp/opp.h  |  2 +-
 2 files changed, 13 insertions(+), 12 deletions(-)

drivers/opp/core.c +0 -0

index 34f7e530d941..5c7f130a8de2 100644

drivers/opp/core.c static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, · line 1537

Quoted 9 lines

drivers/opp/core.c +3 -2

@@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 	return true;
 }
 
-int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)
+int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2,
+		     bool rate_not_available)
 {
-	if (opp1->rate != opp2->rate)
+	if (!rate_not_available && opp1->rate != opp2->rate)

rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required.

Quoted 3 lines

The rate_not_available is negated in the condition. This change is required because both rates are 0 and then we should proceed to the levels comparison.

Won't that happen without this patch ?

23.12.2020 07:34, Viresh Kumar пишет:

Quoted 5 lines · Dmitry Osipenko

Quoted 4 lines

22.12.2020 12:12, Viresh Kumar пишет:

Quoted 2 lines

rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required.

Quoted 5 lines

Quoted 3 lines

The rate_not_available is negated in the condition. This change is required because both rates are 0 and then we should proceed to the levels comparison.

Won't that happen without this patch ?

No

Quoted 6 lines

23.12.2020 07:34, Viresh Kumar пишет:

Quoted 5 lines · Dmitry Osipenko

Quoted 4 lines

22.12.2020 12:12, Viresh Kumar пишет:

Quoted 2 lines

rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

The rate_not_available is negated in the condition. This change is required because both rates are 0 and then we should proceed to the levels comparison.

Won't that happen without this patch ?

No

This is how the code looks like currently:

int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2)

{
	if (opp1->rate != opp2->rate)
		return opp1->rate < opp2->rate ? -1 : 1;
	if (opp1->bandwidth && opp2->bandwidth &&
	    opp1->bandwidth[0].peak != opp2->bandwidth[0].peak)
		return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1;
	if (opp1->level != opp2->level)
		return opp1->level < opp2->level ? -1 : 1;
	return 0;
}

Lets consider the case you are focussing on, where rate is 0 for both the OPPs, bandwidth isn't there and we want to run the level comparison here.

Since both the rates are 0, (opp1->rate != opp2->rate) will fail and so we will move to bandwidth check which will fail too. And so we will get to the level comparison.

What am I missing here ? I am sure there is something for sure as you won't have missed this..

Make set_performance_state() callback optional in order to remove the need from power domain drivers to implement a dummy callback. If callback isn't implemented by a GENPD driver, then the performance state is passed to the parent domain.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/base/power/domain.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/base/power/domain.c +5 -7

index 9a14eedacb92..a3e1bfc233d4 100644
@@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
                        goto err;
        }
 
-       ret = genpd->set_performance_state(genpd, state);
-       if (ret)
-               goto err;
+       if (genpd->set_performance_state) {
+               ret = genpd->set_performance_state(genpd, state);
+               if (ret)
+                       goto err;
+       }
 
        genpd->performance_state = state;
        return 0;

@@ -399,9 +401,6 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
        if (!genpd)
                return -ENODEV;
 
-       if (unlikely(!genpd->set_performance_state))
-               return -EINVAL;
-
        if (WARN_ON(!dev->power.subsys_data ||
                     !dev->power.subsys_data->domain_data))
                return -EINVAL;
--

2.29.2

Add "performance" column to debug summary which shows performance state of all power domains and theirs devices.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/base/power/domain.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

drivers/base/power/domain.c +17 -6

index a3e1bfc233d4..1c60dae8540c 100644
@@ -2951,7 +2951,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
        else
                WARN_ON(1);
 
-       seq_puts(s, p);
+       seq_printf(s, "%-25s  ", p);
+}
+
+static void perf_status_str(struct seq_file *s, struct device *dev)
+{
+       struct generic_pm_domain_data *gpd_data;
+
+       gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+       seq_put_decimal_ull(s, "", gpd_data->performance_state);
 }
 
 static int genpd_summary_one(struct seq_file *s,

@@ -2979,7 +2987,7 @@ static int genpd_summary_one(struct seq_file *s,
        else
                snprintf(state, sizeof(state), "%s",
                         status_lookup[genpd->status]);
-       seq_printf(s, "%-30s  %-15s ", genpd->name, state);
+       seq_printf(s, "%-30s  %-50s %u", genpd->name, state, genpd->performance_state);
 
        /*
         * Modifications on the list require holding locks on both

@@ -2987,7 +2995,10 @@ static int genpd_summary_one(struct seq_file *s,
         * Also genpd->name is immutable.
         */
        list_for_each_entry(link, &genpd->parent_links, parent_node) {
-               seq_printf(s, "%s", link->child->name);
+               if (list_is_first(&link->parent_node, &genpd->parent_links))
+                       seq_printf(s, "\n%50s ", link->child->name);
+               else
+                       seq_printf(s, "%s", link->child->name);
                if (!list_is_last(&link->parent_node, &genpd->parent_links))
                        seq_puts(s, ", ");
        }

@@ -3001,6 +3012,7 @@ static int genpd_summary_one(struct seq_file *s,
 
                seq_printf(s, "\n    %-50s  ", kobj_path);
                rtpm_status_str(s, pm_data->dev);
+               perf_status_str(s, pm_data->dev);
                kfree(kobj_path);
        }
 

@@ -3016,9 +3028,9 @@ static int summary_show(struct seq_file *s, void *data)
        struct generic_pm_domain *genpd;
        int ret = 0;
 
-       seq_puts(s, "domain                          status          children\n");
+       seq_puts(s, "domain                          status          children                           performance\n");
        seq_puts(s, "    /device                                             runtime status\n");
-       seq_puts(s, "----------------------------------------------------------------------\n");
+       seq_puts(s, "----------------------------------------------------------------------------------------------\n");
 
        ret = mutex_lock_interruptible(&gpd_list_lock);
        if (ret)
--

2.29.2

Quoted 5 lines

Add "performance" column to debug summary which shows performance state of all power domains and theirs devices.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Ulf Hansson <[email protected]>

As I stated for patch 20, I suggest you resend this to the linux-pm mailing list so it can be picked up by Rafael.

The GR3D1 hardware unit needs to pulse hardware reset after removing power clamp, otherwise reset won't be deasserted. Hence give reset a pulse after removing the clamp. This stayed unnoticed previously because power management wasn't supported by the 3D driver until recently and all power gates are usually ungated after bootloader by default.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/soc/tegra/pmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/soc/tegra/pmc.c +1 -0

index fd2ba3c59178..985373ce52b1 100644
@@ -654,6 +654,14 @@ static int tegra_powergate_power_up(struct tegra_powergate *pg,
 
 	usleep_range(10, 20);
 
+	/*

+++ b/drivers/soc/tegra/core-power-domain.c

drivers/soc/tegra/core-power-domain.c new file +13 -0

@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NVIDIA Tegra SoC Core Power Domain Driver
+ */
+
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+
+#include <soc/tegra/common.h>
+

Quoted 20 lines

+++ b/drivers/soc/tegra/core-power-domain.c

drivers/soc/tegra/core-power-domain.c new file +18 -0

@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NVIDIA Tegra SoC Core Power Domain Driver
+ */
+
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+
+#include <soc/tegra/common.h>
+
+static struct lock_class_key tegra_core_domain_lock_class;
+static bool tegra_core_domain_state_synced;
+
+static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd,
+					     unsigned int level)