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

[Stephen Boyd]

Quoting Tanmay Shah (2020-06-08 20:46:23)
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index d02f4eb..2b982f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -5,6 +5,7 @@
>  
>  #define pr_fmt(fmt)    "[drm-dp] %s: " fmt, __func__
>  
> +#include <linux/rational.h>
>  #include <linux/delay.h>
>  #include <linux/iopoll.h>
>  #include <drm/drm_dp_helper.h>
> @@ -134,59 +135,61 @@ static inline void dp_write_ahb(struct dp_catalog_private *catalog,
>         writel(data, catalog->io->dp_controller.base + offset);
>  }
>  
> -static inline u32 dp_read_cc(struct dp_catalog_private *catalog, u32 offset)
> -{
> -       return readl_relaxed(catalog->io->dp_cc_io.base + offset);
> -}
> -

Why was this added in the first place? Remove it from the place it came
in please.

>  static inline void dp_write_phy(struct dp_catalog_private *catalog,
>                                u32 offset, u32 data)
>  {
> +       offset += DP_PHY_REG_OFFSET;
>         /*
>          * To make sure phy reg writes happens before any other operation,
[...]
> @@ -568,17 +574,37 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
>                                         bool fixed_nvid)
>  {
>         u32 pixel_m, pixel_n;
> -       u32 mvid, nvid;
> +       u32 mvid, nvid, div, pixel_div = 0, dispcc_input_rate;
>         u32 const nvid_fixed = DP_LINK_CONSTANT_N_VALUE;
>         u32 const link_rate_hbr2 = 540000;
>         u32 const link_rate_hbr3 = 810000;
> +       unsigned long den, num;
>  
>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>                                 struct dp_catalog_private, dp_catalog);
>  
> -       pixel_m = dp_read_cc(catalog, MMSS_DP_PIXEL_M);
> -       pixel_n = dp_read_cc(catalog, MMSS_DP_PIXEL_N);
> -       DRM_DEBUG_DP("pixel_m=0x%x, pixel_n=0x%x\n", pixel_m, pixel_n);
> +       div = dp_read_phy(catalog, REG_DP_PHY_VCO_DIV);

Why do we need to read the phy? The pixel_div seems to match what the
clk driver is doing so presumably we can make this follow the link rate
being used vs. having to read the phy.

> +       div &= 0x03;
> +
> +       if (div == 0)
> +               pixel_div = 6;
> +       else if (div == 1)
> +               pixel_div = 2;
> +       else if (div == 2)
> +               pixel_div = 4;
> +       else
> +               DRM_ERROR("Invalid pixel mux divider\n");
> +
> +       dispcc_input_rate = (rate * 10) / pixel_div;
> +
> +       rational_best_approximation(dispcc_input_rate, stream_rate_khz,
> +                       (unsigned long)(1 << 16) - 1,
> +                       (unsigned long)(1 << 16) - 1, &den, &num);
> +
> +       den = ~(den - num);
> +       den = den & 0xFFFF;
> +       pixel_m = num;
> +       pixel_n = den;
>  
>         mvid = (pixel_m & 0xFFFF) * 5;
>         nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF);
> diff --git a/drivers/gpu/drm/msm/dp/dp_pll_10nm.c b/drivers/gpu/drm/msm/dp/dp_pll_10nm.c
> new file mode 100644
> index 0000000..998d659
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_pll_10nm.c
> @@ -0,0 +1,903 @@
> +// 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
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +#include <linux/iopoll.h>

Should be a clk-provider.h include here given that this is providing
clks.

> +
> +#include "dp_hpd.h"
> +#include "dp_pll.h"
> +#include "dp_pll_private.h"
> +
> +#define NUM_PROVIDED_CLKS              2
> +
> +#define DP_LINK_CLK_SRC                        0
> +#define DP_PIXEL_CLK_SRC               1
> +
> +static struct dp_pll_db *dp_pdb;
> +
> +static const struct clk_ops dp_10nm_vco_clk_ops = {
> +       .recalc_rate = dp_vco_recalc_rate_10nm,
> +       .set_rate = dp_vco_set_rate_10nm,
> +       .round_rate = dp_vco_round_rate_10nm,
> +       .prepare = dp_vco_prepare_10nm,
> +       .unprepare = dp_vco_unprepare_10nm,
> +};
> +
> +struct dp_pll_10nm_pclksel {
> +       struct clk_hw hw;
> +
> +       /* divider params */
> +       u8 shift;
> +       u8 width;
> +       u8 flags; /* same flags as used by clk_divider struct */
> +
> +       struct dp_pll_db *pll;
> +};
> +
> +#define to_pll_10nm_pclksel(_hw) \
> +       container_of(_hw, struct dp_pll_10nm_pclksel, hw)
> +
> +static const struct clk_parent_data disp_cc_parent_data_0[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .fw_name = "dp_phy_pll_link_clk", .name = "dp_phy_pll_link_clk" },
> +       { .fw_name = "dp_phy_pll_vco_div_clk",
> +                               .name = "dp_phy_pll_vco_div_clk"},
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static struct dp_pll_vco_clk dp_vco_clk = {
> +       .min_rate = DP_VCO_HSCLK_RATE_1620MHZDIV1000,
> +       .max_rate = DP_VCO_HSCLK_RATE_8100MHZDIV1000,
> +};
> +
> +static int dp_pll_mux_set_parent_10nm(struct clk_hw *hw, u8 val)
> +{
> +       struct dp_pll_10nm_pclksel *pclksel = to_pll_10nm_pclksel(hw);
> +       struct dp_pll_db *dp_res = pclksel->pll;
> +       struct dp_io_pll *pll_io = &dp_res->base->pll_io;
> +       u32 auxclk_div;
> +
> +       auxclk_div = PLL_REG_R(pll_io->phy_base, REG_DP_PHY_VCO_DIV);
> +       auxclk_div &= ~0x03;
> +
> +       if (val == 0)
> +               auxclk_div |= 1;
> +       else if (val == 1)
> +               auxclk_div |= 2;
> +       else if (val == 2)
> +               auxclk_div |= 0;
> +
> +       PLL_REG_W(pll_io->phy_base,
> +                       REG_DP_PHY_VCO_DIV, auxclk_div);
> +       DRM_DEBUG_DP("%s: mux=%d auxclk_div=%x\n", __func__, val, auxclk_div);
> +
> +       return 0;
> +}
> +
> +static u8 dp_pll_mux_get_parent_10nm(struct clk_hw *hw)
> +{
> +       u32 auxclk_div = 0;
> +       struct dp_pll_10nm_pclksel *pclksel = to_pll_10nm_pclksel(hw);
> +       struct dp_pll_db *dp_res = pclksel->pll;
> +       struct dp_io_pll *pll_io = &dp_res->base->pll_io;
> +       u8 val = 0;
> +
> +       auxclk_div = PLL_REG_R(pll_io->phy_base, REG_DP_PHY_VCO_DIV);
> +       auxclk_div &= 0x03;
> +
> +       if (auxclk_div == 1) /* Default divider */
> +               val = 0;
> +       else if (auxclk_div == 2)
> +               val = 1;
> +       else if (auxclk_div == 0)
> +               val = 2;
> +
> +       DRM_DEBUG_DP("%s: auxclk_div=%d, val=%d\n", __func__, auxclk_div, val);
> +
> +       return val;
> +}
> +
> +static int dp_pll_clk_mux_determine_rate(struct clk_hw *hw,
> +                                    struct clk_rate_request *req)
> +{
> +       unsigned long rate = 0;
> +       int ret = 0;
> +
> +       rate = clk_get_rate(hw->clk);
> +
> +       if (rate <= 0) {
> +               DRM_ERROR("Rate is not set properly\n");
> +               return -EINVAL;
> +       }
> +
> +       req->rate = rate;
> +
> +       DRM_DEBUG_DP("%s: rate=%ld\n", __func__, req->rate);
> +       /* Set the new parent of mux if there is a new valid parent */
> +       if (hw->clk && req->best_parent_hw->clk) {
> +               ret = clk_set_parent(hw->clk, req->best_parent_hw->clk);

Why do we need to call clk consumer APIs from the clk provider ops? This
is pretty confusing what's going on here.

> +               if (ret) {
> +                       DRM_ERROR("%s: clk_set_parent failed: ret=%d\n",
> +                                       __func__, ret);
> +                       return ret;
> +               }
> +       }
> +       return 0;
> +}
> +
> +static unsigned long dp_pll_mux_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long parent_rate)
> +{
> +       struct clk_hw *div_clk_hw = NULL, *vco_clk_hw = NULL;
> +       struct dp_pll_vco_clk *vco;
> +
> +       div_clk_hw = clk_hw_get_parent(hw);
> +       if (!div_clk_hw)
> +               return 0;
> +
> +       vco_clk_hw = clk_hw_get_parent(div_clk_hw);
> +       if (!vco_clk_hw)
> +               return 0;
> +
> +       vco = to_dp_vco_hw(vco_clk_hw);
> +       if (!vco)
> +               return 0;
> +
> +       if (vco->rate == DP_VCO_HSCLK_RATE_8100MHZDIV1000)
> +               return (vco->rate / 6);
> +       else if (vco->rate == DP_VCO_HSCLK_RATE_5400MHZDIV1000)
> +               return (vco->rate / 4);
> +       else
> +               return (vco->rate / 2);
> +}
> +
> +static int dp_pll_10nm_get_provider(struct msm_dp_pll *pll,
> +                                    struct clk **link_clk_provider,
> +                                    struct clk **pixel_clk_provider)
> +{
> +       struct clk_hw_onecell_data *hw_data = pll->hw_data;
> +
> +       if (link_clk_provider)
> +               *link_clk_provider = hw_data->hws[DP_LINK_CLK_SRC]->clk;
> +       if (pixel_clk_provider)
> +               *pixel_clk_provider = hw_data->hws[DP_PIXEL_CLK_SRC]->clk;
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops dp_10nm_pclksel_clk_ops = {
> +       .get_parent = dp_pll_mux_get_parent_10nm,
> +       .set_parent = dp_pll_mux_set_parent_10nm,
> +       .recalc_rate = dp_pll_mux_recalc_rate,
> +       .determine_rate = dp_pll_clk_mux_determine_rate,
> +};
> +
> +static struct clk_hw *dp_pll_10nm_pixel_clk_sel(struct dp_pll_db *pll_10nm)
> +{
> +       struct device *dev = &pll_10nm->pdev->dev;
> +       struct dp_pll_10nm_pclksel *pll_pclksel;
> +       struct clk_init_data pclksel_init = {
> +               .parent_data = disp_cc_parent_data_0,
> +               .num_parents = 3,
> +               .name = "dp_phy_pll_vco_div_clk",

So the dp_phy_pll_vco_div_clk has a potential parent that is
dp_phy_pll_vco_div_clk. Huh?

> +               .ops = &dp_10nm_pclksel_clk_ops,
> +       };
> +       int ret;
> +
> +       pll_pclksel = devm_kzalloc(dev, sizeof(*pll_pclksel), GFP_KERNEL);
> +       if (!pll_pclksel)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pll_pclksel->pll = pll_10nm;
> +       pll_pclksel->shift = 0;
> +       pll_pclksel->width = 4;
> +       pll_pclksel->flags = CLK_DIVIDER_ONE_BASED;

Is this flag used?

> +       pll_pclksel->hw.init = &pclksel_init;
> +
> +       ret = clk_hw_register(dev, &pll_pclksel->hw);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return &pll_pclksel->hw;
> +}
> +
> +static int dp_pll_10nm_register(struct dp_pll_db *pll_10nm)
> +{
> +       struct clk_hw_onecell_data *hw_data;
> +       int ret;
> +       struct clk_hw *hw;
> +
> +       struct msm_dp_pll *pll = pll_10nm->base;
> +       struct device *dev = &pll_10nm->pdev->dev;
> +       struct clk_hw **hws = pll_10nm->hws;
> +       int num = 0;
> +       struct clk_init_data vco_init = {
> +               .parent_data = &(const struct clk_parent_data){
> +                               .fw_name = "bi_tcxo",
> +               },
> +               .num_parents = 1,
> +               .name = "dp_vco_clk",
> +               .ops = &dp_10nm_vco_clk_ops,
> +       };

I thought the plan was to not have a vco clk? Just expose the two clks
for the link and the vco divider. Furthermore, drop the divider
"parents" and implement a single clk that programs the right divider
value for the various link rates chosen.

> +
> +       DRM_DEBUG_DP("DP->id = %d", pll_10nm->id);
> +
> +       hw_data = devm_kzalloc(dev, sizeof(*hw_data) +
> +                              NUM_PROVIDED_CLKS * sizeof(struct clk_hw *),
> +                              GFP_KERNEL);
> +       if (!hw_data)
> +               return -ENOMEM;
> +
> +       dp_vco_clk.hw.init = &vco_init;
> +       ret = clk_hw_register(dev, &dp_vco_clk.hw);
> +       if (ret)
> +               return ret;
> +       hws[num++] = &dp_vco_clk.hw;
> +
> +       hw = clk_hw_register_fixed_factor(dev, "dp_phy_pll_link_clk",
> +                               "dp_vco_clk", CLK_SET_RATE_PARENT, 1, 10);
> +
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       hws[num++] = hw;
> +       hw_data->hws[DP_LINK_CLK_SRC] = hw;
> +
> +       hw = clk_hw_register_fixed_factor(dev, "dp_vco_divsel_two_clk_src",
> +                                       "dp_vco_clk",  0, 1, 2);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       hws[num++] = hw;
> +
> +       hw = clk_hw_register_fixed_factor(dev, "dp_vco_divsel_four_clk_src",
> +                                        "dp_vco_clk", 0, 1, 4);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       hws[num++] = hw;
> +
> +       hw = clk_hw_register_fixed_factor(dev, "dp_vco_divsel_six_clk_src",
> +                                        "dp_vco_clk", 0, 1, 6);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       hws[num++] = hw;
> +
> +       hw = dp_pll_10nm_pixel_clk_sel(pll_10nm);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +
> +       hws[num++] = hw;
> +       hw_data->hws[DP_PIXEL_CLK_SRC] = hw;
> +
> +       pll_10nm->num_hws = num;
> +
> +       hw_data->num = NUM_PROVIDED_CLKS;
> +       pll->hw_data = hw_data;
> +
> +       ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
> +                                    pll->hw_data);
> +       if (ret) {
> +               DRM_DEV_ERROR(dev, "failed to register clk provider: %d\n",
> +                               ret);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +int msm_dp_pll_10nm_init(struct msm_dp_pll *pll, int id)
> +{
> +       struct dp_pll_db *dp_10nm_pll;
> +       struct platform_device *pdev = pll->pdev;
> +       int ret;
> +
> +       dp_10nm_pll = devm_kzalloc(&pdev->dev,
> +                                       sizeof(*dp_10nm_pll), GFP_KERNEL);
> +       if (!dp_10nm_pll)
> +               return -ENOMEM;
> +
> +       DRM_DEBUG_DP("DP PLL%d", id);
> +
> +       dp_10nm_pll->base = pll;
> +       dp_10nm_pll->pdev = pll->pdev;
> +       dp_10nm_pll->id = id;
> +       dp_pdb = dp_10nm_pll;
> +       pll->priv = (void *)dp_10nm_pll;
> +       dp_vco_clk.priv = pll;
> +
> +       ret = of_property_read_u32(pdev->dev.of_node, "cell-index",
> +                               &dp_10nm_pll->index);
> +       if (ret) {
> +               DRM_ERROR("Unable to get the cell-index ret=%d\n", ret);
> +               dp_10nm_pll->index = 0;
> +       }

Is the cell-index used for anything?

> +
> +       ret = dp_pll_10nm_register(dp_10nm_pll);
> +       if (ret) {
> +               DRM_DEV_ERROR(&pdev->dev, "failed to register PLL: %d\n", ret);
> +               return ret;
> +       }
> +
> +       pll->get_provider = dp_pll_10nm_get_provider;
> +
> +       return ret;
> +}