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
