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

[tanmay@codeaurora.org]

On 2020-04-23 17:26, Stephen Boyd wrote:
> 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.
> 
This will be taken care at later point of time. For now we have removed
PLL bindings and made PLL as module of DP driver.

>> + *
>> + */
>> +
>> +#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!
> 
Same here.
>> +
>> +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.
> 
Same here.
>> +
>> +       DRM_DEBUG_DP("%s: rrate=%ld\n", __func__, rrate);
>> +
>> +       *parent_rate = rrate;
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno