On Wed, Oct 10, 2018 at 10:15:59AM -0700, Chandan Uddaraju wrote:
> Add the needed DP PLL specific files to support
> display port interface on msm targets.
>
> The DP driver calls the DP PLL driver registration.
> The DP driver sets the link and pixel clock sources.
>
> Signed-off-by: Chandan Uddaraju <chandanu@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/Kconfig | 16 +
> drivers/gpu/drm/msm/Makefile | 6 +
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 50 +++
> drivers/gpu/drm/msm/dp/dp_display.h | 3 +
> drivers/gpu/drm/msm/dp/dp_parser.h | 3 +
> drivers/gpu/drm/msm/dp/dp_power.c | 77 +++-
> drivers/gpu/drm/msm/dp/dp_power.h | 2 +
> drivers/gpu/drm/msm/dp/pll/dp_pll.c | 153 ++++++++
> drivers/gpu/drm/msm/dp/pll/dp_pll.h | 64 ++++
> drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c | 401 +++++++++++++++++++
> drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h | 94 +++++
> drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c | 531 ++++++++++++++++++++++++++
> 13 files changed, 1389 insertions(+), 12 deletions(-)
> create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.c
> create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll.h
> create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.c
> create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm.h
> create mode 100644 drivers/gpu/drm/msm/dp/pll/dp_pll_10nm_util.c
>
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index c363f24..1e0b9158 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -58,6 +58,22 @@ config DRM_MSM_DP
> driver. DP external display support is enabled
> through this config option. It can be primary or
> secondary display on device.
> +
> +config DRM_MSM_DP_PLL
> + bool "Enable DP PLL driver in MSM DRM"
> + depends on DRM_MSM_DP && COMMON_CLK
> + default y
The default should be n
> + help
> + Choose this option to enable DP PLL driver which provides DP
> + source clocks under common clock framework.
> +
> +config DRM_MSM_DP_10NM_PLL
> + bool "Enable DP 10nm PLL driver in MSM DRM (used by SDM845)"
> + depends on DRM_MSM_DP
Should this be DRM_MSM_DP_PLL instead?
> + default y
The default should be n
> + help
> + Choose this option if DP PLL on SDM845 is used on the platform.
> +
> config DRM_MSM_DSI
> bool "Enable DSI support in MSM DRM driver"
> depends on DRM_MSM
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 765a8d8..8d18353 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -137,4 +137,10 @@ msm-$(CONFIG_DRM_MSM_DSI_14NM_PHY) += dsi/pll/dsi_pll_14nm.o
> msm-$(CONFIG_DRM_MSM_DSI_10NM_PHY) += dsi/pll/dsi_pll_10nm.o
> endif
>
> +ifeq ($(CONFIG_DRM_MSM_DP_PLL),y)
> +msm-y += dp/pll/dp_pll.o
> +msm-y += dp/pll/dp_pll_10nm.o
> +msm-y += dp/pll/dp_pll_10nm_util.o
Instead of the ifeq, these should follow the form:
msm-$(CONFIG_BLAH) +=
> +endif
> +
> obj-$(CONFIG_DRM_MSM) += msm.o
> 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;
> + }
Can we please remove the unnecessary null checks from this patch too?
> +
> + 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);
What if the pll driver goes away before the dp driver?
> +
> + of_node_put(pll_node);
> +
> + if (!pll_pdev || !dp_parser->pll) {
> + dev_err(&pdev->dev, "%s: pll driver is not ready\n", __func__);
This patch (and even just this function) has both pr_err and dev_err, oy!
Please convert everything to one logging medium. The msm driver was recently
converted to DRM_DEV_*, so let's go with that for all of msm/dp/*
> + return -EPROBE_DEFER;
> + }
> +
> + dp_parser->pll_dev = get_device(&pll_pdev->dev);
> +
> + 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");
Print rc, here and everywhere else.
> + 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);
> +
> #endif /* _DP_DISPLAY_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index b39ea02..372997e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -16,6 +16,7 @@
> #define _DP_PARSER_H_
>
> #include "dpu_io_util.h"
> +#include "pll/dp_pll.h"
>
> #define DP_LABEL "MDSS DP DISPLAY"
> #define AUX_CFG_LEN 10
> @@ -175,6 +176,8 @@ struct dp_parser {
> struct dp_pinctrl pinctrl;
> struct dp_io io;
> struct dp_display_data disp_data;
> + struct msm_dp_pll *pll;
> + struct device *pll_dev;
Store pll_dev in msm_dp_pll as 'dev' and remove this?
>
> u8 l_map[4];
> struct dp_aux_cfg aux_cfg[AUX_CFG_LEN];
> 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);
Hopefully this gets simplified when you de-abstract the rest of the dp driver.
> + 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)) {
I think I commented on this in the other patch, but don't use strings to do
lookups, please just store the pointer directly if you need a reference.
> + num--;
> + cfg++;
> + }
> +
> + if (num && power->link_provider) {
> + power->link_clk_src = clk_get_parent(cfg->clk);
Check return for ERR_PTR
> + if (power->link_clk_src) {
Indenting is wrong on this block
> + 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;
Make thi:
if (!power->link_clk_src) {
DRM_DEV_ERROR(...)
return -EINVAL;
}
ret = clk_set_parent(power->link_clk_src, power->link_provider);
if (ret) {
/* propertly handle error */
}
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("%s clock could not be set parent\n", name);
> + rc = -EINVAL;
> + }
Same thing here, flip the if condition to check for error and return early, thus
saving yourself a level of indentation for the successful case.
> +exit:
> + return rc;
Remove exit label and return directly above
> +}
> +
> static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
> {
> int rc = 0;
> @@ -472,8 +520,12 @@ static int dp_power_set_pixel_clk_parent(struct dp_power *dp_power)
>
> power = container_of(dp_power, struct dp_power_private, dp_power);
>
> - if (power->pixel_clk_rcg && power->pixel_parent)
> - clk_set_parent(power->pixel_clk_rcg, power->pixel_parent);
> + if (power->pixel_clk_rcg && power->pixel_provider) {
> + clk_set_parent(power->pixel_clk_rcg, power->pixel_provider);
s/pixel_parent/pixel_provider/ isn't related to this change, can you please use
the final name in the main dp patch so it's correct in this one?
> + pr_debug("%s: is the parent of clk=%s\n",
> + __clk_get_name(power->pixel_provider),
> + __clk_get_name(power->pixel_clk_rcg));
Same comment here, this debug can go in the main patch.
> + }
> exit:
> return rc;
> }
> @@ -577,6 +629,7 @@ struct dp_power *dp_power_get(struct dp_parser *parser)
> dp_power->init = dp_power_init;
> dp_power->deinit = dp_power_deinit;
> dp_power->clk_enable = dp_power_clk_enable;
> + dp_power->set_link_clk_parent = dp_power_set_link_clk_parent;
Same comment here regarding the unnecessary indirection, just call this thing
directly where applicable.
> dp_power->set_pixel_clk_parent = dp_power_set_pixel_clk_parent;
> dp_power->power_client_init = dp_power_client_init;
> dp_power->power_client_deinit = dp_power_client_deinit;
> 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);
> 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.
> + *
> + */
SPDX please (and elsewhere).
> +
> +#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;
> + }
> +
> +clk_err:
remove clk_err and return directly above
> + return rc;
> +}
> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
static please
> + 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;
This should be set in the type-specific init function (ie:
msm_dp_pll_10nm_init()). That will let you simplify this entire function to:
{
switch (type) {
case MSM_DP_PLL_10NM:
return msm_dp_pll_10nm_init(pdev, id);
default:
DRM_DEV_ERROR(&pdev->dev, "Invalid pll type: %d\n", type);
return ERR_PTR(-ENXIO);
}
}
> +
> + 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
I don't think you need this ifdef check, you've already provided a stub for the
init function
> + { .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;
This is what of_device_id->data is for, use it to store the type. Can you also
check the rest of the driver and do the same there?
> + else
> + type = MSM_DP_PLL_MAX;
> +
> + pll = msm_dp_pll_init(pdev, type, 0);
> + if (IS_ERR_OR_NULL(pll)) {
> + dev_info(dev,
Why info level?
> + "%s: pll init failed: %ld, need separate pll clk driver\n",
> + __func__, PTR_ERR(pll));
> + return -ENODEV;
> + }
> +
> + 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);
Hmm.
> + pll = NULL;
No need to clear a local variable
> + }
> +
> + platform_set_drvdata(pdev, NULL);
You don't need to clear this either
> +
> + 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);
> +}
> 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>
> +#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))
These macros aren't really useful, tbh. It'd be better to have a function that
takes a msm_dp_pll, offset and data.
> +
> +enum msm_dp_pll_type {
> + MSM_DP_PLL_10NM,
> + MSM_DP_PLL_MAX
> +};
> +
> +struct msm_dp_pll {
> + enum msm_dp_pll_type type;
Storing this doesn't seem to gain you anything. So move the dp_pll_type enum
into msm_dp_pll.c, turf the _MAX and make embed it in the .data element of the
of_device_id structs. Then you can use it when you're initializing and promptly
throw it away.
> + struct clk_hw clk_hw;
> + unsigned long rate; /* current vco rate */
> + u64 min_rate; /* min vco rate */
> + u64 max_rate; /* max vco rate */
> + bool pll_on;
The clk_hw/etc part of this patch should be reviewed by swboyd (cc'd). It's not
really in my wheelhouse, tbh.
> + void *priv;
Was going to comment on using an opaque pointer, but it not used anywhere \o/
Please remove
> + /* 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)
Please make this a type-safe inline function.
> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
> + enum msm_dp_pll_type type, int id);
Remove this and make that func static
> +
> +int msm_dp_pll_util_parse_dt_clock(struct platform_device *pdev,
> + struct msm_dp_pll *pll);
> +
> +#ifdef CONFIG_DRM_MSM_DP_10NM_PLL
> +struct msm_dp_pll *msm_dp_pll_10nm_init(struct platform_device *pdev, int id);
> +#else
> +struct msm_dp_pll *msm_dp_pll_10nm_init(struct platform_device *pdev, int id)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +#endif
> +#endif /* __DP_PLL_H */
> 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 | |
> + * | +----------+-----------+ |
> + * +------------------------------+
> + * |
> + * +------------<---------v------------>----------+
> + * | |
> + * +-----v------------+ |
> + * | dp_link_clk_src | |
> + * | divsel_ten | |
> + * +---------+--------+ |
> + * | |
> + * | |
> + * 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
Some of your vertical lines seem misaligned here, can you please fix this up?
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/clk.h>
> +
> +#include "dp_pll_10nm.h"
> +
> +#define NUM_PROVIDED_CLKS 2
> +
> +#define DP_LINK_CLK_SRC 0
> +#define DP_PIXEL_CLK_SRC 1
Instead of using an array to store the clocks in one place, why not just store a
pointer for each clk? Then you can get rid of these and just use the clk
directly.
> +
> +static struct dp_pll_10nm *dp_pdb;
This isn't used (nor should it be).
> +
> +/* Op structures */
> +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)
typesafe static inline please. Everywhere you use pclksel, you just grab ->pll
from the result and never use pclksel again. So make the function return
dp_pll_10nm * directly and save yourself the local var.
> +
> +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 */
This comment isn't really useful :) Could you please #define all of the
hardcoded values in the patch?
> +
> + 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;
> +
> + PLL_REG_W(dp_res->phy_base,
> + DP_PHY_VCO_DIV, auxclk_div);
> + /* Make sure the PHY registers writes are done */
> + wmb();
Same comments about needing wmb to work around using _relaxed
> + 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;
> +}
> +
> +static int clk_mux_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + int ret = 0;
no need to init this to 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);
What about the return value? All other clk_set_parent calls in this patch are
also unchecked, so please add those as well.
> +
> + 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)
According to the header documentation, clk_get_parent can return ERR_PTR as
well. Same comment applies to other callsites.
> + 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;
> +
> + if (vco->rate == DP_VCO_HSCLK_RATE_8100MHZDIV1000)
> + return (vco->rate / 6);
Unnecessary () here and below
> + 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 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;
Why have a return value when the only place always returns 0? Make it void and
simplify error checking at the callsite.
> +}
> +
> +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" },
> + .num_parents = 3,
> + .name = "dp_vco_divided_clk_src_mux",
> + .flags = CLK_IGNORE_UNUSED,
> + .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;
> + pll_pclksel->hw.init = &pclksel_init;
pclksel_init goes out of scope at the end of the function, yet it is persisted
in pll_pclksel. That should be fixed.
> +
> + 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;
Same comment about scope here
> + 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");
> + 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;
I'm going to leave this chunk for Stephen to comment on, but again I'd rather
not store these clocks as an array and do string lookups on them.
> +
> + 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);
Please remove (or convert to DRM_DEV_DEBUG)
> +
> + 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");
Print the error if pll_base is ERR_PTR, same for below.
> + return ERR_PTR(-ENOMEM);
You should preserve the error if pll_base is an ERR_PTR, same for below.
> + }
> +
> + 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);
If this is truly an error condition, we should return an error. If it's not,
downgrade this to info
> + 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;
> + 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
Isn't MHZDIV1000 just KHZ? Same for below
> +#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
These can go in the c file (and probably the others too)
> +
> +#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
> + * suspend/resume scenario. Cached the vco rate for such plls.
> + */
> + unsigned long vco_cached_rate;
> + u32 cached_cfg0;
> + u32 cached_cfg1;
> + u32 cached_outdiv;
> +
> + uint32_t index;
> +};
> +
> +#define to_dp_pll_10nm(x) container_of(x, struct dp_pll_10nm, base)
typesafe inline pls
> +
> +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>
> +
> +#include "dp_pll.h"
> +#include "dp_pll_10nm.h"
> +#include "dp_extcon.h"
> +
> +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);
> + 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);
> + 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;
Since this is all static, a static const lookup table would probably be more
appropriate.
> + default:
> + return -EINVAL;
Log this please
> + }
> + 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);
> +
> + /* 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();
> +
> + 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 +
> + 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;
> + } else {
> + pll_locked = true;
> + }
> +
> + return pll_locked;
> +}
> +
> +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 +
> + 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);
> + }
I think you could probably simplify this code block with a bit of effort,
especially the top condition.
> +
> + 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);
why udelay?
> +
> + 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;
Remove lock_err and return directly above.
> +}
> +
> +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)) {
if (dp_res->vco_cached_rate && 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;
> + }
> + }
> +
> + 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;
Do you need locking around the prepare/unprepare functions?
> +error:
> + return rc;
Same comment as always
> +}
> +
> +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;
> + dp_pll_disable_10nm(hw);
> +
> + //dp_res->handoff_resources = false;
??
> + 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__);
Should you return early here?
> +
> + 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;
Hopefully this function will become easier to parse with #define'd values
> +}
> +
> +long dp_vco_round_rate_10nm(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + 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;
You're assuming min_rate is < 2.7MHz and max_rate > 5.4 MHz. This is true in the
current code, but could change in the future. Fortunatley min/max_rate are only
used here. So delete them from the struct and use the DP_VCO_HSCLK_RATE_* values
directly here.
> +
> + pr_debug("%s: rrate=%ld\n", __func__, rrate);
> +
> + *parent_rate = rrate;
> + return rrate;
> +}
> +
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
