[Freedreno] [DPU PATCH v2 3/3] drm/msm/dp: add support for DP PLL driver
Sam Ravnborg
sam at ravnborg.org
Mon Jan 7 22:14:02 UTC 2019
Hi Chandan
A few comments in the following.
Mostly nitpicks / style stuff, not a throughly review.
Sam
> +config DRM_MSM_DP_PLL
> + bool "Enable DP PLL driver in MSM DRM"
So DRM_MSM_DP_PLL cannot be 'm'.
> --- 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
> +endif
Please write this in the Kbuild caninical style like this:
msm-$(DRM_MSM_DP_PLL) += dp/pll/dp_pll.o
msm-$(DRM_MSM_DP_PLL) += dp/pll/dp_pll_10nm.o
etc.
Or even better - descend into msm/dp/pll to build it - this is normal kernel style.
> + if (!dp_parser) {
> + DRM_ERROR("Parser not initialized.\n");
> + return -EINVAL;
> + }
> +
> + pll_node = of_parse_phandle(pdev->dev.of_node, "pll-node", 0);
> + if (!pll_node) {
> + DRM_DEV_ERROR(&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) {
> + DRM_DEV_ERROR(&pdev->dev, "%s: pll driver is not ready\n", __func__);
> + return -EPROBE_DEFER;
> + }
The use of DRM_*ERROR is inconsistent.
In one place DRM_ERROR is used, and string ends with '.'
In one place DRM_DEV_ERROR is used with a simple string.
In one place DRM_DEV_ERROR is used where the __func__ is added as parameter.
When reading the code such inconsistencies makes it harder to follow the code.
> +
> + 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;
> @@ -114,6 +156,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> + rc = dp_get_pll(dp);
> + if (rc) {
> + DRM_ERROR(" DP get PLL instance failed\n");
Any reason why the error is indented with a space?
Also, is the DRM*ERROR in dp_get_pll() not enough?
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> index 76e2d3b..40d7e73 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.h
> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> @@ -14,6 +14,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 {
This chunk is unrelated - it just added some missing doc.
Do it belong in another patch?
> 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..28f0e92
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#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) {
> + DRM_ERROR("clocks are not defined\n");
> + goto clk_err;
> + }
You have a pdev->dev, so use DRM_DEV_ERROR()
> +
> +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)) {
> + DRM_DEV_ERROR(dev, "%s: failed to init DP PLL\n", __func__);
> + return pll;
> + }
> +
> + pll->type = type;
> +
> + DBG("DP:%d PLL registered", id);
Avoid rolling your own DEBUG macros.
> +}
> +
> +static const struct of_device_id dp_pll_dt_match[] = {
> +#ifdef CONFIG_DRM_MSM_DP_10NM_PLL
> + { .compatible = "qcom,dp-pll-10nm" },
> +#endif
> + {}
> +};
We only have one entry here.
> +
> +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;
So the if will always be true, because we can only match one compatible - no?
> +
> + pll = msm_dp_pll_init(pdev, type, 0);
> + if (IS_ERR_OR_NULL(pll)) {
> + dev_info(dev,
> + "%s: pll init failed: %ld, need separate pll clk driver\n",
> + __func__, PTR_ERR(pll));
dev_info => DRM_DEV_ERROR
> +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);
Debug artifact?
> +#define PLL_REG_W(base, offset, data) \
> + writel_relaxed((data), (base) + (offset))
> +#define PLL_REG_R(base, offset) readl_relaxed((base) + (offset))
I recall you wrote in the commit message that the _relaxed
variants was dropped?
> +
> +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,
> + struct msm_dp_pll *pll);
Indent of sencond line with additional function arguments
has inconsistent indent.
Follwo same style in all files, preferable the DRM style.
> +
> +#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
I cannot see how this works if CONFIG_DRM_MSM_DP_10NM_PLL is not defined.
It looks like you have both a function in a header (no static inline?)
and a function with the same name in a .c file.
Maybe this driver was not built without the CONFIG_DRM_MSM_DP_10NM_PLL set to y?
> +/*
> + * 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
Nice drawing!
Try to avoid mixing space and tabs in the above.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/clk.h>
Please sort in alphabetic order.
> +/* Op structures */
Op => Ops?
> +}
> +
> +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);
Should you check error code here?
> +
> + return 0;
> +}
> +
> +}
> +
> +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);
Avoid own DBG macros.
> +
> + DBG("DP PLL%d", id);
Again.
More information about the Freedreno
mailing list