Message-ID: <95406b922b3aee8a70e4ca8997710622@codeaurora.org>
Date:   Thu, 11 Jun 2020 13:31:30 -0700
From:   tanmay@...eaurora.org
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, abhinavk@...eaurora.org,
        seanpaul@...omium.org, dri-devel@...ts.freedesktop.org,
        Vara Reddy <varar@...eaurora.org>, sam@...nborg.org,
        linux-clk@...r.kernel.org, chandanu@...eaurora.org
Subject: Re: [PATCH v6 4/5] drm/msm/dp: add support for DP PLL driver

Hi Stephen,

Thanks for reviews.
Please ignore previous response to this patch. Here, I have re-organized 
it.

Thanks,

On 2020-06-11 13:07, tanmay@...eaurora.org wrote:
> On 2020-06-09 19:06, Stephen Boyd wrote:
>> 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.
>> 
> Sure. I will remove it as part of DP base driver patch.
>>>  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.
>> 
> 
> As mentioned in above diagram, there is an additional divider in the
> PLL after the VCO (vco_divided_clk_src).
> The input rate to the dispcc branch is (vco_rate * 10)/ 
> vco_dividied_clk_src.
> In order to know the MNDs at the dispcc, we need to know the input 
> rate.
> This register read is to figure out which divider value is currently
> being set in the PLL.
> This input rate is not the same as the link rate. When we move the
> PHY/PLL to a separate driver,
> we would have take care of finding a different way to get this input 
> rate.
>>> +       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.
>> 
> Yes. currently it is included in dp_parser.h but I will include here as 
> well.
>>> +
>>> +#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.
>> 
> Sure. Use of clk_set_parent is redundant here and I will remove it.
>>> +               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?
>> 
> Thats right. I will remove dp_phy_pll_vco_div_clk from parent list
> disp_cc_parent_data_0.
> 
>>> +               .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?
>> 
> No it is redundant. I will remove.
>>> +       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.
>> 
This will be taken care at later point of time.

>>> +
>>> +       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?
>> 
> No it is redundant and will be removed in next patch.
>>> +
>>> +       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;
>>> +}
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@...ts.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Powered by blists - more mailing lists