lkml.org 
[lkml]   [2020]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2020-11-10 21:30    [W:5.697 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site