diff mbox series

[v1,11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling

Message ID 20201104234427.26477-12-digetx@gmail.com
State New
Headers show
Series [v1,01/30] dt-bindings: host1x: Document OPP and voltage regulator properties | expand

Commit Message

Dmitry Osipenko Nov. 4, 2020, 11:44 p.m. UTC
Add OPP and SoC core voltage scaling support to the display controller
driver. This is required for enabling system-wide DVFS on older Tegra
SoCs.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/Kconfig |   1 +
 drivers/gpu/drm/tegra/dc.c    | 138 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/tegra/dc.h    |   5 ++
 3 files changed, 143 insertions(+), 1 deletion(-)

Comments

Thierry Reding Nov. 10, 2020, 8:29 p.m. UTC | #1
On Thu, Nov 05, 2020 at 02:44:08AM +0300, Dmitry Osipenko wrote:
> Add OPP and SoC core voltage scaling support to the display controller
> driver. This is required for enabling system-wide DVFS on older Tegra
> SoCs.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/Kconfig |   1 +
>  drivers/gpu/drm/tegra/dc.c    | 138 +++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/tegra/dc.h    |   5 ++
>  3 files changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 1650a448eabd..9eec4c3fbd3b 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -12,6 +12,7 @@ config DRM_TEGRA
>  	select INTERCONNECT
>  	select IOMMU_IOVA
>  	select CEC_CORE if CEC_NOTIFIER
> +	select PM_OPP
>  	help
>  	  Choose this option if you have an NVIDIA Tegra SoC.
>  
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index fd7c8828652d..babcb66a335b 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -11,9 +11,13 @@
>  #include <linux/interconnect.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  
> +#include <soc/tegra/common.h>
> +#include <soc/tegra/fuse.h>
>  #include <soc/tegra/pmc.h>
>  
>  #include <drm/drm_atomic.h>
> @@ -1699,6 +1703,55 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
>  	return 0;
>  }
>  
> +static void tegra_dc_update_voltage_state(struct tegra_dc *dc,
> +					  struct tegra_dc_state *state)
> +{
> +	struct dev_pm_opp *opp;
> +	unsigned long rate;
> +	int err, min_uV;
> +
> +	/* OPP usage is optional */
> +	if (!dc->opp_table)
> +		return;
> +
> +	/* calculate actual pixel clock rate which depends on internal divider */
> +	rate = DIV_ROUND_UP(clk_get_rate(dc->clk) * 2, state->div + 2);
> +
> +	/* find suitable OPP for the rate */
> +	opp = dev_pm_opp_find_freq_ceil(dc->dev, &rate);
> +
> +	if (opp == ERR_PTR(-ERANGE))
> +		opp = dev_pm_opp_find_freq_floor(dc->dev, &rate);
> +
> +	if (IS_ERR(opp)) {
> +		dev_err(dc->dev, "failed to find OPP for %lu Hz: %ld\n",
> +			rate, PTR_ERR(opp));
> +		return;
> +	}
> +
> +	min_uV = dev_pm_opp_get_voltage(opp);
> +	dev_pm_opp_put(opp);
> +
> +	/*
> +	 * Voltage scaling is optional and trying to set voltage for a dummy
> +	 * regulator will error out.
> +	 */
> +	if (!device_property_present(dc->dev, "core-supply"))
> +		return;

This is a potentially heavy operation, so I think we should avoid that
here. How about you use devm_regulator_get_optional() in ->probe()? That
returns -ENODEV if no regulator was specified, in which case you can set
dc->core_reg = NULL and use that as the condition here.

> +
> +	/*
> +	 * Note that the minimum core voltage depends on the pixel clock
> +	 * rate (which depends on internal clock divider of CRTC) and not on
> +	 * the rate of the display controller clock. This is why we're not
> +	 * using dev_pm_opp_set_rate() API and instead are managing the
> +	 * voltage by ourselves.
> +	 */
> +	err = regulator_set_voltage(dc->core_reg, min_uV, INT_MAX);
> +	if (err)
> +		dev_err(dc->dev, "failed to set CORE voltage to %duV: %d\n",
> +			min_uV, err);
> +}

Also, I'd prefer if the flow here was more linear, such as:

	if (dc->core_reg) {
		err = regulator_set_voltage(...);
		...
	}

> +
>  static void tegra_dc_commit_state(struct tegra_dc *dc,
>  				  struct tegra_dc_state *state)
>  {
> @@ -1738,6 +1791,8 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
>  	if (err < 0)
>  		dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
>  			dc->clk, state->pclk, err);
> +
> +	tegra_dc_update_voltage_state(dc, state);
>  }
>  
>  static void tegra_dc_stop(struct tegra_dc *dc)
> @@ -2521,6 +2576,7 @@ static int tegra_dc_runtime_suspend(struct host1x_client *client)
>  
>  	clk_disable_unprepare(dc->clk);
>  	pm_runtime_put_sync(dev);
> +	regulator_disable(dc->core_reg);
>  
>  	return 0;
>  }
> @@ -2531,10 +2587,16 @@ static int tegra_dc_runtime_resume(struct host1x_client *client)
>  	struct device *dev = client->dev;
>  	int err;
>  
> +	err = regulator_enable(dc->core_reg);
> +	if (err < 0) {
> +		dev_err(dev, "failed to enable CORE regulator: %d\n", err);
> +		return err;
> +	}
> +
>  	err = pm_runtime_get_sync(dev);
>  	if (err < 0) {
>  		dev_err(dev, "failed to get runtime PM: %d\n", err);
> -		return err;
> +		goto disable_regulator;
>  	}
>  
>  	if (dc->soc->has_powergate) {
> @@ -2564,6 +2626,9 @@ static int tegra_dc_runtime_resume(struct host1x_client *client)
>  	clk_disable_unprepare(dc->clk);
>  put_rpm:
>  	pm_runtime_put_sync(dev);
> +disable_regulator:
> +	regulator_disable(dc->core_reg);
> +
>  	return err;
>  }
>  
> @@ -2879,6 +2944,72 @@ static int tegra_dc_couple(struct tegra_dc *dc)
>  	return 0;
>  }
>  
> +static void tegra_dc_deinit_opp_table(void *data)
> +{
> +	struct tegra_dc *dc = data;
> +
> +	dev_pm_opp_of_remove_table(dc->dev);
> +	dev_pm_opp_put_supported_hw(dc->opp_table);
> +	dev_pm_opp_put_regulators(dc->opp_table);
> +}
> +
> +static int devm_tegra_dc_opp_table_init(struct tegra_dc *dc)
> +{
> +	struct opp_table *hw_opp_table;
> +	u32 hw_version;
> +	int err;
> +
> +	/* voltage scaling is optional */
> +	dc->core_reg = devm_regulator_get(dc->dev, "core");
> +	if (IS_ERR(dc->core_reg))
> +		return dev_err_probe(dc->dev, PTR_ERR(dc->core_reg),
> +				     "failed to get CORE regulator\n");
> +
> +	/* legacy device-trees don't have OPP table */
> +	if (!device_property_present(dc->dev, "operating-points-v2"))
> +		return 0;

"Legacy" is a bit confusing here. For one, no device trees currently
have these tables and secondly, for newer SoCs we may never need them.

> +
> +	dc->opp_table = dev_pm_opp_get_opp_table(dc->dev);
> +	if (IS_ERR(dc->opp_table))
> +		return dev_err_probe(dc->dev, PTR_ERR(dc->opp_table),
> +				     "failed to prepare OPP table\n");
> +
> +	if (of_machine_is_compatible("nvidia,tegra20"))
> +		hw_version = BIT(tegra_sku_info.soc_process_id);
> +	else
> +		hw_version = BIT(tegra_sku_info.soc_speedo_id);
> +
> +	hw_opp_table = dev_pm_opp_set_supported_hw(dc->dev, &hw_version, 1);
> +	err = PTR_ERR_OR_ZERO(hw_opp_table);

What's the point of this? A more canonical version would be:

	if (IS_ERR(hw_opp_table)) {
		err = PTR_ERR(hw_opp_table);
		dev_err(dc->dev, ...);
		goto put_table;
	}

That uses the same number of lines but is much easier to read, in my
opinion, because it is the canonical form.

> +	if (err) {
> +		dev_err(dc->dev, "failed to set supported HW: %d\n", err);
> +		goto put_table;
> +	}
> +
> +	err = dev_pm_opp_of_add_table(dc->dev);
> +	if (err) {
> +		dev_err(dc->dev, "failed to add OPP table: %d\n", err);
> +		goto put_hw;
> +	}
> +
> +	err = devm_add_action(dc->dev, tegra_dc_deinit_opp_table, dc);
> +	if (err)
> +		goto remove_table;

Do these functions return positive values? If not, I'd prefer if this
check was more explicit (i.e. err < 0) for consistency with the rest of
this code.

> +
> +	dev_info(dc->dev, "OPP HW ver. 0x%x\n", hw_version);
> +
> +	return 0;
> +
> +remove_table:
> +	dev_pm_opp_of_remove_table(dc->dev);
> +put_hw:
> +	dev_pm_opp_put_supported_hw(dc->opp_table);
> +put_table:
> +	dev_pm_opp_put_opp_table(dc->opp_table);
> +
> +	return err;
> +}
> +
>  static int tegra_dc_probe(struct platform_device *pdev)
>  {
>  	struct tegra_dc *dc;
> @@ -2937,6 +3068,10 @@ static int tegra_dc_probe(struct platform_device *pdev)
>  		tegra_powergate_power_off(dc->powergate);
>  	}
>  
> +	err = devm_tegra_dc_opp_table_init(dc);
> +	if (err < 0)
> +		return err;
> +
>  	dc->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(dc->regs))
>  		return PTR_ERR(dc->regs);
> @@ -3007,6 +3142,7 @@ struct platform_driver tegra_dc_driver = {
>  	.driver = {
>  		.name = "tegra-dc",
>  		.of_match_table = tegra_dc_of_match,
> +		.sync_state = tegra_soc_device_sync_state,
>  	},
>  	.probe = tegra_dc_probe,
>  	.remove = tegra_dc_remove,
> diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> index ba4ed35139fb..fd774fc5c2e4 100644
> --- a/drivers/gpu/drm/tegra/dc.h
> +++ b/drivers/gpu/drm/tegra/dc.h
> @@ -13,6 +13,8 @@
>  
>  #include "drm.h"
>  
> +struct opp_table;
> +struct regulator;
>  struct tegra_output;
>  
>  #define TEGRA_DC_LEGACY_PLANES_NUM	6
> @@ -107,6 +109,9 @@ struct tegra_dc {
>  	struct drm_info_list *debugfs_files;
>  
>  	const struct tegra_dc_soc_info *soc;
> +
> +	struct opp_table *opp_table;
> +	struct regulator *core_reg;

We typically use a _supply suffix on regulators to avoid confusing this
with "register".

Thierry
Mark Brown Nov. 11, 2020, 11:55 a.m. UTC | #2
On Wed, Nov 11, 2020 at 12:23:41AM +0300, Dmitry Osipenko wrote:
> 10.11.2020 23:32, Mark Brown пишет:

> >>> +	if (!device_property_present(dc->dev, "core-supply"))
> >>> +		return;

> >> This is a potentially heavy operation, so I think we should avoid that
> >> here. How about you use devm_regulator_get_optional() in ->probe()? That
> >> returns -ENODEV if no regulator was specified, in which case you can set
> >> dc->core_reg = NULL and use that as the condition here.

> > Or enumerate the configurable voltages after getting the regulator and
> > handle that appropriately which would be more robust in case there's
> > missing or unusual constraints.

> I already changed that code to use regulator_get_optional() for v2.

That doesn't look entirely appropriate given that the core does most
likely require some kind of power to operate.

> Regarding the enumerating supported voltage.. I think this should be
> done by the OPP core, but regulator core doesn't work well if
> regulator_get() is invoked more than one time for the same device, at
> least there is a loud debugfs warning about an already existing

I don't understand why this would be an issue - if nothing else the core
could just offer an interface to trigger the check.

> directory for a regulator. It's easy to check whether the debug
> directory exists before creating it, like thermal framework does it for
> example, but then there were some other more difficult issues.. I don't
> recall what they were right now. Perhaps will be easier to simply get a
> error from regulator_set_voltage() for now because it shouldn't ever
> happen in practice, unless device-tree has wrong constraints.

The constraints might not be wrong, there might be some board which has
a constraint somewhere for
Dmitry Osipenko Nov. 15, 2020, 5:42 p.m. UTC | #3
13.11.2020 20:28, Mark Brown пишет:
> On Fri, Nov 13, 2020 at 08:13:49PM +0300, Dmitry Osipenko wrote:

>> 13.11.2020 19:15, Mark Brown пишет:

> 

>>> My point here is that the driver shouldn't be checking for a dummy

>>> regulator, the driver should be checking the features that are provided

>>> to it by the regulator and handling those.

> 

>> I understand yours point, but then we need dummy regulator to provide

>> all the features, and currently it does the opposite.

> 

> As could any other regulator?


Yes

>>> It doesn't matter if this is

>>> a dummy regulator or an actual regulator with limited features, the

>>> effect is the same and the handling should be the same.  If the driver

>>> is doing anything to handle dummy regulators explicitly as dummy

>>> regulators it is doing it wrong.

> 

>> It matters because dummy regulator errors out all checks and changes

>> other than enable/disable, instead of accepting them. If we could add an

>> option for dummy regulator to succeed all the checks and accept all the

>> values, then it could become more usable.

> 

> I'm a bit confused here TBH - I'm not sure I see a substantial

> difference between a consumer detecting that it can't set any voltages

> at all and the handling for an optional regulator.  Either way if it's

> going to carry on and assume that whatever voltage is there works for

> everything it boils down to setting a flag saying to skip the set

> voltage operation.  I think you are too focused on the specific

> implementation you currently have here.

> 

> We obviously can't just accept voltage change operations when we've no

> idea what the current voltage of the device is.

> 

>>> To repeat you should *only* be using regulator_get_optional() in the

>>> case where the supply may be physically absent which is not the case

>>> here.

>>

>> Alright, but then we either need to improve regulator core to make dummy

>> regulator a bit more usable, or continue to work around it in drivers.

>> What should we do?

> 

> As I keep saying the consumer driver should be enumerating the voltages

> it can set, if it can't find any and wants to continue then it can just

> skip setting voltages later on.  If only some are unavailable then it

> probably wants to eliminate those specific OPPs instead.


I'm seeing a dummy regulator as a helper for consumer drivers which
removes burden of handling an absent (optional) regulator. Is this a
correct understanding of a dummy regulator?

Older DTBs don't have a regulator and we want to keep them working. This
is equal to a physically absent regulator and in this case it's an
optional regulator, IMO.

Consumer drivers definitely should check voltages, but this should be
done only for a physical regulator.
Mark Brown Nov. 16, 2020, 1:33 p.m. UTC | #4
On Sun, Nov 15, 2020 at 08:42:10PM +0300, Dmitry Osipenko wrote:
> 13.11.2020 20:28, Mark Brown пишет:

> >> What should we do?

> > As I keep saying the consumer driver should be enumerating the voltages
> > it can set, if it can't find any and wants to continue then it can just
> > skip setting voltages later on.  If only some are unavailable then it
> > probably wants to eliminate those specific OPPs instead.

> I'm seeing a dummy regulator as a helper for consumer drivers which
> removes burden of handling an absent (optional) regulator. Is this a
> correct understanding of a dummy regulator?

> Older DTBs don't have a regulator and we want to keep them working. This
> is equal to a physically absent regulator and in this case it's an
> optional regulator, IMO.

No, you are failing to understand the purpose of this code.  To
reiterate unless the device supports operating with the supply
physically absent then the driver should not be attempting to use
regulator_get_optional().  That exists specifically for the case where
the supply may be absent, nothing else.  The dummy regulator is there
precisely for the case where the system does not describe supplies that
we know are required for the device to function, it fixes up that
omission so we don't need to open code handling of this in every single
consumer driver.

Regulators that are present but not described by the firmware are a
clearly different case to regulators that are not physically there,
hardware with actually optional regulators will generally require some
configuration for this case.
Mark Brown Nov. 19, 2020, 3:19 p.m. UTC | #5
On Thu, Nov 19, 2020 at 05:22:43PM +0300, Dmitry Osipenko wrote:
> 16.11.2020 16:33, Mark Brown пишет:

> > No, you are failing to understand the purpose of this code.  To
> > reiterate unless the device supports operating with the supply
> > physically absent then the driver should not be attempting to use
> > regulator_get_optional().  That exists specifically for the case where

> The original intention of regulator_get_optional() is clear to me, but
> nothing really stops drivers from slightly re-purposing this API, IMO.

> Drivers should be free to assume that if regulator isn't defined by
> firmware, then it's physically absent if this doesn't break anything. Of
> course in some cases it's unsafe to make such assumptions. I think it's
> a bit unpractical to artificially limit API usage without a good reason,
> i.e. if nothing breaks underneath of a driver.

If the supply can be physically absent without breaking anything then
this is the intended use case for optional regulators.  This is a *very*
uncommon.

> > Regulators that are present but not described by the firmware are a
> > clearly different case to regulators that are not physically there,
> > hardware with actually optional regulators will generally require some
> > configuration for this case.

> I have good news. After spending some more time on trying out different
> things, I found that my previous assumption about the fixed-regulator
> was wrong, it actually accepts voltage changes, i.e. regulator consumer
> doesn't get a error on a voltage-change. This is exactly what is needed
> for the OPP core to work properly.

To be clear when you set a voltage range you will get the minimum
voltage that can be supported within that range on the system given all
the other constraints the system has.  For fixed voltage regulators or
regulators constraints to not change voltage this means that if whatever
voltage they are fixed at is in the range requested then the API will
report success.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index 1650a448eabd..9eec4c3fbd3b 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -12,6 +12,7 @@  config DRM_TEGRA
 	select INTERCONNECT
 	select IOMMU_IOVA
 	select CEC_CORE if CEC_NOTIFIER
+	select PM_OPP
 	help
 	  Choose this option if you have an NVIDIA Tegra SoC.
 
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index fd7c8828652d..babcb66a335b 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -11,9 +11,13 @@ 
 #include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 
+#include <soc/tegra/common.h>
+#include <soc/tegra/fuse.h>
 #include <soc/tegra/pmc.h>
 
 #include <drm/drm_atomic.h>
@@ -1699,6 +1703,55 @@  int tegra_dc_state_setup_clock(struct tegra_dc *dc,
 	return 0;
 }
 
+static void tegra_dc_update_voltage_state(struct tegra_dc *dc,
+					  struct tegra_dc_state *state)
+{
+	struct dev_pm_opp *opp;
+	unsigned long rate;
+	int err, min_uV;
+
+	/* OPP usage is optional */
+	if (!dc->opp_table)
+		return;
+
+	/* calculate actual pixel clock rate which depends on internal divider */
+	rate = DIV_ROUND_UP(clk_get_rate(dc->clk) * 2, state->div + 2);
+
+	/* find suitable OPP for the rate */
+	opp = dev_pm_opp_find_freq_ceil(dc->dev, &rate);
+
+	if (opp == ERR_PTR(-ERANGE))
+		opp = dev_pm_opp_find_freq_floor(dc->dev, &rate);
+
+	if (IS_ERR(opp)) {
+		dev_err(dc->dev, "failed to find OPP for %lu Hz: %ld\n",
+			rate, PTR_ERR(opp));
+		return;
+	}
+
+	min_uV = dev_pm_opp_get_voltage(opp);
+	dev_pm_opp_put(opp);
+
+	/*
+	 * Voltage scaling is optional and trying to set voltage for a dummy
+	 * regulator will error out.
+	 */
+	if (!device_property_present(dc->dev, "core-supply"))
+		return;
+
+	/*
+	 * Note that the minimum core voltage depends on the pixel clock
+	 * rate (which depends on internal clock divider of CRTC) and not on
+	 * the rate of the display controller clock. This is why we're not
+	 * using dev_pm_opp_set_rate() API and instead are managing the
+	 * voltage by ourselves.
+	 */
+	err = regulator_set_voltage(dc->core_reg, min_uV, INT_MAX);
+	if (err)
+		dev_err(dc->dev, "failed to set CORE voltage to %duV: %d\n",
+			min_uV, err);
+}
+
 static void tegra_dc_commit_state(struct tegra_dc *dc,
 				  struct tegra_dc_state *state)
 {
@@ -1738,6 +1791,8 @@  static void tegra_dc_commit_state(struct tegra_dc *dc,
 	if (err < 0)
 		dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
 			dc->clk, state->pclk, err);
+
+	tegra_dc_update_voltage_state(dc, state);
 }
 
 static void tegra_dc_stop(struct tegra_dc *dc)
@@ -2521,6 +2576,7 @@  static int tegra_dc_runtime_suspend(struct host1x_client *client)
 
 	clk_disable_unprepare(dc->clk);
 	pm_runtime_put_sync(dev);
+	regulator_disable(dc->core_reg);
 
 	return 0;
 }
@@ -2531,10 +2587,16 @@  static int tegra_dc_runtime_resume(struct host1x_client *client)
 	struct device *dev = client->dev;
 	int err;
 
+	err = regulator_enable(dc->core_reg);
+	if (err < 0) {
+		dev_err(dev, "failed to enable CORE regulator: %d\n", err);
+		return err;
+	}
+
 	err = pm_runtime_get_sync(dev);
 	if (err < 0) {
 		dev_err(dev, "failed to get runtime PM: %d\n", err);
-		return err;
+		goto disable_regulator;
 	}
 
 	if (dc->soc->has_powergate) {
@@ -2564,6 +2626,9 @@  static int tegra_dc_runtime_resume(struct host1x_client *client)
 	clk_disable_unprepare(dc->clk);
 put_rpm:
 	pm_runtime_put_sync(dev);
+disable_regulator:
+	regulator_disable(dc->core_reg);
+
 	return err;
 }
 
@@ -2879,6 +2944,72 @@  static int tegra_dc_couple(struct tegra_dc *dc)
 	return 0;
 }
 
+static void tegra_dc_deinit_opp_table(void *data)
+{
+	struct tegra_dc *dc = data;
+
+	dev_pm_opp_of_remove_table(dc->dev);
+	dev_pm_opp_put_supported_hw(dc->opp_table);
+	dev_pm_opp_put_regulators(dc->opp_table);
+}
+
+static int devm_tegra_dc_opp_table_init(struct tegra_dc *dc)
+{
+	struct opp_table *hw_opp_table;
+	u32 hw_version;
+	int err;
+
+	/* voltage scaling is optional */
+	dc->core_reg = devm_regulator_get(dc->dev, "core");
+	if (IS_ERR(dc->core_reg))
+		return dev_err_probe(dc->dev, PTR_ERR(dc->core_reg),
+				     "failed to get CORE regulator\n");
+
+	/* legacy device-trees don't have OPP table */
+	if (!device_property_present(dc->dev, "operating-points-v2"))
+		return 0;
+
+	dc->opp_table = dev_pm_opp_get_opp_table(dc->dev);
+	if (IS_ERR(dc->opp_table))
+		return dev_err_probe(dc->dev, PTR_ERR(dc->opp_table),
+				     "failed to prepare OPP table\n");
+
+	if (of_machine_is_compatible("nvidia,tegra20"))
+		hw_version = BIT(tegra_sku_info.soc_process_id);
+	else
+		hw_version = BIT(tegra_sku_info.soc_speedo_id);
+
+	hw_opp_table = dev_pm_opp_set_supported_hw(dc->dev, &hw_version, 1);
+	err = PTR_ERR_OR_ZERO(hw_opp_table);
+	if (err) {
+		dev_err(dc->dev, "failed to set supported HW: %d\n", err);
+		goto put_table;
+	}
+
+	err = dev_pm_opp_of_add_table(dc->dev);
+	if (err) {
+		dev_err(dc->dev, "failed to add OPP table: %d\n", err);
+		goto put_hw;
+	}
+
+	err = devm_add_action(dc->dev, tegra_dc_deinit_opp_table, dc);
+	if (err)
+		goto remove_table;
+
+	dev_info(dc->dev, "OPP HW ver. 0x%x\n", hw_version);
+
+	return 0;
+
+remove_table:
+	dev_pm_opp_of_remove_table(dc->dev);
+put_hw:
+	dev_pm_opp_put_supported_hw(dc->opp_table);
+put_table:
+	dev_pm_opp_put_opp_table(dc->opp_table);
+
+	return err;
+}
+
 static int tegra_dc_probe(struct platform_device *pdev)
 {
 	struct tegra_dc *dc;
@@ -2937,6 +3068,10 @@  static int tegra_dc_probe(struct platform_device *pdev)
 		tegra_powergate_power_off(dc->powergate);
 	}
 
+	err = devm_tegra_dc_opp_table_init(dc);
+	if (err < 0)
+		return err;
+
 	dc->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(dc->regs))
 		return PTR_ERR(dc->regs);
@@ -3007,6 +3142,7 @@  struct platform_driver tegra_dc_driver = {
 	.driver = {
 		.name = "tegra-dc",
 		.of_match_table = tegra_dc_of_match,
+		.sync_state = tegra_soc_device_sync_state,
 	},
 	.probe = tegra_dc_probe,
 	.remove = tegra_dc_remove,
diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index ba4ed35139fb..fd774fc5c2e4 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -13,6 +13,8 @@ 
 
 #include "drm.h"
 
+struct opp_table;
+struct regulator;
 struct tegra_output;
 
 #define TEGRA_DC_LEGACY_PLANES_NUM	6
@@ -107,6 +109,9 @@  struct tegra_dc {
 	struct drm_info_list *debugfs_files;
 
 	const struct tegra_dc_soc_info *soc;
+
+	struct opp_table *opp_table;
+	struct regulator *core_reg;
 };
 
 static inline struct tegra_dc *