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; > +}