Messages in this thread | | | Date | Thu, 11 Jun 2020 13:31:30 -0700 | From | tanmay@codeauro ... | 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@codeaurora.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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
| |