mbox series

[RESEND,v3,00/15] drm/msm/dp: clear power and parser submodules away

Message ID 20240126-dp-power-parser-cleanup-v3-0-098d5f581dd3@linaro.org
Headers show
Series drm/msm/dp: clear power and parser submodules away | expand

Message

Dmitry Baryshkov Jan. 26, 2024, 6:26 p.m. UTC 
Reshuffle code in the DP driver, cleaning up clocks and DT parsing and
dropping the dp_power and dp_parser submodules.

Initially I started by looking onto stream_pixel clock handling only to
find several wrapping layers around a single clocks. After inlining
and/or dropping them (and thus dp_power submodule), it was more or less
natural to continue cleaning up the dp_parser until it got removed
completely.

---
Changes in v3:
- Fixed crash in the DP when there is no next bridge (Kuogee)
- Removed excess documentation for the removed dp_parser::io field
- Link to v2: https://lore.kernel.org/r/20231231-dp-power-parser-cleanup-v2-0-fc3e902a6f5b@linaro.org

Changes in v2:
- Fixed unrelated power->ctrl change in comment (Konrad)
- Made sure that all functions use reverse-Christmas-tree flow (Konrad)
- Fixed indents in several moved functions
- Added a patch splitting dp_ctlr_clk_enable
- Link to v1: https://lore.kernel.org/r/20231229225650.912751-1-dmitry.baryshkov@linaro.org

---
Dmitry Baryshkov (15):
      drm/msm/dp: drop unused parser definitions
      drm/msm/dp: drop unused fields from dp_power_private
      drm/msm/dp: parse DT from dp_parser_get
      drm/msm/dp: inline dp_power_(de)init
      drm/msm/dp: fold dp_power into dp_ctrl module
      drm/msm/dp: simplify stream clocks handling
      drm/msm/dp: stop parsing clock names from DT
      drm/msm/dp: split dp_ctrl_clk_enable into four functuions
      drm/msm/dp: move phy_configure_opts to dp_ctrl
      drm/msm/dp: remove PHY handling from dp_catalog.c
      drm/msm/dp: handle PHY directly in dp_ctrl
      drm/msm/dp: move all IO handling to dp_catalog
      drm/msm/dp: move link property handling to dp_panel
      drm/msm/dp: move next_bridge handling to dp_display
      drm/msm/dp: drop dp_parser

 drivers/gpu/drm/msm/Makefile        |   2 -
 drivers/gpu/drm/msm/dp/dp_aux.c     |   9 +-
 drivers/gpu/drm/msm/dp/dp_aux.h     |   2 +
 drivers/gpu/drm/msm/dp/dp_catalog.c | 156 +++++++++++-----
 drivers/gpu/drm/msm/dp/dp_catalog.h |   6 +-
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 358 ++++++++++++++++++++++++------------
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |  17 +-
 drivers/gpu/drm/msm/dp/dp_debug.c   |   1 -
 drivers/gpu/drm/msm/dp/dp_display.c | 102 +++-------
 drivers/gpu/drm/msm/dp/dp_display.h |   3 +-
 drivers/gpu/drm/msm/dp/dp_panel.c   |  66 +++++++
 drivers/gpu/drm/msm/dp/dp_parser.c  | 327 --------------------------------
 drivers/gpu/drm/msm/dp/dp_parser.h  | 155 ----------------
 drivers/gpu/drm/msm/dp/dp_power.c   | 183 ------------------
 drivers/gpu/drm/msm/dp/dp_power.h   |  95 ----------
 15 files changed, 465 insertions(+), 1017 deletions(-)
---
base-commit: 39676dfe52331dba909c617f213fdb21015c8d10
change-id: 20231231-dp-power-parser-cleanup-9e3a5f9a6821

Best regards,

Comments

Kuogee Hsieh Jan. 26, 2024, 10:18 p.m. UTC | #1 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> Drop several unused and obsolete definitions from the dp_parser module.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_parser.h | 46 --------------------------------------
>   1 file changed, 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 1f068626d445..90a2cdbbe344 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -12,7 +12,6 @@
>   
>   #include "msm_drv.h"
>   
> -#define DP_LABEL "MDSS DP DISPLAY"
>   #define DP_MAX_PIXEL_CLK_KHZ	675000
>   #define DP_MAX_NUM_DP_LANES	4
>   #define DP_LINK_RATE_HBR2	540000 /* kbytes */
> @@ -21,7 +20,6 @@ enum dp_pm_type {
>   	DP_CORE_PM,
>   	DP_CTRL_PM,
>   	DP_STREAM_PM,
> -	DP_PHY_PM,
>   	DP_MAX_PM
>   };
>   
> @@ -43,28 +41,10 @@ static inline const char *dp_parser_pm_name(enum dp_pm_type module)
>   	case DP_CORE_PM:	return "DP_CORE_PM";
>   	case DP_CTRL_PM:	return "DP_CTRL_PM";
>   	case DP_STREAM_PM:	return "DP_STREAM_PM";
> -	case DP_PHY_PM:		return "DP_PHY_PM";
>   	default:		return "???";
>   	}
>   }
>   
> -/**
> - * struct dp_display_data  - display related device tree data.
> - *
> - * @ctrl_node: referece to controller device
> - * @phy_node:  reference to phy device
> - * @is_active: is the controller currently active
> - * @name: name of the display
> - * @display_type: type of the display
> - */
> -struct dp_display_data {
> -	struct device_node *ctrl_node;
> -	struct device_node *phy_node;
> -	bool is_active;
> -	const char *name;
> -	const char *display_type;
> -};
> -
>   /**
>    * struct dp_ctrl_resource - controller's IO related data
>    *
> @@ -77,28 +57,6 @@ struct dp_io {
>   	union phy_configure_opts phy_opts;
>   };
>   
> -/**
> - * struct dp_pinctrl - DP's pin control
> - *
> - * @pin: pin-controller's instance
> - * @state_active: active state pin control
> - * @state_hpd_active: hpd active state pin control
> - * @state_suspend: suspend state pin control
> - */
> -struct dp_pinctrl {
> -	struct pinctrl *pin;
> -	struct pinctrl_state *state_active;
> -	struct pinctrl_state *state_hpd_active;
> -	struct pinctrl_state *state_suspend;
> -};
> -
> -/* Regulators for DP devices */
> -struct dp_reg_entry {
> -	char name[32];
> -	int enable_load;
> -	int disable_load;
> -};
> -
>   struct dss_module_power {
>   	unsigned int num_clk;
>   	struct clk_bulk_data *clocks;
> @@ -109,16 +67,12 @@ struct dss_module_power {
>    *
>    * @pdev: platform data of the client
>    * @mp: gpio, regulator and clock related data
> - * @pinctrl: pin-control related data
> - * @disp_data: controller's display related data
>    * @parse: function to be called by client to parse device tree.
>    */
>   struct dp_parser {
>   	struct platform_device *pdev;
>   	struct dss_module_power mp[DP_MAX_PM];
> -	struct dp_pinctrl pinctrl;
>   	struct dp_io io;
> -	struct dp_display_data disp_data;
>   	u32 max_dp_lanes;
>   	u32 max_dp_link_rate;
>   	struct drm_bridge *next_bridge;
>
Kuogee Hsieh Jan. 26, 2024, 10:21 p.m. UTC | #2 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> All supported platforms use the same clocks configuration. Instead of
> parsing names from DT in a pretty complex manner, use the static
> configuration. If at some point newer (or older) platforms have
> different clock configuration, this clock config can be moved to the
> device data.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c   |  73 ++++++++++++++++++------
>   drivers/gpu/drm/msm/dp/dp_ctrl.h   |   6 ++
>   drivers/gpu/drm/msm/dp/dp_parser.c | 112 -------------------------------------
>   drivers/gpu/drm/msm/dp/dp_parser.h |  22 --------
>   4 files changed, 63 insertions(+), 150 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 56a424a82a1b..cfcf6136ffa6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -69,6 +69,11 @@ struct dp_vc_tu_mapping_table {
>   	u8 tu_size_minus1;
>   };
>   
> +struct dss_module_power {
> +	unsigned int num_clk;
> +	struct clk_bulk_data *clocks;
> +};
> +
>   struct dp_ctrl_private {
>   	struct dp_ctrl dp_ctrl;
>   	struct drm_device *drm_dev;
> @@ -79,6 +84,7 @@ struct dp_ctrl_private {
>   	struct dp_parser *parser;
>   	struct dp_catalog *catalog;
>   
> +	struct dss_module_power mp[DP_MAX_PM];
>   	struct clk *pixel_clk;
>   
>   	struct completion idle_comp;
> @@ -90,6 +96,15 @@ struct dp_ctrl_private {
>   	bool stream_clks_on;
>   };
>   
> +static inline const char *dp_pm_name(enum dp_pm_type module)
> +{
> +	switch (module) {
> +	case DP_CORE_PM:	return "DP_CORE_PM";
> +	case DP_CTRL_PM:	return "DP_CTRL_PM";
> +	default:		return "???";
> +	}
> +}
> +
>   static int dp_aux_link_configure(struct drm_dp_aux *aux,
>   					struct dp_link_info *link)
>   {
> @@ -1334,7 +1349,7 @@ int dp_ctrl_clk_enable(struct dp_ctrl *dp_ctrl,
>   	if (pm_type != DP_CORE_PM &&
>   	    pm_type != DP_CTRL_PM) {
>   		DRM_ERROR("unsupported ctrl module: %s\n",
> -			  dp_parser_pm_name(pm_type));
> +			  dp_pm_name(pm_type));
>   		return -EINVAL;
>   	}
>   
> @@ -1354,7 +1369,7 @@ int dp_ctrl_clk_enable(struct dp_ctrl *dp_ctrl,
>   		if ((pm_type == DP_CTRL_PM) && (!ctrl->core_clks_on)) {
>   			drm_dbg_dp(ctrl->drm_dev,
>   				   "Enable core clks before link clks\n");
> -			mp = &ctrl->parser->mp[DP_CORE_PM];
> +			mp = &ctrl->mp[DP_CORE_PM];
>   
>   			ret = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
>   			if (ret)
> @@ -1364,7 +1379,7 @@ int dp_ctrl_clk_enable(struct dp_ctrl *dp_ctrl,
>   		}
>   	}
>   
> -	mp = &ctrl->parser->mp[pm_type];
> +	mp = &ctrl->mp[pm_type];
>   	if (enable) {
>   		ret = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
>   		if (ret)
> @@ -1380,7 +1395,7 @@ int dp_ctrl_clk_enable(struct dp_ctrl *dp_ctrl,
>   
>   	drm_dbg_dp(ctrl->drm_dev, "%s clocks for %s\n",
>   		   enable ? "enable" : "disable",
> -		   dp_parser_pm_name(pm_type));
> +		   dp_pm_name(pm_type));
>   	drm_dbg_dp(ctrl->drm_dev,
>   		   "stream_clks:%s link_clks:%s core_clks:%s\n",
>   		   ctrl->stream_clks_on ? "on" : "off",
> @@ -2158,30 +2173,56 @@ irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>   	return ret;
>   }
>   
> +static const char *core_clks[] = {
> +	"core_iface",
> +	"core_aux",
> +};
> +
> +static const char *ctrl_clks[] = {
> +	"ctrl_link",
> +	"ctrl_link_iface",
> +};
> +
>   static int dp_ctrl_clk_init(struct dp_ctrl *dp_ctrl)
>   {
> -	struct dp_ctrl_private *ctrl_private;
> -	int rc = 0;
> -	struct dss_module_power *core, *ctrl;
> +	struct dp_ctrl_private *ctrl;
> +	struct dss_module_power *core, *link;
>   	struct device *dev;
> +	int i, rc;
> +
> +	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +	dev = ctrl->dev;
>   
> -	ctrl_private = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dev = ctrl_private->dev;
> +	core = &ctrl->mp[DP_CORE_PM];
> +	link = &ctrl->mp[DP_CTRL_PM];
>   
> -	core = &ctrl_private->parser->mp[DP_CORE_PM];
> -	ctrl = &ctrl_private->parser->mp[DP_CTRL_PM];
> +	core->num_clk = ARRAY_SIZE(core_clks);
> +	core->clocks = devm_kcalloc(dev, core->num_clk, sizeof(*core->clocks), GFP_KERNEL);
> +	if (!core->clocks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < core->num_clk; i++)
> +		core->clocks[i].id = core_clks[i];
>   
>   	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
>   	if (rc)
>   		return rc;
>   
> -	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
> +	link->num_clk = ARRAY_SIZE(ctrl_clks);
> +	link->clocks = devm_kcalloc(dev, link->num_clk, sizeof(*link->clocks), GFP_KERNEL);
> +	if (!link->clocks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < link->num_clk; i++)
> +		link->clocks[i].id = ctrl_clks[i];
> +
> +	rc = devm_clk_bulk_get(dev, link->num_clk, link->clocks);
>   	if (rc)
> -		return -ENODEV;
> +		return rc;
>   
> -	ctrl_private->pixel_clk = devm_clk_get(dev, "stream_pixel");
> -	if (IS_ERR(ctrl_private->pixel_clk))
> -		return PTR_ERR(ctrl_private->pixel_clk);
> +	ctrl->pixel_clk = devm_clk_get(dev, "stream_pixel");
> +	if (IS_ERR(ctrl->pixel_clk))
> +		return PTR_ERR(ctrl->pixel_clk);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 85da5a7e5307..d8007a9d8260 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -17,6 +17,12 @@ struct dp_ctrl {
>   	bool wide_bus_en;
>   };
>   
> +enum dp_pm_type {
> +	DP_CORE_PM,
> +	DP_CTRL_PM,
> +	DP_MAX_PM
> +};
> +
>   int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
>   int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index fe2b75f7555a..de7cfc340f0c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -141,114 +141,6 @@ static int dp_parser_misc(struct dp_parser *parser)
>   	return 0;
>   }
>   
> -static inline bool dp_parser_check_prefix(const char *clk_prefix,
> -						const char *clk_name)
> -{
> -	return !strncmp(clk_prefix, clk_name, strlen(clk_prefix));
> -}
> -
> -static int dp_parser_init_clk_data(struct dp_parser *parser)
> -{
> -	int num_clk, i, rc;
> -	int core_clk_count = 0, ctrl_clk_count = 0;
> -	const char *clk_name;
> -	struct device *dev = &parser->pdev->dev;
> -	struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> -	struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> -
> -	num_clk = of_property_count_strings(dev->of_node, "clock-names");
> -	if (num_clk <= 0) {
> -		DRM_ERROR("no clocks are defined\n");
> -		return -EINVAL;
> -	}
> -
> -	for (i = 0; i < num_clk; i++) {
> -		rc = of_property_read_string_index(dev->of_node,
> -				"clock-names", i, &clk_name);
> -		if (rc < 0)
> -			return rc;
> -
> -		if (dp_parser_check_prefix("core", clk_name))
> -			core_clk_count++;
> -
> -		if (dp_parser_check_prefix("ctrl", clk_name))
> -			ctrl_clk_count++;
> -	}
> -
> -	/* Initialize the CORE power module */
> -	if (core_clk_count == 0) {
> -		DRM_ERROR("no core clocks are defined\n");
> -		return -EINVAL;
> -	}
> -
> -	core_power->num_clk = core_clk_count;
> -	core_power->clocks = devm_kcalloc(dev,
> -			core_power->num_clk, sizeof(struct clk_bulk_data),
> -			GFP_KERNEL);
> -	if (!core_power->clocks)
> -		return -ENOMEM;
> -
> -	/* Initialize the CTRL power module */
> -	if (ctrl_clk_count == 0) {
> -		DRM_ERROR("no ctrl clocks are defined\n");
> -		return -EINVAL;
> -	}
> -
> -	ctrl_power->num_clk = ctrl_clk_count;
> -	ctrl_power->clocks = devm_kcalloc(dev,
> -			ctrl_power->num_clk, sizeof(struct clk_bulk_data),
> -			GFP_KERNEL);
> -	if (!ctrl_power->clocks) {
> -		ctrl_power->num_clk = 0;
> -		return -ENOMEM;
> -	}
> -
> -	return num_clk;
> -}
> -
> -static int dp_parser_clock(struct dp_parser *parser)
> -{
> -	int rc = 0, i = 0;
> -	int num_clk = 0;
> -	int core_clk_index = 0, ctrl_clk_index = 0;
> -	int core_clk_count = 0, ctrl_clk_count = 0;
> -	const char *clk_name;
> -	struct device *dev = &parser->pdev->dev;
> -	struct dss_module_power *core_power = &parser->mp[DP_CORE_PM];
> -	struct dss_module_power *ctrl_power = &parser->mp[DP_CTRL_PM];
> -
> -	rc =  dp_parser_init_clk_data(parser);
> -	if (rc < 0) {
> -		DRM_ERROR("failed to initialize power data %d\n", rc);
> -		return rc;
> -	}
> -
> -	num_clk = rc;
> -
> -	core_clk_count = core_power->num_clk;
> -	ctrl_clk_count = ctrl_power->num_clk;
> -
> -	for (i = 0; i < num_clk; i++) {
> -		rc = of_property_read_string_index(dev->of_node, "clock-names",
> -				i, &clk_name);
> -		if (rc) {
> -			DRM_ERROR("error reading clock-names %d\n", rc);
> -			return rc;
> -		}
> -		if (dp_parser_check_prefix("core", clk_name) &&
> -				core_clk_index < core_clk_count) {
> -			core_power->clocks[core_clk_index].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
> -			core_clk_index++;
> -		} else if (dp_parser_check_prefix("ctrl", clk_name) &&
> -			   ctrl_clk_index < ctrl_clk_count) {
> -			ctrl_power->clocks[ctrl_clk_index].id = devm_kstrdup(dev, clk_name, GFP_KERNEL);
> -			ctrl_clk_index++;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>   int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser)
>   {
>   	struct platform_device *pdev = parser->pdev;
> @@ -280,10 +172,6 @@ static int dp_parser_parse(struct dp_parser *parser)
>   	if (rc)
>   		return rc;
>   
> -	rc = dp_parser_clock(parser);
> -	if (rc)
> -		return rc;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index c6fe26602e07..cad82c4d07da 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -16,12 +16,6 @@
>   #define DP_MAX_NUM_DP_LANES	4
>   #define DP_LINK_RATE_HBR2	540000 /* kbytes */
>   
> -enum dp_pm_type {
> -	DP_CORE_PM,
> -	DP_CTRL_PM,
> -	DP_MAX_PM
> -};
> -
>   struct dss_io_region {
>   	size_t len;
>   	void __iomem *base;
> @@ -34,15 +28,6 @@ struct dss_io_data {
>   	struct dss_io_region p0;
>   };
>   
> -static inline const char *dp_parser_pm_name(enum dp_pm_type module)
> -{
> -	switch (module) {
> -	case DP_CORE_PM:	return "DP_CORE_PM";
> -	case DP_CTRL_PM:	return "DP_CTRL_PM";
> -	default:		return "???";
> -	}
> -}
> -
>   /**
>    * struct dp_ctrl_resource - controller's IO related data
>    *
> @@ -55,20 +40,13 @@ struct dp_io {
>   	union phy_configure_opts phy_opts;
>   };
>   
> -struct dss_module_power {
> -	unsigned int num_clk;
> -	struct clk_bulk_data *clocks;
> -};
> -
>   /**
>    * struct dp_parser - DP parser's data exposed to clients
>    *
>    * @pdev: platform data of the client
> - * @mp: gpio, regulator and clock related data
>    */
>   struct dp_parser {
>   	struct platform_device *pdev;
> -	struct dss_module_power mp[DP_MAX_PM];
>   	struct dp_io io;
>   	u32 max_dp_lanes;
>   	u32 max_dp_link_rate;
>
Kuogee Hsieh Jan. 26, 2024, 10:23 p.m. UTC | #3 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> There is little point in sharing phy configuration structure between
> several modules. Move it to dp_ctrl, which becomes the only submodule
> re-configuring the PHY.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_catalog.c | 19 -----------------
>   drivers/gpu/drm/msm/dp/dp_catalog.h |  2 --
>   drivers/gpu/drm/msm/dp/dp_ctrl.c    | 41 ++++++++++++++++++++++++-------------
>   drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ---
>   4 files changed, 27 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 5142aeb705a4..e07651768805 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -765,25 +765,6 @@ void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog)
>   	dp_write_ahb(catalog, REG_DP_PHY_CTRL, 0x0);
>   }
>   
> -int dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog,
> -		u8 v_level, u8 p_level)
> -{
> -	struct dp_catalog_private *catalog = container_of(dp_catalog,
> -				struct dp_catalog_private, dp_catalog);
> -	struct dp_io *dp_io = catalog->io;
> -	struct phy *phy = dp_io->phy;
> -	struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
> -
> -	/* TODO: Update for all lanes instead of just first one */
> -	opts_dp->voltage[0] = v_level;
> -	opts_dp->pre[0] = p_level;
> -	opts_dp->set_voltages = 1;
> -	phy_configure(phy, &dp_io->phy_opts);
> -	opts_dp->set_voltages = 0;
> -
> -	return 0;
> -}
> -
>   void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
>   			u32 pattern)
>   {
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 38786e855b51..ba7c62ba7ca3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -111,8 +111,6 @@ void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
>   u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
>   u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
>   void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog);
> -int dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
> -				u8 p_level);
>   int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
>   u32 dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog);
>   void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e367eb8e5bea..4aea72a2b8e8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -87,6 +87,8 @@ struct dp_ctrl_private {
>   
>   	struct clk *pixel_clk;
>   
> +	union phy_configure_opts phy_opts;
> +
>   	struct completion idle_comp;
>   	struct completion psr_op_comp;
>   	struct completion video_comp;
> @@ -1017,6 +1019,21 @@ static int dp_ctrl_wait4video_ready(struct dp_ctrl_private *ctrl)
>   	return ret;
>   }
>   
> +static int dp_ctrl_set_vx_px(struct dp_ctrl_private *ctrl,
> +			     u8 v_level, u8 p_level)
> +{
> +	union phy_configure_opts *phy_opts = &ctrl->phy_opts;
> +
> +	/* TODO: Update for all lanes instead of just first one */
> +	phy_opts->dp.voltage[0] = v_level;
> +	phy_opts->dp.pre[0] = p_level;
> +	phy_opts->dp.set_voltages = 1;
> +	phy_configure(ctrl->parser->io.phy, phy_opts);
> +	phy_opts->dp.set_voltages = 0;
> +
> +	return 0;
> +}
> +
>   static int dp_ctrl_update_vx_px(struct dp_ctrl_private *ctrl)
>   {
>   	struct dp_link *link = ctrl->link;
> @@ -1029,7 +1046,7 @@ static int dp_ctrl_update_vx_px(struct dp_ctrl_private *ctrl)
>   	drm_dbg_dp(ctrl->drm_dev,
>   		"voltage level: %d emphasis level: %d\n",
>   			voltage_swing_level, pre_emphasis_level);
> -	ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog,
> +	ret = dp_ctrl_set_vx_px(ctrl,
>   		voltage_swing_level, pre_emphasis_level);
>   
>   	if (ret)
> @@ -1425,16 +1442,14 @@ static void dp_ctrl_link_clk_disable(struct dp_ctrl *dp_ctrl)
>   static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>   {
>   	int ret = 0;
> -	struct dp_io *dp_io = &ctrl->parser->io;
> -	struct phy *phy = dp_io->phy;
> -	struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
> +	struct phy *phy = ctrl->parser->io.phy;
>   	const u8 *dpcd = ctrl->panel->dpcd;
>   
> -	opts_dp->lanes = ctrl->link->link_params.num_lanes;
> -	opts_dp->link_rate = ctrl->link->link_params.rate / 100;
> -	opts_dp->ssc = drm_dp_max_downspread(dpcd);
> +	ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes;
> +	ctrl->phy_opts.dp.link_rate = ctrl->link->link_params.rate / 100;
> +	ctrl->phy_opts.dp.ssc = drm_dp_max_downspread(dpcd);
>   
> -	phy_configure(phy, &dp_io->phy_opts);
> +	phy_configure(phy, &ctrl->phy_opts);
>   	phy_power_on(phy);
>   
>   	dev_pm_opp_set_rate(ctrl->dev, ctrl->link->link_params.rate * 1000);
> @@ -1572,14 +1587,12 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
>   
>   static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   {
> +	struct phy *phy = ctrl->parser->io.phy;
>   	int ret = 0;
> -	struct dp_io *dp_io = &ctrl->parser->io;
> -	struct phy *phy = dp_io->phy;
> -	struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
>   
>   	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
> -	opts_dp->lanes = ctrl->link->link_params.num_lanes;
> -	phy_configure(phy, &dp_io->phy_opts);
> +	ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes;
> +	phy_configure(phy, &ctrl->phy_opts);
>   	/*
>   	 * Disable and re-enable the mainlink clock since the
>   	 * link clock might have been adjusted as part of the
> @@ -1659,7 +1672,7 @@ static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
>   
>   	drm_dbg_dp(ctrl->drm_dev, "request: 0x%x\n", pattern_requested);
>   
> -	if (dp_catalog_ctrl_update_vx_px(ctrl->catalog,
> +	if (dp_ctrl_set_vx_px(ctrl,
>   			ctrl->link->phy_params.v_level,
>   			ctrl->link->phy_params.p_level)) {
>   		DRM_ERROR("Failed to set v/p levels\n");
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index cad82c4d07da..b28052e87101 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -7,8 +7,6 @@
>   #define _DP_PARSER_H_
>   
>   #include <linux/platform_device.h>
> -#include <linux/phy/phy.h>
> -#include <linux/phy/phy-dp.h>
>   
>   #include "msm_drv.h"
>   
> @@ -37,7 +35,6 @@ struct dss_io_data {
>   struct dp_io {
>   	struct dss_io_data dp_controller;
>   	struct phy *phy;
> -	union phy_configure_opts phy_opts;
>   };
>   
>   /**
>
Kuogee Hsieh Jan. 26, 2024, 10:23 p.m. UTC | #4 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> There is little point in going trough dp_parser->io indirection each
> time the driver needs to access the PHY. Store the pointer directly in
> dp_ctrl_private.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_ctrl.c    | 37 +++++++++++++------------------------
>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>   drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
>   3 files changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 4aea72a2b8e8..fc7ce315ae41 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -76,9 +76,10 @@ struct dp_ctrl_private {
>   	struct drm_dp_aux *aux;
>   	struct dp_panel *panel;
>   	struct dp_link *link;
> -	struct dp_parser *parser;
>   	struct dp_catalog *catalog;
>   
> +	struct phy *phy;
> +
>   	unsigned int num_core_clks;
>   	struct clk_bulk_data *core_clks;
>   
> @@ -1028,7 +1029,7 @@ static int dp_ctrl_set_vx_px(struct dp_ctrl_private *ctrl,
>   	phy_opts->dp.voltage[0] = v_level;
>   	phy_opts->dp.pre[0] = p_level;
>   	phy_opts->dp.set_voltages = 1;
> -	phy_configure(ctrl->parser->io.phy, phy_opts);
> +	phy_configure(ctrl->phy, phy_opts);
>   	phy_opts->dp.set_voltages = 0;
>   
>   	return 0;
> @@ -1442,7 +1443,7 @@ static void dp_ctrl_link_clk_disable(struct dp_ctrl *dp_ctrl)
>   static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>   {
>   	int ret = 0;
> -	struct phy *phy = ctrl->parser->io.phy;
> +	struct phy *phy = ctrl->phy;
>   	const u8 *dpcd = ctrl->panel->dpcd;
>   
>   	ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes;
> @@ -1540,12 +1541,10 @@ void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter)
>   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	dp_catalog_ctrl_phy_reset(ctrl->catalog);
>   	phy_init(phy);
> @@ -1557,12 +1556,10 @@ void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>   void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	dp_catalog_ctrl_phy_reset(ctrl->catalog);
>   	phy_exit(phy);
> @@ -1587,7 +1584,7 @@ static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
>   
>   static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   {
> -	struct phy *phy = ctrl->parser->io.phy;
> +	struct phy *phy = ctrl->phy;
>   	int ret = 0;
>   
>   	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
> @@ -1617,11 +1614,9 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   
>   static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
>   {
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>   
> @@ -2047,12 +2042,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>   void dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	/* set dongle to D3 (power off) mode */
>   	dp_link_psm_config(ctrl->link, &ctrl->panel->link_info, true);
> @@ -2080,12 +2073,10 @@ void dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>   void dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>   
> @@ -2103,12 +2094,10 @@ void dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
>   void dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
> -	struct dp_io *dp_io;
>   	struct phy *phy;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> -	dp_io = &ctrl->parser->io;
> -	phy = dp_io->phy;
> +	phy = ctrl->phy;
>   
>   	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>   
> @@ -2225,7 +2214,7 @@ static int dp_ctrl_clk_init(struct dp_ctrl *dp_ctrl)
>   struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>   			struct dp_panel *panel,	struct drm_dp_aux *aux,
>   			struct dp_catalog *catalog,
> -			struct dp_parser *parser)
> +			struct phy *phy)
>   {
>   	struct dp_ctrl_private *ctrl;
>   	int ret;
> @@ -2259,12 +2248,12 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>   	init_completion(&ctrl->video_comp);
>   
>   	/* in parameters */
> -	ctrl->parser   = parser;
>   	ctrl->panel    = panel;
>   	ctrl->aux      = aux;
>   	ctrl->link     = link;
>   	ctrl->catalog  = catalog;
>   	ctrl->dev      = dev;
> +	ctrl->phy      = phy;
>   
>   	ret = dp_ctrl_clk_init(&ctrl->dp_ctrl);
>   	if (ret) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 023f14d0b021..6e9f375b856a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -28,7 +28,7 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl);
>   struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>   			struct dp_panel *panel,	struct drm_dp_aux *aux,
>   			struct dp_catalog *catalog,
> -			struct dp_parser *parser);
> +			struct phy *phy);
>   
>   void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable);
>   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c1a51c498e01..b8388e04bd0f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -760,7 +760,8 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   	}
>   
>   	dp->ctrl = dp_ctrl_get(dev, dp->link, dp->panel, dp->aux,
> -			       dp->catalog, dp->parser);
> +			       dp->catalog,
> +			       dp->parser->io.phy);
>   	if (IS_ERR(dp->ctrl)) {
>   		rc = PTR_ERR(dp->ctrl);
>   		DRM_ERROR("failed to initialize ctrl, rc = %d\n", rc);
>
Kuogee Hsieh Jan. 26, 2024, 10:24 p.m. UTC | #5 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> Instead of passing link properties through the separate struct, parse
> them directly in the dp_panel.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c |  8 -----
>   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
>   drivers/gpu/drm/msm/dp/dp_panel.c   | 66 +++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/dp/dp_parser.c  | 54 ------------------------------
>   drivers/gpu/drm/msm/dp/dp_parser.h  |  4 ---
>   5 files changed, 66 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5ad96989c5f2..f19cb8c7e8cb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -356,12 +356,6 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>   	int rc = 0;
>   	struct edid *edid;
>   
> -	dp->panel->max_dp_lanes = dp->parser->max_dp_lanes;
> -	dp->panel->max_dp_link_rate = dp->parser->max_dp_link_rate;
> -
> -	drm_dbg_dp(dp->drm_dev, "max_lanes=%d max_link_rate=%d\n",
> -		dp->panel->max_dp_lanes, dp->panel->max_dp_link_rate);
> -
>   	rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
>   	if (rc)
>   		goto end;
> @@ -381,8 +375,6 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>   	dp->audio_supported = drm_detect_monitor_audio(edid);
>   	dp_panel_handle_sink_request(dp->panel);
>   
> -	dp->dp_display.max_dp_lanes = dp->parser->max_dp_lanes;
> -
>   	/*
>   	 * set sink to normal operation mode -- D0
>   	 * before dpcd read
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 102f3507d824..70759dd1bfd0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -28,7 +28,6 @@ struct msm_dp {
>   
>   	bool wide_bus_en;
>   
> -	u32 max_dp_lanes;
>   	struct dp_audio *dp_audio;
>   	bool psr_supported;
>   };
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 127f6af995cd..8242541a81b9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -7,8 +7,12 @@
>   
>   #include <drm/drm_connector.h>
>   #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
>   #include <drm/drm_print.h>
>   
> +#define DP_MAX_NUM_DP_LANES	4
> +#define DP_LINK_RATE_HBR2	540000 /* kbytes */
> +
>   struct dp_panel_private {
>   	struct device *dev;
>   	struct drm_device *drm_dev;
> @@ -138,6 +142,9 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>   
>   	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>   
> +	drm_dbg_dp(panel->drm_dev, "max_lanes=%d max_link_rate=%d\n",
> +		dp_panel->max_dp_lanes, dp_panel->max_dp_link_rate);
> +
>   	rc = dp_panel_read_dpcd(dp_panel);
>   	if (rc) {
>   		DRM_ERROR("read dpcd failed %d\n", rc);
> @@ -386,10 +393,65 @@ int dp_panel_init_panel_info(struct dp_panel *dp_panel)
>   	return 0;
>   }
>   
> +static u32 dp_panel_link_frequencies(struct device_node *of_node)
> +{
> +	struct device_node *endpoint;
> +	u64 frequency = 0;
> +	int cnt;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */
> +	if (!endpoint)
> +		return 0;
> +
> +	cnt = of_property_count_u64_elems(endpoint, "link-frequencies");
> +
> +	if (cnt > 0)
> +		of_property_read_u64_index(endpoint, "link-frequencies",
> +						cnt - 1, &frequency);
> +	of_node_put(endpoint);
> +
> +	do_div(frequency,
> +		10 * /* from symbol rate to link rate */
> +		1000); /* kbytes */
> +
> +	return frequency;
> +}
> +
> +static int dp_panel_parse_dt(struct dp_panel *dp_panel)
> +{
> +	struct dp_panel_private *panel;
> +	struct device_node *of_node;
> +	int cnt;
> +
> +	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> +	of_node = panel->dev->of_node;
> +
> +	/*
> +	 * data-lanes is the property of dp_out endpoint
> +	 */
> +	cnt = drm_of_get_data_lanes_count_ep(of_node, 1, 0, 1, DP_MAX_NUM_DP_LANES);
> +	if (cnt < 0) {
> +		/* legacy code, data-lanes is the property of mdss_dp node */
> +		cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
> +	}
> +
> +	if (cnt > 0)
> +		dp_panel->max_dp_lanes = cnt;
> +	else
> +		dp_panel->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> +
> +	dp_panel->max_dp_link_rate = dp_panel_link_frequencies(of_node);
> +	if (!dp_panel->max_dp_link_rate)
> +		dp_panel->max_dp_link_rate = DP_LINK_RATE_HBR2;
> +
> +	return 0;
> +}
> +
>   struct dp_panel *dp_panel_get(struct dp_panel_in *in)
>   {
>   	struct dp_panel_private *panel;
>   	struct dp_panel *dp_panel;
> +	int ret;
>   
>   	if (!in->dev || !in->catalog || !in->aux || !in->link) {
>   		DRM_ERROR("invalid input\n");
> @@ -408,6 +470,10 @@ struct dp_panel *dp_panel_get(struct dp_panel_in *in)
>   	dp_panel = &panel->dp_panel;
>   	dp_panel->max_bw_code = DP_LINK_BW_8_1;
>   
> +	ret = dp_panel_parse_dt(dp_panel);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	return dp_panel;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 2d0dd4353cdf..aa135d5cedbd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -24,56 +24,6 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
>   	return 0;
>   }
>   
> -static u32 dp_parser_link_frequencies(struct device_node *of_node)
> -{
> -	struct device_node *endpoint;
> -	u64 frequency = 0;
> -	int cnt;
> -
> -	endpoint = of_graph_get_endpoint_by_regs(of_node, 1, 0); /* port@1 */
> -	if (!endpoint)
> -		return 0;
> -
> -	cnt = of_property_count_u64_elems(endpoint, "link-frequencies");
> -
> -	if (cnt > 0)
> -		of_property_read_u64_index(endpoint, "link-frequencies",
> -						cnt - 1, &frequency);
> -	of_node_put(endpoint);
> -
> -	do_div(frequency,
> -		10 * /* from symbol rate to link rate */
> -		1000); /* kbytes */
> -
> -	return frequency;
> -}
> -
> -static int dp_parser_misc(struct dp_parser *parser)
> -{
> -	struct device_node *of_node = parser->pdev->dev.of_node;
> -	int cnt;
> -
> -	/*
> -	 * data-lanes is the property of dp_out endpoint
> -	 */
> -	cnt = drm_of_get_data_lanes_count_ep(of_node, 1, 0, 1, DP_MAX_NUM_DP_LANES);
> -	if (cnt < 0) {
> -		/* legacy code, data-lanes is the property of mdss_dp node */
> -		cnt = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
> -	}
> -
> -	if (cnt > 0)
> -		parser->max_dp_lanes = cnt;
> -	else
> -		parser->max_dp_lanes = DP_MAX_NUM_DP_LANES; /* 4 lanes */
> -
> -	parser->max_dp_link_rate = dp_parser_link_frequencies(of_node);
> -	if (!parser->max_dp_link_rate)
> -		parser->max_dp_link_rate = DP_LINK_RATE_HBR2;
> -
> -	return 0;
> -}
> -
>   int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser)
>   {
>   	struct platform_device *pdev = parser->pdev;
> @@ -101,10 +51,6 @@ static int dp_parser_parse(struct dp_parser *parser)
>   	if (rc)
>   		return rc;
>   
> -	rc = dp_parser_misc(parser);
> -	if (rc)
> -		return rc;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 7306768547a6..21a66932e35e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -11,8 +11,6 @@
>   #include "msm_drv.h"
>   
>   #define DP_MAX_PIXEL_CLK_KHZ	675000
> -#define DP_MAX_NUM_DP_LANES	4
> -#define DP_LINK_RATE_HBR2	540000 /* kbytes */
>   
>   /**
>    * struct dp_parser - DP parser's data exposed to clients
> @@ -23,8 +21,6 @@
>   struct dp_parser {
>   	struct platform_device *pdev;
>   	struct phy *phy;
> -	u32 max_dp_lanes;
> -	u32 max_dp_link_rate;
>   	struct drm_bridge *next_bridge;
>   };
>   
>
Kuogee Hsieh Jan. 26, 2024, 10:25 p.m. UTC | #6 
On 1/26/2024 10:26 AM, Dmitry Baryshkov wrote:
> Finally drop separate "parsing" submodule. There is no need in it
> anymore. All submodules handle DT properties directly rather than
> passing them via the separate structure pointer.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/Makefile        |  1 -
>   drivers/gpu/drm/msm/dp/dp_aux.h     |  1 +
>   drivers/gpu/drm/msm/dp/dp_catalog.h |  1 -
>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  3 +-
>   drivers/gpu/drm/msm/dp/dp_debug.c   |  1 -
>   drivers/gpu/drm/msm/dp/dp_display.c | 18 +++++------
>   drivers/gpu/drm/msm/dp/dp_display.h |  2 ++
>   drivers/gpu/drm/msm/dp/dp_parser.c  | 61 -------------------------------------
>   drivers/gpu/drm/msm/dp/dp_parser.h  | 39 ------------------------
>   9 files changed, 12 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 8dbdf3fba69e..543e04fa72e3 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -127,7 +127,6 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
>   	dp/dp_drm.o \
>   	dp/dp_link.o \
>   	dp/dp_panel.o \
> -	dp/dp_parser.o \
>   	dp/dp_audio.o
>   
>   msm-$(CONFIG_DRM_FBDEV_EMULATION) += msm_fbdev.o
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
> index 16d9b1758748..f47d591c1f54 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -16,6 +16,7 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
>   void dp_aux_deinit(struct drm_dp_aux *dp_aux);
>   void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
>   
> +struct phy;
>   struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>   			      struct phy *phy,
>   			      bool is_edp);
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 989e4c4fd6fa..a724a986b6ee 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -8,7 +8,6 @@
>   
>   #include <drm/drm_modes.h>
>   
> -#include "dp_parser.h"
>   #include "disp/msm_disp_snapshot.h"
>   
>   /* interrupts */
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 6e9f375b856a..fa014cee7e21 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -9,7 +9,6 @@
>   #include "dp_aux.h"
>   #include "dp_panel.h"
>   #include "dp_link.h"
> -#include "dp_parser.h"
>   #include "dp_catalog.h"
>   
>   struct dp_ctrl {
> @@ -17,6 +16,8 @@ struct dp_ctrl {
>   	bool wide_bus_en;
>   };
>   
> +struct phy;
> +
>   int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>   int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
>   void dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
> index 6c281dc095b9..ac68554801a4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -9,7 +9,6 @@
>   #include <drm/drm_connector.h>
>   #include <drm/drm_file.h>
>   
> -#include "dp_parser.h"
>   #include "dp_catalog.h"
>   #include "dp_aux.h"
>   #include "dp_ctrl.h"
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index de1306a88748..67956e34436d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -9,12 +9,12 @@
>   #include <linux/debugfs.h>
>   #include <linux/component.h>
>   #include <linux/of_irq.h>
> +#include <linux/phy/phy.h>
>   #include <linux/delay.h>
>   #include <drm/display/drm_dp_aux_bus.h>
>   
>   #include "msm_drv.h"
>   #include "msm_kms.h"
> -#include "dp_parser.h"
>   #include "dp_ctrl.h"
>   #include "dp_catalog.h"
>   #include "dp_aux.h"
> @@ -87,7 +87,6 @@ struct dp_display_private {
>   	struct drm_device *drm_dev;
>   	struct dentry *root;
>   
> -	struct dp_parser  *parser;
>   	struct dp_catalog *catalog;
>   	struct drm_dp_aux *aux;
>   	struct dp_link    *link;
> @@ -704,14 +703,11 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   	struct dp_panel_in panel_in = {
>   		.dev = dev,
>   	};
> +	struct phy *phy;
>   
> -	dp->parser = dp_parser_get(dp->dp_display.pdev);
> -	if (IS_ERR(dp->parser)) {
> -		rc = PTR_ERR(dp->parser);
> -		DRM_ERROR("failed to initialize parser, rc = %d\n", rc);
> -		dp->parser = NULL;
> -		goto error;
> -	}
> +	phy = devm_phy_get(dev, "dp");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
>   
>   	dp->catalog = dp_catalog_get(dev);
>   	if (IS_ERR(dp->catalog)) {
> @@ -722,7 +718,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   	}
>   
>   	dp->aux = dp_aux_get(dev, dp->catalog,
> -			     dp->parser->phy,
> +			     phy,
>   			     dp->dp_display.is_edp);
>   	if (IS_ERR(dp->aux)) {
>   		rc = PTR_ERR(dp->aux);
> @@ -753,7 +749,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>   
>   	dp->ctrl = dp_ctrl_get(dev, dp->link, dp->panel, dp->aux,
>   			       dp->catalog,
> -			       dp->parser->phy);
> +			       phy);
>   	if (IS_ERR(dp->ctrl)) {
>   		rc = PTR_ERR(dp->ctrl);
>   		DRM_ERROR("failed to initialize ctrl, rc = %d\n", rc);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 70759dd1bfd0..234dada88687 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -10,6 +10,8 @@
>   #include <sound/hdmi-codec.h>
>   #include "disp/msm_disp_snapshot.h"
>   
> +#define DP_MAX_PIXEL_CLK_KHZ	675000
> +
>   struct msm_dp {
>   	struct drm_device *drm_dev;
>   	struct platform_device *pdev;
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> deleted file mode 100644
> index f95ab3c5c72c..000000000000
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
> - */
> -
> -#include <linux/of_gpio.h>
> -#include <linux/phy/phy.h>
> -
> -#include <drm/drm_of.h>
> -#include <drm/drm_print.h>
> -#include <drm/drm_bridge.h>
> -
> -#include "dp_parser.h"
> -#include "dp_reg.h"
> -
> -static int dp_parser_ctrl_res(struct dp_parser *parser)
> -{
> -	struct platform_device *pdev = parser->pdev;
> -
> -	parser->phy = devm_phy_get(&pdev->dev, "dp");
> -	if (IS_ERR(parser->phy))
> -		return PTR_ERR(parser->phy);
> -
> -	return 0;
> -}
> -
> -static int dp_parser_parse(struct dp_parser *parser)
> -{
> -	int rc = 0;
> -
> -	if (!parser) {
> -		DRM_ERROR("invalid input\n");
> -		return -EINVAL;
> -	}
> -
> -	rc = dp_parser_ctrl_res(parser);
> -	if (rc)
> -		return rc;
> -
> -	return 0;
> -}
> -
> -struct dp_parser *dp_parser_get(struct platform_device *pdev)
> -{
> -	struct dp_parser *parser;
> -	int ret;
> -
> -	parser = devm_kzalloc(&pdev->dev, sizeof(*parser), GFP_KERNEL);
> -	if (!parser)
> -		return ERR_PTR(-ENOMEM);
> -
> -	parser->pdev = pdev;
> -
> -	ret = dp_parser_parse(parser);
> -	if (ret) {
> -		dev_err(&pdev->dev, "device tree parsing failed\n");
> -		return ERR_PTR(ret);
> -	}
> -
> -	return parser;
> -}
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> deleted file mode 100644
> index 38fd335d5950..000000000000
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
> - */
> -
> -#ifndef _DP_PARSER_H_
> -#define _DP_PARSER_H_
> -
> -#include <linux/platform_device.h>
> -
> -#include "msm_drv.h"
> -
> -#define DP_MAX_PIXEL_CLK_KHZ	675000
> -
> -/**
> - * struct dp_parser - DP parser's data exposed to clients
> - *
> - * @pdev: platform data of the client
> - * @phy: PHY handle
> - */
> -struct dp_parser {
> -	struct platform_device *pdev;
> -	struct phy *phy;
> -};
> -
> -/**
> - * dp_parser_get() - get the DP's device tree parser module
> - *
> - * @pdev: platform data of the client
> - * return: pointer to dp_parser structure.
> - *
> - * This function provides client capability to parse the
> - * device tree and populate the data structures. The data
> - * related to clock, regulators, pin-control and other
> - * can be parsed using this module.
> - */
> -struct dp_parser *dp_parser_get(struct platform_device *pdev);
> -
> -#endif
>
Dmitry Baryshkov Feb. 19, 2024, 12:30 p.m. UTC | #7 
On Fri, 26 Jan 2024 20:26:19 +0200, Dmitry Baryshkov wrote:
> Reshuffle code in the DP driver, cleaning up clocks and DT parsing and
> dropping the dp_power and dp_parser submodules.
> 
> Initially I started by looking onto stream_pixel clock handling only to
> find several wrapping layers around a single clocks. After inlining
> and/or dropping them (and thus dp_power submodule), it was more or less
> natural to continue cleaning up the dp_parser until it got removed
> completely.
> 
> [...]

Applied, thanks!

[01/15] drm/msm/dp: drop unused parser definitions
        https://gitlab.freedesktop.org/lumag/msm/-/commit/08c5b691ee54
[02/15] drm/msm/dp: drop unused fields from dp_power_private
        https://gitlab.freedesktop.org/lumag/msm/-/commit/9aeb50ea0ea9
[03/15] drm/msm/dp: parse DT from dp_parser_get
        https://gitlab.freedesktop.org/lumag/msm/-/commit/31a01db14b90
[04/15] drm/msm/dp: inline dp_power_(de)init
        https://gitlab.freedesktop.org/lumag/msm/-/commit/47103b582412
[05/15] drm/msm/dp: fold dp_power into dp_ctrl module
        https://gitlab.freedesktop.org/lumag/msm/-/commit/17cb153f81df
[06/15] drm/msm/dp: simplify stream clocks handling
        https://gitlab.freedesktop.org/lumag/msm/-/commit/9bd0946d5ca1
[07/15] drm/msm/dp: stop parsing clock names from DT
        https://gitlab.freedesktop.org/lumag/msm/-/commit/77d0243a3313
[08/15] drm/msm/dp: split dp_ctrl_clk_enable into four functuions
        https://gitlab.freedesktop.org/lumag/msm/-/commit/e518c27218c6
[09/15] drm/msm/dp: move phy_configure_opts to dp_ctrl
        https://gitlab.freedesktop.org/lumag/msm/-/commit/b4745f741e79
[10/15] drm/msm/dp: remove PHY handling from dp_catalog.c
        https://gitlab.freedesktop.org/lumag/msm/-/commit/64eba0d63c70
[11/15] drm/msm/dp: handle PHY directly in dp_ctrl
        https://gitlab.freedesktop.org/lumag/msm/-/commit/f304bda5bfda
[12/15] drm/msm/dp: move all IO handling to dp_catalog
        https://gitlab.freedesktop.org/lumag/msm/-/commit/1577814118e7
[13/15] drm/msm/dp: move link property handling to dp_panel
        https://gitlab.freedesktop.org/lumag/msm/-/commit/3ffe15b30a63
[14/15] drm/msm/dp: move next_bridge handling to dp_display
        https://gitlab.freedesktop.org/lumag/msm/-/commit/b3b1d122a80b
[15/15] drm/msm/dp: drop dp_parser
        https://gitlab.freedesktop.org/lumag/msm/-/commit/6215f1558bab

Best regards,