[DPU PATCH v2 3/3] drm/msm/dp: add support for DP PLL driver
chandanu at codeaurora.org
chandanu at codeaurora.org
Tue Feb 26 00:15:31 UTC 2019
Hello Sam,
Only DP core driver comments have been addressed in V2.
PLL driver comments in V1 and your comments in V2 version will be
addressed in V3.
Responded to your comments/queries below.
On 2019-01-07 14:14, Sam Ravnborg wrote:
> 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'.
No, we don't have support for modular driver.
>
>> --- 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.
>
Will fix this in V3.
> Or even better - descend into msm/dp/pll to build it - this is normal
> kernel style.
>
I am following the existing format in drm/msm folder.
>> + 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.
>
Will address this in V3.
>> +
>> + 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?
>
Will fix this in V3.
>> 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?
>
Will remove this in V3.
>> 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.
>
Will fix this in V3.
>> +}
>> +
>> +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.
>
Will remove the ifdefs in V3.
>> +
>> +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?
>
Yes, you are right. Will remove this unnecessary check in v3.
>
>> +
>> + 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
>
Will fix this in V3.
>
>> +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?
>
Will fix this in V3.
>
>> +#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?
>
Only DP core driver comments have been addressed in V2. PLL driver
comments will be fixed in V3.
>> +
>> +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.
>
Will fix them in V3.
>> +
>> +#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.
>
>
Will be addressed in V3.
>> + *
>> + */
>> +
>> +#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.
>
Will fix this in V3.
>> +/* Op structures */
> Op => Ops?
>
Will address this in V3.
>> +}
>> +
>> +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?
>
Will fix this in V3.
>> +
>> + 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.
Will replace DBG with DRM_DEBUG_DP in V3.
Thanks
Chandan
More information about the dri-devel
mailing list