This patch is too big. Please split it up more. I have far too many
review comments on it so it will help if you keep splitting the patch up
into smaller parts that can be more easily reviewed and fixed.
Quoting Chandan Uddaraju (2018-10-10 10:15:59)
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 08a52f5..e23beee 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1051,6 +1051,7 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
> {
> int ret = 0;
>
> + ctrl->power->set_link_clk_parent(ctrl->power);
> ctrl->power->set_pixel_clk_parent(ctrl->power);
>
> dp_ctrl_set_clock_rate(ctrl, "ctrl_link_clk",
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8c98399..2bf6635 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -72,6 +72,48 @@ struct dp_display_private {
> {}
> };
>
> +static int dp_get_pll(struct dp_display_private *dp_priv)
> +{
> + struct platform_device *pdev = NULL;
> + struct platform_device *pll_pdev;
> + struct device_node *pll_node;
> + struct dp_parser *dp_parser = NULL;
> +
> + if (!dp_priv) {
> + pr_err("Invalid Arguments\n");
> + return -EINVAL;
> + }
> +
> + pdev = dp_priv->pdev;
> + dp_parser = dp_priv->parser;
> +
> + if (!dp_parser) {
> + pr_err("Parser not initialized.\n");
> + return -EINVAL;
> + }
> +
> + pll_node = of_parse_phandle(pdev->dev.of_node, "pll-node", 0);
> + if (!pll_node) {
> + dev_err(&pdev->dev, "cannot find pll device\n");
> + return -ENXIO;
> + }
> +
> + pll_pdev = of_find_device_by_node(pll_node);
> + if (pll_pdev)
> + dp_parser->pll = platform_get_drvdata(pll_pdev);
> +
> + of_node_put(pll_node);
> +
> + if (!pll_pdev || !dp_parser->pll) {
> + dev_err(&pdev->dev, "%s: pll driver is not ready\n", __func__);
> + return -EPROBE_DEFER;
> + }
> +
> + dp_parser->pll_dev = get_device(&pll_pdev->dev);
Why do we need this code? Are we working around some lack of ability to
link different devices together by resource requesting?
> +
> + return 0;
> +}
> +
> static irqreturn_t dp_display_irq(int irq, void *dev_id)
> {
> struct dp_display_private *dp = dev_id;
> @@ -125,6 +167,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> + rc = dp_get_pll(dp);
> + if (rc) {
> + pr_err(" DP get PLL instance failed\n");
> + goto end;
> + }
> +
> rc = dp->aux->drm_aux_register(dp->aux);
> if (rc) {
> pr_err("DRM DP AUX register failed\n");
> @@ -921,6 +969,7 @@ int __init msm_dp_register(void)
> {
> int ret;
>
> + msm_dp_pll_driver_register();
> ret = platform_driver_register(&dp_display_driver);
> if (ret) {
> pr_err("driver register failed");
> @@ -932,6 +981,7 @@ int __init msm_dp_register(void)
>
> void __exit msm_dp_unregister(void)
> {
> + msm_dp_pll_driver_unregister();
> platform_driver_unregister(&dp_display_driver);
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index ca5eae5..9cd6a6b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -52,4 +52,7 @@ struct msm_dp {
>
> int dp_display_get_num_of_displays(void);
> int dp_display_get_displays(void **displays, int count);
> +void __init msm_dp_pll_driver_register(void);
> +void __exit msm_dp_pll_driver_unregister(void);
__init and __exit in header files does nothing.
> +
> #endif /* _DP_DISPLAY_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 1d58480..3a1679c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -23,7 +23,9 @@ struct dp_power_private {
> struct dp_parser *parser;
> struct platform_device *pdev;
> struct clk *pixel_clk_rcg;
> - struct clk *pixel_parent;
> + struct clk *link_clk_src;
> + struct clk *pixel_provider;
> + struct clk *link_provider;
>
> struct dp_power dp_power;
>
> @@ -154,6 +156,16 @@ static int dp_power_clk_init(struct dp_power_private *power, bool enable)
> goto exit;
> }
>
> + if (power->parser->pll && power->parser->pll->get_provider) {
> + rc = power->parser->pll->get_provider(power->parser->pll,
> + &power->link_provider, &power->pixel_provider);
> + if (rc) {
> + pr_info("%s: can't get provider from pll, don't set parent\n",
> + __func__);
> + return 0;
> + }
> + }
> +
> if (enable) {
> rc = msm_dss_get_clk(dev, core->clk_config, core->num_clk);
> if (rc) {
> @@ -173,17 +185,10 @@ static int dp_power_clk_init(struct dp_power_private *power, bool enable)
> if (IS_ERR(power->pixel_clk_rcg)) {
> pr_debug("Unable to get DP pixel clk RCG\n");
> power->pixel_clk_rcg = NULL;
> - }
> -
> - power->pixel_parent = devm_clk_get(dev, "pixel_parent");
> - if (IS_ERR(power->pixel_parent)) {
> - pr_debug("Unable to get DP pixel RCG parent\n");
> - power->pixel_parent = NULL;
> + rc = -ENODEV;
> + goto ctrl_get_error;
> }
> } else {
> - if (power->pixel_parent)
> - devm_clk_put(dev, power->pixel_parent);
> -
> if (power->pixel_clk_rcg)
> devm_clk_put(dev, power->pixel_clk_rcg);
>
> @@ -459,6 +464,49 @@ static void dp_power_client_deinit(struct dp_power *dp_power)
>
> }
>
> +static int dp_power_set_link_clk_parent(struct dp_power *dp_power)
> +{
> + int rc = 0;
> + struct dp_power_private *power;
> + u32 num;
> + struct dss_clk *cfg;
> + char *name = "ctrl_link_clk";
> +
> + if (!dp_power) {
> + pr_err("invalid power data\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + power = container_of(dp_power, struct dp_power_private, dp_power);
> +
> + num = power->parser->mp[DP_CTRL_PM].num_clk;
> + cfg = power->parser->mp[DP_CTRL_PM].clk_config;
> +
> + while (num && strcmp(cfg->clk_name, name)) {
> + num--;
> + cfg++;
> + }
> +
> + if (num && power->link_provider) {
> + power->link_clk_src = clk_get_parent(cfg->clk);
> + if (power->link_clk_src) {
> + clk_set_parent(power->link_clk_src, power->link_provider);
> + pr_debug("%s: is the parent of clk=%s\n",
> + __clk_get_name(power->link_provider),
> + __clk_get_name(power->link_clk_src));
> + } else {
> + pr_err("couldn't get parent for clk=%s\n", name);
> + rc = -EINVAL;
> + }
> + } else {
> + pr_err("%s clock could not be set parent\n", name);
> + rc = -EINVAL;
> + }
Hopefully this can just be done with assigned-clock-parents.
> +exit:
> + return rc;
> +}
> +
> static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> {
> int rc = 0;
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> index d1adaaf..5effd32 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.h
> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> @@ -24,6 +24,7 @@
> * @init: initializes the regulators/core clocks/GPIOs/pinctrl
> * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl
> * @clk_enable: enable/disable the DP clocks
> + * @set_link_clk_parent: set the parent of DP link clock
> * @set_pixel_clk_parent: set the parent of DP pixel clock
> */
> struct dp_power {
> @@ -31,6 +32,7 @@ struct dp_power {
> int (*deinit)(struct dp_power *power);
> int (*clk_enable)(struct dp_power *power, enum dp_pm_type pm_type,
> bool enable);
> + int (*set_link_clk_parent)(struct dp_power *power);
> int (*set_pixel_clk_parent)(struct dp_power *power);
Seeing these clk wrappers is quite scary. Is this driver written with
some sort of linux abstraction layer inside?
> int (*power_client_init)(struct dp_power *power);
> void (*power_client_deinit)(struct dp_power *power);
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll.c b/drivers/gpu/drm/msm/dp/pll/dp_pll.c
> new file mode 100644
> index 0000000..a8612b5
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll.c
> @@ -0,0 +1,153 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include "dp_pll.h"
> +
> +int msm_dp_pll_util_parse_dt_clock(struct platform_device *pdev,
> + struct msm_dp_pll *pll)
> +{
> + u32 i = 0, rc = 0;
> + struct dss_module_power *mp = &pll->mp;
> + const char *clock_name;
> + u32 clock_rate;
> +
> + mp->num_clk = of_property_count_strings(pdev->dev.of_node,
> + "clock-names");
> + if (mp->num_clk <= 0) {
> + pr_err("clocks are not defined\n");
> + goto clk_err;
> + }
> +
> + mp->clk_config = devm_kzalloc(&pdev->dev,
> + sizeof(struct dss_clk) * mp->num_clk, GFP_KERNEL);
> + if (!mp->clk_config) {
> + rc = -ENOMEM;
> + mp->num_clk = 0;
> + goto clk_err;
> + }
> +
> + for (i = 0; i < mp->num_clk; i++) {
> + of_property_read_string_index(pdev->dev.of_node, "clock-names",
> + i, &clock_name);
> + strlcpy(mp->clk_config[i].clk_name, clock_name,
> + sizeof(mp->clk_config[i].clk_name));
> +
> + of_property_read_u32_index(pdev->dev.of_node, "clock-rate",
> + i, &clock_rate);
> + mp->clk_config[i].rate = clock_rate;
> +
> + if (!clock_rate)
> + mp->clk_config[i].type = DSS_CLK_AHB;
> + else
> + mp->clk_config[i].type = DSS_CLK_PCLK;
Is this custom assigned-clock-rates?
> + }
> +
> +clk_err:
> + return rc;
> +}
> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
> + enum msm_dp_pll_type type, int id)
> +{
> + struct device *dev = &pdev->dev;
> + struct msm_dp_pll *pll;
> +
> + switch (type) {
> + case MSM_DP_PLL_10NM:
> + pll = msm_dp_pll_10nm_init(pdev, id);
> + break;
> + default:
> + pll = ERR_PTR(-ENXIO);
> + break;
> + }
> +
> + if (IS_ERR(pll)) {
> + dev_err(dev, "%s: failed to init DP PLL\n", __func__);
> + return pll;
> + }
> +
> + pll->type = type;
> +
> + DBG("DP:%d PLL registered", id);
> +
> + return pll;
> +}
> +
> +static const struct of_device_id dp_pll_dt_match[] = {
> +#ifdef CONFIG_DRM_MSM_DP_10NM_PLL
Don't do this.
> + { .compatible = "qcom,dp-pll-10nm" },
> +#endif
> + {}
> +};
> +
> +static int dp_pll_driver_probe(struct platform_device *pdev)
> +{
> + struct msm_dp_pll *pll;
> + struct device *dev = &pdev->dev;
> + const struct of_device_id *match;
> + enum msm_dp_pll_type type;
> +
> + match = of_match_node(dp_pll_dt_match, dev->of_node);
> + if (!match)
> + return -ENODEV;
> +
> + if (!strcmp(match->compatible, "qcom,dp-pll-10nm"))
> + type = MSM_DP_PLL_10NM;
> + else
> + type = MSM_DP_PLL_MAX;
> +
> + pll = msm_dp_pll_init(pdev, type, 0);
> + if (IS_ERR_OR_NULL(pll)) {
IS_ERR_OR_NULL is typically a bad sign.
> + dev_info(dev,
> + "%s: pll init failed: %ld, need separate pll clk driver\n",
> + __func__, PTR_ERR(pll));
Well if it's NULL, then it's not a PTR_ERR.
> + return -ENODEV;
And then we just overwrite whatever is returned, ouch.
> + }
> +
> + platform_set_drvdata(pdev, pll);
> +
> + return 0;
> +}
> +
> +static int dp_pll_driver_remove(struct platform_device *pdev)
> +{
> + struct msm_dp_pll *pll = platform_get_drvdata(pdev);
> +
> + if (pll) {
> + //msm_dsi_pll_destroy(pll);
Leftovers?
> + pll = NULL;
> + }
> +
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +static struct platform_driver dp_pll_platform_driver = {
> + .probe = dp_pll_driver_probe,
> + .remove = dp_pll_driver_remove,
> + .driver = {
> + .name = "msm_dp_pll",
> + .of_match_table = dp_pll_dt_match,
> + },
> +};
> +
> +void __init msm_dp_pll_driver_register(void)
> +{
> + platform_driver_register(&dp_pll_platform_driver);
> +}
> +
> +void __exit msm_dp_pll_driver_unregister(void)
> +{
> + platform_driver_unregister(&dp_pll_platform_driver);
> +}
Consider module_platform_driver()? Why is this directly probed from
somewhere else? Some sort of DPU design?
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll.h b/drivers/gpu/drm/msm/dp/pll/dp_pll.h
> new file mode 100644
> index 0000000..d52a81a
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll.h
> @@ -0,0 +1,64 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __DP_PLL_H
> +#define __DP_PLL_H
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
Typically a bad sign to see a clk provider and consumer in the same
place.
> +#include <linux/platform_device.h>
> +
> +#include "dpu_io_util.h"
> +#include "msm_drv.h"
> +
> +#define PLL_REG_W(base, offset, data) \
> + writel_relaxed((data), (base) + (offset))
> +#define PLL_REG_R(base, offset) readl_relaxed((base) + (offset))
Can't we just use writel and readl directly?
> +
> +enum msm_dp_pll_type {
> + MSM_DP_PLL_10NM,
> + MSM_DP_PLL_MAX
> +};
> +
> +struct msm_dp_pll {
> + enum msm_dp_pll_type type;
> + struct clk_hw clk_hw;
> + unsigned long rate; /* current vco rate */
Why are we duplicating what is known to the framework?
> + u64 min_rate; /* min vco rate */
> + u64 max_rate; /* max vco rate */
Use clk_hw_set_rate_range?
> + bool pll_on;
More duplicating of what clk framework knows?
> + void *priv;
> + /* Pll specific resources like GPIO, power supply, clocks, etc*/
> + struct dss_module_power mp;
> + int (*get_provider)(struct msm_dp_pll *pll,
> + struct clk **link_clk_provider,
> + struct clk **pixel_clk_provider);
> +};
> +
> +#define hw_clk_to_pll(x) container_of(x, struct msm_dp_pll, clk_hw)
Usually this would be called to_msm_dp_pll().
> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
> + enum msm_dp_pll_type type, int id);
> +
> +int msm_dp_pll_util_parse_dt_clock(struct platform_device *pdev,
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c
> new file mode 100644
> index 0000000..a180413
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c
> @@ -0,0 +1,401 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + * +------------------------------+
> + * | DP_VCO_CLK |
> + * | |
> + * | +-------------------+ |
> + * | | (DP PLL/VCO) | |
> + * | +---------+---------+ |
> + * | v |
> + * | +----------+-----------+ |
> + * | | hsclk_divsel_clk_src | |
> + * | +----------+-----------+ |
Is this a divider?
> + * +------------------------------+
> + * |
> + * +------------<---------v------------>----------+
> + * | |
> + * +-----v------------+ |
> + * | dp_link_clk_src | |
> + * | divsel_ten | |
> + * +---------+--------+ |
And dispcc gets a fixed div-10 on the output of the PLL divider?
> + * | |
> + * | |
> + * 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
> + * |
> + * +----------+---------+
> + * | vco_divided_clk |
> + * | _src_mux |
> + * +---------+----------+
> + * |
> + * v
> + * Input to DISPCC block
> + * for DP pixel clock
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/clk.h>
I'd expect clk-provider.h here, instead of clk.h
> +
> +#include "dp_pll_10nm.h"
> +
> +#define NUM_PROVIDED_CLKS 2
> +
> +#define DP_LINK_CLK_SRC 0
> +#define DP_PIXEL_CLK_SRC 1
> +
> +static struct dp_pll_10nm *dp_pdb;
> +
> +/* Op structures */
Drop useless comments please.
> +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_10nm *pll;
> +};
> +#define to_pll_10nm_pclksel(_hw) container_of(_hw, struct dp_pll_10nm_pclksel, hw)
> +
> +static int dp_mux_set_parent_10nm(struct clk_hw *hw, u8 val)
> +{
> + struct dp_pll_10nm_pclksel *pclksel = to_pll_10nm_pclksel(hw);
> + struct dp_pll_10nm *dp_res = pclksel->pll;
> + u32 auxclk_div;
> +
> + auxclk_div = PLL_REG_R(dp_res->phy_base, DP_PHY_VCO_DIV);
> + auxclk_div &= ~0x03; /* bits 0 to 1 */
> +
> + if (val == 0) /* mux parent index = 0 */
> + auxclk_div |= 1;
> + else if (val == 1) /* mux parent index = 1 */
> + auxclk_div |= 2;
> + else if (val == 2) /* mux parent index = 2 */
> + auxclk_div |= 0;
Is this really a mux? It looks like it's a divider that divides the PLL
frequency down. Why add in an extra level of set_parent things when we
could just as easily do division and call up to parent with some
multiplied rate?
> +
> + PLL_REG_W(dp_res->phy_base,
> + DP_PHY_VCO_DIV, auxclk_div);
> + /* Make sure the PHY registers writes are done */
Which this wmb() doesn't do.
> + wmb();
> + pr_debug("%s: mux=%d auxclk_div=%x\n", __func__, val, auxclk_div);
> +
> + return 0;
> +}
> +
> +static u8 dp_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_10nm *dp_res = pclksel->pll;
> + u8 val = 0;
> +
> + pr_err("clk_hw->init->name = %s\n", hw->init->name);
> + auxclk_div = PLL_REG_R(dp_res->phy_base, 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;
> +
> + pr_debug("%s: auxclk_div=%d, val=%d\n", __func__, auxclk_div, val);
> +
> + return val;
Yes, all very odd!
> +}
> +
> +static int clk_mux_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + int ret = 0;
> +
> + ret = __clk_mux_determine_rate_closest(hw, req);
> + if (ret)
> + return ret;
> +
> + /* Set the new parent of mux if there is a new valid parent */
> + if (hw->clk && req->best_parent_hw->clk)
> + clk_set_parent(hw->clk, req->best_parent_hw->clk);
No idea what this is, but again we shouldn't be touching the hardware in
determine_rate, and we certainly shouldn't be using clk consumer APIs
from within clk provider ops.
> +
> + return 0;
> +}
> +
> +static unsigned long mux_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk *div_clk = NULL, *vco_clk = NULL;
> + struct msm_dp_pll *vco = NULL;
> +
> + div_clk = clk_get_parent(hw->clk);
> + if (!div_clk)
> + return 0;
> +
> + vco_clk = clk_get_parent(div_clk);
> + if (!vco_clk)
> + return 0;
> +
> + vco = hw_clk_to_pll(__clk_get_hw(vco_clk));
> + if (!vco)
> + return 0;
All this parent checking should go away.
> +
> + 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);
I'd expect the recalc_rate callback to read some hardware register and
figure out what rate is coming through the clk. This doesn't look to be
doing that. Instead it's reaching up two levels and then looking at some
cached rate inside there to figure out what rate is coming here? That is
weird.
> +}
> +
> +static int dp_pll_10nm_get_provider(struct msm_dp_pll *pll,
> + struct clk **link_clk_provider,
> + struct clk **pixel_clk_provider)
> +{
> + struct dp_pll_10nm *pll_10nm = to_dp_pll_10nm(pll);
> + struct clk_hw_onecell_data *hw_data = pll_10nm->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;
> +}
I don't know what this function is for, but it looks like some weird
wrapper around kernel things which just makes for more reading and less
transferable knowledge because we have to learn how different drivers
decide to wrap kernel constructs.
> +
> +static const struct clk_ops dp_10nm_pclksel_clk_ops = {
> + .get_parent = dp_mux_get_parent_10nm,
> + .set_parent = dp_mux_set_parent_10nm,
> + .recalc_rate = mux_recalc_rate,
> + .determine_rate = clk_mux_determine_rate,
> +};
> +
> +static struct clk_hw *dp_pll_10nm_pixel_clk_sel(struct dp_pll_10nm *pll_10nm)
> +{
> + struct device *dev = &pll_10nm->pdev->dev;
> + struct dp_pll_10nm_pclksel *pll_pclksel;
> + struct clk_init_data pclksel_init = {
> + .parent_names = (const char *[]){
> + "dp_vco_divsel_two_clk_src",
> + "dp_vco_divsel_four_clk_src",
> + "dp_vco_divsel_six_clk_src" },
As I said earlier, this should probably just be a divider and not a mux.
> + .num_parents = 3,
> + .name = "dp_vco_divided_clk_src_mux",
> + .flags = CLK_IGNORE_UNUSED,
Why have this flag? Seems like a workaround for some problem, but who
knows what that problem is.
> + .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;
And it is a divider? But not really?
> + 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_10nm *pll_10nm)
> +{
> + char clk_name[32], parent[32], vco_name[32];
> + struct clk_init_data vco_init = {
> + .parent_names = (const char *[]){ "bi_tcxo" },
> + .num_parents = 1,
> + .name = vco_name,
> + .flags = CLK_IGNORE_UNUSED,
> + .ops = &dp_10nm_vco_clk_ops,
> + };
> + struct device *dev = &pll_10nm->pdev->dev;
> + struct clk_hw **hws = pll_10nm->hws;
> + struct clk_hw_onecell_data *hw_data;
> + struct clk_hw *hw;
> + int num = 0;
> + int ret;
> +
> + DBG("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;
> +
> + snprintf(vco_name, 32, "dp_vco_clk");
> + pll_10nm->base.clk_hw.init = &vco_init;
> + ret = clk_hw_register(dev, &pll_10nm->base.clk_hw);
> + if (ret)
> + return ret;
> + hws[num++] = &pll_10nm->base.clk_hw;
> +
> + snprintf(clk_name, 32, "dp_link_clk_divsel_ten");
> + snprintf(parent, 32, "dp_vco_clk");
Why are we snprintf strings that are static? Why not just pass them in
places they're used?
> + hw = clk_hw_register_fixed_factor(dev, clk_name, parent,
> + 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;
> +
> + snprintf(clk_name, 32, "dp_vco_divsel_two_clk_src");
> + snprintf(parent, 32, "dp_vco_clk");
> + hw = clk_hw_register_fixed_factor(dev, clk_name, parent,
> + 0, 1, 2);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + hws[num++] = hw;
> +
> + snprintf(clk_name, 32, "dp_vco_divsel_four_clk_src");
> + snprintf(parent, 32, "dp_vco_clk");
> + hw = clk_hw_register_fixed_factor(dev, clk_name, parent,
> + 0, 1, 4);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + hws[num++] = hw;
> +
> + snprintf(clk_name, 32, "dp_vco_divsel_six_clk_src");
> + snprintf(parent, 32, "dp_vco_clk");
> + hw = clk_hw_register_fixed_factor(dev, clk_name, parent,
> + 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;
The good news is that this should be sharply reduced by implementing a
divider instead of fixed dividers that a virtual mux picks from.
> +
> + pll_10nm->num_hws = num;
> +
> + hw_data->num = NUM_PROVIDED_CLKS;
> + pll_10nm->hw_data = hw_data;
> +
> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
> + pll_10nm->hw_data);
> + if (ret) {
> + dev_err(dev, "failed to register clk provider: %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +struct msm_dp_pll *msm_dp_pll_10nm_init(struct platform_device *pdev, int id)
> +{
> + struct dp_pll_10nm *dp_10nm_pll;
> + struct msm_dp_pll *pll;
> + int ret;
> +
> + if (!pdev)
> + return ERR_PTR(-ENODEV);
> +
> + dp_10nm_pll = devm_kzalloc(&pdev->dev, sizeof(*dp_10nm_pll), GFP_KERNEL);
> + if (!dp_10nm_pll)
> + return ERR_PTR(-ENOMEM);
> +
> + DBG("DP PLL%d", id);
> +
> + dp_10nm_pll->pdev = pdev;
> + dp_10nm_pll->id = id;
> + dp_pdb = dp_10nm_pll;
> +
> + dp_10nm_pll->pll_base = msm_ioremap(pdev, "pll_base", "DP_PLL");
> + if (IS_ERR_OR_NULL(dp_10nm_pll->pll_base)) {
> + dev_err(&pdev->dev, "failed to map CMN PLL base\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + dp_10nm_pll->phy_base = msm_ioremap(pdev, "phy_base", "DP_PHY");
> + if (IS_ERR_OR_NULL(dp_10nm_pll->phy_base)) {
> + dev_err(&pdev->dev, "failed to map CMN PHY base\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + dp_10nm_pll->ln_tx0_base = msm_ioremap(pdev, "ln_tx0_base", "DP_LN_TX0");
> + if (IS_ERR_OR_NULL(dp_10nm_pll->ln_tx0_base)) {
> + dev_err(&pdev->dev, "failed to map CMN LN_TX0 base\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + dp_10nm_pll->ln_tx1_base = msm_ioremap(pdev, "ln_tx1_base", "DP_LN_TX1");
> + if (IS_ERR_OR_NULL(dp_10nm_pll->ln_tx1_base)) {
> + dev_err(&pdev->dev, "failed to map CMN LN_TX1 base\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "cell-index",
> + &dp_10nm_pll->index);
> + if (ret) {
> + pr_err("Unable to get the cell-index ret=%d\n", ret);
> + dp_10nm_pll->index = 0;
> + }
> +
> + ret = msm_dp_pll_util_parse_dt_clock(pdev, &dp_10nm_pll->base);
> + if (ret) {
> + pr_err("Unable to parse dt clocks ret=%d\n", ret);
> + return ERR_PTR(ret);
> + }
> +
> + ret = dp_pll_10nm_register(dp_10nm_pll);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register PLL: %d\n", ret);
> + return ERR_PTR(ret);
> + }
> +
> + pll = &dp_10nm_pll->base;
> + pll->min_rate = DP_VCO_HSCLK_RATE_1620MHZDIV1000;
> + pll->max_rate = DP_VCO_HSCLK_RATE_8100MHZDIV1000;
Pass this to the framework with clk_hw_set_rate_range() and let the
common clk framework clamp min/max for you.
> + pll->get_provider = dp_pll_10nm_get_provider;
> +
> + return pll;
> +}
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h
> new file mode 100644
> index 0000000..f966beb
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h
> @@ -0,0 +1,94 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __DP_PLL_10NM_H
> +#define __DP_PLL_10NM_H
> +
> +#include "dp_pll.h"
> +#include "dp_reg.h"
> +
> +#define DP_VCO_HSCLK_RATE_1620MHZDIV1000 1620000UL
> +#define DP_VCO_HSCLK_RATE_2700MHZDIV1000 2700000UL
> +#define DP_VCO_HSCLK_RATE_5400MHZDIV1000 5400000UL
> +#define DP_VCO_HSCLK_RATE_8100MHZDIV1000 8100000UL
> +
> +#define NUM_DP_CLOCKS_MAX 6
> +
> +#define DP_PHY_PLL_POLL_SLEEP_US 500
> +#define DP_PHY_PLL_POLL_TIMEOUT_US 10000
> +
> +#define DP_VCO_RATE_8100MHZDIV1000 8100000UL
> +#define DP_VCO_RATE_9720MHZDIV1000 9720000UL
> +#define DP_VCO_RATE_10800MHZDIV1000 10800000UL
> +
> +struct dp_pll_10nm {
> + struct msm_dp_pll base;
> +
> + int id;
> + struct platform_device *pdev;
> +
> + void __iomem *pll_base;
> + void __iomem *phy_base;
> + void __iomem *ln_tx0_base;
> + void __iomem *ln_tx1_base;
> +
> + /* private clocks: */
> + struct clk_hw *hws[NUM_DP_CLOCKS_MAX];
> + u32 num_hws;
> +
> + /* clock-provider: */
> + struct clk_hw_onecell_data *hw_data;
> +
> + /* lane and orientation settings */
> + u8 lane_cnt;
> + u8 orientation;
> +
> + /* COM PHY settings */
> + u32 hsclk_sel;
> + u32 dec_start_mode0;
> + u32 div_frac_start1_mode0;
> + u32 div_frac_start2_mode0;
> + u32 div_frac_start3_mode0;
> + u32 integloop_gain0_mode0;
> + u32 integloop_gain1_mode0;
> + u32 vco_tune_map;
> + u32 lock_cmp1_mode0;
> + u32 lock_cmp2_mode0;
> + u32 lock_cmp3_mode0;
> + u32 lock_cmp_en;
> +
> + /* PHY vco divider */
> + u32 phy_vco_div;
> + /*
> + * Certain pll's needs to update the same vco rate after resume in
PLLs is plural, not possessive.
> + * suspend/resume scenario. Cached the vco rate for such plls.
There you go!
> + */
> + unsigned long vco_cached_rate;
> + u32 cached_cfg0;
> + u32 cached_cfg1;
> + u32 cached_outdiv;
Is this to restore across suspend/resume?
> +
> + uint32_t index;
We use u32 in kernel code, not uint32_t.
> +};
> +
> +#define to_dp_pll_10nm(x) container_of(x, struct dp_pll_10nm, base)
> +
> +int dp_vco_set_rate_10nm(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate);
> +unsigned long dp_vco_recalc_rate_10nm(struct clk_hw *hw,
> + unsigned long parent_rate);
> +long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate);
> +int dp_vco_prepare_10nm(struct clk_hw *hw);
> +void dp_vco_unprepare_10nm(struct clk_hw *hw);
> +#endif /* __DP_PLL_10NM_H */
> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
> new file mode 100644
> index 0000000..9eb6881
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
> @@ -0,0 +1,531 @@
> +/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/iopoll.h>
> +#include <linux/delay.h>
Where's clk-provider.h?
> +
> +#include "dp_pll.h"
> +#include "dp_pll_10nm.h"
> +#include "dp_extcon.h"
Is this used?
> +
> +static int dp_vco_pll_init_db_10nm(struct msm_dp_pll *pll,
> + unsigned long rate)
> +{
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> + u32 spare_value = 0;
> +
> + spare_value = PLL_REG_R(dp_res->phy_base, DP_PHY_SPARE0);
> + dp_res->lane_cnt = spare_value & 0x0F;
> + dp_res->orientation = (spare_value & 0xF0) >> 4;
> +
> + pr_debug("%s: spare_value=0x%x, ln_cnt=0x%x, orientation=0x%x\n",
> + __func__, spare_value, dp_res->lane_cnt, dp_res->orientation);
> +
> + switch (rate) {
> + case DP_VCO_HSCLK_RATE_1620MHZDIV1000:
> + pr_debug("%s: VCO rate: %ld\n", __func__,
> + DP_VCO_RATE_9720MHZDIV1000);
These debug prints don't match the case statement above. Why?
> + dp_res->hsclk_sel = 0x0c;
> + dp_res->dec_start_mode0 = 0x69;
> + dp_res->div_frac_start1_mode0 = 0x00;
> + dp_res->div_frac_start2_mode0 = 0x80;
> + dp_res->div_frac_start3_mode0 = 0x07;
> + dp_res->integloop_gain0_mode0 = 0x3f;
> + dp_res->integloop_gain1_mode0 = 0x00;
> + dp_res->vco_tune_map = 0x00;
> + dp_res->lock_cmp1_mode0 = 0x6f;
> + dp_res->lock_cmp2_mode0 = 0x08;
> + dp_res->lock_cmp3_mode0 = 0x00;
> + dp_res->phy_vco_div = 0x1;
> + dp_res->lock_cmp_en = 0x00;
> + break;
> + case DP_VCO_HSCLK_RATE_2700MHZDIV1000:
> + pr_debug("%s: VCO rate: %ld\n", __func__,
> + DP_VCO_RATE_10800MHZDIV1000);
> + dp_res->hsclk_sel = 0x04;
> + dp_res->dec_start_mode0 = 0x69;
> + dp_res->div_frac_start1_mode0 = 0x00;
> + dp_res->div_frac_start2_mode0 = 0x80;
> + dp_res->div_frac_start3_mode0 = 0x07;
> + dp_res->integloop_gain0_mode0 = 0x3f;
> + dp_res->integloop_gain1_mode0 = 0x00;
> + dp_res->vco_tune_map = 0x00;
> + dp_res->lock_cmp1_mode0 = 0x0f;
> + dp_res->lock_cmp2_mode0 = 0x0e;
> + dp_res->lock_cmp3_mode0 = 0x00;
> + dp_res->phy_vco_div = 0x1;
> + dp_res->lock_cmp_en = 0x00;
> + break;
> + case DP_VCO_HSCLK_RATE_5400MHZDIV1000:
> + pr_debug("%s: VCO rate: %ld\n", __func__,
> + DP_VCO_RATE_10800MHZDIV1000);
> + dp_res->hsclk_sel = 0x00;
> + dp_res->dec_start_mode0 = 0x8c;
> + dp_res->div_frac_start1_mode0 = 0x00;
> + dp_res->div_frac_start2_mode0 = 0x00;
> + dp_res->div_frac_start3_mode0 = 0x0a;
> + dp_res->integloop_gain0_mode0 = 0x3f;
> + dp_res->integloop_gain1_mode0 = 0x00;
> + dp_res->vco_tune_map = 0x00;
> + dp_res->lock_cmp1_mode0 = 0x1f;
> + dp_res->lock_cmp2_mode0 = 0x1c;
> + dp_res->lock_cmp3_mode0 = 0x00;
> + dp_res->phy_vco_div = 0x2;
> + dp_res->lock_cmp_en = 0x00;
> + break;
> + case DP_VCO_HSCLK_RATE_8100MHZDIV1000:
> + pr_debug("%s: VCO rate: %ld\n", __func__,
> + DP_VCO_RATE_8100MHZDIV1000);
Ah this one matches though.
> + dp_res->hsclk_sel = 0x03;
> + dp_res->dec_start_mode0 = 0x69;
> + dp_res->div_frac_start1_mode0 = 0x00;
> + dp_res->div_frac_start2_mode0 = 0x80;
> + dp_res->div_frac_start3_mode0 = 0x07;
> + dp_res->integloop_gain0_mode0 = 0x3f;
> + dp_res->integloop_gain1_mode0 = 0x00;
> + dp_res->vco_tune_map = 0x00;
> + dp_res->lock_cmp1_mode0 = 0x2f;
> + dp_res->lock_cmp2_mode0 = 0x2a;
> + dp_res->lock_cmp3_mode0 = 0x00;
> + dp_res->phy_vco_div = 0x0;
> + dp_res->lock_cmp_en = 0x08;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int dp_config_vco_rate_10nm(struct msm_dp_pll *pll,
> + unsigned long rate)
> +{
> + u32 res = 0;
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> +
> + res = dp_vco_pll_init_db_10nm(pll, rate);
> + if (res) {
> + pr_err("VCO Init DB failed\n");
> + return res;
> + }
> +
> + if (dp_res->lane_cnt != 4) {
> + if (dp_res->orientation == ORIENTATION_CC2)
> + PLL_REG_W(dp_res->phy_base, DP_PHY_PD_CTL, 0x6d);
> + else
> + PLL_REG_W(dp_res->phy_base, DP_PHY_PD_CTL, 0x75);
> + } else {
> + PLL_REG_W(dp_res->phy_base, DP_PHY_PD_CTL, 0x7d);
> + }
> +
> + /* Make sure the PHY register writes are done */
> + wmb();
> +
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_SVS_MODE_CLK_SEL, 0x01);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_SYSCLK_EN_SEL, 0x37);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_SYS_CLK_CTRL, 0x02);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CLK_ENABLE1, 0x0e);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_SYSCLK_BUF_ENABLE, 0x06);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CLK_SEL, 0x30);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CMN_CONFIG, 0x02);
All these lines could be much shorter with a local variable, 'base',
that got assigned once at the start and then used throughout.
> +
> + /* Different for each clock rates */
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_HSCLK_SEL, dp_res->hsclk_sel);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_DEC_START_MODE0, dp_res->dec_start_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_DIV_FRAC_START1_MODE0, dp_res->div_frac_start1_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_DIV_FRAC_START2_MODE0, dp_res->div_frac_start2_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_DIV_FRAC_START3_MODE0, dp_res->div_frac_start3_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_INTEGLOOP_GAIN0_MODE0, dp_res->integloop_gain0_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_INTEGLOOP_GAIN1_MODE0, dp_res->integloop_gain1_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_VCO_TUNE_MAP, dp_res->vco_tune_map);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_LOCK_CMP1_MODE0, dp_res->lock_cmp1_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_LOCK_CMP2_MODE0, dp_res->lock_cmp2_mode0);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_LOCK_CMP3_MODE0, dp_res->lock_cmp3_mode0);
> + /* Make sure the PLL register writes are done */
> + wmb();
Is this to make sure that the next register writes happen after the
above register writes? The comment could say that instead, but then
realize that they may be sitting somewhere in the bus waiting to write
to the device still, so if it really has to be written to the device we
need to read back the last register written to be sure that the write
posted to the device.
> +
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_BG_TIMER, 0x0a);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CORECLK_DIV_MODE0, 0x0a);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_VCO_TUNE_CTRL, 0x00);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x3f);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CORE_CLK_EN, 0x1f);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_PLL_IVCO, 0x07);
> + PLL_REG_W(dp_res->pll_base,
> + QSERDES_COM_LOCK_CMP_EN, dp_res->lock_cmp_en);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_PLL_CCTRL_MODE0, 0x36);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_PLL_RCTRL_MODE0, 0x16);
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_CP_CTRL_MODE0, 0x06);
> + /* Make sure the PHY register writes are done */
> + wmb();
> +
> + if (dp_res->orientation == ORIENTATION_CC2)
> + PLL_REG_W(dp_res->phy_base, DP_PHY_MODE, 0x4c);
> + else
> + PLL_REG_W(dp_res->phy_base, DP_PHY_MODE, 0x5c);
> + /* Make sure the PLL register writes are done */
> + wmb();
> +
> + /* TX Lane configuration */
> + PLL_REG_W(dp_res->phy_base,
> + DP_PHY_TX0_TX1_LANE_CTL, 0x05);
> + PLL_REG_W(dp_res->phy_base,
> + DP_PHY_TX2_TX3_LANE_CTL, 0x05);
> +
> + /* TX-0 register configuration */
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TRANSCEIVER_BIAS_EN, 0x1a);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_VMODE_CTRL1, 0x40);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_PRE_STALL_LDO_BOOST_EN, 0x30);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_INTERFACE_SELECT, 0x3d);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_CLKBUF_ENABLE, 0x0f);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_RESET_TSYNC_EN, 0x03);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TRAN_DRVR_EMP_EN, 0x03);
> + PLL_REG_W(dp_res->ln_tx0_base,
> + TXn_PARRATE_REC_DETECT_IDLE_EN, 0x00);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TX_INTERFACE_MODE, 0x00);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TX_BAND, 0x4);
> +
> + /* TX-1 register configuration */
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TRANSCEIVER_BIAS_EN, 0x1a);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_VMODE_CTRL1, 0x40);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_PRE_STALL_LDO_BOOST_EN, 0x30);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_INTERFACE_SELECT, 0x3d);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_CLKBUF_ENABLE, 0x0f);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_RESET_TSYNC_EN, 0x03);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TRAN_DRVR_EMP_EN, 0x03);
> + PLL_REG_W(dp_res->ln_tx1_base,
> + TXn_PARRATE_REC_DETECT_IDLE_EN, 0x00);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TX_INTERFACE_MODE, 0x00);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TX_BAND, 0x4);
> + /* Make sure the PHY register writes are done */
> + wmb();
> +
> + /* dependent on the vco frequency */
> + PLL_REG_W(dp_res->phy_base, DP_PHY_VCO_DIV, dp_res->phy_vco_div);
> +
> + return res;
> +}
> +
> +static bool dp_10nm_pll_lock_status(struct dp_pll_10nm *dp_res)
> +{
> + u32 status;
> + bool pll_locked;
> +
> + /* poll for PLL lock status */
> + if (readl_poll_timeout_atomic((dp_res->pll_base +
Why atomic? As far as I can tell this is done in sleepable context
because it all falls into clk_prepare() path.
> + QSERDES_COM_C_READY_STATUS),
> + status,
> + ((status & BIT(0)) > 0),
> + DP_PHY_PLL_POLL_SLEEP_US,
> + DP_PHY_PLL_POLL_TIMEOUT_US)) {
> + pr_err("%s: C_READY status is not high. Status=%x\n",
> + __func__, status);
> + pll_locked = false;
Just 'return false'?
> + } else {
> + pll_locked = true;
> + }
> +
> + return pll_locked;
And reduce this to 'return true'?
> +}
> +
> +static bool dp_10nm_phy_rdy_status(struct dp_pll_10nm *dp_res)
> +{
> + u32 status;
> + bool phy_ready = true;
> +
> + /* poll for PHY ready status */
> + if (readl_poll_timeout_atomic((dp_res->phy_base +
No need for atomic?
> + DP_PHY_STATUS),
> + status,
> + ((status & (BIT(1))) > 0),
> + DP_PHY_PLL_POLL_SLEEP_US,
> + DP_PHY_PLL_POLL_TIMEOUT_US)) {
> + pr_err("%s: Phy_ready is not high. Status=%x\n",
> + __func__, status);
> + phy_ready = false;
> + }
> +
> + return phy_ready;
> +}
> +
> +static int dp_pll_enable_10nm(struct clk_hw *hw)
> +{
> + int rc = 0;
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> + u32 bias_en, drvr_en;
> +
> + PLL_REG_W(dp_res->phy_base, DP_PHY_AUX_CFG2, 0x04);
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x01);
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x05);
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x01);
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x09);
> + wmb(); /* Make sure the PHY register writes are done */
> +
> + PLL_REG_W(dp_res->pll_base, QSERDES_COM_RESETSM_CNTRL, 0x20);
> + wmb(); /* Make sure the PLL register writes are done */
> +
> + if (!dp_10nm_pll_lock_status(dp_res)) {
> + rc = -EINVAL;
> + goto lock_err;
> + }
> +
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x19);
> + /* Make sure the PHY register writes are done */
> + wmb();
> + /* poll for PHY ready status */
> + if (!dp_10nm_phy_rdy_status(dp_res)) {
> + rc = -EINVAL;
> + goto lock_err;
> + }
> +
> + pr_debug("%s: PLL is locked\n", __func__);
> +
> + if (dp_res->lane_cnt == 1) {
> + bias_en = 0x3e;
> + drvr_en = 0x13;
> + } else {
> + bias_en = 0x3f;
> + drvr_en = 0x10;
> + }
> +
> + if (dp_res->lane_cnt != 4) {
> + if (dp_res->orientation == ORIENTATION_CC1) {
> + PLL_REG_W(dp_res->ln_tx1_base,
> + TXn_HIGHZ_DRVR_EN, drvr_en);
> + PLL_REG_W(dp_res->ln_tx1_base,
> + TXn_TRANSCEIVER_BIAS_EN, bias_en);
> + } else {
> + PLL_REG_W(dp_res->ln_tx0_base,
> + TXn_HIGHZ_DRVR_EN, drvr_en);
> + PLL_REG_W(dp_res->ln_tx0_base,
> + TXn_TRANSCEIVER_BIAS_EN, bias_en);
> + }
> + } else {
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_HIGHZ_DRVR_EN, drvr_en);
> + PLL_REG_W(dp_res->ln_tx0_base,
> + TXn_TRANSCEIVER_BIAS_EN, bias_en);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_HIGHZ_DRVR_EN, drvr_en);
> + PLL_REG_W(dp_res->ln_tx1_base,
> + TXn_TRANSCEIVER_BIAS_EN, bias_en);
> + }
> +
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TX_POL_INV, 0x0a);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TX_POL_INV, 0x0a);
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x18);
> + udelay(2000);
What are we waiting for? You need an explicit readl() if you're waiting
for that DP_PHY_CFG write to post and do something.
> +
> + PLL_REG_W(dp_res->phy_base, DP_PHY_CFG, 0x19);
> +
> + /*
> + * Make sure all the register writes are completed before
> + * doing any other operation
> + */
> + wmb();
> +
> + /* poll for PHY ready status */
> + if (!dp_10nm_phy_rdy_status(dp_res)) {
> + rc = -EINVAL;
> + goto lock_err;
> + }
> +
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TX_DRV_LVL, 0x38);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TX_DRV_LVL, 0x38);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_TX_EMP_POST1_LVL, 0x20);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_TX_EMP_POST1_LVL, 0x20);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_RES_CODE_LANE_OFFSET_TX, 0x06);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_RES_CODE_LANE_OFFSET_TX, 0x06);
> + PLL_REG_W(dp_res->ln_tx0_base, TXn_RES_CODE_LANE_OFFSET_RX, 0x07);
> + PLL_REG_W(dp_res->ln_tx1_base, TXn_RES_CODE_LANE_OFFSET_RX, 0x07);
> + /* Make sure the PHY register writes are done */
> + wmb();
> +
> +lock_err:
> + return rc;
> +}
> +
> +static int dp_pll_disable_10nm(struct clk_hw *hw)
> +{
> + int rc = 0;
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> +
> + /* Assert DP PHY power down */
> + PLL_REG_W(dp_res->phy_base, DP_PHY_PD_CTL, 0x2);
> + /*
> + * Make sure all the register writes to disable PLL are
> + * completed before doing any other operation
> + */
> + wmb();
> +
> + return rc;
> +}
> +
> +
> +int dp_vco_prepare_10nm(struct clk_hw *hw)
> +{
> + int rc = 0;
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> +
> + pr_debug("rate=%ld\n", pll->rate);
> + if ((dp_res->vco_cached_rate != 0)
> + && (dp_res->vco_cached_rate == pll->rate)) {
> + rc = pll->clk_hw.init->ops->set_rate(hw,
> + dp_res->vco_cached_rate, dp_res->vco_cached_rate);
> + if (rc) {
> + pr_err("index=%d vco_set_rate failed. rc=%d\n",
> + rc, dp_res->index);
> + goto error;
Please just return instead of goto return.
> + }
> + }
> +
> + rc = dp_pll_enable_10nm(hw);
> + if (rc) {
> + pr_err("ndx=%d failed to enable dp pll\n",
> + dp_res->index);
> + goto error;
> + }
> +
> + pll->pll_on = true;
> +error:
> + return rc;
> +}
> +
> +void dp_vco_unprepare_10nm(struct clk_hw *hw)
> +{
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> +
> + if (!dp_res) {
> + pr_err("Invalid input parameter\n");
> + return;
> + }
> +
> + if (!pll->pll_on) {
> + pr_err("pll resource can't be enabled\n");
> + return;
> + }
> + dp_res->vco_cached_rate = pll->rate;
Why are we caching rates on unprepare? I'd think we would want to save
away the rate regardless of on/off state and then restore the rate when
the device is powered back on. That wouldn't necessarily follow the on/off state.
> + dp_pll_disable_10nm(hw);
> +
> + //dp_res->handoff_resources = false;
Please remove commented out code.
> + pll->pll_on = false;
> +}
> +
> +int dp_vco_set_rate_10nm(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + int rc;
> +
> + pr_debug("DP lane CLK rate=%ld\n", rate);
> +
> + rc = dp_config_vco_rate_10nm(pll, rate);
> + if (rc)
> + pr_err("%s: Failed to set clk rate\n", __func__);
> +
> + pll->rate = rate;
> +
> + return 0;
> +}
> +
> +unsigned long dp_vco_recalc_rate_10nm(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> + struct dp_pll_10nm *dp_res = to_dp_pll_10nm(pll);
> + u32 div, hsclk_div, link_clk_div = 0;
> + u64 vco_rate;
> +
> + div = PLL_REG_R(dp_res->pll_base, QSERDES_COM_HSCLK_SEL);
> + div &= 0x0f;
> +
> + if (div == 12)
> + hsclk_div = 6; /* Default */
> + else if (div == 4)
> + hsclk_div = 4;
> + else if (div == 0)
> + hsclk_div = 2;
> + else if (div == 3)
> + hsclk_div = 1;
> + else {
> + pr_debug("unknown divider. forcing to default\n");
> + hsclk_div = 5;
> + }
> +
> + div = PLL_REG_R(dp_res->phy_base, DP_PHY_AUX_CFG2);
> + div >>= 2;
> +
> + if ((div & 0x3) == 0)
> + link_clk_div = 5;
> + else if ((div & 0x3) == 1)
> + link_clk_div = 10;
> + else if ((div & 0x3) == 2)
> + link_clk_div = 20;
> + else
> + pr_err("%s: unsupported div. Phy_mode: %d\n", __func__, div);
> +
> + if (link_clk_div == 20) {
> + vco_rate = DP_VCO_HSCLK_RATE_2700MHZDIV1000;
> + } else {
> + if (hsclk_div == 6)
> + vco_rate = DP_VCO_HSCLK_RATE_1620MHZDIV1000;
> + else if (hsclk_div == 4)
> + vco_rate = DP_VCO_HSCLK_RATE_2700MHZDIV1000;
> + else if (hsclk_div == 2)
> + vco_rate = DP_VCO_HSCLK_RATE_5400MHZDIV1000;
> + else
> + vco_rate = DP_VCO_HSCLK_RATE_8100MHZDIV1000;
> + }
> +
> + pr_debug("returning vco rate = %lu\n", (unsigned long)vco_rate);
> +
> + dp_res->vco_cached_rate = pll->rate = vco_rate;
> + return (unsigned long)vco_rate;
> +}
> +
> +long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
Implement determine_rate op instead. It lets you clamp rates better.
> +{
> + unsigned long rrate = rate;
> + struct msm_dp_pll *pll = hw_clk_to_pll(hw);
> +
> + if (rate <= pll->min_rate)
> + rrate = pll->min_rate;
> + else if (rate <= DP_VCO_HSCLK_RATE_2700MHZDIV1000)
> + rrate = DP_VCO_HSCLK_RATE_2700MHZDIV1000;
> + else if (rate <= DP_VCO_HSCLK_RATE_5400MHZDIV1000)
> + rrate = DP_VCO_HSCLK_RATE_5400MHZDIV1000;
> + else
> + rrate = pll->max_rate;
> +
> + pr_debug("%s: rrate=%ld\n", __func__, rrate);
> +
> + *parent_rate = rrate;
> + return rrate;
This whole function looks not very useful though. Just pass up the rate
to the parent and have it do the rounding?
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
