[Freedreno] [DPU PATCH v5 4/5] drm/msm/dp: add support for DP PLL driver

[Stephen Boyd]

Quoting Tanmay Shah (2020-03-31 17:30:30)
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c
> new file mode 100644
> index 0000000..aa845d0
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + *              +------------------------------+
> + *              |         DP_VCO_CLK           |
> + *              |                              |
> + *              |    +-------------------+     |
> + *              |    |   (DP PLL/VCO)    |     |
> + *              |    +---------+---------+     |
> + *              |              v               |
> + *              |   +----------+-----------+   |
> + *              |   | hsclk_divsel_clk_src |   |
> + *              |   +----------+-----------+   |
> + *              +------------------------------+
> + *                              |
> + *          +---------<---------v------------>----------+
> + *          |                                           |
> + * +--------v---------+                                 |
> + * |    dp_phy_pll    |                                 |
> + * |     link_clk     |                                 |
> + * +--------+---------+                                 |
> + *          |                                           |
> + *          |                                           |
> + *          v                                           v
> + * Input to DISPCC block                                |
> + * for link clk, crypto clk                             |
> + * and interface clock                                  |
> + *                                                      |
> + *                                                      |
> + *      +--------<------------+-----------------+---<---+
> + *      |                     |                 |
> + * +----v---------+  +--------v-----+  +--------v------+
> + * | vco_divided  |  | vco_divided  |  | vco_divided   |
> + * |    _clk_src  |  |    _clk_src  |  |    _clk_src   |
> + * |              |  |              |  |               |
> + * |divsel_six    |  |  divsel_two  |  |  divsel_four  |
> + * +-------+------+  +-----+--------+  +--------+------+
> + *         |                 |                  |
> + *         v---->----------v-------------<------v
> + *                         |
> + *              +----------+---------+
> + *              |   dp_phy_pll_vco   |
> + *              |       div_clk      |
> + *              +---------+----------+
> + *                        |
> + *                        v
> + *              Input to DISPCC block
> + *              for DP pixel clock

I suspect this shouldn't be a complicated clk provider at all. Take a
look at commit 42d068472ddf ("phy: Add DisplayPort configuration
options") for how the phy should manage the link rate, etc. If the
dispcc pixel clock needs to know what rate is coming in, then a single
clk_hw can be implemented here that tells the consumer (i.e. dispcc) the
rate that it will see at the output of this node. Otherwise, modeling
the clk tree inside this PLL block like this is super overly complicated
and wasteful. Don't do it.

> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +
> +#include "dp_pll_10nm.h"
> +
> +#define NUM_PROVIDED_CLKS              2
> +
> +#define DP_LINK_CLK_SRC                        0
> +#define DP_PIXEL_CLK_SRC               1
> +
[...]
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
> new file mode 100644
> index 0000000..fff2e8d
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
> @@ -0,0 +1,524 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt)    "%s: " fmt, __func__
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +
> +#include "dp_hpd.h"
> +#include "dp_pll.h"
> +#include "dp_pll_10nm.h"
> +
[...]
> +
> +static int dp_config_vco_rate_10nm(struct msm_dp_pll *pll,
> +               unsigned long rate)
> +{
> +       u32 res = 0;
> +       struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> +
> +       res = dp_vco_pll_init_db_10nm(pll, rate);
> +       if (res) {
> +               DRM_ERROR("VCO Init DB failed\n");
> +               return res;
> +       }
> +
> +       if (dp_res->lane_cnt != 4) {
> +               if (dp_res->orientation == ORIENTATION_CC2)
> +                       PLL_REG_W(dp_res->phy_base, REG_DP_PHY_PD_CTL, 0x6d);
> +               else
> +                       PLL_REG_W(dp_res->phy_base, REG_DP_PHY_PD_CTL, 0x75);
> +       } else {
> +               PLL_REG_W(dp_res->phy_base, REG_DP_PHY_PD_CTL, 0x7d);
> +       }

For example, this part here can be done through the phy configuration
ops. A lane count check in the set rate clk op is quite odd!

> +
> +long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
> +                       unsigned long *parent_rate)
> +{
> +       unsigned long rrate = rate;
> +       struct msm_dp_pll *pll = to_msm_dp_pll(hw);
> +
> +       if (rate <= pll->min_rate)
> +               rrate = pll->min_rate;
> +       else if (rate <= DP_VCO_HSCLK_RATE_2700MHZDIV1000)
> +               rrate = DP_VCO_HSCLK_RATE_2700MHZDIV1000;
> +       else if (rate <= DP_VCO_HSCLK_RATE_5400MHZDIV1000)
> +               rrate = DP_VCO_HSCLK_RATE_5400MHZDIV1000;
> +       else
> +               rrate = pll->max_rate;

This is basically link rate setting through the clk framework. Calling
clk_set_rate() on the pixel clk is complicated and opaque. I'd expect to
see the DP controller driver set the link rate on the phy with
phy_configure(), and then that can change the rate that is seen
downstream at the pixel clk. Does the pixel clk need to do anything with
the rate? Probably not? I suspect it can just enable the pixel clk when
it needs to and disable it when it doesn't need it.

> +
> +       DRM_DEBUG_DP("%s: rrate=%ld\n", __func__, rrate);
> +
> +       *parent_rate = rrate;