RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP
TX Combo PHY based on a Samsung IP block, and there are also two
independent eDP display interface on RK3588 Soc.

Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without
the tests of audio, PSR and other eDP 1.3 specific features.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v2:
- Add support for the other eDP output edp1
---
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   | 82 ++++++++++++++++---
 include/drm/bridge/analogix_dp.h              |  3 +-
 2 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 871606a31ef1..4c9a55776ada 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -51,10 +51,12 @@ struct rockchip_grf_reg_field {
 /**
  * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
  * @lcdc_sel: grf register field of lcdc_sel
+ * @edp_mode: grf register field of edp_mode
  * @chip_type: specific chip type
  */
 struct rockchip_dp_chip_data {
 	const struct rockchip_grf_reg_field lcdc_sel;
+	const struct rockchip_grf_reg_field edp_mode;
 	u32	chip_type;
 };
 
@@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
 		return ret;
 	}
 
+	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1);
+	if (ret != 0)
+		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);
+
 	return ret;
 }
 
 static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data);
+	int ret;
+
+	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 0);
+	if (ret != 0)
+		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);
 
 	clk_disable_unprepare(dp->pclk);
 
@@ -203,6 +214,10 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
 	struct rockchip_dp_device *dp = encoder_to_dp(encoder);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
+	struct of_endpoint endpoint;
+	struct device_node *remote_port, *remote_port_parent;
+	char name[32];
+	u32 port_id;
 	int ret;
 
 	crtc = rockchip_dp_drm_get_new_crtc(encoder, state);
@@ -220,13 +235,27 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
 		return;
 	}
 
-	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
+	ret = drm_of_encoder_active_endpoint(dp->dev->of_node, encoder, &endpoint);
 	if (ret < 0)
 		return;
 
-	DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
+	remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node);
+	if (remote_port_parent) {
+		if (of_get_child_by_name(remote_port_parent, "ports")) {
+			remote_port = of_graph_get_remote_port(endpoint.local_node);
+			of_property_read_u32(remote_port, "reg", &port_id);
+			of_node_put(remote_port);
+			sprintf(name, "%s vp%d", remote_port_parent->full_name, port_id);
+		} else {
+			sprintf(name, "%s %s",
+				remote_port_parent->full_name, endpoint.id ? "vopl" : "vopb");
+		}
+		of_node_put(remote_port_parent);
+
+		DRM_DEV_DEBUG(dp->dev, "%s output to edp\n", name);
+	}
 
-	ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, ret);
+	ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, endpoint.id);
 	if (ret != 0)
 		DRM_DEV_ERROR(dp->dev, "Could not write to GRF: %d\n", ret);
 
@@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 	const struct rockchip_dp_chip_data *dp_data;
 	struct drm_panel *panel = NULL;
 	struct rockchip_dp_device *dp;
+	int id, i;
 	int ret;
 
 	dp_data = of_device_get_match_data(dev);
@@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 	if (!dp)
 		return -ENOMEM;
 
+	id = of_alias_get_id(dev->of_node, "edp");
+	if (id < 0)
+		id = 0;
+
+	i = 0;
+	while (is_rockchip(dp_data[i].chip_type))
+		i++;
+
+	if (id >= i) {
+		DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id);
+		return -ENODEV;
+	}
+
 	dp->dev = dev;
 	dp->adp = ERR_PTR(-ENODEV);
-	dp->data = dp_data;
+	dp->data = &dp_data[id];
 	dp->plat_data.panel = panel;
 	dp->plat_data.dev_type = dp->data->chip_type;
 	dp->plat_data.power_on = rockchip_dp_poweron;
@@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev)
 static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
 		rockchip_dp_resume, NULL);
 
-static const struct rockchip_dp_chip_data rk3399_edp = {
-	.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
-	.chip_type = RK3399_EDP,
+static const struct rockchip_dp_chip_data rk3399_edp[] = {
+	{
+		.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
+		.chip_type = RK3399_EDP,
+	},
+	{ /* sentinel */ }
+};
+
+static const struct rockchip_dp_chip_data rk3288_dp[] = {
+	{
+		.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
+		.chip_type = RK3288_DP,
+	},
+	{ /* sentinel */ }
 };
 
-static const struct rockchip_dp_chip_data rk3288_dp = {
-	.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
-	.chip_type = RK3288_DP,
+static const struct rockchip_dp_chip_data rk3588_edp[] = {
+	{
+		.edp_mode = GRF_REG_FIELD(0x0000, 0, 0),
+		.chip_type = RK3588_EDP,
+	},
+	{
+		.edp_mode = GRF_REG_FIELD(0x0004, 0, 0),
+		.chip_type = RK3588_EDP,
+	},
+	{ /* sentinel */ }
 };
 
 static const struct of_device_id rockchip_dp_dt_ids[] = {
 	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
 	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
+	{.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp },
 	{}
 };
 MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 6002c5666031..54086cb2d97d 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -15,11 +15,12 @@ enum analogix_dp_devtype {
 	EXYNOS_DP,
 	RK3288_DP,
 	RK3399_EDP,
+	RK3588_EDP,
 };
 
 static inline bool is_rockchip(enum analogix_dp_devtype type)
 {
-	return type == RK3288_DP || type == RK3399_EDP;
+	return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP;
 }
 
 struct analogix_dp_plat_data {
-- 
2.34.1

On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote:
> RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP
> TX Combo PHY based on a Samsung IP block, and there are also two
> independent eDP display interface on RK3588 Soc.
> 
> Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without
> the tests of audio, PSR and other eDP 1.3 specific features.
> 
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> - Add support for the other eDP output edp1
> ---
>  .../gpu/drm/rockchip/analogix_dp-rockchip.c   | 82 ++++++++++++++++---
>  include/drm/bridge/analogix_dp.h              |  3 +-
>  2 files changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 871606a31ef1..4c9a55776ada 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field {
>  /**
>   * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
>   * @lcdc_sel: grf register field of lcdc_sel
> + * @edp_mode: grf register field of edp_mode
>   * @chip_type: specific chip type
>   */
>  struct rockchip_dp_chip_data {
>  	const struct rockchip_grf_reg_field lcdc_sel;
> +	const struct rockchip_grf_reg_field edp_mode;
>  	u32	chip_type;
>  };
>  
> @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
>  		return ret;
>  	}
>  
> +	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1);
> +	if (ret != 0)
> +		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);

Is it to be called for non-eDP hosts too? Or for older hosts?

> +
>  	return ret;
>  }
>  
>  static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
>  {
>  	struct rockchip_dp_device *dp = pdata_encoder_to_dp(plat_data);
> +	int ret;
> +
> +	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 0);
> +	if (ret != 0)
> +		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);

Same question.

>  
>  	clk_disable_unprepare(dp->pclk);
>  
> @@ -203,6 +214,10 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
>  	struct rockchip_dp_device *dp = encoder_to_dp(encoder);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
> +	struct of_endpoint endpoint;
> +	struct device_node *remote_port, *remote_port_parent;
> +	char name[32];
> +	u32 port_id;
>  	int ret;
>  
>  	crtc = rockchip_dp_drm_get_new_crtc(encoder, state);
> @@ -220,13 +235,27 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> -	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> +	ret = drm_of_encoder_active_endpoint(dp->dev->of_node, encoder, &endpoint);
>  	if (ret < 0)
>  		return;
>  
> -	DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
> +	remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node);
> +	if (remote_port_parent) {
> +		if (of_get_child_by_name(remote_port_parent, "ports")) {
> +			remote_port = of_graph_get_remote_port(endpoint.local_node);
> +			of_property_read_u32(remote_port, "reg", &port_id);
> +			of_node_put(remote_port);
> +			sprintf(name, "%s vp%d", remote_port_parent->full_name, port_id);
> +		} else {
> +			sprintf(name, "%s %s",
> +				remote_port_parent->full_name, endpoint.id ? "vopl" : "vopb");
> +		}
> +		of_node_put(remote_port_parent);
> +
> +		DRM_DEV_DEBUG(dp->dev, "%s output to edp\n", name);
> +	}

The "output to dp" message got dropped. Is it expected?

>  
> -	ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, ret);
> +	ret = rockchip_grf_field_write(dp->grf, &dp->data->lcdc_sel, endpoint.id);
>  	if (ret != 0)
>  		DRM_DEV_ERROR(dp->dev, "Could not write to GRF: %d\n", ret);
>  
> @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  	const struct rockchip_dp_chip_data *dp_data;
>  	struct drm_panel *panel = NULL;
>  	struct rockchip_dp_device *dp;
> +	int id, i;
>  	int ret;
>  
>  	dp_data = of_device_get_match_data(dev);
> @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  	if (!dp)
>  		return -ENOMEM;
>  
> +	id = of_alias_get_id(dev->of_node, "edp");
> +	if (id < 0)
> +		id = 0;

Alias is not documented.

Please check how other platforms handle device -> ID conversion and
consider following it (yes, the best option currently known is to
hardcode bus addresses into the driver data).

> +
> +	i = 0;
> +	while (is_rockchip(dp_data[i].chip_type))
> +		i++;
> +
> +	if (id >= i) {
> +		DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id);
> +		return -ENODEV;
> +	}

Is it applicable to non-eDP case?

> +
>  	dp->dev = dev;
>  	dp->adp = ERR_PTR(-ENODEV);
> -	dp->data = dp_data;
> +	dp->data = &dp_data[id];
>  	dp->plat_data.panel = panel;
>  	dp->plat_data.dev_type = dp->data->chip_type;
>  	dp->plat_data.power_on = rockchip_dp_poweron;
> @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev)
>  static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
>  		rockchip_dp_resume, NULL);
>  
> -static const struct rockchip_dp_chip_data rk3399_edp = {
> -	.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> -	.chip_type = RK3399_EDP,
> +static const struct rockchip_dp_chip_data rk3399_edp[] = {
> +	{
> +		.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> +		.chip_type = RK3399_EDP,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static const struct rockchip_dp_chip_data rk3288_dp[] = {
> +	{
> +		.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> +		.chip_type = RK3288_DP,
> +	},
> +	{ /* sentinel */ }
>  };
>  
> -static const struct rockchip_dp_chip_data rk3288_dp = {
> -	.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> -	.chip_type = RK3288_DP,
> +static const struct rockchip_dp_chip_data rk3588_edp[] = {
> +	{
> +		.edp_mode = GRF_REG_FIELD(0x0000, 0, 0),
> +		.chip_type = RK3588_EDP,
> +	},
> +	{
> +		.edp_mode = GRF_REG_FIELD(0x0004, 0, 0),
> +		.chip_type = RK3588_EDP,
> +	},
> +	{ /* sentinel */ }
>  };
>  
>  static const struct of_device_id rockchip_dp_dt_ids[] = {
>  	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
>  	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
> +	{.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 6002c5666031..54086cb2d97d 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -15,11 +15,12 @@ enum analogix_dp_devtype {
>  	EXYNOS_DP,
>  	RK3288_DP,
>  	RK3399_EDP,
> +	RK3588_EDP,
>  };
>  
>  static inline bool is_rockchip(enum analogix_dp_devtype type)
>  {
> -	return type == RK3288_DP || type == RK3399_EDP;
> +	return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP;
>  }
>  
>  struct analogix_dp_plat_data {
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov:
> On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote:
> > RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP
> > TX Combo PHY based on a Samsung IP block, and there are also two
> > independent eDP display interface on RK3588 Soc.
> > 
> > Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without
> > the tests of audio, PSR and other eDP 1.3 specific features.
> > 
> > Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Add support for the other eDP output edp1
> > ---
> >  .../gpu/drm/rockchip/analogix_dp-rockchip.c   | 82 ++++++++++++++++---
> >  include/drm/bridge/analogix_dp.h              |  3 +-
> >  2 files changed, 74 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index 871606a31ef1..4c9a55776ada 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field {
> >  /**
> >   * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
> >   * @lcdc_sel: grf register field of lcdc_sel
> > + * @edp_mode: grf register field of edp_mode
> >   * @chip_type: specific chip type
> >   */
> >  struct rockchip_dp_chip_data {
> >  	const struct rockchip_grf_reg_field lcdc_sel;
> > +	const struct rockchip_grf_reg_field edp_mode;
> >  	u32	chip_type;
> >  };
> >  
> > @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
> >  		return ret;
> >  	}
> >  
> > +	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1);
> > +	if (ret != 0)
> > +		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);
> 
> Is it to be called for non-eDP hosts too? Or for older hosts?

The implementation of rockchip_grf_field_write (in patch1) seems to
take care of checking if that field actually exists and doing nothing if
not.

I think eDP/DP is more a naming thing, the Analogix controller is called
an eDP controller in all documentation things.

Even back on rk3288, the Analogix-DP, still is called an eDP controller in
documentation, with the only difference being that it does not contain
another additional "dedicated" DP controller

> > @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
> >  	const struct rockchip_dp_chip_data *dp_data;
> >  	struct drm_panel *panel = NULL;
> >  	struct rockchip_dp_device *dp;
> > +	int id, i;
> >  	int ret;
> >  
> >  	dp_data = of_device_get_match_data(dev);
> > @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev)
> >  	if (!dp)
> >  		return -ENOMEM;
> >  
> > +	id = of_alias_get_id(dev->of_node, "edp");
> > +	if (id < 0)
> > +		id = 0;
> 
> Alias is not documented.

additionally, aliases should never be used to determine actual device
functionionality - e.g. the hw-device-number

> Please check how other platforms handle device -> ID conversion and
> consider following it (yes, the best option currently known is to
> hardcode bus addresses into the driver data).

see for example the dw-dsi + dw-dsi2 for existing implementations.



> > +	i = 0;
> > +	while (is_rockchip(dp_data[i].chip_type))
> > +		i++;
> > +
> > +	if (id >= i) {
> > +		DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id);
> > +		return -ENODEV;
> > +	}
> 
> Is it applicable to non-eDP case?

same as above, it's always called eDP in all pieces of documentation,
(Compliant with DP 1.2 and eDP 1.3)

Heiko

> > +
> >  	dp->dev = dev;
> >  	dp->adp = ERR_PTR(-ENODEV);
> > -	dp->data = dp_data;
> > +	dp->data = &dp_data[id];
> >  	dp->plat_data.panel = panel;
> >  	dp->plat_data.dev_type = dp->data->chip_type;
> >  	dp->plat_data.power_on = rockchip_dp_poweron;
> > @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev)
> >  static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
> >  		rockchip_dp_resume, NULL);
> >  
> > -static const struct rockchip_dp_chip_data rk3399_edp = {
> > -	.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> > -	.chip_type = RK3399_EDP,
> > +static const struct rockchip_dp_chip_data rk3399_edp[] = {
> > +	{
> > +		.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> > +		.chip_type = RK3399_EDP,
> > +	},
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static const struct rockchip_dp_chip_data rk3288_dp[] = {
> > +	{
> > +		.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> > +		.chip_type = RK3288_DP,
> > +	},
> > +	{ /* sentinel */ }
> >  };
> >  
> > -static const struct rockchip_dp_chip_data rk3288_dp = {
> > -	.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> > -	.chip_type = RK3288_DP,
> > +static const struct rockchip_dp_chip_data rk3588_edp[] = {
> > +	{
> > +		.edp_mode = GRF_REG_FIELD(0x0000, 0, 0),
> > +		.chip_type = RK3588_EDP,
> > +	},
> > +	{
> > +		.edp_mode = GRF_REG_FIELD(0x0004, 0, 0),
> > +		.chip_type = RK3588_EDP,
> > +	},
> > +	{ /* sentinel */ }
> >  };
> >  
> >  static const struct of_device_id rockchip_dp_dt_ids[] = {
> >  	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
> >  	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
> > +	{.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
> > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> > index 6002c5666031..54086cb2d97d 100644
> > --- a/include/drm/bridge/analogix_dp.h
> > +++ b/include/drm/bridge/analogix_dp.h
> > @@ -15,11 +15,12 @@ enum analogix_dp_devtype {
> >  	EXYNOS_DP,
> >  	RK3288_DP,
> >  	RK3399_EDP,
> > +	RK3588_EDP,
> >  };
> >  
> >  static inline bool is_rockchip(enum analogix_dp_devtype type)
> >  {
> > -	return type == RK3288_DP || type == RK3399_EDP;
> > +	return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP;
> >  }
> >  
> >  struct analogix_dp_plat_data {
> 
>

Hi,

On 2024/12/16 17:11, Heiko Stübner wrote:
> Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov:
>> On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote:
>>> RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP
>>> TX Combo PHY based on a Samsung IP block, and there are also two
>>> independent eDP display interface on RK3588 Soc.
>>>
>>> Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without
>>> the tests of audio, PSR and other eDP 1.3 specific features.
>>>
>>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Add support for the other eDP output edp1
>>> ---
>>>   .../gpu/drm/rockchip/analogix_dp-rockchip.c   | 82 ++++++++++++++++---
>>>   include/drm/bridge/analogix_dp.h              |  3 +-
>>>   2 files changed, 74 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> index 871606a31ef1..4c9a55776ada 100644
>>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field {
>>>   /**
>>>    * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
>>>    * @lcdc_sel: grf register field of lcdc_sel
>>> + * @edp_mode: grf register field of edp_mode
>>>    * @chip_type: specific chip type
>>>    */
>>>   struct rockchip_dp_chip_data {
>>>   	const struct rockchip_grf_reg_field lcdc_sel;
>>> +	const struct rockchip_grf_reg_field edp_mode;
>>>   	u32	chip_type;
>>>   };
>>>   
>>> @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
>>>   		return ret;
>>>   	}
>>>   
>>> +	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1);
>>> +	if (ret != 0)
>>> +		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);
>>
>> Is it to be called for non-eDP hosts too? Or for older hosts?
> 
> The implementation of rockchip_grf_field_write (in patch1) seems to
> take care of checking if that field actually exists and doing nothing if
> not.
> 
> I think eDP/DP is more a naming thing, the Analogix controller is called
> an eDP controller in all documentation things.
> 
> Even back on rk3288, the Analogix-DP, still is called an eDP controller in
> documentation, with the only difference being that it does not contain
> another additional "dedicated" DP controller
> 
>>> @@ -396,6 +425,7 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>>>   	const struct rockchip_dp_chip_data *dp_data;
>>>   	struct drm_panel *panel = NULL;
>>>   	struct rockchip_dp_device *dp;
>>> +	int id, i;
>>>   	int ret;
>>>   
>>>   	dp_data = of_device_get_match_data(dev);
>>> @@ -410,9 +440,22 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>>>   	if (!dp)
>>>   		return -ENOMEM;
>>>   
>>> +	id = of_alias_get_id(dev->of_node, "edp");
>>> +	if (id < 0)
>>> +		id = 0;
>>
>> Alias is not documented.
> 
> additionally, aliases should never be used to determine actual device
> functionionality - e.g. the hw-device-number
> 
>> Please check how other platforms handle device -> ID conversion and
>> consider following it (yes, the best option currently known is to
>> hardcode bus addresses into the driver data).
> 
> see for example the dw-dsi + dw-dsi2 for existing implementations.
> 

Indeed, I will fix it in the next version.

> 
> 
>>> +	i = 0;
>>> +	while (is_rockchip(dp_data[i].chip_type))
>>> +		i++;
>>> +
>>> +	if (id >= i) {
>>> +		DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id);
>>> +		return -ENODEV;
>>> +	}
>>
>> Is it applicable to non-eDP case?
> 
> same as above, it's always called eDP in all pieces of documentation,
> (Compliant with DP 1.2 and eDP 1.3)
> 
> Heiko
> 
>>> +
>>>   	dp->dev = dev;
>>>   	dp->adp = ERR_PTR(-ENODEV);
>>> -	dp->data = dp_data;
>>> +	dp->data = &dp_data[id];
>>>   	dp->plat_data.panel = panel;
>>>   	dp->plat_data.dev_type = dp->data->chip_type;
>>>   	dp->plat_data.power_on = rockchip_dp_poweron;
>>> @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev)
>>>   static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
>>>   		rockchip_dp_resume, NULL);
>>>   
>>> -static const struct rockchip_dp_chip_data rk3399_edp = {
>>> -	.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
>>> -	.chip_type = RK3399_EDP,
>>> +static const struct rockchip_dp_chip_data rk3399_edp[] = {
>>> +	{
>>> +		.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
>>> +		.chip_type = RK3399_EDP,
>>> +	},
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static const struct rockchip_dp_chip_data rk3288_dp[] = {
>>> +	{
>>> +		.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
>>> +		.chip_type = RK3288_DP,
>>> +	},
>>> +	{ /* sentinel */ }
>>>   };
>>>   
>>> -static const struct rockchip_dp_chip_data rk3288_dp = {
>>> -	.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
>>> -	.chip_type = RK3288_DP,
>>> +static const struct rockchip_dp_chip_data rk3588_edp[] = {
>>> +	{
>>> +		.edp_mode = GRF_REG_FIELD(0x0000, 0, 0),
>>> +		.chip_type = RK3588_EDP,
>>> +	},
>>> +	{
>>> +		.edp_mode = GRF_REG_FIELD(0x0004, 0, 0),
>>> +		.chip_type = RK3588_EDP,
>>> +	},
>>> +	{ /* sentinel */ }
>>>   };
>>>   
>>>   static const struct of_device_id rockchip_dp_dt_ids[] = {
>>>   	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
>>>   	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
>>> +	{.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp },
>>>   	{}
>>>   };
>>>   MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
>>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>>> index 6002c5666031..54086cb2d97d 100644
>>> --- a/include/drm/bridge/analogix_dp.h
>>> +++ b/include/drm/bridge/analogix_dp.h
>>> @@ -15,11 +15,12 @@ enum analogix_dp_devtype {
>>>   	EXYNOS_DP,
>>>   	RK3288_DP,
>>>   	RK3399_EDP,
>>> +	RK3588_EDP,
>>>   };
>>>   
>>>   static inline bool is_rockchip(enum analogix_dp_devtype type)
>>>   {
>>> -	return type == RK3288_DP || type == RK3399_EDP;
>>> +	return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP;
>>>   }
>>>   
>>>   struct analogix_dp_plat_data {
>>
>>
> 
> 
> 
> 
> 
> 

Best regards,
Damon

On Mon, Dec 16, 2024 at 10:11:54AM +0100, Heiko Stübner wrote:
> Am Montag, 16. Dezember 2024, 09:57:41 CET schrieb Dmitry Baryshkov:
> > On Mon, Dec 16, 2024 at 11:12:17AM +0800, Damon Ding wrote:
> > > RK3588 integrates the analogix eDP 1.3 TX controller IP and the HDMI/eDP
> > > TX Combo PHY based on a Samsung IP block, and there are also two
> > > independent eDP display interface on RK3588 Soc.
> > > 
> > > Add just the basic support for now, i.e. RGB output up to 4K@60Hz, without
> > > the tests of audio, PSR and other eDP 1.3 specific features.
> > > 
> > > Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> > > 
> > > ---
> > > 
> > > Changes in v2:
> > > - Add support for the other eDP output edp1
> > > ---
> > >  .../gpu/drm/rockchip/analogix_dp-rockchip.c   | 82 ++++++++++++++++---
> > >  include/drm/bridge/analogix_dp.h              |  3 +-
> > >  2 files changed, 74 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > > index 871606a31ef1..4c9a55776ada 100644
> > > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > > @@ -51,10 +51,12 @@ struct rockchip_grf_reg_field {
> > >  /**
> > >   * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
> > >   * @lcdc_sel: grf register field of lcdc_sel
> > > + * @edp_mode: grf register field of edp_mode
> > >   * @chip_type: specific chip type
> > >   */
> > >  struct rockchip_dp_chip_data {
> > >  	const struct rockchip_grf_reg_field lcdc_sel;
> > > +	const struct rockchip_grf_reg_field edp_mode;
> > >  	u32	chip_type;
> > >  };
> > >  
> > > @@ -134,12 +136,21 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data)
> > >  		return ret;
> > >  	}
> > >  
> > > +	ret = rockchip_grf_field_write(dp->grf, &dp->data->edp_mode, 1);
> > > +	if (ret != 0)
> > > +		DRM_DEV_ERROR(dp->dev, "failed to set edp mode %d\n", ret);
> > 
> > Is it to be called for non-eDP hosts too? Or for older hosts?
> 
> The implementation of rockchip_grf_field_write (in patch1) seems to
> take care of checking if that field actually exists and doing nothing if
> not.
> 
> I think eDP/DP is more a naming thing, the Analogix controller is called
> an eDP controller in all documentation things.
> 
> Even back on rk3288, the Analogix-DP, still is called an eDP controller in
> documentation, with the only difference being that it does not contain
> another additional "dedicated" DP controller

At the very least this should be mentioned in the commit message.

> > > +	i = 0;
> > > +	while (is_rockchip(dp_data[i].chip_type))
> > > +		i++;
> > > +
> > > +	if (id >= i) {
> > > +		DRM_DEV_ERROR(dev, "invalid edp id: %d\n", id);
> > > +		return -ENODEV;
> > > +	}
> > 
> > Is it applicable to non-eDP case?
> 
> same as above, it's always called eDP in all pieces of documentation,
> (Compliant with DP 1.2 and eDP 1.3)

Ack

> 
> Heiko
> 
> > > +
> > >  	dp->dev = dev;
> > >  	dp->adp = ERR_PTR(-ENODEV);
> > > -	dp->data = dp_data;
> > > +	dp->data = &dp_data[id];
> > >  	dp->plat_data.panel = panel;
> > >  	dp->plat_data.dev_type = dp->data->chip_type;
> > >  	dp->plat_data.power_on = rockchip_dp_poweron;
> > > @@ -464,19 +507,38 @@ static int rockchip_dp_resume(struct device *dev)
> > >  static DEFINE_RUNTIME_DEV_PM_OPS(rockchip_dp_pm_ops, rockchip_dp_suspend,
> > >  		rockchip_dp_resume, NULL);
> > >  
> > > -static const struct rockchip_dp_chip_data rk3399_edp = {
> > > -	.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> > > -	.chip_type = RK3399_EDP,
> > > +static const struct rockchip_dp_chip_data rk3399_edp[] = {
> > > +	{
> > > +		.lcdc_sel = GRF_REG_FIELD(0x6250, 5, 5),
> > > +		.chip_type = RK3399_EDP,
> > > +	},
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +static const struct rockchip_dp_chip_data rk3288_dp[] = {
> > > +	{
> > > +		.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> > > +		.chip_type = RK3288_DP,
> > > +	},
> > > +	{ /* sentinel */ }
> > >  };
> > >  
> > > -static const struct rockchip_dp_chip_data rk3288_dp = {
> > > -	.lcdc_sel = GRF_REG_FIELD(0x025c, 5, 5),
> > > -	.chip_type = RK3288_DP,
> > > +static const struct rockchip_dp_chip_data rk3588_edp[] = {
> > > +	{
> > > +		.edp_mode = GRF_REG_FIELD(0x0000, 0, 0),
> > > +		.chip_type = RK3588_EDP,
> > > +	},
> > > +	{
> > > +		.edp_mode = GRF_REG_FIELD(0x0004, 0, 0),
> > > +		.chip_type = RK3588_EDP,
> > > +	},
> > > +	{ /* sentinel */ }
> > >  };
> > >  
> > >  static const struct of_device_id rockchip_dp_dt_ids[] = {
> > >  	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
> > >  	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
> > > +	{.compatible = "rockchip,rk3588-edp", .data = &rk3588_edp },
> > >  	{}
> > >  };
> > >  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
> > > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> > > index 6002c5666031..54086cb2d97d 100644
> > > --- a/include/drm/bridge/analogix_dp.h
> > > +++ b/include/drm/bridge/analogix_dp.h
> > > @@ -15,11 +15,12 @@ enum analogix_dp_devtype {
> > >  	EXYNOS_DP,
> > >  	RK3288_DP,
> > >  	RK3399_EDP,
> > > +	RK3588_EDP,
> > >  };
> > >  
> > >  static inline bool is_rockchip(enum analogix_dp_devtype type)
> > >  {
> > > -	return type == RK3288_DP || type == RK3399_EDP;
> > > +	return type == RK3288_DP || type == RK3399_EDP || type == RK3588_EDP;
> > >  }
> > >  
> > >  struct analogix_dp_plat_data {
> > 
> > 
> 
> 
> 
> 

-- 
With best wishes
Dmitry