| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <159175481823.242598.9387748150892951863@swboyd.mtv.corp.google.com>
Date: Tue, 09 Jun 2020 19:06:58 -0700
From: Stephen Boyd <swboyd@...omium.org>
To: Tanmay Shah <tanmay@...eaurora.org>,
dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org
Cc: sam@...nborg.org, seanpaul@...omium.org,
freedreno@...ts.freedesktop.org, chandanu@...eaurora.org,
robdclark@...il.com, abhinavk@...eaurora.org,
nganji@...eaurora.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, Vara Reddy <varar@...eaurora.org>,
Tanmay Shah <tanmay@...eaurora.org>
Subject: Re: [PATCH v6 4/5] drm/msm/dp: add support for DP PLL driver
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;
> +}
Powered by blists - more mailing lists