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

[Thierry Reding]

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@xxxxxxxxx>
> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: PGP signature